| Message ID | 20241114-usb-typec-controller-enhancements-v2-3-362376856aea@zuehlke.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: hd3ss3220: enhance driver with port type, power opmode, and role preference settings | expand |
Hello, > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > 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@linux.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@linux.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. > > > > > > > > > > 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@zuehlke.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); } > > > > > > > > > > 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
Hello Biju, Thanks for your response and sorry for the delay. > -----Original Message----- > From: Biju Das <biju.das.jz@bp.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@zuehlke.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@linux.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@linux.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@zuehlke.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@denx.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? > > > 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
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c index e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a01f311fb60b4284da 100644 --- a/drivers/usb/typec/hd3ss3220.c +++ b/drivers/usb/typec/hd3ss3220.c @@ -35,10 +35,16 @@ #define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS BIT(4) /* Register HD3SS3220_REG_GEN_CTRL*/ +#define HD3SS3220_REG_GEN_CTRL_DISABLE_TERM BIT(0) #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK (BIT(2) | BIT(1)) #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT 0x00 #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK BIT(1) #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC (BIT(2) | BIT(1)) +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK (BIT(5) | BIT(4)) +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DEFAULT 0x00 +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP BIT(5) +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP BIT(4) +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP (BIT(5) | BIT(4)) struct hd3ss3220 { struct device *dev; @@ -75,6 +81,52 @@ static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opm current_mode); } +static int hd3ss3220_set_port_type(struct hd3ss3220 *hd3ss3220, int type) +{ + int mode_select, err; + + switch (type) { + case TYPEC_PORT_SRC: + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP; + break; + case TYPEC_PORT_SNK: + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP; + break; + case TYPEC_PORT_DRP: + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP; + break; + default: + dev_err(hd3ss3220->dev, "bad port type: %d\n", type); + return -EINVAL; + } + + /* Disable termination before changing MODE_SELECT as required by datasheet */ + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM); + if (err < 0) { + dev_err(hd3ss3220->dev, "Failed to disable port for mode change: %d\n", err); + return err; + } + + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, + HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK, + mode_select); + if (err < 0) { + dev_err(hd3ss3220->dev, "Failed to change mode: %d\n", err); + regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0); + return err; + } + + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0); + if (err < 0) + dev_err(hd3ss3220->dev, "Failed to re-enable port after mode change: %d\n", err); + + return err; +} + static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref) { return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, @@ -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, }; static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220) @@ -273,6 +333,10 @@ static int hd3ss3220_probe(struct i2c_client *client) if (ret != 0 && ret != -EINVAL && ret != -ENXIO) goto err_put_role; + ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type); + if (ret < 0) + goto err_put_role; + hd3ss3220->port = typec_register_port(&client->dev, &typec_cap); if (IS_ERR(hd3ss3220->port)) { ret = PTR_ERR(hd3ss3220->port);
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@zuehlke.com> --- drivers/usb/typec/hd3ss3220.c | 66 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-)