diff mbox series

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

Message ID 20191007121601.25996-2-rogerq@ti.com
State Accepted
Commit 02ffc26df96b487f476b8945eaef13c7f170e79a
Headers show
Series [1/2] usb: cdns3: fix cdns3_core_init_role() | expand

Commit Message

Roger Quadros Oct. 7, 2019, 12:16 p.m. UTC 
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(-)

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

Comments

Pawel Laszczak Oct. 16, 2019, 4:32 a.m. UTC | #1 
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
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>


>--

>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.

>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

--
Regards,
Pawel
Roger Quadros Oct. 16, 2019, 9:20 a.m. UTC | #2 
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.

> 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
Pawel Laszczak Oct. 16, 2019, 9:38 a.m. UTC | #3 
>

>

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

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
diff mbox series

Patch

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:
+	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;