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@xxxxxxxxxxx>
> > > ---
> > > 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