Hi Amelie, On Tue, Jul 7, 2020 at 6:13 PM Amelie DELAUNAY <amelie.delaunay@xxxxxx> 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. [...] > > +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). I see - thank you for the explanation! Best regards, Martin
