> 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 ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
