Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Amelie DELAUNAY]

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/