> -----Original Message----- > From: Paweł Anikiel <p...@semihalf.com> > Sent: Tuesday, 21 June, 2022 12:00 AM > To: Chee, Tien Fong <tien.fong.c...@intel.com> > Cc: Vasut, Marek <ma...@denx.de>; simon.k.r.goldschm...@gmail.com; > michal.si...@xilinx.com; u-boot@lists.denx.de; s...@chromium.org; > feste...@denx.de; ja...@amarulasolutions.com; > andre.przyw...@arm.com; Armstrong, Neil <narmstr...@baylibre.com>; > pbrobin...@gmail.com; thar...@gateworks.com; paul....@linaro.org; > christianshew...@gmail.com; adrian.fiergol...@fastree3d.com; > marek.be...@nic.cz; Denk, Wolfgang <w...@denx.de>; Lim, Elly Siew Chin > <elly.siew.chin....@intel.com>; upstr...@semihalf.com; > ams...@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.c...@intel.com> > wrote: > > > > > > > > > -----Original Message----- > > > From: Paweł Anikiel <p...@semihalf.com> > > > Sent: Monday, 20 June, 2022 8:14 PM > > > To: Chee, Tien Fong <tien.fong.c...@intel.com> > > > Cc: Vasut, Marek <ma...@denx.de>; simon.k.r.goldschm...@gmail.com; > > > michal.si...@xilinx.com; u-boot@lists.denx.de; s...@chromium.org; > > > feste...@denx.de; ja...@amarulasolutions.com; > > > andre.przyw...@arm.com; Armstrong, Neil <narmstr...@baylibre.com>; > > > pbrobin...@gmail.com; thar...@gateworks.com; paul....@linaro.org; > > > christianshew...@gmail.com; adrian.fiergol...@fastree3d.com; > > > marek.be...@nic.cz; Denk, Wolfgang <w...@denx.de>; Lim, Elly Siew > Chin > > > <elly.siew.chin....@intel.com>; upstr...@semihalf.com; > > > ams...@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.c...@intel.com> wrote: > > > > > > > > Hi, > > > > > > > > > -----Original Message----- > > > > > From: Paweł Anikiel <p...@semihalf.com> > > > > > Sent: Friday, 17 June, 2022 6:47 PM > > > > > To: Vasut, Marek <ma...@denx.de>; > > > > > simon.k.r.goldschm...@gmail.com; Chee, Tien Fong > > > > > <tien.fong.c...@intel.com>; michal.si...@xilinx.com > > > > > Cc: u-boot@lists.denx.de; s...@chromium.org; feste...@denx.de; > > > > > ja...@amarulasolutions.com; andre.przyw...@arm.com; Armstrong, > > > Neil > > > > > <narmstr...@baylibre.com>; pbrobin...@gmail.com; > > > > > thar...@gateworks.com; paul....@linaro.org; > > > > > christianshew...@gmail.com; adrian.fiergol...@fastree3d.com; > > > > > marek.be...@nic.cz; Denk, Wolfgang <w...@denx.de>; Lim, Elly Siew > > > Chin > > > > > <elly.siew.chin....@intel.com>; upstr...@semihalf.com; > > > > > ams...@chromium.org; Paweł Anikiel <p...@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 <p...@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.c...@intel.com> Regards Tien Fong
