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

[Amelie DELAUNAY <amelie.delaunay@st.com>]

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/