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-socfpga/tim... S10: https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-socfpga/tim...
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/clo..., 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).
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.
Regards, Paweł