[PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

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(-)

-- 
2.37.2

[PATCH 1/6] timer: orion-timer: Add support for other Armada SoC's

[Stefan Roese]

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 },
 	{}
 };
 
-- 
2.37.2

[PATCH 2/6] timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support

[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
+
 static uint64_t orion_timer_get_count(struct udevice *dev)
 {
 	struct orion_timer_priv *priv = dev_get_priv(dev);
-- 
2.37.2

[PATCH 3/6] arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms

[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
 
 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 */
-- 
2.37.2

[PATCH 4/6] arm: mvebu: dts: Makefile: Compile Armada 375 dtb in a separate step

[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			\
-- 
2.37.2

[PATCH 5/6] arm: mvebu: dts: armada-375.dtsi: Add timer0 & timer1

[Stefan Roese]

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>,
-- 
2.37.2

[PATCH 6/6] arm: mvebu: dts: mvebu-u-boot.dtsi: Add "u-boot, dm-pre-reloc" to timer DT node

[Stefan Roese]

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;
-- 
2.37.2

Re: [PATCH 2/6] timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support

[Michael Walle]

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

Re: [PATCH 3/6] arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms

[Michael Walle]

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 */

Re: [PATCH 2/6] timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support

[Stefan Roese]

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

Re: [PATCH 3/6] arm: mvebu: Use CONFIG_TIMER on all MVEBU & KIRKWOOD platforms

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 2/6] timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support

[Simon Glass]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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
>

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[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

Re: [PATCH 2/6] timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support

[Stefan Roese]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 2/6] timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support

[Simon Glass]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Simon Glass]

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

Re: [PATCH 2/6] timer: orion-timer: Add timer_get_boot_us() for BOOTSTAGE support

[Stefan Roese]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[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!

Thanks,
Stefan

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Herbrechtsmeier]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Simon Glass]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Tony Dinh]

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

Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

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

[PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards

[Stefan Roese]

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(-)

-- 
2.37.2