| Message ID | 20220921140625.999002-5-sr@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | bootstage: Migrate from timer_get_boot_us() to timer_get_us() | expand |
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
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
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
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
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
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
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
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
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
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);
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(+)