| Message ID | 20201123115051.30047-1-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "usb: cdns3: core: quit if it uses role switch class" | expand |
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.
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
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
+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
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,
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
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 >
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:
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: >
> >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
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;
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(-)