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

[Roger Quadros]

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@xxxxxx>
> > > > Signed-off-by: Sekhar Nori <nsekhar@xxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxx>
> > > Tested-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
> > >
> > > --
> > > Regards,
> > > Pawel
> >
> > --
> > cheers,
> > -roger
> > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki