[PATCH v4 0/5] usb: host: dwc2: use driver model for PHY and CLOCK

[PATCH v4 0/5] usb: host: dwc2: use driver model for PHY and CLOCK

[Patrick Delaunay]

In this serie I update the DWC2 host driver to use the device tree
information and the associated PHY and CLOCK drivers when they are
availables.

The V4 is rebased on latest master (v2020.04-rc2).
CI-Tavis build is OK:
    https://travis-ci.org/patrickdelaunay/u-boot/builds/651479714

NB: CI-Travis build was OK for all target after V3:
    https://travis-ci.org/patrickdelaunay/u-boot/builds/609496187
    As in V2, I cause the warnings for some boards:
    drivers/usb/host/built-in.o: In function `dwc2_usb_remove':
    drivers/usb/host/dwc2.c:1441: undefined reference to `clk_disable_bulk'

I test this serie on stm32mp157c-ev1 board, with PHY and CLK
support

The U-CLASS are provided by:
- PHY by USBPHYC driver = ./drivers/phy/phy-stm32-usbphyc.c
- CLOCK by RCC clock driver = drivers/clk/clk_stm32mp1.c
- RESET by RCC reset driver = drivers/reset/stm32-reset.c

And I activate the configuration
+CONFIG_USB_DWC2=y

PS: it is not the default configuration to avoid conflict with gadget
    driver

To solve a binding issue, I also deactivate the gadget support:
by default only one driver is bound to the usbotg_hs node with "snps,dwc2"
compatible, and today it is the device one (the first in the driver list).

I also need to deactivate hnp-srp support with:

&usbotg_hs {
	/* need to disable ONLY for HOST support */
	hnp-srp-disable;
};

WARNING: OTG with device or host support is not correctly handle by DWC2
         driver (see example for dynamic OTG role in DWC3 driver).

The tests executed on the stm32mp157c-ev1 target:

STM32MP> usb start
starting USB...
Bus usb-otg at 49000000: USB DWC2
Bus usbh-ehci at 5800d000: USB EHCI 1.00
scanning bus usb-otg at 49000000 for devices... 2 USB Device(s) found
scanning bus usbh-ehci at 5800d000 for devices... 3 USB Device(s) found
       scanning usb for storage devices... 2 Storage Device(s) found
STM32MP> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
  |   U-Boot Root Hub
  |
  +-2  Mass Storage (480 Mb/s, 300mA)
       Verbatim STORE N GO 070731C8ACD7EE97

  1  Hub (480 Mb/s, 0mA)
  |  u-boot EHCI Host Controller
  |
  +-2  Hub (480 Mb/s, 2mA)

STM32MP> ls usb 0
<DIR>       4096 .
<DIR>       4096 ..
<DIR>      16384 lost+found
<DIR>       4096 record
         1490212 xipImage
        21058006 vmlinux

STM32MP> load usb 0 0xC0000000 vmlinux
21058006 bytes read in 10851 ms (1.9 MiB/s)


Changes in v4:
- Add stub for all functions using 'struct clk' or 'struct clk_bulk'
  after remarks on v3

Changes in v3:
- Add stub for clk_disable_bulk

Changes in v2:
- update dev_err
- update commit message
- change dev_err to dev_dbg for PHY function call
- treat dwc2_shutdown_phy error
- add clk_disable_bulk in dwc2_usb_remove

Patrick Delaunay (5):
  dm: clk: add stub when CONFIG_CLK is desactivated
  usb: host: dwc2: add phy support
  usb: host: dwc2: add clk support
  usb: host: dwc2: force reset assert
  usb: host: dwc2: add trace to have clean usb start

 drivers/usb/host/dwc2.c | 100 ++++++++++++++++++++++++++++++++++++++-
 include/clk.h           | 101 ++++++++++++++++++++++++++++++++++------
 2 files changed, 187 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH v4 1/5] dm: clk: add stub when CONFIG_CLK is desactivated

[Patrick Delaunay]

Add stub for functions clk_...() when CONFIG_CLK is desactivated.

This patch avoids compilation issues for driver using these API
without protection (#if CONFIG_IS_ENABLED(CLK))

For example, before this patch we have undefined reference to
`clk_disable_bulk') for code:
  clk_disable_bulk(&priv->clks);
  clk_release_bulk(&priv->clks);

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

Changes in v4:
- Add stub for all functions using 'struct clk' or 'struct clk_bulk'
  after remarks on v3

Changes in v3:
- Add stub for clk_disable_bulk

Changes in v2: None

 include/clk.h | 101 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 88 insertions(+), 13 deletions(-)

diff --git a/include/clk.h b/include/clk.h
index 3336301815..1fb415ddc8 100644
--- a/include/clk.h
+++ b/include/clk.h
@@ -312,6 +312,7 @@ static inline int clk_release_bulk(struct clk_bulk *bulk)
 	return clk_release_all(bulk->clks, bulk->count);
 }
 
+#if CONFIG_IS_ENABLED(CLK)
 /**
  * clk_request - Request a clock by provider-specific ID.
  *
@@ -433,19 +434,6 @@ int clk_disable_bulk(struct clk_bulk *bulk);
  */
 bool clk_is_match(const struct clk *p, const struct clk *q);
 
-int soc_clk_dump(void);
-
-/**
- * clk_valid() - check if clk is valid
- *
- * @clk:	the clock to check
- * @return true if valid, or false
- */
-static inline bool clk_valid(struct clk *clk)
-{
-	return clk && !!clk->dev;
-}
-
 /**
  * clk_get_by_id() - Get the clock by its ID
  *
@@ -465,6 +453,93 @@ int clk_get_by_id(ulong id, struct clk **clkp);
  * @return true on binded, or false on no
  */
 bool clk_dev_binded(struct clk *clk);
+
+#else /* CONFIG_IS_ENABLED(CLK) */
+
+static inline int clk_request(struct udevice *dev, struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_free(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline ulong clk_get_rate(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline struct clk *clk_get_parent(struct clk *clk)
+{
+	return (struct clk *)-ENOSYS;
+}
+
+static inline long long clk_get_parent_rate(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline ulong clk_set_rate(struct clk *clk, ulong rate)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_enable(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_enable_bulk(struct clk_bulk *bulk)
+{
+	return bulk && bulk->count == 0 ? 0 : -ENOSYS;
+}
+
+static inline int clk_disable(struct clk *clk)
+{
+	return -ENOSYS;
+}
+
+static inline int clk_disable_bulk(struct clk_bulk *bulk)
+{
+	return bulk && bulk->count == 0 ? 0 : -ENOSYS;
+}
+
+static inline bool clk_is_match(const struct clk *p, const struct clk *q)
+{
+	return false;
+}
+
+static inline int clk_get_by_id(ulong id, struct clk **clkp)
+{
+	return -ENOSYS;
+}
+
+static inline bool clk_dev_binded(struct clk *clk)
+{
+	return false;
+}
+#endif /* CONFIG_IS_ENABLED(CLK) */
+
+/**
+ * clk_valid() - check if clk is valid
+ *
+ * @clk:	the clock to check
+ * @return true if valid, or false
+ */
+static inline bool clk_valid(struct clk *clk)
+{
+	return clk && !!clk->dev;
+}
+
+int soc_clk_dump(void);
+
 #endif
 
 #define clk_prepare_enable(clk) clk_enable(clk)
-- 
2.17.1

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

[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@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) {
+			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);
-- 
2.17.1

[PATCH v4 3/5] usb: host: dwc2: add clk support

[Patrick Delaunay]

Add support for clock with driver model.

This patch don't added dependency because when CONFIG_CLK
is not activated the clk function are stubbed.

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

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/usb/host/dwc2.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 5e7ffaddd9..d56d0e61b5 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -5,14 +5,15 @@
  */
 
 #include <common.h>
+#include <clk.h>
 #include <cpu_func.h>
 #include <dm.h>
 #include <errno.h>
 #include <generic-phy.h>
-#include <usb.h>
 #include <malloc.h>
 #include <memalign.h>
 #include <phys2bus.h>
+#include <usb.h>
 #include <usbroothubdes.h>
 #include <wait_bit.h>
 #include <asm/io.h>
@@ -39,6 +40,7 @@ struct dwc2_priv {
 	struct udevice *vbus_supply;
 #endif
 	struct phy phy;
+	struct clk_bulk clks;
 #else
 	uint8_t *aligned_buffer;
 	uint8_t *status_buffer;
@@ -1377,6 +1379,26 @@ static int dwc2_shutdown_phy(struct udevice *dev)
 	return 0;
 }
 
+static int dwc2_clk_init(struct udevice *dev)
+{
+	struct dwc2_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = clk_get_bulk(dev, &priv->clks);
+	if (ret == -ENOSYS || ret == -ENOENT)
+		return 0;
+	if (ret)
+		return ret;
+
+	ret = clk_enable_bulk(&priv->clks);
+	if (ret) {
+		clk_release_bulk(&priv->clks);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int dwc2_usb_probe(struct udevice *dev)
 {
 	struct dwc2_priv *priv = dev_get_priv(dev);
@@ -1385,6 +1407,10 @@ static int dwc2_usb_probe(struct udevice *dev)
 
 	bus_priv->desc_before_addr = true;
 
+	ret = dwc2_clk_init(dev);
+	if (ret)
+		return ret;
+
 	ret = dwc2_setup_phy(dev);
 	if (ret)
 		return ret;
@@ -1410,6 +1436,8 @@ static int dwc2_usb_remove(struct udevice *dev)
 	dwc2_uninit_common(priv->regs);
 
 	reset_release_bulk(&priv->resets);
+	clk_disable_bulk(&priv->clks);
+	clk_release_bulk(&priv->clks);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v4 4/5] usb: host: dwc2: force reset assert

[Patrick Delaunay]

Assert reset before deassert in dwc2_reset;
this patch solve issues when the DWC2 registers are already
initialized with value incompatible with host mode.

Force a hardware reset of the IP reset all the DWC2 registers at
default value, the host driver start with a clean state
(Core Soft reset doen in dwc_otg_core_reset is not enought
 to reset all register).

The error can occurs in U-Boot when DWC2 device gadget driver
force device mode (called by ums or dfu command, before to execute
the usb start command).

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

Changes in v4: None
Changes in v3: None
Changes in v2:
- add clk_disable_bulk in dwc2_usb_remove

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

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index d56d0e61b5..f53913cde4 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -1151,6 +1151,8 @@ static int dwc2_reset(struct udevice *dev)
 			return ret;
 	}
 
+	/* force reset to clear all IP register */
+	reset_assert_bulk(&priv->resets);
 	ret = reset_deassert_bulk(&priv->resets);
 	if (ret) {
 		reset_release_bulk(&priv->resets);
-- 
2.17.1

[PATCH v4 5/5] usb: host: dwc2: add trace to have clean usb start

[Patrick Delaunay]

Solve issue for the display of "usb start" command on stm32mp1
because one carriage return is missing in DWC2 probe.

Before the patch:

STM32MP> usb start
starting USB...
Bus usb-otg at 49000000:    Bus usbh-ehci at 5800d000:   USB EHCI 1.00

after the patch:

STM32MP> usb start
starting USB...
Bus usb-otg at 49000000: USB DWC2
Bus usbh-ehci at 5800d000: USB EHCI 1.00

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

Changes in v4: None
Changes in v3: None
Changes in v2: None

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

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index f53913cde4..8cc395037d 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -1219,6 +1219,8 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv)
 	if (readl(&regs->gintsts) & DWC2_GINTSTS_CURMODE_HOST)
 		mdelay(1000);
 
+	printf("USB DWC2\n");
+
 	return 0;
 }
 
-- 
2.17.1

[PATCH v4 0/5] usb: host: dwc2: use driver model for PHY and CLOCK

[Marek Vasut]

On 2/18/20 9:34 AM, Patrick Delaunay wrote:
> 
> In this serie I update the DWC2 host driver to use the device tree
> information and the associated PHY and CLOCK drivers when they are
> availables.
> 
> The V4 is rebased on latest master (v2020.04-rc2).
> CI-Tavis build is OK:
>     https://travis-ci.org/patrickdelaunay/u-boot/builds/651479714
> 
> NB: CI-Travis build was OK for all target after V3:
>     https://travis-ci.org/patrickdelaunay/u-boot/builds/609496187
>     As in V2, I cause the warnings for some boards:
>     drivers/usb/host/built-in.o: In function `dwc2_usb_remove':
>     drivers/usb/host/dwc2.c:1441: undefined reference to `clk_disable_bulk'
> 
> I test this serie on stm32mp157c-ev1 board, with PHY and CLK
> support
> 
> The U-CLASS are provided by:
> - PHY by USBPHYC driver = ./drivers/phy/phy-stm32-usbphyc.c
> - CLOCK by RCC clock driver = drivers/clk/clk_stm32mp1.c
> - RESET by RCC reset driver = drivers/reset/stm32-reset.c
> 
> And I activate the configuration
> +CONFIG_USB_DWC2=y

Simon, can you test this on SOCFPGA ?

[...]

[PATCH v4 0/5] usb: host: dwc2: use driver model for PHY and CLOCK

[Simon Goldschmidt]

On Tue, Feb 18, 2020 at 6:53 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/18/20 9:34 AM, Patrick Delaunay wrote:
> >
> > In this serie I update the DWC2 host driver to use the device tree
> > information and the associated PHY and CLOCK drivers when they are
> > availables.
> >
> > The V4 is rebased on latest master (v2020.04-rc2).
> > CI-Tavis build is OK:
> >     https://travis-ci.org/patrickdelaunay/u-boot/builds/651479714
> >
> > NB: CI-Travis build was OK for all target after V3:
> >     https://travis-ci.org/patrickdelaunay/u-boot/builds/609496187
> >     As in V2, I cause the warnings for some boards:
> >     drivers/usb/host/built-in.o: In function `dwc2_usb_remove':
> >     drivers/usb/host/dwc2.c:1441: undefined reference to `clk_disable_bulk'
> >
> > I test this serie on stm32mp157c-ev1 board, with PHY and CLK
> > support
> >
> > The U-CLASS are provided by:
> > - PHY by USBPHYC driver = ./drivers/phy/phy-stm32-usbphyc.c
> > - CLOCK by RCC clock driver = drivers/clk/clk_stm32mp1.c
> > - RESET by RCC reset driver = drivers/reset/stm32-reset.c
> >
> > And I activate the configuration
> > +CONFIG_USB_DWC2=y
>
> Simon, can you test this on SOCFPGA ?

I can test if it probes, but I don't have anything running on that USB port
the socfpga_socrates board has. Would that be enought to test?

Regards,
Simon

>
> [...]

[PATCH v4 1/5] dm: clk: add stub when CONFIG_CLK is desactivated

[Simon Goldschmidt]

Am 18.02.2020 um 09:34 schrieb Patrick Delaunay:
> Add stub for functions clk_...() when CONFIG_CLK is desactivated.
> 
> This patch avoids compilation issues for driver using these API
> without protection (#if CONFIG_IS_ENABLED(CLK))
> 
> For example, before this patch we have undefined reference to
> `clk_disable_bulk') for code:
>   clk_disable_bulk(&priv->clks);
>   clk_release_bulk(&priv->clks);
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> 
> Changes in v4:
> - Add stub for all functions using 'struct clk' or 'struct clk_bulk'
>   after remarks on v3
> 
> Changes in v3:
> - Add stub for clk_disable_bulk
> 
> Changes in v2: None
> 
>  include/clk.h | 101 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 88 insertions(+), 13 deletions(-)
> 
> diff --git a/include/clk.h b/include/clk.h
> index 3336301815..1fb415ddc8 100644
> --- a/include/clk.h
> +++ b/include/clk.h
> @@ -312,6 +312,7 @@ static inline int clk_release_bulk(struct clk_bulk *bulk)
>  	return clk_release_all(bulk->clks, bulk->count);
>  }
>  
> +#if CONFIG_IS_ENABLED(CLK)
>  /**
>   * clk_request - Request a clock by provider-specific ID.
>   *
> @@ -433,19 +434,6 @@ int clk_disable_bulk(struct clk_bulk *bulk);
>   */
>  bool clk_is_match(const struct clk *p, const struct clk *q);
>  
> -int soc_clk_dump(void);
> -
> -/**
> - * clk_valid() - check if clk is valid
> - *
> - * @clk:	the clock to check
> - * @return true if valid, or false
> - */
> -static inline bool clk_valid(struct clk *clk)
> -{
> -	return clk && !!clk->dev;
> -}
> -
>  /**
>   * clk_get_by_id() - Get the clock by its ID
>   *
> @@ -465,6 +453,93 @@ int clk_get_by_id(ulong id, struct clk **clkp);
>   * @return true on binded, or false on no
>   */
>  bool clk_dev_binded(struct clk *clk);
> +
> +#else /* CONFIG_IS_ENABLED(CLK) */
> +
> +static inline int clk_request(struct udevice *dev, struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_free(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline ulong clk_get_rate(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline struct clk *clk_get_parent(struct clk *clk)
> +{
> +	return (struct clk *)-ENOSYS;

This should use ERR_PTR() to care for platforms defining
CONFIG_ERR_PTR_OFFSET.

> +}
> +
> +static inline long long clk_get_parent_rate(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline ulong clk_set_rate(struct clk *clk, ulong rate)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_enable(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_enable_bulk(struct clk_bulk *bulk)
> +{
> +	return bulk && bulk->count == 0 ? 0 : -ENOSYS;

For this test to work, someone would need to set bulk->count to 0. This
is normally done by clk_get_bulk(), but you defined it to only return an
error.

I guess it works for you because all clk_bulk objects you use are from
the heap (which is currently zeroed out in U-Boot) or if they are on the
stack, you have if/else code that doesn't bring you here. Still it seems
wrong.

Regards,
Simon

> +}
> +
> +static inline int clk_disable(struct clk *clk)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline int clk_disable_bulk(struct clk_bulk *bulk)
> +{
> +	return bulk && bulk->count == 0 ? 0 : -ENOSYS;
> +}
> +
> +static inline bool clk_is_match(const struct clk *p, const struct clk *q)
> +{
> +	return false;
> +}
> +
> +static inline int clk_get_by_id(ulong id, struct clk **clkp)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline bool clk_dev_binded(struct clk *clk)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_IS_ENABLED(CLK) */
> +
> +/**
> + * clk_valid() - check if clk is valid
> + *
> + * @clk:	the clock to check
> + * @return true if valid, or false
> + */
> +static inline bool clk_valid(struct clk *clk)
> +{
> +	return clk && !!clk->dev;
> +}
> +
> +int soc_clk_dump(void);
> +
>  #endif
>  
>  #define clk_prepare_enable(clk) clk_enable(clk)
> 

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

[Simon Goldschmidt]

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@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@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);
> 

[PATCH v4 3/5] usb: host: dwc2: add clk support

[Simon Goldschmidt]

Am 18.02.2020 um 09:35 schrieb Patrick Delaunay:
> Add support for clock with driver model.
> 
> This patch don't added dependency because when CONFIG_CLK
> is not activated the clk function are stubbed.
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/usb/host/dwc2.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> index 5e7ffaddd9..d56d0e61b5 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -5,14 +5,15 @@
>   */
>  
>  #include <common.h>
> +#include <clk.h>
>  #include <cpu_func.h>
>  #include <dm.h>
>  #include <errno.h>
>  #include <generic-phy.h>
> -#include <usb.h>
>  #include <malloc.h>
>  #include <memalign.h>
>  #include <phys2bus.h>
> +#include <usb.h>
>  #include <usbroothubdes.h>
>  #include <wait_bit.h>
>  #include <asm/io.h>
> @@ -39,6 +40,7 @@ struct dwc2_priv {
>  	struct udevice *vbus_supply;
>  #endif
>  	struct phy phy;
> +	struct clk_bulk clks;
>  #else
>  	uint8_t *aligned_buffer;
>  	uint8_t *status_buffer;
> @@ -1377,6 +1379,26 @@ static int dwc2_shutdown_phy(struct udevice *dev)
>  	return 0;
>  }
>  
> +static int dwc2_clk_init(struct udevice *dev)
> +{
> +	struct dwc2_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = clk_get_bulk(dev, &priv->clks);
> +	if (ret == -ENOSYS || ret == -ENOENT)
> +		return 0;
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable_bulk(&priv->clks);
> +	if (ret) {
> +		clk_release_bulk(&priv->clks);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int dwc2_usb_probe(struct udevice *dev)
>  {
>  	struct dwc2_priv *priv = dev_get_priv(dev);
> @@ -1385,6 +1407,10 @@ static int dwc2_usb_probe(struct udevice *dev)
>  
>  	bus_priv->desc_before_addr = true;
>  
> +	ret = dwc2_clk_init(dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = dwc2_setup_phy(dev);
>  	if (ret)
>  		return ret;
> @@ -1410,6 +1436,8 @@ static int dwc2_usb_remove(struct udevice *dev)
>  	dwc2_uninit_common(priv->regs);
>  
>  	reset_release_bulk(&priv->resets);
> +	clk_disable_bulk(&priv->clks);
> +	clk_release_bulk(&priv->clks);
>  
>  	return 0;
>  }
> 

[PATCH v4 0/5] usb: host: dwc2: use driver model for PHY and CLOCK

[Simon Goldschmidt]

Am 19.02.2020 um 08:27 schrieb Simon Goldschmidt:
> On Tue, Feb 18, 2020 at 6:53 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/18/20 9:34 AM, Patrick Delaunay wrote:
>>>
>>> In this serie I update the DWC2 host driver to use the device tree
>>> information and the associated PHY and CLOCK drivers when they are
>>> availables.
>>>
>>> The V4 is rebased on latest master (v2020.04-rc2).
>>> CI-Tavis build is OK:
>>>     https://travis-ci.org/patrickdelaunay/u-boot/builds/651479714
>>>
>>> NB: CI-Travis build was OK for all target after V3:
>>>     https://travis-ci.org/patrickdelaunay/u-boot/builds/609496187
>>>     As in V2, I cause the warnings for some boards:
>>>     drivers/usb/host/built-in.o: In function `dwc2_usb_remove':
>>>     drivers/usb/host/dwc2.c:1441: undefined reference to `clk_disable_bulk'
>>>
>>> I test this serie on stm32mp157c-ev1 board, with PHY and CLK
>>> support
>>>
>>> The U-CLASS are provided by:
>>> - PHY by USBPHYC driver = ./drivers/phy/phy-stm32-usbphyc.c
>>> - CLOCK by RCC clock driver = drivers/clk/clk_stm32mp1.c
>>> - RESET by RCC reset driver = drivers/reset/stm32-reset.c
>>>
>>> And I activate the configuration
>>> +CONFIG_USB_DWC2=y
>>
>> Simon, can you test this on SOCFPGA ?
> 
> I can test if it probes, but I don't have anything running on that USB port
> the socfpga_socrates board has. Would that be enought to test?

Tested the whole series on socfpga_socrates by instantiating the driver.
Shows the same behaviour as before (I still have no OTG cable to test
attaching a storage device).

Regards,
Simon

> 
> Regards,
> Simon
> 
>>
>> [...]

[PATCH v4 1/5] dm: clk: add stub when CONFIG_CLK is desactivated

[Patrick DELAUNAY]

Hi Simon,

> From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Sent: mercredi 4 mars 2020 20:49
> 
> Am 18.02.2020 um 09:34 schrieb Patrick Delaunay:
> > Add stub for functions clk_...() when CONFIG_CLK is desactivated.
> >
> > This patch avoids compilation issues for driver using these API
> > without protection (#if CONFIG_IS_ENABLED(CLK))
> >
> > For example, before this patch we have undefined reference to
> > `clk_disable_bulk') for code:
> >   clk_disable_bulk(&priv->clks);
> >   clk_release_bulk(&priv->clks);
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> >
> > Changes in v4:
> > - Add stub for all functions using 'struct clk' or 'struct clk_bulk'
> >   after remarks on v3
> >
> > Changes in v3:
> > - Add stub for clk_disable_bulk
> >
> > Changes in v2: None
> >
> >  include/clk.h | 101
> > +++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 88 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/clk.h b/include/clk.h index
> > 3336301815..1fb415ddc8 100644
> > --- a/include/clk.h
> > +++ b/include/clk.h
> > @@ -312,6 +312,7 @@ static inline int clk_release_bulk(struct clk_bulk *bulk)
> >  	return clk_release_all(bulk->clks, bulk->count);  }
> >
> > +#if CONFIG_IS_ENABLED(CLK)
> >  /**
> >   * clk_request - Request a clock by provider-specific ID.
> >   *
> > @@ -433,19 +434,6 @@ int clk_disable_bulk(struct clk_bulk *bulk);
> >   */
> >  bool clk_is_match(const struct clk *p, const struct clk *q);
> >
> > -int soc_clk_dump(void);
> > -
> > -/**
> > - * clk_valid() - check if clk is valid
> > - *
> > - * @clk:	the clock to check
> > - * @return true if valid, or false
> > - */
> > -static inline bool clk_valid(struct clk *clk) -{
> > -	return clk && !!clk->dev;
> > -}
> > -
> >  /**
> >   * clk_get_by_id() - Get the clock by its ID
> >   *
> > @@ -465,6 +453,93 @@ int clk_get_by_id(ulong id, struct clk **clkp);
> >   * @return true on binded, or false on no
> >   */
> >  bool clk_dev_binded(struct clk *clk);
> > +
> > +#else /* CONFIG_IS_ENABLED(CLK) */
> > +
> > +static inline int clk_request(struct udevice *dev, struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_free(struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline ulong clk_get_rate(struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline struct clk *clk_get_parent(struct clk *clk) {
> > +	return (struct clk *)-ENOSYS;
> 
> This should use ERR_PTR() to care for platforms defining
> CONFIG_ERR_PTR_OFFSET.

Yes I take the point for V5.

> > +}
> > +
> > +static inline long long clk_get_parent_rate(struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline ulong clk_set_rate(struct clk *clk, ulong rate) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_set_parent(struct clk *clk, struct clk *parent)
> > +{
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_enable(struct clk *clk) {
> > +	return -ENOSYS;
> > +}
> > +
> > +static inline int clk_enable_bulk(struct clk_bulk *bulk) {
> > +	return bulk && bulk->count == 0 ? 0 : -ENOSYS;
> 
> For this test to work, someone would need to set bulk->count to 0. This is normally
> done by clk_get_bulk(), but you defined it to only return an error.
> 
> I guess it works for you because all clk_bulk objects you use are from the heap
> (which is currently zeroed out in U-Boot) or if they are on the stack, you have
> if/else code that doesn't bring you here. Still it seems wrong.

Yes exactly, it is working for me as the bulk was in private date, so zero allocated.

I will update on V5

> Regards,
> Simon

Thanks for the review

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

[Patrick DELAUNAY]

> -----Original Message-----
> From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Sent: mercredi 4 mars 2020 20:52
> To: Patrick DELAUNAY <patrick.delaunay@st.com>; u-boot at lists.denx.de
> Cc: ley.foon.tan at intel.com; b.galvani at gmail.com; Daniel Schwierzeck
> <daniel.schwierzeck@gmail.com>; Marek Vasut <marex@denx.de>; Michal
> Suchanek <msuchanek@suse.de>; Simon Glass <sjg@chromium.org>; U-Boot
> STM32 <uboot-stm32@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@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@gmail.com>

Thanks 
Regards

Patrick