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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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�����٥





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux