usb: dwc3: Set default mode for dwc_usb31

usb: dwc3: Set default mode for dwc_usb31

[Andy Shevchenko]

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!

usb: dwc3: Set default mode for dwc_usb31

[Felipe Balbi]

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;
> }

usb: dwc3: Set default mode for dwc_usb31

[Andy Shevchenko]

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.

usb: dwc3: Set default mode for dwc_usb31

[Thinh Nguyen]

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

usb: dwc3: Set default mode for dwc_usb31

[Thinh Nguyen]

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

usb: dwc3: Set default mode for dwc_usb31

[Andy Shevchenko]

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

usb: dwc3: Set default mode for dwc_usb31

[Thinh Nguyen]

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))
+			mode = USB_DR_MODE_PERIPHERAL;
 	}
 
 	if (mode != dwc->dr_mode) {