| Message ID | 1405437890-6468-3-git-send-email-pdeschrijver@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote: [...] > diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c > index d081732..65cde4e 100644 > --- a/drivers/clk/tegra/clk.c > +++ b/drivers/clk/tegra/clk.c > @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id, > > tegra_clk_apply_init_table_func tegra_clk_apply_init_table; > > -void __init tegra_clocks_apply_init_table(void) > +static int __init tegra_clocks_apply_init_table(void) > { > if (!tegra_clk_apply_init_table) > - return; > + return 0; Shouldn't this be an error? Or perhaps WARN()? To make sure this gets noticed since it's obviously a mistake. I'm wondering if perhaps we could simply remove this check and let the kernel crash if it isn't a valid function pointer. Is there a case where this not being set at this point is even possible (or valid?). If not, perhaps it would be better to just call the SoC generation versions of this function from here directly rather than going through a function pointer? Thierry
On Wed, Jul 16, 2014 at 09:19:33AM +0200, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote: > [...] > > diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c > > index d081732..65cde4e 100644 > > --- a/drivers/clk/tegra/clk.c > > +++ b/drivers/clk/tegra/clk.c > > @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id, > > > > tegra_clk_apply_init_table_func tegra_clk_apply_init_table; > > > > -void __init tegra_clocks_apply_init_table(void) > > +static int __init tegra_clocks_apply_init_table(void) > > { > > if (!tegra_clk_apply_init_table) > > - return; > > + return 0; > > Shouldn't this be an error? Or perhaps WARN()? To make sure this gets An arch_initcall will be called for every ARM platform I think? In case this gets called on a non-Tegra platform, tegra_clk_apply_init_table will not be set and therefore a silent return 0; seems the most appropriate thing to do to me? Cheers, Peter.
On 07/16/2014 02:27 AM, Peter De Schrijver wrote: > On Wed, Jul 16, 2014 at 09:19:33AM +0200, Thierry Reding wrote: >> * PGP Signed by an unknown key >> >> On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote: >> [...] >>> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c >>> index d081732..65cde4e 100644 >>> --- a/drivers/clk/tegra/clk.c >>> +++ b/drivers/clk/tegra/clk.c >>> @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id, >>> >>> tegra_clk_apply_init_table_func tegra_clk_apply_init_table; >>> >>> -void __init tegra_clocks_apply_init_table(void) >>> +static int __init tegra_clocks_apply_init_table(void) >>> { >>> if (!tegra_clk_apply_init_table) >>> - return; >>> + return 0; >> >> Shouldn't this be an error? Or perhaps WARN()? To make sure this gets > > An arch_initcall will be called for every ARM platform I think? In case > this gets called on a non-Tegra platform, tegra_clk_apply_init_table will not > be set and therefore a silent return 0; seems the most appropriate thing to do > to me? This is one reason that doing all the initialization from separate initcalls sucks. Much better to have a single top-level initialization function that calls exactly what is needed, only what is needed, and only runs on the correct SoCs. But failing that, I guess you need to say something like of_is_compatible(root node, "nvidia Tegra"), but of course the definition of "nvidia Tegra" is an ever-growing list of possible values that needs to be used from each separate initcall...
On Mon, Jul 21, 2014 at 03:43:08PM -0600, Stephen Warren wrote: > On 07/16/2014 02:27 AM, Peter De Schrijver wrote: > > On Wed, Jul 16, 2014 at 09:19:33AM +0200, Thierry Reding wrote: > >> * PGP Signed by an unknown key > >> > >> On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote: > >> [...] > >>> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c > >>> index d081732..65cde4e 100644 > >>> --- a/drivers/clk/tegra/clk.c > >>> +++ b/drivers/clk/tegra/clk.c > >>> @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id, > >>> > >>> tegra_clk_apply_init_table_func tegra_clk_apply_init_table; > >>> > >>> -void __init tegra_clocks_apply_init_table(void) > >>> +static int __init tegra_clocks_apply_init_table(void) > >>> { > >>> if (!tegra_clk_apply_init_table) > >>> - return; > >>> + return 0; > >> > >> Shouldn't this be an error? Or perhaps WARN()? To make sure this gets > > > > An arch_initcall will be called for every ARM platform I think? In case > > this gets called on a non-Tegra platform, tegra_clk_apply_init_table will not > > be set and therefore a silent return 0; seems the most appropriate thing to do > > to me? > > This is one reason that doing all the initialization from separate > initcalls sucks. Much better to have a single top-level initialization > function that calls exactly what is needed, only what is needed, and > only runs on the correct SoCs. > > But failing that, I guess you need to say something like > of_is_compatible(root node, "nvidia Tegra"), but of course the > definition of "nvidia Tegra" is an ever-growing list of possible values > that needs to be used from each separate initcall... FWIW, we have soc_is_tegra() now in include/soc/tegra/common.h which is meant to be used for exactly this purpose. I agree that it isn't optimal but it's pretty good. It should be easy to refactor this to make it callable from a top-level initialization function when a decision regarding that has been made. Thierry
On 07/15/2014 09:24 AM, Peter De Schrijver wrote: > tegra_clocks_apply_init_table needs to be called after the udelay loop has > been calibrated (see 441f199a37cfd66c5dd8dd45490bd3ea6971117d why that is). Instead of just the commit ID, can you please mention the commit subject too: 441f199a37cf "clk: tegra: defer application of init table" > On existing Tegra SoCs this was done by calling tegra_clocks_apply_init_table > from tegra_dt_init. To make this also work on ARM64, we need to > change this into an initcall. tegra_dt_init is called from customize_machine > which is an arch_initcall. Therefore this should also work on existing 32bit > Tegra SoCs. I still strongly dislike performing this basic initialization from random separate initcalls. I think we should create a single initcall for all the Tegra initialization, even if it isn't able to be a machine descriptor hook function any more. That said, discussions re: that are ongoing in other threads, so it's no worth reworking this patch yet.
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c index 15ac9fc..6c5cad5 100644 --- a/arch/arm/mach-tegra/tegra.c +++ b/arch/arm/mach-tegra/tegra.c @@ -32,7 +32,6 @@ #include <linux/slab.h> #include <linux/sys_soc.h> #include <linux/usb/tegra_usb_phy.h> -#include <linux/clk/tegra.h> #include <linux/irqchip.h> #include <asm/hardware/cache-l2x0.h> @@ -96,8 +95,6 @@ static void __init tegra_dt_init(void) tegra_pmc_init(); - tegra_clocks_apply_init_table(); - soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL); if (!soc_dev_attr) goto out; diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index d081732..65cde4e 100644 --- a/drivers/clk/tegra/clk.c +++ b/drivers/clk/tegra/clk.c @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id, tegra_clk_apply_init_table_func tegra_clk_apply_init_table; -void __init tegra_clocks_apply_init_table(void) +static int __init tegra_clocks_apply_init_table(void) { if (!tegra_clk_apply_init_table) - return; + return 0; tegra_clk_apply_init_table(); + + return 0; } +arch_initcall(tegra_clocks_apply_init_table); diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h index 3ca9fca..19c4208 100644 --- a/include/linux/clk/tegra.h +++ b/include/linux/clk/tegra.h @@ -120,6 +120,4 @@ static inline void tegra_cpu_clock_resume(void) } #endif -void tegra_clocks_apply_init_table(void); - #endif /* __LINUX_CLK_TEGRA_H_ */
tegra_clocks_apply_init_table needs to be called after the udelay loop has been calibrated (see 441f199a37cfd66c5dd8dd45490bd3ea6971117d why that is). On existing Tegra SoCs this was done by calling tegra_clocks_apply_init_table from tegra_dt_init. To make this also work on ARM64, we need to change this into an initcall. tegra_dt_init is called from customize_machine which is an arch_initcall. Therefore this should also work on existing 32bit Tegra SoCs. Tested on Tegra20 (ventana), Tegra30 (beaverboard), Tegra124 (jetson TK1) and Tegra132. Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> --- arch/arm/mach-tegra/tegra.c | 3 --- drivers/clk/tegra/clk.c | 7 +++++-- include/linux/clk/tegra.h | 2 -- 3 files changed, 5 insertions(+), 7 deletions(-)