| Message ID | 20220218232000.4790-1-tharvey@gateworks.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | imx8m{m,n}_venice: update env memory layout | expand |
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/
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
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>
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
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).
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
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(-)