Hi Heikki,
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: Monday, November 25, 2024 11:28 AM
>
> Hi Olivér,
>
> Sorry to keep you waiting.
>
> On Mon, Nov 18, 2024 at 02:00:41PM +0000, Facklam, Olivér wrote:
> > Hello Heikki,
> >
> > Thanks for reviewing.
> >
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > Sent: Monday, November 18, 2024 12:50 PM
> > >
> > > Hi Oliver,
> > >
> > > I'm sorry, I noticed a problem with this...
> > >
> > > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> > > > The TI HD3SS3220 Type-C controller supports configuring the port
> > > > type it will operate as through the MODE_SELECT field of the
> > > > General Control Register.
> > > >
> > > > Configure the port type based on the fwnode property "power-role"
> > > > during probe, and through the port_type_set typec_operation.
> > > >
> > > > The MODE_SELECT field can only be changed when the controller is
> > > > in unattached state, so follow the sequence recommended by the
> > > > datasheet
> > > to:
> > > > 1. disable termination on CC pins to disable the controller 2.
> > > > change the mode 3. re-enable termination
> > > >
> > > > This will effectively cause a connected device to disconnect for
> > > > the duration of the mode change.
> > >
> > > Changing the type of the port is really problematic, and IMO we
> > > should actually never support that.
> >
> > Could you clarify why you think it is problematic?
>
> It's not completely clear to me what it's meant for. If it was just for fixing the
> type of the port to be sink, source or DRP before connections, it would make
> sense, but since it can be use even when there is an actice connection (there
> is nothing preventing that), it can in practice be used to swap the role.
>
> And in some cases in the past where this attribute file was proposed to be
> used with some other drivers, the actual goal really ended up being to be
> able to just swap the role with an existing connection instead of being able to
> fix the type of the port. The commit message made it sound like that could be
> the goal in this case as well, but maybe I misunderstood.
>
> Even in cases where it's clear that the intention is to just fix the role before
> connections, why would user space needs to control that is still not
> completely clear, at least not to me.
The idea is to give the user the possibility to control/restrict how the port is
operating even if they have an actual dual-role capable port.
Let me clarify. In my use case, I have a DRP port, and in most cases I would
like to use it as such.
However, there are cases where this operating mode causes additional
difficulties -- for example when connecting to another dual-role port
implementing the same role preference (e.g. 2 Try.SNK devices connected
together). Then the role outcome is random.
Since this chip doesn't support PD, there is no way to switch the role from
userspace.
When I know I'm going to be working with these types of connections, it
would be better if I can restrict the operation mode to "sink-only" (for example)
for that duration. Without needing to change my device tree.
Sure, the mechanism can be abused to switch the role on an active connection,
but that was not the primary idea here.
I would even argue that calling a port type change during an active
connection terminates that connection, and starts a new connection
from scratch with the new behavior.
>
> > > Consider for example, if your port is sink only, then the platform
> > > almost certainly can't drive the VBUS. This patch would still allow
> > > the port to be changed to source port.
> >
> > In my testing, it appeared to me that when registering a type-c port
> > with "typec_cap.type = TYPEC_PORT_SNK" (for example), then the type-c
> > class disables the port_type_store functionality:
> > if (port->cap->type != TYPEC_PORT_DRP ||
> > !port->ops || !port->ops->port_type_set) {
> > dev_dbg(dev, "changing port type not supported\n");
> > return -EOPNOTSUPP;
> > }
> >
> > So to my understanding, a platform which cannot drive VBUS should
> > simply set the fwnode `power-role="sink"`. Since patch 2/4 correctly
> > parses this property, wouldn't that solve this case?
>
> True. I stand corrected.
>
> > > Sorry for not realising this in v1.
> > >
> > > I think what you want here is just a power role swap. Currently
> > > power role swap is only supported when USB PD is supported in the
> > > class code, but since the USB Type-C specification quite clearly
> > > states that power role and data role swap can be optionally
> > > supported even when USB PD is not supported (section 2.3.3) we need to
> fix that:
> >
> > My interpretation of section 2.3.3 is that the 2 mechanisms allowing
> > power role swap are:
> > - USB PD (after initial connection)
> > - "as part of the initial connection process": to me this is simply referring to
> the
> > Try.SRC / Try.SNK mechanism, for which we already have
> > the "try_role" callback.
> >
> > Maybe I'm misunderstanding what the intentions are behind each of the
> > typec_operations, so if you could clarify that (or give some pointer),
> > that would be appreciated. My understanding:
> > - "try_role": set Try.SRC / Try.SNK / no preference for a dual-role
> > port for initial connection
> > - "pr_set" / "dr_set" / "vconn_set": swap power and data role resp.
> > after the initial connection using USB-PD.
> > - "port_type_set": configure what port type to operate as, i.e. which initial
> connection
> > state machine from the USB-C standard to apply for the next
> > connection Please correct me if any of these are incorrect.
>
> I don't know what's the intention with the port_type attribute file
> unfortunately.
>
> > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > > index 58f40156de56..ee81909565a4 100644
> > > --- a/drivers/usb/typec/class.c
> > > +++ b/drivers/usb/typec/class.c
> > > @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device
> > > *dev,
> > > return -EOPNOTSUPP;
> > > }
> > >
> > > - if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> > > - dev_dbg(dev, "partner unable to swap power role\n");
> > > - return -EIO;
> > > - }
> > > -
> > > ret = sysfs _match_string(typec_roles, buf);
> > > if (ret < 0)
> > > return ret;
> > >
> > >
> > > After that it should be possible to do power role swap also in this
> > > driver when the port is DRP capable.
> > >
> > > > Signed-off-by: Oliver Facklam <oliver.facklam@xxxxxxxxxxx>
> > > > ---
> > > > drivers/usb/typec/hd3ss3220.c | 66
> > > ++++++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 65 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/typec/hd3ss3220.c
> > > b/drivers/usb/typec/hd3ss3220.c
> > > > index
> > >
> e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a
> > > 01f311fb60b4284da 100644
> > > > --- a/drivers/usb/typec/hd3ss3220.c
> > > > +++ b/drivers/usb/typec/hd3ss3220.c
> > [...]
> > > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port
> > > *port, enum typec_data_role role)
> > > > return ret;
> > > > }
> > > >
> > > > +static int hd3ss3220_port_type_set(struct typec_port *port, enum
> > > typec_port_type type)
> > > > +{
> > > > + struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> > > > +
> > > > + return hd3ss3220_set_port_type(hd3ss3220, type); }
> > >
> > > This wrapper seems completely useless. You only need one function
> > > here for the callback.
> >
> > The wrapper is to extract the struct hd3ss3220 from the typec_port.
> > The underlying hd3ss3220_set_port_type I am also using during probe to
> > configure initial port type.
>
> Ah, I missed that. Sorry about that.
>
> > One point worth mentioning here is that if the MODE_SELECT register is
> > not configured, the chip will operate according to a default which is
> > chosen by an external pin (sorry if this was not detailed enough in
> > commit msg)
> > >From the datasheet:
> > -------------------
> > | PORT | 4 | I | Tri-level input pin to indicate port mode. The state
> > | of this pin is sampled when HD3SS3220's
> > ENn_CC is asserted low, and VDD5 is active. This pin is also
> sampled following a
> > I2C_SOFT_RESET.
> > H - DFP (Pull-up to VDD5 if DFP mode is desired)
> > NC - DRP (Leave unconnected if DRP mode is desired)
> > L - UFP (Pull-down or tie to GND if UFP mode is desired)
> >
> > In our use case, it was not desirable to leave this default based on
> > wiring, and it makes more sense to me to allow the configuration to
> > come from the fwnode property. Hence the port type setting in probe().
>
> I get that, but that just means you want to fix the type during probe, no?
> Why do you need to expose this to the user space?
I've been thinking a bit more about this "fixing the type during probe" feature.
My current implementation always fixes the type, even if no device tree entry
for "power-role" was found. Could that cause issues for people relying on the
configuration through the PORT pin?
I could consider a solution where if the property is absent, the type is not
reconfigured during the probe. Although then I would have to do manual parsing
of that DT property. With typec_get_fw_cap() from patch 2/4, I loose
the information about individual properties being present/absent.
Would be interested in hearing your thoughts.
>
> > > > static const struct typec_operations hd3ss3220_ops = {
> > > > - .dr_set = hd3ss3220_dr_set
> > > > + .dr_set = hd3ss3220_dr_set,
> > > > + .port_type_set = hd3ss3220_port_type_set,
> > > > };
> > >
> > > So here I think you should implement the pr_set callback instead.
> >
> > I can do that, but based on the MODE_SELECT register description, it
> > seems to me that this setting is fundamentally changing the operation
> > mode of the chip, i.e. the state machine that is being run for initial
> connection.
> > So there would have to be a way of "resetting" it to be a dual-role
> > port again, which the "pr_set" callback doesn't seem to have?
> > This register can be written to set the HD3SS3220 mode
> > operation. The ADDR pin must be set to I2C mode. If the default
> > is maintained, HD3SS3220 shall operate according to the PORT
> > pin levels and modes. The MODE_SELECT can only be
> > changed when in the unattached state.
> > 00 - DRP mode (start from unattached.SNK) (default)
> > 01 - UFP mode (unattached.SNK)
> > 10 - DFP mode (unattached.SRC)
> > 11 - DRP mode (start from unattached.SNK)
>
> Okay, I see. This is not a case for pr_set.
>
> I'm still confused about the use case here. It seems you are not interested in
> role swapping after all, so why would you need this functionality to be
> exposed to the user space?
>
> I'm sorry if I've missed something.
>
> About the port_type attribute file itself. I would feel more comfortable with it
> if it was allowed to be written only when there is nothing connected to the
> port. At the very least, I think it should be documented better so what it's
> really meant for would be more clear to everybody.
After researching some more about this operation, I came across the driver
for TUSB320 [1] which seems to have a very similar behavior (also from TI).
[1] - https://lore.kernel.org/all/20220730180500.152004-1-marex@xxxxxxx/T/#ma7a322bc207414e4185c29d257ff30f5769f5d70
For one variant of the chip, the implementation relies on the CC disabling like
in this patch. A different variant tests the current connection status before
proceeding.
>
> thanks,
>
> --
> heikki
Thanks,
Oliver
