Hi Pali,
On 17.08.22 10:36, Pali Rohár wrote:
On Wednesday 17 August 2022 01:23:34 Michael Walle wrote:
Am 2022-08-17 00:39, schrieb Pali Rohár:
On Wednesday 17 August 2022 00:14:20 Pali Rohár wrote:
On Tuesday 16 August 2022 23:34:13 Michael Walle wrote:
Am 2022-08-16 22:00, schrieb Pali Rohár:
Bit 21 in SAR register specifies if TCLK is running at 166 MHz or 200 MHz. This information is undocumented in public Marvell Kirkwood Functional Specifications [2], but is available in Linux v3.15 kirkwood code [1].
Commit 8ac303d49f89 ("arm: kirkwood: Do not overwrite CONFIG_SYS_TCLK") broke support for Marvell 88F6281 SoCs because it was expected that all those SoCs have TCLK running at 200 MHz as specified in Marvell 88F6281 Hardware Specifications [3].
Fix broken support for 88F6281 by detecting CONFIG_SYS_TCLK from SAR register, like it was doing Linux v3.15.
[1] - https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/a... [2] - https://web.archive.org/web/20130730091033/http://www.marvell.com/embedded-p... [3] - https://web.archive.org/web/20120620073511/http://www.marvell.com/embedded-p...
Fixes: 8ac303d49f89 ("arm: kirkwood: Do not overwrite CONFIG_SYS_TCLK") Signed-off-by: Pali Rohár pali@kernel.org
Michael, please test this patch, if it fixes your boards.
Thanks, but this doesn't compile:
In file included from lib/time.c:19: lib/time.c: In function ‘timer_get_boot_us’: ./arch/arm/include/asm/io.h:108:19: error: token "{" is not valid in preprocessor expressions 108 | #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) | ^ ./arch/arm/include/asm/arch/kw88f6281.h:18:29: note: in expansion of macro ‘readl’ 18 | #define CONFIG_SYS_TCLK ((readl(CONFIG_SAR_REG) & BIT(21)) ? \ | ^~~~~ ./arch/arm/include/asm/arch/config.h:56:32: note: in expansion of macro ‘CONFIG_SYS_TCLK’ 56 | #define CONFIG_SYS_TIMER_RATE CONFIG_SYS_TCLK | ^~~~~~~~~~~~~~~ lib/time.c:50:5: note: in expansion of macro ‘CONFIG_SYS_TIMER_RATE’ 50 | #if CONFIG_SYS_TIMER_RATE == 1000000 | ^~~~~~~~~~~~~~~~~~~~~ ./arch/arm/include/asm/io.h:108:19: error: token "{" is not valid in preprocessor expressions 108 | #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) | ^ ./arch/arm/include/asm/arch/kw88f6281.h:18:29: note: in expansion of macro ‘readl’ 18 | #define CONFIG_SYS_TCLK ((readl(CONFIG_SAR_REG) & BIT(21)) ? \ | ^~~~~ ./arch/arm/include/asm/arch/config.h:56:32: note: in expansion of macro ‘CONFIG_SYS_TCLK’ 56 | #define CONFIG_SYS_TIMER_RATE CONFIG_SYS_TCLK | ^~~~~~~~~~~~~~~ lib/time.c:52:7: note: in expansion of macro ‘CONFIG_SYS_TIMER_RATE’ 52 | #elif CONFIG_SYS_TIMER_RATE > 1000000 | ^~~~~~~~~~~~~~~~~~~~~ make[1]: *** [scripts/Makefile.build:257: lib/time.o] Fehler 1
-michael
Ah :-( And why it works on other Armada mvebu boards? Because of this:
$ git grep CONFIG_SYS_TIMER_RATE arch/arm/mach-mvebu arch/arm/mach-mvebu/include/mach/config.h:#define CONFIG_SYS_TIMER_RATE 25000000
I have looked how is this option used and it is unit for CONFIG_SYS_TIMER_COUNTER option. On all kirkwood and mvebu platforms is that register set to SoC Global Timer 0 which on Armada 38x uses either fixed 25MHz clock or NBCLK clock, together with SoC Global Timer 0 Ratio. When NBCLK then the frequency of that timer register is: NBCLK / 2 / (2 ^ ratio). Default ratio is 0 (but can be configured up to 7). By default fixed 25MHz clock is used.
Hence for mvebu it is OK as U-Boot does not change default values of SoC Global Timer 0.
For kirkwood it looks like that those timers are based on TCLK (page 283 in Functional Specifications).
How to fix it? I do not know right now. I have just quirk & dirty solution by moving code from preprocessor to compiler:
diff --git a/lib/time.c b/lib/time.c index 96074b84af69..f2ffc0498d39 100644 --- a/lib/time.c +++ b/lib/time.c @@ -46,13 +46,15 @@ unsigned long notrace timer_read_counter(void) ulong timer_get_boot_us(void) { ulong count = timer_read_counter();
-#if CONFIG_SYS_TIMER_RATE == 1000000
- return count;
-#elif CONFIG_SYS_TIMER_RATE > 1000000
- return lldiv(count, CONFIG_SYS_TIMER_RATE / 1000000);
-#elif defined(CONFIG_SYS_TIMER_RATE)
- return (unsigned long long)count * 1000000 / CONFIG_SYS_TIMER_RATE;
+#ifdef CONFIG_SYS_TIMER_RATE
- const ulong timer_rate = CONFIG_SYS_TIMER_RATE;
- if (timer_rate)
~~~~~~~~~~Here needs to be if (timer_rate == 1000000)
return count;- else if (timer_rate > 1000000)
return lldiv(count, timer_rate / 1000000);- else
return (unsigned long long)count * 1000000 / timer_rate;At least it could be used for verifying if CONFIG_SYS_TCLK is working.
FWIW, this is working.
Anyway, it looks like that this timer code is deprecated and boards should migrate to use DM.
So maybe the proper fix for this would be to add kirkwood dm driver?
Eventually sure, but for now I'd like to have a working board without spending too much time on fixing this. I'd propose to revert the broken commit for now and then, we can work on a proper timer driver.
But I can already say that the u-boot binary is almost at its size limit. I've looked into CONFIG_TIMER and it seems that we'd also need the whole clock infrastructure (and DM_CLK). No?
So, for now I see only two options:
Either apply that change which moves preprocessor macros to C code and apply this one fix.
Or revert that my kirkwood commit 8ac303d49f89 and do not apply this one fix.
Stefan, what do you prefer?
I prefer option 1. Could you please work on a new patch for this and include some reasoning for this change in the commit message?
Thanks, Stefan