[U-Boot] [RESEND PATCH 0/5] usb: host: dwc2: use driver model for PHY and CLOCK
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@49000000: USB DWC2 Bus usbh-ehci@5800d000: USB EHCI 1.00 scanning bus usb-otg@49000000 for devices... 2 USB Device(s) found scanning bus usbh-ehci@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(-)
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);
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);
[...]
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
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.
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
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.
[...]
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; }
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 ?
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
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.
[...]
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);
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);
[...]
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
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 ?
[...]
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
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.
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
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 ?
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); }
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.
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
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@49000000: Bus usbh-ehci@5800d000: USB EHCI 1.00
after the patch:
STM32MP> usb start starting USB... Bus usb-otg@49000000: USB DWC2 Bus usbh-ehci@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(®s->gintsts) & DWC2_GINTSTS_CURMODE_HOST) mdelay(1000);
+ printf("USB DWC2\n"); + return 0; }
participants (3)
-
Marek Vasut -
Patrick DELAUNAY -
Patrick Delaunay