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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Biju,

Thanks for your response and sorry for the delay.

> -----Original Message-----
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 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@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.
> > > > >
> > > > > > 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); }
> > >
> > > > > >  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?

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?

> 
> 
> Cheers,
> Biju

Here are my test logs for the "port type switch" functionality.
The hd3ss3220 driver doesn't log much at all, I added one debug line
(NOT part of the patch series) for testing purposes:
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index c6922989a3cd..76fea657398a 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -199,6 +199,7 @@ static const struct typec_operations hd3ss3220_ops = {
 static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
 {
 	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
+	dev_info(hd3ss3220->dev, "Propagating role %s\n", usb_role_string(role_state));
 
 	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);

My test setup is as follows:
+-------------------------------------------+      +--------------------+
|        My device (Linux 6.12)             |      |                    |
| +-----------------+                       |      |    Remote device   |
| |                 +------I2C----+         |      |                    |
| |   i.MX8 M Plus  |    +--------v-------+ |      |   - phone          |
| |   with USB3 DRD |    |                | |      |   - USB-to-Ethernet|
| |   controller    +----+ TI HD3SS3220   +-+------+     dongle         |
| |                 |    |                | |      |   - computer       |
| +-----------------+    +----------------+ |      |                    |
+-------------------------------------------+      +--------------------+

========
Case 1: Device tree: power-role = "sink";
========
# cat /sys/class/typec/port0/port_type
[sink]
# echo "source" > /sys/class/typec/port0/port_type
zsh: permission denied: /sys/class/typec/port0/port_type
# echo "dual" > /sys/class/typec/port0/port_type
zsh: permission denied: /sys/class/typec/port0/port_type

-> plug in ethernet adapter --> nothing happens
-> unplug
-> plug in Samsung phone

[  147.581160] hd3ss3220 4-0047: Propagating role device
# cat /sys/class/typec/port0/data_role
host [device]

-> unplug

[  485.495874] hd3ss3220 4-0047: Propagating role none

========
Case 2: Device tree: power-role = "source";
========
# cat /sys/class/typec/port0/port_type
[source]
# echo "sink" > /sys/class/typec/port0/port_type
zsh: permission denied: /sys/class/typec/port0/port_type

->plug in ethernet adapter

[   94.590028] hd3ss3220 4-0047: Propagating role host
[   94.733892] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[   94.739427] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 3
[   94.747464] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x000080a001000010
[   94.756900] xhci-hcd xhci-hcd.1.auto: irq 198, io mem 0x38100000
[   94.763060] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[   94.768565] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 4
[   94.776246] xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
[   94.783382] hub 3-0:1.0: USB hub found
[   94.787178] hub 3-0:1.0: 1 port detected
[   94.791484] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[   94.800068] hub 4-0:1.0: USB hub found
[   94.803859] hub 4-0:1.0: 1 port detected
[   95.781709] usb 4-1: new SuperSpeed USB device number 2 using xhci-hcd
[   96.195802] usbcore: registered new interface driver cdc_ether
[   96.396140] cdc_ncm 4-1:2.0: MAC-Address: a0:ce:c8:9e:54:a4
[   96.401753] cdc_ncm 4-1:2.0: setting rx_max = 16384
[   96.419345] cdc_ncm 4-1:2.0: setting tx_max = 16384
[   96.434385] cdc_ncm 4-1:2.0 eth1: register 'cdc_ncm' at usb-xhci-hcd.1.auto-1, CDC NCM (NO ZLP), a0:ce:c8:9e:54:a4
[   96.444940] usbcore: registered new interface driver cdc_ncm

-> unplug

[  133.501500] hd3ss3220 4-0047: Propagating role none
[  133.506518] xhci-hcd xhci-hcd.1.auto: remove, state 1
[  133.511636] usb usb4: USB disconnect, device number 1
[  133.516732] usb 4-1: USB disconnect, device number 2
[  133.521934] cdc_ncm 4-1:2.0 eth1: unregister 'cdc_ncm' usb-xhci-hcd.1.auto-1, CDC NCM (NO ZLP)
[  133.602023] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[  133.610348] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  133.616023] usb usb3: USB disconnect, device number 1
[  133.624475] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[  133.733092] using host ethernet address: f8:dc:7a:a0:c0:ae
[  133.733104] using self ethernet address: 1A:22:33:44:55:66
[  133.739134] g_ncm gadget.0: HOST MAC f8:dc:7a:a0:c0:ae
[  133.749825] g_ncm gadget.0: MAC 1a:22:33:44:55:66
[  133.754587] g_ncm gadget.0: NCM Gadget
[  133.758354] g_ncm gadget.0: g_ncm ready

-> plug in phone --> phone is getting charged
-> unplug

==========
Case 3: Device tree: power-role = "dual";
==========
# cat /sys/class/typec/port0/port_type
[dual] source sink

-> plug in ethernet adapter

[  252.688228] hd3ss3220 4-0047: Propagating role host
[  252.823920] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  252.829468] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 3
[  252.837506] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x000080a001000010
[  252.846943] xhci-hcd xhci-hcd.1.auto: irq 198, io mem 0x38100000
[  252.853098] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  252.858614] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 4
[  252.866307] xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
[  252.873424] hub 3-0:1.0: USB hub found
[  252.877221] hub 3-0:1.0: 1 port detected
[  252.881532] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[  252.890122] hub 4-0:1.0: USB hub found
[  252.893911] hub 4-0:1.0: 1 port detected
[  253.872375] usb 4-1: new SuperSpeed USB device number 2 using xhci-hcd
[  254.282643] usbcore: registered new interface driver cdc_ether
[  254.483834] cdc_ncm 4-1:2.0: MAC-Address: a0:ce:c8:9e:54:a4
[  254.489447] cdc_ncm 4-1:2.0: setting rx_max = 16384
[  254.505900] cdc_ncm 4-1:2.0: setting tx_max = 16384
[  254.520769] cdc_ncm 4-1:2.0 eth1: register 'cdc_ncm' at usb-xhci-hcd.1.auto-1, CDC NCM (NO ZLP), a0:ce:c8:9e:54:a4
[  254.531322] usbcore: registered new interface driver cdc_ncm

-> unplug

[  273.270877] usb 4-1: USB disconnect, device number 2
[  273.276084] cdc_ncm 4-1:2.0 eth1: unregister 'cdc_ncm' usb-xhci-hcd.1.auto-1, CDC NCM (NO ZLP)
[  274.192136] hd3ss3220 4-0047: Propagating role none
[  274.197161] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  274.202251] usb usb4: USB disconnect, device number 1
[  274.207994] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[  274.213739] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  274.218841] usb usb3: USB disconnect, device number 1
[  274.225032] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[  274.335934] using host ethernet address: f8:dc:7a:a0:c0:ae
[  274.335947] using self ethernet address: 1A:22:33:44:55:66
[  274.341994] g_ncm gadget.0: HOST MAC f8:dc:7a:a0:c0:ae
[  274.352692] g_ncm gadget.0: MAC 1a:22:33:44:55:66
[  274.357455] g_ncm gadget.0: NCM Gadget
[  274.361225] g_ncm gadget.0: g_ncm ready

-> plug in Windows PC
[  322.321716] hd3ss3220 4-0047: Propagating role device
-> unplug
[  343.825254] hd3ss3220 4-0047: Propagating role none

-> plug in phone
[  454.410340] hd3ss3220 4-0047: Propagating role host
[  454.558026] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  454.563594] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 3
[  454.571652] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x000080a001000010
[  454.581104] xhci-hcd xhci-hcd.1.auto: irq 198, io mem 0x38100000
[  454.587295] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  454.592836] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 4
[  454.600546] xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
[  454.607623] hub 3-0:1.0: USB hub found
[  454.611475] hub 3-0:1.0: 1 port detected
[  454.615817] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[  454.624451] hub 4-0:1.0: USB hub found
[  454.628295] hub 4-0:1.0: 1 port detected
[  455.085270] usb 4-1: new SuperSpeed USB device number 2 using xhci-hcd
[  455.203088] cdc_acm 4-1:1.1: ttyACM0: USB ACM device
[  455.208249] usbcore: registered new interface driver cdc_acm
[  455.213947] cdc_acm: USB Abstract Control Model driver for USB modems and ISDN adapters

-> unplug
[  477.940537] usb 4-1: USB disconnect, device number 2
[  477.960408] hd3ss3220 4-0047: Propagating role none
[  477.965448] xhci-hcd xhci-hcd.1.auto: remove, state 1
[  477.970570] usb usb4: USB disconnect, device number 1
[  477.999753] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[  478.005519] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  478.010605] usb usb3: USB disconnect, device number 1
[  478.016507] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[  478.124154] using host ethernet address: f8:dc:7a:a0:c0:ae
[  478.124168] using self ethernet address: 1A:22:33:44:55:66
[  478.130222] g_ncm gadget.0: HOST MAC f8:dc:7a:a0:c0:ae
[  478.140901] g_ncm gadget.0: MAC 1a:22:33:44:55:66
[  478.145663] g_ncm gadget.0: NCM Gadget
[  478.149433] g_ncm gadget.0: g_ncm ready


# echo "source" > /sys/class/typec/port0/port_type
# cat /sys/class/typec/port0/port_type
# dual [source] sink
-> plug in ethernet dongle --> same as before
-> plug in phone --> same as before
-> plug in Windows PC
[  593.662330] hd3ss3220 4-0047: Propagating role host
[  593.794008] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  593.800172] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 3
[  593.808248] xhci-hcd xhci-hcd.1.auto: hcc params 0x0220fe6d hci version 0x110 quirks 0x000080a001000010
[  593.817695] xhci-hcd xhci-hcd.1.auto: irq 198, io mem 0x38100000
[  593.823871] xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
[  593.829390] xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 4
[  593.837075] xhci-hcd xhci-hcd.1.auto: Host supports USB 3.0 SuperSpeed
[  593.844370] hub 3-0:1.0: USB hub found
[  593.848153] hub 3-0:1.0: 1 port detected
[  593.852335] usb usb4: We don't know the algorithms for LPM for this host, disabling LPM.
[  593.860805] hub 4-0:1.0: USB hub found
[  593.864586] hub 4-0:1.0: 1 port detected

# echo "sink" > /sys/class/typec/port0/port_type
# cat /sys/class/typec/port0/port_type
dual source [sink]

-> plug in ethernet dongle --> nothing happens
-> plug in laptop / phone
[  726.770383] hd3ss3220 4-0047: Propagating role device
-> unplug
[  729.842140] hd3ss3220 4-0047: Propagating role none

As you can see, setting the port type really changes the behavior of the port
w.r.t. the initial connection setup with the peer.
I hope this helps.

Best regards
Oliver





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux