| Message ID | 1359325055-5160-1-git-send-email-dev@lynxeye.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 01/27/2013 03:17 PM, Lucas Stach wrote: > The USB clocks are just clock gates, so no need to set a specific clock. > In fact trying to set a specific clock is just a NOP if the requested > clockrate is the same as those of the parent (clk_m) or will trigger a > WARN_ON() if rates don't match up. > > As we are not setting a specific rate, nor activating the clocks at > init, there is no point in keeping the the usb entries in the clock init > table. I'm not convinced here; aren't the USB clocks supposed to be driven by PLL U? Prashant, Peter, Venu, can you please comment here. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, den 28.01.2013, 11:37 -0700 schrieb Stephen Warren: > On 01/27/2013 03:17 PM, Lucas Stach wrote: > > The USB clocks are just clock gates, so no need to set a specific clock. > > In fact trying to set a specific clock is just a NOP if the requested > > clockrate is the same as those of the parent (clk_m) or will trigger a > > WARN_ON() if rates don't match up. > > > > As we are not setting a specific rate, nor activating the clocks at > > init, there is no point in keeping the the usb entries in the clock init > > table. > > I'm not convinced here; aren't the USB clocks supposed to be driven by > PLL U? > > Prashant, Peter, Venu, can you please comment here. > I was a bit confused at first, too. But what I am removing here is the clockgate init for the USB controllers. The clocks driven by PLL_U are the USB PHY clocks, which are a separate set of clocks. Regards, Lucas -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/28/2013 11:45 AM, Lucas Stach wrote: > Am Montag, den 28.01.2013, 11:37 -0700 schrieb Stephen Warren: >> On 01/27/2013 03:17 PM, Lucas Stach wrote: >>> The USB clocks are just clock gates, so no need to set a specific clock. >>> In fact trying to set a specific clock is just a NOP if the requested >>> clockrate is the same as those of the parent (clk_m) or will trigger a >>> WARN_ON() if rates don't match up. >>> >>> As we are not setting a specific rate, nor activating the clocks at >>> init, there is no point in keeping the the usb entries in the clock init >>> table. >> >> I'm not convinced here; aren't the USB clocks supposed to be driven by >> PLL U? >> >> Prashant, Peter, Venu, can you please comment here. > > I was a bit confused at first, too. But what I am removing here is the > clockgate init for the USB controllers. The clocks driven by PLL_U are > the USB PHY clocks, which are a separate set of clocks. I don't think these are always separate. If you look at the USB driver drivers/usb/phy/tegra_usb_phy.c, you'll see that for UTMI there's a clk_get_sys("utmip-pad"), which per drivers/clk/tegra/clk-tegra20.c is an alias for clock "usbd" which is the USB1 controller clock (it's also aliased to "tegra-ehci.0"). However, for ULPI, there's a clk_get_sys(ulpi_config->clk), which is "cdev2", which is separate from any USB controller clock. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, den 28.01.2013, 12:13 -0700 schrieb Stephen Warren: > On 01/28/2013 11:45 AM, Lucas Stach wrote: > > Am Montag, den 28.01.2013, 11:37 -0700 schrieb Stephen Warren: > >> On 01/27/2013 03:17 PM, Lucas Stach wrote: > >>> The USB clocks are just clock gates, so no need to set a specific clock. > >>> In fact trying to set a specific clock is just a NOP if the requested > >>> clockrate is the same as those of the parent (clk_m) or will trigger a > >>> WARN_ON() if rates don't match up. > >>> > >>> As we are not setting a specific rate, nor activating the clocks at > >>> init, there is no point in keeping the the usb entries in the clock init > >>> table. > >> > >> I'm not convinced here; aren't the USB clocks supposed to be driven by > >> PLL U? > >> > >> Prashant, Peter, Venu, can you please comment here. > > > > I was a bit confused at first, too. But what I am removing here is the > > clockgate init for the USB controllers. The clocks driven by PLL_U are > > the USB PHY clocks, which are a separate set of clocks. > > I don't think these are always separate. > > If you look at the USB driver drivers/usb/phy/tegra_usb_phy.c, you'll > see that for UTMI there's a clk_get_sys("utmip-pad"), which per > drivers/clk/tegra/clk-tegra20.c is an alias for clock "usbd" which is > the USB1 controller clock (it's also aliased to "tegra-ehci.0"). > However, for ULPI, there's a clk_get_sys(ulpi_config->clk), which is > "cdev2", which is separate from any USB controller clock. > I'm not sure here. The TRM is not really clear on that one, but if you look at the schematic diagram of the USB complex PLL_U is really only used for the PHYs, not the controllers itself. Though I don't know if the controller gets it's clock from the PHY in the UTMI case (like ULPI). So I also would like some NVIDIA clock expert to comment on this. In either case the explicit init isn't needed. Regards, Lucas -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/28/2013 12:23 PM, Lucas Stach wrote: > Am Montag, den 28.01.2013, 12:13 -0700 schrieb Stephen Warren: >> On 01/28/2013 11:45 AM, Lucas Stach wrote: >>> Am Montag, den 28.01.2013, 11:37 -0700 schrieb Stephen Warren: >>>> On 01/27/2013 03:17 PM, Lucas Stach wrote: >>>>> The USB clocks are just clock gates, so no need to set a specific clock. >>>>> In fact trying to set a specific clock is just a NOP if the requested >>>>> clockrate is the same as those of the parent (clk_m) or will trigger a >>>>> WARN_ON() if rates don't match up. >>>>> >>>>> As we are not setting a specific rate, nor activating the clocks at >>>>> init, there is no point in keeping the the usb entries in the clock init >>>>> table. >>>> >>>> I'm not convinced here; aren't the USB clocks supposed to be driven by >>>> PLL U? >>>> >>>> Prashant, Peter, Venu, can you please comment here. >>> >>> I was a bit confused at first, too. But what I am removing here is the >>> clockgate init for the USB controllers. The clocks driven by PLL_U are >>> the USB PHY clocks, which are a separate set of clocks. >> >> I don't think these are always separate. >> >> If you look at the USB driver drivers/usb/phy/tegra_usb_phy.c, you'll >> see that for UTMI there's a clk_get_sys("utmip-pad"), which per >> drivers/clk/tegra/clk-tegra20.c is an alias for clock "usbd" which is >> the USB1 controller clock (it's also aliased to "tegra-ehci.0"). >> However, for ULPI, there's a clk_get_sys(ulpi_config->clk), which is >> "cdev2", which is separate from any USB controller clock. >> > I'm not sure here. The TRM is not really clear on that one, but if you > look at the schematic diagram of the USB complex PLL_U is really only > used for the PHYs, not the controllers itself. Though I don't know if > the controller gets it's clock from the PHY in the UTMI case (like > ULPI). So I also would like some NVIDIA clock expert to comment on this. > In either case the explicit init isn't needed. My suspicion is that the usb* clocks are actually driven by pll_u_out* rather than clk_m as the clock driver currently states. In which case, these initializations are probably intended to have the side-effect of filtering up and configuring pll_u itself. Perhaps the answer is still to remove those entries and replace them with a pll_u initialization. Anyway, hopefully Prashant can shed some light here. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 28, 2013 at 07:37:55PM +0100, Stephen Warren wrote: > On 01/27/2013 03:17 PM, Lucas Stach wrote: > > The USB clocks are just clock gates, so no need to set a specific clock. > > In fact trying to set a specific clock is just a NOP if the requested > > clockrate is the same as those of the parent (clk_m) or will trigger a > > WARN_ON() if rates don't match up. > > > > As we are not setting a specific rate, nor activating the clocks at > > init, there is no point in keeping the the usb entries in the clock init > > table. > > I'm not convinced here; aren't the USB clocks supposed to be driven by > PLL U? > > Prashant, Peter, Venu, can you please comment here. According to our downstream code, they are indeed just clockgates with clk_m as the parent. I couldn't find any confirmation in the TRM though. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c index 5d41569..f08cffc 100644 --- a/drivers/clk/tegra/clk-tegra20.c +++ b/drivers/clk/tegra/clk-tegra20.c @@ -1253,9 +1253,6 @@ static __initdata struct tegra_clk_init_table init_table[] = { {cclk, clk_max, 0, 1}, {uarta, pll_p, 0, 1}, {uartd, pll_p, 0, 1}, - {usbd, clk_max, 12000000, 0}, - {usb2, clk_max, 12000000, 0}, - {usb3, clk_max, 12000000, 0}, {pll_a, clk_max, 56448000, 1}, {pll_a_out0, clk_max, 11289600, 1}, {cdev1, clk_max, 0, 1},
The USB clocks are just clock gates, so no need to set a specific clock. In fact trying to set a specific clock is just a NOP if the requested clockrate is the same as those of the parent (clk_m) or will trigger a WARN_ON() if rates don't match up. As we are not setting a specific rate, nor activating the clocks at init, there is no point in keeping the the usb entries in the clock init table. Signed-off-by: Lucas Stach <dev@lynxeye.de> --- Trace produced by system with 13MHz clk_m: tegra_init_from_table: Failed to set rate 12000000 of usbd ------------[ cut here ]------------ WARNING: at drivers/clk/tegra/clk.c:64 tegra_init_from_table+0xc0/0x158() Modules linked in: [<c0013a84>] (unwind_backtrace+0x0/0xf8) from [<c0021a24>](warn_slowpath_common+0x4c/0x64) [<c0021a24>] (warn_slowpath_common+0x4c/0x64) from [<c0021a58>] (warn_slowpath_null+0x1c/0x24) [<c0021a58>] (warn_slowpath_null+0x1c/0x24) from [<c069343c>] (tegra_init_from_table+0xc0/0x158) [<c069343c>] (tegra_init_from_table+0xc0/0x158) from [<c0694878>] (tegra20_clock_init+0x1398/0x13d4) [<c0694878>] (tegra20_clock_init+0x1398/0x13d4) from [<c0693298>] (of_clk_init+0x30/0x58) [<c0693298>] (of_clk_init+0x30/0x58) from [<c0681e5c>] (tegra_dt_init_irq+0x8/0x1c) [<c0681e5c>] (tegra_dt_init_irq+0x8/0x1c) from [<c067d334>] (init_IRQ+0x14/0x1c) [<c067d334>] (init_IRQ+0x14/0x1c) from [<c067b6b4>] (start_kernel+0x1a0/0x2f8) [<c067b6b4>] (start_kernel+0x1a0/0x2f8) from [<0000807c>] (0x807c) ---[ end trace 1b75b31a2719ed1c ]--- --- drivers/clk/tegra/clk-tegra20.c | 3 --- 1 file changed, 3 deletions(-)