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