SAMA5D3 Xplained: SPL broken after panic added to /lib/time.c:94

["Manuel Luís Reis" <mluis.reis@gmail.com>]

> Okay, I am glad that you somehow found a solution.
> I am not sure why the ddr2_init call to udelay fails. Maybe we should
> initialize something (timer) before calling ddr2_init ?
> Or timers are initialized just after ddr2 , in which case we need to
> revert the order ?

I don't really understand how the timer gets initiated. Doesn't the
UCLASS framework handle it?  It does seem to be after board_init_f.
There is an option to make it available early, but it doesn't seem to
be implemented for this board. Maybe Claudiu can give a few pointers?
If I get some free time I'll try to dig further on this topic.

> To make a proper review, please use 'git send-email' command to send
> your patch , not attach it to the email here.
>
> If you modify the device tree, and some other code, you need to have two
> separate patches.
> each patch has to adhere to the subject rule : subsystem: component:
> change ...
>
> Check other commits in the area you are committing to see how they look like

Got it. Thanks.

I have sent the first patch for the device tree, but didn't go quite
as planned. Let me know. if there is a need to correct anything.

Cheers

On Mon, 5 Apr 2021 at 17:00, <Eugen.Hristev@microchip.com> wrote:
>
> On 4/5/21 6:42 PM, Manuel Lu?s Reis wrote:
>
> > Hi Eugen,
> >
> >  > Does the node have the property
> >  >
> >  > u-boot,dm-pre-reloc;
> >  >
> >  > Maybe try to add this and see if it changes anything ?
> >
> > Thanks. Indeed it didn't have those properties. I have added them and
> > also had to remove the udelay call in ddr2_init for the fix to work.
> > The udelay call in ddr2_init was also failing (maybe too early for a
> > timer call!?), but it seems to be redundant as it sits between two nops,
> > and is working well as far as I can tell. If extra time is needed in
> > that place, maybe extra NOPs could be considered instead.
> >
> > I am sending a patch for the fix. Could you review it please?
>
> Okay, I am glad that you somehow found a solution.
> I am not sure why the ddr2_init call to udelay fails. Maybe we should
> initialize something (timer) before calling ddr2_init ?
> Or timers are initialized just after ddr2 , in which case we need to
> revert the order ?
>
> To make a proper review, please use 'git send-email' command to send
> your patch , not attach it to the email here.
>
> If you modify the device tree, and some other code, you need to have two
> separate patches.
> each patch has to adhere to the subject rule : subsystem: component:
> change ...
>
> Check other commits in the area you are committing to see how they look like
>
> >
> > Let me know if you'd like me to change anything.
> >
> > Thanks again,
> > --------------------------------------------------------------------------------------------------------
> >
> >
> >  From cff45e15a96facc702c852a9beff27907b2c92e5 Mon Sep 17 00:00:00 2001
> > From: Manuel Reis <mluis.reis at gmail.com <mailto:mluis.reis@gmail.com>>
> > Date: Mon, 5 Apr 2021 16:31:38 +0100
> > Subject: [PATCH] add u-boot properties to pit timer
> >
> > timer node was not detected when udelay was used.
> > udelay call removed from ddr2_init as: it sits between two NOP;
> >
> > Signed-off-by: Manuel Reis <mluis.reis@gmail.com
> > <mailto:mluis.reis@gmail.com>>
> > ---
> >   arch/arm/dts/sama5d3.dtsi   | 1 +
> >   arch/arm/mach-at91/mpddrc.c | 3 ---
> >   2 files changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/dts/sama5d3.dtsi b/arch/arm/dts/sama5d3.dtsi
> > index 6ed218eaad..3cd30d574d 100644
> > --- a/arch/arm/dts/sama5d3.dtsi
> > +++ b/arch/arm/dts/sama5d3.dtsi
> > @@ -1316,6 +1316,7 @@
> >                          };
> >
> >                          pit: timer at fffffe30 {
> > +                               u-boot,dm-pre-reloc;
> >                                  compatible = "atmel,at91sam9260-pit";
> >                                  reg = <0xfffffe30 0xf>;
> >                                  interrupts = <3 IRQ_TYPE_LEVEL_HIGH 5>;
> > diff --git a/arch/arm/mach-at91/mpddrc.c b/arch/arm/mach-at91/mpddrc.c
> > index 5422c05456..2e52595e30 100644
> > --- a/arch/arm/mach-at91/mpddrc.c
> > +++ b/arch/arm/mach-at91/mpddrc.c
> > @@ -66,9 +66,6 @@ int ddr2_init(const unsigned int base,
> >          /* Issue a NOP command */
> >          atmel_mpddr_op(mpddr, ATMEL_MPDDRC_MR_MODE_NOP_CMD, ram_address);
> >
> > -       /* A 200 us is provided to precede any signal toggle */
> > -       udelay(200);
> > -
> >          /* Issue a NOP command */
> >          atmel_mpddr_op(mpddr, ATMEL_MPDDRC_MR_MODE_NOP_CMD, ram_address);
> >
> > --
> > 2.27.0
> >
> >
> >
> >
> >
> >
> >
> > On Mon, 5 Apr 2021 at 14:58, <Eugen.Hristev@microchip.com
> > <mailto:Eugen.Hristev@microchip.com>> wrote:
> >
> >     On 4/5/21 3:47 PM, Manuel Lu?s Reis wrote:
> >      > Hi Eugen,
> >      >
> >      >>   As Sean Anderson pointed out : the call to timer must have not
> >     worked at
> >      >> all before this change that now breaks things.
> >      >> Have you tried removing the call to the timer from this
> >     mem_init, to see
> >      >> if this makes the board boot correctly ?
> >      >>
> >      >> In mem_init I guess there are multiple calls to this timer function.
> >      >
> >      > Indeed I have. After my previous emails trying to find if there was
> >      > anything messing with the timer, I went the other way and dug further
> >      > into mem_init and ddr2_init. In ddr2_init there is a call to udelay,
> >      > udelay(200) in arch/arm/mach-at91/mpddrc.c line 70.
> >      >
> >      > I have commented this out in v2020.07 with no visible
> >     consequences. In
> >      > v2021.01, SPL did move a bit further, BUT when dealing with the sd
> >      > card, within the mmc driver, the errors continued. There are quite a
> >      > few udelay() calls within the mmc drivers, so I felt I shouldn't be
> >      > changing that.
> >      >
> >      > Instead, I went to check the udelay -> get_ticks -> dm_timer_init
> >      > function. And it seems the problem is exactly here. The function
> >     tries
> >      > to find a timer in the device tree, if I understand correctly, but it
> >      > doesn't find one, returning the -ENODEV  error, on the following
> >      > function: ret = uclass_first_device_err(UCLASS_TIMER, &dev);
> >      >
> >      > I checked the device tree and the pit timer is there. The UCLASS
> >      > driver looks good to me as per documentation. I have also tried
> >     adding
> >      > the "chosen" "tick-timer" parameter in the device tree pointing
> >     to the
> >      > pit timer so it would be recognized in the PLATDATA section of this
> >      > function, but still it wasn't recognized. (I might not have done this
> >      > correctly though it was compiled successfully)
> >      >
> >      > So there seems to be an issue of some kind of misconfiguration with
> >      > the declaration of the timer in the device tree that it doesn't get
> >      > read back.
> >
> >
> >     Does the node have the property
> >
> >     u-boot,dm-pre-reloc;
> >
> >     Maybe try to add this and see if it changes anything ?
> >
> >      >
> >      > Any ideas?
> >      >
> >      > Thanks
> >      >
> >      >
> >      >
> >      >
> >      >
> >      >
> >      >
> >      >
> >      > On Mon, 5 Apr 2021 at 13:44, <Eugen.Hristev@microchip.com
> >     <mailto:Eugen.Hristev@microchip.com>> wrote:
> >      >>
> >      >> On 4/4/21 7:25 PM, Manuel Reis wrote:
> >      >>> Hi again,
> >      >>>
> >      >>> I guess I misunderstood things a bit. Apologies for that.
> >      >>> Nevertheless, timer_init in board_init_f is pointing to the weak
> >      >>> timer_init in /lib/time.c, which is empty.
> >      >>>
> >      >>> Is there a way we can force start the pit timer?
> >      >>> Any pointer would be very helpful. Let me know if you'd like me
> >     to do
> >      >>> something in particular.
> >      >>>
> >      >>> Thanks
> >      >>>
> >      >>>
> >      >>>
> >      >>> On sex, 2 abr, 2021 at 18:15, Manuel Lu?s Reis
> >     <mluis.reis at gmail.com <mailto:mluis.reis@gmail.com>>
> >      >>> wrote:
> >      >>>> FYI:
> >      >>>>
> >      >>>> the output from serial port:
> >      >>>> <debug_uart>
> >      >>>> board_init_f spl_atmel.c 130 mem_init 182 ddr2_init mpddr.c
> >     66udelay
> >      >>>> lib time.c 196__udelay lib time.c 177Could not initialize
> >     timer (err
> >      >>>> -11)
> >      >>>>
> >      >>>> udelay lib time.c 196__udelay lib time.c 177Could not initialize
> >      >>>> timer (err -11)
> >      >>>>
> >      >>>> udelay lib time.c 196__udelay lib time.c 177Could not initialize
> >      >>>> timer (err -11)
> >      >>>> ...
> >      >>>>
> >      >>>>
> >      >>>>
> >      >>>>
> >      >>>> On Fri, 2 Apr 2021 at 18:12, Manuel Lu?s Reis
> >     <mluis.reis at gmail.com <mailto:mluis.reis@gmail.com>>
> >      >>>> wrote:
> >      >>>>>
> >      >>>>>   > As it seems from the dump of dm_dump_all() the
> >     atmel_pit_timer is
> >      >>>>> not
> >      >>>>>   > probed. I did a bit of debug and the dm_timer_init() ->
> >      >>>>>   > uclass_first_device() -> uclass_find_first_device() found
> >     zero
> >      >>>>> timers
> >      >>>>>   > registered for UCLASS_TIMER. The driver is compiled.  Also
> >      >>>>> checked that
> >      >>>>>   > atmel_pit_timer probe function is not called at all. The
> >     question
> >      >>>>> should be
> >      >>>>>   > why it is not probed at all?
> >      >>>>>
> >      >>>>>   Hi,
> >      >>>>>
> >      >>>>>   So, I put objdump and puts to some good use and could
> >     backtrace the
> >      >>>>>   initial error to a udelay call in ddr2_init function on mpddr.c
> >      >>>>> file.
> >      >>>>>   This function is called from mem_init on
> >      >>>>>   sama5d3_xplained/sama5d3_xplained.c, which in turn is
> >     called from
> >      >>>>>   board_init_f on spl_atmel.c.
> >      >>
> >      >> Hi Manuel,
> >      >>
> >      >> As Sean Anderson pointed out : the call to timer must have not
> >     worked at
> >      >> all before this change that now breaks things.
> >      >> Have you tried removing the call to the timer from this
> >     mem_init, to see
> >      >> if this makes the board boot correctly ?
> >      >>
> >      >> In mem_init I guess there are multiple calls to this timer function.
> >      >>
> >      >>>>>   I couldn't, however, find which timer_init function is
> >     being called
> >      >>>>>   here. I still have a few to try, but disassembly gives me a
> >     pretty
> >      >>>>>   empty function:
> >      >>>>>
> >      >>>>>   00303690 <timer_init>:
> >      >>>>>     303690:       e3a00000        mov     r0, #0
> >      >>>>>     303694:       e12fff1e        bx      lr
> >      >>>>>
> >      >>>>>   This work didn't give me many answers, but I got a couple
> >     of newbie
> >      >>>>> questions:
> >      >>>>>
> >      >>>>>   Why is it calling board_init_f from spl_atmel.c and not
> >     spl_at91.c?
> >      >>>>>   Isn't the latter the appropriate architecture for this board?
> >      >>>>>   Do you know which timer_init function it is getting here?
> >     Could
> >      >>>>> this
> >      >>>>>   be the reason timer isn't getting probed? >>>>>
> >      >>>>>   Cheers,
> >      >>>
> >      >>>
> >      >>
> >
>