| Message ID | 20190215162225.19284-1-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 68ef236274793066b9ba3154b16c0acc1c891e5c |
| Headers | show |
| Series | [v3] usb: chipidea: Grab the (legacy) USB PHY by phandle first | expand |
> 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
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
> > 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 > &data=02%7C01%7Cpeter.chen%40nxp.com%7Cadca65357daa4fe678dc08d > 6957b9c49%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6368607554 > 66805805&sdata=c%2FdLqUhFxe1yz7lrqUoXZCvINiq1rdKJmMAuaH6Fr1k%3 > D&reserved=0
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 > > &data=02%7C01%7Cpeter.chen%40nxp.com%7Cadca65357daa4fe678dc08d > > 6957b9c49%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6368607554 > > 66805805&sdata=c%2FdLqUhFxe1yz7lrqUoXZCvINiq1rdKJmMAuaH6Fr1k%3 > > D&reserved=0
> > 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
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
> > 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
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
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 &&
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(-)