| Message ID | 1468935397-11926-2-git-send-email-mirza.krak@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jul 19, 2016 at 03:36:32PM +0200, Mirza Krak wrote: > From: Mirza Krak <mirza.krak@gmail.com> > > Add TEGRA20_CLK_NOR to init tabel and set a "sane" default rate. > > Signed-off-by: Mirza Krak <mirza.krak@gmail.com> > --- > drivers/clk/tegra/clk-tegra20.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c > index 837e5cb..aefc044 100644 > --- a/drivers/clk/tegra/clk-tegra20.c > +++ b/drivers/clk/tegra/clk-tegra20.c > @@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = { > { TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 }, > { TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 }, > { TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 }, > + { TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 86500000, 0 }, Yay for inconsistent naming in the hardware. It would've been nice if this clock was called GMI. Oh well... Could you perhaps explain in the commit message why 86.5 MHz is a sane default? I'm totally unfamiliar with this controller, so maybe it's self-explanatory, but it seems a rather odd value for a clock frequency. Thierry
2016-07-25 13:17 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>: > On Tue, Jul 19, 2016 at 03:36:32PM +0200, Mirza Krak wrote: >> From: Mirza Krak <mirza.krak@gmail.com> >> >> Add TEGRA20_CLK_NOR to init tabel and set a "sane" default rate. >> >> Signed-off-by: Mirza Krak <mirza.krak@gmail.com> >> --- >> drivers/clk/tegra/clk-tegra20.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c >> index 837e5cb..aefc044 100644 >> --- a/drivers/clk/tegra/clk-tegra20.c >> +++ b/drivers/clk/tegra/clk-tegra20.c >> @@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = { >> { TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 }, >> { TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 }, >> { TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 }, >> + { TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 86500000, 0 }, > > Yay for inconsistent naming in the hardware. It would've been nice if > this clock was called GMI. Oh well... I am allowed to change clk name? Can do that when I re-spin this series. > > Could you perhaps explain in the commit message why 86.5 MHz is a sane > default? I'm totally unfamiliar with this controller, so maybe it's > self-explanatory, but it seems a rather odd value for a clock frequency. I used a value that I have in a downstream kernel based on the L4T. This frequency is well tested and has worked for me, and wanted to avoid setting it to maximum rate. My guess is that they used 86.5 MHz because 92 MHz is max rate on Tegra2 and they put it slightly below that. What is otherwise recommended when initializing clocks? The rate could depend on the chip that is attached but otherwise I would like to set it close to max (or max) rate for performance. Best Regards, Mirza -- 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, Jul 25, 2016 at 02:28:48PM +0200, Mirza Krak wrote: > 2016-07-25 13:17 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>: > > On Tue, Jul 19, 2016 at 03:36:32PM +0200, Mirza Krak wrote: > >> From: Mirza Krak <mirza.krak@gmail.com> > >> > >> Add TEGRA20_CLK_NOR to init tabel and set a "sane" default rate. > >> > >> Signed-off-by: Mirza Krak <mirza.krak@gmail.com> > >> --- > >> drivers/clk/tegra/clk-tegra20.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c > >> index 837e5cb..aefc044 100644 > >> --- a/drivers/clk/tegra/clk-tegra20.c > >> +++ b/drivers/clk/tegra/clk-tegra20.c > >> @@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = { > >> { TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 }, > >> { TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 }, > >> { TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 }, > >> + { TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 86500000, 0 }, > > > > Yay for inconsistent naming in the hardware. It would've been nice if > > this clock was called GMI. Oh well... > > I am allowed to change clk name? Can do that when I re-spin this series. No, leave it as-is. The clock is referred to as NOR in the clock and reset controller. I was just saying that it would've been nice if it had been named GMI consistently. > > Could you perhaps explain in the commit message why 86.5 MHz is a sane > > default? I'm totally unfamiliar with this controller, so maybe it's > > self-explanatory, but it seems a rather odd value for a clock frequency. > > I used a value that I have in a downstream kernel based on the L4T. > This frequency is well tested and has worked for me, and wanted to > avoid setting it to maximum rate. My guess is that they used 86.5 MHz > because 92 MHz is max rate on Tegra2 and they put it slightly below > that. > > What is otherwise recommended when initializing clocks? The rate could > depend on the chip that is attached but otherwise I would like to set > it close to max (or max) rate for performance. I think we usually set the default rate to the maximum and let drivers clock them down as they see fit. This is so that things run fast by default and we often don't have much in the way of power management, at least not when drivers are first merged. Maximum clock rate ensure that we get good performance by default, rather than having to rely on drivers to kick it up a notch. Thierry
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c index 837e5cb..aefc044 100644 --- a/drivers/clk/tegra/clk-tegra20.c +++ b/drivers/clk/tegra/clk-tegra20.c @@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = { { TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 }, { TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 }, { TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 }, + { TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 86500000, 0 }, { TEGRA20_CLK_SBC1, TEGRA20_CLK_PLL_P, 100000000, 0 }, { TEGRA20_CLK_SBC2, TEGRA20_CLK_PLL_P, 100000000, 0 }, { TEGRA20_CLK_SBC3, TEGRA20_CLK_PLL_P, 100000000, 0 },