[PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Roger Quadros]

This reverts commit 50642709f6590fe40afa6d22c32f23f5b842aed5.

This commit breaks hardware based role switching on TI platforms.
cdns->role_sw is always going to be non-zero as it is a pointer
to the usb_role_switch instance. Some other means needs to be used
if hardware based role switching is not required by the platform.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/cdns3/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index a0f73d4711ae..4c1445cf2ad0 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -280,10 +280,6 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
 	enum usb_role real_role, current_role;
 	int ret = 0;
 
-	/* Depends on role switch class */
-	if (cdns->role_sw)
-		return 0;
-
 	pm_runtime_get_sync(cdns->dev);
 
 	current_role = cdns->role;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Peter Chen]

On 20-11-23 13:50:51, Roger Quadros wrote:
> This reverts commit 50642709f6590fe40afa6d22c32f23f5b842aed5.
> 
> This commit breaks hardware based role switching on TI platforms.
> cdns->role_sw is always going to be non-zero as it is a pointer
> to the usb_role_switch instance. Some other means needs to be used
> if hardware based role switching is not required by the platform.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/cdns3/core.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index a0f73d4711ae..4c1445cf2ad0 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -280,10 +280,6 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
>  	enum usb_role real_role, current_role;
>  	int ret = 0;
>  
> -	/* Depends on role switch class */
> -	if (cdns->role_sw)
> -		return 0;
> -
>  	pm_runtime_get_sync(cdns->dev);
>  
>  	current_role = cdns->role;
> -- 

Hi Roger,

I am sorry about that. Do you use role switch /sys entry, if you have
used, I prefer using "usb-role-switch" property at dts to judge if
SoC OTG signals or external signals for role switch. If you have not
used it, I prefer only setting cdns->role_sw for role switch use cases.

-- 

Thanks,
Peter Chen

Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Roger Quadros]

Peter,

On 24/11/2020 08:43, Peter Chen wrote:
> On 20-11-23 13:50:51, Roger Quadros wrote:
>> This reverts commit 50642709f6590fe40afa6d22c32f23f5b842aed5.
>>
>> This commit breaks hardware based role switching on TI platforms.
>> cdns->role_sw is always going to be non-zero as it is a pointer
>> to the usb_role_switch instance. Some other means needs to be used
>> if hardware based role switching is not required by the platform.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>   drivers/usb/cdns3/core.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index a0f73d4711ae..4c1445cf2ad0 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -280,10 +280,6 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
>>   	enum usb_role real_role, current_role;
>>   	int ret = 0;
>>   
>> -	/* Depends on role switch class */
>> -	if (cdns->role_sw)
>> -		return 0;
>> -
>>   	pm_runtime_get_sync(cdns->dev);
>>   
>>   	current_role = cdns->role;
>> -- 
> 
> Hi Roger,
> 
> I am sorry about that. Do you use role switch /sys entry, if you have
> used, I prefer using "usb-role-switch" property at dts to judge if
> SoC OTG signals or external signals for role switch. If you have not
> used it, I prefer only setting cdns->role_sw for role switch use cases.
> 

We use both hardware role switch and /sys entries for manually forcing
a certain role.

We do not set any "usb-role-switch" property at DTS.

Currently cdns->role_sw is being always set by driver irrespective of
any DT property, so this patch is clearly wrong and needs to be reverted.

What do you think?

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

RE: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Peter Chen]

Best regards,
Peter Chen

> -----Original Message-----
> From: Roger Quadros <rogerq@ti.com>
> Sent: 2020年11月24日 17:39
> To: Peter Chen <peter.chen@nxp.com>
> Cc: pawell@cadence.com; gregkh@linuxfoundation.org; balbi@kernel.org;
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"
> 
> Peter,
> 
> On 24/11/2020 08:43, Peter Chen wrote:
> > On 20-11-23 13:50:51, Roger Quadros wrote:
> >> This reverts commit 50642709f6590fe40afa6d22c32f23f5b842aed5.
> >>
> >> This commit breaks hardware based role switching on TI platforms.
> >> cdns->role_sw is always going to be non-zero as it is a pointer
> >> to the usb_role_switch instance. Some other means needs to be used if
> >> hardware based role switching is not required by the platform.
> >>
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> ---
> >>   drivers/usb/cdns3/core.c | 4 ----
> >>   1 file changed, 4 deletions(-)
> >>
> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> >> index a0f73d4711ae..4c1445cf2ad0 100644
> >> --- a/drivers/usb/cdns3/core.c
> >> +++ b/drivers/usb/cdns3/core.c
> >> @@ -280,10 +280,6 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> >>   	enum usb_role real_role, current_role;
> >>   	int ret = 0;
> >>
> >> -	/* Depends on role switch class */
> >> -	if (cdns->role_sw)
> >> -		return 0;
> >> -
> >>   	pm_runtime_get_sync(cdns->dev);
> >>
> >>   	current_role = cdns->role;
> >> --
> >
> > Hi Roger,
> >
> > I am sorry about that. Do you use role switch /sys entry, if you have
> > used, I prefer using "usb-role-switch" property at dts to judge if SoC
> > OTG signals or external signals for role switch. If you have not used
> > it, I prefer only setting cdns->role_sw for role switch use cases.
> >
> 
> We use both hardware role switch and /sys entries for manually forcing a
> certain role.
> 
> We do not set any "usb-role-switch" property at DTS.
> 
> Currently cdns->role_sw is being always set by driver irrespective of any DT
> property, so this patch is clearly wrong and needs to be reverted.
> 
> What do you think?
> 

Could you accept below fix?

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 2e469139769f..fdd52e87a7b2 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
        enum usb_role real_role, current_role;
        int ret = 0;

-       /* Depends on role switch class */
-       if (cdns->role_sw)
+       /* quit if switch role through external signals */
+       if (device_property_read_bool(cdns->dev, "usb-role-switch"))
                return 0;

        pm_runtime_get_sync(cdns->dev);

Peter

Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Roger Quadros]

+Heikki

Peter,

On 24/11/2020 11:57, Peter Chen wrote:
> 
> 
> 
> Best regards,
> Peter Chen
> 
>> -----Original Message-----
>> From: Roger Quadros <rogerq@ti.com>
>> Sent: 2020年11月24日 17:39
>> To: Peter Chen <peter.chen@nxp.com>
>> Cc: pawell@cadence.com; gregkh@linuxfoundation.org; balbi@kernel.org;
>> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"
>>
>> Peter,
>>
>> On 24/11/2020 08:43, Peter Chen wrote:
>>> On 20-11-23 13:50:51, Roger Quadros wrote:
>>>> This reverts commit 50642709f6590fe40afa6d22c32f23f5b842aed5.
>>>>
>>>> This commit breaks hardware based role switching on TI platforms.
>>>> cdns->role_sw is always going to be non-zero as it is a pointer
>>>> to the usb_role_switch instance. Some other means needs to be used if
>>>> hardware based role switching is not required by the platform.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>    drivers/usb/cdns3/core.c | 4 ----
>>>>    1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>> index a0f73d4711ae..4c1445cf2ad0 100644
>>>> --- a/drivers/usb/cdns3/core.c
>>>> +++ b/drivers/usb/cdns3/core.c
>>>> @@ -280,10 +280,6 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
>>>>    	enum usb_role real_role, current_role;
>>>>    	int ret = 0;
>>>>
>>>> -	/* Depends on role switch class */
>>>> -	if (cdns->role_sw)
>>>> -		return 0;
>>>> -
>>>>    	pm_runtime_get_sync(cdns->dev);
>>>>
>>>>    	current_role = cdns->role;
>>>> --
>>>
>>> Hi Roger,
>>>
>>> I am sorry about that. Do you use role switch /sys entry, if you have
>>> used, I prefer using "usb-role-switch" property at dts to judge if SoC
>>> OTG signals or external signals for role switch. If you have not used
>>> it, I prefer only setting cdns->role_sw for role switch use cases.
>>>
>>
>> We use both hardware role switch and /sys entries for manually forcing a
>> certain role.
>>
>> We do not set any "usb-role-switch" property at DTS.
>>
>> Currently cdns->role_sw is being always set by driver irrespective of any DT
>> property, so this patch is clearly wrong and needs to be reverted.
>>
>> What do you think?
>>
> 
> Could you accept below fix?
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 2e469139769f..fdd52e87a7b2 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
>          enum usb_role real_role, current_role;
>          int ret = 0;
> 
> -       /* Depends on role switch class */
> -       if (cdns->role_sw)
> +       /* quit if switch role through external signals */
> +       if (device_property_read_bool(cdns->dev, "usb-role-switch"))
>                  return 0;
> 
>          pm_runtime_get_sync(cdns->dev);

Although this will fix the issue I don't think this is making the driver to behave
as expected with usb-role-switch property.

Now, even if usb-role-switch property is not present the driver will still register
the role switch driver.

I think we need to register the role switch driver only if usb-role-switch property
is present. We would also need to set the default role if role-switch-default-mode is present.

How about the following? It still doesn't handle role-switch-default-mode property though.


diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 4c1445cf2ad0..986b56a9940c 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -532,11 +532,13 @@ static int cdns3_probe(struct platform_device *pdev)
  	if (device_property_read_bool(dev, "usb-role-switch"))
  		sw_desc.fwnode = dev->fwnode;
  
-	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
-	if (IS_ERR(cdns->role_sw)) {
-		ret = PTR_ERR(cdns->role_sw);
-		dev_warn(dev, "Unable to register Role Switch\n");
-		goto err3;
+	if (device_property_read_bool(cdns->dev, "usb-role-switch")) {
+		cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
+		if (IS_ERR(cdns->role_sw)) {
+			ret = PTR_ERR(cdns->role_sw);
+			dev_warn(dev, "Unable to register Role Switch\n");
+			goto err3;
+		}
  	}
  
  	if (cdns->wakeup_irq) {



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

Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Heikki Krogerus]

On Tue, Nov 24, 2020 at 12:33:34PM +0200, Roger Quadros wrote:
> +Heikki
> 
> Peter,
> 
> On 24/11/2020 11:57, Peter Chen wrote:
> > 
> > 
> > 
> > Best regards,
> > Peter Chen
> > 
> > > -----Original Message-----
> > > From: Roger Quadros <rogerq@ti.com>
> > > Sent: 2020年11月24日 17:39
> > > To: Peter Chen <peter.chen@nxp.com>
> > > Cc: pawell@cadence.com; gregkh@linuxfoundation.org; balbi@kernel.org;
> > > linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"
> > > 
> > > Peter,
> > > 
> > > On 24/11/2020 08:43, Peter Chen wrote:
> > > > On 20-11-23 13:50:51, Roger Quadros wrote:
> > > > > This reverts commit 50642709f6590fe40afa6d22c32f23f5b842aed5.
> > > > > 
> > > > > This commit breaks hardware based role switching on TI platforms.
> > > > > cdns->role_sw is always going to be non-zero as it is a pointer
> > > > > to the usb_role_switch instance. Some other means needs to be used if
> > > > > hardware based role switching is not required by the platform.
> > > > > 
> > > > > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > > > > ---
> > > > >    drivers/usb/cdns3/core.c | 4 ----
> > > > >    1 file changed, 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > > > > index a0f73d4711ae..4c1445cf2ad0 100644
> > > > > --- a/drivers/usb/cdns3/core.c
> > > > > +++ b/drivers/usb/cdns3/core.c
> > > > > @@ -280,10 +280,6 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> > > > >    	enum usb_role real_role, current_role;
> > > > >    	int ret = 0;
> > > > > 
> > > > > -	/* Depends on role switch class */
> > > > > -	if (cdns->role_sw)
> > > > > -		return 0;
> > > > > -
> > > > >    	pm_runtime_get_sync(cdns->dev);
> > > > > 
> > > > >    	current_role = cdns->role;
> > > > > --
> > > > 
> > > > Hi Roger,
> > > > 
> > > > I am sorry about that. Do you use role switch /sys entry, if you have
> > > > used, I prefer using "usb-role-switch" property at dts to judge if SoC
> > > > OTG signals or external signals for role switch. If you have not used
> > > > it, I prefer only setting cdns->role_sw for role switch use cases.
> > > > 
> > > 
> > > We use both hardware role switch and /sys entries for manually forcing a
> > > certain role.
> > > 
> > > We do not set any "usb-role-switch" property at DTS.
> > > 
> > > Currently cdns->role_sw is being always set by driver irrespective of any DT
> > > property, so this patch is clearly wrong and needs to be reverted.
> > > 
> > > What do you think?
> > > 
> > 
> > Could you accept below fix?
> > 
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index 2e469139769f..fdd52e87a7b2 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> >          enum usb_role real_role, current_role;
> >          int ret = 0;
> > 
> > -       /* Depends on role switch class */
> > -       if (cdns->role_sw)
> > +       /* quit if switch role through external signals */
> > +       if (device_property_read_bool(cdns->dev, "usb-role-switch"))
> >                  return 0;
> > 
> >          pm_runtime_get_sync(cdns->dev);
> 
> Although this will fix the issue I don't think this is making the driver to behave
> as expected with usb-role-switch property.
> 
> Now, even if usb-role-switch property is not present the driver will still register
> the role switch driver.
> 
> I think we need to register the role switch driver only if usb-role-switch property
> is present. We would also need to set the default role if role-switch-default-mode is present.
> 
> How about the following? It still doesn't handle role-switch-default-mode property though.
> 
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 4c1445cf2ad0..986b56a9940c 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -532,11 +532,13 @@ static int cdns3_probe(struct platform_device *pdev)
>  	if (device_property_read_bool(dev, "usb-role-switch"))
>  		sw_desc.fwnode = dev->fwnode;
> -	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> -	if (IS_ERR(cdns->role_sw)) {
> -		ret = PTR_ERR(cdns->role_sw);
> -		dev_warn(dev, "Unable to register Role Switch\n");
> -		goto err3;
> +	if (device_property_read_bool(cdns->dev, "usb-role-switch")) {
> +		cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> +		if (IS_ERR(cdns->role_sw)) {
> +			ret = PTR_ERR(cdns->role_sw);
> +			dev_warn(dev, "Unable to register Role Switch\n");
> +			goto err3;
> +		}
>  	}
>  	if (cdns->wakeup_irq) {

Makes sense to me. FWIW:

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

thanks,

-- 
heikki

Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Peter Chen]

On 20-11-24 12:33:34, Roger Quadros wrote:
> > > > 
> > > > I am sorry about that. Do you use role switch /sys entry, if you have
> > > > used, I prefer using "usb-role-switch" property at dts to judge if SoC
> > > > OTG signals or external signals for role switch. If you have not used
> > > > it, I prefer only setting cdns->role_sw for role switch use cases.
> > > > 
> > > 
> > > We use both hardware role switch and /sys entries for manually forcing a
> > > certain role.
> > > 
> > > We do not set any "usb-role-switch" property at DTS.
> > > 
> > > Currently cdns->role_sw is being always set by driver irrespective of any DT
> > > property, so this patch is clearly wrong and needs to be reverted.
> > > 
> > > What do you think?
> > > 
> > 
> > Could you accept below fix?
> > 
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index 2e469139769f..fdd52e87a7b2 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> >          enum usb_role real_role, current_role;
> >          int ret = 0;
> > 
> > -       /* Depends on role switch class */
> > -       if (cdns->role_sw)
> > +       /* quit if switch role through external signals */
> > +       if (device_property_read_bool(cdns->dev, "usb-role-switch"))
> >                  return 0;
> > 
> >          pm_runtime_get_sync(cdns->dev);
> 
> Although this will fix the issue I don't think this is making the driver to behave
> as expected with usb-role-switch property.
> 
> Now, even if usb-role-switch property is not present the driver will still register
> the role switch driver.
> 
> I think we need to register the role switch driver only if usb-role-switch property
> is present. We would also need to set the default role if role-switch-default-mode is present.
> 
> How about the following? It still doesn't handle role-switch-default-mode property though.
> 

Roger, you said you also use /sys entries (I suppose it means through role
switch class) to do role switch, with your change, there will be no /sys
entry for role switch.

Peter

> > > We use both hardware role switch and /sys entries for manually forcing a
> > > certain role.




> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 4c1445cf2ad0..986b56a9940c 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -532,11 +532,13 @@ static int cdns3_probe(struct platform_device *pdev)
>  	if (device_property_read_bool(dev, "usb-role-switch"))
>  		sw_desc.fwnode = dev->fwnode;
> -	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> -	if (IS_ERR(cdns->role_sw)) {
> -		ret = PTR_ERR(cdns->role_sw);
> -		dev_warn(dev, "Unable to register Role Switch\n");
> -		goto err3;
> +	if (device_property_read_bool(cdns->dev, "usb-role-switch")) {
> +		cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> +		if (IS_ERR(cdns->role_sw)) {
> +			ret = PTR_ERR(cdns->role_sw);
> +			dev_warn(dev, "Unable to register Role Switch\n");
> +			goto err3;
> +		}
>  	}
>  	if (cdns->wakeup_irq) {
> 
> 
> 
> cheers,
> -roger
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 

Thanks,
Peter Chen

Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Roger Quadros]

Peter,

On 24/11/2020 13:47, Peter Chen wrote:
> On 20-11-24 12:33:34, Roger Quadros wrote:
>>>>>
>>>>> I am sorry about that. Do you use role switch /sys entry, if you have
>>>>> used, I prefer using "usb-role-switch" property at dts to judge if SoC
>>>>> OTG signals or external signals for role switch. If you have not used
>>>>> it, I prefer only setting cdns->role_sw for role switch use cases.
>>>>>
>>>>
>>>> We use both hardware role switch and /sys entries for manually forcing a
>>>> certain role.
>>>>
>>>> We do not set any "usb-role-switch" property at DTS.
>>>>
>>>> Currently cdns->role_sw is being always set by driver irrespective of any DT
>>>> property, so this patch is clearly wrong and needs to be reverted.
>>>>
>>>> What do you think?
>>>>
>>>
>>> Could you accept below fix?
>>>
>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>> index 2e469139769f..fdd52e87a7b2 100644
>>> --- a/drivers/usb/cdns3/core.c
>>> +++ b/drivers/usb/cdns3/core.c
>>> @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
>>>           enum usb_role real_role, current_role;
>>>           int ret = 0;
>>>
>>> -       /* Depends on role switch class */
>>> -       if (cdns->role_sw)
>>> +       /* quit if switch role through external signals */
>>> +       if (device_property_read_bool(cdns->dev, "usb-role-switch"))
>>>                   return 0;
>>>
>>>           pm_runtime_get_sync(cdns->dev);
>>
>> Although this will fix the issue I don't think this is making the driver to behave
>> as expected with usb-role-switch property.
>>
>> Now, even if usb-role-switch property is not present the driver will still register
>> the role switch driver.
>>
>> I think we need to register the role switch driver only if usb-role-switch property
>> is present. We would also need to set the default role if role-switch-default-mode is present.
>>
>> How about the following? It still doesn't handle role-switch-default-mode property though.
>>
> 
> Roger, you said you also use /sys entries (I suppose it means through role
> switch class) to do role switch, with your change, there will be no /sys
> entry for role switch.

Sorry for the confusion. Although we do need both features (SW role switch + HW role switch)
I don't think it is required to operate simultaneously. If users need SW control they can set the DT flag.

cheers,
-roger

> 
> Peter
> 
>>>> We use both hardware role switch and /sys entries for manually forcing a
>>>> certain role.
> 
> 
> 
> 
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index 4c1445cf2ad0..986b56a9940c 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -532,11 +532,13 @@ static int cdns3_probe(struct platform_device *pdev)
>>   	if (device_property_read_bool(dev, "usb-role-switch"))
>>   		sw_desc.fwnode = dev->fwnode;
>> -	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
>> -	if (IS_ERR(cdns->role_sw)) {
>> -		ret = PTR_ERR(cdns->role_sw);
>> -		dev_warn(dev, "Unable to register Role Switch\n");
>> -		goto err3;
>> +	if (device_property_read_bool(cdns->dev, "usb-role-switch")) {
>> +		cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
>> +		if (IS_ERR(cdns->role_sw)) {
>> +			ret = PTR_ERR(cdns->role_sw);
>> +			dev_warn(dev, "Unable to register Role Switch\n");
>> +			goto err3;
>> +		}
>>   	}
>>   	if (cdns->wakeup_irq) {
>>
>>
>>
>> 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

Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Peter Chen]

On 20-11-24 14:22:25, Roger Quadros wrote:
> Peter,
> 
> On 24/11/2020 13:47, Peter Chen wrote:
> > On 20-11-24 12:33:34, Roger Quadros wrote:
> > > > > > 
> > > > > > I am sorry about that. Do you use role switch /sys entry, if you have
> > > > > > used, I prefer using "usb-role-switch" property at dts to judge if SoC
> > > > > > OTG signals or external signals for role switch. If you have not used
> > > > > > it, I prefer only setting cdns->role_sw for role switch use cases.
> > > > > > 
> > > > > 
> > > > > We use both hardware role switch and /sys entries for manually forcing a
> > > > > certain role.
> > > > > 
> > > > > We do not set any "usb-role-switch" property at DTS.
> > > > > 
> > > > > Currently cdns->role_sw is being always set by driver irrespective of any DT
> > > > > property, so this patch is clearly wrong and needs to be reverted.
> > > > > 
> > > > > What do you think?
> > > > > 
> > > > 
> > > > Could you accept below fix?
> > > > 
> > > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > > > index 2e469139769f..fdd52e87a7b2 100644
> > > > --- a/drivers/usb/cdns3/core.c
> > > > +++ b/drivers/usb/cdns3/core.c
> > > > @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> > > >           enum usb_role real_role, current_role;
> > > >           int ret = 0;
> > > > 
> > > > -       /* Depends on role switch class */
> > > > -       if (cdns->role_sw)
> > > > +       /* quit if switch role through external signals */
> > > > +       if (device_property_read_bool(cdns->dev, "usb-role-switch"))
> > > >                   return 0;
> > > > 
> > > >           pm_runtime_get_sync(cdns->dev);
> > > 
> > > Although this will fix the issue I don't think this is making the driver to behave
> > > as expected with usb-role-switch property.
> > > 
> > > Now, even if usb-role-switch property is not present the driver will still register
> > > the role switch driver.
> > > 
> > > I think we need to register the role switch driver only if usb-role-switch property
> > > is present. We would also need to set the default role if role-switch-default-mode is present.
> > > 
> > > How about the following? It still doesn't handle role-switch-default-mode property though.
> > > 
> > 
> > Roger, you said you also use /sys entries (I suppose it means through role
> > switch class) to do role switch, with your change, there will be no /sys
> > entry for role switch.
> 
> Sorry for the confusion. Although we do need both features (SW role switch + HW role switch)
> I don't think it is required to operate simultaneously. If users need SW control they can set the DT flag.
> 

I see. I prefer embracing all things related to role switch under the
firmware entry condition. Besides, I find another issue that devm_request_irq
for wakeup_irq does not call usb_role_switch_unregister if it has
failed. So, probably, two patches are needed. I am OK you send the
patches to fix both.

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 2e469139769f..fc6a8152406c 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -427,7 +427,6 @@ static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
  */
 static int cdns3_probe(struct platform_device *pdev)
 {
-	struct usb_role_switch_desc sw_desc = { };
 	struct device *dev = &pdev->dev;
 	struct resource	*res;
 	struct cdns3 *cdns;
@@ -529,18 +528,21 @@ static int cdns3_probe(struct platform_device *pdev)
 	if (ret)
 		goto err2;
 
-	sw_desc.set = cdns3_role_set;
-	sw_desc.get = cdns3_role_get;
-	sw_desc.allow_userspace_control = true;
-	sw_desc.driver_data = cdns;
-	if (device_property_read_bool(dev, "usb-role-switch"))
+	if (device_property_read_bool(dev, "usb-role-switch")) {
+		struct usb_role_switch_desc sw_desc = { };
+
+		sw_desc.set = cdns3_role_set;
+		sw_desc.get = cdns3_role_get;
+		sw_desc.allow_userspace_control = true;
+		sw_desc.driver_data = cdns;
 		sw_desc.fwnode = dev->fwnode;
 
-	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
-	if (IS_ERR(cdns->role_sw)) {
-		ret = PTR_ERR(cdns->role_sw);
-		dev_warn(dev, "Unable to register Role Switch\n");
-		goto err3;
+		cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
+		if (IS_ERR(cdns->role_sw)) {
+			ret = PTR_ERR(cdns->role_sw);
+			dev_warn(dev, "Unable to register Role Switch\n");
+			goto err3;
+		}
 	}
 
 	if (cdns->wakeup_irq) {
@@ -583,7 +585,8 @@ static int cdns3_probe(struct platform_device *pdev)
 	return 0;
 err4:
 	cdns3_drd_exit(cdns);
-	usb_role_switch_unregister(cdns->role_sw);
+	if (cdns->role_sw)
+		usb_role_switch_unregister(cdns->role_sw);
 err3:
 	set_phy_power_off(cdns);
 err2:

-- 

Thanks,
Peter Chen

Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Roger Quadros]

On 25/11/2020 02:36, Peter Chen wrote:
> On 20-11-24 14:22:25, Roger Quadros wrote:
>> Peter,
>>
>> On 24/11/2020 13:47, Peter Chen wrote:
>>> On 20-11-24 12:33:34, Roger Quadros wrote:
>>>>>>>
>>>>>>> I am sorry about that. Do you use role switch /sys entry, if you have
>>>>>>> used, I prefer using "usb-role-switch" property at dts to judge if SoC
>>>>>>> OTG signals or external signals for role switch. If you have not used
>>>>>>> it, I prefer only setting cdns->role_sw for role switch use cases.
>>>>>>>
>>>>>>
>>>>>> We use both hardware role switch and /sys entries for manually forcing a
>>>>>> certain role.
>>>>>>
>>>>>> We do not set any "usb-role-switch" property at DTS.
>>>>>>
>>>>>> Currently cdns->role_sw is being always set by driver irrespective of any DT
>>>>>> property, so this patch is clearly wrong and needs to be reverted.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>
>>>>> Could you accept below fix?
>>>>>
>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>> index 2e469139769f..fdd52e87a7b2 100644
>>>>> --- a/drivers/usb/cdns3/core.c
>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>> @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
>>>>>            enum usb_role real_role, current_role;
>>>>>            int ret = 0;
>>>>>
>>>>> -       /* Depends on role switch class */
>>>>> -       if (cdns->role_sw)
>>>>> +       /* quit if switch role through external signals */
>>>>> +       if (device_property_read_bool(cdns->dev, "usb-role-switch"))
>>>>>                    return 0;
>>>>>
>>>>>            pm_runtime_get_sync(cdns->dev);
>>>>
>>>> Although this will fix the issue I don't think this is making the driver to behave
>>>> as expected with usb-role-switch property.
>>>>
>>>> Now, even if usb-role-switch property is not present the driver will still register
>>>> the role switch driver.
>>>>
>>>> I think we need to register the role switch driver only if usb-role-switch property
>>>> is present. We would also need to set the default role if role-switch-default-mode is present.
>>>>
>>>> How about the following? It still doesn't handle role-switch-default-mode property though.
>>>>
>>>
>>> Roger, you said you also use /sys entries (I suppose it means through role
>>> switch class) to do role switch, with your change, there will be no /sys
>>> entry for role switch.
>>
>> Sorry for the confusion. Although we do need both features (SW role switch + HW role switch)
>> I don't think it is required to operate simultaneously. If users need SW control they can set the DT flag.
>>
> 
> I see. I prefer embracing all things related to role switch under the
> firmware entry condition. Besides, I find another issue that devm_request_irq
> for wakeup_irq does not call usb_role_switch_unregister if it has
> failed. So, probably, two patches are needed. I am OK you send the
> patches to fix both.

Pawel, can you please confirm that if you are ok with either h/w role switching
or s/w role switching but not both working at the same time?

It would have been nice by user to to know the current role even for h/w based
swithcing but it looks like now that won't be possible.

cheers,
-roger
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 2e469139769f..fc6a8152406c 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -427,7 +427,6 @@ static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
>    */
>   static int cdns3_probe(struct platform_device *pdev)
>   {
> -	struct usb_role_switch_desc sw_desc = { };
>   	struct device *dev = &pdev->dev;
>   	struct resource	*res;
>   	struct cdns3 *cdns;
> @@ -529,18 +528,21 @@ static int cdns3_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto err2;
>   
> -	sw_desc.set = cdns3_role_set;
> -	sw_desc.get = cdns3_role_get;
> -	sw_desc.allow_userspace_control = true;
> -	sw_desc.driver_data = cdns;
> -	if (device_property_read_bool(dev, "usb-role-switch"))
> +	if (device_property_read_bool(dev, "usb-role-switch")) {
> +		struct usb_role_switch_desc sw_desc = { };
> +
> +		sw_desc.set = cdns3_role_set;
> +		sw_desc.get = cdns3_role_get;
> +		sw_desc.allow_userspace_control = true;
> +		sw_desc.driver_data = cdns;
>   		sw_desc.fwnode = dev->fwnode;
>   
> -	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> -	if (IS_ERR(cdns->role_sw)) {
> -		ret = PTR_ERR(cdns->role_sw);
> -		dev_warn(dev, "Unable to register Role Switch\n");
> -		goto err3;
> +		cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> +		if (IS_ERR(cdns->role_sw)) {
> +			ret = PTR_ERR(cdns->role_sw);
> +			dev_warn(dev, "Unable to register Role Switch\n");
> +			goto err3;
> +		}
>   	}
>   
>   	if (cdns->wakeup_irq) {
> @@ -583,7 +585,8 @@ static int cdns3_probe(struct platform_device *pdev)
>   	return 0;
>   err4:
>   	cdns3_drd_exit(cdns);
> -	usb_role_switch_unregister(cdns->role_sw);
> +	if (cdns->role_sw)
> +		usb_role_switch_unregister(cdns->role_sw);
>   err3:
>   	set_phy_power_off(cdns);
>   err2:
> 

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

RE: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"

[Pawel Laszczak]

>
>On 25/11/2020 02:36, Peter Chen wrote:
>> On 20-11-24 14:22:25, Roger Quadros wrote:
>>> Peter,
>>>
>>> On 24/11/2020 13:47, Peter Chen wrote:
>>>> On 20-11-24 12:33:34, Roger Quadros wrote:
>>>>>>>>
>>>>>>>> I am sorry about that. Do you use role switch /sys entry, if you have
>>>>>>>> used, I prefer using "usb-role-switch" property at dts to judge if SoC
>>>>>>>> OTG signals or external signals for role switch. If you have not used
>>>>>>>> it, I prefer only setting cdns->role_sw for role switch use cases.
>>>>>>>>
>>>>>>>
>>>>>>> We use both hardware role switch and /sys entries for manually forcing a
>>>>>>> certain role.
>>>>>>>
>>>>>>> We do not set any "usb-role-switch" property at DTS.
>>>>>>>
>>>>>>> Currently cdns->role_sw is being always set by driver irrespective of any DT
>>>>>>> property, so this patch is clearly wrong and needs to be reverted.
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>
>>>>>> Could you accept below fix?
>>>>>>
>>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>>> index 2e469139769f..fdd52e87a7b2 100644
>>>>>> --- a/drivers/usb/cdns3/core.c
>>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>>> @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
>>>>>>            enum usb_role real_role, current_role;
>>>>>>            int ret = 0;
>>>>>>
>>>>>> -       /* Depends on role switch class */
>>>>>> -       if (cdns->role_sw)
>>>>>> +       /* quit if switch role through external signals */
>>>>>> +       if (device_property_read_bool(cdns->dev, "usb-role-switch"))
>>>>>>                    return 0;
>>>>>>
>>>>>>            pm_runtime_get_sync(cdns->dev);
>>>>>
>>>>> Although this will fix the issue I don't think this is making the driver to behave
>>>>> as expected with usb-role-switch property.
>>>>>
>>>>> Now, even if usb-role-switch property is not present the driver will still register
>>>>> the role switch driver.
>>>>>
>>>>> I think we need to register the role switch driver only if usb-role-switch property
>>>>> is present. We would also need to set the default role if role-switch-default-mode is present.
>>>>>
>>>>> How about the following? It still doesn't handle role-switch-default-mode property though.
>>>>>
>>>>
>>>> Roger, you said you also use /sys entries (I suppose it means through role
>>>> switch class) to do role switch, with your change, there will be no /sys
>>>> entry for role switch.
>>>
>>> Sorry for the confusion. Although we do need both features (SW role switch + HW role switch)
>>> I don't think it is required to operate simultaneously. If users need SW control they can set the DT flag.
>>>
>>
>> I see. I prefer embracing all things related to role switch under the
>> firmware entry condition. Besides, I find another issue that devm_request_irq
>> for wakeup_irq does not call usb_role_switch_unregister if it has
>> failed. So, probably, two patches are needed. I am OK you send the
>> patches to fix both.
>
>Pawel, can you please confirm that if you are ok with either h/w role switching
>or s/w role switching but not both working at the same time?
>
>It would have been nice by user to to know the current role even for h/w based
>swithcing but it looks like now that won't be possible.

For the moment it's ok for me, but form testing point of view it could be nice
to have possibility to easy choose between these two modes in dynamically way.

For PCI based on platform probably I will have to add some parameter to cdns3-pci.ko
module to allow selecting modes. I don't  use DTS. 

Don't worry about it now. You can post this fix. 

Cheers,
Pawel 

>
>cheers,
>-roger
>>
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index 2e469139769f..fc6a8152406c 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -427,7 +427,6 @@ static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
>>    */
>>   static int cdns3_probe(struct platform_device *pdev)
>>   {
>> -	struct usb_role_switch_desc sw_desc = { };
>>   	struct device *dev = &pdev->dev;
>>   	struct resource	*res;
>>   	struct cdns3 *cdns;
>> @@ -529,18 +528,21 @@ static int cdns3_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto err2;
>>
>> -	sw_desc.set = cdns3_role_set;
>> -	sw_desc.get = cdns3_role_get;
>> -	sw_desc.allow_userspace_control = true;
>> -	sw_desc.driver_data = cdns;
>> -	if (device_property_read_bool(dev, "usb-role-switch"))
>> +	if (device_property_read_bool(dev, "usb-role-switch")) {
>> +		struct usb_role_switch_desc sw_desc = { };
>> +
>> +		sw_desc.set = cdns3_role_set;
>> +		sw_desc.get = cdns3_role_get;
>> +		sw_desc.allow_userspace_control = true;
>> +		sw_desc.driver_data = cdns;
>>   		sw_desc.fwnode = dev->fwnode;
>>
>> -	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
>> -	if (IS_ERR(cdns->role_sw)) {
>> -		ret = PTR_ERR(cdns->role_sw);
>> -		dev_warn(dev, "Unable to register Role Switch\n");
>> -		goto err3;
>> +		cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
>> +		if (IS_ERR(cdns->role_sw)) {
>> +			ret = PTR_ERR(cdns->role_sw);
>> +			dev_warn(dev, "Unable to register Role Switch\n");
>> +			goto err3;
>> +		}
>>   	}
>>
>>   	if (cdns->wakeup_irq) {
>> @@ -583,7 +585,8 @@ static int cdns3_probe(struct platform_device *pdev)
>>   	return 0;
>>   err4:
>>   	cdns3_drd_exit(cdns);
>> -	usb_role_switch_unregister(cdns->role_sw);
>> +	if (cdns->role_sw)
>> +		usb_role_switch_unregister(cdns->role_sw);
>>   err3:
>>   	set_phy_power_off(cdns);
>>   err2:
>>
>
>--
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki