[PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[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@kernel.org>
---
Michael, please test this patch, if it fixes your boards.
---
 arch/arm/mach-kirkwood/include/mach/kw88f6281.h | 3 ++-
 arch/arm/mach-kirkwood/include/mach/soc.h       | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-kirkwood/include/mach/kw88f6281.h b/arch/arm/mach-kirkwood/include/mach/kw88f6281.h
index 87406081cf54..f86cd0bb6013 100644
--- a/arch/arm/mach-kirkwood/include/mach/kw88f6281.h
+++ b/arch/arm/mach-kirkwood/include/mach/kw88f6281.h
@@ -15,6 +15,7 @@
 #define KW_REGS_PHY_BASE		KW88F6281_REGS_PHYS_BASE
 
 /* TCLK Core Clock definition */
-#define CONFIG_SYS_TCLK	200000000 /* 200MHz */
+#define CONFIG_SYS_TCLK			((readl(CONFIG_SAR_REG) & BIT(21)) ? \
+					166666667 : 200000000)
 
 #endif /* _ASM_ARCH_KW88F6281_H */
diff --git a/arch/arm/mach-kirkwood/include/mach/soc.h b/arch/arm/mach-kirkwood/include/mach/soc.h
index 1d7f2828cd38..5f545c6f4349 100644
--- a/arch/arm/mach-kirkwood/include/mach/soc.h
+++ b/arch/arm/mach-kirkwood/include/mach/soc.h
@@ -62,6 +62,8 @@
 #define MVCPU_WIN_ENABLE	KWCPU_WIN_ENABLE
 #define MVCPU_WIN_DISABLE	KWCPU_WIN_DISABLE
 
+#define CONFIG_SAR_REG		(KW_MPP_BASE + 0x0030)
+
 #if defined (CONFIG_KW88F6281)
 #include <asm/arch/kw88f6281.h>
 #elif defined (CONFIG_KW88F6192)
-- 
2.20.1

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[Michael Walle]

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@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

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[Pali Rohár]

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@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)
+		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.

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[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@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.

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?

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[Michael Walle]

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@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?

-michael

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[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@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?

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[Michael Walle]

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@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

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[Michael Walle]

Am 2022-08-17 10:43, schrieb Michael Walle:
> 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.

Ah damn, we cannot do this depending on wether CONFIG_TIMER is set or 
not.
E.g. on a per-device basis. Or we do some ugly stuff as

#ifdef CONFIG_TIMER
#define CONFIG_SYS_TCLK                        ((readl(CONFIG_SAR_REG) & 
BIT(21)) ? \
                                        166666667 : 200000000)
#else
#define CONFIG_SYS_TCLK 200000000
#endif

So it seems we are back to 1) or 2) :(

-michael

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[Stefan Roese]

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@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?

Thanks,
Stefan

Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register

[Pali Rohár]

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@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.