Re: [PATCH v5 10/13] usb: dwc3: Registering a role switch in the DRD code.

[John Stultz]

On Thu, Mar 28, 2019 at 9:14 PM Yu Chen <chenyu56@xxxxxxxxxx> wrote:
>
> The Type-C drivers use USB role switch API to inform the
> system about the negotiated data role, so registering a role
> switch in the DRD code in order to support platforms with
> USB Type-C connectors.
>

Hey Yu Chen!
  Thanks so much for sending these patches out!  I have run into some
troubles on bootup where things aren't working properly at first, that
seem to be due to state initialization races.  In chasing those down,
I've found some other quirks I wanted to bring up.

> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -479,6 +479,43 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
>         return edev;
>  }
>
> +static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role)
> +{
> +       struct dwc3 *dwc = dev_get_drvdata(dev);
> +       u32 mode;
> +
> +       switch (role) {
> +       case USB_ROLE_HOST:
> +               mode = DWC3_GCTL_PRTCAP_HOST;
> +               break;
> +       case USB_ROLE_DEVICE:
> +               mode = DWC3_GCTL_PRTCAP_DEVICE;
> +               break;
> +       default:
> +               if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
> +                       mode = DWC3_GCTL_PRTCAP_HOST;
> +               else
> +                       mode = DWC3_GCTL_PRTCAP_DEVICE;
> +               break;
> +       }
> +
> +       dwc3_set_mode(dwc, mode);
> +       return 0;
> +}
> +
> +static enum usb_role dwc3_usb_role_switch_get(struct device *dev)
> +{
> +       struct dwc3 *dwc = dev_get_drvdata(dev);
> +       unsigned long flags;
> +       enum usb_role role;
> +
> +       spin_lock_irqsave(&dwc->lock, flags);
> +       role = dwc->current_otg_role;
> +       spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +       return role;
> +}
> +

So the two functions above are a bit asymmetric.  The
dwc3_usb_role_switch_set() can put the device into host or device
mode, which eventually sets the current_dr_role value.  However on the
dwc3_usb_role_switch_get() we return the current_otg_role value.
current_otg_role isn't set unless current_dr_role is
DWC3_GCTL_PRTCAP_OTG, which doesn't seem to happen here.

I suspect in dwc3_usb_role_switch_get() we should return
current_dr_role, unless current_dr_role==DWC3_GCTL_PRTCAP_OTG, in
which case we'd want to return current_otg_role.

Does that make sense?

Nothing really actually use this dwc3_usb_role_switch_get() yet, so
this was easy to overlook, and I only caught it as I was trying to
debug some of the initialization races.

thanks
-john