* [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
@ 2022-08-16 20:00 Pali Rohár
2022-08-16 21:34 ` Michael Walle
0 siblings, 1 reply; 10+ messages in thread
From: Pali Rohár @ 2022-08-16 20:00 UTC (permalink / raw)
To: Stefan Roese, Michael Walle; +Cc: u-boot
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-16 20:00 [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register Pali Rohár
@ 2022-08-16 21:34 ` Michael Walle
2022-08-16 22:14 ` Pali Rohár
0 siblings, 1 reply; 10+ messages in thread
From: Michael Walle @ 2022-08-16 21:34 UTC (permalink / raw)
To: Pali Rohár; +Cc: Stefan Roese, u-boot
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-16 21:34 ` Michael Walle
@ 2022-08-16 22:14 ` Pali Rohár
2022-08-16 22:39 ` Pali Rohár
0 siblings, 1 reply; 10+ messages in thread
From: Pali Rohár @ 2022-08-16 22:14 UTC (permalink / raw)
To: Michael Walle; +Cc: Stefan Roese, u-boot
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.
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-16 22:14 ` Pali Rohár
@ 2022-08-16 22:39 ` Pali Rohár
2022-08-16 23:23 ` Michael Walle
0 siblings, 1 reply; 10+ messages in thread
From: Pali Rohár @ 2022-08-16 22:39 UTC (permalink / raw)
To: Michael Walle; +Cc: Stefan Roese, u-boot
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?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-16 22:39 ` Pali Rohár
@ 2022-08-16 23:23 ` Michael Walle
2022-08-17 8:36 ` Pali Rohár
0 siblings, 1 reply; 10+ messages in thread
From: Michael Walle @ 2022-08-16 23:23 UTC (permalink / raw)
To: Pali Rohár; +Cc: Stefan Roese, u-boot
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-16 23:23 ` Michael Walle
@ 2022-08-17 8:36 ` Pali Rohár
2022-08-17 8:43 ` Michael Walle
2022-08-17 10:42 ` Stefan Roese
0 siblings, 2 replies; 10+ messages in thread
From: Pali Rohár @ 2022-08-17 8:36 UTC (permalink / raw)
To: Michael Walle, Stefan Roese; +Cc: u-boot
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?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-17 8:36 ` Pali Rohár
@ 2022-08-17 8:43 ` Michael Walle
2022-08-17 9:13 ` Michael Walle
2022-08-17 10:42 ` Stefan Roese
1 sibling, 1 reply; 10+ messages in thread
From: Michael Walle @ 2022-08-17 8:43 UTC (permalink / raw)
To: Pali Rohár; +Cc: Stefan Roese, u-boot
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-17 8:43 ` Michael Walle
@ 2022-08-17 9:13 ` Michael Walle
0 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-08-17 9:13 UTC (permalink / raw)
To: Pali Rohár; +Cc: Stefan Roese, u-boot
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-17 8:36 ` Pali Rohár
2022-08-17 8:43 ` Michael Walle
@ 2022-08-17 10:42 ` Stefan Roese
2022-08-17 11:17 ` Pali Rohár
1 sibling, 1 reply; 10+ messages in thread
From: Stefan Roese @ 2022-08-17 10:42 UTC (permalink / raw)
To: Pali Rohár, Michael Walle; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register
2022-08-17 10:42 ` Stefan Roese
@ 2022-08-17 11:17 ` Pali Rohár
0 siblings, 0 replies; 10+ messages in thread
From: Pali Rohár @ 2022-08-17 11:17 UTC (permalink / raw)
To: Michael Walle, Stefan Roese; +Cc: u-boot
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-08-17 11:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-16 20:00 [PATCH] arm: kirkwood: 88f6281: Detect CONFIG_SYS_TCLK from SAR register Pali Rohár
2022-08-16 21:34 ` Michael Walle
2022-08-16 22:14 ` Pali Rohár
2022-08-16 22:39 ` Pali Rohár
2022-08-16 23:23 ` Michael Walle
2022-08-17 8:36 ` Pali Rohár
2022-08-17 8:43 ` Michael Walle
2022-08-17 9:13 ` Michael Walle
2022-08-17 10:42 ` Stefan Roese
2022-08-17 11:17 ` Pali Rohár
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.