diff mbox series

[v4,2/5] usb: host: dwc2: add phy support

Message ID 20200218083503.5468-3-patrick.delaunay@st.com
State Superseded
Headers show
Series usb: host: dwc2: use driver model for PHY and CLOCK | expand

Commit Message

Patrick Delaunay Feb. 18, 2020, 8:35 a.m. UTC
Use generic phy to initialize the PHY associated to the
DWC2 device and available in the device tree.

This patch don't added dependency because when CONFIG_PHY
is not activated, the generic PHY function are stubbed.

Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2:
- update dev_err
- update commit message
- change dev_err to dev_dbg for PHY function call
- treat dwc2_shutdown_phy error

 drivers/usb/host/dwc2.c | 66 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

Comments

Simon Goldschmidt March 4, 2020, 7:51 p.m. UTC | #1
Am 18.02.2020 um 09:35 schrieb Patrick Delaunay:
> Use generic phy to initialize the PHY associated to the
> DWC2 device and available in the device tree.
> 
> This patch don't added dependency because when CONFIG_PHY
> is not activated, the generic PHY function are stubbed.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - update dev_err
> - update commit message
> - change dev_err to dev_dbg for PHY function call
> - treat dwc2_shutdown_phy error
> 
>  drivers/usb/host/dwc2.c | 66 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> index e4efaf1e59..5e7ffaddd9 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -8,6 +8,7 @@
>  #include <cpu_func.h>
>  #include <dm.h>
>  #include <errno.h>
> +#include <generic-phy.h>
>  #include <usb.h>
>  #include <malloc.h>
>  #include <memalign.h>
> @@ -37,6 +38,7 @@ struct dwc2_priv {
>  #ifdef CONFIG_DM_REGULATOR
>  	struct udevice *vbus_supply;
>  #endif
> +	struct phy phy;
>  #else
>  	uint8_t *aligned_buffer;
>  	uint8_t *status_buffer;
> @@ -1322,13 +1324,71 @@ static int dwc2_usb_ofdata_to_platdata(struct udevice *dev)
>  	return 0;
>  }
>  
> +static int dwc2_setup_phy(struct udevice *dev)
> +{
> +	struct dwc2_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
> +	if (ret) {
> +		if (ret != -ENOENT) {

Could you invert this logic and add a comment like "no PHY" or something?

> +			dev_err(dev, "Failed to get USB PHY: %d.\n", ret);
> +			return ret;
> +		}
> +		return 0;
> +	}
> +
> +	ret = generic_phy_init(&priv->phy);
> +	if (ret) {
> +		dev_dbg(dev, "Failed to init USB PHY: %d.\n", ret);
> +		return ret;
> +	}
> +
> +	ret = generic_phy_power_on(&priv->phy);
> +	if (ret) {
> +		dev_dbg(dev, "Failed to power on USB PHY: %d.\n", ret);
> +		generic_phy_exit(&priv->phy);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dwc2_shutdown_phy(struct udevice *dev)
> +{
> +	struct dwc2_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	if (!generic_phy_valid(&priv->phy))

A comment saying that this is for platforms without a phy driver would
be nice.

Other than that:
Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>

> +		return 0;
> +
> +	ret = generic_phy_power_off(&priv->phy);
> +	if (ret) {
> +		dev_dbg(dev, "Failed to power off USB PHY: %d.\n", ret);
> +		return ret;
> +	}
> +
> +	ret = generic_phy_exit(&priv->phy);
> +	if (ret) {
> +		dev_dbg(dev, "Failed to power off USB PHY: %d.\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int dwc2_usb_probe(struct udevice *dev)
>  {
>  	struct dwc2_priv *priv = dev_get_priv(dev);
>  	struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev);
> +	int ret;
>  
>  	bus_priv->desc_before_addr = true;
>  
> +	ret = dwc2_setup_phy(dev);
> +	if (ret)
> +		return ret;
> +
>  	return dwc2_init_common(dev, priv);
>  }
>  
> @@ -1341,6 +1401,12 @@ static int dwc2_usb_remove(struct udevice *dev)
>  	if (ret)
>  		return ret;
>  
> +	ret = dwc2_shutdown_phy(dev);
> +	if (ret) {
> +		dev_dbg(dev, "Failed to shutdown USB PHY: %d.\n", ret);
> +		return ret;
> +	}
> +
>  	dwc2_uninit_common(priv->regs);
>  
>  	reset_release_bulk(&priv->resets);
>
Patrick Delaunay March 5, 2020, 6:24 p.m. UTC | #2
> -----Original Message-----
> From: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> Sent: mercredi 4 mars 2020 20:52
> To: Patrick DELAUNAY <patrick.delaunay at st.com>; u-boot at lists.denx.de
> Cc: ley.foon.tan at intel.com; b.galvani at gmail.com; Daniel Schwierzeck
> <daniel.schwierzeck at gmail.com>; Marek Vasut <marex at denx.de>; Michal
> Suchanek <msuchanek at suse.de>; Simon Glass <sjg at chromium.org>; U-Boot
> STM32 <uboot-stm32 at st-md-mailman.stormreply.com>
> Subject: Re: [PATCH v4 2/5] usb: host: dwc2: add phy support
> Importance: High
> 
> Am 18.02.2020 um 09:35 schrieb Patrick Delaunay:
> > Use generic phy to initialize the PHY associated to the
> > DWC2 device and available in the device tree.
> >
> > This patch don't added dependency because when CONFIG_PHY is not
> > activated, the generic PHY function are stubbed.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> >
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2:
> > - update dev_err
> > - update commit message
> > - change dev_err to dev_dbg for PHY function call
> > - treat dwc2_shutdown_phy error
> >
> >  drivers/usb/host/dwc2.c | 66
> > +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index
> > e4efaf1e59..5e7ffaddd9 100644
> > --- a/drivers/usb/host/dwc2.c
> > +++ b/drivers/usb/host/dwc2.c
> > @@ -8,6 +8,7 @@
> >  #include <cpu_func.h>
> >  #include <dm.h>
> >  #include <errno.h>
> > +#include <generic-phy.h>
> >  #include <usb.h>
> >  #include <malloc.h>
> >  #include <memalign.h>
> > @@ -37,6 +38,7 @@ struct dwc2_priv {
> >  #ifdef CONFIG_DM_REGULATOR
> >  	struct udevice *vbus_supply;
> >  #endif
> > +	struct phy phy;
> >  #else
> >  	uint8_t *aligned_buffer;
> >  	uint8_t *status_buffer;
> > @@ -1322,13 +1324,71 @@ static int dwc2_usb_ofdata_to_platdata(struct
> udevice *dev)
> >  	return 0;
> >  }
> >
> > +static int dwc2_setup_phy(struct udevice *dev) {
> > +	struct dwc2_priv *priv = dev_get_priv(dev);
> > +	int ret;
> > +
> > +	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
> > +	if (ret) {
> > +		if (ret != -ENOENT) {
> 
> Could you invert this logic and add a comment like "no PHY" or something?

Yes in V5, it is more clear

	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
	if (ret) {
		if (ret == -ENOENT)
			return 0; /* no PHY, nothing to do */
		dev_err(dev, "Failed to get USB PHY: %d.\n", ret);
		return ret;
	}

> > +			dev_err(dev, "Failed to get USB PHY: %d.\n", ret);
> > +			return ret;
> > +		}
> > +		return 0;
> > +	}
> > +
> > +	ret = generic_phy_init(&priv->phy);
> > +	if (ret) {
> > +		dev_dbg(dev, "Failed to init USB PHY: %d.\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = generic_phy_power_on(&priv->phy);
> > +	if (ret) {
> > +		dev_dbg(dev, "Failed to power on USB PHY: %d.\n", ret);
> > +		generic_phy_exit(&priv->phy);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwc2_shutdown_phy(struct udevice *dev) {
> > +	struct dwc2_priv *priv = dev_get_priv(dev);
> > +	int ret;
> > +
> > +	if (!generic_phy_valid(&priv->phy))
> 
> A comment saying that this is for platforms without a phy driver would be nice.

Yes in V5:

static int dwc2_shutdown_phy(struct udevice *dev)
{
	struct dwc2_priv *priv = dev_get_priv(dev);
	int ret;

	/* PHY is not valid when generic_phy_get_by_index() = -ENOENT */
	if (!generic_phy_valid(&priv->phy))
		return 0; /* no PHY, nothing to do */


> Other than that:
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>

Thanks 
Regards

Patrick
diff mbox series

Patch

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index e4efaf1e59..5e7ffaddd9 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -8,6 +8,7 @@ 
 #include <cpu_func.h>
 #include <dm.h>
 #include <errno.h>
+#include <generic-phy.h>
 #include <usb.h>
 #include <malloc.h>
 #include <memalign.h>
@@ -37,6 +38,7 @@  struct dwc2_priv {
 #ifdef CONFIG_DM_REGULATOR
 	struct udevice *vbus_supply;
 #endif
+	struct phy phy;
 #else
 	uint8_t *aligned_buffer;
 	uint8_t *status_buffer;
@@ -1322,13 +1324,71 @@  static int dwc2_usb_ofdata_to_platdata(struct udevice *dev)
 	return 0;
 }
 
+static int dwc2_setup_phy(struct udevice *dev)
+{
+	struct dwc2_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
+	if (ret) {
+		if (ret != -ENOENT) {
+			dev_err(dev, "Failed to get USB PHY: %d.\n", ret);
+			return ret;
+		}
+		return 0;
+	}
+
+	ret = generic_phy_init(&priv->phy);
+	if (ret) {
+		dev_dbg(dev, "Failed to init USB PHY: %d.\n", ret);
+		return ret;
+	}
+
+	ret = generic_phy_power_on(&priv->phy);
+	if (ret) {
+		dev_dbg(dev, "Failed to power on USB PHY: %d.\n", ret);
+		generic_phy_exit(&priv->phy);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dwc2_shutdown_phy(struct udevice *dev)
+{
+	struct dwc2_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	if (!generic_phy_valid(&priv->phy))
+		return 0;
+
+	ret = generic_phy_power_off(&priv->phy);
+	if (ret) {
+		dev_dbg(dev, "Failed to power off USB PHY: %d.\n", ret);
+		return ret;
+	}
+
+	ret = generic_phy_exit(&priv->phy);
+	if (ret) {
+		dev_dbg(dev, "Failed to power off USB PHY: %d.\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int dwc2_usb_probe(struct udevice *dev)
 {
 	struct dwc2_priv *priv = dev_get_priv(dev);
 	struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev);
+	int ret;
 
 	bus_priv->desc_before_addr = true;
 
+	ret = dwc2_setup_phy(dev);
+	if (ret)
+		return ret;
+
 	return dwc2_init_common(dev, priv);
 }
 
@@ -1341,6 +1401,12 @@  static int dwc2_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	ret = dwc2_shutdown_phy(dev);
+	if (ret) {
+		dev_dbg(dev, "Failed to shutdown USB PHY: %d.\n", ret);
+		return ret;
+	}
+
 	dwc2_uninit_common(priv->regs);
 
 	reset_release_bulk(&priv->resets);