| Message ID | 20191002231617.3670-3-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | dwc3 role-switch handling for HiKey960 | expand |
Hi John, On 03-10-2019 01:16, John Stultz wrote: > From: Yu Chen <chenyu56@huawei.com> > > This patch adds notifier for drivers want to be informed of the usb role > switch. I do not see any patches in this series actually using this new notifier. Maybe it is best to drop this patch until we actually have in-kernel users of this new API show up ? Regards, Hans > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > Cc: Yu Chen <chenyu56@huawei.com> > Cc: Felipe Balbi <balbi@kernel.org> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Jun Li <lijun.kernel@gmail.com> > Cc: Valentin Schneider <valentin.schneider@arm.com> > Cc: linux-usb@vger.kernel.org > Cc: devicetree@vger.kernel.org > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Yu Chen <chenyu56@huawei.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > drivers/usb/roles/class.c | 35 ++++++++++++++++++++++++++++++++++- > include/linux/usb/role.h | 16 ++++++++++++++++ > 2 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c > index 94b4e7db2b94..418e762d5d72 100644 > --- a/drivers/usb/roles/class.c > +++ b/drivers/usb/roles/class.c > @@ -20,6 +20,7 @@ struct usb_role_switch { > struct device dev; > struct mutex lock; /* device lock*/ > enum usb_role role; > + struct blocking_notifier_head nh; > > /* From descriptor */ > struct device *usb2_port; > @@ -49,8 +50,10 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) > mutex_lock(&sw->lock); > > ret = sw->set(sw->dev.parent, role); > - if (!ret) > + if (!ret) { > sw->role = role; > + blocking_notifier_call_chain(&sw->nh, role, NULL); > + } > > mutex_unlock(&sw->lock); > > @@ -58,6 +61,35 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) > } > EXPORT_SYMBOL_GPL(usb_role_switch_set_role); > > +int usb_role_switch_register_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + int ret = blocking_notifier_chain_register(&sw->nh, nb); > + enum usb_role role; > + > + if (ret) > + return ret; > + > + /* Initialize the notifier that was just registered */ > + mutex_lock(&sw->lock); > + if (sw->get) > + role = sw->get(sw->dev.parent); > + else > + role = sw->role; > + blocking_notifier_call_chain(&sw->nh, role, NULL); > + mutex_unlock(&sw->lock); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier); > + > +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + return blocking_notifier_chain_unregister(&sw->nh, nb); > +} > +EXPORT_SYMBOL_GPL(usb_role_switch_unregister_notifier); > + > /** > * usb_role_switch_get_role - Get the USB role for a switch > * @sw: USB role switch > @@ -296,6 +328,7 @@ usb_role_switch_register(struct device *parent, > return ERR_PTR(-ENOMEM); > > mutex_init(&sw->lock); > + BLOCKING_INIT_NOTIFIER_HEAD(&sw->nh); > > sw->allow_userspace_control = desc->allow_userspace_control; > sw->usb2_port = desc->usb2_port; > diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h > index 2d77f97df72d..8dbf7940b7da 100644 > --- a/include/linux/usb/role.h > +++ b/include/linux/usb/role.h > @@ -54,6 +54,10 @@ struct usb_role_switch * > usb_role_switch_register(struct device *parent, > const struct usb_role_switch_desc *desc); > void usb_role_switch_unregister(struct usb_role_switch *sw); > +int usb_role_switch_register_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb); > +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb); > #else > static inline int usb_role_switch_set_role(struct usb_role_switch *sw, > enum usb_role role) > @@ -87,6 +91,18 @@ usb_role_switch_register(struct device *parent, > } > > static inline void usb_role_switch_unregister(struct usb_role_switch *sw) { } > + > +static int usb_role_switch_register_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + return -ENODEV; > +} > + > +static int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + return -ENODEV; > +} > #endif > > #endif /* __LINUX_USB_ROLE_H */ >
On Thu, Oct 3, 2019 at 4:26 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Oct 02, 2019 at 11:16:16PM +0000, John Stultz wrote: > > From: Yu Chen <chenyu56@huawei.com> > > > > This patch adds notifier for drivers want to be informed of the usb role > > switch. > > Ick, I hate notifiers, they always come back to cause problems. > > What's just wrong with a "real" call to who ever needs to know this? > And who does need to know this anyway? Like Hans said, if we don't have > a user for it, we should not add it. So in this case, its used for interactions between the dwc3 driver and the hikey960 integrated USB hub, which is controlled via gpio (which I didn't submit here as I was trying to keep things short and reviewable, but likely misjudged). The HiKey960 has only one USB controller, but in order to support both USB-C gadget/OTG and USB-A (host only) ports. When the USB-C connection is attached, it powers down and disconnects the hub. When the USB-C connection is detached, it powers the hub on and connects the controller to the hub. This is pretty HiKey960 specific, so I think the notifier is useful to let the gpio hub logic tie into the role switching events. Suggestions for alternative approaches? thanks -john
Hi, On 03-10-2019 22:45, John Stultz wrote: > On Thu, Oct 3, 2019 at 4:26 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> >> On Wed, Oct 02, 2019 at 11:16:16PM +0000, John Stultz wrote: >>> From: Yu Chen <chenyu56@huawei.com> >>> >>> This patch adds notifier for drivers want to be informed of the usb role >>> switch. >> >> Ick, I hate notifiers, they always come back to cause problems. >> >> What's just wrong with a "real" call to who ever needs to know this? >> And who does need to know this anyway? Like Hans said, if we don't have >> a user for it, we should not add it. > > So in this case, its used for interactions between the dwc3 driver and > the hikey960 integrated USB hub, which is controlled via gpio (which I > didn't submit here as I was trying to keep things short and > reviewable, but likely misjudged). > > The HiKey960 has only one USB controller, but in order to support both > USB-C gadget/OTG and USB-A (host only) ports. When the USB-C > connection is attached, it powers down and disconnects the hub. When > the USB-C connection is detached, it powers the hub on and connects > the controller to the hub. When you say one controller, do you mean 1 host and 1 gadget controller, or is this one of these lovely devices where a gadget controller gets abused as / confused with a proper host controller? And since you are doing a usb-role-switch driver, I guess that the role-switch is integrated inside the SoC, so you only get one pair of USB datalines to the outside ? This does seem rather special, it might help if you can provide a diagram with both the relevant bits inside the SoC as well as what lives outside the Soc. even if it is in ASCII art... Regards, Hans
On Thu, Oct 03, 2019 at 10:56:24PM +0200, Hans de Goede wrote: > Hi, > > On 03-10-2019 22:45, John Stultz wrote: > > On Thu, Oct 3, 2019 at 4:26 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, Oct 02, 2019 at 11:16:16PM +0000, John Stultz wrote: > > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > > > This patch adds notifier for drivers want to be informed of the usb role > > > > switch. > > > > > > Ick, I hate notifiers, they always come back to cause problems. > > > > > > What's just wrong with a "real" call to who ever needs to know this? > > > And who does need to know this anyway? Like Hans said, if we don't have > > > a user for it, we should not add it. > > > > So in this case, its used for interactions between the dwc3 driver and > > the hikey960 integrated USB hub, which is controlled via gpio (which I > > didn't submit here as I was trying to keep things short and > > reviewable, but likely misjudged). > > > > The HiKey960 has only one USB controller, but in order to support both > > USB-C gadget/OTG and USB-A (host only) ports. When the USB-C > > connection is attached, it powers down and disconnects the hub. When > > the USB-C connection is detached, it powers the hub on and connects > > the controller to the hub. > > When you say one controller, do you mean 1 host and 1 gadget controller, > or is this one of these lovely devices where a gadget controller gets > abused as / confused with a proper host controller? > > And since you are doing a usb-role-switch driver, I guess that the > role-switch is integrated inside the SoC, so you only get one pair > of USB datalines to the outside ? Unless I'm mistaken, the dwc3 driver in this case is the usb-role-switch. The DWC3 IP includes both USB dost and device blocks, i.e. it's a dual role controller. Drivers like tcpm.c that negotiate the actual role need to tell the outcome of the negotiation to the dwc3 driver. So I think this part is OK. The platform has also some kind of discrete switch for routing the signals to either Standard-A (the hub) or Type-C connector, so it does not represent the usb-role-switch. It should however affect the USB role, as if that switch routes the data signals to the Standard-A port (to the hub) instead of USB Type-C, the USB role needs to be fixed to host mode. I guess this series does not include the driver for that discrete switch/mux. I don't remember/know how that switch was planned to be handled. > This does seem rather special, it might help if you can provide a diagram > with both the relevant bits inside the SoC as well as what lives outside > the Soc. even if it is in ASCII art... thanks, -- heikki
Hi, On 10/3/19 11:33 PM, John Stultz wrote: > On Thu, Oct 3, 2019 at 1:56 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 03-10-2019 22:45, John Stultz wrote: >>> The HiKey960 has only one USB controller, but in order to support both >>> USB-C gadget/OTG and USB-A (host only) ports. When the USB-C >>> connection is attached, it powers down and disconnects the hub. When >>> the USB-C connection is detached, it powers the hub on and connects >>> the controller to the hub. >> >> When you say one controller, do you mean 1 host and 1 gadget controller, >> or is this one of these lovely devices where a gadget controller gets >> abused as / confused with a proper host controller? > > I'm not totally sure myself, but I believe it's the latter, as the > host ports have to be disabled in order for the gadet/otg port to > function. > > There was a similar situation w/ the original HiKey board (dwc2 > controller) as well, though the switching was done fully in hardware > and we only needed some minor tweaks to the driver to keep the state > transitions straight. > >> And since you are doing a usb-role-switch driver, I guess that the >> role-switch is integrated inside the SoC, so you only get one pair >> of USB datalines to the outside ? > > I believe so, but again, I don't have a ton of knowledge about the SoC > details, Chen Yu would probably be the right person to answer, but I > don't know if he's doing upstreaming anymore. > >> This does seem rather special, it might help if you can provide a diagram >> with both the relevant bits inside the SoC as well as what lives outside >> the Soc. even if it is in ASCII art... > > There is a schematic pdf here: > https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf > > The larger block diagram on page 3 might be helpful, but you can find > more details on the usb hub bits on page 17 and 18. Ok, so I took a quick look at the schematic and it is really funky. The USB3 superspeed data pairs are only going to the USB-3 hub and only the USB-2 lines are muxed between the TypeC and the HUB, so in theory superspeed devices could keep working while the TypeC is in device mode, since their data lines will still be connected, but I guess the controller in the SoC is switched to device mode then so this does not work. Likewise Vbus is an all or nothing thing, either both the TypeC connector + the 2 Type-A reeptacles get Vusb or none of them get Vusb. Also it is seems to use the TypeC connector in host-mode together with the A receptacles. I must say this is a weird design... Anyways back the code to add a usb role switch notifier. I do not think that this is a good idea, this is making "core" changes to deal with a special case. If you are going to use a notfier for this then IMHO the notifier should be part of the hikey960 usb role swtich driver and not be in the usb-role-switch class code, since this is very much a device specific hack. Regards, Hans
On Fri, Oct 18, 2019 at 1:21 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 18-10-2019 22:12, John Stultz wrote: > > On Fri, Oct 18, 2019 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> On 18-10-2019 21:53, John Stultz wrote: > >>> On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > >>>> Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that > >>>> there is a data struct with vendor specific callbacks and that the > >>>> drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that. > >>>> > >>>> So you may want something similar here. But things are tricky here, > >>>> because when nothing is connected you want to provide Vbus for > >>>> the USB-A ports, which means that if someone then connects a > >>>> USB-A to C cable to connect the board to a PC (switching the port > >>>> to device mode) there will be a time when both sides are supplying > >>>> 5V if I remember the schedule correctly. > >>> > >>> Ok. Thanks for the pointer, I'll take a look at that to see if I can > >>> get it to work. > >>> > >>>> I think that the original hack might not be that bad, the whole hw > >>>> design seems so, erm, broken, that you probably cannot do proper > >>>> roleswapping anyways. So just tying Vbus to host mode might be > >>>> fine, the question then becomes again how can some other piece > >>>> of code listen to the role-switch events... > >>> > >>> So, at least in the current approach (see the v3 series), I've > >>> basically set the hub driver as an role-switch intermediary, sitting > >>> between the calls from the tcpm to the dwc3 driver. It actually works > >>> better then the earlier notifier method (which had some issues with > >>> reliably establishing the initial state on boot). Does that approach > >>> work for you? > >> > >> That sounds like it might be a nice solution. But I have not seen the > >> code, I think I was not Cc-ed on v3. Do you have a patchwork or > >> lore.kernel.org link for me? > > > > Oh! I think I had you on CC, maybe it got caught in your spam folder? > > More likely I just deleted mail to aggressively, sorry. > > > My apologies either way! The thread is here: > > https://lore.kernel.org/lkml/20191016033340.1288-1-john.stultz@linaro.org/ > > > > And the hub/role-switch-intermediary driver is here: > > https://lore.kernel.org/lkml/20191016033340.1288-12-john.stultz@linaro.org/ > > Hm, this looks very nice actually, much much better then the notifier stuff! > > As for your: > > "NOTE: It was noted that controlling the TYPEC_VBUS_POWER_OFF and > TYPEC_VBUS_POWER_ON values here is not reccomended. I'm looking > for a way to remove that bit from the logic here, but wanted to > still get feedback on this approach." > > Comment in the commit message, normally a type-c port would turn external Vbus > off until a sink is connected, IIRC the same Vbus is also used for the TupeA ports > on the hub, so that would mean those are unusable when nothing is connected to > the TypeC port, which is not what you want. Uh, so I think for the HiKey960, the type-A ports on the hub are separately powered via the hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_OFF/ON) call. At least, with the current driver, the functionality is working as expected: remove the USB-C cable, and devices connected to the hub power on, plug something into the USB-C port and devices plugged into the hub shutdown. But maybe I'm missing what you mean? > So I think that given the special case / hack-ish hw you have, that just setting > Vbus based on the role is ok(ish). Ok. I'm happy to stick with what works here, since it is at least the oddness is isolated to the device specific hub driver. thanks -john
On Fri, Oct 18, 2019 at 2:05 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 18-10-2019 22:37, John Stultz wrote: > > At least, with the current driver, the functionality is working as > > expected: remove the USB-C cable, and devices connected to the hub > > power on, plug something into the USB-C port and devices plugged into > > the hub shutdown. > > > > But maybe I'm missing what you mean? > > Ok, so double checking the schematic I do see separate Vbus-es for the > TypeC port and the TypeA ports, with the TypeC port one being controlled > by GPIO_202_VBUS_TYPEC. So ideally that gpio would be controlled to > enable/disable vbus by the tcpm framework. So I've given this a shot, adding a gpio regulator for the type-c vbus, and added a set_vbus hook to the tcpci_rt1711 with logic to enable and disable the regulator depending on the source state. I've also added some debug logic to check the regulator disabling/enabling is working properly. However, doing the type-c vbus control via the tcpm logic doesn't seem to be working properly. The issue seems to be when the USB-C cable is unplugged the device goes into ROLE_NONE, we switch to the on-board hub. Then when we connect a USB-C hub to the type-c port, we switch to ROLE_HOST, and power on the regulator, and that starts to power on the USB-C hub devices. However, since this disconnects/powers down the on-board hub, we see the on-board hub device disconnect. I'm guessing the hub disconnection causes some confusion in the state machine, as then I see the state change from state change SRC_ATTACHED -> SRC_UNATTACHED, and set_vbus is immediately called with source=0 and the regulator is disabled, and we switch back to ROLE_NONE (which powers on the onboard hub). The system then seems to quickly oscillate between the ROLE_HOST and ROLE_NONE switching the regulator on and off fairly quickly (see log below for more details) and never really settling for one state or the other. Any off-hand thoughts on what might be going wrong here? I'm fine to continue digging and working on this approach, but I also don't want to have to pollute the core code too much for this oddball hardware (esp since doing the vbus control in the role-switch intermediary does work ok - or at least better then this approach so far). thanks -john Starts in ROLE_NONE with nothing connected to type-c port, with the on-board hub powered on, then we connect a type-c usb hub. [ 57.828323] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 58.031325] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms] [ 58.031525] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY [ 58.135273] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms] [ 58.135296] JDB: tcpm_state_machine_work state chage: SNK_TRY_WAIT->SRC_TRYWAIT [ 58.149344] JDB: tcpm_state_machine_work state chage: SRC_TRYWAIT->SRC_TRYWAIT [ 58.251273] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED [delayed 100 ms] [ 58.251297] JDB: tcpm_state_machine_work state chage: SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED [ 58.269076] JDB: tcpm_state_machine_work state chage: SNK_UNATTACHED->TOGGLING [ 58.276789] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING [ 58.323506] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 58.527310] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED [delayed 200 ms] [ 58.527788] JDB: hub_usb_role_switch_set switching to ROLE_HOST! [ 58.541555] usb usb1-port1: disabled by hub (EMI?), re-enabling... [ 58.548654] usb 1-1: USB disconnect, device number 2 [ 58.554077] usb 1-1.5: USB disconnect, device number 3 [ 58.560133] JDB: rt1711h_set_vbus source: 1 sink: 0 [ 58.565377] JDB: rt1711h_set_vbus enabling regulator! [ 58.570495] type-c-vbus-current-regulator: being enabled! JDB! [ 58.586202] type-c-vbus-current-regulator: enabled successfully?! JDB! [ 58.602350] rt1711h_set_vbus: vbus := On [ 58.602354] rt1711h_set_vbus: charge is already Off [ 58.747321] usb 2-1: USB disconnect, device number 2 [ 58.819706] JDB: tcpm_state_machine_work state chage: SRC_ATTACHED->SRC_ATTACHED [ 58.871270] usb 1-1: new high-speed USB device number 4 using xhci-hcd [ 59.030402] usb 1-1: New USB device found, idVendor=2109, idProduct=2813, bcdDevice=90.11 [ 59.038677] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [ 59.045881] usb 1-1: Product: USB2.0 Hub [ 59.049838] usb 1-1: Manufacturer: VIA Labs, Inc. [ 59.104926] hub 1-1:1.0: USB hub found [ 59.109112] hub 1-1:1.0: 4 ports detected [ 59.327259] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms] [ 59.327710] JDB: rt1711h_set_vbus source: 0 sink: 0 [ 59.340022] JDB: rt1711h_set_vbus disabling regulator! [ 59.345296] type-c-vbus-current-regulator: disabled successfully?! JDB! (ret=0) [ 59.353458] rt1711h_set_vbus: vbus := Off [ 59.353465] rt1711h_set_vbus: charge is already Off [ 59.483278] usb 1-1.1: new low-speed USB device number 5 using xhci-hcd [ 59.571494] JDB: hub_usb_role_switch_set switching to ROLE_NONE! [ 59.577810] usb 1-1: USB disconnect, device number 4 [ 59.586675] JDB: tcpm_state_machine_work state chage: SRC_UNATTACHED->TOGGLING [ 59.593896] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING [ 59.600757] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 59.627362] usb 1-1.1: Device not responding to setup address. [ 59.661413] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 59.839438] usb 1-1.1: Device not responding to setup address. [ 59.863252] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms] [ 59.863428] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY [ 59.967359] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms] [ 59.967383] JDB: tcpm_state_machine_work state chage: SNK_TRY_WAIT->SRC_TRYWAIT [ 59.981452] JDB: tcpm_state_machine_work state chage: SRC_TRYWAIT->SRC_TRYWAIT [ 60.051272] usb 1-1.1: device not accepting address 5, error -71 [ 60.083337] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED [delayed 100 ms] [ 60.083365] JDB: tcpm_state_machine_work state chage: SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED [ 60.101151] JDB: tcpm_state_machine_work state chage: SNK_UNATTACHED->TOGGLING [ 60.108462] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING [ 60.155642] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 60.183338] usb 2-1: new SuperSpeed Gen 1 USB device number 3 using xhci-hcd [ 60.207782] usb 1-1-port1: attempt power cycle [ 60.212603] usb 2-1: New USB device found, idVendor=0424, idProduct=5734, bcdDevice= 2.02 [ 60.220923] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0 [ 60.228147] usb 2-1: Product: USB5734 [ 60.231883] usb 2-1: Manufacturer: Microchip Tech [ 60.256450] hub 2-1:1.0: USB hub found [ 60.260360] hub 2-1:1.0: 5 ports detected [ 60.359385] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED [delayed 200 ms] [ 60.359853] JDB: hub_usb_role_switch_set switching to ROLE_HOST! [ 60.374310] JDB: rt1711h_set_vbus source: 1 sink: 0 [ 60.379386] JDB: rt1711h_set_vbus enabling regulator! [ 60.384485] type-c-vbus-current-regulator: being enabled! JDB! [ 60.390552] hub 2-1:1.0: hub_ext_port_status failed (err = -71) [ 60.396544] type-c-vbus-current-regulator: enabled successfully?! JDB! [ 60.403538] usb 2-1: Failed to suspend device, error -71 [ 60.403694] usb 2-1: USB disconnect, device number 3 [ 60.413841] rt1711h_set_vbus: vbus := On [ 60.413844] rt1711h_set_vbus: charge is already Off [ 60.631357] JDB: tcpm_state_machine_work state chage: SRC_ATTACHED->SRC_ATTACHED [ 60.815285] usb 1-1: new high-speed USB device number 9 using xhci-hcd [ 60.969662] usb 1-1: New USB device found, idVendor=2109, idProduct=2813, bcdDevice=90.11 [ 60.977964] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [ 60.985156] usb 1-1: Product: USB2.0 Hub [ 60.989419] usb 1-1: Manufacturer: VIA Labs, Inc. [ 61.056894] hub 1-1:1.0: USB hub found [ 61.061194] hub 1-1:1.0: 4 ports detected [ 61.119310] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms] [ 61.119759] JDB: rt1711h_set_vbus source: 0 sink: 0 [ 61.131951] JDB: rt1711h_set_vbus disabling regulator! [ 61.137141] type-c-vbus-current-regulator: disabled successfully?! JDB! (ret=0) [ 61.145007] rt1711h_set_vbus: vbus := Off [ 61.145010] rt1711h_set_vbus: charge is already Off [ 61.362956] JDB: hub_usb_role_switch_set switching to ROLE_NONE! [ 61.374082] usb 1-1: USB disconnect, device number 9 [ 61.380600] JDB: tcpm_state_machine_work state chage: SRC_UNATTACHED->TOGGLING [ 61.390394] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING [ 61.397257] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 61.419292] usb 1-1.1: new low-speed USB device number 10 using xhci-hcd [ 61.427378] usb 1-1-port1: attempt power cycle [ 61.452874] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 61.655250] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms] [ 61.655398] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY [ 61.723383] usb 2-1: new SuperSpeed Gen 1 USB device number 4 using xhci-hcd [ 61.748163] usb 2-1: New USB device found, idVendor=0424, idProduct=5734, bcdDevice= 2.02 [ 61.757846] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0 [ 61.763291] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms] [ 61.763317] JDB: tcpm_state_machine_work state chage: SNK_TRY_WAIT->SRC_TRYWAIT [ 61.766781] usb 2-1: Product: USB5734 [ 61.782560] JDB: tcpm_state_machine_work state chage: SRC_TRYWAIT->SRC_TRYWAIT [ 61.790221] usb 2-1: Manufacturer: Microchip Tech [ 61.824476] hub 2-1:1.0: USB hub found [ 61.828701] hub 2-1:1.0: 5 ports detected [ 61.883350] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED [delayed 100 ms] [ 61.883378] JDB: tcpm_state_machine_work state chage: SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED [ 61.901040] JDB: tcpm_state_machine_work state chage: SNK_UNATTACHED->TOGGLING [ 61.909513] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING [ 61.955483] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 62.035296] usb 1-1: new high-speed USB device number 14 using xhci-hcd [ 62.159263] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED [delayed 200 ms] [ 62.159750] JDB: hub_usb_role_switch_set switching to ROLE_HOST! [ 62.174502] JDB: rt1711h_set_vbus source: 1 sink: 0 [ 62.179534] JDB: rt1711h_set_vbus enabling regulator! [ 62.185067] type-c-vbus-current-regulator: being enabled! JDB! [ 62.191039] type-c-vbus-current-regulator: enabled successfully?! JDB! [ 62.198180] usb 1-1: device descriptor read/all, error -71 [ 62.203769] rt1711h_set_vbus: vbus := On [ 62.203775] rt1711h_set_vbus: charge is already Off [ 62.351356] usb 2-1: USB disconnect, device number 4 [ 62.421558] JDB: tcpm_state_machine_work state chage: SRC_ATTACHED->SRC_ATTACHED [ 62.543282] usb 1-1: new high-speed USB device number 15 using xhci-hcd [ 62.696916] usb 1-1: New USB device found, idVendor=2109, idProduct=2813, bcdDevice=90.11 [ 62.705142] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [ 62.712616] usb 1-1: Product: USB2.0 Hub [ 62.716595] usb 1-1: Manufacturer: VIA Labs, Inc. [ 62.784743] hub 1-1:1.0: USB hub found [ 62.788841] hub 1-1:1.0: 4 ports detected [ 62.911249] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms] [ 62.911598] JDB: rt1711h_set_vbus source: 0 sink: 0 [ 62.923769] JDB: rt1711h_set_vbus disabling regulator! [ 62.928940] type-c-vbus-current-regulator: disabled successfully?! JDB! (ret=0) [ 62.936711] rt1711h_set_vbus: vbus := Off [ 62.936714] rt1711h_set_vbus: charge is already Off [ 63.143272] usb 1-1.1: new low-speed USB device number 16 using xhci-hcd [ 63.154684] JDB: hub_usb_role_switch_set switching to ROLE_NONE! [ 63.160941] usb 1-1-port1: cannot reset (err = -71) [ 63.161185] usb 1-1: USB disconnect, device number 15 [ 63.171398] JDB: tcpm_state_machine_work state chage: SRC_UNATTACHED->TOGGLING [ 63.175823] usb 1-1-port1: attempt power cycle [ 63.181995] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING [ 63.182155] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 63.244450] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 63.447246] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms] [ 63.447391] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY [ 63.507335] usb 2-1: new SuperSpeed Gen 1 USB device number 5 using xhci-hcd [ 63.532130] usb 2-1: New USB device found, idVendor=0424, idProduct=5734, bcdDevice= 2.02 [ 63.542169] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0 [ 63.549402] usb 2-1: Product: USB5734 [ 63.551286] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms] [ 63.551313] JDB: tcpm_state_machine_work state chage: SNK_TRY_WAIT->SRC_TRYWAIT [ 63.554868] usb 2-1: Manufacturer: Microchip Tech [ 63.571708] JDB: tcpm_state_machine_work state chage: SRC_TRYWAIT->SRC_TRYWAIT [ 63.600310] hub 2-1:1.0: USB hub found [ 63.604194] hub 2-1:1.0: 5 ports detected [ 63.675303] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED [delayed 100 ms] [ 63.675331] JDB: tcpm_state_machine_work state chage: SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED [ 63.693027] JDB: tcpm_state_machine_work state chage: SNK_UNATTACHED->TOGGLING [ 63.701676] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING [ 63.747517] JDB: tcpm_state_machine_work state chage: SRC_ATTACH_WAIT->SRC_ATTACH_WAIT [ 63.827281] usb 1-1: new high-speed USB device number 20 using xhci-hcd [ 63.941498] JDB: tcpm_state_machine_work state chage: SRC_UNATTACHED->TOGGLING [ 63.948872] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING [ 63.979728] usb 1-1: New USB device found, idVendor=0424, idProduct=2734, bcdDevice= 2.02 [ 63.988033] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [ 63.995636] usb 1-1: Product: USB2734 [ 63.999540] usb 1-1: Manufacturer: Microchip Tech [ 64.064532] hub 1-1:1.0: USB hub found [ 64.068557] hub 1-1:1.0: 5 ports detected [ 64.415290] usb 1-1.5: new high-speed USB device number 21 using xhci-hcd [ 64.520307] usb 1-1.5: New USB device found, idVendor=0424, idProduct=2740, bcdDevice= 2.00 [ 64.528969] usb 1-1.5: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [ 64.536345] usb 1-1.5: Product: Hub Controller [ 64.540828] usb 1-1.5: Manufacturer: Microchip Tech
On Thu, Nov 14, 2019 at 2:11 AM Hans de Goede <hdegoede@redhat.com> wrote: > On 22-10-2019 07:58, John Stultz wrote: > > I'm fine to > > continue digging and working on this approach, but I also don't want > > to have to pollute the core code too much for this oddball hardware > > (esp since doing the vbus control in the role-switch intermediary does > > work ok - or at least better then this approach so far). > > Given the special nature of the hardware I'm fine with the OTG intermediary > approach here. IMHO it is fine to just stick with that and to not spend > too much time on this. Ok. That was what I was leaning towards as well. Thanks again for all the review and feedback here! I really appreciate it! -john
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c index 94b4e7db2b94..418e762d5d72 100644 --- a/drivers/usb/roles/class.c +++ b/drivers/usb/roles/class.c @@ -20,6 +20,7 @@ struct usb_role_switch { struct device dev; struct mutex lock; /* device lock*/ enum usb_role role; + struct blocking_notifier_head nh; /* From descriptor */ struct device *usb2_port; @@ -49,8 +50,10 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) mutex_lock(&sw->lock); ret = sw->set(sw->dev.parent, role); - if (!ret) + if (!ret) { sw->role = role; + blocking_notifier_call_chain(&sw->nh, role, NULL); + } mutex_unlock(&sw->lock); @@ -58,6 +61,35 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) } EXPORT_SYMBOL_GPL(usb_role_switch_set_role); +int usb_role_switch_register_notifier(struct usb_role_switch *sw, + struct notifier_block *nb) +{ + int ret = blocking_notifier_chain_register(&sw->nh, nb); + enum usb_role role; + + if (ret) + return ret; + + /* Initialize the notifier that was just registered */ + mutex_lock(&sw->lock); + if (sw->get) + role = sw->get(sw->dev.parent); + else + role = sw->role; + blocking_notifier_call_chain(&sw->nh, role, NULL); + mutex_unlock(&sw->lock); + + return 0; +} +EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier); + +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, + struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&sw->nh, nb); +} +EXPORT_SYMBOL_GPL(usb_role_switch_unregister_notifier); + /** * usb_role_switch_get_role - Get the USB role for a switch * @sw: USB role switch @@ -296,6 +328,7 @@ usb_role_switch_register(struct device *parent, return ERR_PTR(-ENOMEM); mutex_init(&sw->lock); + BLOCKING_INIT_NOTIFIER_HEAD(&sw->nh); sw->allow_userspace_control = desc->allow_userspace_control; sw->usb2_port = desc->usb2_port; diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h index 2d77f97df72d..8dbf7940b7da 100644 --- a/include/linux/usb/role.h +++ b/include/linux/usb/role.h @@ -54,6 +54,10 @@ struct usb_role_switch * usb_role_switch_register(struct device *parent, const struct usb_role_switch_desc *desc); void usb_role_switch_unregister(struct usb_role_switch *sw); +int usb_role_switch_register_notifier(struct usb_role_switch *sw, + struct notifier_block *nb); +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, + struct notifier_block *nb); #else static inline int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) @@ -87,6 +91,18 @@ usb_role_switch_register(struct device *parent, } static inline void usb_role_switch_unregister(struct usb_role_switch *sw) { } + +static int usb_role_switch_register_notifier(struct usb_role_switch *sw, + struct notifier_block *nb) +{ + return -ENODEV; +} + +static int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, + struct notifier_block *nb) +{ + return -ENODEV; +} #endif #endif /* __LINUX_USB_ROLE_H */