[U-Boot] [PATCH][for 1.3.5] Fix handling of mem reserves for ramdisk
Hello Kumar,
When we call fdt_chosen in bootm we get a dummy mem reserve added for the ramdisk location before its relocated. We need to delete that mem reserve before we call fdt_initrd() for the final fixup.
Signed-off-by: Kumar Gala galak@kernel.crashing.org
lib_ppc/bootm.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 348421f..c801021 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -180,8 +180,22 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
#if defined(CONFIG_OF_LIBFDT) /* fixup the initrd now that we know where it should be */
- if ((of_flat_tree) && (initrd_start && initrd_end))
- if ((of_flat_tree) && (initrd_start && initrd_end)) {
uint64_t addr, size;int total = fdt_num_mem_rsv(of_flat_tree);int j;/* The call to fdt_chosen created a dummy mem rsv, delete it */for (j = 0; j < total; j++) {fdt_get_mem_rsv(of_flat_tree, j, &addr, &size);if (addr == images->rd_start) {fdt_del_mem_rsv(of_flat_tree, j);break;}}- fdt_initrd(of_flat_tree, initrd_start, initrd_end, 1);
- }
#endif debug ("## Transferring control to Linux (at address %08lx) ...\n", (ulong)kernel); -- 1.5.5.1
Hmm... OK, now the code will do the same as before, but is this optimal?
Why must we do a
a) dummy mem reservation b) search for it c) delete it d) make the right mem reservation?
Think the steps a) - c) are useless ...
Please have a look at the following patch, it deletes the steps a) - c) and fixes the fdt chosen command ...
[PATCH] powerpc: Fix bootm to boot up again with a Ramdisk.
Patch http://git.denx.de/?p=u-boot.git;a=commitdiff;h=2a1a2cb6e2b87ee550e6f27b647d...
didnt remove the dummy mem reservation which was set up in fdt_chosen, and this stopped Linux from booting with a Ramdisk. This patch fixes this, by deleting the useless dummy mem reservation.
Signed-off-by: Heiko Schocher hs@denx.de --- common/cmd_fdt.c | 3 ++- common/fdt_support.c | 4 +--- include/fdt_support.h | 2 +- lib_ppc/bootm.c | 5 +++-- 4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c index 0593bad..288a5c4 100644 --- a/common/cmd_fdt.c +++ b/common/cmd_fdt.c @@ -450,7 +450,8 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) initrd_end = simple_strtoul(argv[3], NULL, 16); }
- fdt_chosen(working_fdt, initrd_start, initrd_end, 1); + fdt_chosen(working_fdt, 1); + fdt_initrd(working_fdt, initrd_start, initrd_end, 1); } /* resize the fdt */ else if (strncmp(argv[1], "re", 2) == 0) { diff --git a/common/fdt_support.c b/common/fdt_support.c index a7773ab..8ceeb0f 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -165,7 +165,7 @@ int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end, int force) return 0; }
-int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) +int fdt_chosen(void *fdt, int force) { int nodeoffset; int err; @@ -215,8 +215,6 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) } }
- fdt_initrd(fdt, initrd_start, initrd_end, force); - #ifdef CONFIG_OF_STDOUT_VIA_ALIAS path = fdt_getprop(fdt, nodeoffset, "linux,stdout-path", NULL); if ((path == NULL) || force) diff --git a/include/fdt_support.h b/include/fdt_support.h index 424c3c6..ceaadc2 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -28,7 +28,7 @@
#include <fdt.h>
-int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force); +int fdt_chosen(void *fdt, int force); int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end, int force); void do_fixup_by_path(void *fdt, const char *path, const char *prop, const void *val, int len, int create); diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 348421f..d581493 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -145,8 +145,9 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) * if the user wants it (the logic is in the subroutines). */ if (of_size) { - /* pass in dummy initrd info, we'll fix up later */ - if (fdt_chosen(of_flat_tree, images->rd_start, images->rd_end, 0) < 0) { + /* we dont have to pass anymore the dummy initrd info! + we'll add this later, immediately with the right values. */ + if (fdt_chosen(of_flat_tree, 0) < 0) { puts ("ERROR: "); puts ("/chosen node create failed"); puts (" - must RESET the board to recover.\n");
Hmm... OK, now the code will do the same as before, but is this optimal?
Why must we do a
a) dummy mem reservation b) search for it c) delete it d) make the right mem reservation?
Think the steps a) - c) are useless ...
Please have a look at the following patch, it deletes the steps a) - c) and fixes the fdt chosen command ...
[PATCH] powerpc: Fix bootm to boot up again with a Ramdisk.
Patch http://git.denx.de/?p=u-boot.git;a=commitdiff;h=2a1a2cb6e2b87ee550e6f27b647d...
didnt remove the dummy mem reservation which was set up in fdt_chosen, and this stopped Linux from booting with a Ramdisk. This patch fixes this, by deleting the useless dummy mem reservation.
Signed-off-by: Heiko Schocher hs@denx.de
common/cmd_fdt.c | 3 ++- common/fdt_support.c | 4 +--- include/fdt_support.h | 2 +- lib_ppc/bootm.c | 5 +++-- 4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c index 0593bad..288a5c4 100644 --- a/common/cmd_fdt.c +++ b/common/cmd_fdt.c @@ -450,7 +450,8 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) initrd_end = simple_strtoul(argv[3], NULL, 16); }
fdt_chosen(working_fdt, initrd_start, initrd_end, 1);
fdt_chosen(working_fdt, 1);fdt_initrd(working_fdt, initrd_start, initrd_end, 1);
You are removing functionality, if you want to do this add a command 'fdt initrd' that sets the initrd props and the mem reserve information.
} /* resize the fdt */ else if (strncmp(argv[1], "re", 2) == 0) { diff --git a/common/fdt_support.c b/common/fdt_support.c index a7773ab..8ceeb0f 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -165,7 +165,7 @@ int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end, int force) return 0; }
-int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) +int fdt_chosen(void *fdt, int force) { int nodeoffset; int err; @@ -215,8 +215,6 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) } }
- fdt_initrd(fdt, initrd_start, initrd_end, force);
#ifdef CONFIG_OF_STDOUT_VIA_ALIAS path = fdt_getprop(fdt, nodeoffset, "linux,stdout-path", NULL); if ((path == NULL) || force) diff --git a/include/fdt_support.h b/include/fdt_support.h index 424c3c6..ceaadc2 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -28,7 +28,7 @@
#include <fdt.h>
-int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force); +int fdt_chosen(void *fdt, int force); int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end, int force); void do_fixup_by_path(void *fdt, const char *path, const char *prop, const void *val, int len, int create); diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 348421f..d581493 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -145,8 +145,9 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) * if the user wants it (the logic is in the subroutines). */ if (of_size) {
/* pass in dummy initrd info, we'll fix up later */if (fdt_chosen(of_flat_tree, images->rd_start, images->rd_end, 0)< 0) {
/* we dont have to pass anymore the dummy initrd info!we'll add this later, immediately with the rightvalues. */
if (fdt_chosen(of_flat_tree, 0) < 0) { puts ("ERROR: "); puts ("/chosen node create failed"); puts (" - must RESET the board to recover.\n");
The reason we had the code before was to try and make sure the size of the fdt was as close to its final size as possible before we dealt with the ramdisk relocation (boot_ramdisk_high()) that included the properties and the memreserve in the fdt. Step's a)-c) are there to make sure the size is correct.
- k
Dear Kumar Gala,
In message 6E892604-9B2A-4338-8DF7-C58688FC5A94@kernel.crashing.org you wrote:
The reason we had the code before was to try and make sure the size of the fdt was as close to its final size as possible before we dealt with the ramdisk relocation (boot_ramdisk_high()) that included the properties and the memreserve in the fdt. Step's a)-c) are there to make sure the size is correct.
Well, but the size needed for this should be a constant, so it should be sufficioent to determine it once and then use a hard coded constant; eventually add some padding.
Doing this in runtime is a waste of resources (flash memory, RAM, and time - not to mention human time for maintaining the code).
Best regards,
Wolfgang Denk
On Sep 8, 2008, at 9:10 AM, Wolfgang Denk wrote:
Dear Kumar Gala,
In message <6E892604-9B2A-4338-8DF7- C58688FC5A94@kernel.crashing.org> you wrote:
The reason we had the code before was to try and make sure the size of the fdt was as close to its final size as possible before we dealt with the ramdisk relocation (boot_ramdisk_high()) that included the properties and the memreserve in the fdt. Step's a)-c) are there to make sure the size is correct.
Well, but the size needed for this should be a constant, so it should be sufficioent to determine it once and then use a hard coded constant; eventually add some padding.
Doing this in runtime is a waste of resources (flash memory, RAM, and time - not to mention human time for maintaining the code).
I'm ok with determining the size bump and doing it that way.
- k
Hello Kumar,
Kumar Gala wrote:
On Sep 8, 2008, at 9:10 AM, Wolfgang Denk wrote:
Dear Kumar Gala,
In message 6E892604-9B2A-4338-8DF7-C58688FC5A94@kernel.crashing.org you wrote:
The reason we had the code before was to try and make sure the size of the fdt was as close to its final size as possible before we dealt with the ramdisk relocation (boot_ramdisk_high()) that included the properties and the memreserve in the fdt. Step's a)-c) are there to make sure the size is correct.
Well, but the size needed for this should be a constant, so it should be sufficioent to determine it once and then use a hard coded constant; eventually add some padding.
Doing this in runtime is a waste of resources (flash memory, RAM, and time - not to mention human time for maintaining the code).
I'm ok with determining the size bump and doing it that way.
[PATCH] powerpc: Fix bootm to boot up again with a Ramdisk
Patch http://git.denx.de/?p=u-boot.git;a=commitdiff;h=2a1a2cb6e2b87ee550e6f27b647d...
didnt remove the dummy mem reservation in fdt_chosen, and this stopped Linux from booting with a Ramdisk. This patch fixes this, by deleting the useless dummy mem reservation.
When booting with a Ramdisk, a fix offset FDT_RAMDISK_OVERHEAD is now added to of_size, so we dont need anymore a dummy mem reservation.
Signed-off-by: Heiko Schocher hs@denx.de --- common/cmd_fdt.c | 3 ++- common/fdt_support.c | 4 +--- include/fdt.h | 2 ++ include/fdt_support.h | 2 +- lib_ppc/bootm.c | 8 ++++++-- 5 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c index 0593bad..288a5c4 100644 --- a/common/cmd_fdt.c +++ b/common/cmd_fdt.c @@ -450,7 +450,8 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) initrd_end = simple_strtoul(argv[3], NULL, 16); }
- fdt_chosen(working_fdt, initrd_start, initrd_end, 1); + fdt_chosen(working_fdt, 1); + fdt_initrd(working_fdt, initrd_start, initrd_end, 1); } /* resize the fdt */ else if (strncmp(argv[1], "re", 2) == 0) { diff --git a/common/fdt_support.c b/common/fdt_support.c index a7773ab..8ceeb0f 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -165,7 +165,7 @@ int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end, int force) return 0; }
-int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) +int fdt_chosen(void *fdt, int force) { int nodeoffset; int err; @@ -215,8 +215,6 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) } }
- fdt_initrd(fdt, initrd_start, initrd_end, force); - #ifdef CONFIG_OF_STDOUT_VIA_ALIAS path = fdt_getprop(fdt, nodeoffset, "linux,stdout-path", NULL); if ((path == NULL) || force) diff --git a/include/fdt.h b/include/fdt.h index 48ccfd9..d09efe9 100644 --- a/include/fdt.h +++ b/include/fdt.h @@ -57,4 +57,6 @@ struct fdt_property { #define FDT_V16_SIZE FDT_V3_SIZE #define FDT_V17_SIZE (FDT_V16_SIZE + sizeof(uint32_t))
+/* adding a ramdisk needs 0x44 bytes in version 2008-10 */ +#define FDT_RAMDISK_OVERHEAD 0x80 #endif /* _FDT_H */ diff --git a/include/fdt_support.h b/include/fdt_support.h index 424c3c6..ceaadc2 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -28,7 +28,7 @@
#include <fdt.h>
-int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force); +int fdt_chosen(void *fdt, int force); int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end, int force); void do_fixup_by_path(void *fdt, const char *path, const char *prop, const void *val, int len, int create); diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 348421f..d428fa1 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -145,8 +145,9 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) * if the user wants it (the logic is in the subroutines). */ if (of_size) { - /* pass in dummy initrd info, we'll fix up later */ - if (fdt_chosen(of_flat_tree, images->rd_start, images->rd_end, 0) < 0) { + /* we dont have to pass anymore the dummy initrd info! + we'll add this later, immediately with the right values. */ + if (fdt_chosen(of_flat_tree, 0) < 0) { puts ("ERROR: "); puts ("/chosen node create failed"); puts (" - must RESET the board to recover.\n"); @@ -169,6 +170,9 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) goto error; of_size = ret;
+ /* adding a ramdisk needs 0x44 bytes in version 2008-10 */ + if ((of_flat_tree) && (initrd_start && initrd_end)) + of_size += FDT_RAMDISK_OVERHEAD; /* Create a new LMB reservation */ lmb_reserve(lmb, (ulong)of_flat_tree, of_size); }
Dear Heiko Schocher,
In message 48C7830B.6010205@denx.de you wrote:
[PATCH] powerpc: Fix bootm to boot up again with a Ramdisk
Please omit this rom the commit message - it just duplicates the Subject line.
Also, it's probably not "[for 1.3.5]" (which will never see the light of this virtuality) but "for 2008.10".
Patch http://git.denx.de/?p=u-boot.git;a=commitdiff;h=2a1a2cb6e2b87ee550e6f27b647d...
didnt remove the dummy mem reservation in fdt_chosen, and this stopped Linux from booting with a Ramdisk. This patch fixes this, by deleting the useless dummy mem reservation.
When booting with a Ramdisk, a fix offset FDT_RAMDISK_OVERHEAD is now added to of_size, so we dont need anymore a dummy mem reservation.
...
+/* adding a ramdisk needs 0x44 bytes in version 2008-10 */
2008.10, not 2008-10.
--- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -145,8 +145,9 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) * if the user wants it (the logic is in the subroutines). */ if (of_size) {
/* pass in dummy initrd info, we'll fix up later */if (fdt_chosen(of_flat_tree, images->rd_start, images->rd_end, 0) < 0) {
/* we dont have to pass anymore the dummy initrd info!we'll add this later, immediately with the right values. */
Incorrect multiline format.
But the information is already included with the #define above and the commit message. I suggest to delete the comment completely.
@@ -169,6 +170,9 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) goto error; of_size = ret;
/* adding a ramdisk needs 0x44 bytes in version 2008-10 */
Ditto - I suggest to delete the redudant information.
if ((of_flat_tree) && (initrd_start && initrd_end))of_size += FDT_RAMDISK_OVERHEAD;
Except for this nitpicking:
Acked-by: Wolfgang Denk wd@denx.de
Kumar, what do you think?
Best regards,
Wolfgang Denk
Hello Kumar,
Kumar Gala wrote: [...]
Please have a look at the following patch, it deletes the steps a) - c) and fixes the fdt chosen command ...
[...]
diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c index 0593bad..288a5c4 100644 --- a/common/cmd_fdt.c +++ b/common/cmd_fdt.c @@ -450,7 +450,8 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[]) initrd_end = simple_strtoul(argv[3], NULL, 16); }
fdt_chosen(working_fdt, initrd_start, initrd_end, 1);
fdt_chosen(working_fdt, 1);fdt_initrd(working_fdt, initrd_start, initrd_end, 1);You are removing functionality, if you want to do this add a command 'fdt initrd' that sets the initrd props and the mem reserve information.
Hmm... really? Your patch
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=2a1a2cb6e2b87ee550e6f27b647d...
moved functionality from fdt_chosen in the fdt_initrd function and called fdt_initrd() in fdt_chosen ().
My patch removes now this fdt_initrd() call from fdt_chosen(), and so I call it here to fix the fdt_chosen command ... maybe I miss something?
} /* resize the fdt */ else if (strncmp(argv[1], "re", 2) == 0) {diff --git a/common/fdt_support.c b/common/fdt_support.c index a7773ab..8ceeb0f 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -165,7 +165,7 @@ int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end, int force) return 0; }
-int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) +int fdt_chosen(void *fdt, int force) { int nodeoffset; int err; @@ -215,8 +215,6 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) } }
- fdt_initrd(fdt, initrd_start, initrd_end, force);
#ifdef CONFIG_OF_STDOUT_VIA_ALIAS
[...]
The reason we had the code before was to try and make sure the size of the fdt was as close to its final size as possible before we dealt with the ramdisk relocation (boot_ramdisk_high()) that included the properties and the memreserve in the fdt. Step's a)-c) are there to make sure the size is correct.
Hmmm... and thats the reason why we risk to forgot a memreservation? (Keep in mind, I think actually it is not possible to boot Linux with actual u-boot from a Ramdisk in Flash on a powerpc!)
Is this really so essential to know the final fdt size?
While writing this, and if the final fdt size is so important, I wanted to propose a fix Ramdisk address we can later find and delete, but then I saw your patch
http://lists.denx.de/pipermail/u-boot/2008-September/040054.html
does this principally! So this patch is okay for me.
bye Heiko
participants (3)
-
Heiko Schocher -
Kumar Gala -
Wolfgang Denk