[PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
Pali Rohár
pali at kernel.org
Wed Aug 17 13:17:42 CEST 2022
On Wednesday 17 August 2022 12:42:57 Stefan Roese wrote:
> 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/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?
>
> 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?
Michael, will you prepare and test it? I think you have now everything required.
More information about the U-Boot
mailing list