[PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
Thanks, Stefan
Stefan Roese (6): timer: orion-timer: Add support for other Armada SoC's timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms arm: mvebu: dts: Makefile: Compile Armada 375 dtb in a separate step arm: mvebu: dts: armada-375.dtsi: Add timer0 & timer1 arm: mvebu: dts: mvebu-u-boot.dtsi: Add "u-boot,dm-pre-reloc" to timer DT node
arch/arm/Kconfig | 4 ++ arch/arm/dts/Makefile | 6 ++- arch/arm/dts/armada-375.dtsi | 4 +- arch/arm/dts/mvebu-u-boot.dtsi | 11 ++++ arch/arm/mach-mvebu/include/mach/config.h | 5 -- drivers/timer/Kconfig | 5 +- drivers/timer/orion-timer.c | 66 +++++++++++++++++++++-- 7 files changed, 89 insertions(+), 12 deletions(-)
This patch adds support for other Marvell Armada SoC's, supporting the 25MHz fixed clock operation, like the Armada XP etc.
Signed-off-by: Stefan Roese sr@denx.de --- drivers/timer/Kconfig | 5 ++++- drivers/timer/orion-timer.c | 44 ++++++++++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index 404929014810..b0814acca3fb 100644 --- a/drivers/timer/Kconfig +++ b/drivers/timer/Kconfig @@ -197,8 +197,11 @@ config OMAP_TIMER config ORION_TIMER bool "Orion timer support" depends on TIMER + default y if ARCH_KIRKWOOD || (ARCH_MVEBU && ARMADA_32BIT) + select TIMER_EARLY if ARCH_MVEBU help - Select this to enable an timer for Orion devices. + Select this to enable an timer for Orion and Armada devices + like Armada XP etc.
config RISCV_TIMER bool "RISC-V timer support" diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c index fd30e1bf036c..02ed138642b8 100644 --- a/drivers/timer/orion-timer.c +++ b/drivers/timer/orion-timer.c @@ -11,10 +11,36 @@ #define TIMER0_RELOAD 0x10 #define TIMER0_VAL 0x14
+enum input_clock_type { + INPUT_CLOCK_NON_FIXED, + INPUT_CLOCK_25MHZ, /* input clock rate is fixed to 25MHz */ +}; + struct orion_timer_priv { void *base; };
+#define MVEBU_TIMER_FIXED_RATE_25MHZ 25000000 + +#if defined(CONFIG_ARCH_MVEBU) && IS_ENABLED(CONFIG_TIMER_EARLY) +/** + * timer_early_get_rate() - Get the timer rate before driver model + */ +unsigned long notrace timer_early_get_rate(void) +{ + return MVEBU_TIMER_FIXED_RATE_25MHZ; +} + +/** + * timer_early_get_count() - Get the timer count before driver model + * + */ +u64 notrace timer_early_get_count(void) +{ + return ~readl(MVEBU_TIMER_BASE + TIMER0_VAL); +} +#endif + static uint64_t orion_timer_get_count(struct udevice *dev) { struct orion_timer_priv *priv = dev_get_priv(dev); @@ -25,6 +51,7 @@ static uint64_t orion_timer_get_count(struct udevice *dev) static int orion_timer_probe(struct udevice *dev) { struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev); + enum input_clock_type type = dev_get_driver_data(dev); struct orion_timer_priv *priv = dev_get_priv(dev);
priv->base = devfdt_remap_addr_index(dev, 0); @@ -33,11 +60,20 @@ static int orion_timer_probe(struct udevice *dev) return -ENOMEM; }
- uc_priv->clock_rate = CONFIG_SYS_TCLK; - writel(~0, priv->base + TIMER0_VAL); writel(~0, priv->base + TIMER0_RELOAD);
+ if (type == INPUT_CLOCK_25MHZ) { + /* + * On Armada XP / 38x ..., the 25MHz clock source needs to + * be enabled + */ + setbits_le32(priv->base + TIMER_CTRL, BIT(11)); + uc_priv->clock_rate = MVEBU_TIMER_FIXED_RATE_25MHZ; + } else { + uc_priv->clock_rate = CONFIG_SYS_TCLK; + } + /* enable timer */ setbits_le32(priv->base + TIMER_CTRL, TIMER0_EN | TIMER0_RELOAD_EN);
@@ -49,7 +85,9 @@ static const struct timer_ops orion_timer_ops = { };
static const struct udevice_id orion_timer_ids[] = { - { .compatible = "marvell,orion-timer" }, + { .compatible = "marvell,orion-timer", .data = INPUT_CLOCK_NON_FIXED }, + { .compatible = "marvell,armada-370-timer", .data = INPUT_CLOCK_25MHZ }, + { .compatible = "marvell,armada-xp-timer", .data = INPUT_CLOCK_25MHZ }, {} };
Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE enabled, like pogo_v4.
Signed-off-by: Stefan Roese sr@denx.de --- drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c index 02ed138642b8..7e920eaeaa40 100644 --- a/drivers/timer/orion-timer.c +++ b/drivers/timer/orion-timer.c @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{ + u64 ticks = 0; + u32 rate = 1; + u64 us; + int ret; + + ret = dm_timer_init(); + if (!ret) { + /* The timer is available */ + rate = timer_get_rate(gd->timer); + timer_get_count(gd->timer, &ticks); + } else { + return 0; + } + + us = (ticks * 1000) / rate; + return us; +} +#endif + static uint64_t orion_timer_get_count(struct udevice *dev) { struct orion_timer_priv *priv = dev_get_priv(dev);
Am 2022-08-30 13:53, schrieb Stefan Roese:
Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE enabled, like pogo_v4.
Signed-off-by: Stefan Roese sr@denx.de
drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c index 02ed138642b8..7e920eaeaa40 100644 --- a/drivers/timer/orion-timer.c +++ b/drivers/timer/orion-timer.c @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{
- u64 ticks = 0;
- u32 rate = 1;
- u64 us;
- int ret;
- ret = dm_timer_init();
- if (!ret) {
/* The timer is available */rate = timer_get_rate(gd->timer);timer_get_count(gd->timer, &ticks);- } else {
return 0;- }
- us = (ticks * 1000) / rate;
- return us;
+} +#endif
This is duplicate code in almost all the timer drivers, shouldn't this be a (weak) default implementation in timer-uclass.c? Also, do you need to guard it with CONFIG_IS_ENABLED(BOOTSTAGE), aren't unused functions discarded anyway?
-michael
Adding Simon to Cc...
On 30.08.22 14:00, Michael Walle wrote:
Am 2022-08-30 13:53, schrieb Stefan Roese:
Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE enabled, like pogo_v4.
Signed-off-by: Stefan Roese sr@denx.de
drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c index 02ed138642b8..7e920eaeaa40 100644 --- a/drivers/timer/orion-timer.c +++ b/drivers/timer/orion-timer.c @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{ + u64 ticks = 0; + u32 rate = 1; + u64 us; + int ret;
+ ret = dm_timer_init(); + if (!ret) { + /* The timer is available */ + rate = timer_get_rate(gd->timer); + timer_get_count(gd->timer, &ticks); + } else { + return 0; + }
+ us = (ticks * 1000) / rate; + return us; +} +#endif
This is duplicate code in almost all the timer drivers, shouldn't this be a (weak) default implementation in timer-uclass.c?
Yes. I was lazy and just copied this function and did not notice, that even more timer drivers have this function. Of course it makes sense to not duplicate code here, but have a common function for this. Frankly I don't even know why exactly this function is needed, as I did not look into BOOTSTAGE yet.
Simon, do we really need this function? Can't bootstage just use the "normal" timer functionality instead?
Also, do you need to guard it with CONFIG_IS_ENABLED(BOOTSTAGE), aren't unused functions discarded anyway?
Yes, this should be the case. Also just a result of my lazy copy-and- past.
Thanks, Stefan
Hi Stefan,
On Tue, 30 Aug 2022 at 06:08, Stefan Roese sr@denx.de wrote:
Adding Simon to Cc...
On 30.08.22 14:00, Michael Walle wrote:
Am 2022-08-30 13:53, schrieb Stefan Roese:
Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE enabled, like pogo_v4.
Signed-off-by: Stefan Roese sr@denx.de
drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c index 02ed138642b8..7e920eaeaa40 100644 --- a/drivers/timer/orion-timer.c +++ b/drivers/timer/orion-timer.c @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{
- u64 ticks = 0;
- u32 rate = 1;
- u64 us;
- int ret;
- ret = dm_timer_init();
- if (!ret) {
/* The timer is available */rate = timer_get_rate(gd->timer);timer_get_count(gd->timer, &ticks);- } else {
return 0;- }
- us = (ticks * 1000) / rate;
- return us;
+} +#endif
This is duplicate code in almost all the timer drivers, shouldn't this be a (weak) default implementation in timer-uclass.c?
Yes. I was lazy and just copied this function and did not notice, that even more timer drivers have this function. Of course it makes sense to not duplicate code here, but have a common function for this. Frankly I don't even know why exactly this function is needed, as I did not look into BOOTSTAGE yet.
Simon, do we really need this function? Can't bootstage just use the "normal" timer functionality instead?
It is needed because bootstage is called before driver model is ready. In fact it can be used to time driver model things.
The function here isn't that useful, since we cannot call dm_timer_init() before driver model is set up.
The timer_get_boot_us() function can by in a timer driver, but must exist outside the driver infrastructure. It must init the hard and then return the correct time. One driver model probes the timer driver, it must then continue on in that way.
Actually it looks like all the timers do this wrong. See arch/arm/mach-imx/syscounter.c or arch/arm/cpu/armv8/generic_timer.c for some ideas.
Also, do you need to guard it with CONFIG_IS_ENABLED(BOOTSTAGE), aren't unused functions discarded anyway?
Yes, this should be the case. Also just a result of my lazy copy-and- past.
Regards, SImon
Hi Simon,
On 30.08.22 17:56, Simon Glass wrote:
Hi Stefan,
On Tue, 30 Aug 2022 at 06:08, Stefan Roese sr@denx.de wrote:
Adding Simon to Cc...
On 30.08.22 14:00, Michael Walle wrote:
Am 2022-08-30 13:53, schrieb Stefan Roese:
Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE enabled, like pogo_v4.
Signed-off-by: Stefan Roese sr@denx.de
drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c index 02ed138642b8..7e920eaeaa40 100644 --- a/drivers/timer/orion-timer.c +++ b/drivers/timer/orion-timer.c @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{
- u64 ticks = 0;
- u32 rate = 1;
- u64 us;
- int ret;
- ret = dm_timer_init();
- if (!ret) {
/* The timer is available */rate = timer_get_rate(gd->timer);timer_get_count(gd->timer, &ticks);- } else {
return 0;- }
- us = (ticks * 1000) / rate;
- return us;
+} +#endif
This is duplicate code in almost all the timer drivers, shouldn't this be a (weak) default implementation in timer-uclass.c?
Yes. I was lazy and just copied this function and did not notice, that even more timer drivers have this function. Of course it makes sense to not duplicate code here, but have a common function for this. Frankly I don't even know why exactly this function is needed, as I did not look into BOOTSTAGE yet.
Simon, do we really need this function? Can't bootstage just use the "normal" timer functionality instead?
It is needed because bootstage is called before driver model is ready. In fact it can be used to time driver model things.
I see, makes sense. This brings up my next questions though, why isn't CONFIG_TIMER_EARLY enough in this case? AFAICT it's targeted exactly for this early (pre DM) bootstage. From drivers/timer/Kconfig:
config TIMER_EARLY bool "Allow timer to be used early in U-Boot" depends on TIMER # initr_bootstage() requires a timer and is called before initr_dm() # so only the early timer is available default y if X86 && BOOTSTAGE help In some cases the timer must be accessible before driver model is active. Examples include when using CONFIG_TRACE to trace U-Boot's execution before driver model is set up. Enable this option to use an early timer. These functions must be supported by your timer driver: timer_early_get_count() and timer_early_get_rate().
So again, do we really need timer_get_boot_us() or isn't it enough to select TIMER_EARLY when BOOTSTAGE is enabled?
Thanks, Stefan
The function here isn't that useful, since we cannot call dm_timer_init() before driver model is set up.
The timer_get_boot_us() function can by in a timer driver, but must exist outside the driver infrastructure. It must init the hard and then return the correct time. One driver model probes the timer driver, it must then continue on in that way.
Actually it looks like all the timers do this wrong. See arch/arm/mach-imx/syscounter.c or arch/arm/cpu/armv8/generic_timer.c for some ideas.
Thanks, Stefan
Hi Stefan,
On Tue, 30 Aug 2022 at 23:57, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 30.08.22 17:56, Simon Glass wrote:
Hi Stefan,
On Tue, 30 Aug 2022 at 06:08, Stefan Roese sr@denx.de wrote:
Adding Simon to Cc...
On 30.08.22 14:00, Michael Walle wrote:
Am 2022-08-30 13:53, schrieb Stefan Roese:
Add timer_get_boot_us() to support boards, that have CONFIG_BOOTSTAGE enabled, like pogo_v4.
Signed-off-by: Stefan Roese sr@denx.de
drivers/timer/orion-timer.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c index 02ed138642b8..7e920eaeaa40 100644 --- a/drivers/timer/orion-timer.c +++ b/drivers/timer/orion-timer.c @@ -41,6 +41,28 @@ u64 notrace timer_early_get_count(void) } #endif
+#if CONFIG_IS_ENABLED(BOOTSTAGE) +ulong timer_get_boot_us(void) +{
- u64 ticks = 0;
- u32 rate = 1;
- u64 us;
- int ret;
- ret = dm_timer_init();
- if (!ret) {
/* The timer is available */rate = timer_get_rate(gd->timer);timer_get_count(gd->timer, &ticks);- } else {
return 0;- }
- us = (ticks * 1000) / rate;
- return us;
+} +#endif
This is duplicate code in almost all the timer drivers, shouldn't this be a (weak) default implementation in timer-uclass.c?
Yes. I was lazy and just copied this function and did not notice, that even more timer drivers have this function. Of course it makes sense to not duplicate code here, but have a common function for this. Frankly I don't even know why exactly this function is needed, as I did not look into BOOTSTAGE yet.
Simon, do we really need this function? Can't bootstage just use the "normal" timer functionality instead?
It is needed because bootstage is called before driver model is ready. In fact it can be used to time driver model things.
I see, makes sense. This brings up my next questions though, why isn't CONFIG_TIMER_EARLY enough in this case? AFAICT it's targeted exactly for this early (pre DM) bootstage. From drivers/timer/Kconfig:
config TIMER_EARLY bool "Allow timer to be used early in U-Boot" depends on TIMER # initr_bootstage() requires a timer and is called before initr_dm() # so only the early timer is available default y if X86 && BOOTSTAGE help In some cases the timer must be accessible before driver model is active. Examples include when using CONFIG_TRACE to trace U-Boot's execution before driver model is set up. Enable this option to use an early timer. These functions must be supported by your timer driver: timer_early_get_count() and timer_early_get_rate().
So again, do we really need timer_get_boot_us() or isn't it enough to select TIMER_EARLY when BOOTSTAGE is enabled?
The timer is for milliseconds but for bootstage we need microseconds.
Perhaps the ultimate solution here is to support a microsecond timer through the TIMER api and use the TIMER_EARLY thing to provide timer_get_boot_us(), perhaps renaming to timer_early_get_us() ?
Thanks, Stefan
The function here isn't that useful, since we cannot call dm_timer_init() before driver model is set up.
The timer_get_boot_us() function can by in a timer driver, but must exist outside the driver infrastructure. It must init the hard and then return the correct time. One driver model probes the timer driver, it must then continue on in that way.
Actually it looks like all the timers do this wrong. See arch/arm/mach-imx/syscounter.c or arch/arm/cpu/armv8/generic_timer.c for some ideas.
Regards, Simuon
Hi Simon,
On 31.08.22 19:44, Simon Glass wrote:
<snip>
It is needed because bootstage is called before driver model is ready. In fact it can be used to time driver model things.
I see, makes sense. This brings up my next questions though, why isn't CONFIG_TIMER_EARLY enough in this case? AFAICT it's targeted exactly for this early (pre DM) bootstage. From drivers/timer/Kconfig:
config TIMER_EARLY bool "Allow timer to be used early in U-Boot" depends on TIMER # initr_bootstage() requires a timer and is called before initr_dm() # so only the early timer is available default y if X86 && BOOTSTAGE help In some cases the timer must be accessible before driver model is active. Examples include when using CONFIG_TRACE to trace U-Boot's execution before driver model is set up. Enable this option to use an early timer. These functions must be supported by your timer driver: timer_early_get_count() and timer_early_get_rate().
So again, do we really need timer_get_boot_us() or isn't it enough to select TIMER_EARLY when BOOTSTAGE is enabled?
The timer is for milliseconds but for bootstage we need microseconds.
Perhaps the ultimate solution here is to support a microsecond timer through the TIMER api and use the TIMER_EARLY thing to provide timer_get_boot_us(), perhaps renaming to timer_early_get_us() ?
Yes, sounds like a plan. We should consolidate these implementations. Let me think about it and perhaps do some basic implementations and tests for a while.
Thanks, Stefan
Now that the new timer support is available for these platforms, let's select this IF for all these platforms. This way it's not necessary that each board changes it's config header.
Signed-off-by: Stefan Roese sr@denx.de --- arch/arm/Kconfig | 4 ++++ arch/arm/mach-mvebu/include/mach/config.h | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 0b72e4f6503e..60f524a2d118 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -618,6 +618,7 @@ config ARCH_KIRKWOOD select BOARD_EARLY_INIT_F select CPU_ARM926EJS select GPIO_EXTRA_HEADER + select TIMER
config ARCH_MVEBU bool "Marvell MVEBU family (Armada XP/375/38x/3700/7K/8K)" @@ -629,6 +630,8 @@ config ARCH_MVEBU select GPIO_EXTRA_HEADER select SPL_DM_SPI if SPL select SPL_DM_SPI_FLASH if SPL + select SPL_TIMER if SPL + select TIMER select OF_CONTROL select OF_SEPARATE select SPI @@ -639,6 +642,7 @@ config ARCH_ORION5X select CPU_ARM926EJS select GPIO_EXTRA_HEADER select SPL_SEPARATE_BSS if SPL + select TIMER
config TARGET_STV0991 bool "Support stv0991" diff --git a/arch/arm/mach-mvebu/include/mach/config.h b/arch/arm/mach-mvebu/include/mach/config.h index 4add0d9e1030..9b5036c31dd3 100644 --- a/arch/arm/mach-mvebu/include/mach/config.h +++ b/arch/arm/mach-mvebu/include/mach/config.h @@ -41,9 +41,4 @@ #endif #endif
-/* Use common timer */ -#define CONFIG_SYS_TIMER_COUNTS_DOWN -#define CONFIG_SYS_TIMER_COUNTER (MVEBU_TIMER_BASE + 0x14) -#define CONFIG_SYS_TIMER_RATE 25000000 - #endif /* __MVEBU_CONFIG_H */
Am 2022-08-30 13:53, schrieb Stefan Roese:
Now that the new timer support is available for these platforms, let's select this IF for all these platforms. This way it's not necessary that each board changes it's config header.
Signed-off-by: Stefan Roese sr@denx.de
arch/arm/Kconfig | 4 ++++ arch/arm/mach-mvebu/include/mach/config.h | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 0b72e4f6503e..60f524a2d118 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -618,6 +618,7 @@ config ARCH_KIRKWOOD select BOARD_EARLY_INIT_F select CPU_ARM926EJS select GPIO_EXTRA_HEADER
- select TIMER
If selected by the arch now (and the timer driver defaulting to y on this arch), could you clean the lschvl2 and lsxhl_defconfigs up and remove the two symbols there?
config ARCH_MVEBU bool "Marvell MVEBU family (Armada XP/375/38x/3700/7K/8K)" @@ -629,6 +630,8 @@ config ARCH_MVEBU select GPIO_EXTRA_HEADER select SPL_DM_SPI if SPL select SPL_DM_SPI_FLASH if SPL
- select SPL_TIMER if SPL
- select TIMER select OF_CONTROL select OF_SEPARATE select SPI
@@ -639,6 +642,7 @@ config ARCH_ORION5X select CPU_ARM926EJS select GPIO_EXTRA_HEADER select SPL_SEPARATE_BSS if SPL
- select TIMER
config TARGET_STV0991 bool "Support stv0991" diff --git a/arch/arm/mach-mvebu/include/mach/config.h b/arch/arm/mach-mvebu/include/mach/config.h index 4add0d9e1030..9b5036c31dd3 100644 --- a/arch/arm/mach-mvebu/include/mach/config.h +++ b/arch/arm/mach-mvebu/include/mach/config.h @@ -41,9 +41,4 @@ #endif #endif
-/* Use common timer */ -#define CONFIG_SYS_TIMER_COUNTS_DOWN -#define CONFIG_SYS_TIMER_COUNTER (MVEBU_TIMER_BASE + 0x14) -#define CONFIG_SYS_TIMER_RATE 25000000
#endif /* __MVEBU_CONFIG_H */
On 30.08.22 14:04, Michael Walle wrote:
Am 2022-08-30 13:53, schrieb Stefan Roese:
Now that the new timer support is available for these platforms, let's select this IF for all these platforms. This way it's not necessary that each board changes it's config header.
Signed-off-by: Stefan Roese sr@denx.de
arch/arm/Kconfig | 4 ++++ arch/arm/mach-mvebu/include/mach/config.h | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 0b72e4f6503e..60f524a2d118 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -618,6 +618,7 @@ config ARCH_KIRKWOOD select BOARD_EARLY_INIT_F select CPU_ARM926EJS select GPIO_EXTRA_HEADER + select TIMER
If selected by the arch now (and the timer driver defaulting to y on this arch), could you clean the lschvl2 and lsxhl_defconfigs up and remove the two symbols there?
Sure. I can add another patch for this in the next patchset version. Please complain, if I forget to do this. ;)
Thanks, Stefan
config ARCH_MVEBU bool "Marvell MVEBU family (Armada XP/375/38x/3700/7K/8K)" @@ -629,6 +630,8 @@ config ARCH_MVEBU select GPIO_EXTRA_HEADER select SPL_DM_SPI if SPL select SPL_DM_SPI_FLASH if SPL + select SPL_TIMER if SPL + select TIMER select OF_CONTROL select OF_SEPARATE select SPI @@ -639,6 +642,7 @@ config ARCH_ORION5X select CPU_ARM926EJS select GPIO_EXTRA_HEADER select SPL_SEPARATE_BSS if SPL + select TIMER
config TARGET_STV0991 bool "Support stv0991" diff --git a/arch/arm/mach-mvebu/include/mach/config.h b/arch/arm/mach-mvebu/include/mach/config.h index 4add0d9e1030..9b5036c31dd3 100644 --- a/arch/arm/mach-mvebu/include/mach/config.h +++ b/arch/arm/mach-mvebu/include/mach/config.h @@ -41,9 +41,4 @@ #endif #endif
-/* Use common timer */ -#define CONFIG_SYS_TIMER_COUNTS_DOWN -#define CONFIG_SYS_TIMER_COUNTER (MVEBU_TIMER_BASE + 0x14) -#define CONFIG_SYS_TIMER_RATE 25000000
#endif /* __MVEBU_CONFIG_H */
Viele Grüße, Stefan Roese
This patch changes the compilation, so that the Armada 375 board(s) are compiled in a separate step. This is necessary for the timer dts conversion, as A375 has a different / timer description in the dts.
Signed-off-by: Stefan Roese sr@denx.de --- arch/arm/dts/Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile index 7330121dbaba..03e79e14681f 100644 --- a/arch/arm/dts/Makefile +++ b/arch/arm/dts/Makefile @@ -233,8 +233,11 @@ dtb-$(CONFIG_ARCH_TEGRA) += tegra20-harmony.dtb \ tegra210-p3450-0000.dtb
ifdef CONFIG_ARMADA_32BIT +ifdef CONFIG_ARMADA_375 +dtb-$(CONFIG_ARCH_MVEBU) += \ + armada-375-db.dtb +else dtb-$(CONFIG_ARCH_MVEBU) += \ - armada-375-db.dtb \ armada-385-atl-x530.dtb \ armada-385-atl-x530DP.dtb \ armada-385-db-88f6820-amc.dtb \ @@ -254,6 +257,7 @@ dtb-$(CONFIG_ARCH_MVEBU) += \ armada-xp-maxbcm.dtb \ armada-xp-synology-ds414.dtb \ armada-xp-theadorable.dtb +endif else dtb-$(CONFIG_ARCH_MVEBU) += \ armada-3720-db.dtb \
Add the DT bindings / descriptions for timer0 & timer1, exactly as done in mainline Linux.
Signed-off-by: Stefan Roese sr@denx.de --- arch/arm/dts/armada-375.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/dts/armada-375.dtsi b/arch/arm/dts/armada-375.dtsi index 20a8c352b2f1..a044b3fc994f 100644 --- a/arch/arm/dts/armada-375.dtsi +++ b/arch/arm/dts/armada-375.dtsi @@ -187,7 +187,7 @@ reg = <0xc000 0x58>; };
- timer@c600 { + timer0: timer@c600 { compatible = "arm,cortex-a9-twd-timer"; reg = <0xc600 0x20>; interrupts = <GIC_PPI 13 (IRQ_TYPE_EDGE_RISING | GIC_CPU_MASK_SIMPLE(2))>; @@ -416,7 +416,7 @@ interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>; };
- timer@20300 { + timer1: timer@20300 { compatible = "marvell,armada-375-timer", "marvell,armada-370-timer"; reg = <0x20300 0x30>, <0x21040 0x30>; interrupts-extended = <&gic GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
Adding the "u-boot,dm-pre-reloc" DT property to the timer node is necesssary to support the timer in the early boot phases (e.g. SPL & pre-reloc).
Signed-off-by: Stefan Roese sr@denx.de --- arch/arm/dts/mvebu-u-boot.dtsi | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/arch/arm/dts/mvebu-u-boot.dtsi b/arch/arm/dts/mvebu-u-boot.dtsi index 5538f95148de..db4bf39920b1 100644 --- a/arch/arm/dts/mvebu-u-boot.dtsi +++ b/arch/arm/dts/mvebu-u-boot.dtsi @@ -15,6 +15,17 @@ u-boot,dm-pre-reloc; };
+#ifdef CONFIG_ARMADA_375 +/* Armada 375 has multiple timers, use timer1 here */ +&timer1 { + u-boot,dm-pre-reloc; +}; +#else +&timer { + u-boot,dm-pre-reloc; +}; +#endif + #ifdef CONFIG_SPL_SPI &spi0 { u-boot,dm-pre-reloc;
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote:
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5 <frozen here>
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1 <frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch). Thanks, Tony
Thanks, Stefan
Stefan Roese (6): timer: orion-timer: Add support for other Armada SoC's timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms arm: mvebu: dts: Makefile: Compile Armada 375 dtb in a separate step arm: mvebu: dts: armada-375.dtsi: Add timer0 & timer1 arm: mvebu: dts: mvebu-u-boot.dtsi: Add "u-boot,dm-pre-reloc" to timer DT node
arch/arm/Kconfig | 4 ++ arch/arm/dts/Makefile | 6 ++- arch/arm/dts/armada-375.dtsi | 4 +- arch/arm/dts/mvebu-u-boot.dtsi | 11 ++++ arch/arm/mach-mvebu/include/mach/config.h | 5 -- drivers/timer/Kconfig | 5 +- drivers/timer/orion-timer.c | 66 +++++++++++++++++++++-- 7 files changed, 89 insertions(+), 12 deletions(-)
-- 2.37.2
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote:
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5
<frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1
<frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
Thanks, Stefan
Thanks, Tony
Thanks, Stefan
Stefan Roese (6): timer: orion-timer: Add support for other Armada SoC's timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms arm: mvebu: dts: Makefile: Compile Armada 375 dtb in a separate step arm: mvebu: dts: armada-375.dtsi: Add timer0 & timer1 arm: mvebu: dts: mvebu-u-boot.dtsi: Add "u-boot,dm-pre-reloc" to timer DT node
arch/arm/Kconfig | 4 ++ arch/arm/dts/Makefile | 6 ++- arch/arm/dts/armada-375.dtsi | 4 +- arch/arm/dts/mvebu-u-boot.dtsi | 11 ++++ arch/arm/mach-mvebu/include/mach/config.h | 5 -- drivers/timer/Kconfig | 5 +- drivers/timer/orion-timer.c | 66 +++++++++++++++++++++-- 7 files changed, 89 insertions(+), 12 deletions(-)
-- 2.37.2
Viele Grüße, Stefan Roese
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote:
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote:
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5
<frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1
<frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
Thanks, Stefan
Hi Tony,
On 31.08.22 07:08, Stefan Roese wrote:
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote:
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote:
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5
<frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1
<frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
I can't reproduce this problem on my Armada XP platform. When your system is frozen, can you interrupt the sleep cmd via Ctrl-C? Can you please also test without this patchset applied, if a series of sleep commands works fine there?
Thanks, Stefan
Hi Stefan,
On Tue, Aug 30, 2022 at 11:19 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 07:08, Stefan Roese wrote:
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote:
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote:
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5
<frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1
<frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
I can't reproduce this problem on my Armada XP platform. When your system is frozen, can you interrupt the sleep cmd via Ctrl-C? Can you please also test without this patchset applied, if a series of sleep commands works fine there?
Indeed, it works without the patchset. I ran an older version (U-Boot 2021.10) and entered some random sleep periods such as
Pogo_V4> sleep 5 Pogo_V4> sleep 20.5 Pogo_V4> sleep 35 Pogo_V4> sleep 15.3 Pogo_V4> sleep 33.33 Pogo_V4> sleep 77.23
And it is also true that I could not do Ctrl-C when it is frozen running with the new timer.
Thanks, Tony
Thanks, Stefan
Hi Stefan,
On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote:
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote:
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5
<frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1
<frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
I've added debug printfs and possibly tracked down this issue. Seems like in the 2nd call to sleep, get_timer(0) did not reset the start number.
cmd/sleep.c static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { ulong start = get_timer(0);
"do_sleep got a timer start = 2015" is the 1st call to sleep 5. "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
<BEGIN log> Pogo_V4> sleep 5 do_sleep got a timer start = 2015 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 2015 curent 100 do_sleep start 2015 curent 200 do_sleep start 2015 curent 300 <snip> do_sleep start 2015 curent 4900 do_sleep end of sleep ... current = 5000 Pogo_V4>
Pogo_V4> sleep 10 do_sleep got a timer start = 16304 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping... <snip>
<END log>
So somewhere in the DM timer, "start" got accumulated. I think each get_timer(0) should be a different timer instance. It looks like the same timer instance is used again and again, causing the "start "to grow bigger, and at one point it might just overflow.
Thanks, Tony
Thanks, Stefan
Hi Tony,
On 31.08.22 08:30, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote:
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote:
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5
<frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1
<frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
I've added debug printfs and possibly tracked down this issue. Seems like in the 2nd call to sleep, get_timer(0) did not reset the start number.
cmd/sleep.c static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { ulong start = get_timer(0);
"do_sleep got a timer start = 2015" is the 1st call to sleep 5. "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
<BEGIN log> Pogo_V4> sleep 5 do_sleep got a timer start = 2015 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 2015 curent 100 do_sleep start 2015 curent 200 do_sleep start 2015 curent 300 <snip> do_sleep start 2015 curent 4900 do_sleep end of sleep ... current = 5000 Pogo_V4>
Pogo_V4> sleep 10 do_sleep got a timer start = 16304 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping...
<snip>
<END log>
So somewhere in the DM timer, "start" got accumulated. I think each get_timer(0) should be a different timer instance. It looks like the same timer instance is used again and again, causing the "start "to grow bigger, and at one point it might just overflow.
Frankly I don't really understand the problem you describe above. What do you mean with "timer instance"? get_timer(0) will return different values, depending on when you call this function. So where exactly is the problem with the 2nd "sleep 10" above?
Thanks, Stefan
Hi Stefan,
On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 08:30, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote:
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote:
This patchset enhaces the recently added Orion Timer driver to support all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this timer support is then enabled per default for those platforms, so that the board config files don't need to be changed. Also necessary is some dts hacking, so that the timer DT node is available in early U-Boot stages.
I've successfully tested this patchset on an Armada XP board. Additional test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5
<frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1
<frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
I've added debug printfs and possibly tracked down this issue. Seems like in the 2nd call to sleep, get_timer(0) did not reset the start number.
cmd/sleep.c static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { ulong start = get_timer(0);
"do_sleep got a timer start = 2015" is the 1st call to sleep 5. "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
<BEGIN log> Pogo_V4> sleep 5 do_sleep got a timer start = 2015 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 2015 curent 100 do_sleep start 2015 curent 200 do_sleep start 2015 curent 300 <snip> do_sleep start 2015 curent 4900 do_sleep end of sleep ... current = 5000 Pogo_V4>
Pogo_V4> sleep 10 do_sleep got a timer start = 16304 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping...
<snip>
<END log>
So somewhere in the DM timer, "start" got accumulated. I think each get_timer(0) should be a different timer instance. It looks like the same timer instance is used again and again, causing the "start "to grow bigger, and at one point it might just overflow.
Frankly I don't really understand the problem you describe above. What do you mean with "timer instance"? get_timer(0) will return different values, depending on when you call this function. So where exactly is the problem with the 2nd "sleep 10" above?
Please ignore what I said above! I misunderstood what get_timer() does. I'll try again on another KIrkwood board to see if the behavior will be the same.
Thanks, Tony
Thanks, Stefan
Hi Stefan,
On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 08:30, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote:
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote: > > This patchset enhaces the recently added Orion Timer driver to support > all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this > timer support is then enabled per default for those platforms, so that > the board config files don't need to be changed. Also necessary is > some dts hacking, so that the timer DT node is available in early > U-Boot stages. > > I've successfully tested this patchset on an Armada XP board. Additional > test on other boards and platforms are very welcome and necessary.
I've run some tests with the following 2 Kirkwood boards: Cloud Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV).
It seems that it was either frozen or the timer did not expire at some subsequent sleep commands. Sometime it happened at 2nd command, some time at a later sleep command. For example,
=== Pogo V4 (the 1st sleep command works correctly at 10 seconds on my stopwatch)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) Pogoplug V4
Hit any key to stop autoboot: 0 Pogo_V4> sleep 10 Pogo_V4> sleep 31.5
<frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
=== Sheevaplug (RTC battery is old, so the date was not updated, but the clock seems OK)
U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) Marvell-Sheevaplug Hit any key to stop autoboot: 0 => date Date: 2000-01-01 (Saturday) Time: 0:02:55 => sleep 10 => date Date: 2000-01-01 (Saturday) Time: 0:03:18 => sleep 10 => sleep 20.1
<frozen here>
Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
I've added debug printfs and possibly tracked down this issue. Seems like in the 2nd call to sleep, get_timer(0) did not reset the start number.
cmd/sleep.c static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { ulong start = get_timer(0);
"do_sleep got a timer start = 2015" is the 1st call to sleep 5. "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
<BEGIN log> Pogo_V4> sleep 5 do_sleep got a timer start = 2015 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 2015 curent 100 do_sleep start 2015 curent 200 do_sleep start 2015 curent 300 <snip> do_sleep start 2015 curent 4900 do_sleep end of sleep ... current = 5000 Pogo_V4>
Pogo_V4> sleep 10 do_sleep got a timer start = 16304 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping...
<snip>
<END log>
So somewhere in the DM timer, "start" got accumulated. I think each get_timer(0) should be a different timer instance. It looks like the same timer instance is used again and again, causing the "start "to grow bigger, and at one point it might just overflow.
Frankly I don't really understand the problem you describe above. What do you mean with "timer instance"? get_timer(0) will return different values, depending on when you call this function. So where exactly is the problem with the 2nd "sleep 10" above?
Please ignore what I said above! I misunderstood what get_timer() does. I'll try again on another KIrkwood board to see if the behavior will be the same.
I've run the tests again with the Seagate GoFlex Home board (Kirkwood 88F6281). Below is the log, with my annotated comments starting with ****. In these tests, current = get_timer(start) is inside the while loop. And I printed out the start and current values when (current % 100 == 0).
<BEGIN LOG> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700) Seagate GoFlex Home
SoC: Kirkwood 88F6281_A1 DRAM: 128 MiB Core: 12 devices, 10 uclasses, devicetree: separate NAND: orion_timer_probe successful 256 MiB Loading Environment from NAND... OK In: serial Out: serial Err: serial Net: eth0: ethernet-controller@72000 Hit any key to stop autoboot: 0
GoFlexHome> date Date: 2022-08-31 (Wednesday) Time: 20:43:03
GoFlexHome> sleep 5 do_sleep got a timer start = 1244 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 1244 current 100 <snip> do_sleep start 1244 current 2300 <snip> do_sleep start 1244 current 3700 <snip> do_sleep start 1244 current 4800 do_sleep start 1244 current 4900 do_sleep end of sleep ... current = 5000
**** working as intended
GoFlexHome> sleep 10 do_sleep got a timer start = 11428 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping... do_sleep start 11428 current 100 <snip> do_sleep start 11428 current 2300 <snip> do_sleep start 11428 current 9900 do_sleep end of sleep ... current = 10000
**** working as intended
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100 <snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
GoFlexHome> sleep 20.5 do_sleep got a timer start = 8339 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 8339 current 100 do_sleep start 8339 current 200 <snip> do_sleep start 8339 current 12300 do_sleep start 8339 current 12400 do_sleep start 8339 current 12500 do_sleep start 8339 current 12600 do_sleep start 8339 current 12700 do_sleep start 8339 current 12800 do_sleep start 8339 current 12900 do_sleep start 8339 current 13000 do_sleep start 8339 current 13100
*** It was frozen right here. Control-C could not interrupt the loop, so my guess *** either it was stuck during the call to get_timer(). Or the current value *** overflowed and crashed the system.
<END LOG>
Finally, I removed the patch set, and rerun the sleep tests on this board. They all worked fine, I did not see anything strange.
Thanks, Tony
Hi Stefan,
On Wed, Aug 31, 2022 at 2:53 PM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 08:30, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote:
Hi Tony,
On 31.08.22 00:15, Tony Dinh wrote: > Hi Stefan, > > On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote: >> >> This patchset enhaces the recently added Orion Timer driver to support >> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this >> timer support is then enabled per default for those platforms, so that >> the board config files don't need to be changed. Also necessary is >> some dts hacking, so that the timer DT node is available in early >> U-Boot stages. >> >> I've successfully tested this patchset on an Armada XP board. Additional >> test on other boards and platforms are very welcome and necessary. > > I've run some tests with the following 2 Kirkwood boards: Cloud > Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), > and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV). > > It seems that it was either frozen or the timer did not expire at some > subsequent sleep commands. Sometime it happened at 2nd command, some > time at a later sleep command. For example, > > === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my > stopwatch) > > U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) > Pogoplug V4 > > Hit any key to stop autoboot: 0 > Pogo_V4> sleep 10 > Pogo_V4> sleep 31.5 > <frozen here>
Does the cmd interface support fractial numbers? Please test again with 31 or other integral numbers.
> === Sheevaplug (RTC battery is old, so the date was not updated, but > the clock seems OK) > > U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) > Marvell-Sheevaplug > Hit any key to stop autoboot: 0 > => date > Date: 2000-01-01 (Saturday) Time: 0:02:55 > => sleep 10 > => date > Date: 2000-01-01 (Saturday) Time: 0:03:18 > => sleep 10 > => sleep 20.1 > <frozen here> > > Please let me know what I can do (i.e. perhaps running a debug patch).
Please see above. I assume that the fractional numbers result in very long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
I've added debug printfs and possibly tracked down this issue. Seems like in the 2nd call to sleep, get_timer(0) did not reset the start number.
cmd/sleep.c static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { ulong start = get_timer(0);
"do_sleep got a timer start = 2015" is the 1st call to sleep 5. "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
<BEGIN log> Pogo_V4> sleep 5 do_sleep got a timer start = 2015 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 2015 curent 100 do_sleep start 2015 curent 200 do_sleep start 2015 curent 300 <snip> do_sleep start 2015 curent 4900 do_sleep end of sleep ... current = 5000 Pogo_V4>
Pogo_V4> sleep 10 do_sleep got a timer start = 16304 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping...
<snip>
<END log>
So somewhere in the DM timer, "start" got accumulated. I think each get_timer(0) should be a different timer instance. It looks like the same timer instance is used again and again, causing the "start "to grow bigger, and at one point it might just overflow.
Frankly I don't really understand the problem you describe above. What do you mean with "timer instance"? get_timer(0) will return different values, depending on when you call this function. So where exactly is the problem with the 2nd "sleep 10" above?
Please ignore what I said above! I misunderstood what get_timer() does. I'll try again on another KIrkwood board to see if the behavior will be the same.
I've run the tests again with the Seagate GoFlex Home board (Kirkwood 88F6281). Below is the log, with my annotated comments starting with ****. In these tests, current = get_timer(start) is inside the while loop. And I printed out the start and current values when (current % 100 == 0).
<BEGIN LOG> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700) Seagate GoFlex Home
SoC: Kirkwood 88F6281_A1 DRAM: 128 MiB Core: 12 devices, 10 uclasses, devicetree: separate NAND: orion_timer_probe successful 256 MiB Loading Environment from NAND... OK In: serial Out: serial Err: serial Net: eth0: ethernet-controller@72000 Hit any key to stop autoboot: 0
GoFlexHome> date Date: 2022-08-31 (Wednesday) Time: 20:43:03
GoFlexHome> sleep 5 do_sleep got a timer start = 1244 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 1244 current 100
<snip> do_sleep start 1244 current 2300 <snip> do_sleep start 1244 current 3700 <snip> do_sleep start 1244 current 4800 do_sleep start 1244 current 4900 do_sleep end of sleep ... current = 5000
**** working as intended
GoFlexHome> sleep 10 do_sleep got a timer start = 11428 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping... do_sleep start 11428 current 100
<snip> do_sleep start 11428 current 2300 <snip> do_sleep start 11428 current 9900 do_sleep end of sleep ... current = 10000
**** working as intended
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
GoFlexHome> sleep 20.5 do_sleep got a timer start = 8339 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 8339 current 100 do_sleep start 8339 current 200
<snip> do_sleep start 8339 current 12300 do_sleep start 8339 current 12400 do_sleep start 8339 current 12500 do_sleep start 8339 current 12600 do_sleep start 8339 current 12700 do_sleep start 8339 current 12800 do_sleep start 8339 current 12900 do_sleep start 8339 current 13000 do_sleep start 8339 current 13100
*** It was frozen right here. Control-C could not interrupt the loop, so my guess *** either it was stuck during the call to get_timer(). Or the current value *** overflowed and crashed the system.
<END LOG>
Finally, I removed the patch set, and rerun the sleep tests on this board. They all worked fine, I did not see anything strange.
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void)
ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }
Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
Thanks, Tony
Hi Tony,
On Wed, 31 Aug 2022 at 19:39, Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 2:53 PM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 08:30, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 07:02, Stefan Roese wrote: > Hi Tony, > > On 31.08.22 00:15, Tony Dinh wrote: >> Hi Stefan, >> >> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote: >>> >>> This patchset enhaces the recently added Orion Timer driver to support >>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this >>> timer support is then enabled per default for those platforms, so that >>> the board config files don't need to be changed. Also necessary is >>> some dts hacking, so that the timer DT node is available in early >>> U-Boot stages. >>> >>> I've successfully tested this patchset on an Armada XP board. Additional >>> test on other boards and platforms are very welcome and necessary. >> >> I've run some tests with the following 2 Kirkwood boards: Cloud >> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), >> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV). >> >> It seems that it was either frozen or the timer did not expire at some >> subsequent sleep commands. Sometime it happened at 2nd command, some >> time at a later sleep command. For example, >> >> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my >> stopwatch) >> >> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) >> Pogoplug V4 >> >> Hit any key to stop autoboot: 0 >> Pogo_V4> sleep 10 >> Pogo_V4> sleep 31.5 >> <frozen here> > > Does the cmd interface support fractial numbers? Please test again with > 31 or other integral numbers. > >> === Sheevaplug (RTC battery is old, so the date was not updated, but >> the clock seems OK) >> >> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) >> Marvell-Sheevaplug >> Hit any key to stop autoboot: 0 >> => date >> Date: 2000-01-01 (Saturday) Time: 0:02:55 >> => sleep 10 >> => date >> Date: 2000-01-01 (Saturday) Time: 0:03:18 >> => sleep 10 >> => sleep 20.1 >> <frozen here> >> >> Please let me know what I can do (i.e. perhaps running a debug patch). > > Please see above. I assume that the fractional numbers result in very > long numbers internally, which result in a frozen / hanging system.
I just tested fractional numbers on another board and hey, it just works. Learned something new. So we seem to have a problem here. Let me see, if I can find something.
I've added debug printfs and possibly tracked down this issue. Seems like in the 2nd call to sleep, get_timer(0) did not reset the start number.
cmd/sleep.c static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { ulong start = get_timer(0);
"do_sleep got a timer start = 2015" is the 1st call to sleep 5. "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
<BEGIN log> Pogo_V4> sleep 5 do_sleep got a timer start = 2015 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 2015 curent 100 do_sleep start 2015 curent 200 do_sleep start 2015 curent 300 <snip> do_sleep start 2015 curent 4900 do_sleep end of sleep ... current = 5000 Pogo_V4>
Pogo_V4> sleep 10 do_sleep got a timer start = 16304 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping...
<snip>
<END log>
So somewhere in the DM timer, "start" got accumulated. I think each get_timer(0) should be a different timer instance. It looks like the same timer instance is used again and again, causing the "start "to grow bigger, and at one point it might just overflow.
Frankly I don't really understand the problem you describe above. What do you mean with "timer instance"? get_timer(0) will return different values, depending on when you call this function. So where exactly is the problem with the 2nd "sleep 10" above?
Please ignore what I said above! I misunderstood what get_timer() does. I'll try again on another KIrkwood board to see if the behavior will be the same.
I've run the tests again with the Seagate GoFlex Home board (Kirkwood 88F6281). Below is the log, with my annotated comments starting with ****. In these tests, current = get_timer(start) is inside the while loop. And I printed out the start and current values when (current % 100 == 0).
<BEGIN LOG> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700) Seagate GoFlex Home
SoC: Kirkwood 88F6281_A1 DRAM: 128 MiB Core: 12 devices, 10 uclasses, devicetree: separate NAND: orion_timer_probe successful 256 MiB Loading Environment from NAND... OK In: serial Out: serial Err: serial Net: eth0: ethernet-controller@72000 Hit any key to stop autoboot: 0
GoFlexHome> date Date: 2022-08-31 (Wednesday) Time: 20:43:03
GoFlexHome> sleep 5 do_sleep got a timer start = 1244 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 1244 current 100
<snip> do_sleep start 1244 current 2300 <snip> do_sleep start 1244 current 3700 <snip> do_sleep start 1244 current 4800 do_sleep start 1244 current 4900 do_sleep end of sleep ... current = 5000
**** working as intended
GoFlexHome> sleep 10 do_sleep got a timer start = 11428 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping... do_sleep start 11428 current 100
<snip> do_sleep start 11428 current 2300 <snip> do_sleep start 11428 current 9900 do_sleep end of sleep ... current = 10000
**** working as intended
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
GoFlexHome> sleep 20.5 do_sleep got a timer start = 8339 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 8339 current 100 do_sleep start 8339 current 200
<snip> do_sleep start 8339 current 12300 do_sleep start 8339 current 12400 do_sleep start 8339 current 12500 do_sleep start 8339 current 12600 do_sleep start 8339 current 12700 do_sleep start 8339 current 12800 do_sleep start 8339 current 12900 do_sleep start 8339 current 13000 do_sleep start 8339 current 13100
*** It was frozen right here. Control-C could not interrupt the loop, so my guess *** either it was stuck during the call to get_timer(). Or the current value *** overflowed and crashed the system.
<END LOG>
Finally, I removed the patch set, and rerun the sleep tests on this board. They all worked fine, I did not see anything strange.
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void) ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
Regards, Simon
Hi Simon,
On Wed, Aug 31, 2022 at 7:27 PM Simon Glass sjg@chromium.org wrote:
Hi Tony,
On Wed, 31 Aug 2022 at 19:39, Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 2:53 PM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan,
On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese sr@denx.de wrote:
Hi Tony,
On 31.08.22 08:30, Tony Dinh wrote:
Hi Stefan,
On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese sr@denx.de wrote: > > Hi Tony, > > On 31.08.22 07:02, Stefan Roese wrote: >> Hi Tony, >> >> On 31.08.22 00:15, Tony Dinh wrote: >>> Hi Stefan, >>> >>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese sr@denx.de wrote: >>>> >>>> This patchset enhaces the recently added Orion Timer driver to support >>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this >>>> timer support is then enabled per default for those platforms, so that >>>> the board config files don't need to be changed. Also necessary is >>>> some dts hacking, so that the timer DT node is available in early >>>> U-Boot stages. >>>> >>>> I've successfully tested this patchset on an Armada XP board. Additional >>>> test on other boards and platforms are very welcome and necessary. >>> >>> I've run some tests with the following 2 Kirkwood boards: Cloud >>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), >>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV). >>> >>> It seems that it was either frozen or the timer did not expire at some >>> subsequent sleep commands. Sometime it happened at 2nd command, some >>> time at a later sleep command. For example, >>> >>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my >>> stopwatch) >>> >>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) >>> Pogoplug V4 >>> >>> Hit any key to stop autoboot: 0 >>> Pogo_V4> sleep 10 >>> Pogo_V4> sleep 31.5 >>> <frozen here> >> >> Does the cmd interface support fractial numbers? Please test again with >> 31 or other integral numbers. >> >>> === Sheevaplug (RTC battery is old, so the date was not updated, but >>> the clock seems OK) >>> >>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) >>> Marvell-Sheevaplug >>> Hit any key to stop autoboot: 0 >>> => date >>> Date: 2000-01-01 (Saturday) Time: 0:02:55 >>> => sleep 10 >>> => date >>> Date: 2000-01-01 (Saturday) Time: 0:03:18 >>> => sleep 10 >>> => sleep 20.1 >>> <frozen here> >>> >>> Please let me know what I can do (i.e. perhaps running a debug patch). >> >> Please see above. I assume that the fractional numbers result in very >> long numbers internally, which result in a frozen / hanging system. > > I just tested fractional numbers on another board and hey, it just > works. Learned something new. So we seem to have a problem here. Let > me see, if I can find something.
I've added debug printfs and possibly tracked down this issue. Seems like in the 2nd call to sleep, get_timer(0) did not reset the start number.
cmd/sleep.c static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { ulong start = get_timer(0);
"do_sleep got a timer start = 2015" is the 1st call to sleep 5. "do_sleep got a timer start = 16304" is the 2nd call to sleep 10.
<BEGIN log> Pogo_V4> sleep 5 do_sleep got a timer start = 2015 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 2015 curent 100 do_sleep start 2015 curent 200 do_sleep start 2015 curent 300 <snip> do_sleep start 2015 curent 4900 do_sleep end of sleep ... current = 5000 Pogo_V4>
Pogo_V4> sleep 10 do_sleep got a timer start = 16304 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping...
<snip>
<END log>
So somewhere in the DM timer, "start" got accumulated. I think each get_timer(0) should be a different timer instance. It looks like the same timer instance is used again and again, causing the "start "to grow bigger, and at one point it might just overflow.
Frankly I don't really understand the problem you describe above. What do you mean with "timer instance"? get_timer(0) will return different values, depending on when you call this function. So where exactly is the problem with the 2nd "sleep 10" above?
Please ignore what I said above! I misunderstood what get_timer() does. I'll try again on another KIrkwood board to see if the behavior will be the same.
I've run the tests again with the Seagate GoFlex Home board (Kirkwood 88F6281). Below is the log, with my annotated comments starting with ****. In these tests, current = get_timer(start) is inside the while loop. And I printed out the start and current values when (current % 100 == 0).
<BEGIN LOG> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700) Seagate GoFlex Home
SoC: Kirkwood 88F6281_A1 DRAM: 128 MiB Core: 12 devices, 10 uclasses, devicetree: separate NAND: orion_timer_probe successful 256 MiB Loading Environment from NAND... OK In: serial Out: serial Err: serial Net: eth0: ethernet-controller@72000 Hit any key to stop autoboot: 0
GoFlexHome> date Date: 2022-08-31 (Wednesday) Time: 20:43:03
GoFlexHome> sleep 5 do_sleep got a timer start = 1244 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 1244 current 100
<snip> do_sleep start 1244 current 2300 <snip> do_sleep start 1244 current 3700 <snip> do_sleep start 1244 current 4800 do_sleep start 1244 current 4900 do_sleep end of sleep ... current = 5000
**** working as intended
GoFlexHome> sleep 10 do_sleep got a timer start = 11428 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping... do_sleep start 11428 current 100
<snip> do_sleep start 11428 current 2300 <snip> do_sleep start 11428 current 9900 do_sleep end of sleep ... current = 10000
**** working as intended
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
GoFlexHome> sleep 20.5 do_sleep got a timer start = 8339 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 8339 current 100 do_sleep start 8339 current 200
<snip> do_sleep start 8339 current 12300 do_sleep start 8339 current 12400 do_sleep start 8339 current 12500 do_sleep start 8339 current 12600 do_sleep start 8339 current 12700 do_sleep start 8339 current 12800 do_sleep start 8339 current 12900 do_sleep start 8339 current 13000 do_sleep start 8339 current 13100
*** It was frozen right here. Control-C could not interrupt the loop, so my guess *** either it was stuck during the call to get_timer(). Or the current value *** overflowed and crashed the system.
<END LOG>
Finally, I removed the patch set, and rerun the sleep tests on this board. They all worked fine, I did not see anything strange.
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void) ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
Thanks for your explanation! However, would you agree that code is problematic and needed some improvement ? IOW, depending what the compiler does, it might return the 1st 32 bit of the 64-bit integer result?
Thanks, Tony
Regards, Simon
Hi Tony,
On 01.09.22 09:39, Tony Dinh wrote:
<snip>
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void) ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
Thanks for your explanation! However, would you agree that code is problematic and needed some improvement ? IOW, depending what the compiler does, it might return the 1st 32 bit of the 64-bit integer result?
It will return the lower 32 bits if the system is 32bit, yes.
To check if we have a problem here, please add this (totally untested) code and extend it if it makes sense:
diff --git a/lib/time.c b/lib/time.c index bbf191f67323..ef5252419f3b 100644 --- a/lib/time.c +++ b/lib/time.c @@ -146,7 +146,15 @@ int __weak timer_init(void) /* Returns time in milliseconds */ ulong __weak get_timer(ulong base) { - return tick_to_time(get_ticks()) - base; + u64 ticks = get_ticks(); + u64 time_ms = tick_to_time(ticks); + + if (time_ms & 0xffffffff00000000ULL) + printf("ticks=%lld time_ms=%lld\n", ticks, time_ms); + if ((time_ms - base) & 0xffffffff80000000ULL) + printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n", ticks, time_ms, base, time_ms - base); + + return time_ms - base; }
At least here, you seem to have a wrap around with the 32bits AFAICT:
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
4294952265 = 0xFFFFC549!
Thanks, Stefan
Hi,
Am 01.09.2022 um 11:27 schrieb Stefan Roese:
Hi Tony,
On 01.09.22 09:39, Tony Dinh wrote:
<snip>
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void)
ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }
Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
Thanks for your explanation! However, would you agree that code is problematic and needed some improvement ? IOW, depending what the compiler does, it might return the 1st 32 bit of the 64-bit integer result?
It will return the lower 32 bits if the system is 32bit, yes.
To check if we have a problem here, please add this (totally untested) code and extend it if it makes sense:
diff --git a/lib/time.c b/lib/time.c index bbf191f67323..ef5252419f3b 100644 --- a/lib/time.c +++ b/lib/time.c @@ -146,7 +146,15 @@ int __weak timer_init(void) /* Returns time in milliseconds */ ulong __weak get_timer(ulong base) { - return tick_to_time(get_ticks()) - base; + u64 ticks = get_ticks(); + u64 time_ms = tick_to_time(ticks);
+ if (time_ms & 0xffffffff00000000ULL) + printf("ticks=%lld time_ms=%lld\n", ticks, time_ms); + if ((time_ms - base) & 0xffffffff80000000ULL) + printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n", ticks, time_ms, base, time_ms - base);
+ return time_ms - base; }
At least here, you seem to have a wrap around with the 32bits AFAICT:
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
4294952265 = 0xFFFFC549!
Does the driver use a 32 bit counter without the timer_conv_64 function inside the get_count function?
Regards Stefan
Hi Stefan,
On 01.09.22 13:52, Stefan Herbrechtsmeier wrote:
Hi,
Am 01.09.2022 um 11:27 schrieb Stefan Roese:
Hi Tony,
On 01.09.22 09:39, Tony Dinh wrote:
<snip>
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void)
ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }
Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
Thanks for your explanation! However, would you agree that code is problematic and needed some improvement ? IOW, depending what the compiler does, it might return the 1st 32 bit of the 64-bit integer result?
It will return the lower 32 bits if the system is 32bit, yes.
To check if we have a problem here, please add this (totally untested) code and extend it if it makes sense:
diff --git a/lib/time.c b/lib/time.c index bbf191f67323..ef5252419f3b 100644 --- a/lib/time.c +++ b/lib/time.c @@ -146,7 +146,15 @@ int __weak timer_init(void) /* Returns time in milliseconds */ ulong __weak get_timer(ulong base) { - return tick_to_time(get_ticks()) - base; + u64 ticks = get_ticks(); + u64 time_ms = tick_to_time(ticks);
+ if (time_ms & 0xffffffff00000000ULL) + printf("ticks=%lld time_ms=%lld\n", ticks, time_ms); + if ((time_ms - base) & 0xffffffff80000000ULL) + printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n", ticks, time_ms, base, time_ms - base);
+ return time_ms - base; }
At least here, you seem to have a wrap around with the 32bits AFAICT:
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
4294952265 = 0xFFFFC549!
Does the driver use a 32 bit counter without the timer_conv_64 function inside the get_count function?
Yes, it missed this conversion function call. Thanks for noticing.
Thanks, Stefan
Hi,
On Thu, 1 Sept 2022 at 03:27, Stefan Roese sr@denx.de wrote:
Hi Tony,
On 01.09.22 09:39, Tony Dinh wrote:
<snip>
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void) ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
This is wrong, I meant that get_tbclk() can run at 1MHZ (for example). The tick is 1KHz.
Thanks for your explanation! However, would you agree that code is problematic and needed some improvement ? IOW, depending what the compiler does, it might return the 1st 32 bit of the 64-bit integer result?
Yes, we should really use ulong for the tick count as well. The use of 64-bits seems wrong (on 32-bit machines).
It will return the lower 32 bits if the system is 32bit, yes.
To check if we have a problem here, please add this (totally untested) code and extend it if it makes sense:
diff --git a/lib/time.c b/lib/time.c index bbf191f67323..ef5252419f3b 100644 --- a/lib/time.c +++ b/lib/time.c @@ -146,7 +146,15 @@ int __weak timer_init(void) /* Returns time in milliseconds */ ulong __weak get_timer(ulong base) {
return tick_to_time(get_ticks()) - base;
u64 ticks = get_ticks();u64 time_ms = tick_to_time(ticks);if (time_ms & 0xffffffff00000000ULL)printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);if ((time_ms - base) & 0xffffffff80000000ULL)printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n",ticks, time_ms, base, time_ms - base);
return time_ms - base;At least here, you seem to have a wrap around with the 32bits AFAICT:
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
4294952265 = 0xFFFFC549!
Yes this all needs a look, I think.
Regards, Simon
Hi Stefan R,
On Thu, Sep 1, 2022 at 7:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 1 Sept 2022 at 03:27, Stefan Roese sr@denx.de wrote:
Hi Tony,
On 01.09.22 09:39, Tony Dinh wrote:
<snip>
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void) ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
This is wrong, I meant that get_tbclk() can run at 1MHZ (for example). The tick is 1KHz.
Thanks for your explanation! However, would you agree that code is problematic and needed some improvement ? IOW, depending what the compiler does, it might return the 1st 32 bit of the 64-bit integer result?
Yes, we should really use ulong for the tick count as well. The use of 64-bits seems wrong (on 32-bit machines).
It will return the lower 32 bits if the system is 32bit, yes.
To check if we have a problem here, please add this (totally untested) code and extend it if it makes sense:
diff --git a/lib/time.c b/lib/time.c index bbf191f67323..ef5252419f3b 100644 --- a/lib/time.c +++ b/lib/time.c @@ -146,7 +146,15 @@ int __weak timer_init(void) /* Returns time in milliseconds */ ulong __weak get_timer(ulong base) {
return tick_to_time(get_ticks()) - base;
u64 ticks = get_ticks();u64 time_ms = tick_to_time(ticks);if (time_ms & 0xffffffff00000000ULL)printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);if ((time_ms - base) & 0xffffffff80000000ULL)printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n",ticks, time_ms, base, time_ms - base);
return time_ms - base;
With this patch, indeed it showed a wrap around. And the system was frozen during the next command.
Below is the log (my annotated comment starts with ***).
<BEGIN LOG> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Sep 01 2022 - 15:44:22 -0700) Pogoplug V4
SoC: Kirkwood 88F6281_A1 Model: Cloud Engines PogoPlug Series 4 DRAM: 128 MiB orion_timer_probe Clock Rate 166000000 orion_timer_probe successful Core: 19 devices, 15 uclasses, devicetree: separate NAND: 128 MiB MMC: mvsdio@90000: 0 Loading Environment from NAND... OK In: serial Out: serial Err: serial pcie0.0: Link up Net: eth0: ethernet-controller@72000 Hit any key to stop autoboot: 0 Pogo_V4> sleep 5 do_sleep got a timer start = 14344 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 14344 curent 100 do_sleep start 14344 curent 200 <snip> do_sleep start 14344 curent 4800 do_sleep start 14344 curent 4900 do_sleep end of sleep ... current = 5000
Pogo_V4> sleep 10 do_sleep got a timer start = 22370 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping... do_sleep start 22370 curent 100 do_sleep start 22370 curent 200 do_sleep start 22370 curent 300 do_sleep start 22370 curent 400 <snip> do_sleep start 22370 curent 3300 do_sleep start 22370 curent 3400 do_sleep start 22370 curent 3500 ticks=188 time_ms=0 base=22370 ret=-22370 do_sleep end of sleep ... current = 4294944926
*** we are seeing wrap around here
Pogo_V4> sleep 15 do_sleep got a timer start = 15733 do_sleep delay = 15000 do_sleep delay = 15000 do_sleep sleeping... do_sleep start 15733 curent 100 do_sleep start 15733 curent 200 do_sleep start 15733 curent 300 do_sleep start 15733 curent 400 <snip>
do_sleep start 15733 curent 9900 do_sleep start 15733 curent 10000 do_sleep start 15733 curent 10100
*** And the system was frozen here
<END LOG>
Thanks, Tony
At least here, you seem to have a wrap around with the 32bits AFAICT:
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
4294952265 = 0xFFFFC549!
Yes this all needs a look, I think.
Regards, Simon
Hello,
On Thu, Sep 1, 2022 at 4:46 PM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan R,
On Thu, Sep 1, 2022 at 7:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 1 Sept 2022 at 03:27, Stefan Roese sr@denx.de wrote:
Hi Tony,
On 01.09.22 09:39, Tony Dinh wrote:
<snip>
Some ideas.
The get_timer() function looks wrong assigning an uint64_t to ulong.
lib/time.c
static uint64_t notrace tick_to_time(uint64_t tick) uint64_t notrace get_ticks(void) uint64_t __weak notrace get_ticks(void) ulong __weak get_timer(ulong base) { return tick_to_time(get_ticks()) - base; }Most of the timer infrastructure is using uint64_t. I'm seeing this __weak function get_timer was invoked in Kirkwood boards. Both in sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
This is wrong, I meant that get_tbclk() can run at 1MHZ (for example). The tick is 1KHz.
Thanks for your explanation! However, would you agree that code is problematic and needed some improvement ? IOW, depending what the compiler does, it might return the 1st 32 bit of the 64-bit integer result?
Yes, we should really use ulong for the tick count as well. The use of 64-bits seems wrong (on 32-bit machines).
It will return the lower 32 bits if the system is 32bit, yes.
To check if we have a problem here, please add this (totally untested) code and extend it if it makes sense:
diff --git a/lib/time.c b/lib/time.c index bbf191f67323..ef5252419f3b 100644 --- a/lib/time.c +++ b/lib/time.c @@ -146,7 +146,15 @@ int __weak timer_init(void) /* Returns time in milliseconds */ ulong __weak get_timer(ulong base) {
return tick_to_time(get_ticks()) - base;
u64 ticks = get_ticks();u64 time_ms = tick_to_time(ticks);if (time_ms & 0xffffffff00000000ULL)printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);if ((time_ms - base) & 0xffffffff80000000ULL)printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n",ticks, time_ms, base, time_ms - base);
return time_ms - base;With this patch, indeed it showed a wrap around. And the system was frozen during the next command.
Below is the log (my annotated comment starts with ***).
<BEGIN LOG> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Sep 01 2022 - 15:44:22 -0700) Pogoplug V4
SoC: Kirkwood 88F6281_A1 Model: Cloud Engines PogoPlug Series 4 DRAM: 128 MiB orion_timer_probe Clock Rate 166000000 orion_timer_probe successful Core: 19 devices, 15 uclasses, devicetree: separate NAND: 128 MiB MMC: mvsdio@90000: 0 Loading Environment from NAND... OK In: serial Out: serial Err: serial pcie0.0: Link up Net: eth0: ethernet-controller@72000 Hit any key to stop autoboot: 0 Pogo_V4> sleep 5 do_sleep got a timer start = 14344 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 14344 curent 100 do_sleep start 14344 curent 200
<snip> do_sleep start 14344 curent 4800 do_sleep start 14344 curent 4900 do_sleep end of sleep ... current = 5000
Pogo_V4> sleep 10 do_sleep got a timer start = 22370 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping... do_sleep start 22370 curent 100 do_sleep start 22370 curent 200 do_sleep start 22370 curent 300 do_sleep start 22370 curent 400
<snip> do_sleep start 22370 curent 3300 do_sleep start 22370 curent 3400 do_sleep start 22370 curent 3500 ticks=188 time_ms=0 base=22370 ret=-22370 do_sleep end of sleep ... current = 4294944926
*** we are seeing wrap around here
Pogo_V4> sleep 15 do_sleep got a timer start = 15733 do_sleep delay = 15000 do_sleep delay = 15000 do_sleep sleeping... do_sleep start 15733 curent 100 do_sleep start 15733 curent 200 do_sleep start 15733 curent 300 do_sleep start 15733 curent 400
<snip>
do_sleep start 15733 curent 9900 do_sleep start 15733 curent 10000 do_sleep start 15733 curent 10100
*** And the system was frozen here
<END LOG>
Thanks, Tony
At least here, you seem to have a wrap around with the 32bits AFAICT:
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
4294952265 = 0xFFFFC549!
Yes this all needs a look, I think.
Regards, Simon
I think Stefan H's response above is right.
drivers/timer/orion-timer.c
struct orion_timer_priv { void *base; };
static uint64_t orion_timer_get_count(struct udevice *dev) { struct orion_timer_priv *priv = dev_get_priv(dev); return ~readl(priv->base + TIMER0_VAL); }
To handle the wrap-around in a 32-bit system, it should invoke "u64 timer_conv_64(u32 count)". This function in timer-uclass increments the tbh when the tbl wraps around.
return timer_conv_64(~readl(priv->base + TIMER0_VAL));
I'll patch that and do some tests.
Thanks, Tony
Hi Stefan,
On Thu, Sep 1, 2022 at 7:51 PM Tony Dinh mibodhi@gmail.com wrote:
Hello,
On Thu, Sep 1, 2022 at 4:46 PM Tony Dinh mibodhi@gmail.com wrote:
Hi Stefan R,
On Thu, Sep 1, 2022 at 7:35 AM Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, 1 Sept 2022 at 03:27, Stefan Roese sr@denx.de wrote:
Hi Tony,
On 01.09.22 09:39, Tony Dinh wrote:
<snip>
> Some ideas. > > The get_timer() function looks wrong assigning an uint64_t to ulong. > > lib/time.c > > static uint64_t notrace tick_to_time(uint64_t tick) > uint64_t notrace get_ticks(void) > uint64_t __weak notrace get_ticks(void) > > ulong __weak get_timer(ulong base) > { > return tick_to_time(get_ticks()) - base; > } > > Most of the timer infrastructure is using uint64_t. I'm seeing this > __weak function get_timer was invoked in Kirkwood boards. Both in > sleep and timer commands.
The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer.
This is wrong, I meant that get_tbclk() can run at 1MHZ (for example). The tick is 1KHz.
Thanks for your explanation! However, would you agree that code is problematic and needed some improvement ? IOW, depending what the compiler does, it might return the 1st 32 bit of the 64-bit integer result?
Yes, we should really use ulong for the tick count as well. The use of 64-bits seems wrong (on 32-bit machines).
It will return the lower 32 bits if the system is 32bit, yes.
To check if we have a problem here, please add this (totally untested) code and extend it if it makes sense:
diff --git a/lib/time.c b/lib/time.c index bbf191f67323..ef5252419f3b 100644 --- a/lib/time.c +++ b/lib/time.c @@ -146,7 +146,15 @@ int __weak timer_init(void) /* Returns time in milliseconds */ ulong __weak get_timer(ulong base) {
return tick_to_time(get_ticks()) - base;
u64 ticks = get_ticks();u64 time_ms = tick_to_time(ticks);if (time_ms & 0xffffffff00000000ULL)printf("ticks=%lld time_ms=%lld\n", ticks, time_ms);if ((time_ms - base) & 0xffffffff80000000ULL)printf("ticks=%lld time_ms=%lld base=%ld ret=%lld\n",ticks, time_ms, base, time_ms - base);
return time_ms - base;With this patch, indeed it showed a wrap around. And the system was frozen during the next command.
Below is the log (my annotated comment starts with ***).
<BEGIN LOG> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Sep 01 2022 - 15:44:22 -0700) Pogoplug V4
SoC: Kirkwood 88F6281_A1 Model: Cloud Engines PogoPlug Series 4 DRAM: 128 MiB orion_timer_probe Clock Rate 166000000 orion_timer_probe successful Core: 19 devices, 15 uclasses, devicetree: separate NAND: 128 MiB MMC: mvsdio@90000: 0 Loading Environment from NAND... OK In: serial Out: serial Err: serial pcie0.0: Link up Net: eth0: ethernet-controller@72000 Hit any key to stop autoboot: 0 Pogo_V4> sleep 5 do_sleep got a timer start = 14344 do_sleep delay = 5000 do_sleep delay = 5000 do_sleep sleeping... do_sleep start 14344 curent 100 do_sleep start 14344 curent 200
<snip> do_sleep start 14344 curent 4800 do_sleep start 14344 curent 4900 do_sleep end of sleep ... current = 5000
Pogo_V4> sleep 10 do_sleep got a timer start = 22370 do_sleep delay = 10000 do_sleep delay = 10000 do_sleep sleeping... do_sleep start 22370 curent 100 do_sleep start 22370 curent 200 do_sleep start 22370 curent 300 do_sleep start 22370 curent 400
<snip> do_sleep start 22370 curent 3300 do_sleep start 22370 curent 3400 do_sleep start 22370 curent 3500 ticks=188 time_ms=0 base=22370 ret=-22370 do_sleep end of sleep ... current = 4294944926
*** we are seeing wrap around here
Pogo_V4> sleep 15 do_sleep got a timer start = 15733 do_sleep delay = 15000 do_sleep delay = 15000 do_sleep sleeping... do_sleep start 15733 curent 100 do_sleep start 15733 curent 200 do_sleep start 15733 curent 300 do_sleep start 15733 curent 400
<snip>
do_sleep start 15733 curent 9900 do_sleep start 15733 curent 10000 do_sleep start 15733 curent 10100
*** And the system was frozen here
<END LOG>
Thanks, Tony
At least here, you seem to have a wrap around with the 32bits AFAICT:
GoFlexHome> sleep 20.5 do_sleep got a timer start = 15031 do_sleep delay = 20000 do_sleep delay = 20500 do_sleep sleeping... do_sleep start 15031 current 100
<snip> do_sleep start 15031 current 6400 do_sleep end of sleep ... current = 4294952265
*** Something strange happened here. current should be 6500, but it seems to have garbage. So the loop exits prematurely.
4294952265 = 0xFFFFC549!
Yes this all needs a look, I think.
Regards, Simon
I think Stefan H's response above is right.
drivers/timer/orion-timer.c
struct orion_timer_priv { void *base; };
static uint64_t orion_timer_get_count(struct udevice *dev) { struct orion_timer_priv *priv = dev_get_priv(dev); return ~readl(priv->base + TIMER0_VAL); }
To handle the wrap-around in a 32-bit system, it should invoke "u64 timer_conv_64(u32 count)". This function in timer-uclass increments the tbh when the tbl wraps around.
return timer_conv_64(~readl(priv->base + TIMER0_VAL));I'll patch that and do some tests.
That was it. With the timer_conv_64, ticks go up and never go down. And I don't see the system frozen anymore.
return timer_conv_64(~readl(priv->base + TIMER0_VAL));
Please squash this (or make this change in your V2 patch).
Thanks, Tony
participants (5)
-
Michael Walle -
Simon Glass -
Stefan Herbrechtsmeier -
Stefan Roese -
Tony Dinh