Re: [PATCH 1/2] usb: cdns3: fix cdns3_core_init_role()

Roger Quadros
2019-10-16T09:55:17Z

On 16/10/2019 12:38, Pawel Laszczak wrote:
>>
>>
>> Hi Pawel,
>>
>> On 16/10/2019 07:32, Pawel Laszczak wrote:
>>> Hi Roger
>>>
>>>>
>>>> At startup we should trigger the HW state machine
>>>> only if it is OTG mode. Otherwise we should just
>>>> start the respective role.
>>>>
>>>> Initialize idle role by default. If we don't do this then
>>>> cdns3_idle_role_stop() is not called when switching to
>>>> host/device role and so lane switch mechanism
>>>> doesn't work. This results to super-speed device not working
>>>> in one orientation if it was plugged before driver probe.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>>> ---
>>>> drivers/usb/cdns3/core.c | 20 +++++++++++++++++++-
>>>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>> index 06f1e105be4e..1109dc5a4c39 100644
>>>> --- a/drivers/usb/cdns3/core.c
>>>> +++ b/drivers/usb/cdns3/core.c
>>>> @@ -160,10 +160,28 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>> if (ret)
>>>> goto err;
>>>>
>>>> - if (cdns->dr_mode != USB_DR_MODE_OTG) {
>>>> + /* Initialize idle role to start with */
>>>> + ret = cdns3_role_start(cdns, USB_ROLE_NONE);
>>>> + if (ret)
>>>> + goto err;
>>>> +
>>>> + switch (cdns->dr_mode) {
>>>> + case USB_DR_MODE_UNKNOWN:
>>>
>>> One note in this place. USB_DR_MODE_UNKNOWN is not possible in this place.
>>> If cdns->dr_mode will be USB_DR_MODE_UNKNOWN then driver returns -EINVAL
>>
>> At which place? I could not find.
>
> In this patch we can't see this line:
> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/usb/cdns3/core.c#L159
> There is called cdns3_drd_update_mode(cdns);
>
> int cdns3_drd_update_mode(struct cdns3 *cdns)
> {
> int ret = 0;
>
> switch (cdns->dr_mode) {
> case USB_DR_MODE_PERIPHERAL:
> ret = cdns3_set_mode(cdns, USB_DR_MODE_PERIPHERAL);
> break;
> case USB_DR_MODE_HOST:
> ret = cdns3_set_mode(cdns, USB_DR_MODE_HOST);
> break;
> case USB_DR_MODE_OTG:
> ret = cdns3_init_otg_mode(cdns);
> break;
> default:
> dev_err(cdns->dev, "Unsupported mode of operation %d\n",
> cdns->dr_mode);
> return -EINVAL;
> }
>
> return ret;
> }
>
> After calling cdns3_drd_update_mode we have 2 first lines from this patch
> if (ret)
> goto err;

I see now. I will update this patch to error out if dr_mode is USB_DR_MODE_UNKNOWN.
Thanks.

cheers,
-roger

>
> Pawel
>>
>>> some line before after returning form cdns3_drd_update_mode and in consequence
>>> it jump to err label.
>>>
>>> Maybe for better readability it this condition should be treated here also as error.
>>>
>>>> + case USB_DR_MODE_OTG:
>>>> ret = cdns3_hw_role_switch(cdns);
>>>> if (ret)
>>>> goto err;
>>>> + break;
>>>> + case USB_DR_MODE_PERIPHERAL:
>>>> + ret = cdns3_role_start(cdns, USB_ROLE_DEVICE);
>>>> + if (ret)
>>>> + goto err;
>>>> + break;
>>>> + case USB_DR_MODE_HOST:
>>>> + ret = cdns3_role_start(cdns, USB_ROLE_HOST);
>>>> + if (ret)
>>>> + goto err;
>>>> + break;
>>>> }
>>>>
>>>> return ret;
>>>
>>> Reviewed-by: Pawel Laszczak <pawell@cadence.com>
>>> Tested-by: Pawel Laszczak <pawell@cadence.com>
>>>
>>> --
>>> Regards,
>>> Pawel
>>>
>>
>> --
>> cheers,
>> -roger
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki