> 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.hris...@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.r...@gmail.com <mailto:mluis.r...@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.r...@gmail.com > > <mailto:mluis.r...@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@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.hris...@microchip.com > > <mailto:eugen.hris...@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.hris...@microchip.com > > <mailto:eugen.hris...@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.r...@gmail.com <mailto:mluis.r...@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.r...@gmail.com <mailto:mluis.r...@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, > > >>> > > >>> > > >> > > >