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

[PATCH v3] 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 one 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 are 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>
---
Changes since v2:
* Fixed typos in commit message.

Changes since v1:
* Only consider legacy USB PHY error for fallback as suggested;
* Checked against EPROBE_DEFER before entering fallback.

 drivers/usb/chipidea/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 7bfcbb23c2a4..016e4004fe9d 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -954,8 +954,15 @@ 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->usb_phy) &&
+		    PTR_ERR(ci->usb_phy) != -EPROBE_DEFER)
+			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 v3] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[Peter Chen]

 
> 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 one 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 are 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>
> ---
> Changes since v2:
> * Fixed typos in commit message.
> 
> Changes since v1:
> * Only consider legacy USB PHY error for fallback as suggested;
> * Checked against EPROBE_DEFER before entering fallback.
> 
>  drivers/usb/chipidea/core.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
> 7bfcbb23c2a4..016e4004fe9d 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -954,8 +954,15 @@ 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->usb_phy) &&
> +		    PTR_ERR(ci->usb_phy) != -EPROBE_DEFER)
> +			ci->usb_phy = devm_usb_get_phy(dev->parent,
> +						       USB_PHY_TYPE_USB2);
> 

I think you may need to do above if ci->phy is error, and not the error is -EPROBE_DEFER.

Peter

>  		/* if both generic PHY and USB PHY layers aren't enabled */
>  		if (PTR_ERR(ci->phy) == -ENOSYS &&
> --
> 2.20.1

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

[Paul Kocialkowski]

Hi,

On Mon, 2019-02-18 at 03:04 +0000, Peter Chen 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 one 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 are 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>
> > ---
> > Changes since v2:
> > * Fixed typos in commit message.
> > 
> > Changes since v1:
> > * Only consider legacy USB PHY error for fallback as suggested;
> > * Checked against EPROBE_DEFER before entering fallback.
> > 
> >  drivers/usb/chipidea/core.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
> > 7bfcbb23c2a4..016e4004fe9d 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -954,8 +954,15 @@ 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->usb_phy) &&
> > +		    PTR_ERR(ci->usb_phy) != -EPROBE_DEFER)
> > +			ci->usb_phy = devm_usb_get_phy(dev->parent,
> > +						       USB_PHY_TYPE_USB2);
> > 
> 
> I think you may need to do above if ci->phy is error, and not the error is -EPROBE_DEFER.

As Thomas pointed out during the review of v1, the initial flow is to
try and get both ci->usb_phy and ci->phy and let the code use ci->phy
in priority later.

This change attempts to keep this flow intact. The EPROBE_DEFER check
is in case the initial ci->usb_phy is valid but deferred: we don't want
to overwrite it by calling devm_usb_get_phy which might return an
actual error and result in losing the EPROBE_DEFER information.

Does that make sense to you?

Cheers,

Paul

> Peter
> 
> >  		/* if both generic PHY and USB PHY layers aren't enabled */
> >  		if (PTR_ERR(ci->phy) == -ENOSYS &&
> > --
> > 2.20.1
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Peter Chen]

 
> 
> On Mon, 2019-02-18 at 03:04 +0000, Peter Chen 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 one 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 are 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>
> > > ---
> > > Changes since v2:
> > > * Fixed typos in commit message.
> > >
> > > Changes since v1:
> > > * Only consider legacy USB PHY error for fallback as suggested;
> > > * Checked against EPROBE_DEFER before entering fallback.
> > >
> > >  drivers/usb/chipidea/core.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/chipidea/core.c
> > > b/drivers/usb/chipidea/core.c index 7bfcbb23c2a4..016e4004fe9d
> > > 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -954,8 +954,15 @@ 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->usb_phy) &&
> > > +		    PTR_ERR(ci->usb_phy) != -EPROBE_DEFER)
> > > +			ci->usb_phy = devm_usb_get_phy(dev->parent,
> > > +						       USB_PHY_TYPE_USB2);
> > >
> >
> > I think you may need to do above if ci->phy is error, and not the error is -
> EPROBE_DEFER.
> 
> As Thomas pointed out during the review of v1, the initial flow is to try and get both
> ci->usb_phy and ci->phy and let the code use ci->phy in priority later.
> 

If there is a generic PHY node under USB controller, and there is a USB PHY
at other sides, both ci->phy and ci->usb_phy are valid, I original thought it is
the problem you met.

Since you are trying to fix the legacy USB PHY grab issue, I hope you could consider
the generic PHY as well.

Peter
 

> This change attempts to keep this flow intact. The EPROBE_DEFER check is in
> case the initial ci->usb_phy is valid but deferred: we don't want to overwrite it by
> calling devm_usb_get_phy which might return an actual error and result in losing
> the EPROBE_DEFER information.
> 
> Does that make sense to you?
> 
> Cheers,
> 
> Paul
> 
> > Peter
> >
> > >  		/* if both generic PHY and USB PHY layers aren't enabled */
> > >  		if (PTR_ERR(ci->phy) == -ENOSYS &&
> > > --
> > > 2.20.1
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin.com
> &amp;data=02%7C01%7Cpeter.chen%40nxp.com%7Cadca65357daa4fe678dc08d
> 6957b9c49%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6368607554
> 66805805&amp;sdata=c%2FdLqUhFxe1yz7lrqUoXZCvINiq1rdKJmMAuaH6Fr1k%3
> D&amp;reserved=0

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

[Paul Kocialkowski]

Hi,

On Thu, 2019-02-21 at 03:29 +0000, Peter Chen wrote:
>  
> > On Mon, 2019-02-18 at 03:04 +0000, Peter Chen 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 one 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 are 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>
> > > > ---
> > > > Changes since v2:
> > > > * Fixed typos in commit message.
> > > > 
> > > > Changes since v1:
> > > > * Only consider legacy USB PHY error for fallback as suggested;
> > > > * Checked against EPROBE_DEFER before entering fallback.
> > > > 
> > > >  drivers/usb/chipidea/core.c | 9 ++++++++-
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/chipidea/core.c
> > > > b/drivers/usb/chipidea/core.c index 7bfcbb23c2a4..016e4004fe9d
> > > > 100644
> > > > --- a/drivers/usb/chipidea/core.c
> > > > +++ b/drivers/usb/chipidea/core.c
> > > > @@ -954,8 +954,15 @@ 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->usb_phy) &&
> > > > +		    PTR_ERR(ci->usb_phy) != -EPROBE_DEFER)
> > > > +			ci->usb_phy = devm_usb_get_phy(dev->parent,
> > > > +						       USB_PHY_TYPE_USB2);
> > > > 
> > > 
> > > I think you may need to do above if ci->phy is error, and not the error is -
> > EPROBE_DEFER.
> > 
> > As Thomas pointed out during the review of v1, the initial flow is to try and get both
> > ci->usb_phy and ci->phy and let the code use ci->phy in priority later.
> > 
> 
> If there is a generic PHY node under USB controller, and there is a USB PHY
> at other sides, both ci->phy and ci->usb_phy are valid, I original thought it is
> the problem you met.

Right, this is not the problem we are having. The problem is that
legacy USB PHYs are not grabbed from device-tree. Instead, they are
currently matched with their type in the list of registered USB PHYs,
making it impossible to use 2 distinct USB PHYs this way.

> Since you are trying to fix the legacy USB PHY grab issue, I hope you could consider
> the generic PHY as well.

What do you have in mind about generic PHYs? Everything already works
as expected with them.

Cheers,

Paul

> > This change attempts to keep this flow intact. The EPROBE_DEFER check is in
> > case the initial ci->usb_phy is valid but deferred: we don't want to overwrite it by
> > calling devm_usb_get_phy which might return an actual error and result in losing
> > the EPROBE_DEFER information.
> > 
> > Does that make sense to you?
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > Peter
> > > 
> > > >  		/* if both generic PHY and USB PHY layers aren't enabled */
> > > >  		if (PTR_ERR(ci->phy) == -ENOSYS &&
> > > > --
> > > > 2.20.1
> > --
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin.com
> > &amp;data=02%7C01%7Cpeter.chen%40nxp.com%7Cadca65357daa4fe678dc08d
> > 6957b9c49%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6368607554
> > 66805805&amp;sdata=c%2FdLqUhFxe1yz7lrqUoXZCvINiq1rdKJmMAuaH6Fr1k%3
> > D&amp;reserved=0
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Peter Chen]

 
> > If there is a generic PHY node under USB controller, and there is a
> > USB PHY at other sides, both ci->phy and ci->usb_phy are valid, I
> > original thought it is the problem you met.
> 
> Right, this is not the problem we are having. The problem is that legacy USB PHYs
> are not grabbed from device-tree. Instead, they are currently matched with their
> type in the list of registered USB PHYs, making it impossible to use 2 distinct USB
> PHYs this way.
> 
> > Since you are trying to fix the legacy USB PHY grab issue, I hope you
> > could consider the generic PHY as well.
> 
> What do you have in mind about generic PHYs? Everything already works as
> expected with them.
> 
 
Current code w/o your patch, it is possible both ci->phy and ci->usb_phy are valid
if the USB PHY is not at the device tree, but generic PHY is at the device tree.
If you don't want to fix this issue with this patch, it is ok too. We could fix it later.

Peter

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

[Paul Kocialkowski]

On Thu, 2019-02-21 at 08:37 +0000, Peter Chen wrote:
>  
> > > If there is a generic PHY node under USB controller, and there is a
> > > USB PHY at other sides, both ci->phy and ci->usb_phy are valid, I
> > > original thought it is the problem you met.
> > 
> > Right, this is not the problem we are having. The problem is that legacy USB PHYs
> > are not grabbed from device-tree. Instead, they are currently matched with their
> > type in the list of registered USB PHYs, making it impossible to use 2 distinct USB
> > PHYs this way.
> > 
> > > Since you are trying to fix the legacy USB PHY grab issue, I hope you
> > > could consider the generic PHY as well.
> > 
> > What do you have in mind about generic PHYs? Everything already works as
> > expected with them.
> > 
>  
> Current code w/o your patch, it is possible both ci->phy and ci->usb_phy are valid
> if the USB PHY is not at the device tree, but generic PHY is at the device tree.
> If you don't want to fix this issue with this patch, it is ok too. We could fix it later.

I'm not sure I understand the issue. With my patch, if there is a
generic PHY described in device-tree, then devm_usb_get_phy_by_phandle
for legacy PHY will fail and the code will fallback to
devm_usb_get_phy, which is the same behavior as before.

Is it a problem that we can end up with both a generic and legacy PHY?
I thought this was expected behavior at probe, and the rest of the code
will just use the generic one in priority.

Do you want to make it so that only one (generic or legacy) PHY remains
after probe?

Cheers,

Paul

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

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

[Peter Chen]

 
> > Current code w/o your patch, it is possible both ci->phy and
> > ci->usb_phy are valid if the USB PHY is not at the device tree, but generic PHY is
> at the device tree.
> > If you don't want to fix this issue with this patch, it is ok too. We could fix it later.
> 
> I'm not sure I understand the issue. With my patch, if there is a generic PHY
> described in device-tree, then devm_usb_get_phy_by_phandle for legacy PHY will
> fail and the code will fallback to devm_usb_get_phy, which is the same behavior as
> before.
> 

You are right, but this behavior is incorrect since each controller has only one physical
USB PHY.

> Is it a problem that we can end up with both a generic and legacy PHY?
> I thought this was expected behavior at probe, and the rest of the code will just use
> the generic one in priority.
> 
> Do you want to make it so that only one (generic or legacy) PHY remains after
> probe?
> 
 
Yes, I just want only one valid, either ci->phy or ci->usb_phy, it makes sense.

Peter

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

[Paul Kocialkowski]

Hi,

On Thu, 2019-02-21 at 09:30 +0000, Peter Chen wrote:
>  
> > > Current code w/o your patch, it is possible both ci->phy and
> > > ci->usb_phy are valid if the USB PHY is not at the device tree, but generic PHY is
> > at the device tree.
> > > If you don't want to fix this issue with this patch, it is ok too. We could fix it later.
> > 
> > I'm not sure I understand the issue. With my patch, if there is a generic PHY
> > described in device-tree, then devm_usb_get_phy_by_phandle for legacy PHY will
> > fail and the code will fallback to devm_usb_get_phy, which is the same behavior as
> > before.
> > 
> 
> You are right, but this behavior is incorrect since each controller has only one physical
> USB PHY.
> 
> > Is it a problem that we can end up with both a generic and legacy PHY?
> > I thought this was expected behavior at probe, and the rest of the code will just use
> > the generic one in priority.
> > 
> > Do you want to make it so that only one (generic or legacy) PHY remains after
> > probe?
> > 
>  
> Yes, I just want only one valid, either ci->phy or ci->usb_phy, it makes sense.

Sounds good to me. I'll send out v4 with this patch and an extra one to
refactor the PHY selection path and only attempt to get a single PHY.

Cheers,

Paul

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