[PATCH] imx8m{m,n}_venice: update env memory layout

[PATCH] imx8m{m,n}_venice: update env memory layout

[Tim Harvey]

Update distro config env memory layout:
 - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
   images, kernel_comp_addr_r)
 - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
 - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
 - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 configs/imx8mm_venice_defconfig | 2 +-
 configs/imx8mn_venice_defconfig | 2 +-
 include/configs/imx8mm_venice.h | 9 +++++----
 include/configs/imx8mn_venice.h | 9 +++++----
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/configs/imx8mm_venice_defconfig b/configs/imx8mm_venice_defconfig
index ad4e40d90d65..3ea3527cb0c0 100644
--- a/configs/imx8mm_venice_defconfig
+++ b/configs/imx8mm_venice_defconfig
@@ -21,7 +21,7 @@ CONFIG_SPL=y
 CONFIG_ENV_OFFSET_REDUND=0xff8000
 CONFIG_LTO=y
 CONFIG_DISTRO_DEFAULTS=y
-CONFIG_SYS_LOAD_ADDR=0x40480000
+CONFIG_SYS_LOAD_ADDR=0x48200000
 CONFIG_FIT=y
 CONFIG_FIT_EXTERNAL_OFFSET=0x3000
 CONFIG_SPL_LOAD_FIT=y
diff --git a/configs/imx8mn_venice_defconfig b/configs/imx8mn_venice_defconfig
index b195f222dc19..089e03d84649 100644
--- a/configs/imx8mn_venice_defconfig
+++ b/configs/imx8mn_venice_defconfig
@@ -21,7 +21,7 @@ CONFIG_ENV_OFFSET_REDUND=0xff8000
 CONFIG_SPL_IMX_ROMAPI_LOADADDR=0x48000000
 CONFIG_LTO=y
 CONFIG_DISTRO_DEFAULTS=y
-CONFIG_SYS_LOAD_ADDR=0x40480000
+CONFIG_SYS_LOAD_ADDR=0x48200000
 CONFIG_FIT=y
 CONFIG_FIT_EXTERNAL_OFFSET=0x3000
 CONFIG_SPL_LOAD_FIT=y
diff --git a/include/configs/imx8mm_venice.h b/include/configs/imx8mm_venice.h
index 455a8d0187d8..cbb138229f67 100644
--- a/include/configs/imx8mm_venice.h
+++ b/include/configs/imx8mm_venice.h
@@ -29,10 +29,11 @@
 #endif
 
 #define MEM_LAYOUT_ENV_SETTINGS \
-	"fdt_addr_r=0x44000000\0" \
-	"kernel_addr_r=0x42000000\0" \
-	"ramdisk_addr_r=0x46400000\0" \
-	"scriptaddr=0x46000000\0"
+	"kernel_addr_r=" __stringify(CONFIG_SYS_LOAD_ADDR) "\0" \
+	"fdt_addr_r=0x50200000\0" \
+	"scriptaddr=0x50280000\0" \
+	"ramdisk_addr_r=0x50300000\0" \
+	"kernel_comp_addr_r=0x40200000\0"
 
 /* Enable Distro Boot */
 #ifndef CONFIG_SPL_BUILD
diff --git a/include/configs/imx8mn_venice.h b/include/configs/imx8mn_venice.h
index e7bfcd70af2c..4c95893be4fc 100644
--- a/include/configs/imx8mn_venice.h
+++ b/include/configs/imx8mn_venice.h
@@ -26,10 +26,11 @@
 #endif
 
 #define MEM_LAYOUT_ENV_SETTINGS \
-	"fdt_addr_r=0x44000000\0" \
-	"kernel_addr_r=0x42000000\0" \
-	"ramdisk_addr_r=0x46400000\0" \
-	"scriptaddr=0x46000000\0"
+	"kernel_addr_r=" __stringify(CONFIG_SYS_LOAD_ADDR) "\0" \
+	"fdt_addr_r=0x50200000\0" \
+	"scriptaddr=0x50280000\0" \
+	"ramdisk_addr_r=0x50300000\0" \
+	"kernel_comp_addr_r=0x40200000\0"
 
 /* Enable Distro Boot */
 #ifndef CONFIG_SPL_BUILD
-- 
2.17.1

Re: [PATCH] imx8m{m,n}_venice: update env memory layout

[Fabio Estevam]

Hi Tim,

On Fri, Feb 18, 2022 at 8:20 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> Update distro config env memory layout:
>  - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
>    images, kernel_comp_addr_r)
>  - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
>  - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
>  - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

What about following the suggestion from Heiko at:
https://patchwork.ozlabs.org/project/uboot/patch/20220216122514.1659879-2-heiko.thiery@gmail.com/

Re: [PATCH] imx8m{m,n}_venice: update env memory layout

[Tim Harvey]

On Sat, Feb 26, 2022 at 5:15 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> On Fri, Feb 18, 2022 at 8:20 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > Update distro config env memory layout:
> >  - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
> >    images, kernel_comp_addr_r)
> >  - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
> >  - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
> >  - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>
> What about following the suggestion from Heiko at:
> https://patchwork.ozlabs.org/project/uboot/patch/20220216122514.1659879-2-heiko.thiery@gmail.com/

Fabio,

I don't understand why include/configs/ti_armv7_common.h is always
recommended when it comes to address maps. The comment there doesn't
read that well and there are several addresses that I believe are
outdated or uncommon (fdtaddr,rdaddr are outdated and it seems
loadaddr could be used for dtboaddr)

In my opinion what is relevant when choosing an addressing scheme is
how much space you want to allow for kernel, fdt, scripts, ramdisk.

ti_armv7_common.h allows for
kernel/loadaddr:96M (too small in my opinion on modern boards)
fdt:512M (makes sense)
script:32M (excessive, and I don't understand why this is below
loadaddr vs stacked on top of loadaddr like fdt)
ramdisk: depends on mem size as its the highest addr (makes sense)

For my more modern boards with typically 1-4GiB of dram, I wanted to
allow for more than 96M for a kernel (kernel+ramdisk, fit image etc)
as I commonly exceed 96M when using a buildroot kernel+rootfs that has
something like gstreamer support for imx6/imx8 which is why I came up
with a different scheme which I document well in the commit log as
 - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
   images, kernel_comp_addr_r)
 - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
 - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
 - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script

I must admit I'm not clear what the best choice of
CONFIG_SYS_LOAD_ADDR is for IMX8MM which is what all these addresses
should stack on top of.

I also will admit that defining them as hard coded addresses makes it
difficult to see the sizes of each. It would be better if we had a way
to define them referencing a size and what they stack on top of but
I'm not clear how to do this with the C pre-processor.

Best Regards,

Tim

Re: [PATCH] imx8m{m,n}_venice: update env memory layout

[Fabio Estevam]

Hi Tim,

[Adding Tom on Cc]

On Sat, Feb 26, 2022 at 6:37 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Sat, Feb 26, 2022 at 5:15 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Tim,
> >
> > On Fri, Feb 18, 2022 at 8:20 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > >
> > > Update distro config env memory layout:
> > >  - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
> > >    images, kernel_comp_addr_r)
> > >  - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
> > >  - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
> > >  - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script
> > >
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >
> > What about following the suggestion from Heiko at:
> > https://patchwork.ozlabs.org/project/uboot/patch/20220216122514.1659879-2-heiko.thiery@gmail.com/
>
> Fabio,
>
> I don't understand why include/configs/ti_armv7_common.h is always
> recommended when it comes to address maps. The comment there doesn't
> read that well and there are several addresses that I believe are
> outdated or uncommon (fdtaddr,rdaddr are outdated and it seems
> loadaddr could be used for dtboaddr)
>
> In my opinion what is relevant when choosing an addressing scheme is
> how much space you want to allow for kernel, fdt, scripts, ramdisk.
>
> ti_armv7_common.h allows for
> kernel/loadaddr:96M (too small in my opinion on modern boards)
> fdt:512M (makes sense)
> script:32M (excessive, and I don't understand why this is below
> loadaddr vs stacked on top of loadaddr like fdt)
> ramdisk: depends on mem size as its the highest addr (makes sense)
>
> For my more modern boards with typically 1-4GiB of dram, I wanted to
> allow for more than 96M for a kernel (kernel+ramdisk, fit image etc)
> as I commonly exceed 96M when using a buildroot kernel+rootfs that has
> something like gstreamer support for imx6/imx8 which is why I came up
> with a different scheme which I document well in the commit log as
>  - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
>    images, kernel_comp_addr_r)
>  - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
>  - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
>  - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script

You bring good points and maybe we could use
include/configs/imx8mm_venice.h as a good standard to follow :-)

Reviewed-by: Fabio Estevam <festevam@denx.de>

Re: [PATCH] imx8m{m,n}_venice: update env memory layout

[Tim Harvey]

On Sun, Feb 27, 2022 at 4:22 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> [Adding Tom on Cc]
>
> On Sat, Feb 26, 2022 at 6:37 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Sat, Feb 26, 2022 at 5:15 AM Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > > Hi Tim,
> > >
> > > On Fri, Feb 18, 2022 at 8:20 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > > >
> > > > Update distro config env memory layout:
> > > >  - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
> > > >    images, kernel_comp_addr_r)
> > > >  - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
> > > >  - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
> > > >  - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script
> > > >
> > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > >
> > > What about following the suggestion from Heiko at:
> > > https://patchwork.ozlabs.org/project/uboot/patch/20220216122514.1659879-2-heiko.thiery@gmail.com/
> >
> > Fabio,
> >
> > I don't understand why include/configs/ti_armv7_common.h is always
> > recommended when it comes to address maps. The comment there doesn't
> > read that well and there are several addresses that I believe are
> > outdated or uncommon (fdtaddr,rdaddr are outdated and it seems
> > loadaddr could be used for dtboaddr)
> >
> > In my opinion what is relevant when choosing an addressing scheme is
> > how much space you want to allow for kernel, fdt, scripts, ramdisk.
> >
> > ti_armv7_common.h allows for
> > kernel/loadaddr:96M (too small in my opinion on modern boards)
> > fdt:512M (makes sense)
> > script:32M (excessive, and I don't understand why this is below
> > loadaddr vs stacked on top of loadaddr like fdt)
> > ramdisk: depends on mem size as its the highest addr (makes sense)
> >
> > For my more modern boards with typically 1-4GiB of dram, I wanted to
> > allow for more than 96M for a kernel (kernel+ramdisk, fit image etc)
> > as I commonly exceed 96M when using a buildroot kernel+rootfs that has
> > something like gstreamer support for imx6/imx8 which is why I came up
> > with a different scheme which I document well in the commit log as
> >  - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
> >    images, kernel_comp_addr_r)
> >  - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
> >  - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
> >  - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script
>
> You bring good points and maybe we could use
> include/configs/imx8mm_venice.h as a good standard to follow :-)
>
> Reviewed-by: Fabio Estevam <festevam@denx.de>

Tom,

DId you have any feedback on this conversation about memory addresses?
I'm also wondering if anyone knows how we can define them to make it a
bit clearer about their sizes (some sort of macro foo that allows us
to defined them as sizes on top of a previous address).

Best regards,

Tim

Re: [PATCH] imx8m{m,n}_venice: update env memory layout

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3752 bytes --]

On Thu, Mar 10, 2022 at 07:59:30AM -0800, Tim Harvey wrote:
> On Sun, Feb 27, 2022 at 4:22 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Tim,
> >
> > [Adding Tom on Cc]
> >
> > On Sat, Feb 26, 2022 at 6:37 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > >
> > > On Sat, Feb 26, 2022 at 5:15 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > >
> > > > Hi Tim,
> > > >
> > > > On Fri, Feb 18, 2022 at 8:20 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > > > >
> > > > > Update distro config env memory layout:
> > > > >  - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
> > > > >    images, kernel_comp_addr_r)
> > > > >  - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
> > > > >  - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
> > > > >  - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script
> > > > >
> > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > > >
> > > > What about following the suggestion from Heiko at:
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20220216122514.1659879-2-heiko.thiery@gmail.com/
> > >
> > > Fabio,
> > >
> > > I don't understand why include/configs/ti_armv7_common.h is always
> > > recommended when it comes to address maps. The comment there doesn't
> > > read that well and there are several addresses that I believe are
> > > outdated or uncommon (fdtaddr,rdaddr are outdated and it seems
> > > loadaddr could be used for dtboaddr)
> > >
> > > In my opinion what is relevant when choosing an addressing scheme is
> > > how much space you want to allow for kernel, fdt, scripts, ramdisk.
> > >
> > > ti_armv7_common.h allows for
> > > kernel/loadaddr:96M (too small in my opinion on modern boards)
> > > fdt:512M (makes sense)
> > > script:32M (excessive, and I don't understand why this is below
> > > loadaddr vs stacked on top of loadaddr like fdt)
> > > ramdisk: depends on mem size as its the highest addr (makes sense)
> > >
> > > For my more modern boards with typically 1-4GiB of dram, I wanted to
> > > allow for more than 96M for a kernel (kernel+ramdisk, fit image etc)
> > > as I commonly exceed 96M when using a buildroot kernel+rootfs that has
> > > something like gstreamer support for imx6/imx8 which is why I came up
> > > with a different scheme which I document well in the commit log as
> > >  - loadaddr=0x48200000 allows for 130MB area for uncompressing (ie FIT
> > >    images, kernel_comp_addr_r)
> > >  - fdt_addr_r = loadaddr + 128MB - allows for 128MB kernel
> > >  - scriptaddr = fdt_addr_r + 512KB - allows for 512KB fdt
> > >  - ramdisk_addr_r = scriptaddr + 512KB - allows for 512KB script
> >
> > You bring good points and maybe we could use
> > include/configs/imx8mm_venice.h as a good standard to follow :-)
> >
> > Reviewed-by: Fabio Estevam <festevam@denx.de>
> 
> Tom,
> 
> DId you have any feedback on this conversation about memory addresses?
> I'm also wondering if anyone knows how we can define them to make it a
> bit clearer about their sizes (some sort of macro foo that allows us
> to defined them as sizes on top of a previous address).

Alright, so, yeah, at this point it's true that ti_armv7_common.h could
use some re-evaluation since it's been a while.  What's probably the
best path forward is something under doc/develop/ that explains what the
important environment variables are, general max sizes and links to
where those constraints come from and then an example or two.
Especially since 32 and 64bit ARM have some different constraints (I
want to say that the 96MB kernel size was a practical limitation, but
would need to re-read the kernel docs and maybe check some notes again).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]