Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <3ed330798d31d01fdfbd40571d016a108c9c5040.camel@bootlin.com>
Subject: Re: [PATCH v3] usb: chipidea: Grab the (legacy) USB PHY by phandle
 first
From:   Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To:     Peter Chen <peter.chen@nxp.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Date:   Thu, 21 Feb 2019 09:22:47 +0100
In-Reply-To: <VI1PR04MB53278AE9AB6C14DA2AB96F6A8B7E0@VI1PR04MB5327.eurprd04.prod.outlook.com>
References: <20190215162225.19284-1-paul.kocialkowski@bootlin.com>
         <VI1PR04MB53274C03CD18C3D3ADB767408B630@VI1PR04MB5327.eurprd04.prod.outlook.com>
         <c80ee02db4a22d15a24281384ae561462593684f.camel@bootlin.com>
         <VI1PR04MB53278AE9AB6C14DA2AB96F6A8B7E0@VI1PR04MB5327.eurprd04.prod.outlook.com>
Organization: Bootlin
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.30.5 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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