| Message ID | 20200529201701.521933-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d2353bad2c1eef7a1228645fbb21e7887c633d12 |
| Headers | show |
| Series | ARM: omap2: fix omap5_realtime_timer_init definition | expand |
* Arnd Bergmann <arnd@arndb.de> [200529 20:18]: > There is one more regression introduced by the last build fix: Argh.. I did run make randconfig for like 10 builds after the last fix. > arch/arm/mach-omap2/timer.c:170:6: error: attribute declaration must precede definition [-Werror,-Wignored-attributes] > void __init omap5_realtime_timer_init(void) > ^ > arch/arm/mach-omap2/common.h:118:20: note: previous definition is here > static inline void omap5_realtime_timer_init(void) > ^ > arch/arm/mach-omap2/timer.c:170:13: error: redefinition of 'omap5_realtime_timer_init' > void __init omap5_realtime_timer_init(void) > ^ > arch/arm/mach-omap2/common.h:118:20: note: previous definition is here > static inline void omap5_realtime_timer_init(void) > > Address this by removing the now obsolete #ifdefs in that file and > just building the entire file based on the flag that controls the > omap5_realtime_timer_init function declaration. I think this will introduce other randconfig build failures as SOC_HAS_REALTIME_COUNTER is bool in Kconfig. We still need to call omap5_realtime_timer_init() even if SOC_HAS_REALTIME_COUNTER is not set. Regards, Tony
On Fri, May 29, 2020 at 10:44 PM Tony Lindgren <tony@atomide.com> wrote: > > * Arnd Bergmann <arnd@arndb.de> [200529 20:18]: > > There is one more regression introduced by the last build fix: > > Argh.. I did run make randconfig for like 10 builds > after the last fix. > > > Address this by removing the now obsolete #ifdefs in that file and > > just building the entire file based on the flag that controls the > > omap5_realtime_timer_init function declaration. > > I think this will introduce other randconfig build failures > as SOC_HAS_REALTIME_COUNTER is bool in Kconfig. I did a few hundred randconfig builds with the patch and have not yet seen any further problems. > We still need to call omap5_realtime_timer_init() even if > SOC_HAS_REALTIME_COUNTER is not set. This is what's in the header file: #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER extern void omap5_realtime_timer_init(void); #else static inline void omap5_realtime_timer_init(void) { } #endif In fact, the inline stub is what that caused the regression, so I think it's ok with my patch. Arnd
* Arnd Bergmann <arnd@arndb.de> [200529 21:09]: > On Fri, May 29, 2020 at 10:44 PM Tony Lindgren <tony@atomide.com> wrote: > > > > * Arnd Bergmann <arnd@arndb.de> [200529 20:18]: > > > There is one more regression introduced by the last build fix: > > > > Argh.. I did run make randconfig for like 10 builds > > after the last fix. > > > > > Address this by removing the now obsolete #ifdefs in that file and > > > just building the entire file based on the flag that controls the > > > omap5_realtime_timer_init function declaration. > > > > I think this will introduce other randconfig build failures > > as SOC_HAS_REALTIME_COUNTER is bool in Kconfig. > > I did a few hundred randconfig builds with the patch and have > not yet seen any further problems. Ah right, it works for randconfig builds now but won't boot :) > > We still need to call omap5_realtime_timer_init() even if > > SOC_HAS_REALTIME_COUNTER is not set. > > This is what's in the header file: > > #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER > extern void omap5_realtime_timer_init(void); > #else > static inline void omap5_realtime_timer_init(void) > { > } > #endif > > In fact, the inline stub is what that caused the regression, > so I think it's ok with my patch. To me it seems not having SOC_HAS_REALTIME_COUNTER will cause omap5_realtime_timer_init() not get called? That initializes clocks and calls timer_probe(). So this will result in non-booting system AFAIK, the header file stub should no rely CONFIG_SOC_HAS_REALTIME_COUNTER also, but rather ! CONFIG_SOC_OMAP5 || CONFIG_SOC_DRA7XX. Also the Makefile change at least seems wrong, that can't rely on CONFIG_SOC_HAS_REALTIME_COUNTER. Regards, Tony
On Fri, May 29, 2020 at 11:14 PM Tony Lindgren <tony@atomide.com> wrote: > * Arnd Bergmann <arnd@arndb.de> [200529 21:09]: > > > > #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER > > extern void omap5_realtime_timer_init(void); > > #else > > static inline void omap5_realtime_timer_init(void) > > { > > } > > #endif > > > > In fact, the inline stub is what that caused the regression, > > so I think it's ok with my patch. > > To me it seems not having SOC_HAS_REALTIME_COUNTER will > cause omap5_realtime_timer_init() not get called? Correct, this looked to me like it was the intention of that symbol. Unfortunately there is no help text but it is user selectable: config SOC_HAS_REALTIME_COUNTER bool "Real time free running counter" depends on SOC_OMAP5 || SOC_DRA7XX default y > That initializes clocks and calls timer_probe(). So this > will result in non-booting system AFAIK, the header > file stub should no rely CONFIG_SOC_HAS_REALTIME_COUNTER > also, but rather ! CONFIG_SOC_OMAP5 || CONFIG_SOC_DRA7XX. > > Also the Makefile change at least seems wrong, that > can't rely on CONFIG_SOC_HAS_REALTIME_COUNTER. How about just removing the prompt on CONFIG_SOC_HAS_REALTIME_COUNTER but keeping the rest of my patch? That way it's just always enabled when there is a chip that needs it enabled in the kernel config. The only other usage of the symbol is #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER void set_cntfreq(void); #else static inline void set_cntfreq(void) { } #endif Alternatively, we could just remove the Kconfig symbol altogether and rely on (SOC_OMAP5 || SOC_DRA7XX) everywhere, but that seems a little more fragile in case there is going to be another chip that needs it. Arnd
* Arnd Bergmann <arnd@arndb.de> [200529 21:41]: > On Fri, May 29, 2020 at 11:14 PM Tony Lindgren <tony@atomide.com> wrote: > > * Arnd Bergmann <arnd@arndb.de> [200529 21:09]: > > > > > > #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER > > > extern void omap5_realtime_timer_init(void); > > > #else > > > static inline void omap5_realtime_timer_init(void) > > > { > > > } > > > #endif > > > > > > In fact, the inline stub is what that caused the regression, > > > so I think it's ok with my patch. > > > > To me it seems not having SOC_HAS_REALTIME_COUNTER will > > cause omap5_realtime_timer_init() not get called? > > Correct, this looked to me like it was the intention of that > symbol. Unfortunately there is no help text but it is user > selectable: > > config SOC_HAS_REALTIME_COUNTER > bool "Real time free running counter" > depends on SOC_OMAP5 || SOC_DRA7XX > default y Maybe this is a legacy Kconfig option since Santosh got the cpuidle coupled to switch things to using the always on timers for idle modes years ago already. > > That initializes clocks and calls timer_probe(). So this > > will result in non-booting system AFAIK, the header > > file stub should no rely CONFIG_SOC_HAS_REALTIME_COUNTER > > also, but rather ! CONFIG_SOC_OMAP5 || CONFIG_SOC_DRA7XX. > > > > Also the Makefile change at least seems wrong, that > > can't rely on CONFIG_SOC_HAS_REALTIME_COUNTER. > > How about just removing the prompt on > CONFIG_SOC_HAS_REALTIME_COUNTER but keeping the > rest of my patch? That way it's just always enabled when > there is a chip that needs it enabled in the kernel config. > > The only other usage of the symbol is > > #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER > void set_cntfreq(void); > #else > static inline void set_cntfreq(void) > { > } > #endif Yeah it's already default y, so I'd say let's just get rid of the option. > Alternatively, we could just remove the Kconfig symbol > altogether and rely on (SOC_OMAP5 || SOC_DRA7XX) > everywhere, but that seems a little more fragile in case > there is going to be another chip that needs it. Sounds like we can just remove CONFIG_SOC_HAS_REALTIME_COUNTER and rely on (SOC_OMAP5 || SOC_DRA7XX). Regards, Tony
Hello: This patch was applied to soc/soc.git (refs/heads/for-next). On Fri, 29 May 2020 22:16:45 +0200 you wrote: > There is one more regression introduced by the last build fix: > > arch/arm/mach-omap2/timer.c:170:6: error: attribute declaration must precede definition [-Werror,-Wignored-attributes] > void __init omap5_realtime_timer_init(void) > ^ > arch/arm/mach-omap2/common.h:118:20: note: previous definition is here > static inline void omap5_realtime_timer_init(void) > ^ > arch/arm/mach-omap2/timer.c:170:13: error: redefinition of 'omap5_realtime_timer_init' > void __init omap5_realtime_timer_init(void) > ^ > arch/arm/mach-omap2/common.h:118:20: note: previous definition is here > static inline void omap5_realtime_timer_init(void) > > [...] Here is a summary with links: - ARM: omap2: fix omap5_realtime_timer_init definition https://git.kernel.org/soc/soc/c/d2353bad2c1eef7a1228645fbb21e7887c633d12 You are awesome, thank you!
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile index 40898b1fd7da..732e614c56b2 100644 --- a/arch/arm/mach-omap2/Makefile +++ b/arch/arm/mach-omap2/Makefile @@ -46,9 +46,9 @@ obj-$(CONFIG_SOC_OMAP5) += $(omap-4-5-common) $(smp-y) sleep44xx.o obj-$(CONFIG_SOC_AM43XX) += $(omap-4-5-common) obj-$(CONFIG_SOC_DRA7XX) += $(omap-4-5-common) $(smp-y) sleep44xx.o -omap5-dra7-common = timer.o -obj-$(CONFIG_SOC_OMAP5) += $(omap5-dra7-common) -obj-$(CONFIG_SOC_DRA7XX) += $(omap5-dra7-common) +omap5-dra7-common-$(CONFIG_SOC_HAS_REALTIME_COUNTER) = timer.o +obj-$(CONFIG_SOC_OMAP5) += $(omap5-dra7-common-y) +obj-$(CONFIG_SOC_DRA7XX) += $(omap5-dra7-common-y) # Functions loaded to SRAM obj-$(CONFIG_SOC_OMAP2420) += sram242x.o diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index c1737e737a94..620ba69c8f11 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -39,8 +39,6 @@ #define INCREMENTER_DENUMERATOR_RELOAD_OFFSET 0x14 #define NUMERATOR_DENUMERATOR_MASK 0xfffff000 -#ifdef CONFIG_SOC_HAS_REALTIME_COUNTER - static unsigned long arch_timer_freq; void set_cntfreq(void) @@ -159,14 +157,6 @@ static void __init realtime_counter_init(void) iounmap(base); } -#else - -static inline void realtime_counter_init(void) -{ -} - -#endif /* CONFIG_SOC_HAS_REALTIME_COUNTER */ - void __init omap5_realtime_timer_init(void) { omap_clk_init();
There is one more regression introduced by the last build fix: arch/arm/mach-omap2/timer.c:170:6: error: attribute declaration must precede definition [-Werror,-Wignored-attributes] void __init omap5_realtime_timer_init(void) ^ arch/arm/mach-omap2/common.h:118:20: note: previous definition is here static inline void omap5_realtime_timer_init(void) ^ arch/arm/mach-omap2/timer.c:170:13: error: redefinition of 'omap5_realtime_timer_init' void __init omap5_realtime_timer_init(void) ^ arch/arm/mach-omap2/common.h:118:20: note: previous definition is here static inline void omap5_realtime_timer_init(void) Address this by removing the now obsolete #ifdefs in that file and just building the entire file based on the flag that controls the omap5_realtime_timer_init function declaration. Fixes: d86ad463d670 ("ARM: OMAP2+: Fix regression for using local timer on non-SMP SoCs") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- If this looks ok, I'd apply it directly on top again for the merge window. --- arch/arm/mach-omap2/Makefile | 6 +++--- arch/arm/mach-omap2/timer.c | 10 ---------- 2 files changed, 3 insertions(+), 13 deletions(-)