| Message ID | 20191014080025.11245-3-patrick.delaunay@st.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | usb: host: dwc2: use driver model for PHY and CLOCK | expand |
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. [...]
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; }
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(-)