Messages in this thread | | | Subject | Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support | From | Amelie DELAUNAY <> | Date | Tue, 7 Jul 2020 18:13:47 +0200 |
| |
Hi Martin,
On 7/4/20 7:42 PM, Martin Blumenstingl wrote: > Hello Amelie, > > thank you for this patch - I am hoping that it will help us on Amlogic > Meson8, Meson8b, Meson8m2 and GXBB SoCs as well. > On these SoCs the ID detection is performed by the PHY IP and needs to > be polled. > I think usb_role_switch is the perfect framework for this on dwc2 side. > For the PHY driver I'm going to implement the cable state using the > extcon framework and then having a new usb-conn-extcon driver. This is > just to give you an overview why I'm interested in this. >
I'm wondering, why use extcon framework and not the usb role switch API ? This patch on dwc2 is tested on STM32MP157C-DK2 board with STUSB160x Type-C controller driver recently pushed with usb role switch. You can have a look here https://lore.kernel.org/patchwork/patch/1256238/.
> [...] >> +static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role) >> +{ >> + struct dwc2_hsotg *hsotg = usb_role_switch_get_drvdata(sw); >> + unsigned long flags; >> + >> + /* Skip session not in line with dr_mode */ >> + if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) || >> + (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)) >> + return -EINVAL; >> + >> + /* Skip session if core is in test mode */ >> + if (role == USB_ROLE_NONE && hsotg->test_mode) { >> + dev_dbg(hsotg->dev, "Core is in test mode\n"); >> + return -EBUSY; >> + } >> + >> + spin_lock_irqsave(&hsotg->lock, flags); > due to this spin_lock_irqsave() ... > >> + if (role == USB_ROLE_HOST) { >> + if (dwc2_ovr_avalid(hsotg, true)) >> + goto unlock; >> + >> + if (hsotg->dr_mode == USB_DR_MODE_OTG) >> + /* >> + * This will raise a Connector ID Status Change >> + * Interrupt - connID A >> + */ >> + dwc2_force_mode(hsotg, true); > ... we cannot sleep in here. the call flow is: > dwc2_drd_role_sw_set > spin_lock_irqsave > dwc2_force_mode > dwc2_wait_for_mode > usleep_range >
In fact, with the avalid or bvalid overriding + the debounce filter bypass, GINTSTS_CURMOD is already in the expected mode, so that we exit the loop directly, without running into usleep_range.
>> + } else if (role == USB_ROLE_DEVICE) { >> + if (dwc2_ovr_bvalid(hsotg, true)) >> + goto unlock; >> + >> + if (hsotg->dr_mode == USB_DR_MODE_OTG) >> + /* >> + * This will raise a Connector ID Status Change >> + * Interrupt - connID B >> + */ >> + dwc2_force_mode(hsotg, false); > (same sleeping issue here) > > [...] > +int dwc2_drd_init(struct dwc2_hsotg *hsotg) > +{ > + struct usb_role_switch_desc role_sw_desc = {0}; > + struct usb_role_switch *role_sw; > + int ret; > + > + if (!device_property_read_bool(hsotg->dev, "usb-role-switch")) > + return 0; > should we also return early here if dr_mode != "otg"? >
No, because when VBUS is not connected to the controller, you also need to get the usb_role_none from the usb-role-switch to catch the unattached state (also in Peripheral or Host only mode).
> [...] > @@ -532,6 +534,13 @@ static int dwc2_driver_probe(struct platform_device > *dev) > dwc2_writel(hsotg, ggpio, GGPIO); > } > > + retval = dwc2_drd_init(hsotg); > + if (retval) { > + if (retval != -EPROBE_DEFER) > + dev_err(hsotg->dev, "failed to initialize > dual-role\n"); > + goto error_init; > + } > + > if (hsotg->dr_mode != USB_DR_MODE_HOST) { > retval = dwc2_gadget_init(hsotg); > if (retval) > I think dwc2_driver_probe() needs a new label (for example named > error_drd) which then calls dwc2_drd_exit. See [0] which I have > submitted as a patch for Linux 5.8, so it's not in usb-next yet. >
I agree.
> Also in general I think you need to submit a dt-bindings patch that > documents the usb-role-switch property. Personally I would use > Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml as > reference for that. >
Sure. Will be done in V2 with new label for drd_exit in probe failure path. I'll rebase my patch on yours to avoid conflicts.
> Can you please keep me Cc'ed on a v2 because I'm not subscribed to the > linux-usb mailing list?
OK. Thanks for your review and future tests! Regards, Amelie
> I am going to test this on Amlogic SoCs - once I made "everything else" > work I can give my Tested-by as well. > > > Thank you! > Martin > > > [0] https://patchwork.kernel.org/patch/11642957/
| |