| Message ID | 5fd8c3934df0d740d78616046451f983e07b92eb.1532637584.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: Set default mode for dwc_usb31 | expand |
On Thu, Jul 26, 2018 at 11:52 PM, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > dwc_usb31 does not support OTG mode. If the controller supports DRD but > the dr_mode is not specified or set to OTG, then set the mode to > peripheral. > > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > --- > drivers/usb/dwc3/core.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 21e4931d0cc0..64ba664d467c 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -78,6 +78,14 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) > mode = USB_DR_MODE_HOST; > else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) > mode = USB_DR_MODE_PERIPHERAL; > + > + /* > + * dwc_usb31 does not support OTG mode. If the controller > + * supports DRD but the dr_mode is not specified or set to OTG, > + * then set the mode to peripheral. > + */ > + if (mode == USB_DR_MODE_OTG && dwc3_is_usb31(dwc)) shouldn't be simple else if (dwc3_is_usb31(...)) ? > + mode = USB_DR_MODE_PERIPHERAL; > } > > if (mode != dwc->dr_mode) { > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/26/2018 2:32 PM, Andy Shevchenko wrote: > On Thu, Jul 26, 2018 at 11:52 PM, Thinh Nguyen > <Thinh.Nguyen@synopsys.com> wrote: >> dwc_usb31 does not support OTG mode. If the controller supports DRD but >> the dr_mode is not specified or set to OTG, then set the mode to >> peripheral. >> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >> --- >> drivers/usb/dwc3/core.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 21e4931d0cc0..64ba664d467c 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -78,6 +78,14 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) >> mode = USB_DR_MODE_HOST; >> else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) >> mode = USB_DR_MODE_PERIPHERAL; >> + >> + /* >> + * dwc_usb31 does not support OTG mode. If the controller >> + * supports DRD but the dr_mode is not specified or set to OTG, >> + * then set the mode to peripheral. >> + */ >> + if (mode == USB_DR_MODE_OTG && dwc3_is_usb31(dwc)) > shouldn't be simple > > else if (dwc3_is_usb31(...)) > > ? Yes you're right. :) Thanks, Thinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/26/2018 2:59 PM, Thinh Nguyen wrote: > On 7/26/2018 2:32 PM, Andy Shevchenko wrote: >> On Thu, Jul 26, 2018 at 11:52 PM, Thinh Nguyen >> <Thinh.Nguyen@synopsys.com> wrote: >>> dwc_usb31 does not support OTG mode. If the controller supports DRD but >>> the dr_mode is not specified or set to OTG, then set the mode to >>> peripheral. >>> >>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >>> --- >>> drivers/usb/dwc3/core.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 21e4931d0cc0..64ba664d467c 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -78,6 +78,14 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) >>> mode = USB_DR_MODE_HOST; >>> else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) >>> mode = USB_DR_MODE_PERIPHERAL; >>> + >>> + /* >>> + * dwc_usb31 does not support OTG mode. If the controller >>> + * supports DRD but the dr_mode is not specified or set to OTG, >>> + * then set the mode to peripheral. >>> + */ >>> + if (mode == USB_DR_MODE_OTG && dwc3_is_usb31(dwc)) >> shouldn't be simple >> >> else if (dwc3_is_usb31(...)) >> >> ? Actually, no. We want to set the mode to peripheral _only_ when dr_mode was not specified or set to OTG. Just checking for dwc3_is_usb31(...) is not enough. Thanks, Thinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 27, 2018 at 2:02 AM, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > On 7/26/2018 2:59 PM, Thinh Nguyen wrote: >> On 7/26/2018 2:32 PM, Andy Shevchenko wrote: >>> On Thu, Jul 26, 2018 at 11:52 PM, Thinh Nguyen >>> <Thinh.Nguyen@synopsys.com> wrote: >>>> dwc_usb31 does not support OTG mode. If the controller supports DRD but >>>> the dr_mode is not specified or set to OTG, then set the mode to >>>> peripheral. >>>> >>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >>>> --- >>>> drivers/usb/dwc3/core.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>> index 21e4931d0cc0..64ba664d467c 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>>> @@ -78,6 +78,14 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) >>>> mode = USB_DR_MODE_HOST; >>>> else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) >>>> mode = USB_DR_MODE_PERIPHERAL; >>>> + >>>> + /* >>>> + * dwc_usb31 does not support OTG mode. If the controller >>>> + * supports DRD but the dr_mode is not specified or set to OTG, >>>> + * then set the mode to peripheral. >>>> + */ >>>> + if (mode == USB_DR_MODE_OTG && dwc3_is_usb31(dwc)) >>> shouldn't be simple >>> >>> else if (dwc3_is_usb31(...)) >>> >>> ? > > Actually, no. We want to set the mode to peripheral _only_ when dr_mode > was not specified or set to OTG. Just checking for dwc3_is_usb31(...) is > not enough. How come? If I read the code correctly... When you go to default case in this switch it's possible if and only if you have mode _exactly_ OTG. You can't have mode unknown here either. The check is redundant and absence of else adds additional burden on the all the rest cases.
Hi, Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Fri, Jul 27, 2018 at 2:02 AM, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: >> On 7/26/2018 2:59 PM, Thinh Nguyen wrote: >>> On 7/26/2018 2:32 PM, Andy Shevchenko wrote: >>>> On Thu, Jul 26, 2018 at 11:52 PM, Thinh Nguyen >>>> <Thinh.Nguyen@synopsys.com> wrote: >>>>> dwc_usb31 does not support OTG mode. If the controller supports DRD but >>>>> the dr_mode is not specified or set to OTG, then set the mode to >>>>> peripheral. >>>>> >>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >>>>> --- >>>>> drivers/usb/dwc3/core.c | 8 ++++++++ >>>>> 1 file changed, 8 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>> index 21e4931d0cc0..64ba664d467c 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>>> @@ -78,6 +78,14 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) >>>>> mode = USB_DR_MODE_HOST; >>>>> else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) >>>>> mode = USB_DR_MODE_PERIPHERAL; >>>>> + >>>>> + /* >>>>> + * dwc_usb31 does not support OTG mode. If the controller >>>>> + * supports DRD but the dr_mode is not specified or set to OTG, >>>>> + * then set the mode to peripheral. >>>>> + */ >>>>> + if (mode == USB_DR_MODE_OTG && dwc3_is_usb31(dwc)) >>>> shouldn't be simple >>>> >>>> else if (dwc3_is_usb31(...)) >>>> >>>> ? >> >> Actually, no. We want to set the mode to peripheral _only_ when dr_mode >> was not specified or set to OTG. Just checking for dwc3_is_usb31(...) is >> not enough. > > How come? > > If I read the code correctly... > > When you go to default case in this switch it's possible if and only > if you have mode _exactly_ OTG. You can't have mode unknown here > either. > The check is redundant and absence of else adds additional burden on > the all the rest cases. Look a little closer > mode = dwc->dr_mode; > hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > switch (hw_mode) { ^^^^^^^ Switching on hw_mode, not mode. > case DWC3_GHWPARAMS0_MODE_GADGET: > if (IS_ENABLED(CONFIG_USB_DWC3_HOST)) { > dev_err(dev, > "Controller does not support host mode.\n"); > return -EINVAL; > } > mode = USB_DR_MODE_PERIPHERAL; > break; > case DWC3_GHWPARAMS0_MODE_HOST: > if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) { > dev_err(dev, > "Controller does not support device mode.\n"); > return -EINVAL; > } > mode = USB_DR_MODE_HOST; > break; > default: > if (IS_ENABLED(CONFIG_USB_DWC3_HOST)) > mode = USB_DR_MODE_HOST; > else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) > mode = USB_DR_MODE_PERIPHERAL; both of these are checking for Kconfig choices. > /* > * dwc_usb31 does not support OTG mode. If the controller > * supports DRD but the dr_mode is not specified or set to OTG, > * then set the mode to peripheral. > */ > if (mode == USB_DR_MODE_OTG && dwc3_is_usb31(dwc)) ^^^^^^^^^^^^^^^^^^^^^^^ making sure device_property "dr_mode" is OTG. Note if I don't very that mode is OTG, all I know for sure is that HW is configured for DRD operation and Kconfig enabled support for both Host and Peripheral roles, but I have not yet verified e.g. DeviceTree. > mode = USB_DR_MODE_PERIPHERAL; > }
On Fri, Jul 27, 2018 at 1:14 PM, Felipe Balbi <balbi@kernel.org> wrote: >>>>>> + >>>>>> + /* >>>>>> + * dwc_usb31 does not support OTG mode. If the controller >>>>>> + * supports DRD but the dr_mode is not specified or set to OTG, >>>>>> + * then set the mode to peripheral. >>>>>> + */ >>>>>> + if (mode == USB_DR_MODE_OTG && dwc3_is_usb31(dwc)) >>>>> shouldn't be simple >>>>> >>>>> else if (dwc3_is_usb31(...)) >>>>> >>>>> ? >>> >>> Actually, no. We want to set the mode to peripheral _only_ when dr_mode >>> was not specified or set to OTG. Just checking for dwc3_is_usb31(...) is >>> not enough. >> >> How come? >> >> If I read the code correctly... >> >> When you go to default case in this switch it's possible if and only >> if you have mode _exactly_ OTG. You can't have mode unknown here >> either. >> The check is redundant and absence of else adds additional burden on >> the all the rest cases. > > Look a little closer > >> mode = dwc->dr_mode; >> hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> >> switch (hw_mode) { > ^^^^^^^ > Switching on hw_mode, not mode. Ah, indeed. That's what I have missed. Thanks for detailed explanation!
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 21e4931d0cc0..64ba664d467c 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -78,6 +78,14 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc) mode = USB_DR_MODE_HOST; else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) mode = USB_DR_MODE_PERIPHERAL; + + /* + * dwc_usb31 does not support OTG mode. If the controller + * supports DRD but the dr_mode is not specified or set to OTG, + * then set the mode to peripheral. + */ + if (mode == USB_DR_MODE_OTG && dwc3_is_usb31(dwc)) + mode = USB_DR_MODE_PERIPHERAL; } if (mode != dwc->dr_mode) {
dwc_usb31 does not support OTG mode. If the controller supports DRD but the dr_mode is not specified or set to OTG, then set the mode to peripheral. Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> --- drivers/usb/dwc3/core.c | 8 ++++++++ 1 file changed, 8 insertions(+)