[PATCH 0/2] usb: dwc2: fix phy dependecy

[PATCH 0/2] usb: dwc2: fix phy dependecy

[Daniel Brát]

I had problems getting USB working on my Raspberry Pi 3B+. I tracked
the problem down to the dwc2 driver failing in its probe function
due to CONFIG_GENERIC_PHY not enabled. I fixed the phy dependency
in dwc2 and also enabled phy core in rpi_defconfig.

Daniel Brát (2):
  usb: dwc2: Make having a phy optional
  ARM: rpi_defconfig: Enable PHY core for dwc2 usb driver

 arch/arm/configs/rpi_defconfig |  2 ++
 drivers/usb/dwc2/dwc2.c        | 38 ++++++++++++++++++++--------------
 2 files changed, 24 insertions(+), 16 deletions(-)

-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] usb: dwc2: Make having a phy optional

[Daniel Brát]

Despite using 'phy_optional_get' to get a 'usb2_phy' inside its probe,
the driver would fail if it didnt find it. Also, there is no Kconfig
dependency on 'CONFIG_GENERIC_PHY', so without it defined te phy
functions are unimplemented and always return -ENOSYS making dwc2 probe
always fail too. Since the 'dwc2->phy' is optional, the driver was
modified to reflect this.
Tested working on Raspberry Pi 3B+.

Signed-off-by: Daniel Brát <danek.brat@gmail.com>
---
 drivers/usb/dwc2/dwc2.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/dwc2/dwc2.c b/drivers/usb/dwc2/dwc2.c
index 9893977b8..8f3f8da00 100644
--- a/drivers/usb/dwc2/dwc2.c
+++ b/drivers/usb/dwc2/dwc2.c
@@ -72,20 +72,22 @@ static int dwc2_probe(struct device_d *dev)
 	if (ret)
 		goto clk_disable;
 
-	dwc2->phy = phy_optional_get(dev, "usb2-phy");
-	if (IS_ERR(dwc2->phy)) {
-		ret = PTR_ERR(dwc2->phy);
-		goto clk_disable;
+	if (IS_ENABLED(CONFIG_GENERIC_PHY)) {
+		dwc2->phy = phy_optional_get(dev, "usb2-phy");
+		if (!dwc2->phy || IS_ERR(dwc2->phy)) {
+			dev_warn(dev, "Didnt find any 'usb2-phy'\n");
+			dwc2->phy = NULL;
+		} else {
+			ret = phy_power_on(dwc2->phy);
+			if (ret)
+				goto clk_disable;
+
+			ret = phy_init(dwc2->phy);
+			if (ret)
+				goto phy_power_off;
+		}
 	}
 
-	ret = phy_power_on(dwc2->phy);
-	if (ret)
-		goto clk_disable;
-
-	ret = phy_init(dwc2->phy);
-	if (ret)
-		goto phy_power_off;
-
 	ret = dwc2_check_core_version(dwc2);
 	if (ret)
 		goto error;
@@ -120,9 +122,11 @@ static int dwc2_probe(struct device_d *dev)
 
 	return 0;
 error:
-	phy_exit(dwc2->phy);
+	if (dwc2->phy)
+		phy_exit(dwc2->phy);
 phy_power_off:
-	phy_power_off(dwc2->phy);
+	if (dwc2->phy)
+		phy_power_off(dwc2->phy);
 clk_disable:
 	clk_disable(dwc2->clk);
 clk_put:
@@ -140,8 +144,10 @@ static void dwc2_remove(struct device_d *dev)
 	dwc2_host_uninit(dwc2);
 	dwc2_gadget_uninit(dwc2);
 
-	phy_exit(dwc2->phy);
-	phy_power_off(dwc2->phy);
+	if (dwc2->phy) {
+		phy_exit(dwc2->phy);
+		phy_power_off(dwc2->phy);
+	}
 }
 
 static const struct of_device_id dwc2_platform_dt_ids[] = {
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] usb: dwc2: Make having a phy optional

[Jules Maselbas]

Hi Daniel,

On Wed, Sep 08, 2021 at 09:46:21AM +0200, Daniel Brát wrote:
> Despite using 'phy_optional_get' to get a 'usb2_phy' inside its probe,
> the driver would fail if it didnt find it. Also, there is no Kconfig
> dependency on 'CONFIG_GENERIC_PHY', so without it defined te phy
> functions are unimplemented and always return -ENOSYS making dwc2 probe
> always fail too. Since the 'dwc2->phy' is optional, the driver was
> modified to reflect this.
> Tested working on Raspberry Pi 3B+.
> 
> Signed-off-by: Daniel Brát <danek.brat@gmail.com>
> ---
>  drivers/usb/dwc2/dwc2.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/dwc2.c b/drivers/usb/dwc2/dwc2.c
> index 9893977b8..8f3f8da00 100644
> --- a/drivers/usb/dwc2/dwc2.c
> +++ b/drivers/usb/dwc2/dwc2.c
> @@ -72,20 +72,22 @@ static int dwc2_probe(struct device_d *dev)
>  	if (ret)
>  		goto clk_disable;
>  
> -	dwc2->phy = phy_optional_get(dev, "usb2-phy");
> -	if (IS_ERR(dwc2->phy)) {
> -		ret = PTR_ERR(dwc2->phy);
> -		goto clk_disable;
> +	if (IS_ENABLED(CONFIG_GENERIC_PHY)) {
> +		dwc2->phy = phy_optional_get(dev, "usb2-phy");
> +		if (!dwc2->phy || IS_ERR(dwc2->phy)) {
I think a better way to fix this would be have phy_optional_get to
return NULL when CONFIG_GENERIC_PHY is not enabled. That way this
fix is not limited to dwc2 driver.

NULL is an expected return value for phy_optional_get when there is
no phy, so that must not cause any issue in the driver.

> +			dev_warn(dev, "Didnt find any 'usb2-phy'\n");
> +			dwc2->phy = NULL;
> +		} else {
> +			ret = phy_power_on(dwc2->phy);
> +			if (ret)
> +				goto clk_disable;
> +
> +			ret = phy_init(dwc2->phy);
> +			if (ret)
> +				goto phy_power_off;
> +		}
>  	}
>  
> -	ret = phy_power_on(dwc2->phy);
> -	if (ret)
> -		goto clk_disable;
> -
> -	ret = phy_init(dwc2->phy);
> -	if (ret)
> -		goto phy_power_off;
> -
>  	ret = dwc2_check_core_version(dwc2);
>  	if (ret)
>  		goto error;
> @@ -120,9 +122,11 @@ static int dwc2_probe(struct device_d *dev)
>  
>  	return 0;
>  error:
> -	phy_exit(dwc2->phy);
> +	if (dwc2->phy)
> +		phy_exit(dwc2->phy);
This is not needed as phy_exit already tests for NULL phy.

>  phy_power_off:
> -	phy_power_off(dwc2->phy);
> +	if (dwc2->phy)
> +		phy_power_off(dwc2->phy);
Same for phy_power_off

>  clk_disable:
>  	clk_disable(dwc2->clk);
>  clk_put:
> @@ -140,8 +144,10 @@ static void dwc2_remove(struct device_d *dev)
>  	dwc2_host_uninit(dwc2);
>  	dwc2_gadget_uninit(dwc2);
>  
> -	phy_exit(dwc2->phy);
> -	phy_power_off(dwc2->phy);
> +	if (dwc2->phy) {
> +		phy_exit(dwc2->phy);
> +		phy_power_off(dwc2->phy);
> +	}
Same as above

>  }
>  
>  static const struct of_device_id dwc2_platform_dt_ids[] = {
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 
> 
> To declare a filtering error, please use the following link : https://www.security-mail.net/reporter.php?mid=6017.61386e3c.c78.0&r=jmaselbas%40kalray.eu&s=barebox-bounces%2Bjmaselbas%3Dkalray.eu%40lists.infradead.org&o=%5BPATCH+1%2F2%5D+usb%3A+dwc2%3A+Make+having+a+phy+optional&verdict=C&c=a2cb297eefa99425f437bd1e182a2eb5ba1e58f8
> 





_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2 v2] usb: dwc2: Make having a phy optional

[Daniel Brát]

Despite using 'phy_optional_get' to get a 'usb2-phy' inside its probe,
the driver would fail if it didnt find it. Also, there is no Kconfig
dependency on 'CONFIG_GENERIC_PHY', so without it defined the phy
functions are unimplemented and always return -ENOSYS making dwc2 probe
always fail too.
The header stub of 'phy_optional_get' was changed to return NULL and
the dwc2 driver was modified accordingly.

Signed-off-by: Daniel Brát <danek.brat@gmail.com>
---
 drivers/usb/dwc2/dwc2.c | 27 +++++++++++++++------------
 include/linux/phy/phy.h |  2 +-
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/dwc2/dwc2.c b/drivers/usb/dwc2/dwc2.c
index 9893977b8..472a0b7d9 100644
--- a/drivers/usb/dwc2/dwc2.c
+++ b/drivers/usb/dwc2/dwc2.c
@@ -73,19 +73,22 @@ static int dwc2_probe(struct device_d *dev)
 		goto clk_disable;
 
 	dwc2->phy = phy_optional_get(dev, "usb2-phy");
-	if (IS_ERR(dwc2->phy)) {
-		ret = PTR_ERR(dwc2->phy);
-		goto clk_disable;
+	if (dwc2->phy) {
+		if (IS_ERR(dwc2->phy)) {
+			dev_warn(dev, "Couldnt get 'usb2-phy': %s\n",
+					strerror(PTR_ERR(dwc2->phy)));
+			goto phy_bypass;
+		}
+
+		ret = phy_power_on(dwc2->phy);
+		if (ret)
+			goto clk_disable;
+
+		ret = phy_init(dwc2->phy);
+		if (ret)
+			goto phy_power_off;
 	}
-
-	ret = phy_power_on(dwc2->phy);
-	if (ret)
-		goto clk_disable;
-
-	ret = phy_init(dwc2->phy);
-	if (ret)
-		goto phy_power_off;
-
+phy_bypass:
 	ret = dwc2_check_core_version(dwc2);
 	if (ret)
 		goto error;
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 679ce6e42..321e546f9 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -195,7 +195,7 @@ static inline struct phy *phy_get(struct device_d *dev, const char *string)
 static inline struct phy *phy_optional_get(struct device_d *dev,
 					   const char *string)
 {
-	return ERR_PTR(-ENOSYS);
+	return NULL;
 }
 
 static inline struct phy *of_phy_get_by_phandle(struct device_d *dev,
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2 v2] usb: dwc2: Make having a phy optional

[Sascha Hauer]

Hi Daniel,

On Wed, Sep 08, 2021 at 02:58:53PM +0200, Daniel Brát wrote:
> Despite using 'phy_optional_get' to get a 'usb2-phy' inside its probe,
> the driver would fail if it didnt find it. Also, there is no Kconfig
> dependency on 'CONFIG_GENERIC_PHY', so without it defined the phy
> functions are unimplemented and always return -ENOSYS making dwc2 probe
> always fail too.
> The header stub of 'phy_optional_get' was changed to return NULL and
> the dwc2 driver was modified accordingly.
> 
> Signed-off-by: Daniel Brát <danek.brat@gmail.com>
> ---
>  drivers/usb/dwc2/dwc2.c | 27 +++++++++++++++------------
>  include/linux/phy/phy.h |  2 +-
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/dwc2.c b/drivers/usb/dwc2/dwc2.c
> index 9893977b8..472a0b7d9 100644
> --- a/drivers/usb/dwc2/dwc2.c
> +++ b/drivers/usb/dwc2/dwc2.c
> @@ -73,19 +73,22 @@ static int dwc2_probe(struct device_d *dev)
>  		goto clk_disable;
>  
>  	dwc2->phy = phy_optional_get(dev, "usb2-phy");
> -	if (IS_ERR(dwc2->phy)) {
> -		ret = PTR_ERR(dwc2->phy);
> -		goto clk_disable;
> +	if (dwc2->phy) {
> +		if (IS_ERR(dwc2->phy)) {
> +			dev_warn(dev, "Couldnt get 'usb2-phy': %s\n",
> +					strerror(PTR_ERR(dwc2->phy)));
> +			goto phy_bypass;
> +		}

You changed the return value of phy_optional_get() to NULL below, so
when phy_optional_get() returns an error we would still have to fail the
probe function here instead of ignoring the error, no?

Sascha

> +
> +		ret = phy_power_on(dwc2->phy);
> +		if (ret)
> +			goto clk_disable;
> +
> +		ret = phy_init(dwc2->phy);
> +		if (ret)
> +			goto phy_power_off;
>  	}
> -
> -	ret = phy_power_on(dwc2->phy);
> -	if (ret)
> -		goto clk_disable;
> -
> -	ret = phy_init(dwc2->phy);
> -	if (ret)
> -		goto phy_power_off;
> -
> +phy_bypass:
>  	ret = dwc2_check_core_version(dwc2);
>  	if (ret)
>  		goto error;
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 679ce6e42..321e546f9 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -195,7 +195,7 @@ static inline struct phy *phy_get(struct device_d *dev, const char *string)
>  static inline struct phy *phy_optional_get(struct device_d *dev,
>  					   const char *string)
>  {
> -	return ERR_PTR(-ENOSYS);
> +	return NULL;
>  }
>  
>  static inline struct phy *of_phy_get_by_phandle(struct device_d *dev,
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2 v2] usb: dwc2: Make having a phy optional

[Jules Maselbas]

Hi Daniel,

On Wed, Sep 08, 2021 at 02:58:53PM +0200, Daniel Brát wrote:
> Despite using 'phy_optional_get' to get a 'usb2-phy' inside its probe,
> the driver would fail if it didnt find it. Also, there is no Kconfig
> dependency on 'CONFIG_GENERIC_PHY', so without it defined the phy
> functions are unimplemented and always return -ENOSYS making dwc2 probe
> always fail too.
> The header stub of 'phy_optional_get' was changed to return NULL and
> the dwc2 driver was modified accordingly.
> 
> Signed-off-by: Daniel Brát <danek.brat@gmail.com>
> ---
>  drivers/usb/dwc2/dwc2.c | 27 +++++++++++++++------------
>  include/linux/phy/phy.h |  2 +-
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/dwc2.c b/drivers/usb/dwc2/dwc2.c
> index 9893977b8..472a0b7d9 100644
> --- a/drivers/usb/dwc2/dwc2.c
> +++ b/drivers/usb/dwc2/dwc2.c
> @@ -73,19 +73,22 @@ static int dwc2_probe(struct device_d *dev)
>  		goto clk_disable;
>  
>  	dwc2->phy = phy_optional_get(dev, "usb2-phy");
> -	if (IS_ERR(dwc2->phy)) {
> -		ret = PTR_ERR(dwc2->phy);
> -		goto clk_disable;
> +	if (dwc2->phy) {
> +		if (IS_ERR(dwc2->phy)) {
> +			dev_warn(dev, "Couldnt get 'usb2-phy': %s\n",
> +					strerror(PTR_ERR(dwc2->phy)));
> +			goto phy_bypass;
> +		}
> +
> +		ret = phy_power_on(dwc2->phy);
> +		if (ret)
> +			goto clk_disable;
> +
> +		ret = phy_init(dwc2->phy);
> +		if (ret)
> +			goto phy_power_off;
>  	}
> -
> -	ret = phy_power_on(dwc2->phy);
> -	if (ret)
> -		goto clk_disable;
> -
> -	ret = phy_init(dwc2->phy);
> -	if (ret)
> -		goto phy_power_off;
> -
> +phy_bypass:
>  	ret = dwc2_check_core_version(dwc2);
>  	if (ret)
>  		goto error;
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 679ce6e42..321e546f9 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -195,7 +195,7 @@ static inline struct phy *phy_get(struct device_d *dev, const char *string)
>  static inline struct phy *phy_optional_get(struct device_d *dev,
>  					   const char *string)
>  {
> -	return ERR_PTR(-ENOSYS);
> +	return NULL;
>  }
I am interested in getting this modification merged. Beside I don't
think the dwc2 driver modification is needed, as phy_optional_get is
intended to return NULL when no phy are available. For me it makes
sense that NULL could be return by phy_optional_get when the phy
support is not compiled in.

Do you have the time to send a new patch with only this modification ?

Thanks,
Jules





_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2 v2] usb: dwc2: Make having a phy optional

[Daniel Brát]

Hi Jules,

On Tue, 19 Oct 2021 18:16:54 +0200, Jules Maselbas wrote:
> I am interested in getting this modification merged. Beside I don't
> think the dwc2 driver modification is needed, as phy_optional_get is
> intended to return NULL when no phy are available. For me it makes
> sense that NULL could be return by phy_optional_get when the phy
> support is not compiled in.
> 
> Do you have the time to send a new patch with only this modification ?

I apologize for not responding to this thread sooner, had a lot on my plate
lately. I agree that simply having the 'phy_optional_get' return NULL when
unimplemented is better than making changes in the dwc2 driver itself and
it makes sense in general for it to behave this ways. All the other functions
using the phy already check for NULL anyway, so it is perfect fit.
I submitted it as a new patch as you suggested.

Daniel


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2 v2] usb: dwc2: Make having a phy optional

[Jules Maselbas]

Hi,

On Wed, Oct 20, 2021 at 11:07:38AM +0200, Daniel Brát wrote:
> Hi Jules,
> 
> On Tue, 19 Oct 2021 18:16:54 +0200, Jules Maselbas wrote:
> > I am interested in getting this modification merged. Beside I don't
> > think the dwc2 driver modification is needed, as phy_optional_get is
> > intended to return NULL when no phy are available. For me it makes
> > sense that NULL could be return by phy_optional_get when the phy
> > support is not compiled in.
> > 
> > Do you have the time to send a new patch with only this modification ?
> 
> I apologize for not responding to this thread sooner, had a lot on my plate
That's ok

> lately. I agree that simply having the 'phy_optional_get' return NULL when
> unimplemented is better than making changes in the dwc2 driver itself and
> it makes sense in general for it to behave this ways. All the other functions
> using the phy already check for NULL anyway, so it is perfect fit.
> I submitted it as a new patch as you suggested.
nice :)

Thanks.





_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] ARM: rpi_defconfig: Enable PHY core for dwc2 usb driver

[Daniel Brát]

The Raspbery Pi device tree defines 'usb2_phy' optionally used by the
dwc2 usb driver, so lets enable PHY core for it in the defconfig.

Signed-off-by: Daniel Brát <danek.brat@gmail.com>
---
 arch/arm/configs/rpi_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/rpi_defconfig b/arch/arm/configs/rpi_defconfig
index f42b8819a..b823a9a3d 100644
--- a/arch/arm/configs/rpi_defconfig
+++ b/arch/arm/configs/rpi_defconfig
@@ -96,6 +96,8 @@ CONFIG_WATCHDOG_BCM2835=y
 CONFIG_GPIO_RASPBERRYPI_EXP=y
 CONFIG_PINCTRL_BCM283X=y
 CONFIG_REGULATOR=y
+CONFIG_GENERIC_PHY=y
+CONFIG_USB_NOP_XCEIV=y
 CONFIG_FS_EXT4=y
 CONFIG_FS_TFTP=y
 CONFIG_FS_NFS=y
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox