Hello,
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: Thursday, November 28, 2024 2:15 PM
> Subject: Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port
> type
>
> Hi,
>
> On Wed, Nov 27, 2024 at 08:02:55AM +0000, Facklam, Olivér wrote:
> > 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.
>
> Okay, thanks for the explanation.
>
> > > > > 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.
>
> I don't think it's a problem to check does the property exist directly in the
> driver. It sounds like that's what should be done in this case.
Thanks for the input.
I'll shortly send a v3 where I drop patch 2/4, and instead have the parsing
of the relevant property in patch 3/4 and 4/4 respectively.
>
> Br,
>
> --
> heikki
Thanks
Oliver
