Hi Martin,

On 7/7/20 8:55 PM, Martin Blumenstingl wrote:

Hi Amelie,

On Tue, Jul 7, 2020 at 6:13 PM Amelie DELAUNAY [off-list ref] wrote:

quoted

Hi Martin,

On 7/4/20 7:42 PM, Martin Blumenstingl wrote:

quoted

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/.

one of the boards that I'm working on is for example the Odroid-C1. It
has a Micro-USB port and there's no Type-C controller present.

in the next few days I'll try to send my idea as RFC, but this is the
.dts I've come up with so far:
&usb0 {
     dr_mode = "otg";
     usb-role-switch;

     connector {
         compatible = "extcon-usb-b-connector", "usb-b-connector";
         type = "micro";
         extcon = <&usb0_phy>;
         vbus-supply = <&usb_vbus>;
     };
};

I did this for two reasons:
1. I think the PHY is not a connector and thus it's driver shouldn't
implement any connector specific logic (managing VBUS)
2. without the connector there would be a circular dependency: the USB
controller needs the PHY to initialize but the PHY would need the USB
controller so it can manage the role switch

(or in other words: the connector replaces the Type-C controller in this case)

quoted

quoted

[...]

quoted

+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() ...

quoted

+     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.

on my Amlogic SoC this is not the case:
The kernel complains because of that usleep_range from within the
spinlock context

Please let me know if/how I can help debug this.

Could you please test with:

static int dwc2_drd_role_sw_set(struct device *dev, enum usb_role role)
{
	struct dwc2_hsotg *hsotg = dev_get_drvdata(dev);
	unsigned long flags;
	int already = 0;

	/* 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);

	if (role == USB_ROLE_HOST) {
		already = dwc2_ovr_avalid(hsotg, true);
	} else if (role == USB_ROLE_DEVICE) {
		already = dwc2_ovr_bvalid(hsotg, true);
		/* This clear DCTL.SFTDISCON bit */
		dwc2_hsotg_core_connect(hsotg);
	} else {
		if (dwc2_is_device_mode(hsotg)) {
		    if (!dwc2_ovr_bvalid(hsotg, false))
			/* This set DCTL.SFTDISCON bit */
			dwc2_hsotg_core_disconnect(hsotg);
		} else {
			dwc2_ovr_avalid(hsotg, false);
		}
	}

	spin_unlock_irqrestore(&hsotg->lock, flags);

	if (!already &&
	    role != USB_ROLE_NONE && hsotg->dr_mode == USB_DR_MODE_OTG)
		/* This will raise a Connector ID Status Change Interrupt */
		dwc2_force_mode(hsotg, role == USB_ROLE_HOST);

	dev_dbg(hsotg->dev, "%s-session valid\n",
		role == USB_ROLE_NONE ? "No" :
		role == USB_ROLE_HOST ? "A" : "B");

	return 0;
}


dwc2_force_mode is called outside the spin_lock_irqsave so the kernel 
should not complain. I've tested on my setup and the behavior seems the 
same.

Regards,
Amelie