RE: [PATCH v3 2/2] usb: dwc2: add 'mode' which based on Kconfig select or dts setting

From: Paul Zimmerman
Date: Tue Aug 05 2014 - 16:08:56 EST

> From: Kever Yang [mailto:kever.yang@xxxxxxxxxxxxxx]
> Sent: Monday, August 04, 2014 6:46 PM
>
> On 08/05/2014 12:34 AM, Doug Anderson wrote:
>
> > Normally I'd say that you should have added Paul's "Acked-by" since
> > you fixed his nit and he told you to include his Acked-by when his nit
> > was fixed, but...
> There are some more changes than Paul's suggestion, so I'm not sure
> if Paul need more review to give me the "Acked-by", I get it now.

Yes, you did the right thing by leaving off my ack (although you should
have mentioned the additional changes in your email).

> >> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> >> index 27d2c9b..738bec2 100644
> >> --- a/drivers/usb/dwc2/core.c
> >> +++ b/drivers/usb/dwc2/core.c
> >> @@ -118,6 +118,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
> >> {
> >> u32 greset;
> >> int count = 0;
> >> + u32 gusbcfg;
> >>
> >> dev_vdbg(hsotg->dev, "%s()\n", __func__);
> >>
> >> @@ -148,6 +149,23 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
> >> }
> >> } while (greset & GRSTCTL_CSFTRST);
> >>
> >> + if (hsotg->dr_mode == USB_DR_MODE_HOST) {
> >> + gusbcfg = readl(hsotg->regs + GUSBCFG);
> >> + gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
> >> + gusbcfg |= GUSBCFG_FORCEHOSTMODE;
> >> + writel(gusbcfg, hsotg->regs + GUSBCFG);
> >> + } else if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) {
> >> + gusbcfg = readl(hsotg->regs + GUSBCFG);
> >> + gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
> >> + gusbcfg |= GUSBCFG_FORCEDEVMODE;
> >> + writel(gusbcfg, hsotg->regs + GUSBCFG);
> >> + } else if (hsotg->dr_mode == USB_DR_MODE_OTG) {
> >> + gusbcfg = readl(hsotg->regs + GUSBCFG);
> >> + gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
> >> + gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
> >> + writel(gusbcfg, hsotg->regs + GUSBCFG);
> >> + }
> >> +
> >> /*
> >> * NOTE: This long sleep is _very_ important, otherwise the core will
> >> * not stay in host mode after a connector ID change!
> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> >> index 1efd10c..52a4fd2 100644
> >> --- a/drivers/usb/dwc2/core.h
> >> +++ b/drivers/usb/dwc2/core.h
> >> @@ -501,6 +501,10 @@ struct dwc2_hw_params {
> >> * a_peripheral and b_device=>b_host) this may not match
> >> * the core, but allows the software to determine
> >> * transitions
> >> + * @dr_mode: Requested mode of operation, one of following:
> >> + * - USB_DR_MODE_PERIPHERAL
> >> + * - USB_DR_MODE_HOST
> >> + * - USB_DR_MODE_OTG
> >> * @queuing_high_bandwidth: True if multiple packets of a high-bandwidth
> >> * transfer are in process of being queued
> >> * @srp_success: Stores status of SRP request in the case of a FS PHY
> >> @@ -592,6 +596,7 @@ struct dwc2_hsotg {
> >> /** Params to actually use */
> >> struct dwc2_core_params *core_params;
> >> enum usb_otg_state op_state;
> >> + enum usb_dr_mode dr_mode;
> >>
> >> unsigned int queuing_high_bandwidth:1;
> >> unsigned int srp_success:1;
> >> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> >> index a10e7a3..4d2c738 100644
> >> --- a/drivers/usb/dwc2/platform.c
> >> +++ b/drivers/usb/dwc2/platform.c
> >> @@ -42,6 +42,8 @@
> >> #include <linux/of_device.h>
> >> #include <linux/platform_device.h>
> >>
> >> +#include <linux/usb/of.h>
> >> +
> >> #include "core.h"
> >> #include "hcd.h"
> >>
> >> @@ -171,6 +173,16 @@ static int dwc2_driver_probe(struct platform_device *dev)
> >> dev_dbg(&dev->dev, "mapped PA %08lx to VA %p\n",
> >> (unsigned long)res->start, hsotg->regs);
> >>
> >> + hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
> >> +
> >> + if (IS_ENABLED(CONFIG_USB_DWC2_HOST))
> >> + hsotg->dr_mode = USB_DR_MODE_HOST;
> >> + else if (IS_ENABLED(CONFIG_USB_DWC2_GADGET))
> >> + hsotg->dr_mode = USB_DR_MODE_PERIPHERAL;
> >> +
> >> + if (hsotg->dr_mode == USB_DR_MODE_UNKNOWN)
> >> + hsotg->dr_mode = USB_DR_MODE_OTG;
> >> +
> > Please remove this whole chunk. Specifically:
> >
> > * CONFIG_USB_DWC2_GADGET is not a config option and is nowhere in any
> > KConfig files.
> This should be USB_DWC2_PERIPHERAL, and I just copy it wrong from dwc3.
> >
> > * CONFIG_USB_DWC2_HOST claims in Kconfig that we'll be in "host only"
> > mode, but I don't think that's right. The only way you'll get any
> > host functionality is if CONFIG_USB_DWC2_HOST is defined, so we'd need
> > that defined even for OTG mode.
> Yeap, I add the chunk above because I'm confused by "Host only mode" for
> CONFIG_USB_DWC2_HOST in Kconfig. If it's the "Host only mode", I should
> add the dr_mode for it to make sure the controller works in host mode.
>
> Maybe dwc2 is refer to the Kconfig in dwc3 for there is a choice from
> one of "Host only mode", "Gadget only mode" and "Dual Role mode",
> which means the role is decided by the Kconfig.
>
> In my opinion, there maybe more than one controller in a Soc, and for
> different
> usage, the mode select should not be done in Kconfig, it's better to do that
> in dts file.
> I agree with you that the CONFIG_USB_DWC2_HOST should defined for host
> functionality, not for role definition.
>
> Paul: what do you think?

I'm confused about how your dual-role mode implementation is supposed
to work. Right now, the host and peripheral modes are compiled as
separate drivers, and there is no dual-role mode in the Kconfig. So I
don't see how your code can work. Have you tested it?

Dinh Nguyen is working on a patch series to add dual-role support, I
think you have seen that, right? That adds the Kconfig options for all
three modes. Maybe you should build on top of that?

--
Paul