| Message ID | 20190116101051.21202-1-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: chipidea: Grab the (legacy) USB PHY by phandle first | expand |
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
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
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
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
> > 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! >
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! > > > >
> 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
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 &&
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(-)