Hi,
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> writes:
> On Fri, Jul 27, 2018 at 2:02 AM, Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>>> ---
>>>>> 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;
> }
--
balbi
Attachment:
signature.asc
Description: PGP signature
