[U-Boot] [PATCH v2 3/3] sunxi: Normalise FEL support

[Simon Glass <sjg@chromium.org>]

Hi Siarhei,

On 10 February 2015 at 20:05, Siarhei Siamashka
<siarhei.siamashka@gmail.com> wrote:
> On Mon, 9 Feb 2015 15:23:17 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Siarhei,
>>
>> On 7 February 2015 at 20:48, Siarhei Siamashka
>> <siarhei.siamashka@gmail.com> wrote:
>> > On Sat,  7 Feb 2015 10:47:30 -0700
>> > Simon Glass <sjg@chromium.org> wrote:
>> >
>> >> Make sunxi's FEL code fit with the normal U-Boot boot sequence instead of
>> >> creating its own. There are some #ifdefs required in start.S. Future work
>> >> will hopefully remove these.
>> >>
>> >> This series is available at u-boot-dm, branch sunxi-working.
>> >>
>> >> Signed-off-by: Simon Glass <sjg@chromium.org>
>> >> ---
>> >>
>> >> Changes in v2:
>> >> - Adjust for new save_boot_params() API
>> >> - Drop patch to change r0 to r2 in start.S
>> >> - Add #ifdefs to start.S to deal with FEL
>> >> - Use 'Fast Early Loader' as the full name for FEL
>> >
>> > Thanks for working on these patches. It looks like we are finally
>> > getting really close to resolving the sunxi FEL boot problem.
>> >
>> > Some comments are below.
>> >
>> >>  arch/arm/cpu/armv7/start.S                  |  5 +-
>> >>  arch/arm/cpu/armv7/sunxi/Makefile           |  4 +-
>> >>  arch/arm/cpu/armv7/sunxi/board.c            | 21 ++++++++
>> >>  arch/arm/cpu/armv7/sunxi/config.mk          |  2 -
>> >>  arch/arm/cpu/armv7/sunxi/fel_utils.S        | 25 +++++++++
>> >>  arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds | 82 -----------------------------
>> >>  arch/arm/include/asm/arch-sunxi/sys_proto.h | 10 ++++
>> >>  board/sunxi/Kconfig                         | 10 ++++
>> >>  include/configs/sunxi-common.h              |  6 +--
>> >>  scripts/Makefile.spl                        |  2 -
>> >>  10 files changed, 73 insertions(+), 94 deletions(-)
>> >>  create mode 100644 arch/arm/cpu/armv7/sunxi/fel_utils.S
>> >>  delete mode 100644 arch/arm/cpu/armv7/sunxi/u-boot-spl-fel.lds
>> >>
>> >> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>> >> index 9b49ece..098a83a 100644
>> >> --- a/arch/arm/cpu/armv7/start.S
>> >> +++ b/arch/arm/cpu/armv7/start.S
>> >> @@ -54,7 +54,8 @@ save_boot_params_ret:
>> >>   * (OMAP4 spl TEXT_BASE is not 32 byte aligned.
>> >>   * Continue to use ROM code vector only in OMAP4 spl)
>> >>   */
>> >> -#if !(defined(CONFIG_OMAP44XX) && defined(CONFIG_SPL_BUILD))
>> >> +#if !(defined(CONFIG_OMAP44XX) && defined(CONFIG_SPL_BUILD)) && \
>> >> +             !defined(CONFIG_SPL_FEL)
>> >
>> > Maybe we can just update the 'save_boot_params' function to save the
>> > important configuration registers and then undo this configuration
>> > in 'return_to_fel'? This allows us to avoid the sunxi specific ifdefs
>> > in the core ARM code.
>> >
>> > In this particular case, restoring VBAR and CPSR is important because
>> > the BROM code uses an IRQ handler for FEL (presumably USB related).
>> > And we want to ensure that this IRQ handler works properly again
>> > after resuming the FEL code.
>>
>> We could indeed.
>>
>> It would avoid the #ifdef but my understanding is that you might be
>> able to avoid having to 'return' to the BROM through another means. In
>> any case we could perhaps leave that change until the next release?
>
> I just previously thought that the FEL tool could store the return
> address somewhere in beginning of SRAM in the eGON header extension.
> In this case, restoring the 'lr' and 'sp' registers would be unnecessary
> and the use of the 'save_boot_params' function could be dropped.
>
> But if we still need to save/restore VBAR and the other control
> registers, then this idea about not saving 'lr' and 'sp' does not
> really improve anything.

OK I see see.

>
> [...]
>
>> >>  u32 spl_boot_device(void)
>> >>  {
>> >> +     /*
>> >> +      * Have we been asked to return to the FEL portion of the boot ROM?
>> >> +      * TODO: We need a more robust test here, or bracket this with
>> >> +      * #ifdef CONFIG_SPL_FEL.
>> >> +      */
>> >> +     if (fel_stash.lr >= 0xffff0000 && fel_stash.lr < 0xffff4000)
>> >> +             return BOOT_DEVICE_BOARD;
>> >>       return BOOT_DEVICE_MMC1;
>> >
>> > It is probably better to do it this way:
>> >
>> > #ifdef CONFIG_SPL_FEL
>> >         return BOOT_DEVICE_BOARD;
>> > #else
>> >         if (memcmp((void *)4, "eGON.BT0", 8) == 0)
>> >                 return BOOT_DEVICE_MMC1;
>> >         else
>> >                 return BOOT_DEVICE_BOARD;
>> > #endif
>> >
>> > The memcmp (or equivalent code) ensures that it is compatible with
>> >     https://github.com/ssvb/sunxi-tools/commit/aa7e1880986e5c9a825b08aed9dc5621b821805f
>> >
>> > Then the new 'fel spl u-boot-sunxi-with-spl.bin' command for loading
>> > and executing the SPL works fine without CONFIG_SPL_FEL defined
>> > (because the SD card specific "eGON.BT0" signature is replaced with
>> > the "eGON.FEL" signature by the 'fel' tool in the device memory
>> > before transferring control to the SPL code). Needless to say that
>> > the SPL built this way still works when written to the SD card.
>> >
>> > And if CONFIG_SPL_FEL is defined, then the FEL support still works in a
>> > legacy way (via 'fel write 0x2000 u-boot-spl.bin; fel exe 0x2000').
>> >
>> > We would only need to drop the legacy way in u-boot v2015.07 if the
>> > new one proves to be problem free by that time :-)
>>
>> So you mean that we can drop CONFIG_SPL_FEL?
>
> Yes, we can already do this. But in order to play safe, the
> CONFIG_SPL_FEL option could be still kept for a while to provide
> the users with an alternative method just in case.

OK.

>
>> [snip]
>>
>> >
>> > ENTRY(save_boot_params)
>> >         ldr     r0, =fel_stash
>> >         str     sp, [r0, #0]
>> >         str     lr, [r0, #4]
>> >         mrs     lr, cpsr                @ Read CPSR
>> >         str     lr, [r0, #8]
>> >         mrc     p15, 0, lr, c1, c0, 0   @ Read CP15 SCTLR
>> >         str     lr, [r0, #12]
>> >         mrc     p15, 0, lr, c12, c0, 0  @ Read VBAR
>> >         str     lr, [r0, #16]
>> >         mrc     p15, 0, lr, c1, c0, 0   @ Read CP15 Control
>> >         str     lr, [r0, #20]
>> >         b       save_boot_params_ret
>> > ENDPROC(save_boot_params)
>> >
>> > ENTRY(return_to_fel)
>> >         ldr     r0, =fel_stash
>> >         ldr     lr, [r0, #20]
>> >         mcr     p15, 0, lr, c1, c0, 0   @ Write CP15 Control
>> >         ldr     lr, [r0, #16]
>> >         mcr     p15, 0, lr, c12, c0, 0  @ Write VBAR
>> >         ldr     lr, [r0, #12]
>> >         mcr     p15, 0, lr, c1, c0, 0   @ Write CP15 SCTLR
>> >         ldr     lr, [r0, #8]
>> >         msr     cpsr, lr                @ Write CPSR
>> >         ldr     sp, [r0, #0]
>> >         ldr     lr, [r0, #4]
>> >         bx      lr
>> > ENDPROC(return_to_fel)
>> >
>> >
>> > This seems to work for me. However we probably might try to skip
>> > restoring some of these registers. Also somebody might want to
>> > review the order in which the registers should be restored.
>>
>> I can certainly incorporate this into the patch.
>>
>> Is that what you would prefer? It seems more complicated, and also a
>> bit odd to save a whole load of data that we then corrupt and restore,
>> but it is fine with me.
>
> This is a way not to touch the Albert's code in 'start.S', but instead
> deal with the problem in the sunxi code.
>
> If Albert has objections against the introduction of special flags to
> skip setup of these control registers, then I think that saving them
> early and restoring back to the original values when returning back
> to the BROM as the only possible alternative.
>
> The BROM will not be able to work correctly if the VBAR register is
> clobbered by the SPL.
>
> Yes, this all is a bit odd. But as Hans explained earlier, the main
> u-boot binary has to be pretty much standalone and can't rely on the
> stuff being configured by the SPL:
>
>     http://lists.denx.de/pipermail/u-boot/2015-January/202204.html

As I may have mentioned I am not keen on this. If this is aimed at
engineers they can decide how they want to configure things for
development purposes. But I will leave this to you.

Albert specifically requested #ifdef instead of run-time so I don't
think he objects.

>
>
> [...]
>
>> > One more theoretical concern is the placement of the stack in the legacy
>> > FEL mode after your changes. Now it seems to be moved to 0x8000, which
>> > might be fine though, according to the memory map from
>> >
>> >     https://github.com/hno/Allwinner-Info/blob/master/FEL-usb/USB-protocol.txt
>> >
>> > If we want to preserve the old behaviour, then we would need to place
>> > it around 0x5d00.
>>
>> I'm wondering if it might be better for you to take over and respin
>> this patch? My role has perhaps been to provide a proof-of-concept
>> implementation, but I don't need to do the final bit.
>>
>> Let me know what you think.
>
> I'm just applying these changes on top of your patches (just all the
> same as is discussed here):
>
>     https://github.com/ssvb/u-boot-sunxi/commit/6385323f7704b3802f594a05fc8c373a4617ebbc
>
> And after this, the CONFIG_SPL_FEL define becomes unnecessary. Even
> though this old method still works (relying on the CONFIG_SPL_FEL
> define and uploading the SPL to 0x2000).
>
> You could squash these changes in your third patch if nobody has
> objections.
>
> Regarding the stack location. The stack at 0x8000 might be also
> perfectly fine. We have not done a detailed analysis of the BROM
> disassembly, so can't be absolutely sure about anything. But
> based on just treating the BROM as a black box and running some
> experimental tests, it looks OK so far.
>
> Maybe Hans has an opinion about how we should deal with it?

I believe that the patch I sent works OK. So maybe the best thing is
for you to send a new patch on top of that.

If Albert can ACK my series (+/- the last patch) and then you can take
it through the sunxi tree I think. (I don't mind, this is just a
suggestion).

What I am getting at is that I feel I have provided a possible
solution but I don't want to get in the way of you deciding how best
to arrange things for sunxi. I think it is reasonable for you to merge
a few changes on top of what I have for this release. My goal is to
drop gdata. I'm really pleased to learn about FEL but I don't mind too
much exactly how it works.

Regards,
Simon