[PATCH v4 1/2] usb: chipidea: Grab the (legacy) USB PHY by phandle first

[PATCH v4 1/2] 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 sinve v3:
* Added extra patch to refactor PHY selection and only keep a single one.

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

[PATCH v4 2/2] usb: chipidea: Refactor USB PHY selection and keep a single PHY

[Paul Kocialkowski]

Refactor the code in charge of looking up the USB PHY when no platdata
is provided. Attempt to get a generic USB PHY first, then look for a
legacy USB PHY through device-tree and finally get any registered PHY
with the correct type.

This way, only a single USB PHY is obtained and the flow is easier to
understand and follow.

All error pointers (except for EPROBE_DEFER) are considered as PHY
not found.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/usb/chipidea/core.c | 49 ++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 016e4004fe9d..27749ace2d93 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -954,32 +954,47 @@ 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);
+		/* Look for a generic PHY first */
 		ci->phy = devm_phy_get(dev->parent, "usb-phy");
 
-		/* Fallback to grabbing any registered USB2 PHY */
-		if (IS_ERR(ci->usb_phy) &&
-		    PTR_ERR(ci->usb_phy) != -EPROBE_DEFER)
+		if (PTR_ERR(ci->phy) == -EPROBE_DEFER) {
+			ret = -EPROBE_DEFER;
+			goto ulpi_exit;
+		} else if (IS_ERR(ci->phy)) {
+			ci->phy = NULL;
+		}
+
+		/* Look for a legacy USB PHY from device-tree next */
+		if (!ci->phy) {
+			ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent,
+								  "phys", 0);
+
+			if (PTR_ERR(ci->usb_phy) == -EPROBE_DEFER) {
+				ret = -EPROBE_DEFER;
+				goto ulpi_exit;
+			} else if (IS_ERR(ci->usb_phy)) {
+				ci->usb_phy = NULL;
+			}
+		}
+
+		/* Look for any registered legacy USB PHY as last resort */
+		if (!ci->phy && !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 &&
-				PTR_ERR(ci->usb_phy) == -ENXIO) {
-			ret = -ENXIO;
-			goto ulpi_exit;
+			if (PTR_ERR(ci->usb_phy) == -EPROBE_DEFER) {
+				ret = -EPROBE_DEFER;
+				goto ulpi_exit;
+			} else if (IS_ERR(ci->usb_phy)) {
+				ci->usb_phy = NULL;
+			}
 		}
 
-		if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy)) {
-			ret = -EPROBE_DEFER;
+		/* No USB PHY was found in the end */
+		if (!ci->phy && !ci->usb_phy) {
+			ret = -ENXIO;
 			goto ulpi_exit;
 		}
-
-		if (IS_ERR(ci->phy))
-			ci->phy = NULL;
-		else if (IS_ERR(ci->usb_phy))
-			ci->usb_phy = NULL;
 	}
 
 	ret = ci_usb_phy_init(ci);
-- 
2.20.1

RE: [PATCH v4 2/2] usb: chipidea: Refactor USB PHY selection and keep a single PHY

[Peter Chen]

 
> 
> Refactor the code in charge of looking up the USB PHY when no platdata is
> provided. Attempt to get a generic USB PHY first, then look for a legacy USB PHY
> through device-tree and finally get any registered PHY with the correct type.
> 
> This way, only a single USB PHY is obtained and the flow is easier to understand
> and follow.
> 
> All error pointers (except for EPROBE_DEFER) are considered as PHY not found.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/usb/chipidea/core.c | 49 ++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
> 016e4004fe9d..27749ace2d93 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -954,32 +954,47 @@ 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);
> +		/* Look for a generic PHY first */
>  		ci->phy = devm_phy_get(dev->parent, "usb-phy");
> 
> -		/* Fallback to grabbing any registered USB2 PHY */
> -		if (IS_ERR(ci->usb_phy) &&
> -		    PTR_ERR(ci->usb_phy) != -EPROBE_DEFER)
> +		if (PTR_ERR(ci->phy) == -EPROBE_DEFER) {
> +			ret = -EPROBE_DEFER;
> +			goto ulpi_exit;
> +		} else if (IS_ERR(ci->phy)) {
> +			ci->phy = NULL;
> +		}
> +
> +		/* Look for a legacy USB PHY from device-tree next */
> +		if (!ci->phy) {
> +			ci->usb_phy = devm_usb_get_phy_by_phandle(dev->parent,
> +								  "phys", 0);
> +
> +			if (PTR_ERR(ci->usb_phy) == -EPROBE_DEFER) {
> +				ret = -EPROBE_DEFER;
> +				goto ulpi_exit;
> +			} else if (IS_ERR(ci->usb_phy)) {
> +				ci->usb_phy = NULL;
> +			}
> +		}
> +
> +		/* Look for any registered legacy USB PHY as last resort */
> +		if (!ci->phy && !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 &&
> -				PTR_ERR(ci->usb_phy) == -ENXIO) {
> -			ret = -ENXIO;
> -			goto ulpi_exit;
> +			if (PTR_ERR(ci->usb_phy) == -EPROBE_DEFER) {
> +				ret = -EPROBE_DEFER;
> +				goto ulpi_exit;
> +			} else if (IS_ERR(ci->usb_phy)) {
> +				ci->usb_phy = NULL;
> +			}
>  		}
> 
> -		if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy)) {
> -			ret = -EPROBE_DEFER;
> +		/* No USB PHY was found in the end */
> +		if (!ci->phy && !ci->usb_phy) {
> +			ret = -ENXIO;
>  			goto ulpi_exit;
>  		}
> -
> -		if (IS_ERR(ci->phy))
> -			ci->phy = NULL;
> -		else if (IS_ERR(ci->usb_phy))
> -			ci->usb_phy = NULL;
>  	}
> 
 
This patch should work, I will queue it if test is ok.

Peter