| Message ID | CAJ+vNU3mb5GqVYkPCMa7WGzhgM-H15Nuzsv0_a5iRTf9aQx--g@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Stefano Babic |
| Headers | show |
On Tue, May 13, 2014 at 10:03 PM, Tim Harvey <tharvey@gateworks.com> wrote: > > On Tue, May 13, 2014 at 9:58 PM, Tim Harvey <tharvey@gateworks.com> wrote: > > On Wed, May 7, 2014 at 2:29 AM, Stefano Babic <sbabic@denx.de> wrote: > >> Hi Tim, > >> > >> On 06/05/2014 20:18, Tim Harvey wrote: > >>> > >>> Stefano / York, > >>> > >>> While preparing for a v3 patch series of my IMX6 SPL bootloader, I > >>> find that commit dec1861be90c948ea9fb771927d3d26a994d2e20 [1] breaks > >>> the above code because gd is now needed within setup_i2c. > >>> > >>> I've always been a bit fuzzy on the order of the above calls so I dug > >>> through the code and I think I understand things better. Please > >>> correct any wrong assumptions I'm making below: > >>> - assignment to gd should 'always' be first (before anything needs > >>> it, so why not do it first) > >>> - arch_cpu_init() should go next as this sets up very low level > >>> CPU/SoC resources (in this case AIPS config and watchdog disable) > >>> - board_early_init_f() should be next as that sets up any board-specific iomux > >>> - any additional iomux necessary for SPL should go next (I take care > >>> of i2c iomux and setup here) > >>> - timer_init() next as you need a timer for UART and mxc i2c (for > >>> delays and busy checks) > >>> - preloader_console_init() next as we are now able to send something > >>> over the UART (this gives me early debug for sdram config now too!) > >>> - sdram setup goes next > >>> - after sdram is setup, the bss can be cleared > >>> - board_init_r - pass over to generic SPL code which will load/call > >>> an image based on boot device > >> > >> I think your analyses is correct. > >> > >>> > >>> So, if the above is correct, I should rework the above function as follows: > >>> > >>> void board_init_f(ulong dummy) > >>> { > >>> struct ventana_board_info ventana_info; > >>> int board_model; > >>> > >>> /* Set global data pointer. */ > >>> gd = &gdata; > >>> > >>> /* setup AIPS and disable watchdog */ > >>> arch_cpu_init(); > >>> > >>> /* iomux and setup of i2c */ > >>> board_early_init_f(); > >>> i2c_setup_iomux(); > >>> > >>> /* setup GP timer */ > >>> timer_init(); > >>> > >>> /* UART clocks enabled and gd valid - init serial console */ > >>> preloader_console_init(); > >>> > >>> /* read/validate EEPROM info to determine board model and SDRAM cfg */ > >>> board_model = read_eeprom(I2C_GSC, &ventana_info); > >>> > >>> /* provide some some default: 32bit 128MB */ > >>> if (GW_UNKNOWN == board_model) { > >>> ventana_info.sdram_width = 2; > >>> ventana_info.sdram_size = 3; > >>> } > >>> > >>> /* configure MMDC for SDRAM width/size and per-model calibration */ > >>> spl_dram_init(8 << ventana_info.sdram_width, > >>> 16 << ventana_info.sdram_size, > >>> board_model); > >>> > >>> /* Clear the BSS. */ > >>> memset(__bss_start, 0, __bss_end - __bss_start); > >>> > >>> /* load/boot image from boot device */ > >>> board_init_r(NULL, 0); > >>> } > >> > >> It seems reasonable, go on this way. > >> > >> Regards, > >> Stefano > >> > > > > Stefano, > > > > I've just found that one of my boards fails with the above re-org. > > Strangely a board which has the same mem layout, mem width/size, CPU > > and nand does not fail. > > > > If I make the following change: > > (sorry, accidentally fat fingered and sent instead of pasting). I'll continue: > > --- a/board/gateworks/gw_ventana/gw_ventana_spl.c > +++ b/board/gateworks/gw_ventana/gw_ventana_spl.c > @@ -380,9 +380,6 @@ void board_init_f(ulong dummy) > */ > memset((void *)gd, 0, sizeof(struct global_data)); > > - /* setup AIPS and disable watchdog */ > - arch_cpu_init(); > - > /* iomux and setup of i2c */ > board_early_init_f(); > i2c_setup_iomux(); > @@ -407,6 +404,9 @@ void board_init_f(ulong dummy) > 16 << ventana_info.sdram_size, > board_model); > > + /* setup AIPS and disable watchdog */ > + arch_cpu_init(); > + > /* Clear the BSS. */ > memset(__bss_start, 0, __bss_end - __bss_start); > > Things start to work properly. I took a look at arch_cpu_init() and > the call that really needs to be moved (or just duplicated here) is > mxs_dma_init() to start APBH DMA. The failure mode of the one board is > that apparently it hangs while loading u-boot.img from NAND (which of > course uses mxs_nand and thus APBH DMA). > > Anyone know what's going on here and why mxs_dma_init() needs to be > called after MMDC setup, and before clearing the BSS? I don't like > having magic placement of functions without understanding why. > > Thanks, > > Tim I figured this one out - it has nothing to do with the order of calling arch_cpu_init() its that the MMDC isn't always 'ready' by the time the BSS is cleared and thus in my failure case the BSS isn't getting entirely cleared which causes the spl_image global var to not be cleared as expected and triggers an invalid codepath. I will update the mmdc config patch when I find the right solution. Sorry for the noise. Tim
Hi Tim, On 15/05/2014 00:32, Tim Harvey wrote: > > I figured this one out - it has nothing to do with the order of > calling arch_cpu_init() its that the MMDC isn't always 'ready' by the > time the BSS is cleared and thus in my failure case the BSS isn't > getting entirely cleared which causes the spl_image global var to not > be cleared as expected and triggers an invalid codepath. I will update > the mmdc config patch when I find the right solution. Thanks for the explanation ! I cannot guess why arch_cpu_init() should be called later. Nice you found the reason ! > > Sorry for the noise. There was no noise ;-) Best regards, Stefano
--- a/board/gateworks/gw_ventana/gw_ventana_spl.c +++ b/board/gateworks/gw_ventana/gw_ventana_spl.c @@ -380,9 +380,6 @@ void board_init_f(ulong dummy) */ memset((void *)gd, 0, sizeof(struct global_data)); - /* setup AIPS and disable watchdog */ - arch_cpu_init(); - /* iomux and setup of i2c */ board_early_init_f(); i2c_setup_iomux(); @@ -407,6 +404,9 @@ void board_init_f(ulong dummy) 16 << ventana_info.sdram_size, board_model); + /* setup AIPS and disable watchdog */ + arch_cpu_init(); + /* Clear the BSS. */ memset(__bss_start, 0, __bss_end - __bss_start);