[PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
Michael Walle
michael at walle.cc
Wed Aug 17 10:43:05 CEST 2022
Am 2022-08-17 10:36, schrieb Pali Rohár:
> 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/arm/mach-kirkwood/common.c?h=v3.15#n542
>> > > > > [2] -
>> > > > > https://web.archive.org/web/20130730091033/http://www.marvell.com/embedded-processors/kirkwood/assets/FS_88F6180_9x_6281_OpenSource.pdf
>> > > > > [3] -
>> > > > > https://web.archive.org/web/20120620073511/http://www.marvell.com/embedded-processors/kirkwood/assets/HW_88F6281_OpenSource.pdf
>> > > > >
>> > > > > Fixes: 8ac303d49f89 ("arm: kirkwood: Do not overwrite CONFIG_SYS_TCLK")
>> > > > > Signed-off-by: Pali Rohár <pali at 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;
>> > > #else
>> > > /* Assume the counter is in microseconds */
>> > > return count;
>> > >
>> > > 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:
>
> 1) Either apply that change which moves preprocessor macros to C code
> and apply this one fix.
>
> 2) Or revert that my kirkwood commit 8ac303d49f89 and do not apply this
> one fix.
>
> Stefan, what do you prefer?
3) There is a chance that the timer code is actually pretty
straight-forward
and we don't need DM_CLK.
FWIW I'm working on 3) at the moment. So if that works out, we can use
CONFIG_TIMER on kirkwood and then apply your initial patch. As there are
no bugfix releases for u-boot anyway, we don't have to care for a
backport
fix.
-michael
More information about the U-Boot
mailing list