[PATCH v3 08/11] socfpga: arria10: Replace delays with busy waiting in cm_full_cfg

[Chee, Tien Fong]

> -----Original Message-----
> From: Paweł Anikiel <pan at semihalf.com>
> Sent: Tuesday, 21 June, 2022 12:00 AM
> To: Chee, Tien Fong <tien.fong.chee at intel.com>
> Cc: Vasut, Marek <marex at denx.de>; simon.k.r.goldschmidt at gmail.com;
> michal.simek at xilinx.com; u-boot at lists.denx.de; sjg at chromium.org;
> festevam at denx.de; jagan at amarulasolutions.com;
> andre.przywara at arm.com; Armstrong, Neil <narmstrong at baylibre.com>;
> pbrobinson at gmail.com; tharvey at gateworks.com; paul.liu at linaro.org;
> christianshewitt at gmail.com; adrian.fiergolski at fastree3d.com;
> marek.behun at nic.cz; Denk, Wolfgang <wd at denx.de>; Lim, Elly Siew Chin
> <elly.siew.chin.lim at intel.com>; upstream at semihalf.com;
> amstan at 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 at intel.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Paweł Anikiel <pan at semihalf.com>
> > > Sent: Monday, 20 June, 2022 8:14 PM
> > > To: Chee, Tien Fong <tien.fong.chee at intel.com>
> > > Cc: Vasut, Marek <marex at denx.de>; simon.k.r.goldschmidt at gmail.com;
> > > michal.simek at xilinx.com; u-boot at lists.denx.de; sjg at chromium.org;
> > > festevam at denx.de; jagan at amarulasolutions.com;
> > > andre.przywara at arm.com; Armstrong, Neil <narmstrong at baylibre.com>;
> > > pbrobinson at gmail.com; tharvey at gateworks.com; paul.liu at linaro.org;
> > > christianshewitt at gmail.com; adrian.fiergolski at fastree3d.com;
> > > marek.behun at nic.cz; Denk, Wolfgang <wd at denx.de>; Lim, Elly Siew
> Chin
> > > <elly.siew.chin.lim at intel.com>; upstream at semihalf.com;
> > > amstan at 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 at intel.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > > -----Original Message-----
> > > > > From: Paweł Anikiel <pan at semihalf.com>
> > > > > Sent: Friday, 17 June, 2022 6:47 PM
> > > > > To: Vasut, Marek <marex at denx.de>;
> > > > > simon.k.r.goldschmidt at gmail.com; Chee, Tien Fong
> > > > > <tien.fong.chee at intel.com>; michal.simek at xilinx.com
> > > > > Cc: u-boot at lists.denx.de; sjg at chromium.org; festevam at denx.de;
> > > > > jagan at amarulasolutions.com; andre.przywara at arm.com; Armstrong,
> > > Neil
> > > > > <narmstrong at baylibre.com>; pbrobinson at gmail.com;
> > > > > tharvey at gateworks.com; paul.liu at linaro.org;
> > > > > christianshewitt at gmail.com; adrian.fiergolski at fastree3d.com;
> > > > > marek.behun at nic.cz; Denk, Wolfgang <wd at denx.de>; Lim, Elly Siew
> > > Chin
> > > > > <elly.siew.chin.lim at intel.com>; upstream at semihalf.com;
> > > > > amstan at chromium.org; Paweł Anikiel <pan at 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 at 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 at intel.com>

Regards
Tien Fong