Hi Facklam, Olivér,
> -----Original Message-----
> From: Facklam, Olivér <oliver.facklam@xxxxxxxxxxx>
> Sent: 27 November 2024 08:03
> Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type
>
> 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.
>
Can you please provide your test logs?
Previously I tested 2 devices with
Switching roles host->device->host using
echo device > /sys/class/typec/port0/data_role
and
echo host > /sys/class/typec/port0/data_role
Cheers,
Biju
