[PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support
Amelie DELAUNAY
amelie.delaunay at st.com
Tue Jul 7 12:13:47 EDT 2020
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/
More information about the linux-arm-kernel
mailing list