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/
