Message ID | 20161024164634.4330-8-ahaslam@baylibre.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote: > From: Axel Haslam <ahaslam@baylibre.com> > > While probing ochi phy with usb20 phy as a parent clock for usb11_phy, > the usb20_phy clock enable would time out. This is because the usb20 > module clock needs to enabled while trying to lock the usb20_phy PLL. > > Call clk enable and get for the usb20 peripheral before trying to > enable the phy PLL. > > Signed-off-by: Axel Haslam <ahaslam@baylibre.com> > --- This patch can be combined with "ARM: davinci: da8xx: add usb phy clocks" since that patch has not been merged yet. If you like, I can resubmit my patches from this series along with the changes from this patch.
On Tue, Oct 25, 2016 at 4:53 AM, David Lechner <david@lechnology.com> wrote: > On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote: >> >> From: Axel Haslam <ahaslam@baylibre.com> >> >> While probing ochi phy with usb20 phy as a parent clock for usb11_phy, >> the usb20_phy clock enable would time out. This is because the usb20 >> module clock needs to enabled while trying to lock the usb20_phy PLL. >> >> Call clk enable and get for the usb20 peripheral before trying to >> enable the phy PLL. >> >> Signed-off-by: Axel Haslam <ahaslam@baylibre.com> >> --- > > > > This patch can be combined with "ARM: davinci: da8xx: add usb phy clocks" > since that patch has not been merged yet. yes, agree, these should be merged. > > If you like, I can resubmit my patches from this series along with the > changes from this patch. Ok, if you can resubmit those patches with this change included i can reference them and start making the series shorter. i will also submit in separate patches the regulator changes, as requested by Mark. Regards Axel. >
On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote: > diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c > index 9e41a7f..982e105 100644 > --- a/arch/arm/mach-davinci/usb-da8xx.c > +++ b/arch/arm/mach-davinci/usb-da8xx.c > @@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate) > > static void usb20_phy_clk_enable(struct clk *clk) > { > + struct clk *usb20_clk; > u32 val; > u32 timeout = 500000; /* 500 msec */ > > val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); > > + usb20_clk = clk_get(NULL, "usb20"); We should not be using a NULL device pointer here. Can you pass the musb device pointer available in the same file? Also, da850_clks[] in da850.c needs to be fixed to add the matching device name. > + if (IS_ERR(usb20_clk)) { > + pr_err("could not get usb20 clk\n"); > + return; > + } Thanks, Sekhar
On 10/25/2016 05:12 AM, Sekhar Nori wrote: > On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote: >> diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c >> index 9e41a7f..982e105 100644 >> --- a/arch/arm/mach-davinci/usb-da8xx.c >> +++ b/arch/arm/mach-davinci/usb-da8xx.c >> @@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate) >> >> static void usb20_phy_clk_enable(struct clk *clk) >> { >> + struct clk *usb20_clk; >> u32 val; >> u32 timeout = 500000; /* 500 msec */ >> >> val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); >> >> + usb20_clk = clk_get(NULL, "usb20"); > > We should not be using a NULL device pointer here. Can you pass the musb > device pointer available in the same file? Also, da850_clks[] in da850.c > needs to be fixed to add the matching device name. This clock can be used for usb 1.1 PHY even when musb is not being used, so I don't think we can depend on having a musb device here. Also, in a previous review, it was decided that the usb clocks should *not* be added to da850_clks[] [1]. Instead, they are dynamically registered elsewhere. [1]: http://www.gossamer-threads.com/lists/linux/kernel/2396533 > >> + if (IS_ERR(usb20_clk)) { >> + pr_err("could not get usb20 clk\n"); >> + return; >> + } > > Thanks, > Sekhar >
On Tuesday 25 October 2016 09:35 PM, David Lechner wrote: > On 10/25/2016 05:12 AM, Sekhar Nori wrote: >> On Monday 24 October 2016 10:16 PM, ahaslam@baylibre.com wrote: >>> diff --git a/arch/arm/mach-davinci/usb-da8xx.c >>> b/arch/arm/mach-davinci/usb-da8xx.c >>> index 9e41a7f..982e105 100644 >>> --- a/arch/arm/mach-davinci/usb-da8xx.c >>> +++ b/arch/arm/mach-davinci/usb-da8xx.c >>> @@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate) >>> >>> static void usb20_phy_clk_enable(struct clk *clk) >>> { >>> + struct clk *usb20_clk; >>> u32 val; >>> u32 timeout = 500000; /* 500 msec */ >>> >>> val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); >>> >>> + usb20_clk = clk_get(NULL, "usb20"); >> >> We should not be using a NULL device pointer here. Can you pass the musb >> device pointer available in the same file? Also, da850_clks[] in da850.c >> needs to be fixed to add the matching device name. > > This clock can be used for usb 1.1 PHY even when musb is not being used, > so I don't think we can depend on having a musb device here. Replied to this against the same question in v6 patch series you posted. > Also, in a previous review, it was decided that the usb clocks should > *not* be added to da850_clks[] [1]. Instead, they are dynamically > registered elsewhere. Thats only the USB phy clocks since there is associated enable()/disable()/set_parent() code which is better kept outside of da850.c for readability. No other reason. Lookup for the USB 2.0 module clock is already present in da850.c Thanks, Sekhar
diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c index 9e41a7f..982e105 100644 --- a/arch/arm/mach-davinci/usb-da8xx.c +++ b/arch/arm/mach-davinci/usb-da8xx.c @@ -53,11 +53,19 @@ int __init da8xx_register_usb_refclkin(int rate) static void usb20_phy_clk_enable(struct clk *clk) { + struct clk *usb20_clk; u32 val; u32 timeout = 500000; /* 500 msec */ val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); + usb20_clk = clk_get(NULL, "usb20"); + if (IS_ERR(usb20_clk)) { + pr_err("could not get usb20 clk\n"); + return; + } + + clk_prepare_enable(usb20_clk); /* * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1 * host may use the PLL clock without USB 2.0 OTG being used. @@ -70,11 +78,14 @@ static void usb20_phy_clk_enable(struct clk *clk) while (--timeout) { val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); if (val & CFGCHIP2_PHYCLKGD) - return; + goto done; udelay(1); } pr_err("Timeout waiting for USB 2.0 PHY clock good.\n"); +done: + clk_disable_unprepare(usb20_clk); + clk_put(usb20_clk); } static void usb20_phy_clk_disable(struct clk *clk)