Hello Heikki,
Thanks for reviewing.
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: Monday, November 18, 2024 12:50 PM
> To: Facklam, Olivér <oliver.facklam@xxxxxxxxxxx>
> Cc: Biju Das <biju.das@xxxxxxxxxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; von Heyl, Benedict
> <benedict.vonheyl@xxxxxxxxxxx>; Först, Mathis
> <mathis.foerst@xxxxxxxxxxx>; Glettig, Michael
> <Michael.Glettig@xxxxxxxxxxx>
> Subject: Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port
> type
>
> 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?
>
> 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?
>
> 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.
>
> 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.
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)
