Hi Geert,
I am trying to understand this thing, it looks useful!
On Wed, Sep 2, 2020 at 5:36 PM Geert Uytterhoeven
<geert+renesas@xxxxxxxxx> wrote:
> @@ -254,8 +254,56 @@ not_angel:
This looks like it happens before CONFIG_ARM_ATAG_DTB_COMPAT
meaning it will not use the DTB that is augmented with ATAGs,
especially not ATAG_MEM (0x54410002) correct?
That seems sad, because that would actually be useful to me.
Can you see if it is possible to put this in after the ATAGs have
been merged into the DTB?
> #ifdef CONFIG_AUTO_ZRELADDR
> +#ifdef CONFIG_USE_OF
> /*
> - * Find the start of physical memory. As we are executing
> + * Find the start of physical memory.
> + * Try the DTB first, if available.
> + */
> + adr r0, LC1
> + ldr sp, [r0] @ get stack location
> + add sp, sp, r0 @ apply relocation
> +
> +#ifdef CONFIG_ARM_APPENDED_DTB
> + /*
> + * Look for an appended DTB. If found, use it and
> + * move stack away from it.
> + */
> + ldr r6, [r0, #4] @ get &_edata
> + add r6, r6, r0 @ relocate it
> + ldmia r6, {r0, r5} @ get DTB signature and size
> +#ifndef __ARMEB__
> + ldr r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
> + /* convert DTB size to little endian */
> + eor r2, r5, r5, ror #16
> + bic r2, r2, #0x00ff0000
> + mov r5, r5, ror #8
> + eor r5, r5, r2, lsr #8
> +#else
> + ldr r1, =0xd00dfeed
> +#endif
We now have two and even three copies of this code,
sort of. Right above CONFIG_ARM_ATAG_DTB_COMPAT
there is this:
#ifdef CONFIG_ARM_APPENDED_DTB
(...)
ldr lr, [r6, #0]
#ifndef __ARMEB__
ldr r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
#else
ldr r1, =0xd00dfeed
#endif
cmp lr, r1
bne dtb_check_done @ not found
Then inside CONFIG_ARM_ATAG_DTB_COMPAT:
/* Get the initial DTB size */
ldr r5, [r6, #4]
#ifndef __ARMEB__
/* convert to little endian */
eor r1, r5, r5, ror #16
bic r1, r1, #0x00ff0000
mov r5, r5, ror #8
eor r5, r5, r1, lsr #8
#endif
Then at the end after deciding to use the appended
device tree we get the DTB size *again* and moves
the sp beyond the DTB permanently as we do not
want to damage the DTB of course.
To me it looks like the DTB size gets added to sp
a second time? First it is bumped by your code,
then when the appended DTB is detected and
augmented further down, it gets bumped again
for no reason here:
/* relocate some pointers past the appended dtb */
add r6, r6, r5
add r10, r10, r5
add sp, sp, r5
dtb_check_done:
I don't know if I'm right, I may be confused...
It would be better if we could avoid copy/paste and
merge the code in here so we first check if there is a
DTB, else there is not much to do, and if there is, we
get the size, move the sp and do both operations:
1. Check for ATAGs and augment the DTB
2. Check for memory size in the DTB
This way the ATAG-augmented DTB can be used
for checking for the memory start.
I understand that you need the physical address
before turning on the caches, so what I am thinking
is if it is possible to move the check for appended
DTB and ATAG augmentation business up before
the cache enablement in a separate patch and then
modify it from there. Then you could potentially
merge these two things.
Maybe there is something conceptually wrong with
this that I have overlooked... :/
Yours,
Linus Walleij
