[PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Paul Kocialkowski]

According to the chipidea driver bindings, the USB PHY is specified via
the "phys" phandle node. However, this only takes effect for USB PHYs
that use the common PHY framework. For legacy USB PHYs, a simple lookup
based on the USB PHY type is done instead.

This does not play out well when more than USB PHY is registered, since
the first registered PHY matching the type will always be returned
regardless of what the driver was bound to.

Fix this by looking up the PHY based on the "phys" phandle node.
Although generic PHYS and rather matched by their "phys-name" and not
the "phys" phandle directly, there is no helper for similar lookup on
legacy PHYs and it's probably not worth the effort to add it.

When no legacy USB PHY is found by phandle, fallback to grabbing any
registered USB2 PHY. This ensures backward compatibility if some users
were actually relying on this mechanism.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/usb/chipidea/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 7bfcbb23c2a4..11d3ee1e3fe5 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -954,8 +954,14 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 	} else if (ci->platdata->usb_phy) {
 		ci->usb_phy = ci->platdata->usb_phy;
 	} else {
+		ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys",
+							  0);
 		ci->phy = devm_phy_get(dev->parent, "usb-phy");
-		ci->usb_phy = devm_usb_get_phy(dev->parent, USB_PHY_TYPE_USB2);
+
+		/* Fallback to grabbing any registered USB2 PHY */
+		if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
+			ci->usb_phy = devm_usb_get_phy(dev->parent,
+						       USB_PHY_TYPE_USB2);
 
 		/* if both generic PHY and USB PHY layers aren't enabled */
 		if (PTR_ERR(ci->phy) == -ENOSYS &&
-- 
2.20.1

Re: [PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Thomas Petazzoni]

Hello,

Thanks for the patch!

On Wed, 16 Jan 2019 11:10:51 +0100, Paul Kocialkowski wrote:
> According to the chipidea driver bindings, the USB PHY is specified via
> the "phys" phandle node. However, this only takes effect for USB PHYs
> that use the common PHY framework. For legacy USB PHYs, a simple lookup
> based on the USB PHY type is done instead.
> 
> This does not play out well when more than USB PHY is registered, since

"more than *one*"

> the first registered PHY matching the type will always be returned
> regardless of what the driver was bound to.
> 
> Fix this by looking up the PHY based on the "phys" phandle node.
> Although generic PHYS and rather matched by their "phys-name"

I'm confused by "Although generic PHYS and rather matched". Perhaps
s/and/are/ ?

Also PHYS -> PHYs

> and not
> the "phys" phandle directly, there is no helper for similar lookup on
> legacy PHYs and it's probably not worth the effort to add it.
> 
> When no legacy USB PHY is found by phandle, fallback to grabbing any
> registered USB2 PHY. This ensures backward compatibility if some users
> were actually relying on this mechanism.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/usb/chipidea/core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 7bfcbb23c2a4..11d3ee1e3fe5 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -954,8 +954,14 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  	} else if (ci->platdata->usb_phy) {
>  		ci->usb_phy = ci->platdata->usb_phy;
>  	} else {
> +		ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys",
> +							  0);
>  		ci->phy = devm_phy_get(dev->parent, "usb-phy");
> -		ci->usb_phy = devm_usb_get_phy(dev->parent, USB_PHY_TYPE_USB2);

I'm not sure why you change the order of legacy PHY lookup vs. generic
PHY lookup.

> +
> +		/* Fallback to grabbing any registered USB2 PHY */
> +		if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
> +			ci->usb_phy = devm_usb_get_phy(dev->parent,
> +						       USB_PHY_TYPE_USB2);

Why is this conditional on the generic PHY lookup failing?

Don't we simply want:

	ci->phy = devm_phy_get(dev->parent, "usb-phy");
	ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys", 0);
	if (IS_ERR(ci->usb_phy))
		ci->usb_phy = devm_usb_get_phy(dev->parent, USB_PHY_TYPE_USB2);

 ?

Does this needs a "Fixes:" tag ? It's not fixing a regression because
nobody complained until now, but it's really fixing a behavior that
wasn't correct.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Paul Kocialkowski]

Hi,

On Wed, 2019-01-16 at 11:53 +0100, Thomas Petazzoni wrote:
> Hello,
> 
> Thanks for the patch!

And thanks for the review!

> On Wed, 16 Jan 2019 11:10:51 +0100, Paul Kocialkowski wrote:
> > According to the chipidea driver bindings, the USB PHY is specified via
> > the "phys" phandle node. However, this only takes effect for USB PHYs
> > that use the common PHY framework. For legacy USB PHYs, a simple lookup
> > based on the USB PHY type is done instead.
> > 
> > This does not play out well when more than USB PHY is registered, since
> 
> "more than *one*"
> 
> > the first registered PHY matching the type will always be returned
> > regardless of what the driver was bound to.
> > 
> > Fix this by looking up the PHY based on the "phys" phandle node.
> > Although generic PHYS and rather matched by their "phys-name"
> 
> I'm confused by "Although generic PHYS and rather matched". Perhaps
> s/and/are/ ?
> 
> Also PHYS -> PHYs

Good catches, will be fixed in v2.

> > and not
> > the "phys" phandle directly, there is no helper for similar lookup on
> > legacy PHYs and it's probably not worth the effort to add it.
> > 
> > When no legacy USB PHY is found by phandle, fallback to grabbing any
> > registered USB2 PHY. This ensures backward compatibility if some users
> > were actually relying on this mechanism.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/usb/chipidea/core.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 7bfcbb23c2a4..11d3ee1e3fe5 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -954,8 +954,14 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> >  	} else if (ci->platdata->usb_phy) {
> >  		ci->usb_phy = ci->platdata->usb_phy;
> >  	} else {
> > +		ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys",
> > +							  0);
> >  		ci->phy = devm_phy_get(dev->parent, "usb-phy");
> > -		ci->usb_phy = devm_usb_get_phy(dev->parent, USB_PHY_TYPE_USB2);
> 
> I'm not sure why you change the order of legacy PHY lookup vs. generic
> PHY lookup.

You're right, there is no particular reason to do that.

> > +
> > +		/* Fallback to grabbing any registered USB2 PHY */
> > +		if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
> > +			ci->usb_phy = devm_usb_get_phy(dev->parent,
> > +						       USB_PHY_TYPE_USB2);
> 
> Why is this conditional on the generic PHY lookup failing?
> 
> Don't we simply want:
> 
> 	ci->phy = devm_phy_get(dev->parent, "usb-phy");
> 	ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys", 0);
> 	if (IS_ERR(ci->usb_phy))
> 		ci->usb_phy = devm_usb_get_phy(dev->parent, USB_PHY_TYPE_USB2);
> 
>  ?

Well, the code dealing with the PHY later on will use ci->phy over ci-
>usb_phy (so generic PHY API first). As a result, if the
devm_usb_get_phy_by_phandle lookup fails but we got a generic PHY, the
latter will be used and there is no need for a fallback. That's why I
put both conditions there. Maybe that's too much of an assumption?

> Does this needs a "Fixes:" tag ? It's not fixing a regression because
> nobody complained until now, but it's really fixing a behavior that
> wasn't correct.

Yes I it this makes sense to consider that this was incorrect behavior
starting from the moment the dt bindings were formalized for the
driver, which would be commit d7d30c911dd957e274c3da6910d4286862ab1d78.

Do you think that would nake sense?

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Thomas Petazzoni]

Hello,

On Wed, 16 Jan 2019 14:30:28 +0100, Paul Kocialkowski wrote:

> > Why is this conditional on the generic PHY lookup failing?
> > 
> > Don't we simply want:
> > 
> > 	ci->phy = devm_phy_get(dev->parent, "usb-phy");
> > 	ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys", 0);
> > 	if (IS_ERR(ci->usb_phy))
> > 		ci->usb_phy = devm_usb_get_phy(dev->parent, USB_PHY_TYPE_USB2);
> > 
> >  ?  
> 
> Well, the code dealing with the PHY later on will use ci->phy over ci-
> >usb_phy (so generic PHY API first). As a result, if the  
> devm_usb_get_phy_by_phandle lookup fails but we got a generic PHY, the
> latter will be used and there is no need for a fallback. That's why I
> put both conditions there. Maybe that's too much of an assumption?

Well prior to your code, there was already a possibility for both
ci->phy and ci->usb_phy to be valid. I don't think it's really useful
to avoid the fallback when a generic PHY has already been found, it's
confusing. If really you want to clarify that, it should be:

	/* Let's first try to find a generic PHY */
	ci->phy = devm_phy_get(dev->parent, "usb-phy");
	if (IS_ERR(ci->phy)) {
		/* Fall back to legacy USB PHY */
		ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys", 0);
		if (IS_ERR(ci->usb_phy))
			ci->usb_phy = devm_usb_get_phy(dev->parent, USB_PHY_TYPE_USB2);
	}

With that, you would only have either ci->phy or ci->usb_phy be valid,
and never both. With  your change, you can have ci->phy and ci->usb_phy
both be valid if the legacy USB PHY was found using
devm_usb_get_phy_by_phandle(), but not if we fell back to
devm_usb_get_phy().

> > Does this needs a "Fixes:" tag ? It's not fixing a regression because
> > nobody complained until now, but it's really fixing a behavior that
> > wasn't correct.  
> 
> Yes I it this makes sense to consider that this was incorrect behavior
> starting from the moment the dt bindings were formalized for the
> driver, which would be commit d7d30c911dd957e274c3da6910d4286862ab1d78.
> 
> Do you think that would nake sense?

Up to the maintainer I'd say. I don't have any preference here.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Paul Kocialkowski]

Hi,

On Wed, 2019-01-16 at 14:44 +0100, Thomas Petazzoni wrote:
> Well prior to your code, there was already a possibility for both
> ci->phy and ci->usb_phy to be valid. I don't think it's really useful
> to avoid the fallback when a generic PHY has already been found, it's
> confusing. If really you want to clarify that, it should be:
> 
> 	/* Let's first try to find a generic PHY */
> 	ci->phy = devm_phy_get(dev->parent, "usb-phy");
> 	if (IS_ERR(ci->phy)) {
> 		/* Fall back to legacy USB PHY */
> 		ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys", 0);
> 		if (IS_ERR(ci->usb_phy))
> 			ci->usb_phy = devm_usb_get_phy(dev->parent, USB_PHY_TYPE_USB2);
> 	}
> 
> With that, you would only have either ci->phy or ci->usb_phy be valid,
> and never both. With  your change, you can have ci->phy and ci->usb_phy
> both be valid if the legacy USB PHY was found using
> devm_usb_get_phy_by_phandle(), but not if we fell back to
> devm_usb_get_phy().

Okay that makes sense, your suggestion is indeed more consistent with
the existing behavior. I'll go with that in the next revision!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

RE: [PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Peter Chen]

 
> 
> On Wed, 2019-01-16 at 14:44 +0100, Thomas Petazzoni wrote:
> > Well prior to your code, there was already a possibility for both
> > ci->phy and ci->usb_phy to be valid. I don't think it's really useful
> > to avoid the fallback when a generic PHY has already been found, it's
> > confusing. If really you want to clarify that, it should be:
> >
> > 	/* Let's first try to find a generic PHY */
> > 	ci->phy = devm_phy_get(dev->parent, "usb-phy");
> > 	if (IS_ERR(ci->phy)) {

Please consider -EPROBE_DEFER case

> > 		/* Fall back to legacy USB PHY */
> > 		ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys",
> 0);

Please consider -EPROBE_DEFER case

> > 		if (IS_ERR(ci->usb_phy))
> > 			ci->usb_phy = devm_usb_get_phy(dev->parent,
> USB_PHY_TYPE_USB2);
> > 	}
> >

Below code 
                if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy)) {
                        ret = -EPROBE_DEFER;
                        goto ulpi_exit;
                }    

needs to change as:

                if (PTR_ERR(ci->phy) ==-EPROBE_DEFER  ||  PTR_ERR(ci->usb_phy) == -EPROBE_DEFER) {
                        ret = -EPROBE_DEFER;
                        goto ulpi_exit;
                }    

Peter

> > With that, you would only have either ci->phy or ci->usb_phy be valid,
> > and never both. With  your change, you can have ci->phy and
> > ci->usb_phy both be valid if the legacy USB PHY was found using
> > devm_usb_get_phy_by_phandle(), but not if we fell back to
> > devm_usb_get_phy().
> 
> Okay that makes sense, your suggestion is indeed more consistent with the existing
> behavior. I'll go with that in the next revision!
> 

Re: [PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Paul Kocialkowski]

Hi,

On Thu, 2019-01-17 at 06:44 +0000, Peter Chen wrote:
>  
> > On Wed, 2019-01-16 at 14:44 +0100, Thomas Petazzoni wrote:
> > > Well prior to your code, there was already a possibility for both
> > > ci->phy and ci->usb_phy to be valid. I don't think it's really useful
> > > to avoid the fallback when a generic PHY has already been found, it's
> > > confusing. If really you want to clarify that, it should be:
> > > 
> > > 	/* Let's first try to find a generic PHY */
> > > 	ci->phy = devm_phy_get(dev->parent, "usb-phy");
> > > 	if (IS_ERR(ci->phy)) {
> 
> Please consider -EPROBE_DEFER case
> 
> > > 		/* Fall back to legacy USB PHY */
> > > 		ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys",
> > 0);
> 
> Please consider -EPROBE_DEFER case
> 
> > > 		if (IS_ERR(ci->usb_phy))
> > > 			ci->usb_phy = devm_usb_get_phy(dev->parent,
> > USB_PHY_TYPE_USB2);
> > > 	}
> > > 
> 
> Below code 
>                 if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy)) {
>                         ret = -EPROBE_DEFER;
>                         goto ulpi_exit;
>                 }    
> 
> needs to change as:
> 
>                 if (PTR_ERR(ci->phy) ==-EPROBE_DEFER  ||  PTR_ERR(ci->usb_phy) == -EPROBE_DEFER) {
>                         ret = -EPROBE_DEFER;
>                         goto ulpi_exit;
>                 }    

Well I think this was more of a general outline than proper code to be
included in the driver anyway :)

I'm rather considering implementing what Thomas suggested at first,
which is going for the fallback even if a generic PHY was found, as to
stick more closely to the existing behavior.

Cheers,

Paul

> Peter
> 
> > > With that, you would only have either ci->phy or ci->usb_phy be valid,
> > > and never both. With  your change, you can have ci->phy and
> > > ci->usb_phy both be valid if the legacy USB PHY was found using
> > > devm_usb_get_phy_by_phandle(), but not if we fell back to
> > > devm_usb_get_phy().
> > 
> > Okay that makes sense, your suggestion is indeed more consistent with the existing
> > behavior. I'll go with that in the next revision!
> > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

RE: [PATCH] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Peter Chen]

 
> On Thu, 2019-01-17 at 06:44 +0000, Peter Chen wrote:
> >
> > > On Wed, 2019-01-16 at 14:44 +0100, Thomas Petazzoni wrote:
> > > > Well prior to your code, there was already a possibility for both
> > > > ci->phy and ci->usb_phy to be valid. I don't think it's really
> > > > ci->useful
> > > > to avoid the fallback when a generic PHY has already been found,
> > > > it's confusing. If really you want to clarify that, it should be:
> > > >
> > > > 	/* Let's first try to find a generic PHY */
> > > > 	ci->phy = devm_phy_get(dev->parent, "usb-phy");
> > > > 	if (IS_ERR(ci->phy)) {
> >
> > Please consider -EPROBE_DEFER case
> >
> > > > 		/* Fall back to legacy USB PHY */
> > > > 		ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent, "phys",
> > > 0);
> >
> > Please consider -EPROBE_DEFER case
> >
> > > > 		if (IS_ERR(ci->usb_phy))
> > > > 			ci->usb_phy = devm_usb_get_phy(dev->parent,
> > > USB_PHY_TYPE_USB2);
> > > > 	}
> > > >
> >
> > Below code
> >                 if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy)) {
> >                         ret = -EPROBE_DEFER;
> >                         goto ulpi_exit;
> >                 }
> >
> > needs to change as:
> >
> >                 if (PTR_ERR(ci->phy) ==-EPROBE_DEFER  ||  PTR_ERR(ci->usb_phy)
> == -EPROBE_DEFER) {
> >                         ret = -EPROBE_DEFER;
> >                         goto ulpi_exit;
> >                 }
> 
> Well I think this was more of a general outline than proper code to be included in
> the driver anyway :)
> 
> I'm rather considering implementing what Thomas suggested at first, which is going
> for the fallback even if a generic PHY was found, as to stick more closely to the
> existing behavior.
> 

Since one USB2 controller is impossible has two USB PHYs, if both PHYs (generic PHY
and USB PHY) are found, there must be error at code or dts. Besides, -EPROBE_DEFER
is not an real error, it means the PHY(USB-PHY) is existed, just not be ready.

Peter

> Cheers,
> 
> Paul
> 
> > Peter
> >
> > > > With that, you would only have either ci->phy or ci->usb_phy be
> > > > valid, and never both. With  your change, you can have ci->phy and
> > > > ci->usb_phy both be valid if the legacy USB PHY was found using
> > > > devm_usb_get_phy_by_phandle(), but not if we fell back to
> > > > devm_usb_get_phy().
> > >
> > > Okay that makes sense, your suggestion is indeed more consistent
> > > with the existing behavior. I'll go with that in the next revision!
> > >
> >
> >
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin.com
> &data=02%7C01%7Cpeter.chen%40nxp.com%7C30302a1c0b414965af2608d
> 67c9a30f6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6368333990
> 21182420&sdata=lD7kt%2F%2FmUWp7FfLD9iMlxjfvS4ediv4uA%2BForbDSW
> kE%3D&reserved=0