* [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us()
@ 2022-09-21 14:06 Stefan Roese
2022-09-21 14:06 ` [PATCH 01/10] arm: arch_timer: Add timer_early functions Stefan Roese
` (9 more replies)
0 siblings, 10 replies; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini
While working on the bootstage support for Armada XP (orion-timer)
I noticed, that we currently have two API's supporting early timer
functionality. These are:
a) timer_early_get_rate() & timer_early_get_count() which integrates
into the "normal" timer functions like timer_get() & timer_get_us()
in the early boot phase, if CONFIG_TIMER_EARLY is enabled
b) timer_get_boot_us(), which was introduced with bootstage IIUTC
IMHO it makes more sense to not introduce a new API for this early
timer functionality but re-use the "normal" API instead. This patchset
migrates the timer_get_boot_us() implementations to the timer_early
functions. This is done by:
- Implementing the timer_early functions in the drivers currently
supporting timer_get_boot_us()
- Migrating bootcount to using timer_get_us() instead of
timer_get_boot_us()
- Completely removing timer_get_boot_us()
With some minor tweaks in board_r/f for the dm_timer_init() and
timer_init() call.
I've tested this on Armada XP and sandbox.
Thanks,
Stefan
Stefan Roese (10):
arm: arch_timer: Add timer_early functions
arm: imx: syscounter: Add timer_early functions
arm: armv8: generic_timer: Add timer_early functions
timer: cadence-ttc: Add timer_early functions
timer: omap-timer: Add timer_early functions
timer: rockchip_timer: Add timer_early functions
board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
board_f/r: Don't call timer_init() when TIMER is enabled
bootstage: Migrate from timer_get_boot_us() to timer_get_us()
bootstage/timer: Treewide remove timer_get_boot_us()
arch/arm/cpu/armv7/arch_timer.c | 15 ++++++++---
arch/arm/cpu/armv8/generic_timer.c | 9 ++++---
arch/arm/mach-imx/syscounter.c | 12 +++++++--
arch/sandbox/cpu/cpu.c | 11 --------
boot/Kconfig | 1 +
common/board_f.c | 4 +--
common/board_r.c | 5 ++--
common/bootstage.c | 26 +++++++++---------
drivers/timer/Kconfig | 1 -
drivers/timer/cadence-ttc.c | 9 ++++---
drivers/timer/omap-timer.c | 9 ++++---
drivers/timer/orion-timer.c | 8 ------
drivers/timer/rockchip_timer.c | 42 ++++++++++++++++--------------
drivers/timer/tsc_timer.c | 5 ----
include/bootstage.h | 17 ++++--------
lib/time.c | 20 --------------
16 files changed, 87 insertions(+), 107 deletions(-)
--
2.37.3
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 01/10] arm: arch_timer: Add timer_early functions
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-21 14:06 ` [PATCH 02/10] arm: imx: syscounter: " Stefan Roese
` (8 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini, Patrick Delaunay
Currently this timer driver provides timer_get_boot_us() to support the
BOOTSTAGE functionality. This patch adds the timer_early functions so
that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
is enabled.
timer_get_boot_us() will get removed in a follow-up patch, once the
BOOTSTAGE interface is migrated to timer_get_us().
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
---
arch/arm/cpu/armv7/arch_timer.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm/cpu/armv7/arch_timer.c b/arch/arm/cpu/armv7/arch_timer.c
index d96406f7626f..80cfaf2d09a4 100644
--- a/arch/arm/cpu/armv7/arch_timer.c
+++ b/arch/arm/cpu/armv7/arch_timer.c
@@ -62,3 +62,19 @@ ulong get_tbclk(void)
{
return gd->arch.timer_rate_hz;
}
+
+unsigned long notrace timer_early_get_rate(void)
+{
+ if (!gd->arch.timer_rate_hz)
+ timer_init();
+
+ return gd->arch.timer_rate_hz;
+}
+
+u64 notrace timer_early_get_count(void)
+{
+ if (!gd->arch.timer_rate_hz)
+ timer_init();
+
+ return get_ticks();
+}
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 02/10] arm: imx: syscounter: Add timer_early functions
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
2022-09-21 14:06 ` [PATCH 01/10] arm: arch_timer: Add timer_early functions Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-21 14:06 ` [PATCH 03/10] arm: armv8: generic_timer: " Stefan Roese
` (7 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini, Jun Nie, Shawn Guo, Fabio Estevam, Stefano Babic
Currently this timer driver provides timer_get_boot_us() to support the
BOOTSTAGE functionality. This patch adds the timer_early functions so
that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
is enabled.
timer_get_boot_us() will get removed in a follow-up patch, once the
BOOTSTAGE interface is migrated to timer_get_us().
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Jun Nie <jun.nie@linaro.org>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Fabio Estevam <festevam@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
---
arch/arm/mach-imx/syscounter.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c
index df478a232637..dbe55ee3913d 100644
--- a/arch/arm/mach-imx/syscounter.c
+++ b/arch/arm/mach-imx/syscounter.c
@@ -109,6 +109,22 @@ ulong timer_get_boot_us(void)
return tick_to_time(get_ticks());
}
+unsigned long notrace timer_early_get_rate(void)
+{
+ if (!gd->arch.timer_rate_hz)
+ timer_init();
+
+ return gd->arch.timer_rate_hz;
+}
+
+u64 notrace timer_early_get_count(void)
+{
+ if (!gd->arch.timer_rate_hz)
+ timer_init();
+
+ return get_ticks();
+}
+
void __udelay(unsigned long usec)
{
unsigned long long tmp;
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 03/10] arm: armv8: generic_timer: Add timer_early functions
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
2022-09-21 14:06 ` [PATCH 01/10] arm: arch_timer: Add timer_early functions Stefan Roese
2022-09-21 14:06 ` [PATCH 02/10] arm: imx: syscounter: " Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-21 14:06 ` [PATCH 04/10] timer: cadence-ttc: " Stefan Roese
` (6 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini, Michal Simek, Andre Przywara
Currently this timer driver provides timer_get_boot_us() to support the
BOOTSTAGE functionality. This patch adds the timer_early functions so
that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
is enabled.
timer_get_boot_us() will get removed in a follow-up patch, once the
BOOTSTAGE interface is migrated to timer_get_us().
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Andre Przywara <andre.przywara@arm.com>
---
arch/arm/cpu/armv8/generic_timer.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/cpu/armv8/generic_timer.c b/arch/arm/cpu/armv8/generic_timer.c
index f27a74b9d09b..01a1a167311e 100644
--- a/arch/arm/cpu/armv8/generic_timer.c
+++ b/arch/arm/cpu/armv8/generic_timer.c
@@ -115,3 +115,13 @@ ulong timer_get_boot_us(void)
return val / get_tbclk();
}
+
+unsigned long notrace timer_early_get_rate(void)
+{
+ return get_tbclk();
+}
+
+u64 notrace timer_early_get_count(void)
+{
+ return get_ticks();
+}
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
` (2 preceding siblings ...)
2022-09-21 14:06 ` [PATCH 03/10] arm: armv8: generic_timer: " Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-21 14:06 ` [PATCH 05/10] timer: omap-timer: " Stefan Roese
` (5 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini, Michal Simek
Currently this timer driver provides timer_get_boot_us() to support the
BOOTSTAGE functionality. This patch adds the timer_early functions so
that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
is enabled.
timer_get_boot_us() will get removed in a follow-up patch, once the
BOOTSTAGE interface is migrated to timer_get_us().
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
---
drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
index 2eff45060ad6..e26c7923a140 100644
--- a/drivers/timer/cadence-ttc.c
+++ b/drivers/timer/cadence-ttc.c
@@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
}
#endif
+unsigned long notrace timer_early_get_rate(void)
+{
+ return 1;
+}
+
+u64 notrace timer_early_get_count(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;
+}
+
static u64 cadence_ttc_get_count(struct udevice *dev)
{
struct cadence_ttc_priv *priv = dev_get_priv(dev);
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 05/10] timer: omap-timer: Add timer_early functions
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
` (3 preceding siblings ...)
2022-09-21 14:06 ` [PATCH 04/10] timer: cadence-ttc: " Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-21 14:06 ` [PATCH 06/10] timer: rockchip_timer: " Stefan Roese
` (4 subsequent siblings)
9 siblings, 0 replies; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini, Christian Gmeiner, Dario Binacchi
Currently this timer driver provides timer_get_boot_us() to support the
BOOTSTAGE functionality. This patch adds the timer_early functions so
that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
is enabled.
timer_get_boot_us() will get removed in a follow-up patch, once the
BOOTSTAGE interface is migrated to timer_get_us().
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Dario Binacchi <dariobin@libero.it>
---
drivers/timer/omap-timer.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/timer/omap-timer.c b/drivers/timer/omap-timer.c
index aa2e4360c1bb..315dbb634aa7 100644
--- a/drivers/timer/omap-timer.c
+++ b/drivers/timer/omap-timer.c
@@ -106,6 +106,31 @@ ulong timer_get_boot_us(void)
}
#endif
+unsigned long notrace timer_early_get_rate(void)
+{
+ return 1;
+}
+
+u64 notrace timer_early_get_count(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;
+}
+
static const struct timer_ops omap_timer_ops = {
.get_count = omap_timer_get_count,
};
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 06/10] timer: rockchip_timer: Add timer_early functions
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
` (4 preceding siblings ...)
2022-09-21 14:06 ` [PATCH 05/10] timer: omap-timer: " Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-24 7:57 ` Kever Yang
2022-09-21 14:06 ` [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER Stefan Roese
` (3 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini, Kever Yang, Philipp Tomsich
Currently this timer driver provides timer_get_boot_us() to support the
BOOTSTAGE functionality. This patch adds the timer_early functions so
that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
is enabled.
timer_get_boot_us() will get removed in a follow-up patch, once the
BOOTSTAGE interface is migrated to timer_get_us().
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Kever Yang <kever.yang@rock-chips.com>
Cc: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
drivers/timer/rockchip_timer.c | 50 ++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/timer/rockchip_timer.c b/drivers/timer/rockchip_timer.c
index 62eacb986890..6e3483edce72 100644
--- a/drivers/timer/rockchip_timer.c
+++ b/drivers/timer/rockchip_timer.c
@@ -87,6 +87,56 @@ ulong timer_get_boot_us(void)
}
#endif
+static u64 timer_early_get_count_rate(uint32_t *rate)
+{
+ uint64_t ticks = 0;
+
+ *rate = 1;
+ if (CONFIG_IS_ENABLED(OF_REAL)) {
+ /* We have been called so early that the DM is not ready,... */
+ ofnode node = offset_to_ofnode(-1);
+ struct rk_timer *timer = NULL;
+
+ /*
+ * ... so we try to access the raw timer, if it is specified
+ * via the tick-timer property in /chosen.
+ */
+ node = ofnode_get_chosen_node("tick-timer");
+ if (!ofnode_valid(node)) {
+ debug("%s: no /chosen/tick-timer\n", __func__);
+ return 0;
+ }
+
+ timer = (struct rk_timer *)ofnode_get_addr(node);
+
+ /* This timer is down-counting */
+ ticks = ~0ULL - rockchip_timer_get_curr_value(timer);
+ if (ofnode_read_u32(node, "clock-frequency", rate)) {
+ debug("%s: could not read clock-frequency\n", __func__);
+ return 0;
+ }
+ } else {
+ return 0;
+ }
+
+ return ticks;
+}
+
+unsigned long notrace timer_early_get_rate(void)
+{
+ uint32_t rate;
+
+ timer_early_get_count_rate(&rate);
+ return rate;
+}
+
+u64 notrace timer_early_get_count(void)
+{
+ uint32_t rate;
+
+ return timer_early_get_count_rate(&rate);
+}
+
static u64 rockchip_timer_get_count(struct udevice *dev)
{
struct rockchip_timer_priv *priv = dev_get_priv(dev);
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
` (5 preceding siblings ...)
2022-09-21 14:06 ` [PATCH 06/10] timer: rockchip_timer: " Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-21 14:06 ` [PATCH 08/10] board_f/r: Don't call timer_init() when TIMER is enabled Stefan Roese
` (2 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini
The early timer functions provided via CONFIG_TIMER_EARLY don't need
CONFIG_TIMER to be enabled, as they don't make use of the DM timer
and uclass interface. This patch now allow the selection of
CONFIG_TIMER_EARLY w/o CONFIG_TIMER, enabling this early timer
functionality also for non CONFIG_TIMER drivers.
With this change it's necessary to guard the dm_timer_init() call
in initr_dm_devices() & initf_dm() additionally via CONFIG_TIMER.
Signed-off-by: Stefan Roese <sr@denx.de>
---
common/board_f.c | 2 +-
common/board_r.c | 2 +-
drivers/timer/Kconfig | 1 -
3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/common/board_f.c b/common/board_f.c
index f92d7b9faf4c..b1f67bfa72aa 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -787,7 +787,7 @@ static int initf_dm(void)
if (ret)
return ret;
- if (IS_ENABLED(CONFIG_TIMER_EARLY)) {
+ if (IS_ENABLED(CONFIG_TIMER) && IS_ENABLED(CONFIG_TIMER_EARLY)) {
ret = dm_timer_init();
if (ret)
return ret;
diff --git a/common/board_r.c b/common/board_r.c
index 50670b5615a5..56575e552df8 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -254,7 +254,7 @@ static int initr_dm_devices(void)
{
int ret;
- if (IS_ENABLED(CONFIG_TIMER_EARLY)) {
+ if (IS_ENABLED(CONFIG_TIMER) && IS_ENABLED(CONFIG_TIMER_EARLY)) {
ret = dm_timer_init();
if (ret)
return ret;
diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
index fd8745ffc2e0..91107b662b7e 100644
--- a/drivers/timer/Kconfig
+++ b/drivers/timer/Kconfig
@@ -39,7 +39,6 @@ config VPL_TIMER
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
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 08/10] board_f/r: Don't call timer_init() when TIMER is enabled
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
` (6 preceding siblings ...)
2022-09-21 14:06 ` [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-21 14:06 ` [PATCH 09/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
2022-09-21 14:06 ` [PATCH 10/10] bootstage/timer: Treewide remove timer_get_boot_us() Stefan Roese
9 siblings, 0 replies; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini
The timer initialization is done implicitly when CONFIG_TIMER is
enabled. There is no need to re-configure the timer again. Even worse,
another call to the timer init function may lead to a re-init of the
counter register and therefore non consecutive timer values.
This patch makes sure, that timer_init() is not called in case that
CONFIG_TIMER is configured.
Signed-off-by: Stefan Roese <sr@denx.de>
---
common/board_f.c | 2 +-
common/board_r.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/common/board_f.c b/common/board_f.c
index b1f67bfa72aa..fbc5d3e9d205 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -851,7 +851,7 @@ static const init_fnc_t init_sequence_f[] = {
/* get CPU and bus clocks according to the environment variable */
get_clocks, /* get CPU and bus clocks (etc.) */
#endif
-#if !defined(CONFIG_M68K)
+#if !defined(CONFIG_M68K) && !IS_ENABLED(CONFIG_TIMER)
timer_init, /* initialize timer */
#endif
#if defined(CONFIG_BOARD_POSTCLK_INIT)
diff --git a/common/board_r.c b/common/board_r.c
index 56575e552df8..149cfdc3d351 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -745,7 +745,8 @@ static init_fnc_t init_sequence_r[] = {
kgdb_init,
#endif
interrupt_init,
-#if defined(CONFIG_MICROBLAZE) || defined(CONFIG_M68K)
+#if (defined(CONFIG_MICROBLAZE) || defined(CONFIG_M68K)) && \
+ !IS_ENABLED(CONFIG_TIMER)
timer_init, /* initialize timer */
#endif
#if defined(CONFIG_LED_STATUS)
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 09/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us()
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
` (7 preceding siblings ...)
2022-09-21 14:06 ` [PATCH 08/10] board_f/r: Don't call timer_init() when TIMER is enabled Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-28 10:20 ` Simon Glass
2022-09-30 11:30 ` Michal Simek
2022-09-21 14:06 ` [PATCH 10/10] bootstage/timer: Treewide remove timer_get_boot_us() Stefan Roese
9 siblings, 2 replies; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot; +Cc: sjg, trini
This patch migrates the bootstage code from using the boot specific
timer_get_boot_us() timer function to the common timer_get_us()
function. This can only be done, also supporting the early boot phase,
when CONFIG_TIMER_EARLY is enabled. This way, the common timer functions
provide this functionality. Because of this, CONFIG_TIMER_EARLY is
now selected via CONFIG_BOOTSTAGE.
Additionally all 'uint32_t' timer occurances are changed to 'ulong', as
this is the return value of timer_get_us(). Otherwise this might lead
to 32bit vs 64bit conversion issues on 64bit platforms.
Also the start time of the bootstage recording is now saved. Otherwise
bigger timing offsets are seen on platforms where the timer does not
start with 0.
Signed-off-by: Stefan Roese <sr@denx.de>
---
boot/Kconfig | 1 +
common/bootstage.c | 26 ++++++++++++++------------
include/bootstage.h | 17 +++++------------
3 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig
index 6b3b8f072cb9..2ac7752a2eef 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -642,6 +642,7 @@ menu "Boot timing"
config BOOTSTAGE
bool "Boot timing and reporting"
+ select TIMER_EARLY
help
Enable recording of boot time while booting. To use it, insert
calls to bootstage_mark() with a suitable BOOTSTAGE_ID from
diff --git a/common/bootstage.c b/common/bootstage.c
index 326c40f1561f..6a655b20196b 100644
--- a/common/bootstage.c
+++ b/common/bootstage.c
@@ -30,7 +30,7 @@ enum {
struct bootstage_record {
ulong time_us;
- uint32_t start_us;
+ ulong start_us;
const char *name;
int flags; /* see enum bootstage_flags */
enum bootstage_id id;
@@ -39,6 +39,7 @@ struct bootstage_record {
struct bootstage_data {
uint rec_count;
uint next_id;
+ ulong start_time_us;
struct bootstage_record record[RECORD_COUNT];
};
@@ -132,7 +133,7 @@ ulong bootstage_add_record(enum bootstage_id id, const char *name,
if (!rec) {
if (data->rec_count < RECORD_COUNT) {
rec = &data->record[data->rec_count++];
- rec->time_us = mark;
+ rec->time_us = mark - data->start_time_us;
rec->name = name;
rec->flags = flags;
rec->id = id;
@@ -150,7 +151,7 @@ ulong bootstage_add_record(enum bootstage_id id, const char *name,
ulong bootstage_error_name(enum bootstage_id id, const char *name)
{
return bootstage_add_record(id, name, BOOTSTAGEF_ERROR,
- timer_get_boot_us());
+ timer_get_us());
}
ulong bootstage_mark_name(enum bootstage_id id, const char *name)
@@ -160,7 +161,7 @@ ulong bootstage_mark_name(enum bootstage_id id, const char *name)
if (id == BOOTSTAGE_ID_ALLOC)
flags = BOOTSTAGEF_ALLOC;
- return bootstage_add_record(id, name, flags, timer_get_boot_us());
+ return bootstage_add_record(id, name, flags, timer_get_us());
}
ulong bootstage_mark_code(const char *file, const char *func, int linenum)
@@ -190,11 +191,11 @@ ulong bootstage_mark_code(const char *file, const char *func, int linenum)
return bootstage_mark_name(BOOTSTAGE_ID_ALLOC, str);
}
-uint32_t bootstage_start(enum bootstage_id id, const char *name)
+ulong bootstage_start(enum bootstage_id id, const char *name)
{
struct bootstage_data *data = gd->bootstage;
struct bootstage_record *rec = ensure_id(data, id);
- ulong start_us = timer_get_boot_us();
+ ulong start_us = timer_get_us();
if (rec) {
rec->start_us = start_us;
@@ -204,15 +205,15 @@ uint32_t bootstage_start(enum bootstage_id id, const char *name)
return start_us;
}
-uint32_t bootstage_accum(enum bootstage_id id)
+ulong bootstage_accum(enum bootstage_id id)
{
struct bootstage_data *data = gd->bootstage;
struct bootstage_record *rec = ensure_id(data, id);
- uint32_t duration;
+ ulong duration;
if (!rec)
return 0;
- duration = (uint32_t)timer_get_boot_us() - rec->start_us;
+ duration = timer_get_us() - rec->start_us;
rec->time_us += duration;
return duration;
@@ -239,11 +240,11 @@ static const char *get_record_name(char *buf, int len,
return buf;
}
-static uint32_t print_time_record(struct bootstage_record *rec, uint32_t prev)
+static ulong print_time_record(struct bootstage_record *rec, ulong prev)
{
char buf[20];
- if (prev == -1U) {
+ if (prev == -1) {
printf("%11s", "");
print_grouped_ull(rec->time_us, BOOTSTAGE_DIGITS);
} else {
@@ -331,7 +332,7 @@ void bootstage_report(void)
{
struct bootstage_data *data = gd->bootstage;
struct bootstage_record *rec = data->record;
- uint32_t prev;
+ ulong prev;
int i;
printf("Timer summary in microseconds (%d records):\n",
@@ -530,6 +531,7 @@ int bootstage_init(bool first)
if (first) {
data->next_id = BOOTSTAGE_ID_USER;
bootstage_add_record(BOOTSTAGE_ID_AWAKE, "reset", 0, 0);
+ data->start_time_us = timer_get_us();
}
return 0;
diff --git a/include/bootstage.h b/include/bootstage.h
index 7088d0b875e4..e4516f5ca632 100644
--- a/include/bootstage.h
+++ b/include/bootstage.h
@@ -3,7 +3,7 @@
* This file implements recording of each stage of the boot process. It is
* intended to implement timing of each stage, reporting this information
* to the user and passing it to the OS for logging / further analysis.
- * Note that it requires timer_get_boot_us() to be defined by the board
+ * Note that it requires CONFIG_TIMER_EARLY to be defined by the board
*
* Copyright (c) 2011 The Chromium OS Authors.
*/
@@ -215,13 +215,6 @@ enum bootstage_id {
BOOTSTAGE_ID_ALLOC,
};
-/*
- * Return the time since boot in microseconds, This is needed for bootstage
- * and should be defined in CPU- or board-specific code. If undefined then
- * you will get a link error.
- */
-ulong timer_get_boot_us(void);
-
#if defined(USE_HOSTCC) || !CONFIG_IS_ENABLED(SHOW_BOOT_PROGRESS)
#define show_boot_progress(val) do {} while (0)
#else
@@ -313,7 +306,7 @@ ulong bootstage_mark_code(const char *file, const char *func,
* @param name Textual name to display for this id in the report (maybe NULL)
* Return: start timestamp in microseconds
*/
-uint32_t bootstage_start(enum bootstage_id id, const char *name);
+ulong bootstage_start(enum bootstage_id id, const char *name);
/**
* Mark the end of a bootstage activity
@@ -327,7 +320,7 @@ uint32_t bootstage_start(enum bootstage_id id, const char *name);
* less the start time recorded in the last bootstage_start() call
* with this id.
*/
-uint32_t bootstage_accum(enum bootstage_id id);
+ulong bootstage_accum(enum bootstage_id id);
/* Print a report about boot time */
void bootstage_report(void);
@@ -418,12 +411,12 @@ static inline ulong bootstage_mark_code(const char *file, const char *func,
return 0;
}
-static inline uint32_t bootstage_start(enum bootstage_id id, const char *name)
+static inline ulong bootstage_start(enum bootstage_id id, const char *name)
{
return 0;
}
-static inline uint32_t bootstage_accum(enum bootstage_id id)
+static inline ulong bootstage_accum(enum bootstage_id id)
{
return 0;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 10/10] bootstage/timer: Treewide remove timer_get_boot_us()
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
` (8 preceding siblings ...)
2022-09-21 14:06 ` [PATCH 09/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
@ 2022-09-21 14:06 ` Stefan Roese
2022-09-25 14:15 ` Simon Glass
9 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-21 14:06 UTC (permalink / raw)
To: u-boot
Cc: sjg, trini, Patrick Delaunay, Jun Nie, Shawn Guo, Fabio Estevam,
Stefano Babic, Michal Simek, Andre Przywara, Christian Gmeiner,
Dario Binacchi, Kever Yang, Philipp Tomsich
With the bootstage migration to timer_get_us() provided via
CONFIG_TIMER_EARLY timer_get_boot_us() is superfluous now. This patch
removes all occurances from the code.
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Jun Nie <jun.nie@linaro.org>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Fabio Estevam <festevam@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Dario Binacchi <dariobin@libero.it>
Cc: Kever Yang <kever.yang@rock-chips.com>
Cc: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
arch/arm/cpu/armv7/arch_timer.c | 9 ------
arch/arm/cpu/armv8/generic_timer.c | 7 -----
arch/arm/mach-imx/syscounter.c | 8 ------
arch/sandbox/cpu/cpu.c | 11 -------
drivers/timer/cadence-ttc.c | 22 --------------
drivers/timer/omap-timer.c | 22 --------------
drivers/timer/orion-timer.c | 8 ------
drivers/timer/rockchip_timer.c | 46 ------------------------------
drivers/timer/tsc_timer.c | 5 ----
lib/time.c | 20 -------------
10 files changed, 158 deletions(-)
diff --git a/arch/arm/cpu/armv7/arch_timer.c b/arch/arm/cpu/armv7/arch_timer.c
index 80cfaf2d09a4..375b7e2797ef 100644
--- a/arch/arm/cpu/armv7/arch_timer.c
+++ b/arch/arm/cpu/armv7/arch_timer.c
@@ -49,15 +49,6 @@ unsigned long long get_ticks(void)
return (((unsigned long long)gd->arch.tbu) << 32) | gd->arch.tbl;
}
-
-ulong timer_get_boot_us(void)
-{
- if (!gd->arch.timer_rate_hz)
- timer_init();
-
- return lldiv(get_ticks(), gd->arch.timer_rate_hz / 1000000);
-}
-
ulong get_tbclk(void)
{
return gd->arch.timer_rate_hz;
diff --git a/arch/arm/cpu/armv8/generic_timer.c b/arch/arm/cpu/armv8/generic_timer.c
index 01a1a167311e..680c547eaf7d 100644
--- a/arch/arm/cpu/armv8/generic_timer.c
+++ b/arch/arm/cpu/armv8/generic_timer.c
@@ -109,13 +109,6 @@ unsigned long usec2ticks(unsigned long usec)
return ticks;
}
-ulong timer_get_boot_us(void)
-{
- u64 val = get_ticks() * 1000000;
-
- return val / get_tbclk();
-}
-
unsigned long notrace timer_early_get_rate(void)
{
return get_tbclk();
diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c
index dbe55ee3913d..d592329c163e 100644
--- a/arch/arm/mach-imx/syscounter.c
+++ b/arch/arm/mach-imx/syscounter.c
@@ -101,14 +101,6 @@ ulong get_timer(ulong base)
return tick_to_time(get_ticks()) - base;
}
-ulong timer_get_boot_us(void)
-{
- if (!gd->arch.timer_rate_hz)
- timer_init();
-
- return tick_to_time(get_ticks());
-}
-
unsigned long notrace timer_early_get_rate(void)
{
if (!gd->arch.timer_rate_hz)
diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index d077948dd7bd..9c99839a1044 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -362,14 +362,3 @@ done:
fail:
return NULL;
}
-
-ulong timer_get_boot_us(void)
-{
- static uint64_t base_count;
- uint64_t count = os_get_nsec();
-
- if (!base_count)
- base_count = count;
-
- return (count - base_count) / 1000;
-}
diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
index e26c7923a140..39b97825318b 100644
--- a/drivers/timer/cadence-ttc.c
+++ b/drivers/timer/cadence-ttc.c
@@ -36,28 +36,6 @@ struct cadence_ttc_priv {
struct cadence_ttc_regs *regs;
};
-#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
-
unsigned long notrace timer_early_get_rate(void)
{
return 1;
diff --git a/drivers/timer/omap-timer.c b/drivers/timer/omap-timer.c
index 315dbb634aa7..c680c4487154 100644
--- a/drivers/timer/omap-timer.c
+++ b/drivers/timer/omap-timer.c
@@ -84,28 +84,6 @@ static int omap_timer_of_to_plat(struct udevice *dev)
return 0;
}
-#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
-
unsigned long notrace timer_early_get_rate(void)
{
return 1;
diff --git a/drivers/timer/orion-timer.c b/drivers/timer/orion-timer.c
index d0eab3ce781d..c0eadff81cf2 100644
--- a/drivers/timer/orion-timer.c
+++ b/drivers/timer/orion-timer.c
@@ -86,14 +86,6 @@ u64 notrace timer_early_get_count(void)
return orion_timer_get_count((void *)MVEBU_TIMER_BASE);
}
-ulong timer_get_boot_us(void)
-{
- u64 ticks;
-
- ticks = timer_early_get_count();
- return lldiv(ticks * 1000, timer_early_get_rate());
-}
-
/* DM timer functions */
static uint64_t dm_orion_timer_get_count(struct udevice *dev)
{
diff --git a/drivers/timer/rockchip_timer.c b/drivers/timer/rockchip_timer.c
index 6e3483edce72..7bf79d5f073e 100644
--- a/drivers/timer/rockchip_timer.c
+++ b/drivers/timer/rockchip_timer.c
@@ -41,52 +41,6 @@ static inline int64_t rockchip_timer_get_curr_value(struct rk_timer *timer)
return cntr;
}
-#if CONFIG_IS_ENABLED(BOOTSTAGE)
-ulong timer_get_boot_us(void)
-{
- uint64_t ticks = 0;
- uint32_t rate;
- uint64_t 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 if (CONFIG_IS_ENABLED(OF_REAL) && ret == -EAGAIN) {
- /* We have been called so early that the DM is not ready,... */
- ofnode node = offset_to_ofnode(-1);
- struct rk_timer *timer = NULL;
-
- /*
- * ... so we try to access the raw timer, if it is specified
- * via the tick-timer property in /chosen.
- */
- node = ofnode_get_chosen_node("tick-timer");
- if (!ofnode_valid(node)) {
- debug("%s: no /chosen/tick-timer\n", __func__);
- return 0;
- }
-
- timer = (struct rk_timer *)ofnode_get_addr(node);
-
- /* This timer is down-counting */
- ticks = ~0uLL - rockchip_timer_get_curr_value(timer);
- if (ofnode_read_u32(node, "clock-frequency", &rate)) {
- debug("%s: could not read clock-frequency\n", __func__);
- return 0;
- }
- } else {
- return 0;
- }
-
- us = (ticks * 1000) / rate;
- return us;
-}
-#endif
-
static u64 timer_early_get_count_rate(uint32_t *rate)
{
uint64_t ticks = 0;
diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
index 192c7b71a5a3..3cb2abed0c67 100644
--- a/drivers/timer/tsc_timer.c
+++ b/drivers/timer/tsc_timer.c
@@ -363,11 +363,6 @@ ulong notrace timer_get_us(void)
return get_ticks() / get_tbclk_mhz();
}
-ulong timer_get_boot_us(void)
-{
- return timer_get_us();
-}
-
void __udelay(unsigned long usec)
{
u64 now = get_ticks();
diff --git a/lib/time.c b/lib/time.c
index f3aaf472d103..7492b2bd9c62 100644
--- a/lib/time.c
+++ b/lib/time.c
@@ -42,26 +42,6 @@ unsigned long notrace timer_read_counter(void)
return readl(CONFIG_SYS_TIMER_COUNTER);
#endif
}
-
-ulong timer_get_boot_us(void)
-{
- ulong count = timer_read_counter();
-
-#ifdef CONFIG_SYS_TIMER_RATE
- const ulong timer_rate = CONFIG_SYS_TIMER_RATE;
-
- if (timer_rate == 1000000)
- return count;
- else if (timer_rate > 1000000)
- return lldiv(count, timer_rate / 1000000);
- else
- return (unsigned long long)count * 1000000 / timer_rate;
-#else
- /* Assume the counter is in microseconds */
- return count;
-#endif
-}
-
#else
extern unsigned long __weak timer_read_counter(void);
#endif
--
2.37.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 06/10] timer: rockchip_timer: Add timer_early functions
2022-09-21 14:06 ` [PATCH 06/10] timer: rockchip_timer: " Stefan Roese
@ 2022-09-24 7:57 ` Kever Yang
0 siblings, 0 replies; 33+ messages in thread
From: Kever Yang @ 2022-09-24 7:57 UTC (permalink / raw)
To: Stefan Roese, u-boot; +Cc: sjg, trini, Philipp Tomsich
On 2022/9/21 22:06, Stefan Roese wrote:
> Currently this timer driver provides timer_get_boot_us() to support the
> BOOTSTAGE functionality. This patch adds the timer_early functions so
> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
> is enabled.
>
> timer_get_boot_us() will get removed in a follow-up patch, once the
> BOOTSTAGE interface is migrated to timer_get_us().
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Kever Yang <kever.yang@rock-chips.com>
> Cc: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Thanks,
- Kever
> ---
> drivers/timer/rockchip_timer.c | 50 ++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/drivers/timer/rockchip_timer.c b/drivers/timer/rockchip_timer.c
> index 62eacb986890..6e3483edce72 100644
> --- a/drivers/timer/rockchip_timer.c
> +++ b/drivers/timer/rockchip_timer.c
> @@ -87,6 +87,56 @@ ulong timer_get_boot_us(void)
> }
> #endif
>
> +static u64 timer_early_get_count_rate(uint32_t *rate)
> +{
> + uint64_t ticks = 0;
> +
> + *rate = 1;
> + if (CONFIG_IS_ENABLED(OF_REAL)) {
> + /* We have been called so early that the DM is not ready,... */
> + ofnode node = offset_to_ofnode(-1);
> + struct rk_timer *timer = NULL;
> +
> + /*
> + * ... so we try to access the raw timer, if it is specified
> + * via the tick-timer property in /chosen.
> + */
> + node = ofnode_get_chosen_node("tick-timer");
> + if (!ofnode_valid(node)) {
> + debug("%s: no /chosen/tick-timer\n", __func__);
> + return 0;
> + }
> +
> + timer = (struct rk_timer *)ofnode_get_addr(node);
> +
> + /* This timer is down-counting */
> + ticks = ~0ULL - rockchip_timer_get_curr_value(timer);
> + if (ofnode_read_u32(node, "clock-frequency", rate)) {
> + debug("%s: could not read clock-frequency\n", __func__);
> + return 0;
> + }
> + } else {
> + return 0;
> + }
> +
> + return ticks;
> +}
> +
> +unsigned long notrace timer_early_get_rate(void)
> +{
> + uint32_t rate;
> +
> + timer_early_get_count_rate(&rate);
> + return rate;
> +}
> +
> +u64 notrace timer_early_get_count(void)
> +{
> + uint32_t rate;
> +
> + return timer_early_get_count_rate(&rate);
> +}
> +
> static u64 rockchip_timer_get_count(struct udevice *dev)
> {
> struct rockchip_timer_priv *priv = dev_get_priv(dev);
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 01/10] arm: arch_timer: Add timer_early functions
2022-09-21 14:06 ` [PATCH 01/10] arm: arch_timer: Add timer_early functions Stefan Roese
@ 2022-09-25 14:15 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2022-09-25 14:15 UTC (permalink / raw)
To: Stefan Roese; +Cc: U-Boot Mailing List, Tom Rini, Patrick Delaunay
On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>
> Currently this timer driver provides timer_get_boot_us() to support the
> BOOTSTAGE functionality. This patch adds the timer_early functions so
> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
> is enabled.
>
> timer_get_boot_us() will get removed in a follow-up patch, once the
> BOOTSTAGE interface is migrated to timer_get_us().
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> ---
> arch/arm/cpu/armv7/arch_timer.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 02/10] arm: imx: syscounter: Add timer_early functions
2022-09-21 14:06 ` [PATCH 02/10] arm: imx: syscounter: " Stefan Roese
@ 2022-09-25 14:15 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2022-09-25 14:15 UTC (permalink / raw)
To: Stefan Roese
Cc: U-Boot Mailing List, Tom Rini, Jun Nie, Shawn Guo, Fabio Estevam,
Stefano Babic
On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>
> Currently this timer driver provides timer_get_boot_us() to support the
> BOOTSTAGE functionality. This patch adds the timer_early functions so
> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
> is enabled.
>
> timer_get_boot_us() will get removed in a follow-up patch, once the
> BOOTSTAGE interface is migrated to timer_get_us().
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Jun Nie <jun.nie@linaro.org>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Fabio Estevam <festevam@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
> arch/arm/mach-imx/syscounter.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 03/10] arm: armv8: generic_timer: Add timer_early functions
2022-09-21 14:06 ` [PATCH 03/10] arm: armv8: generic_timer: " Stefan Roese
@ 2022-09-25 14:15 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2022-09-25 14:15 UTC (permalink / raw)
To: Stefan Roese; +Cc: U-Boot Mailing List, Tom Rini, Michal Simek, Andre Przywara
On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>
> Currently this timer driver provides timer_get_boot_us() to support the
> BOOTSTAGE functionality. This patch adds the timer_early functions so
> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
> is enabled.
>
> timer_get_boot_us() will get removed in a follow-up patch, once the
> BOOTSTAGE interface is migrated to timer_get_us().
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> ---
> arch/arm/cpu/armv8/generic_timer.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-09-21 14:06 ` [PATCH 04/10] timer: cadence-ttc: " Stefan Roese
@ 2022-09-25 14:15 ` Simon Glass
2022-09-26 14:11 ` Stefan Roese
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2022-09-25 14:15 UTC (permalink / raw)
To: Stefan Roese; +Cc: U-Boot Mailing List, Tom Rini, Michal Simek
Hi Stefan,
On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>
> Currently this timer driver provides timer_get_boot_us() to support the
> BOOTSTAGE functionality. This patch adds the timer_early functions so
> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
> is enabled.
>
> timer_get_boot_us() will get removed in a follow-up patch, once the
> BOOTSTAGE interface is migrated to timer_get_us().
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> ---
> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
> index 2eff45060ad6..e26c7923a140 100644
> --- a/drivers/timer/cadence-ttc.c
> +++ b/drivers/timer/cadence-ttc.c
> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
> }
> #endif
>
> +unsigned long notrace timer_early_get_rate(void)
> +{
> + return 1;
> +}
> +
> +u64 notrace timer_early_get_count(void)
> +{
> + u64 ticks = 0;
> + u32 rate = 1;
> + u64 us;
> + int ret;
> +
> + ret = dm_timer_init();
I don't think you can call this if you want to support bootstage,
since driver model may not be inited.
> + 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;
> +}
> +
> static u64 cadence_ttc_get_count(struct udevice *dev)
> {
> struct cadence_ttc_priv *priv = dev_get_priv(dev);
> --
> 2.37.3
>
REgards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
2022-09-21 14:06 ` [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER Stefan Roese
@ 2022-09-25 14:15 ` Simon Glass
2022-09-26 13:52 ` Stefan Roese
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2022-09-25 14:15 UTC (permalink / raw)
To: Stefan Roese; +Cc: U-Boot Mailing List, Tom Rini
Hi Stefan,
On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>
> The early timer functions provided via CONFIG_TIMER_EARLY don't need
> CONFIG_TIMER to be enabled, as they don't make use of the DM timer
> and uclass interface. This patch now allow the selection of
> CONFIG_TIMER_EARLY w/o CONFIG_TIMER, enabling this early timer
> functionality also for non CONFIG_TIMER drivers.
>
> With this change it's necessary to guard the dm_timer_init() call
> in initr_dm_devices() & initf_dm() additionally via CONFIG_TIMER.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
> common/board_f.c | 2 +-
> common/board_r.c | 2 +-
> drivers/timer/Kconfig | 1 -
> 3 files changed, 2 insertions(+), 3 deletions(-)
I don't like this as it complicates the logic and also seems to be
adding a new feature to legacy code.
Instead, let's enable the early timer only for driver model.
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 10/10] bootstage/timer: Treewide remove timer_get_boot_us()
2022-09-21 14:06 ` [PATCH 10/10] bootstage/timer: Treewide remove timer_get_boot_us() Stefan Roese
@ 2022-09-25 14:15 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2022-09-25 14:15 UTC (permalink / raw)
To: Stefan Roese
Cc: U-Boot Mailing List, Tom Rini, Patrick Delaunay, Jun Nie,
Shawn Guo, Fabio Estevam, Stefano Babic, Michal Simek,
Andre Przywara, Christian Gmeiner, Dario Binacchi, Kever Yang,
Philipp Tomsich
On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>
> With the bootstage migration to timer_get_us() provided via
> CONFIG_TIMER_EARLY timer_get_boot_us() is superfluous now. This patch
> removes all occurances from the code.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Jun Nie <jun.nie@linaro.org>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Fabio Estevam <festevam@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: Dario Binacchi <dariobin@libero.it>
> Cc: Kever Yang <kever.yang@rock-chips.com>
> Cc: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
> arch/arm/cpu/armv7/arch_timer.c | 9 ------
> arch/arm/cpu/armv8/generic_timer.c | 7 -----
> arch/arm/mach-imx/syscounter.c | 8 ------
> arch/sandbox/cpu/cpu.c | 11 -------
> drivers/timer/cadence-ttc.c | 22 --------------
> drivers/timer/omap-timer.c | 22 --------------
> drivers/timer/orion-timer.c | 8 ------
> drivers/timer/rockchip_timer.c | 46 ------------------------------
> drivers/timer/tsc_timer.c | 5 ----
> lib/time.c | 20 -------------
> 10 files changed, 158 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
2022-09-25 14:15 ` Simon Glass
@ 2022-09-26 13:52 ` Stefan Roese
2022-09-28 1:54 ` Simon Glass
0 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-26 13:52 UTC (permalink / raw)
To: Simon Glass; +Cc: U-Boot Mailing List, Tom Rini
Hi Simon,
On 25.09.22 16:15, Simon Glass wrote:
> Hi Stefan,
>
> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>
>> The early timer functions provided via CONFIG_TIMER_EARLY don't need
>> CONFIG_TIMER to be enabled, as they don't make use of the DM timer
>> and uclass interface. This patch now allow the selection of
>> CONFIG_TIMER_EARLY w/o CONFIG_TIMER, enabling this early timer
>> functionality also for non CONFIG_TIMER drivers.
>>
>> With this change it's necessary to guard the dm_timer_init() call
>> in initr_dm_devices() & initf_dm() additionally via CONFIG_TIMER.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> ---
>> common/board_f.c | 2 +-
>> common/board_r.c | 2 +-
>> drivers/timer/Kconfig | 1 -
>> 3 files changed, 2 insertions(+), 3 deletions(-)
>
> I don't like this as it complicates the logic and also seems to be
> adding a new feature to legacy code.
>
> Instead, let's enable the early timer only for driver model.
Hmmm, not sure how this should work. Do you have this in mind (instead
of this patch)?
diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
index fd8745ffc2e0..30d6efe98f29 100644
--- a/drivers/timer/Kconfig
+++ b/drivers/timer/Kconfig
@@ -39,7 +39,7 @@ config VPL_TIMER
config TIMER_EARLY
bool "Allow timer to be used early in U-Boot"
- depends on TIMER
+ depends on DM
# initr_bootstage() requires a timer and is called before
initr_dm()
# so only the early timer is available
default y if X86 && BOOTSTAGE
This results in some compilation errors, like this:
$ make stm32mp15_basic_defconfig
$ make -sj
/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
common/board_f.o: in function `initf_dm':
/home/stefan/git/u-boot/u-boot-marvell/common/board_f.c:791: undefined
reference to `dm_timer_init'
/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
common/board_r.o: in function `initr_dm_devices':
/home/stefan/git/u-boot/u-boot-marvell/common/board_r.c:258: undefined
reference to `dm_timer_init'
make: *** [Makefile:1790: u-boot] Error 1
I might be missing something. Or it's not that easy and we still need
my original implementation.
Thanks,
Stefan
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-09-25 14:15 ` Simon Glass
@ 2022-09-26 14:11 ` Stefan Roese
2022-09-28 1:54 ` Simon Glass
0 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-26 14:11 UTC (permalink / raw)
To: Simon Glass, Michal Simek; +Cc: U-Boot Mailing List, Tom Rini
Hi Simon,
Hi Michal,
On 25.09.22 16:15, Simon Glass wrote:
> Hi Stefan,
>
> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>
>> Currently this timer driver provides timer_get_boot_us() to support the
>> BOOTSTAGE functionality. This patch adds the timer_early functions so
>> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
>> is enabled.
>>
>> timer_get_boot_us() will get removed in a follow-up patch, once the
>> BOOTSTAGE interface is migrated to timer_get_us().
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> ---
>> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
>> index 2eff45060ad6..e26c7923a140 100644
>> --- a/drivers/timer/cadence-ttc.c
>> +++ b/drivers/timer/cadence-ttc.c
>> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
>> }
>> #endif
>>
>> +unsigned long notrace timer_early_get_rate(void)
>> +{
>> + return 1;
>> +}
>> +
>> +u64 notrace timer_early_get_count(void)
>> +{
>> + u64 ticks = 0;
>> + u32 rate = 1;
>> + u64 us;
>> + int ret;
>> +
>> + ret = dm_timer_init();
>
> I don't think you can call this if you want to support bootstage,
> since driver model may not be inited.
Yes, thanks for noticing. Still, this code is copied from the original
timer_get_boot_us() function in this driver. Which also has problems
with early timer access AFAICT.
Michal, you are the author of the timer_get_boot_us() implementation
in commit 56c0e646c4f6a ("timer: cadence: Implement timer_get_boot_us").
How is this supposed to work in the early boot phase, before DM is
initialized?
Thanks,
Stefan
>> + 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;
>> +}
>> +
>> static u64 cadence_ttc_get_count(struct udevice *dev)
>> {
>> struct cadence_ttc_priv *priv = dev_get_priv(dev);
>> --
>> 2.37.3
>>
>
> REgards,
> Simon
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-09-26 14:11 ` Stefan Roese
@ 2022-09-28 1:54 ` Simon Glass
2022-09-30 12:02 ` Michal Simek
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2022-09-28 1:54 UTC (permalink / raw)
To: Stefan Roese; +Cc: Michal Simek, U-Boot Mailing List, Tom Rini
On Mon, 26 Sept 2022 at 08:11, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
> Hi Michal,
>
> On 25.09.22 16:15, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
> >>
> >> Currently this timer driver provides timer_get_boot_us() to support the
> >> BOOTSTAGE functionality. This patch adds the timer_early functions so
> >> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
> >> is enabled.
> >>
> >> timer_get_boot_us() will get removed in a follow-up patch, once the
> >> BOOTSTAGE interface is migrated to timer_get_us().
> >>
> >> Signed-off-by: Stefan Roese <sr@denx.de>
> >> Cc: Michal Simek <michal.simek@xilinx.com>
> >> ---
> >> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
> >> 1 file changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
> >> index 2eff45060ad6..e26c7923a140 100644
> >> --- a/drivers/timer/cadence-ttc.c
> >> +++ b/drivers/timer/cadence-ttc.c
> >> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
> >> }
> >> #endif
> >>
> >> +unsigned long notrace timer_early_get_rate(void)
> >> +{
> >> + return 1;
> >> +}
> >> +
> >> +u64 notrace timer_early_get_count(void)
> >> +{
> >> + u64 ticks = 0;
> >> + u32 rate = 1;
> >> + u64 us;
> >> + int ret;
> >> +
> >> + ret = dm_timer_init();
> >
> > I don't think you can call this if you want to support bootstage,
> > since driver model may not be inited.
>
> Yes, thanks for noticing. Still, this code is copied from the original
> timer_get_boot_us() function in this driver. Which also has problems
> with early timer access AFAICT.
Yes, good point.
Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Michal, you are the author of the timer_get_boot_us() implementation
> in commit 56c0e646c4f6a ("timer: cadence: Implement timer_get_boot_us").
> How is this supposed to work in the early boot phase, before DM is
> initialized?
>
> Thanks,
> Stefan
>
> >> + 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;
> >> +}
> >> +
> >> static u64 cadence_ttc_get_count(struct udevice *dev)
> >> {
> >> struct cadence_ttc_priv *priv = dev_get_priv(dev);
> >> --
> >> 2.37.3
> >>
> >
> > REgards,
> > Simon
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
2022-09-26 13:52 ` Stefan Roese
@ 2022-09-28 1:54 ` Simon Glass
2022-09-30 5:36 ` Stefan Roese
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2022-09-28 1:54 UTC (permalink / raw)
To: Stefan Roese; +Cc: U-Boot Mailing List, Tom Rini
Hi Stefan,
On Mon, 26 Sept 2022 at 07:52, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 25.09.22 16:15, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
> >>
> >> The early timer functions provided via CONFIG_TIMER_EARLY don't need
> >> CONFIG_TIMER to be enabled, as they don't make use of the DM timer
> >> and uclass interface. This patch now allow the selection of
> >> CONFIG_TIMER_EARLY w/o CONFIG_TIMER, enabling this early timer
> >> functionality also for non CONFIG_TIMER drivers.
> >>
> >> With this change it's necessary to guard the dm_timer_init() call
> >> in initr_dm_devices() & initf_dm() additionally via CONFIG_TIMER.
> >>
> >> Signed-off-by: Stefan Roese <sr@denx.de>
> >> ---
> >> common/board_f.c | 2 +-
> >> common/board_r.c | 2 +-
> >> drivers/timer/Kconfig | 1 -
> >> 3 files changed, 2 insertions(+), 3 deletions(-)
> >
> > I don't like this as it complicates the logic and also seems to be
> > adding a new feature to legacy code.
> >
> > Instead, let's enable the early timer only for driver model.
>
> Hmmm, not sure how this should work. Do you have this in mind (instead
> of this patch)?
>
> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> index fd8745ffc2e0..30d6efe98f29 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -39,7 +39,7 @@ config VPL_TIMER
>
> config TIMER_EARLY
> bool "Allow timer to be used early in U-Boot"
> - depends on TIMER
> + depends on DM
> # initr_bootstage() requires a timer and is called before
> initr_dm()
> # so only the early timer is available
> default y if X86 && BOOTSTAGE
>
> This results in some compilation errors, like this:
>
> $ make stm32mp15_basic_defconfig
> $ make -sj
> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> common/board_f.o: in function `initf_dm':
> /home/stefan/git/u-boot/u-boot-marvell/common/board_f.c:791: undefined
> reference to `dm_timer_init'
> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> common/board_r.o: in function `initr_dm_devices':
> /home/stefan/git/u-boot/u-boot-marvell/common/board_r.c:258: undefined
> reference to `dm_timer_init'
> make: *** [Makefile:1790: u-boot] Error 1
>
> I might be missing something. Or it's not that easy and we still need
> my original implementation.
Well, given that TIMER depends on DM, I don't think you need to change that.
Also, TIMER_EARLY depends on TIMER (as it should), you should just be
able to check for IS_ENABLED(CONFIG_TIMER)
We can disable the early timer stuff for things that don't use CONFIG_TIMER
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 09/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us()
2022-09-21 14:06 ` [PATCH 09/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
@ 2022-09-28 10:20 ` Simon Glass
2022-09-30 11:30 ` Michal Simek
1 sibling, 0 replies; 33+ messages in thread
From: Simon Glass @ 2022-09-28 10:20 UTC (permalink / raw)
To: Stefan Roese; +Cc: U-Boot Mailing List, Tom Rini
On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>
> This patch migrates the bootstage code from using the boot specific
> timer_get_boot_us() timer function to the common timer_get_us()
> function. This can only be done, also supporting the early boot phase,
> when CONFIG_TIMER_EARLY is enabled. This way, the common timer functions
> provide this functionality. Because of this, CONFIG_TIMER_EARLY is
> now selected via CONFIG_BOOTSTAGE.
>
> Additionally all 'uint32_t' timer occurances are changed to 'ulong', as
> this is the return value of timer_get_us(). Otherwise this might lead
> to 32bit vs 64bit conversion issues on 64bit platforms.
>
> Also the start time of the bootstage recording is now saved. Otherwise
> bigger timing offsets are seen on platforms where the timer does not
> start with 0.
Can you split this into three patches? You are changing three
different things at once. Also please add a comment for start offset
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
> boot/Kconfig | 1 +
> common/bootstage.c | 26 ++++++++++++++------------
> include/bootstage.h | 17 +++++------------
> 3 files changed, 20 insertions(+), 24 deletions(-)
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
2022-09-28 1:54 ` Simon Glass
@ 2022-09-30 5:36 ` Stefan Roese
2022-09-30 13:28 ` Simon Glass
0 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-30 5:36 UTC (permalink / raw)
To: Simon Glass; +Cc: U-Boot Mailing List, Tom Rini
Hi Simon,
On 28.09.22 03:54, Simon Glass wrote:
> Hi Stefan,
>
> On Mon, 26 Sept 2022 at 07:52, Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Simon,
>>
>> On 25.09.22 16:15, Simon Glass wrote:
>>> Hi Stefan,
>>>
>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> The early timer functions provided via CONFIG_TIMER_EARLY don't need
>>>> CONFIG_TIMER to be enabled, as they don't make use of the DM timer
>>>> and uclass interface. This patch now allow the selection of
>>>> CONFIG_TIMER_EARLY w/o CONFIG_TIMER, enabling this early timer
>>>> functionality also for non CONFIG_TIMER drivers.
>>>>
>>>> With this change it's necessary to guard the dm_timer_init() call
>>>> in initr_dm_devices() & initf_dm() additionally via CONFIG_TIMER.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> ---
>>>> common/board_f.c | 2 +-
>>>> common/board_r.c | 2 +-
>>>> drivers/timer/Kconfig | 1 -
>>>> 3 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> I don't like this as it complicates the logic and also seems to be
>>> adding a new feature to legacy code.
>>>
>>> Instead, let's enable the early timer only for driver model.
>>
>> Hmmm, not sure how this should work. Do you have this in mind (instead
>> of this patch)?
>>
>> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
>> index fd8745ffc2e0..30d6efe98f29 100644
>> --- a/drivers/timer/Kconfig
>> +++ b/drivers/timer/Kconfig
>> @@ -39,7 +39,7 @@ config VPL_TIMER
>>
>> config TIMER_EARLY
>> bool "Allow timer to be used early in U-Boot"
>> - depends on TIMER
>> + depends on DM
>> # initr_bootstage() requires a timer and is called before
>> initr_dm()
>> # so only the early timer is available
>> default y if X86 && BOOTSTAGE
>>
>> This results in some compilation errors, like this:
>>
>> $ make stm32mp15_basic_defconfig
>> $ make -sj
>> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
>> common/board_f.o: in function `initf_dm':
>> /home/stefan/git/u-boot/u-boot-marvell/common/board_f.c:791: undefined
>> reference to `dm_timer_init'
>> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
>> common/board_r.o: in function `initr_dm_devices':
>> /home/stefan/git/u-boot/u-boot-marvell/common/board_r.c:258: undefined
>> reference to `dm_timer_init'
>> make: *** [Makefile:1790: u-boot] Error 1
>>
>> I might be missing something. Or it's not that easy and we still need
>> my original implementation.
>
> Well, given that TIMER depends on DM, I don't think you need to change that.
>
> Also, TIMER_EARLY depends on TIMER (as it should), you should just be
> able to check for IS_ENABLED(CONFIG_TIMER)
>
> We can disable the early timer stuff for things that don't use CONFIG_TIMER
The result of this would be, that BOOTSTAGE is not available for
platforms that don't have TIMER_EARLY (& TIMER) enabled, like
stm32mp15_basic and some others. Actually I think this is a good
move, as it "encourages" people to move to enable CONFIG_TIMER.
So should I move forward this way? Depending BOOTSTAGE on TIMER?
Thanks,
Stefan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 09/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us()
2022-09-21 14:06 ` [PATCH 09/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
2022-09-28 10:20 ` Simon Glass
@ 2022-09-30 11:30 ` Michal Simek
1 sibling, 0 replies; 33+ messages in thread
From: Michal Simek @ 2022-09-30 11:30 UTC (permalink / raw)
To: Stefan Roese; +Cc: u-boot, sjg, trini
Hi Stefan,
st 21. 9. 2022 v 16:08 odesílatel Stefan Roese <sr@denx.de> napsal:
>
> This patch migrates the bootstage code from using the boot specific
> timer_get_boot_us() timer function to the common timer_get_us()
> function. This can only be done, also supporting the early boot phase,
> when CONFIG_TIMER_EARLY is enabled. This way, the common timer functions
> provide this functionality. Because of this, CONFIG_TIMER_EARLY is
> now selected via CONFIG_BOOTSTAGE.
>
> Additionally all 'uint32_t' timer occurances are changed to 'ulong', as
> this is the return value of timer_get_us(). Otherwise this might lead
> to 32bit vs 64bit conversion issues on 64bit platforms.
>
> Also the start time of the bootstage recording is now saved. Otherwise
> bigger timing offsets are seen on platforms where the timer does not
> start with 0.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
> boot/Kconfig | 1 +
> common/bootstage.c | 26 ++++++++++++++------------
> include/bootstage.h | 17 +++++------------
> 3 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index 6b3b8f072cb9..2ac7752a2eef 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -642,6 +642,7 @@ menu "Boot timing"
>
> config BOOTSTAGE
> bool "Boot timing and reporting"
> + select TIMER_EARLY
> help
> Enable recording of boot time while booting. To use it, insert
> calls to bootstage_mark() with a suitable BOOTSTAGE_ID from
> diff --git a/common/bootstage.c b/common/bootstage.c
> index 326c40f1561f..6a655b20196b 100644
> --- a/common/bootstage.c
> +++ b/common/bootstage.c
> @@ -30,7 +30,7 @@ enum {
>
> struct bootstage_record {
> ulong time_us;
> - uint32_t start_us;
> + ulong start_us;
> const char *name;
> int flags; /* see enum bootstage_flags */
> enum bootstage_id id;
> @@ -39,6 +39,7 @@ struct bootstage_record {
> struct bootstage_data {
> uint rec_count;
> uint next_id;
> + ulong start_time_us;
> struct bootstage_record record[RECORD_COUNT];
> };
>
> @@ -132,7 +133,7 @@ ulong bootstage_add_record(enum bootstage_id id, const char *name,
> if (!rec) {
> if (data->rec_count < RECORD_COUNT) {
> rec = &data->record[data->rec_count++];
> - rec->time_us = mark;
> + rec->time_us = mark - data->start_time_us;
> rec->name = name;
> rec->flags = flags;
> rec->id = id;
> @@ -150,7 +151,7 @@ ulong bootstage_add_record(enum bootstage_id id, const char *name,
> ulong bootstage_error_name(enum bootstage_id id, const char *name)
> {
> return bootstage_add_record(id, name, BOOTSTAGEF_ERROR,
> - timer_get_boot_us());
> + timer_get_us());
> }
>
> ulong bootstage_mark_name(enum bootstage_id id, const char *name)
> @@ -160,7 +161,7 @@ ulong bootstage_mark_name(enum bootstage_id id, const char *name)
> if (id == BOOTSTAGE_ID_ALLOC)
> flags = BOOTSTAGEF_ALLOC;
>
> - return bootstage_add_record(id, name, flags, timer_get_boot_us());
> + return bootstage_add_record(id, name, flags, timer_get_us());
> }
>
> ulong bootstage_mark_code(const char *file, const char *func, int linenum)
> @@ -190,11 +191,11 @@ ulong bootstage_mark_code(const char *file, const char *func, int linenum)
> return bootstage_mark_name(BOOTSTAGE_ID_ALLOC, str);
> }
>
> -uint32_t bootstage_start(enum bootstage_id id, const char *name)
> +ulong bootstage_start(enum bootstage_id id, const char *name)
> {
> struct bootstage_data *data = gd->bootstage;
> struct bootstage_record *rec = ensure_id(data, id);
> - ulong start_us = timer_get_boot_us();
> + ulong start_us = timer_get_us();
>
> if (rec) {
> rec->start_us = start_us;
> @@ -204,15 +205,15 @@ uint32_t bootstage_start(enum bootstage_id id, const char *name)
> return start_us;
> }
>
> -uint32_t bootstage_accum(enum bootstage_id id)
> +ulong bootstage_accum(enum bootstage_id id)
> {
> struct bootstage_data *data = gd->bootstage;
> struct bootstage_record *rec = ensure_id(data, id);
> - uint32_t duration;
> + ulong duration;
>
> if (!rec)
> return 0;
> - duration = (uint32_t)timer_get_boot_us() - rec->start_us;
> + duration = timer_get_us() - rec->start_us;
> rec->time_us += duration;
>
> return duration;
> @@ -239,11 +240,11 @@ static const char *get_record_name(char *buf, int len,
> return buf;
> }
>
> -static uint32_t print_time_record(struct bootstage_record *rec, uint32_t prev)
> +static ulong print_time_record(struct bootstage_record *rec, ulong prev)
> {
> char buf[20];
>
> - if (prev == -1U) {
> + if (prev == -1) {
> printf("%11s", "");
> print_grouped_ull(rec->time_us, BOOTSTAGE_DIGITS);
> } else {
> @@ -331,7 +332,7 @@ void bootstage_report(void)
> {
> struct bootstage_data *data = gd->bootstage;
> struct bootstage_record *rec = data->record;
> - uint32_t prev;
> + ulong prev;
> int i;
>
> printf("Timer summary in microseconds (%d records):\n",
> @@ -530,6 +531,7 @@ int bootstage_init(bool first)
> if (first) {
> data->next_id = BOOTSTAGE_ID_USER;
> bootstage_add_record(BOOTSTAGE_ID_AWAKE, "reset", 0, 0);
> + data->start_time_us = timer_get_us();
> }
>
> return 0;
> diff --git a/include/bootstage.h b/include/bootstage.h
> index 7088d0b875e4..e4516f5ca632 100644
> --- a/include/bootstage.h
> +++ b/include/bootstage.h
> @@ -3,7 +3,7 @@
> * This file implements recording of each stage of the boot process. It is
> * intended to implement timing of each stage, reporting this information
> * to the user and passing it to the OS for logging / further analysis.
> - * Note that it requires timer_get_boot_us() to be defined by the board
> + * Note that it requires CONFIG_TIMER_EARLY to be defined by the board
> *
> * Copyright (c) 2011 The Chromium OS Authors.
> */
> @@ -215,13 +215,6 @@ enum bootstage_id {
> BOOTSTAGE_ID_ALLOC,
> };
>
> -/*
> - * Return the time since boot in microseconds, This is needed for bootstage
> - * and should be defined in CPU- or board-specific code. If undefined then
> - * you will get a link error.
> - */
> -ulong timer_get_boot_us(void);
> -
> #if defined(USE_HOSTCC) || !CONFIG_IS_ENABLED(SHOW_BOOT_PROGRESS)
> #define show_boot_progress(val) do {} while (0)
> #else
> @@ -313,7 +306,7 @@ ulong bootstage_mark_code(const char *file, const char *func,
> * @param name Textual name to display for this id in the report (maybe NULL)
> * Return: start timestamp in microseconds
> */
> -uint32_t bootstage_start(enum bootstage_id id, const char *name);
> +ulong bootstage_start(enum bootstage_id id, const char *name);
>
> /**
> * Mark the end of a bootstage activity
> @@ -327,7 +320,7 @@ uint32_t bootstage_start(enum bootstage_id id, const char *name);
> * less the start time recorded in the last bootstage_start() call
> * with this id.
> */
> -uint32_t bootstage_accum(enum bootstage_id id);
> +ulong bootstage_accum(enum bootstage_id id);
>
> /* Print a report about boot time */
> void bootstage_report(void);
> @@ -418,12 +411,12 @@ static inline ulong bootstage_mark_code(const char *file, const char *func,
> return 0;
> }
>
> -static inline uint32_t bootstage_start(enum bootstage_id id, const char *name)
> +static inline ulong bootstage_start(enum bootstage_id id, const char *name)
> {
> return 0;
> }
>
> -static inline uint32_t bootstage_accum(enum bootstage_id id)
> +static inline ulong bootstage_accum(enum bootstage_id id)
> {
> return 0;
> }
> --
> 2.37.3
>
Just heads up this has been identified as a bad commit when I bisect
it for zynqmp_r5 configuration.
I will dig into it to find out what exactly is causing the issue. But
initf_dm() is failing now.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-09-28 1:54 ` Simon Glass
@ 2022-09-30 12:02 ` Michal Simek
2022-09-30 13:45 ` Stefan Roese
0 siblings, 1 reply; 33+ messages in thread
From: Michal Simek @ 2022-09-30 12:02 UTC (permalink / raw)
To: Simon Glass, Stefan Roese; +Cc: Michal Simek, U-Boot Mailing List, Tom Rini
Hi Simon and Stefan,
On 9/28/22 03:54, Simon Glass wrote:
>> Hi Simon,
>> Hi Michal,
>>
>> On 25.09.22 16:15, Simon Glass wrote:
>>> Hi Stefan,
>>>
>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> Currently this timer driver provides timer_get_boot_us() to support the
>>>> BOOTSTAGE functionality. This patch adds the timer_early functions so
>>>> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
>>>> is enabled.
>>>>
>>>> timer_get_boot_us() will get removed in a follow-up patch, once the
>>>> BOOTSTAGE interface is migrated to timer_get_us().
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>> ---
>>>> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
>>>> 1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
>>>> index 2eff45060ad6..e26c7923a140 100644
>>>> --- a/drivers/timer/cadence-ttc.c
>>>> +++ b/drivers/timer/cadence-ttc.c
>>>> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
>>>> }
>>>> #endif
>>>>
>>>> +unsigned long notrace timer_early_get_rate(void)
>>>> +{
>>>> + return 1;
>>>> +}
>>>> +
>>>> +u64 notrace timer_early_get_count(void)
>>>> +{
>>>> + u64 ticks = 0;
>>>> + u32 rate = 1;
>>>> + u64 us;
>>>> + int ret;
>>>> +
>>>> + ret = dm_timer_init();
>>>
>>> I don't think you can call this if you want to support bootstage,
>>> since driver model may not be inited.
>>
>> Yes, thanks for noticing. Still, this code is copied from the original
>> timer_get_boot_us() function in this driver. Which also has problems
>> with early timer access AFAICT.
>
> Yes, good point.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
I looked at it and kind of interesting code. For getting this working with this
whole series if ttc gets u-boot,dm-pre-reloc everthing is working fine.
diff --git a/arch/arm/dts/zynqmp-r5.dts b/arch/arm/dts/zynqmp-r5.dts
index a72172ef2ea4..8059931f2162 100644
--- a/arch/arm/dts/zynqmp-r5.dts
+++ b/arch/arm/dts/zynqmp-r5.dts
@@ -56,6 +56,7 @@
ttc0: timer@ff110000 {
compatible = "cdns,ttc";
+ u-boot,dm-pre-reloc;
status = "okay";
reg = <0xff110000 0x1000>;
timer-width = <32>;
What I see based on behavior. Every bootstage call is asking for
timer_early_get_count() and because dm_timer_init() is failing 0 is returned.
Inside initf_dm() dm_init_and_scan() is called and initialized timer uclass also
with timer instance. With u-boot,dm-pre-reloc also cadence timer driver.
On the next dm_timer_init() gd->timer is initialized and value is returned and
initf_dm() passes.
Here is the log and output from bootstage.
U-Boot 2022.10-rc5-00130-g1be5bed207c9 (Sep 30 2022 - 14:00:08 +0200)
Model: Xilinx ZynqMP R5
DRAM: 512 MiB
Core: 7 devices, 7 uclasses, devicetree: embed
MMC:
Loading Environment from nowhere... OK
In: serial@ff010000
Out: serial@ff010000
Err: serial@ff010000
Net: No ethernet found.
ZynqMP r5> dm tree
Class Index Probed Driver Name
-----------------------------------------------------------
root 0 [ + ] root_driver root_driver
clk 0 [ + ] fixed_clock |-- clk100
simple_bus 0 [ + ] simple_bus |-- amba
timer 0 [ + ] cadence_ttc | |-- timer@ff110000
serial 0 [ + ] serial_zynq | `-- serial@ff010000
bootstd 0 [ ] bootstd_drv `-- bootstd
bootmeth 0 [ ] bootmeth_distro `-- distro
ZynqMP r5> bootstage report
Timer summary in microseconds (8 records):
Mark Elapsed Stage
0 0 reset
0 0 board_init_f
0 0 dm_f
0 0 dm_r
16,319 16,319 eth_common_init
18,061 1,742 main_loop
18,281 220 cli_loop
29,249 10,968 board_init_r
Accumulated time:
Would be good if you can also add u-boot,dm-pre-reloc before 9/10 patch which
enables CONFIG_TIMER_EARLY.
Thanks,
Michal
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
2022-09-30 5:36 ` Stefan Roese
@ 2022-09-30 13:28 ` Simon Glass
2022-09-30 13:52 ` Stefan Roese
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2022-09-30 13:28 UTC (permalink / raw)
To: Stefan Roese; +Cc: U-Boot Mailing List, Tom Rini
Hi Stefan,
On Thu, 29 Sept 2022 at 23:36, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 28.09.22 03:54, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Mon, 26 Sept 2022 at 07:52, Stefan Roese <sr@denx.de> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 25.09.22 16:15, Simon Glass wrote:
> >>> Hi Stefan,
> >>>
> >>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
> >>>>
> >>>> The early timer functions provided via CONFIG_TIMER_EARLY don't need
> >>>> CONFIG_TIMER to be enabled, as they don't make use of the DM timer
> >>>> and uclass interface. This patch now allow the selection of
> >>>> CONFIG_TIMER_EARLY w/o CONFIG_TIMER, enabling this early timer
> >>>> functionality also for non CONFIG_TIMER drivers.
> >>>>
> >>>> With this change it's necessary to guard the dm_timer_init() call
> >>>> in initr_dm_devices() & initf_dm() additionally via CONFIG_TIMER.
> >>>>
> >>>> Signed-off-by: Stefan Roese <sr@denx.de>
> >>>> ---
> >>>> common/board_f.c | 2 +-
> >>>> common/board_r.c | 2 +-
> >>>> drivers/timer/Kconfig | 1 -
> >>>> 3 files changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> I don't like this as it complicates the logic and also seems to be
> >>> adding a new feature to legacy code.
> >>>
> >>> Instead, let's enable the early timer only for driver model.
> >>
> >> Hmmm, not sure how this should work. Do you have this in mind (instead
> >> of this patch)?
> >>
> >> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> >> index fd8745ffc2e0..30d6efe98f29 100644
> >> --- a/drivers/timer/Kconfig
> >> +++ b/drivers/timer/Kconfig
> >> @@ -39,7 +39,7 @@ config VPL_TIMER
> >>
> >> config TIMER_EARLY
> >> bool "Allow timer to be used early in U-Boot"
> >> - depends on TIMER
> >> + depends on DM
> >> # initr_bootstage() requires a timer and is called before
> >> initr_dm()
> >> # so only the early timer is available
> >> default y if X86 && BOOTSTAGE
> >>
> >> This results in some compilation errors, like this:
> >>
> >> $ make stm32mp15_basic_defconfig
> >> $ make -sj
> >> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> >> common/board_f.o: in function `initf_dm':
> >> /home/stefan/git/u-boot/u-boot-marvell/common/board_f.c:791: undefined
> >> reference to `dm_timer_init'
> >> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
> >> common/board_r.o: in function `initr_dm_devices':
> >> /home/stefan/git/u-boot/u-boot-marvell/common/board_r.c:258: undefined
> >> reference to `dm_timer_init'
> >> make: *** [Makefile:1790: u-boot] Error 1
> >>
> >> I might be missing something. Or it's not that easy and we still need
> >> my original implementation.
> >
> > Well, given that TIMER depends on DM, I don't think you need to change that.
> >
> > Also, TIMER_EARLY depends on TIMER (as it should), you should just be
> > able to check for IS_ENABLED(CONFIG_TIMER)
> >
> > We can disable the early timer stuff for things that don't use CONFIG_TIMER
>
> The result of this would be, that BOOTSTAGE is not available for
> platforms that don't have TIMER_EARLY (& TIMER) enabled, like
> stm32mp15_basic and some others. Actually I think this is a good
> move, as it "encourages" people to move to enable CONFIG_TIMER.
>
> So should I move forward this way? Depending BOOTSTAGE on TIMER?
Yes I think soas it is simplified things and we want people to migrate anyway.
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-09-30 12:02 ` Michal Simek
@ 2022-09-30 13:45 ` Stefan Roese
2022-10-04 11:49 ` Michal Simek
0 siblings, 1 reply; 33+ messages in thread
From: Stefan Roese @ 2022-09-30 13:45 UTC (permalink / raw)
To: Michal Simek, Simon Glass; +Cc: Michal Simek, U-Boot Mailing List, Tom Rini
Hi Michal,
On 30.09.22 14:02, Michal Simek wrote:
> Hi Simon and Stefan,
>
> On 9/28/22 03:54, Simon Glass wrote:
>>> Hi Simon,
>>> Hi Michal,
>>>
>>> On 25.09.22 16:15, Simon Glass wrote:
>>>> Hi Stefan,
>>>>
>>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> Currently this timer driver provides timer_get_boot_us() to support
>>>>> the
>>>>> BOOTSTAGE functionality. This patch adds the timer_early functions so
>>>>> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
>>>>> is enabled.
>>>>>
>>>>> timer_get_boot_us() will get removed in a follow-up patch, once the
>>>>> BOOTSTAGE interface is migrated to timer_get_us().
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>> ---
>>>>> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
>>>>> 1 file changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
>>>>> index 2eff45060ad6..e26c7923a140 100644
>>>>> --- a/drivers/timer/cadence-ttc.c
>>>>> +++ b/drivers/timer/cadence-ttc.c
>>>>> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
>>>>> }
>>>>> #endif
>>>>>
>>>>> +unsigned long notrace timer_early_get_rate(void)
>>>>> +{
>>>>> + return 1;
>>>>> +}
>>>>> +
>>>>> +u64 notrace timer_early_get_count(void)
>>>>> +{
>>>>> + u64 ticks = 0;
>>>>> + u32 rate = 1;
>>>>> + u64 us;
>>>>> + int ret;
>>>>> +
>>>>> + ret = dm_timer_init();
>>>>
>>>> I don't think you can call this if you want to support bootstage,
>>>> since driver model may not be inited.
>>>
>>> Yes, thanks for noticing. Still, this code is copied from the original
>>> timer_get_boot_us() function in this driver. Which also has problems
>>> with early timer access AFAICT.
>>
>> Yes, good point.
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> I looked at it and kind of interesting code. For getting this working
> with this whole series if ttc gets u-boot,dm-pre-reloc everthing is
> working fine.
Yes, makes sense that this is needed.
> diff --git a/arch/arm/dts/zynqmp-r5.dts b/arch/arm/dts/zynqmp-r5.dts
> index a72172ef2ea4..8059931f2162 100644
> --- a/arch/arm/dts/zynqmp-r5.dts
> +++ b/arch/arm/dts/zynqmp-r5.dts
> @@ -56,6 +56,7 @@
>
> ttc0: timer@ff110000 {
> compatible = "cdns,ttc";
> + u-boot,dm-pre-reloc;
> status = "okay";
> reg = <0xff110000 0x1000>;
> timer-width = <32>;
>
> What I see based on behavior. Every bootstage call is asking for
> timer_early_get_count() and because dm_timer_init() is failing 0 is
> returned.
>
> Inside initf_dm() dm_init_and_scan() is called and initialized timer
> uclass also with timer instance. With u-boot,dm-pre-reloc also cadence
> timer driver.
> On the next dm_timer_init() gd->timer is initialized and value is
> returned and initf_dm() passes.
This "early" implementation of the counter is broken, as you have
noticed above. Resulting in timer functions only returning valid
values after dm_timer_init(). This timer driver needs re-written
early_timer functions, that will return proper timer values even in
the early boot phase. Please take a look at e.g. rockchip_timer.c,
perhaps something similar works for the cadence timer driver as well?
> Here is the log and output from bootstage.
>
> U-Boot 2022.10-rc5-00130-g1be5bed207c9 (Sep 30 2022 - 14:00:08 +0200)
>
> Model: Xilinx ZynqMP R5
> DRAM: 512 MiB
> Core: 7 devices, 7 uclasses, devicetree: embed
> MMC:
> Loading Environment from nowhere... OK
> In: serial@ff010000
> Out: serial@ff010000
> Err: serial@ff010000
> Net: No ethernet found.
> ZynqMP r5> dm tree
> Class Index Probed Driver Name
> -----------------------------------------------------------
> root 0 [ + ] root_driver root_driver
> clk 0 [ + ] fixed_clock |-- clk100
> simple_bus 0 [ + ] simple_bus |-- amba
> timer 0 [ + ] cadence_ttc | |-- timer@ff110000
> serial 0 [ + ] serial_zynq | `-- serial@ff010000
> bootstd 0 [ ] bootstd_drv `-- bootstd
> bootmeth 0 [ ] bootmeth_distro `-- distro
> ZynqMP r5> bootstage report
> Timer summary in microseconds (8 records):
> Mark Elapsed Stage
> 0 0 reset
> 0 0 board_init_f
> 0 0 dm_f
> 0 0 dm_r
> 16,319 16,319 eth_common_init
> 18,061 1,742 main_loop
> 18,281 220 cli_loop
> 29,249 10,968 board_init_r
>
> Accumulated time:
Take a look at sandbox - here you will get proper early timing values
as well:
=> bootstage report
Timer summary in microseconds (9 records):
Mark Elapsed Stage
0 0 reset
1 1 board_init_f
7,908 7,907 board_init_r
8,306 398 eth_common_init
8,311 5 main_loop
561,229 552,918 cli_loop
Accumulated time:
9 of_live
39 dm_f
39 dm_r
> Would be good if you can also add u-boot,dm-pre-reloc before 9/10 patch
> which enables CONFIG_TIMER_EARLY.
Will do.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER
2022-09-30 13:28 ` Simon Glass
@ 2022-09-30 13:52 ` Stefan Roese
0 siblings, 0 replies; 33+ messages in thread
From: Stefan Roese @ 2022-09-30 13:52 UTC (permalink / raw)
To: Simon Glass; +Cc: U-Boot Mailing List, Tom Rini
Hi Simon,
On 30.09.22 15:28, Simon Glass wrote:
> Hi Stefan,
>
> On Thu, 29 Sept 2022 at 23:36, Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Simon,
>>
>> On 28.09.22 03:54, Simon Glass wrote:
>>> Hi Stefan,
>>>
>>> On Mon, 26 Sept 2022 at 07:52, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 25.09.22 16:15, Simon Glass wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>> The early timer functions provided via CONFIG_TIMER_EARLY don't need
>>>>>> CONFIG_TIMER to be enabled, as they don't make use of the DM timer
>>>>>> and uclass interface. This patch now allow the selection of
>>>>>> CONFIG_TIMER_EARLY w/o CONFIG_TIMER, enabling this early timer
>>>>>> functionality also for non CONFIG_TIMER drivers.
>>>>>>
>>>>>> With this change it's necessary to guard the dm_timer_init() call
>>>>>> in initr_dm_devices() & initf_dm() additionally via CONFIG_TIMER.
>>>>>>
>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>> ---
>>>>>> common/board_f.c | 2 +-
>>>>>> common/board_r.c | 2 +-
>>>>>> drivers/timer/Kconfig | 1 -
>>>>>> 3 files changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> I don't like this as it complicates the logic and also seems to be
>>>>> adding a new feature to legacy code.
>>>>>
>>>>> Instead, let's enable the early timer only for driver model.
>>>>
>>>> Hmmm, not sure how this should work. Do you have this in mind (instead
>>>> of this patch)?
>>>>
>>>> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
>>>> index fd8745ffc2e0..30d6efe98f29 100644
>>>> --- a/drivers/timer/Kconfig
>>>> +++ b/drivers/timer/Kconfig
>>>> @@ -39,7 +39,7 @@ config VPL_TIMER
>>>>
>>>> config TIMER_EARLY
>>>> bool "Allow timer to be used early in U-Boot"
>>>> - depends on TIMER
>>>> + depends on DM
>>>> # initr_bootstage() requires a timer and is called before
>>>> initr_dm()
>>>> # so only the early timer is available
>>>> default y if X86 && BOOTSTAGE
>>>>
>>>> This results in some compilation errors, like this:
>>>>
>>>> $ make stm32mp15_basic_defconfig
>>>> $ make -sj
>>>> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
>>>> common/board_f.o: in function `initf_dm':
>>>> /home/stefan/git/u-boot/u-boot-marvell/common/board_f.c:791: undefined
>>>> reference to `dm_timer_init'
>>>> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-ld.bfd:
>>>> common/board_r.o: in function `initr_dm_devices':
>>>> /home/stefan/git/u-boot/u-boot-marvell/common/board_r.c:258: undefined
>>>> reference to `dm_timer_init'
>>>> make: *** [Makefile:1790: u-boot] Error 1
>>>>
>>>> I might be missing something. Or it's not that easy and we still need
>>>> my original implementation.
>>>
>>> Well, given that TIMER depends on DM, I don't think you need to change that.
>>>
>>> Also, TIMER_EARLY depends on TIMER (as it should), you should just be
>>> able to check for IS_ENABLED(CONFIG_TIMER)
>>>
>>> We can disable the early timer stuff for things that don't use CONFIG_TIMER
>>
>> The result of this would be, that BOOTSTAGE is not available for
>> platforms that don't have TIMER_EARLY (& TIMER) enabled, like
>> stm32mp15_basic and some others. Actually I think this is a good
>> move, as it "encourages" people to move to enable CONFIG_TIMER.
>>
>> So should I move forward this way? Depending BOOTSTAGE on TIMER?
>
> Yes I think soas it is simplified things and we want people to migrate anyway.
Ok, I'll move forward this way then.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-09-30 13:45 ` Stefan Roese
@ 2022-10-04 11:49 ` Michal Simek
2022-10-04 14:54 ` Stefan Roese
2022-10-04 16:29 ` Simon Glass
0 siblings, 2 replies; 33+ messages in thread
From: Michal Simek @ 2022-10-04 11:49 UTC (permalink / raw)
To: Stefan Roese, Simon Glass; +Cc: Michal Simek, U-Boot Mailing List, Tom Rini
Hi Stefan,
On 9/30/22 15:45, Stefan Roese wrote:
> Hi Michal,
>
> On 30.09.22 14:02, Michal Simek wrote:
>> Hi Simon and Stefan,
>>
>> On 9/28/22 03:54, Simon Glass wrote:
>>>> Hi Simon,
>>>> Hi Michal,
>>>>
>>>> On 25.09.22 16:15, Simon Glass wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>> Currently this timer driver provides timer_get_boot_us() to support the
>>>>>> BOOTSTAGE functionality. This patch adds the timer_early functions so
>>>>>> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
>>>>>> is enabled.
>>>>>>
>>>>>> timer_get_boot_us() will get removed in a follow-up patch, once the
>>>>>> BOOTSTAGE interface is migrated to timer_get_us().
>>>>>>
>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>>> ---
>>>>>> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
>>>>>> 1 file changed, 25 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
>>>>>> index 2eff45060ad6..e26c7923a140 100644
>>>>>> --- a/drivers/timer/cadence-ttc.c
>>>>>> +++ b/drivers/timer/cadence-ttc.c
>>>>>> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
>>>>>> }
>>>>>> #endif
>>>>>>
>>>>>> +unsigned long notrace timer_early_get_rate(void)
>>>>>> +{
>>>>>> + return 1;
>>>>>> +}
>>>>>> +
>>>>>> +u64 notrace timer_early_get_count(void)
>>>>>> +{
>>>>>> + u64 ticks = 0;
>>>>>> + u32 rate = 1;
>>>>>> + u64 us;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = dm_timer_init();
>>>>>
>>>>> I don't think you can call this if you want to support bootstage,
>>>>> since driver model may not be inited.
>>>>
>>>> Yes, thanks for noticing. Still, this code is copied from the original
>>>> timer_get_boot_us() function in this driver. Which also has problems
>>>> with early timer access AFAICT.
>>>
>>> Yes, good point.
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> I looked at it and kind of interesting code. For getting this working with
>> this whole series if ttc gets u-boot,dm-pre-reloc everthing is working fine.
>
> Yes, makes sense that this is needed.
>
>> diff --git a/arch/arm/dts/zynqmp-r5.dts b/arch/arm/dts/zynqmp-r5.dts
>> index a72172ef2ea4..8059931f2162 100644
>> --- a/arch/arm/dts/zynqmp-r5.dts
>> +++ b/arch/arm/dts/zynqmp-r5.dts
>> @@ -56,6 +56,7 @@
>>
>> ttc0: timer@ff110000 {
>> compatible = "cdns,ttc";
>> + u-boot,dm-pre-reloc;
>> status = "okay";
>> reg = <0xff110000 0x1000>;
>> timer-width = <32>;
>>
>> What I see based on behavior. Every bootstage call is asking for
>> timer_early_get_count() and because dm_timer_init() is failing 0 is returned.
>>
>> Inside initf_dm() dm_init_and_scan() is called and initialized timer uclass
>> also with timer instance. With u-boot,dm-pre-reloc also cadence timer driver.
>> On the next dm_timer_init() gd->timer is initialized and value is returned and
>> initf_dm() passes.
>
> This "early" implementation of the counter is broken, as you have
> noticed above. Resulting in timer functions only returning valid
> values after dm_timer_init(). This timer driver needs re-written
> early_timer functions, that will return proper timer values even in
> the early boot phase. Please take a look at e.g. rockchip_timer.c,
> perhaps something similar works for the cadence timer driver as well?
I looked at it and there is a code when OF_REAL is enabled. If not then 0 is
returned. That's why I can't see any difference between rockchip and cadence
when OF_REAL is not enabled.
And not sure how this is working in rockchip case. Who is starting timer in
their case? I see start when probe happens but after it dm_timer_init() should
return 0 and value can be obtained.
>
>> Here is the log and output from bootstage.
>>
>> U-Boot 2022.10-rc5-00130-g1be5bed207c9 (Sep 30 2022 - 14:00:08 +0200)
>>
>> Model: Xilinx ZynqMP R5
>> DRAM: 512 MiB
>> Core: 7 devices, 7 uclasses, devicetree: embed
>> MMC:
>> Loading Environment from nowhere... OK
>> In: serial@ff010000
>> Out: serial@ff010000
>> Err: serial@ff010000
>> Net: No ethernet found.
>> ZynqMP r5> dm tree
>> Class Index Probed Driver Name
>> -----------------------------------------------------------
>> root 0 [ + ] root_driver root_driver
>> clk 0 [ + ] fixed_clock |-- clk100
>> simple_bus 0 [ + ] simple_bus |-- amba
>> timer 0 [ + ] cadence_ttc | |-- timer@ff110000
>> serial 0 [ + ] serial_zynq | `-- serial@ff010000
>> bootstd 0 [ ] bootstd_drv `-- bootstd
>> bootmeth 0 [ ] bootmeth_distro `-- distro
>> ZynqMP r5> bootstage report
>> Timer summary in microseconds (8 records):
>> Mark Elapsed Stage
>> 0 0 reset
>> 0 0 board_init_f
>> 0 0 dm_f
>> 0 0 dm_r
>> 16,319 16,319 eth_common_init
>> 18,061 1,742 main_loop
>> 18,281 220 cli_loop
>> 29,249 10,968 board_init_r
>>
>> Accumulated time:
>
> Take a look at sandbox - here you will get proper early timing values
> as well:
I expect early functions are called before DM is ready that's why to get it work
that early we would have to start timer on the first call of early function.
It is definitely doable but is it worth to do it?
Is there any other use case where early timer counts are needed other then
bootstage?
Thanks,
Michal
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-10-04 11:49 ` Michal Simek
@ 2022-10-04 14:54 ` Stefan Roese
2022-10-04 16:29 ` Simon Glass
1 sibling, 0 replies; 33+ messages in thread
From: Stefan Roese @ 2022-10-04 14:54 UTC (permalink / raw)
To: Michal Simek, Simon Glass
Cc: Michal Simek, U-Boot Mailing List, Tom Rini, Kever Yang, Philipp Tomsich
Hi Michal,
(adding Kever & Philipp to Cc)
On 04.10.22 13:49, Michal Simek wrote:
> Hi Stefan,
>
> On 9/30/22 15:45, Stefan Roese wrote:
>> Hi Michal,
>>
>> On 30.09.22 14:02, Michal Simek wrote:
>>> Hi Simon and Stefan,
>>>
>>> On 9/28/22 03:54, Simon Glass wrote:
>>>>> Hi Simon,
>>>>> Hi Michal,
>>>>>
>>>>> On 25.09.22 16:15, Simon Glass wrote:
>>>>>> Hi Stefan,
>>>>>>
>>>>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>>>>>>
>>>>>>> Currently this timer driver provides timer_get_boot_us() to
>>>>>>> support the
>>>>>>> BOOTSTAGE functionality. This patch adds the timer_early
>>>>>>> functions so
>>>>>>> that the "normal" timer functions can be used, when
>>>>>>> CONFIG_TIMER_EARLY
>>>>>>> is enabled.
>>>>>>>
>>>>>>> timer_get_boot_us() will get removed in a follow-up patch, once the
>>>>>>> BOOTSTAGE interface is migrated to timer_get_us().
>>>>>>>
>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>>>> ---
>>>>>>> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
>>>>>>> 1 file changed, 25 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/timer/cadence-ttc.c
>>>>>>> b/drivers/timer/cadence-ttc.c
>>>>>>> index 2eff45060ad6..e26c7923a140 100644
>>>>>>> --- a/drivers/timer/cadence-ttc.c
>>>>>>> +++ b/drivers/timer/cadence-ttc.c
>>>>>>> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
>>>>>>> }
>>>>>>> #endif
>>>>>>>
>>>>>>> +unsigned long notrace timer_early_get_rate(void)
>>>>>>> +{
>>>>>>> + return 1;
>>>>>>> +}
>>>>>>> +
>>>>>>> +u64 notrace timer_early_get_count(void)
>>>>>>> +{
>>>>>>> + u64 ticks = 0;
>>>>>>> + u32 rate = 1;
>>>>>>> + u64 us;
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + ret = dm_timer_init();
>>>>>>
>>>>>> I don't think you can call this if you want to support bootstage,
>>>>>> since driver model may not be inited.
>>>>>
>>>>> Yes, thanks for noticing. Still, this code is copied from the original
>>>>> timer_get_boot_us() function in this driver. Which also has problems
>>>>> with early timer access AFAICT.
>>>>
>>>> Yes, good point.
>>>>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> I looked at it and kind of interesting code. For getting this working
>>> with this whole series if ttc gets u-boot,dm-pre-reloc everthing is
>>> working fine.
>>
>> Yes, makes sense that this is needed.
>>
>>> diff --git a/arch/arm/dts/zynqmp-r5.dts b/arch/arm/dts/zynqmp-r5.dts
>>> index a72172ef2ea4..8059931f2162 100644
>>> --- a/arch/arm/dts/zynqmp-r5.dts
>>> +++ b/arch/arm/dts/zynqmp-r5.dts
>>> @@ -56,6 +56,7 @@
>>>
>>> ttc0: timer@ff110000 {
>>> compatible = "cdns,ttc";
>>> + u-boot,dm-pre-reloc;
>>> status = "okay";
>>> reg = <0xff110000 0x1000>;
>>> timer-width = <32>;
>>>
>>> What I see based on behavior. Every bootstage call is asking for
>>> timer_early_get_count() and because dm_timer_init() is failing 0 is
>>> returned.
>>>
>>> Inside initf_dm() dm_init_and_scan() is called and initialized timer
>>> uclass also with timer instance. With u-boot,dm-pre-reloc also
>>> cadence timer driver.
>>> On the next dm_timer_init() gd->timer is initialized and value is
>>> returned and initf_dm() passes.
>>
>> This "early" implementation of the counter is broken, as you have
>> noticed above. Resulting in timer functions only returning valid
>> values after dm_timer_init(). This timer driver needs re-written
>> early_timer functions, that will return proper timer values even in
>> the early boot phase. Please take a look at e.g. rockchip_timer.c,
>> perhaps something similar works for the cadence timer driver as well?
>
> I looked at it and there is a code when OF_REAL is enabled. If not then
> 0 is returned. That's why I can't see any difference between rockchip
> and cadence when OF_REAL is not enabled.
>
> And not sure how this is working in rockchip case. Who is starting timer
> in their case?
I have no idea. My best guess is that its already enabled when U-Boot
starts.
> I see start when probe happens but after it
> dm_timer_init() should return 0 and value can be obtained.
Kever / Philipp, could you perhaps shed some light in the current
rockchip early timer implementation timer_get_boot_us(). How does it
work?
>>
>>> Here is the log and output from bootstage.
>>>
>>> U-Boot 2022.10-rc5-00130-g1be5bed207c9 (Sep 30 2022 - 14:00:08 +0200)
>>>
>>> Model: Xilinx ZynqMP R5
>>> DRAM: 512 MiB
>>> Core: 7 devices, 7 uclasses, devicetree: embed
>>> MMC:
>>> Loading Environment from nowhere... OK
>>> In: serial@ff010000
>>> Out: serial@ff010000
>>> Err: serial@ff010000
>>> Net: No ethernet found.
>>> ZynqMP r5> dm tree
>>> Class Index Probed Driver Name
>>> -----------------------------------------------------------
>>> root 0 [ + ] root_driver root_driver
>>> clk 0 [ + ] fixed_clock |-- clk100
>>> simple_bus 0 [ + ] simple_bus |-- amba
>>> timer 0 [ + ] cadence_ttc | |-- timer@ff110000
>>> serial 0 [ + ] serial_zynq | `-- serial@ff010000
>>> bootstd 0 [ ] bootstd_drv `-- bootstd
>>> bootmeth 0 [ ] bootmeth_distro `-- distro
>>> ZynqMP r5> bootstage report
>>> Timer summary in microseconds (8 records):
>>> Mark Elapsed Stage
>>> 0 0 reset
>>> 0 0 board_init_f
>>> 0 0 dm_f
>>> 0 0 dm_r
>>> 16,319 16,319 eth_common_init
>>> 18,061 1,742 main_loop
>>> 18,281 220 cli_loop
>>> 29,249 10,968 board_init_r
>>>
>>> Accumulated time:
>>
>> Take a look at sandbox - here you will get proper early timing values
>> as well:
>
> I expect early functions are called before DM is ready that's why to get
> it work that early we would have to start timer on the first call of
> early function.
> It is definitely doable but is it worth to do it?
Depends on your needs of course.
> Is there any other use case where early timer counts are needed other
> then bootstage?
Definitely. I can think of early code that needs timer functionality,
like udelay() etc. I'm pretty sure there are many occurrences for such
timing usages before DM is initialized on some platforms.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-10-04 11:49 ` Michal Simek
2022-10-04 14:54 ` Stefan Roese
@ 2022-10-04 16:29 ` Simon Glass
2022-10-05 6:59 ` Michal Simek
1 sibling, 1 reply; 33+ messages in thread
From: Simon Glass @ 2022-10-04 16:29 UTC (permalink / raw)
To: Michal Simek; +Cc: Stefan Roese, Michal Simek, U-Boot Mailing List, Tom Rini
Hi,
On Tue, 4 Oct 2022 at 05:50, Michal Simek <michal.simek@amd.com> wrote:
>
> Hi Stefan,
>
> On 9/30/22 15:45, Stefan Roese wrote:
> > Hi Michal,
> >
> > On 30.09.22 14:02, Michal Simek wrote:
> >> Hi Simon and Stefan,
> >>
> >> On 9/28/22 03:54, Simon Glass wrote:
> >>>> Hi Simon,
> >>>> Hi Michal,
> >>>>
> >>>> On 25.09.22 16:15, Simon Glass wrote:
> >>>>> Hi Stefan,
> >>>>>
> >>>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
> >>>>>>
> >>>>>> Currently this timer driver provides timer_get_boot_us() to support the
> >>>>>> BOOTSTAGE functionality. This patch adds the timer_early functions so
> >>>>>> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
> >>>>>> is enabled.
> >>>>>>
> >>>>>> timer_get_boot_us() will get removed in a follow-up patch, once the
> >>>>>> BOOTSTAGE interface is migrated to timer_get_us().
> >>>>>>
> >>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
> >>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
> >>>>>> ---
> >>>>>> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
> >>>>>> 1 file changed, 25 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
> >>>>>> index 2eff45060ad6..e26c7923a140 100644
> >>>>>> --- a/drivers/timer/cadence-ttc.c
> >>>>>> +++ b/drivers/timer/cadence-ttc.c
> >>>>>> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
> >>>>>> }
> >>>>>> #endif
> >>>>>>
> >>>>>> +unsigned long notrace timer_early_get_rate(void)
> >>>>>> +{
> >>>>>> + return 1;
> >>>>>> +}
> >>>>>> +
> >>>>>> +u64 notrace timer_early_get_count(void)
> >>>>>> +{
> >>>>>> + u64 ticks = 0;
> >>>>>> + u32 rate = 1;
> >>>>>> + u64 us;
> >>>>>> + int ret;
> >>>>>> +
> >>>>>> + ret = dm_timer_init();
> >>>>>
> >>>>> I don't think you can call this if you want to support bootstage,
> >>>>> since driver model may not be inited.
> >>>>
> >>>> Yes, thanks for noticing. Still, this code is copied from the original
> >>>> timer_get_boot_us() function in this driver. Which also has problems
> >>>> with early timer access AFAICT.
> >>>
> >>> Yes, good point.
> >>>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>
> >> I looked at it and kind of interesting code. For getting this working with
> >> this whole series if ttc gets u-boot,dm-pre-reloc everthing is working fine.
> >
> > Yes, makes sense that this is needed.
> >
> >> diff --git a/arch/arm/dts/zynqmp-r5.dts b/arch/arm/dts/zynqmp-r5.dts
> >> index a72172ef2ea4..8059931f2162 100644
> >> --- a/arch/arm/dts/zynqmp-r5.dts
> >> +++ b/arch/arm/dts/zynqmp-r5.dts
> >> @@ -56,6 +56,7 @@
> >>
> >> ttc0: timer@ff110000 {
> >> compatible = "cdns,ttc";
> >> + u-boot,dm-pre-reloc;
> >> status = "okay";
> >> reg = <0xff110000 0x1000>;
> >> timer-width = <32>;
> >>
> >> What I see based on behavior. Every bootstage call is asking for
> >> timer_early_get_count() and because dm_timer_init() is failing 0 is returned.
> >>
> >> Inside initf_dm() dm_init_and_scan() is called and initialized timer uclass
> >> also with timer instance. With u-boot,dm-pre-reloc also cadence timer driver.
> >> On the next dm_timer_init() gd->timer is initialized and value is returned and
> >> initf_dm() passes.
> >
> > This "early" implementation of the counter is broken, as you have
> > noticed above. Resulting in timer functions only returning valid
> > values after dm_timer_init(). This timer driver needs re-written
> > early_timer functions, that will return proper timer values even in
> > the early boot phase. Please take a look at e.g. rockchip_timer.c,
> > perhaps something similar works for the cadence timer driver as well?
>
> I looked at it and there is a code when OF_REAL is enabled. If not then 0 is
> returned. That's why I can't see any difference between rockchip and cadence
> when OF_REAL is not enabled.
>
> And not sure how this is working in rockchip case. Who is starting timer in
> their case? I see start when probe happens but after it dm_timer_init() should
> return 0 and value can be obtained.
>
> >
> >> Here is the log and output from bootstage.
> >>
> >> U-Boot 2022.10-rc5-00130-g1be5bed207c9 (Sep 30 2022 - 14:00:08 +0200)
> >>
> >> Model: Xilinx ZynqMP R5
> >> DRAM: 512 MiB
> >> Core: 7 devices, 7 uclasses, devicetree: embed
> >> MMC:
> >> Loading Environment from nowhere... OK
> >> In: serial@ff010000
> >> Out: serial@ff010000
> >> Err: serial@ff010000
> >> Net: No ethernet found.
> >> ZynqMP r5> dm tree
> >> Class Index Probed Driver Name
> >> -----------------------------------------------------------
> >> root 0 [ + ] root_driver root_driver
> >> clk 0 [ + ] fixed_clock |-- clk100
> >> simple_bus 0 [ + ] simple_bus |-- amba
> >> timer 0 [ + ] cadence_ttc | |-- timer@ff110000
> >> serial 0 [ + ] serial_zynq | `-- serial@ff010000
> >> bootstd 0 [ ] bootstd_drv `-- bootstd
> >> bootmeth 0 [ ] bootmeth_distro `-- distro
> >> ZynqMP r5> bootstage report
> >> Timer summary in microseconds (8 records):
> >> Mark Elapsed Stage
> >> 0 0 reset
> >> 0 0 board_init_f
> >> 0 0 dm_f
> >> 0 0 dm_r
> >> 16,319 16,319 eth_common_init
> >> 18,061 1,742 main_loop
> >> 18,281 220 cli_loop
> >> 29,249 10,968 board_init_r
> >>
> >> Accumulated time:
> >
> > Take a look at sandbox - here you will get proper early timing values
> > as well:
>
> I expect early functions are called before DM is ready that's why to get it work
> that early we would have to start timer on the first call of early function.
> It is definitely doable but is it worth to do it?
>
> Is there any other use case where early timer counts are needed other then
> bootstage?
Here is how it should work, I think:
static void notrace ensure_timer_setup(void)
{
// set up the timer if not already done
// avoid using any DM functions
}
static u64 xx_get_count(void)
{
// read and return the timer counter
}
u64 notrace timer_early_get_count(void)
{
ensure_timer_setup();
return xxx_get_count();
}
static u64 xx_timer_get_count(struct udevice *dev)
{
return xxx_get_count();
}
int xxx_timer_probe(struct udevice *dev)
{
ensure_timer_setup()\;
return 0;
}
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 04/10] timer: cadence-ttc: Add timer_early functions
2022-10-04 16:29 ` Simon Glass
@ 2022-10-05 6:59 ` Michal Simek
0 siblings, 0 replies; 33+ messages in thread
From: Michal Simek @ 2022-10-05 6:59 UTC (permalink / raw)
To: Simon Glass; +Cc: Stefan Roese, Michal Simek, U-Boot Mailing List, Tom Rini
Hi,
On 10/4/22 18:29, Simon Glass wrote:
> Hi,
>
> On Tue, 4 Oct 2022 at 05:50, Michal Simek <michal.simek@amd.com> wrote:
>>
>> Hi Stefan,
>>
>> On 9/30/22 15:45, Stefan Roese wrote:
>>> Hi Michal,
>>>
>>> On 30.09.22 14:02, Michal Simek wrote:
>>>> Hi Simon and Stefan,
>>>>
>>>> On 9/28/22 03:54, Simon Glass wrote:
>>>>>> Hi Simon,
>>>>>> Hi Michal,
>>>>>>
>>>>>> On 25.09.22 16:15, Simon Glass wrote:
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> On Wed, 21 Sept 2022 at 08:06, Stefan Roese <sr@denx.de> wrote:
>>>>>>>>
>>>>>>>> Currently this timer driver provides timer_get_boot_us() to support the
>>>>>>>> BOOTSTAGE functionality. This patch adds the timer_early functions so
>>>>>>>> that the "normal" timer functions can be used, when CONFIG_TIMER_EARLY
>>>>>>>> is enabled.
>>>>>>>>
>>>>>>>> timer_get_boot_us() will get removed in a follow-up patch, once the
>>>>>>>> BOOTSTAGE interface is migrated to timer_get_us().
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>>>>> ---
>>>>>>>> drivers/timer/cadence-ttc.c | 25 +++++++++++++++++++++++++
>>>>>>>> 1 file changed, 25 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
>>>>>>>> index 2eff45060ad6..e26c7923a140 100644
>>>>>>>> --- a/drivers/timer/cadence-ttc.c
>>>>>>>> +++ b/drivers/timer/cadence-ttc.c
>>>>>>>> @@ -58,6 +58,31 @@ ulong timer_get_boot_us(void)
>>>>>>>> }
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> +unsigned long notrace timer_early_get_rate(void)
>>>>>>>> +{
>>>>>>>> + return 1;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +u64 notrace timer_early_get_count(void)
>>>>>>>> +{
>>>>>>>> + u64 ticks = 0;
>>>>>>>> + u32 rate = 1;
>>>>>>>> + u64 us;
>>>>>>>> + int ret;
>>>>>>>> +
>>>>>>>> + ret = dm_timer_init();
>>>>>>>
>>>>>>> I don't think you can call this if you want to support bootstage,
>>>>>>> since driver model may not be inited.
>>>>>>
>>>>>> Yes, thanks for noticing. Still, this code is copied from the original
>>>>>> timer_get_boot_us() function in this driver. Which also has problems
>>>>>> with early timer access AFAICT.
>>>>>
>>>>> Yes, good point.
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> I looked at it and kind of interesting code. For getting this working with
>>>> this whole series if ttc gets u-boot,dm-pre-reloc everthing is working fine.
>>>
>>> Yes, makes sense that this is needed.
>>>
>>>> diff --git a/arch/arm/dts/zynqmp-r5.dts b/arch/arm/dts/zynqmp-r5.dts
>>>> index a72172ef2ea4..8059931f2162 100644
>>>> --- a/arch/arm/dts/zynqmp-r5.dts
>>>> +++ b/arch/arm/dts/zynqmp-r5.dts
>>>> @@ -56,6 +56,7 @@
>>>>
>>>> ttc0: timer@ff110000 {
>>>> compatible = "cdns,ttc";
>>>> + u-boot,dm-pre-reloc;
>>>> status = "okay";
>>>> reg = <0xff110000 0x1000>;
>>>> timer-width = <32>;
>>>>
>>>> What I see based on behavior. Every bootstage call is asking for
>>>> timer_early_get_count() and because dm_timer_init() is failing 0 is returned.
>>>>
>>>> Inside initf_dm() dm_init_and_scan() is called and initialized timer uclass
>>>> also with timer instance. With u-boot,dm-pre-reloc also cadence timer driver.
>>>> On the next dm_timer_init() gd->timer is initialized and value is returned and
>>>> initf_dm() passes.
>>>
>>> This "early" implementation of the counter is broken, as you have
>>> noticed above. Resulting in timer functions only returning valid
>>> values after dm_timer_init(). This timer driver needs re-written
>>> early_timer functions, that will return proper timer values even in
>>> the early boot phase. Please take a look at e.g. rockchip_timer.c,
>>> perhaps something similar works for the cadence timer driver as well?
>>
>> I looked at it and there is a code when OF_REAL is enabled. If not then 0 is
>> returned. That's why I can't see any difference between rockchip and cadence
>> when OF_REAL is not enabled.
>>
>> And not sure how this is working in rockchip case. Who is starting timer in
>> their case? I see start when probe happens but after it dm_timer_init() should
>> return 0 and value can be obtained.
>>
>>>
>>>> Here is the log and output from bootstage.
>>>>
>>>> U-Boot 2022.10-rc5-00130-g1be5bed207c9 (Sep 30 2022 - 14:00:08 +0200)
>>>>
>>>> Model: Xilinx ZynqMP R5
>>>> DRAM: 512 MiB
>>>> Core: 7 devices, 7 uclasses, devicetree: embed
>>>> MMC:
>>>> Loading Environment from nowhere... OK
>>>> In: serial@ff010000
>>>> Out: serial@ff010000
>>>> Err: serial@ff010000
>>>> Net: No ethernet found.
>>>> ZynqMP r5> dm tree
>>>> Class Index Probed Driver Name
>>>> -----------------------------------------------------------
>>>> root 0 [ + ] root_driver root_driver
>>>> clk 0 [ + ] fixed_clock |-- clk100
>>>> simple_bus 0 [ + ] simple_bus |-- amba
>>>> timer 0 [ + ] cadence_ttc | |-- timer@ff110000
>>>> serial 0 [ + ] serial_zynq | `-- serial@ff010000
>>>> bootstd 0 [ ] bootstd_drv `-- bootstd
>>>> bootmeth 0 [ ] bootmeth_distro `-- distro
>>>> ZynqMP r5> bootstage report
>>>> Timer summary in microseconds (8 records):
>>>> Mark Elapsed Stage
>>>> 0 0 reset
>>>> 0 0 board_init_f
>>>> 0 0 dm_f
>>>> 0 0 dm_r
>>>> 16,319 16,319 eth_common_init
>>>> 18,061 1,742 main_loop
>>>> 18,281 220 cli_loop
>>>> 29,249 10,968 board_init_r
>>>>
>>>> Accumulated time:
>>>
>>> Take a look at sandbox - here you will get proper early timing values
>>> as well:
>>
>> I expect early functions are called before DM is ready that's why to get it work
>> that early we would have to start timer on the first call of early function.
>> It is definitely doable but is it worth to do it?
>>
>> Is there any other use case where early timer counts are needed other then
>> bootstage?
>
> Here is how it should work, I think:
>
> static void notrace ensure_timer_setup(void)
> {
> // set up the timer if not already done
> // avoid using any DM functions
> }
>
> static u64 xx_get_count(void)
> {
> // read and return the timer counter
> }
>
> u64 notrace timer_early_get_count(void)
> {
> ensure_timer_setup();
> return xxx_get_count();
> }
>
> static u64 xx_timer_get_count(struct udevice *dev)
> {
> return xxx_get_count();
> }
>
> int xxx_timer_probe(struct udevice *dev)
> {
> ensure_timer_setup()\;
>
> return 0;
> }
ok this is clear and understandable.
As far as I see the only information we need is to have address where timer is.
Rockchip and uclass drivers are using tick-timer when multiple timers are
described in DT.
That's why for systems with only one timer this property is not needed.
Also this option is enabled only when OF_REAL is enabled.
When it is clear which link to use for getting address from DT or via DM we can
change drivers pretty easily.
Thanks,
Michal
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2022-10-05 6:59 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-21 14:06 [PATCH 00/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
2022-09-21 14:06 ` [PATCH 01/10] arm: arch_timer: Add timer_early functions Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-21 14:06 ` [PATCH 02/10] arm: imx: syscounter: " Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-21 14:06 ` [PATCH 03/10] arm: armv8: generic_timer: " Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-21 14:06 ` [PATCH 04/10] timer: cadence-ttc: " Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-26 14:11 ` Stefan Roese
2022-09-28 1:54 ` Simon Glass
2022-09-30 12:02 ` Michal Simek
2022-09-30 13:45 ` Stefan Roese
2022-10-04 11:49 ` Michal Simek
2022-10-04 14:54 ` Stefan Roese
2022-10-04 16:29 ` Simon Glass
2022-10-05 6:59 ` Michal Simek
2022-09-21 14:06 ` [PATCH 05/10] timer: omap-timer: " Stefan Roese
2022-09-21 14:06 ` [PATCH 06/10] timer: rockchip_timer: " Stefan Roese
2022-09-24 7:57 ` Kever Yang
2022-09-21 14:06 ` [PATCH 07/10] board_f/r: Allow selection of CONFIG_TIMER_EARLY w/o CONFIG_TIMER Stefan Roese
2022-09-25 14:15 ` Simon Glass
2022-09-26 13:52 ` Stefan Roese
2022-09-28 1:54 ` Simon Glass
2022-09-30 5:36 ` Stefan Roese
2022-09-30 13:28 ` Simon Glass
2022-09-30 13:52 ` Stefan Roese
2022-09-21 14:06 ` [PATCH 08/10] board_f/r: Don't call timer_init() when TIMER is enabled Stefan Roese
2022-09-21 14:06 ` [PATCH 09/10] bootstage: Migrate from timer_get_boot_us() to timer_get_us() Stefan Roese
2022-09-28 10:20 ` Simon Glass
2022-09-30 11:30 ` Michal Simek
2022-09-21 14:06 ` [PATCH 10/10] bootstage/timer: Treewide remove timer_get_boot_us() Stefan Roese
2022-09-25 14:15 ` Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).