Messages in this thread | | | Subject | Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support | From | Amelie DELAUNAY <> | Date | Wed, 8 Jul 2020 18:00:07 +0200 |
| |
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 <amelie.delaunay@st.com> wrote: >> >> 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/. > 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) > >>> [...] >>>> +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. > 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
| |