linux-kernel - RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <TY3PR01MB1134679FB8FFB248F8FDF214586352@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Mon, 2 Dec 2024 14:39:39 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Facklam, Olivér <oliver.facklam@...hlke.com>
CC: Biju Das <biju.das@...renesas.com>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, "linux-usb@...r.kernel.org"
	<linux-usb@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "von Heyl, Benedict"
	<benedict.vonheyl@...hlke.com>, Först, Mathis
	<mathis.foerst@...hlke.com>, "Glettig, Michael"
	<Michael.Glettig@...hlke.com>, Heikki Krogerus
	<heikki.krogerus@...ux.intel.com>
Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port
 type

Hi Facklam, Olivér,

> -----Original Message-----
> From: Facklam, Olivér <oliver.facklam@...hlke.com>
> Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type
> 
> Hello Biju,
> 
> Thanks for your response and sorry for the delay.
> 
> > -----Original Message-----
> > From: Biju Das <biju.das.jz@...renesas.com>
> > Sent: Wednesday, November 27, 2024 9:48 AM
> > Subject: RE: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring
> > port type
> >
> > Hi Facklam, Olivér,
> >
> > > -----Original Message-----
> > > From: Facklam, Olivér <oliver.facklam@...hlke.com>
> > > 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@...ux.intel.com>
> > > > 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@...ux.intel.com>
> > > > > > 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.
> > > > > >
> > > > > > > Signed-off-by: Oliver Facklam <oliver.facklam@...hlke.com>
> > > > > > > ---
> > > > > > >  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); }
> > > >
> > > > > > >  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@...x.de/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?
> 
> Adding them below.
> 
> >
> > 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
> 
> Could you clarify what your test setup was?
> Did you control both sides of the USB connection and execute these commands on both sides?


Yes, Two Renesas RZ/G3E boards connected each other by usb type-c cable and execute these commands on both sides.

1)Host->device->Host  (First board)
2)Device->Host->device (Second board)


Cheers,
Biju