Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role switch support

[Amelie DELAUNAY]

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

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