usb: dwc3: Set default mode for dwc_usb31

[Felipe Balbi <balbi@kernel.org>]

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