-----Original Message----- From: Paweł Anikiel pan@semihalf.com Sent: Tuesday, 21 June, 2022 12:00 AM To: Chee, Tien Fong tien.fong.chee@intel.com Cc: Vasut, Marek marex@denx.de; simon.k.r.goldschmidt@gmail.com; michal.simek@xilinx.com; u-boot@lists.denx.de; sjg@chromium.org; festevam@denx.de; jagan@amarulasolutions.com; andre.przywara@arm.com; Armstrong, Neil narmstrong@baylibre.com; pbrobinson@gmail.com; tharvey@gateworks.com; paul.liu@linaro.org; christianshewitt@gmail.com; adrian.fiergolski@fastree3d.com; marek.behun@nic.cz; Denk, Wolfgang wd@denx.de; Lim, Elly Siew Chin elly.siew.chin.lim@intel.com; upstream@semihalf.com; amstan@chromium.org Subject: Re: [PATCH v3 08/11] socfpga: arria10: Replace delays with busy waiting in cm_full_cfg
On Mon, Jun 20, 2022 at 2:29 PM Chee, Tien Fong tien.fong.chee@intel.com wrote:
-----Original Message----- From: Paweł Anikiel pan@semihalf.com Sent: Monday, 20 June, 2022 8:14 PM To: Chee, Tien Fong tien.fong.chee@intel.com Cc: Vasut, Marek marex@denx.de; simon.k.r.goldschmidt@gmail.com; michal.simek@xilinx.com; u-boot@lists.denx.de; sjg@chromium.org; festevam@denx.de; jagan@amarulasolutions.com; andre.przywara@arm.com; Armstrong, Neil narmstrong@baylibre.com; pbrobinson@gmail.com; tharvey@gateworks.com; paul.liu@linaro.org; christianshewitt@gmail.com; adrian.fiergolski@fastree3d.com; marek.behun@nic.cz; Denk, Wolfgang wd@denx.de; Lim, Elly Siew
Chin
elly.siew.chin.lim@intel.com; upstream@semihalf.com; amstan@chromium.org Subject: Re: [PATCH v3 08/11] socfpga: arria10: Replace delays with busy waiting in cm_full_cfg
On Mon, Jun 20, 2022 at 10:40 AM Chee, Tien Fong tien.fong.chee@intel.com wrote:
Hi,
-----Original Message----- From: Paweł Anikiel pan@semihalf.com Sent: Friday, 17 June, 2022 6:47 PM To: Vasut, Marek marex@denx.de; simon.k.r.goldschmidt@gmail.com; Chee, Tien Fong tien.fong.chee@intel.com; michal.simek@xilinx.com Cc: u-boot@lists.denx.de; sjg@chromium.org; festevam@denx.de; jagan@amarulasolutions.com; andre.przywara@arm.com; Armstrong,
Neil
narmstrong@baylibre.com; pbrobinson@gmail.com; tharvey@gateworks.com; paul.liu@linaro.org; christianshewitt@gmail.com; adrian.fiergolski@fastree3d.com; marek.behun@nic.cz; Denk, Wolfgang wd@denx.de; Lim, Elly Siew
Chin
elly.siew.chin.lim@intel.com; upstream@semihalf.com; amstan@chromium.org; Paweł Anikiel pan@semihalf.com Subject: [PATCH v3 08/11] socfpga: arria10: Replace delays with busy waiting in cm_full_cfg
Using udelay while the clocks aren't fully configured causes the timer system to save the wrong clock rate. Use sdelay and wait_on_value instead (the values used in these functions were found
experimentally).
Signed-off-by: Paweł Anikiel pan@semihalf.com
arch/arm/mach-socfpga/clock_manager_arria10.c | 31 +++++++++++++-----
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-socfpga/clock_manager_arria10.c b/arch/arm/mach-socfpga/clock_manager_arria10.c index 58d5d3fd8a..b48a2b47bc 100644 --- a/arch/arm/mach-socfpga/clock_manager_arria10.c +++ b/arch/arm/mach-socfpga/clock_manager_arria10.c
Did you try to call timer_init() after cm_basic_init() in board_init_f? If that's
working, then no change is required to fix this clock issue.
Seems like timer_init() isn't implemented on Arria 10 (it defaults to the return 0 stub). I also tried dm_timer_init(), no luck.
I did some code digging, the clock rate is read by clk_get_rate(), and the timer rate is set by dw_apb_timer_probe() (drivers/timer/dw-apb- timer.c:77), and there doesn't seem to be a good way of updating that value later.
The only other function I could find that sets the timer rate is timer_pre_probe() from drivers/timer/timer-uclass.c, which very much looks like what we need, but it's static and the name suggests it shouldn't be called manually anyway.
Thanks for the details finding.
I found that both Cyclone 5 and S10 (including all AARCH64 devices) having
own timer_init() as solution for this issue.
Cyclone 5 : https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-
socfp
ga/timer.c S10: https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-
socfp
ga/timer_s10.c
Do you think this is good idea having the same for A10 device?
I don't think overriding timer_init() alone is going to help. (Re)initializing the timer after cm_basic_init() won't help the fact that xdelay() divides the clock ticks (which are correct) by gd->timer->uclass_priv_->clock_rate (https://source.denx.de/u-boot/u-boot/-/blob/master/lib/time.c#L81) (which was incorrectly set by a call to udelay() from cm_full_cfg()).
I honestly don't see how Cyclone/Arria 5 solve this problem, as they don't implement a __udelay(), and their cm_basic_init() also uses timer-based delays (https://source.denx.de/u-boot/u-boot/- /blob/master/arch/arm/mach-socfpga/clock_manager_gen5.c#L98, eventually calls udelay(1) in include/wait_bit.h). I don't have any board on which I could test this on, but I suspect they may also save the wrong clock rate value (causing xdelay() to delay for wrong amounts of time).
Ok, got it, seems there is no better solution since some delays are required in clock manager driver initialization.
Stratix 10 looks okay to me, as it implements its own __udelay() and __usec_to_tick() in SPL.
So a solution would be to implement a __udelay() and a __usec_to_tick(). I don't really know how to do that though, S10 uses the built-in armv8 timer for that.
Reviewed-by: Tien Fong Chee tien.fong.chee@intel.com
Regards Tien Fong