[U-Boot] QSPI XIP boot on am437x

Hi,

On 11/11/2015 02:33 PM, Albert ARIBAUD wrote:

[...]

> Alternatively, you could test the patch at
> 
> 	http://patchwork.ozlabs.org/patch/542558/
> 
> Let us know if this solves your issue. If it does, then it will confirm
> the issue is with arch_setup_gd not being able to set up your GD for
> some reason.
> 
> IMPORTANT: patch 542558 is *not* a final patch (there will be at least
> a v3), and may even never land in u-boot/master; but some equivalent will
> eventually do. Right now, patch 542558 is only a way to test if your
> issue is a GD one.
> 

The above patch affects the code that executes after s_init(), therefore
I don't think above patch matters (_main is run after s_init()).

As you said in your first reply "If s_init() runs before
board_init_f_mem(), then you must move it to
run after board_init_f_mem().", this is the problem with my case.
s_init() is running *before* setting up of global_data in
arch/arm/lib/crt0.S (in _main), hence IMO, calling serial_init() in
s_init() is wrong. This has to be moved to board_init_f() (in
arch/arm/cpu/armv7/am33xx/board.c).
The comment in arch/arm/cpu/armv7/lowlevel_init.S that calls s_init()
also says *not* to access global_data and not to try to start console.
Therefore, I intend to do something like this:



Do you see any issues with above change?

Hello Vignesh,

On Fri, 13 Nov 2015 11:22:19 +0530, Vignesh R <vigneshr@ti.com> wrote:
> Hi,
> 
> On 11/11/2015 02:33 PM, Albert ARIBAUD wrote:
> 
> [...]
> 
> > Alternatively, you could test the patch at
> > 
> > 	http://patchwork.ozlabs.org/patch/542558/
> > 
> > Let us know if this solves your issue. If it does, then it will confirm
> > the issue is with arch_setup_gd not being able to set up your GD for
> > some reason.
> > 
> > IMPORTANT: patch 542558 is *not* a final patch (there will be at least
> > a v3), and may even never land in u-boot/master; but some equivalent will
> > eventually do. Right now, patch 542558 is only a way to test if your
> > issue is a GD one.
> > 
> 
> The above patch affects the code that executes after s_init(), therefore
> I don't think above patch matters (_main is run after s_init()).

See (1) below.

> As you said in your first reply "If s_init() runs before
> board_init_f_mem(), then you must move it to
> run after board_init_f_mem().", this is the problem with my case.
> s_init() is running *before* setting up of global_data in
> arch/arm/lib/crt0.S (in _main), hence IMO, calling serial_init() in
> s_init() is wrong. This has to be moved to board_init_f() (in
> arch/arm/cpu/armv7/am33xx/board.c).
> The comment in arch/arm/cpu/armv7/lowlevel_init.S that calls s_init()
> also says *not* to access global_data and not to try to start console.
> Therefore, I intend to do something like this:
> 
> diff --git a/arch/arm/cpu/armv7/am33xx/board.c
> b/arch/arm/cpu/armv7/am33xx/board.c
> index bd14326cf479..351fc37b0483 100644
> --- a/arch/arm/cpu/armv7/am33xx/board.c
> +++ b/arch/arm/cpu/armv7/am33xx/board.c
> @@ -256,6 +256,11 @@ void board_init_f(ulong dummy)
>  {
>         board_early_init_f();
>         sdram_init();
> +#if defined(CONFIG_NOR_BOOT) || defined(CONFIG_QSPI_BOOT)
> +       gd->baudrate = CONFIG_BAUDRATE;
> +       serial_init();
> +       gd->have_console = 1;
> +#endif
>  }
>  #endif /* end  CONFIG_SPL_BUILD */
> 
> @@ -273,12 +278,6 @@ void s_init(void)
>         set_uart_mux_conf();
>         setup_clocks_for_console();
>         uart_soft_reset();
> -#if defined(CONFIG_NOR_BOOT) || defined(CONFIG_QSPI_BOOT)
> -       /* TODO: This does not work, gd is not available yet */
> -       gd->baudrate = CONFIG_BAUDRATE;
> -       serial_init();
> -       gd->have_console = 1;
> -#endif
>  #if defined(CONFIG_SPL_AM33XX_ENABLE_RTC32K_OSC)
>         /* Enable RTC32K clock */
>         rtc32k_enable();
> 
> Do you see any issues with above change?

(1) So your s_init runs even before board_init_f_mem(), right?

Your working fix seems to imply that as long as s_init() is run after
board_init_f_mem (and any time before board_init_f) it will work. If
so, then another, fix, preferable to the above, would be that the call
to s_init be moved between those to board_init_f_mem and board_init_f.
Can you test that?

Amicalement,

On 11/13/2015 11:58 AM, Albert ARIBAUD wrote:
> Hello Vignesh,
> 
> On Fri, 13 Nov 2015 11:22:19 +0530, Vignesh R <vigneshr@ti.com> wrote:
>> Hi,
>>
>> On 11/11/2015 02:33 PM, Albert ARIBAUD wrote:
>>
>> [...]
>>
>>> Alternatively, you could test the patch at
>>>
>>> 	http://patchwork.ozlabs.org/patch/542558/
>>>
>>> Let us know if this solves your issue. If it does, then it will confirm
>>> the issue is with arch_setup_gd not being able to set up your GD for
>>> some reason.
>>>
>>> IMPORTANT: patch 542558 is *not* a final patch (there will be at least
>>> a v3), and may even never land in u-boot/master; but some equivalent will
>>> eventually do. Right now, patch 542558 is only a way to test if your
>>> issue is a GD one.
>>>
>>
>> The above patch affects the code that executes after s_init(), therefore
>> I don't think above patch matters (_main is run after s_init()).
> 
> See (1) below.
> 
>> As you said in your first reply "If s_init() runs before
>> board_init_f_mem(), then you must move it to
>> run after board_init_f_mem().", this is the problem with my case.
>> s_init() is running *before* setting up of global_data in
>> arch/arm/lib/crt0.S (in _main), hence IMO, calling serial_init() in
>> s_init() is wrong. This has to be moved to board_init_f() (in
>> arch/arm/cpu/armv7/am33xx/board.c).
>> The comment in arch/arm/cpu/armv7/lowlevel_init.S that calls s_init()
>> also says *not* to access global_data and not to try to start console.
>> Therefore, I intend to do something like this:
>>
>> diff --git a/arch/arm/cpu/armv7/am33xx/board.c
>> b/arch/arm/cpu/armv7/am33xx/board.c
>> index bd14326cf479..351fc37b0483 100644
>> --- a/arch/arm/cpu/armv7/am33xx/board.c
>> +++ b/arch/arm/cpu/armv7/am33xx/board.c
>> @@ -256,6 +256,11 @@ void board_init_f(ulong dummy)
>>  {
>>         board_early_init_f();
>>         sdram_init();
>> +#if defined(CONFIG_NOR_BOOT) || defined(CONFIG_QSPI_BOOT)
>> +       gd->baudrate = CONFIG_BAUDRATE;
>> +       serial_init();
>> +       gd->have_console = 1;
>> +#endif
>>  }
>>  #endif /* end  CONFIG_SPL_BUILD */
>>
>> @@ -273,12 +278,6 @@ void s_init(void)
>>         set_uart_mux_conf();
>>         setup_clocks_for_console();
>>         uart_soft_reset();
>> -#if defined(CONFIG_NOR_BOOT) || defined(CONFIG_QSPI_BOOT)
>> -       /* TODO: This does not work, gd is not available yet */
>> -       gd->baudrate = CONFIG_BAUDRATE;
>> -       serial_init();
>> -       gd->have_console = 1;
>> -#endif
>>  #if defined(CONFIG_SPL_AM33XX_ENABLE_RTC32K_OSC)
>>         /* Enable RTC32K clock */
>>         rtc32k_enable();
>>
>> Do you see any issues with above change?
> 
> (1) So your s_init runs even before board_init_f_mem(), right?
> 
> Your working fix seems to imply that as long as s_init() is run after
> board_init_f_mem (and any time before board_init_f) it will work. If
> so, then another, fix, preferable to the above, would be that the call
> to s_init be moved between those to board_init_f_mem and board_init_f.
> Can you test that?

Yes, gd area gets initialized to 0 in board_init_f_mem(). Initializing
the console thereafter fixes the issue. There is nothing between call to
board_init_f_mem and board_init_f. board_init_f gets called right after
board_init_f_mem (in arch/arm/lib/crt0.S), therefore I thought of moved
in serial_init call to board_init_f as above.

Hello Vignesh,

On Mon, 16 Nov 2015 16:00:58 +0530, Vignesh R <vigneshr@ti.com> wrote:
> 
> 
> On 11/13/2015 11:58 AM, Albert ARIBAUD wrote:
> > Hello Vignesh,
> > 
> > On Fri, 13 Nov 2015 11:22:19 +0530, Vignesh R <vigneshr@ti.com> wrote:
> >> Hi,
> >>
> >> On 11/11/2015 02:33 PM, Albert ARIBAUD wrote:
> >>
> >> [...]
> >>
> >>> Alternatively, you could test the patch at
> >>>
> >>> 	http://patchwork.ozlabs.org/patch/542558/
> >>>
> >>> Let us know if this solves your issue. If it does, then it will confirm
> >>> the issue is with arch_setup_gd not being able to set up your GD for
> >>> some reason.
> >>>
> >>> IMPORTANT: patch 542558 is *not* a final patch (there will be at least
> >>> a v3), and may even never land in u-boot/master; but some equivalent will
> >>> eventually do. Right now, patch 542558 is only a way to test if your
> >>> issue is a GD one.
> >>>
> >>
> >> The above patch affects the code that executes after s_init(), therefore
> >> I don't think above patch matters (_main is run after s_init()).
> > 
> > See (1) below.
> > 
> >> As you said in your first reply "If s_init() runs before
> >> board_init_f_mem(), then you must move it to
> >> run after board_init_f_mem().", this is the problem with my case.
> >> s_init() is running *before* setting up of global_data in
> >> arch/arm/lib/crt0.S (in _main), hence IMO, calling serial_init() in
> >> s_init() is wrong. This has to be moved to board_init_f() (in
> >> arch/arm/cpu/armv7/am33xx/board.c).
> >> The comment in arch/arm/cpu/armv7/lowlevel_init.S that calls s_init()
> >> also says *not* to access global_data and not to try to start console.
> >> Therefore, I intend to do something like this:
> >>
> >> diff --git a/arch/arm/cpu/armv7/am33xx/board.c
> >> b/arch/arm/cpu/armv7/am33xx/board.c
> >> index bd14326cf479..351fc37b0483 100644
> >> --- a/arch/arm/cpu/armv7/am33xx/board.c
> >> +++ b/arch/arm/cpu/armv7/am33xx/board.c
> >> @@ -256,6 +256,11 @@ void board_init_f(ulong dummy)
> >>  {
> >>         board_early_init_f();
> >>         sdram_init();
> >> +#if defined(CONFIG_NOR_BOOT) || defined(CONFIG_QSPI_BOOT)
> >> +       gd->baudrate = CONFIG_BAUDRATE;
> >> +       serial_init();
> >> +       gd->have_console = 1;
> >> +#endif
> >>  }
> >>  #endif /* end  CONFIG_SPL_BUILD */
> >>
> >> @@ -273,12 +278,6 @@ void s_init(void)
> >>         set_uart_mux_conf();
> >>         setup_clocks_for_console();
> >>         uart_soft_reset();
> >> -#if defined(CONFIG_NOR_BOOT) || defined(CONFIG_QSPI_BOOT)
> >> -       /* TODO: This does not work, gd is not available yet */
> >> -       gd->baudrate = CONFIG_BAUDRATE;
> >> -       serial_init();
> >> -       gd->have_console = 1;
> >> -#endif
> >>  #if defined(CONFIG_SPL_AM33XX_ENABLE_RTC32K_OSC)
> >>         /* Enable RTC32K clock */
> >>         rtc32k_enable();
> >>
> >> Do you see any issues with above change?
> > 
> > (1) So your s_init runs even before board_init_f_mem(), right?
> > 
> > Your working fix seems to imply that as long as s_init() is run after
> > board_init_f_mem (and any time before board_init_f) it will work. If
> > so, then another, fix, preferable to the above, would be that the call
> > to s_init be moved between those to board_init_f_mem and board_init_f.
> > Can you test that?
> 
> Yes, gd area gets initialized to 0 in board_init_f_mem(). Initializing
> the console thereafter fixes the issue. There is nothing between call to
> board_init_f_mem and board_init_f. board_init_f gets called right after
> board_init_f_mem (in arch/arm/lib/crt0.S), therefore I thought of moved
> in serial_init call to board_init_f as above.

Can you please submit a patch? Bugfixes are welcome even when the merge
window is closed.

Amicalement,

Hi Albert,

On 11/16/2015 05:16 PM, Albert ARIBAUD wrote:
> Hello Vignesh,
> 

[...]

>>>>
>>>> Do you see any issues with above change?
>>>
>>> (1) So your s_init runs even before board_init_f_mem(), right?
>>>
>>> Your working fix seems to imply that as long as s_init() is run after
>>> board_init_f_mem (and any time before board_init_f) it will work. If
>>> so, then another, fix, preferable to the above, would be that the call
>>> to s_init be moved between those to board_init_f_mem and board_init_f.
>>> Can you test that?
>>
>> Yes, gd area gets initialized to 0 in board_init_f_mem(). Initializing
>> the console thereafter fixes the issue. There is nothing between call to
>> board_init_f_mem and board_init_f. board_init_f gets called right after
>> board_init_f_mem (in arch/arm/lib/crt0.S), therefore I thought of moved
>> in serial_init call to board_init_f as above.
> 
> Can you please submit a patch? Bugfixes are welcome even when the merge
> window is closed.
> 

Yeah, I will submit a patch shortly. Thanks!