| Message ID | 1321546766-26770-4-git-send-email-pdeschrijver@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, A nit and two comments below. On Thu, Nov 17, 2011 at 06:19:17PM +0200, Peter De Schrijver wrote: > Rework the tegra20 clock code to support multiple tegra variants : > > * remove tegra2_periph_reset_assert/tegra2_periph_reset_deassert. This > functionality should be in clock.c. > * compile tegra_sdmmc_tap_delay only on tegra20 as this feature will not > be available in future variants. > * don't export clk_measure_input_freq as its functionality is also available > using clk_get_rate(). > > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> > --- > arch/arm/mach-tegra/clock.c | 14 +++++++++----- > arch/arm/mach-tegra/clock.h | 3 --- > arch/arm/mach-tegra/tegra2_clocks.c | 14 +------------- > arch/arm/mach-tegra/timer.c | 12 ++++++++---- > 4 files changed, 18 insertions(+), 25 deletions(-) > > diff --git a/arch/arm/mach-tegra/clock.c b/arch/arm/mach-tegra/clock.c > index f8d41ff..47f6366 100644 > --- a/arch/arm/mach-tegra/clock.c > +++ b/arch/arm/mach-tegra/clock.c > @@ -387,13 +387,15 @@ EXPORT_SYMBOL(tegra_clk_init_from_table); > > void tegra_periph_reset_deassert(struct clk *c) > { > - tegra2_periph_reset_deassert(c); > + BUG_ON(!c->ops->reset); > + c->ops->reset(c, false); > } > EXPORT_SYMBOL(tegra_periph_reset_deassert); > > void tegra_periph_reset_assert(struct clk *c) > { > - tegra2_periph_reset_assert(c); > + BUG_ON(!c->ops->reset); > + c->ops->reset(c, true); > } > EXPORT_SYMBOL(tegra_periph_reset_assert); > > @@ -403,10 +405,11 @@ void __init tegra_init_clock(void) > } > > /* > - * The SDMMC controllers have extra bits in the clock source register that > - * adjust the delay between the clock and data to compenstate for delays > - * on the PCB. > + * The SDMMC controllers on tegra20 have extra bits in the clock source > + * register that adjust the delay between the clock and data to compenstate > + * for delays on the PCB. > */ > +#ifdef CONFIG_ARCH_TEGRA_2x_SOC > void tegra_sdmmc_tap_delay(struct clk *c, int delay) > { > unsigned long flags; > @@ -415,6 +418,7 @@ void tegra_sdmmc_tap_delay(struct clk *c, int delay) > tegra2_sdmmc_tap_delay(c, delay); > spin_unlock_irqrestore(&c->spinlock, flags); > } > +#endif Ifdeffing this out doesn't quite make sense. Better to do a #ifdef in the include file with an #else case that fills in an empty function. This needs to be abstracted differently for the two platforms anyway but that can be done separately from this. Does tegra3 have tap delay setup as well? (I don't have the TRM handy right now). > > diff --git a/arch/arm/mach-tegra/clock.h b/arch/arm/mach-tegra/clock.h > index 688316a..18df129 100644 > --- a/arch/arm/mach-tegra/clock.h > +++ b/arch/arm/mach-tegra/clock.h > @@ -146,11 +146,8 @@ struct tegra_clk_init_table { > }; > > void tegra2_init_clocks(void); > -void tegra2_periph_reset_deassert(struct clk *c); > -void tegra2_periph_reset_assert(struct clk *c); > void clk_init(struct clk *clk); > struct clk *tegra_get_clock_by_name(const char *name); > -unsigned long clk_measure_input_freq(void); This should maybe be done as a separate change, but don't worry about it this time around. > int clk_reparent(struct clk *c, struct clk *parent); > void tegra_clk_init_from_table(struct tegra_clk_init_table *table); > unsigned long clk_get_rate_locked(struct clk *c); > diff --git a/arch/arm/mach-tegra/tegra2_clocks.c b/arch/arm/mach-tegra/tegra2_clocks.c > index 371869d..2ab18f6 100644 > --- a/arch/arm/mach-tegra/tegra2_clocks.c > +++ b/arch/arm/mach-tegra/tegra2_clocks.c > @@ -174,7 +174,7 @@ static int tegra_periph_clk_enable_refcount[3 * 32]; > #define pmc_readl(reg) \ > __raw_readl(reg_pmc_base + (reg)) > > -unsigned long clk_measure_input_freq(void) > +static unsigned long clk_measure_input_freq(void) This has only a single user left, so it can be folded down in that function instead, especially since there's just two switch statements. That can be done in a separate change. -Olof -- 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
Olof Johansson wrote at Friday, November 18, 2011 12:07 PM: > On Thu, Nov 17, 2011 at 06:19:17PM +0200, Peter De Schrijver wrote: > > Rework the tegra20 clock code to support multiple tegra variants : > > > > * remove tegra2_periph_reset_assert/tegra2_periph_reset_deassert. This > > functionality should be in clock.c. > > * compile tegra_sdmmc_tap_delay only on tegra20 as this feature will not > > be available in future variants. > > * don't export clk_measure_input_freq as its functionality is also available > > using clk_get_rate(). ... > > @@ -403,10 +405,11 @@ void __init tegra_init_clock(void) > > } > > > > /* > > - * The SDMMC controllers have extra bits in the clock source register that > > - * adjust the delay between the clock and data to compenstate for delays > > - * on the PCB. > > + * The SDMMC controllers on tegra20 have extra bits in the clock source > > + * register that adjust the delay between the clock and data to compenstate > > + * for delays on the PCB. > > */ > > +#ifdef CONFIG_ARCH_TEGRA_2x_SOC > > void tegra_sdmmc_tap_delay(struct clk *c, int delay) > > { > > unsigned long flags; > > @@ -415,6 +418,7 @@ void tegra_sdmmc_tap_delay(struct clk *c, int delay) > > tegra2_sdmmc_tap_delay(c, delay); > > spin_unlock_irqrestore(&c->spinlock, flags); > > } > > +#endif > > Ifdeffing this out doesn't quite make sense. Better to do a #ifdef in the > include file with an #else case that fills in an empty function. This > needs to be abstracted differently for the two platforms anyway but > that can be done separately from this. Does tegra3 have tap delay setup > as well? (I don't have the TRM handy right now). I vote just rip this function out altogether. It isn't used in mainline, chromeos-2.6.38, chromeos-3.0, nor our internal 2.6.36 or 2.6.39 kernels. Equally, I can't find any documentation of the register bits this code touches in either Tegra20 or Tegra30 TRMs.
On Fri, Nov 18, 2011 at 12:18 PM, Stephen Warren <swarren@nvidia.com> wrote: > Olof Johansson wrote at Friday, November 18, 2011 12:07 PM: >> On Thu, Nov 17, 2011 at 06:19:17PM +0200, Peter De Schrijver wrote: >> > Rework the tegra20 clock code to support multiple tegra variants : >> > >> > * remove tegra2_periph_reset_assert/tegra2_periph_reset_deassert. This >> > functionality should be in clock.c. >> > * compile tegra_sdmmc_tap_delay only on tegra20 as this feature will not >> > be available in future variants. >> > * don't export clk_measure_input_freq as its functionality is also available >> > using clk_get_rate(). > ... >> > @@ -403,10 +405,11 @@ void __init tegra_init_clock(void) >> > } >> > >> > /* >> > - * The SDMMC controllers have extra bits in the clock source register that >> > - * adjust the delay between the clock and data to compenstate for delays >> > - * on the PCB. >> > + * The SDMMC controllers on tegra20 have extra bits in the clock source >> > + * register that adjust the delay between the clock and data to compenstate >> > + * for delays on the PCB. >> > */ >> > +#ifdef CONFIG_ARCH_TEGRA_2x_SOC >> > void tegra_sdmmc_tap_delay(struct clk *c, int delay) >> > { >> > unsigned long flags; >> > @@ -415,6 +418,7 @@ void tegra_sdmmc_tap_delay(struct clk *c, int delay) >> > tegra2_sdmmc_tap_delay(c, delay); >> > spin_unlock_irqrestore(&c->spinlock, flags); >> > } >> > +#endif >> >> Ifdeffing this out doesn't quite make sense. Better to do a #ifdef in the >> include file with an #else case that fills in an empty function. This >> needs to be abstracted differently for the two platforms anyway but >> that can be done separately from this. Does tegra3 have tap delay setup >> as well? (I don't have the TRM handy right now). > > I vote just rip this function out altogether. > > It isn't used in mainline, chromeos-2.6.38, chromeos-3.0, nor our internal > 2.6.36 or 2.6.39 kernels. > > Equally, I can't find any documentation of the register bits this code > touches in either Tegra20 or Tegra30 TRMs. Actually, it used on some of the chromeos platforms, in particular for the sdio wifi interface (see board-seaboard.c on the chromeos tree). Without it we don't have stable operation. I'll get the use side patches posted for it for 3.3. And yeah. I was unable to find documentation too, it required getting questions answered from FAEs. -Olof -- 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
Olof Johansson wrote at Friday, November 18, 2011 2:25 PM: > On Fri, Nov 18, 2011 at 12:18 PM, Stephen Warren <swarren@nvidia.com> wrote: > > Olof Johansson wrote at Friday, November 18, 2011 12:07 PM: > >> On Thu, Nov 17, 2011 at 06:19:17PM +0200, Peter De Schrijver wrote: ... > >> > void tegra_sdmmc_tap_delay(struct clk *c, int delay) ... > > I vote just rip this function out altogether. > > > > It isn't used in mainline, chromeos-2.6.38, chromeos-3.0, nor our internal > > 2.6.36 or 2.6.39 kernels. ... > > Actually, it used on some of the chromeos platforms, in particular for... Oh yes, you're right. I'd grep'd for tegra2_sdmmc_tap_delay instead of tegra_sdmmc_tap_delay, and saw hits in the clock code so didn't figure I'd grep'd for the wrong thing.
On Fri, Nov 18, 2011 at 08:06:49PM +0100, Olof Johansson wrote: > Hi, > > A nit and two comments below. > > On Thu, Nov 17, 2011 at 06:19:17PM +0200, Peter De Schrijver wrote: > > Rework the tegra20 clock code to support multiple tegra variants : > > > > * remove tegra2_periph_reset_assert/tegra2_periph_reset_deassert. This > > functionality should be in clock.c. > > * compile tegra_sdmmc_tap_delay only on tegra20 as this feature will not > > be available in future variants. > > * don't export clk_measure_input_freq as its functionality is also available > > using clk_get_rate(). > > > > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> > > --- > > arch/arm/mach-tegra/clock.c | 14 +++++++++----- > > arch/arm/mach-tegra/clock.h | 3 --- > > arch/arm/mach-tegra/tegra2_clocks.c | 14 +------------- > > arch/arm/mach-tegra/timer.c | 12 ++++++++---- > > 4 files changed, 18 insertions(+), 25 deletions(-) > > > > diff --git a/arch/arm/mach-tegra/clock.c b/arch/arm/mach-tegra/clock.c > > index f8d41ff..47f6366 100644 > > --- a/arch/arm/mach-tegra/clock.c > > +++ b/arch/arm/mach-tegra/clock.c > > @@ -387,13 +387,15 @@ EXPORT_SYMBOL(tegra_clk_init_from_table); > > > > void tegra_periph_reset_deassert(struct clk *c) > > { > > - tegra2_periph_reset_deassert(c); > > + BUG_ON(!c->ops->reset); > > + c->ops->reset(c, false); > > } > > EXPORT_SYMBOL(tegra_periph_reset_deassert); > > > > void tegra_periph_reset_assert(struct clk *c) > > { > > - tegra2_periph_reset_assert(c); > > + BUG_ON(!c->ops->reset); > > + c->ops->reset(c, true); > > } > > EXPORT_SYMBOL(tegra_periph_reset_assert); > > > > @@ -403,10 +405,11 @@ void __init tegra_init_clock(void) > > } > > > > /* > > - * The SDMMC controllers have extra bits in the clock source register that > > - * adjust the delay between the clock and data to compenstate for delays > > - * on the PCB. > > + * The SDMMC controllers on tegra20 have extra bits in the clock source > > + * register that adjust the delay between the clock and data to compenstate > > + * for delays on the PCB. > > */ > > +#ifdef CONFIG_ARCH_TEGRA_2x_SOC > > void tegra_sdmmc_tap_delay(struct clk *c, int delay) > > { > > unsigned long flags; > > @@ -415,6 +418,7 @@ void tegra_sdmmc_tap_delay(struct clk *c, int delay) > > tegra2_sdmmc_tap_delay(c, delay); > > spin_unlock_irqrestore(&c->spinlock, flags); > > } > > +#endif > > Ifdeffing this out doesn't quite make sense. Better to do a #ifdef in the > include file with an #else case that fills in an empty function. This > needs to be abstracted differently for the two platforms anyway but > that can be done separately from this. Does tegra3 have tap delay setup > as well? (I don't have the TRM handy right now). > I couldn't find it in the tegra30 TRM. 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/arch/arm/mach-tegra/clock.c b/arch/arm/mach-tegra/clock.c index f8d41ff..47f6366 100644 --- a/arch/arm/mach-tegra/clock.c +++ b/arch/arm/mach-tegra/clock.c @@ -387,13 +387,15 @@ EXPORT_SYMBOL(tegra_clk_init_from_table); void tegra_periph_reset_deassert(struct clk *c) { - tegra2_periph_reset_deassert(c); + BUG_ON(!c->ops->reset); + c->ops->reset(c, false); } EXPORT_SYMBOL(tegra_periph_reset_deassert); void tegra_periph_reset_assert(struct clk *c) { - tegra2_periph_reset_assert(c); + BUG_ON(!c->ops->reset); + c->ops->reset(c, true); } EXPORT_SYMBOL(tegra_periph_reset_assert); @@ -403,10 +405,11 @@ void __init tegra_init_clock(void) } /* - * The SDMMC controllers have extra bits in the clock source register that - * adjust the delay between the clock and data to compenstate for delays - * on the PCB. + * The SDMMC controllers on tegra20 have extra bits in the clock source + * register that adjust the delay between the clock and data to compenstate + * for delays on the PCB. */ +#ifdef CONFIG_ARCH_TEGRA_2x_SOC void tegra_sdmmc_tap_delay(struct clk *c, int delay) { unsigned long flags; @@ -415,6 +418,7 @@ void tegra_sdmmc_tap_delay(struct clk *c, int delay) tegra2_sdmmc_tap_delay(c, delay); spin_unlock_irqrestore(&c->spinlock, flags); } +#endif #ifdef CONFIG_DEBUG_FS diff --git a/arch/arm/mach-tegra/clock.h b/arch/arm/mach-tegra/clock.h index 688316a..18df129 100644 --- a/arch/arm/mach-tegra/clock.h +++ b/arch/arm/mach-tegra/clock.h @@ -146,11 +146,8 @@ struct tegra_clk_init_table { }; void tegra2_init_clocks(void); -void tegra2_periph_reset_deassert(struct clk *c); -void tegra2_periph_reset_assert(struct clk *c); void clk_init(struct clk *clk); struct clk *tegra_get_clock_by_name(const char *name); -unsigned long clk_measure_input_freq(void); int clk_reparent(struct clk *c, struct clk *parent); void tegra_clk_init_from_table(struct tegra_clk_init_table *table); unsigned long clk_get_rate_locked(struct clk *c); diff --git a/arch/arm/mach-tegra/tegra2_clocks.c b/arch/arm/mach-tegra/tegra2_clocks.c index 371869d..2ab18f6 100644 --- a/arch/arm/mach-tegra/tegra2_clocks.c +++ b/arch/arm/mach-tegra/tegra2_clocks.c @@ -174,7 +174,7 @@ static int tegra_periph_clk_enable_refcount[3 * 32]; #define pmc_readl(reg) \ __raw_readl(reg_pmc_base + (reg)) -unsigned long clk_measure_input_freq(void) +static unsigned long clk_measure_input_freq(void) { u32 clock_autodetect; clk_writel(OSC_FREQ_DET_TRIG | 1, OSC_FREQ_DET); @@ -278,18 +278,6 @@ static struct clk_ops tegra_clk_m_ops = { .disable = tegra2_clk_m_disable, }; -void tegra2_periph_reset_assert(struct clk *c) -{ - BUG_ON(!c->ops->reset); - c->ops->reset(c, true); -} - -void tegra2_periph_reset_deassert(struct clk *c) -{ - BUG_ON(!c->ops->reset); - c->ops->reset(c, false); -} - /* super clock functions */ /* "super clocks" on tegra have two-stage muxes and a clock skipping * super divider. We will ignore the clock skipping divider, since we diff --git a/arch/arm/mach-tegra/timer.c b/arch/arm/mach-tegra/timer.c index 2f1df47..6366654 100644 --- a/arch/arm/mach-tegra/timer.c +++ b/arch/arm/mach-tegra/timer.c @@ -182,14 +182,18 @@ static struct irqaction tegra_timer_irq = { static void __init tegra_init_timer(void) { struct clk *clk; - unsigned long rate = clk_measure_input_freq(); + unsigned long rate; int ret; clk = clk_get_sys("timer", NULL); - if (IS_ERR(clk)) - pr_warn("Unable to get timer clock\n"); - else + if (IS_ERR(clk)) { + pr_warn("Unable to get timer clock." + " Assuming 12Mhz input clock.\n"); + rate = 12000000; + } else { clk_enable(clk); + rate = clk_get_rate(clk); + } /* * rtc registers are used by read_persistent_clock, keep the rtc clock
Rework the tegra20 clock code to support multiple tegra variants : * remove tegra2_periph_reset_assert/tegra2_periph_reset_deassert. This functionality should be in clock.c. * compile tegra_sdmmc_tap_delay only on tegra20 as this feature will not be available in future variants. * don't export clk_measure_input_freq as its functionality is also available using clk_get_rate(). Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> --- arch/arm/mach-tegra/clock.c | 14 +++++++++----- arch/arm/mach-tegra/clock.h | 3 --- arch/arm/mach-tegra/tegra2_clocks.c | 14 +------------- arch/arm/mach-tegra/timer.c | 12 ++++++++---- 4 files changed, 18 insertions(+), 25 deletions(-)