[U-Boot] [RESEND PATCH 0/5] usb: host: dwc2: use driver model for PHY and CLOCK

[U-Boot] [RESEND PATCH 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
available.

I test this serie on stm32mp157c-ev1 board;
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 theusbotg_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)
    |
    +-3  Mass Storage (480 Mb/s, 500mA)
         Generic  USB Storage

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)



Patrick Delaunay (5):
  usb: host: dwc2: add phy support
  usb: host: dwc2: add support for clk
  usb: host: dwc2: force reset assert
  usb: host: dwc2: add usb33d supply support for stm32mp1
  usb: host: dwc2: add trace to have clean usb start

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

-- 
2.17.1

[U-Boot] [RESEND PATCH 1/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.

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

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

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 350d820a6e..eb1026effc 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <dm.h>
 #include <errno.h>
+#include <generic-phy.h>
 #include <usb.h>
 #include <malloc.h>
 #include <memalign.h>
@@ -35,6 +36,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;
@@ -1320,13 +1322,70 @@ 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\n");
+			return ret;
+		}
+		return 0;
+	}
+
+	ret = generic_phy_init(&priv->phy);
+	if (ret) {
+		dev_err(dev, "failed to init usb phy\n");
+		return ret;
+	}
+
+	ret = generic_phy_power_on(&priv->phy);
+	if (ret) {
+		dev_err(dev, "failed to power on usb phy\n");
+		return generic_phy_exit(&priv->phy);
+	}
+
+	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_err(dev, "failed to power off usb phy\n");
+		return ret;
+	}
+
+	ret = generic_phy_exit(&priv->phy);
+	if (ret) {
+		dev_err(dev, "failed to power off usb phy\n");
+		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);
 }
 
@@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	dwc2_shutdown_phy(dev);
+
 	dwc2_uninit_common(priv->regs);
 
 	reset_release_bulk(&priv->resets);
-- 
2.17.1

[U-Boot] [RESEND PATCH 2/5] usb: host: dwc2: add support for clk

[Patrick Delaunay]

Add support for clock with driver model.

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

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

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index eb1026effc..51023b0c2c 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -5,13 +5,14 @@
  */
 
 #include <common.h>
+#include <clk.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>
@@ -37,6 +38,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;
@@ -1374,6 +1376,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);
@@ -1382,6 +1404,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;
@@ -1403,6 +1429,7 @@ static int dwc2_usb_remove(struct udevice *dev)
 	dwc2_uninit_common(priv->regs);
 
 	reset_release_bulk(&priv->resets);
+	clk_release_bulk(&priv->clks);
 
 	return 0;
 }
-- 
2.17.1

[U-Boot] [RESEND PATCH 3/5] usb: host: dwc2: force reset assert

[Patrick Delaunay]

Assert reset before deassert in dwc2_reset;
It should be more safe for DWC2.

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

 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 51023b0c2c..3086411fc4 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev)
 			return ret;
 	}
 
+	reset_assert_bulk(&priv->resets);
+	udelay(2);
 	ret = reset_deassert_bulk(&priv->resets);
 	if (ret) {
 		reset_release_bulk(&priv->resets);
-- 
2.17.1

[U-Boot] [RESEND PATCH 4/5] usb: host: dwc2: add usb33d supply support for stm32mp1

[Patrick Delaunay]

Enable the usb33d-supply on STM32MP1 SoCs (with "st,stm32mp1-hsotg"
compatible), it is the external VBUS and ID sensing comparators supply
needed to perform OTG operation.

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

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

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 3086411fc4..5b499abded 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -1414,6 +1414,24 @@ static int dwc2_usb_probe(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	if (CONFIG_IS_ENABLED(ARCH_STM32MP) &&
+	    device_is_compatible(dev, "st,stm32mp1-hsotg")) {
+		struct udevice *usb33d_supply;
+
+		ret = device_get_supply_regulator(dev, "usb33d-supply",
+						  &usb33d_supply);
+		if (ret) {
+			dev_err(dev,
+				"can't get voltage level detector supply\n");
+		} else {
+			ret = regulator_set_enable(usb33d_supply, true);
+			if (ret) {
+				dev_err(dev,
+					"can't enable voltage level detector supply\n");
+			}
+		}
+	}
+
 	return dwc2_init_common(dev, priv);
 }
 
-- 
2.17.1

[U-Boot] [RESEND PATCH 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.

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>
---

 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 5b499abded..5e1c78d3ed 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -1217,6 +1217,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

[U-Boot] [RESEND PATCH 1/5] usb: host: dwc2: add phy support

[Marek Vasut]

On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> Use generic phy to initialize the PHY associated to the

PHY and USB are abbreviations, should be in capitals.

> DWC2 device and available in the device tree.

[...]

General question -- is the PHY subsystem a mandatory dependency of this
driver now or will it work without the PHY subsystem still ?

> +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\n");

Sentence starts with capital letter, USB and PHY are in capitals. Fix
globally please.

It would be useful to print the $ret value too.

> +			return ret;
> +		}
> +		return 0;
> +	}
> +
> +	ret = generic_phy_init(&priv->phy);
> +	if (ret) {
> +		dev_err(dev, "failed to init usb phy\n");
> +		return ret;
> +	}
> +
> +	ret = generic_phy_power_on(&priv->phy);
> +	if (ret) {
> +		dev_err(dev, "failed to power on usb phy\n");
> +		return generic_phy_exit(&priv->phy);
> +	}
> +
> +	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_err(dev, "failed to power off usb phy\n");
> +		return ret;
> +	}
> +
> +	ret = generic_phy_exit(&priv->phy);
> +	if (ret) {
> +		dev_err(dev, "failed to power off usb phy\n");

Shouldn't all those error prints be produced by the PHY subsystem ?

> +		return ret;

[...]

> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
>  	if (ret)
>  		return ret;
>  
> +	dwc2_shutdown_phy(dev);

This function returns a return value, but it's ignored here ?

>  	dwc2_uninit_common(priv->regs);
>  
>  	reset_release_bulk(&priv->resets);
> 

[...]

[U-Boot] [RESEND PATCH 2/5] usb: host: dwc2: add support for clk

[Marek Vasut]

On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> Add support for clock with driver model.
> 

Same question as with the PHY -- is there now a mandatory dependency on
the DM CLK ?

[...]

> @@ -1403,6 +1429,7 @@ static int dwc2_usb_remove(struct udevice *dev)
>  	dwc2_uninit_common(priv->regs);
>  
>  	reset_release_bulk(&priv->resets);
> +	clk_release_bulk(&priv->clks);

Shouldn't there be some clk_...disable() here ?

[U-Boot] [RESEND PATCH 3/5] usb: host: dwc2: force reset assert

[Marek Vasut]

On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> Assert reset before deassert in dwc2_reset;
> It should be more safe for DWC2.

Can you be more descriptive about this issue ? I have no idea what this
patch does or fixes from the description.

> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> 
>  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 51023b0c2c..3086411fc4 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev)
>  			return ret;
>  	}
>  
> +	reset_assert_bulk(&priv->resets);
> +	udelay(2);

Why is there a 2 uS delay ?

>  	ret = reset_deassert_bulk(&priv->resets);
>  	if (ret) {
>  		reset_release_bulk(&priv->resets);
> 

[...]

[U-Boot] [RESEND PATCH 4/5] usb: host: dwc2: add usb33d supply support for stm32mp1

[Marek Vasut]

On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> Enable the usb33d-supply on STM32MP1 SoCs (with "st,stm32mp1-hsotg"
> compatible), it is the external VBUS and ID sensing comparators supply
> needed to perform OTG operation.

I suspect we might need some dwc2-stm32p1.c SoC-specific driver here.
Adding SoC-specific stuff into common driver doesn't sound right.

[U-Boot] [RESEND PATCH 1/5] usb: host: dwc2: add phy support

[Patrick DELAUNAY]

Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: mardi 15 octobre 2019 01:27

First sorry for my late answer....
 
> On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> > Use generic phy to initialize the PHY associated to the
> 
> PHY and USB are abbreviations, should be in capitals.
> 
> > DWC2 device and available in the device tree.
> 
> [...]
> 
> General question -- is the PHY subsystem a mandatory dependency of this driver
> now or will it work without the PHY subsystem still ?

Normally it is working as all the generic_phy_XXX fucntions
are stubbed in include/generic-phy.h

- generic_phy_get_by_index() return 0 and phy->dev = NULL
- all other function return 0
- generic_phy_valid return FALSE (phy->dev = NULL)

 
> > +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\n");
> 
> Sentence starts with capital letter, USB and PHY are in capitals. Fix globally
> please.
> It would be useful to print the $ret value too.

Yes, in V2

> 
> > +			return ret;
> > +		}
> > +		return 0;
> > +	}
> > +
> > +	ret = generic_phy_init(&priv->phy);
> > +	if (ret) {
> > +		dev_err(dev, "failed to init usb phy\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = generic_phy_power_on(&priv->phy);
> > +	if (ret) {
> > +		dev_err(dev, "failed to power on usb phy\n");
> > +		return generic_phy_exit(&priv->phy);
> > +	}
> > +
> > +	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_err(dev, "failed to power off usb phy\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = generic_phy_exit(&priv->phy);
> > +	if (ret) {
> > +		dev_err(dev, "failed to power off usb phy\n");
> 
> Shouldn't all those error prints be produced by the PHY subsystem ?

Perhaps... but it is not done today in phy u-class (only call ops).

I make the same level of trace than ./drivers/usb/dwc3/core.c
as copy initially the phy support from this driver.

> > +		return ret;
> 
> [...]
> 
> > @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
> >  	if (ret)
> >  		return ret;
> >
> > +	dwc2_shutdown_phy(dev);
> 
> This function returns a return value, but it's ignored here ?

Yes, even if the shutdown of the USB PHY failed, the USB dwc2
 driver continues the procedure to release other ressources...

And the driver expects that the USB PHY will be available for next
request/probe (recovery with phy reset for example).

I use the same logic than dwc3 driver in :
source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove()
drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove()

> 
> >  	dwc2_uninit_common(priv->regs);
> >
> >  	reset_release_bulk(&priv->resets);
> >
> 
> [...]

Regards
Patrick

[U-Boot] [RESEND PATCH 2/5] usb: host: dwc2: add support for clk

[Patrick DELAUNAY]

Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: mardi 15 octobre 2019 01:28
> 
> On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> > Add support for clock with driver model.
> >
> 
> Same question as with the PHY -- is there now a mandatory dependency on the
> DM CLK ?

No I don't think.

Because the clk function are also stubbed in ./include/clk.h
CONFIG_IS_ENABLED(CLK)

But I don't 100% sure as I don't tested it on one platform without DM_CLK...
 
> [...]
> 
> > @@ -1403,6 +1429,7 @@ static int dwc2_usb_remove(struct udevice *dev)
> >  	dwc2_uninit_common(priv->regs);
> >
> >  	reset_release_bulk(&priv->resets);
> > +	clk_release_bulk(&priv->clks);
> 
> Shouldn't there be some clk_...disable() here ?

I don't like make clk_....disable() in U-Boot remove function because the clock
u-class don't managed a counter for each clock user (as it is done in kernel).

We have always a risk to deactivate a clock needed by a several device:
each driver (A&B) enable a common clock with U-Boot clock function, 
but the first clock disable (A) really deactivate the clock even it is still needed
by the other driver (B)

I use the same logical than dwc3 driver: clk_disable_bulk is not called.

static int dwc3_glue_remove(struct udevice *dev)
{
	struct dwc3_glue_data *glue = dev_get_platdata(dev);

	reset_release_bulk(&glue->resets);

	clk_release_bulk(&glue->clks);

	return 0;
}

Regards
Patrick

[U-Boot] [RESEND PATCH 3/5] usb: host: dwc2: force reset assert

[Patrick DELAUNAY]

Hi,

> From: Marek Vasut <marex@denx.de>
> Sent: mardi 15 octobre 2019 01:30
> 
> On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> > Assert reset before deassert in dwc2_reset; It should be more safe for
> > DWC2.
> 
> Can you be more descriptive about this issue ? I have no idea what this patch does
> or fixes from the description.

Yes

I will explain it in V2 commit message.

The issue only occurs if the DWC2 OTG device switch between gadget mode 
and host mode.

For example: 
some registers initialiaze by the command "ums" (device mode is forced for example),
cause problem for the next command "usb start" and vice versa.

Even the existing  software reset in dwc_otg_core_reset is not enough;
the added hardware reset solve all the issues.
 
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> >
> >  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
> > 51023b0c2c..3086411fc4 100644
> > --- a/drivers/usb/host/dwc2.c
> > +++ b/drivers/usb/host/dwc2.c
> > @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev)
> >  			return ret;
> >  	}
> >
> > +	reset_assert_bulk(&priv->resets);
> > +	udelay(2);
> 
> Why is there a 2 uS delay ?

I think: no real reason to have 2 us....

It was jus a reasonable time to be sure that the device reset is correctly
performed, the reset signal is propagated....

but perhaps that no delay is working...
I can test without delay if you prefer...

PS: I use the same value than DWC2 gadget driver:
       Added by my commit c2c74f97afff

static int dwc2_udc_otg_reset_init(struct udevice *dev,
				   struct reset_ctl_bulk *resets)
{
.....
	ret = reset_assert_bulk(resets);

	if (!ret) {
		udelay(2);
		ret = reset_deassert_bulk(resets);
	}
....
}

 
> >  	ret = reset_deassert_bulk(&priv->resets);
> >  	if (ret) {
> >  		reset_release_bulk(&priv->resets);
> >
> 
> [...]

Regards
Patrick

[U-Boot] [RESEND PATCH 4/5] usb: host: dwc2: add usb33d supply support for stm32mp1

[Patrick DELAUNAY]

Hi,

> 
> On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> > Enable the usb33d-supply on STM32MP1 SoCs (with "st,stm32mp1-hsotg"
> > compatible), it is the external VBUS and ID sensing comparators supply
> > needed to perform OTG operation.
> 
> I suspect we might need some dwc2-stm32p1.c SoC-specific driver here.
> Adding SoC-specific stuff into common driver doesn't sound right.

Yes, you are right... I perhaps  need to rework this patch.

Today I will drop this part in the V2 patchset.
I will resubmit a other patch later for this part because I need to cross-checks with Linux driver....

This stm32mp1 specific part also exist in our dwc2 kernel driver but I need to check if it can be upstreamed (modification in binding dwc2 is acceptable).

Patrick

[U-Boot] [RESEND PATCH 1/5] usb: host: dwc2: add phy support

[Marek Vasut]

On 11/6/19 6:40 PM, Patrick DELAUNAY wrote:
> Hi Marek,

Hi,

[...]

>>> +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_err(dev, "failed to power off usb phy\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = generic_phy_exit(&priv->phy);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to power off usb phy\n");
>>
>> Shouldn't all those error prints be produced by the PHY subsystem ?
> 
> Perhaps... but it is not done today in phy u-class (only call ops).
> 
> I make the same level of trace than ./drivers/usb/dwc3/core.c
> as copy initially the phy support from this driver.

So this starts the duplication. Can you move it to the PHY subsystem
instead ?

>>> +		return ret;
>>
>> [...]
>>
>>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
>>>  	if (ret)
>>>  		return ret;
>>>
>>> +	dwc2_shutdown_phy(dev);
>>
>> This function returns a return value, but it's ignored here ?
> 
> Yes, even if the shutdown of the USB PHY failed, the USB dwc2
>  driver continues the procedure to release other ressources...

How can you safely release the rest of the resources if the PHY driver
didn't shut down? I suspect this might lead to some resource corruption, no?

> And the driver expects that the USB PHY will be available for next
> request/probe (recovery with phy reset for example).
> 
> I use the same logic than dwc3 driver in :
> source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove()
> drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove()

dwc3_shutdown_phy() only ever returns 0 though.

-- 
Best regards,
Marek Vasut

[U-Boot] [RESEND PATCH 2/5] usb: host: dwc2: add support for clk

[Marek Vasut]

On 11/6/19 7:03 PM, Patrick DELAUNAY wrote:
> Hi Marek,

Hi,

[...]

>> Same question as with the PHY -- is there now a mandatory dependency on the
>> DM CLK ?
> 
> No I don't think.
> 
> Because the clk function are also stubbed in ./include/clk.h
> CONFIG_IS_ENABLED(CLK)
> 
> But I don't 100% sure as I don't tested it on one platform without DM_CLK...

SoCFPGA is one of those, so +CC Simon.

>> [...]
>>
>>> @@ -1403,6 +1429,7 @@ static int dwc2_usb_remove(struct udevice *dev)
>>>  	dwc2_uninit_common(priv->regs);
>>>
>>>  	reset_release_bulk(&priv->resets);
>>> +	clk_release_bulk(&priv->clks);
>>
>> Shouldn't there be some clk_...disable() here ?
> 
> I don't like make clk_....disable() in U-Boot remove function because the clock
> u-class don't managed a counter for each clock user (as it is done in kernel).
> 
> We have always a risk to deactivate a clock needed by a several device:
> each driver (A&B) enable a common clock with U-Boot clock function, 
> but the first clock disable (A) really deactivate the clock even it is still needed
> by the other driver (B)

But if you don't disable the clock in .remove callback, the clock are
left running and that might cause other problems.

Are there such systems which share single clock enable bit between
multiple DWC2 IPs ?

> I use the same logical than dwc3 driver: clk_disable_bulk is not called.

I suspect that driver might need fixing.

[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [RESEND PATCH 3/5] usb: host: dwc2: force reset assert

[Marek Vasut]

On 11/6/19 7:27 PM, Patrick DELAUNAY wrote:
> Hi,

Hi,

[...]

>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index
>>> 51023b0c2c..3086411fc4 100644
>>> --- a/drivers/usb/host/dwc2.c
>>> +++ b/drivers/usb/host/dwc2.c
>>> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev)
>>>  			return ret;
>>>  	}
>>>
>>> +	reset_assert_bulk(&priv->resets);
>>> +	udelay(2);
>>
>> Why is there a 2 uS delay ?
> 
> I think: no real reason to have 2 us....
> 
> It was jus a reasonable time to be sure that the device reset is correctly
> performed, the reset signal is propagated....
> 
> but perhaps that no delay is working...
> I can test without delay if you prefer...
> 
> PS: I use the same value than DWC2 gadget driver:
>        Added by my commit c2c74f97afff

Isn't there a way to poll the IP to determine whether the reset completed ?

[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [RESEND PATCH 3/5] usb: host: dwc2: force reset assert

[Patrick DELAUNAY]

Hi,

> From: Marek Vasut <marex@denx.de>
> Sent: mercredi 6 novembre 2019 23:00
> 
> On 11/6/19 7:27 PM, Patrick DELAUNAY wrote:
> > Hi,
> 
> Hi,
> 
> [...]
> 
> >>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index
> >>> 51023b0c2c..3086411fc4 100644
> >>> --- a/drivers/usb/host/dwc2.c
> >>> +++ b/drivers/usb/host/dwc2.c
> >>> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev)
> >>>  			return ret;
> >>>  	}
> >>>
> >>> +	reset_assert_bulk(&priv->resets);
> >>> +	udelay(2);
> >>
> >> Why is there a 2 uS delay ?
> >
> > I think: no real reason to have 2 us....
> >
> > It was jus a reasonable time to be sure that the device reset is
> > correctly performed, the reset signal is propagated....
> >
> > but perhaps that no delay is working...
> > I can test without delay if you prefer...
> >
> > PS: I use the same value than DWC2 gadget driver:
> >        Added by my commit c2c74f97afff
> 
> Isn't there a way to poll the IP to determine whether the reset completed ?

It is HW IP reset, the complete state is not available for stm32mp1 reset controller (RCC).
And the need reset duration of depends on each IP (can't be handle in reset u-class).

I check with DWC2 OTG IP expert, and we found in DWC_otg_databook_3.30a.pdf

t_rst: DWC_otg PHY clock domain reset and AHB hclk domain reset over lap time
        (a minimum of 12 cycles of the slowest clock is recommended.)

In our board, we have 209MHz for AHB frequency... USB phy clock is 48MHz
So freq12 cycles is MIN(57ns,  250ns) < 1us.

The 2us value seens a over protection.

I will reduce it to 1us in V2 and I will add comments for.

> [...]
> 
> --
> Best regards,
> Marek Vasut

Regards

[U-Boot] [RESEND PATCH 3/5] usb: host: dwc2: force reset assert

[Marek Vasut]

On 11/8/19 10:53 AM, Patrick DELAUNAY wrote:
> Hi,

Hi,

[...]

>>>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index
>>>>> 51023b0c2c..3086411fc4 100644
>>>>> --- a/drivers/usb/host/dwc2.c
>>>>> +++ b/drivers/usb/host/dwc2.c
>>>>> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev)
>>>>>  			return ret;
>>>>>  	}
>>>>>
>>>>> +	reset_assert_bulk(&priv->resets);
>>>>> +	udelay(2);
>>>>
>>>> Why is there a 2 uS delay ?
>>>
>>> I think: no real reason to have 2 us....
>>>
>>> It was jus a reasonable time to be sure that the device reset is
>>> correctly performed, the reset signal is propagated....
>>>
>>> but perhaps that no delay is working...
>>> I can test without delay if you prefer...
>>>
>>> PS: I use the same value than DWC2 gadget driver:
>>>        Added by my commit c2c74f97afff
>>
>> Isn't there a way to poll the IP to determine whether the reset completed ?
> 
> It is HW IP reset, the complete state is not available for stm32mp1 reset controller (RCC).
> And the need reset duration of depends on each IP (can't be handle in reset u-class).

If it's a SoC specific delay, it should be in the reset driver.

> I check with DWC2 OTG IP expert, and we found in DWC_otg_databook_3.30a.pdf
> 
> t_rst: DWC_otg PHY clock domain reset and AHB hclk domain reset over lap time
>         (a minimum of 12 cycles of the slowest clock is recommended.)
> 
> In our board, we have 209MHz for AHB frequency... USB phy clock is 48MHz
> So freq12 cycles is MIN(57ns,  250ns) < 1us.
> 
> The 2us value seens a over protection.
> 
> I will reduce it to 1us in V2 and I will add comments for.

Well, why don't you put this into the reset driver ? Seems to be a more
fitting place for this. I don't think every single SoC has the same
clock settings.

[U-Boot] [RESEND PATCH 3/5] usb: host: dwc2: force reset assert

[Patrick DELAUNAY]

Hi,

> 
> On 11/8/19 10:53 AM, Patrick DELAUNAY wrote:
> > Hi,
> 
> Hi,
> 
> [...]
> 
> >>>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> >>>>> index
> >>>>> 51023b0c2c..3086411fc4 100644
> >>>>> --- a/drivers/usb/host/dwc2.c
> >>>>> +++ b/drivers/usb/host/dwc2.c
> >>>>> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev)
> >>>>>  			return ret;
> >>>>>  	}
> >>>>>
> >>>>> +	reset_assert_bulk(&priv->resets);
> >>>>> +	udelay(2);
> >>>>
> >>>> Why is there a 2 uS delay ?
> >>>
> >>> I think: no real reason to have 2 us....
> >>>
> >>> It was jus a reasonable time to be sure that the device reset is
> >>> correctly performed, the reset signal is propagated....
> >>>
> >>> but perhaps that no delay is working...
> >>> I can test without delay if you prefer...
> >>>
> >>> PS: I use the same value than DWC2 gadget driver:
> >>>        Added by my commit c2c74f97afff
> >>
> >> Isn't there a way to poll the IP to determine whether the reset completed ?
> >
> > It is HW IP reset, the complete state is not available for stm32mp1 reset
> controller (RCC).
> > And the need reset duration of depends on each IP (can't be handle in reset u-
> class).
> 
> If it's a SoC specific delay, it should be in the reset driver.
> 
> > I check with DWC2 OTG IP expert, and we found in
> > DWC_otg_databook_3.30a.pdf
> >
> > t_rst: DWC_otg PHY clock domain reset and AHB hclk domain reset over lap
> time
> >         (a minimum of 12 cycles of the slowest clock is recommended.)
> >
> > In our board, we have 209MHz for AHB frequency... USB phy clock is
> > 48MHz So freq12 cycles is MIN(57ns,  250ns) < 1us.
> >
> > The 2us value seens a over protection.
> >
> > I will reduce it to 1us in V2 and I will add comments for.
> 
> Well, why don't you put this into the reset driver ? Seems to be a more fitting place
> for this. I don't think every single SoC has the same clock settings.

Ok, I will remove the delay in driver.

Patrick

[U-Boot] [RESEND PATCH 3/5] usb: host: dwc2: force reset assert

[Marek Vasut]

On 11/8/19 11:51 AM, Patrick DELAUNAY wrote:
[...]
>>>> Isn't there a way to poll the IP to determine whether the reset completed ?
>>>
>>> It is HW IP reset, the complete state is not available for stm32mp1 reset
>> controller (RCC).
>>> And the need reset duration of depends on each IP (can't be handle in reset u-
>> class).
>>
>> If it's a SoC specific delay, it should be in the reset driver.
>>
>>> I check with DWC2 OTG IP expert, and we found in
>>> DWC_otg_databook_3.30a.pdf
>>>
>>> t_rst: DWC_otg PHY clock domain reset and AHB hclk domain reset over lap
>> time
>>>         (a minimum of 12 cycles of the slowest clock is recommended.)
>>>
>>> In our board, we have 209MHz for AHB frequency... USB phy clock is
>>> 48MHz So freq12 cycles is MIN(57ns,  250ns) < 1us.
>>>
>>> The 2us value seens a over protection.
>>>
>>> I will reduce it to 1us in V2 and I will add comments for.
>>
>> Well, why don't you put this into the reset driver ? Seems to be a more fitting place
>> for this. I don't think every single SoC has the same clock settings.
> 
> Ok, I will remove the delay in driver.

Does it make sense to put it into the reset driver though ?

[U-Boot] [RESEND PATCH 1/5] usb: host: dwc2: add phy support

[Patrick DELAUNAY]

Hi,

> From: Marek Vasut <marex@denx.de>
> 
> On 11/6/19 6:40 PM, Patrick DELAUNAY wrote:
> > Hi Marek,
> 
> Hi,
> 
> [...]
> 
> >>> +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_err(dev, "failed to power off usb phy\n");
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	ret = generic_phy_exit(&priv->phy);
> >>> +	if (ret) {
> >>> +		dev_err(dev, "failed to power off usb phy\n");
> >>
> >> Shouldn't all those error prints be produced by the PHY subsystem ?
> >
> > Perhaps... but it is not done today in phy u-class (only call ops).
> >
> > I make the same level of trace than ./drivers/usb/dwc3/core.c as copy
> > initially the phy support from this driver.
> 
> So this starts the duplication. Can you move it to the PHY subsystem instead ?

Yes I can, in v2 I will change dev_err to dev_dbg

And I will sent a other serie to change the generic phy (add printf or dev_err) 
and also remove the dev_err for all the caller to avoid duplicated trace.

This generic error is already done in some U-Boot uclass,
- clock (clk_enable)

But sometime only the caller, the driver,  knows if it is a error or a warning,
and it is not done for others uclass, for example:

- Reset: reset_assert/ reset_deassert reset_assert_bulk/ reset_deassert_bulk
- Regulator: regulator_set_enable

> >>> +		return ret;
> >>
> >> [...]
> >>
> >>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
> >>>  	if (ret)
> >>>  		return ret;
> >>>
> >>> +	dwc2_shutdown_phy(dev);
> >>
> >> This function returns a return value, but it's ignored here ?
> >
> > Yes, even if the shutdown of the USB PHY failed, the USB dwc2  driver
> > continues the procedure to release other ressources...
> 
> How can you safely release the rest of the resources if the PHY driver didn't shut
> down? I suspect this might lead to some resource corruption, no?

Yes...and that depends of the PHY driver.

What it is better stategy:
- try to continue to release the resources after the first error and the next probe could works / the error is masked
Or
- stopped the release procedure => the next procedure could failed (resource not available)

> > And the driver expects that the USB PHY will be available for next
> > request/probe (recovery with phy reset for example).
> >
> > I use the same logic than dwc3 driver in :
> > source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove()
> > drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove()
> 
> dwc3_shutdown_phy() only ever returns 0 though.

Yes, but in dwc3_shutdown_phy, the phy operation can have errors
and the "remove" procedure continue (even if ret is never retruned)

ret = generic_phy_power_off(&usb_phys[i]);
ret |= generic_phy_exit(&usb_phys[i]);
if (ret) {
	pr_err("Can't shutdown USB PHY%d for %s\n", i, dev->name);
}

Anyway I will treat error in v2, it should be more clear in dw2c code.

+	ret= dwc2_shutdown_phy(dev);
+	if (ret) {
+		dev_dbg(dev, "Failed to shutdown USB PHY: %d.\n": ret);
+		return ret;
+	}

> --
> Best regards,
> Marek Vasut

Regards

Patrick

[U-Boot] [RESEND PATCH 1/5] usb: host: dwc2: add phy support

[Marek Vasut]

On 11/8/19 2:25 PM, Patrick DELAUNAY wrote:
Hi,

[...]
>>>>> +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_err(dev, "failed to power off usb phy\n");
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	ret = generic_phy_exit(&priv->phy);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "failed to power off usb phy\n");
>>>>
>>>> Shouldn't all those error prints be produced by the PHY subsystem ?
>>>
>>> Perhaps... but it is not done today in phy u-class (only call ops).
>>>
>>> I make the same level of trace than ./drivers/usb/dwc3/core.c as copy
>>> initially the phy support from this driver.
>>
>> So this starts the duplication. Can you move it to the PHY subsystem instead ?
> 
> Yes I can, in v2 I will change dev_err to dev_dbg
> 
> And I will sent a other serie to change the generic phy (add printf or dev_err) 
> and also remove the dev_err for all the caller to avoid duplicated trace.

Thanks

[...]

>>>>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>
>>>>> +	dwc2_shutdown_phy(dev);
>>>>
>>>> This function returns a return value, but it's ignored here ?
>>>
>>> Yes, even if the shutdown of the USB PHY failed, the USB dwc2  driver
>>> continues the procedure to release other ressources...
>>
>> How can you safely release the rest of the resources if the PHY driver didn't shut
>> down? I suspect this might lead to some resource corruption, no?
> 
> Yes...and that depends of the PHY driver.

Does it ?

> What it is better stategy:
> - try to continue to release the resources after the first error and the next probe could works / the error is masked
> Or
> - stopped the release procedure => the next procedure could failed (resource not available)

Stop the release procedure, because then at least the dynamically
allocated data are still tracked by allocator and if the driver so
decided to use them, they will still exist. If you were to release them,
then the driver could either misbehave and/or cause memory corruption.

>>> And the driver expects that the USB PHY will be available for next
>>> request/probe (recovery with phy reset for example).
>>>
>>> I use the same logic than dwc3 driver in :
>>> source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove()
>>> drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove()
>>
>> dwc3_shutdown_phy() only ever returns 0 though.
> 
> Yes, but in dwc3_shutdown_phy, the phy operation can have errors
> and the "remove" procedure continue (even if ret is never retruned)
> 
> ret = generic_phy_power_off(&usb_phys[i]);
> ret |= generic_phy_exit(&usb_phys[i]);
> if (ret) {
> 	pr_err("Can't shutdown USB PHY%d for %s\n", i, dev->name);
> }
> 
> Anyway I will treat error in v2, it should be more clear in dw2c code.
That doesn't sound right.

[...]