[U-Boot-Users] [PATCH v2] [new uImage] ppc: Re-order ramdisk/fdt handling sequence
Doing the fdt before the ramdisk allows us to grow the fdt w/o concern however it does mean we have to go in and fixup the initrd info since we don't know where it will be.
Signed-off-by: Kumar Gala galak@kernel.crashing.org ---
Fixed up the error handling. The old code tried returning an error and should have called do_reset().
lib_ppc/bootm.c | 40 +++++++++++++++++++++++++++++++++++----- 1 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 5158ccc..09e349e 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -71,7 +71,7 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, bd_t *kbd; void (*kernel)(bd_t *, ulong, ulong, ulong, ulong);
- int has_of = 0; + int ret, has_of = 0;
#if defined(CONFIG_OF_LIBFDT) char *of_flat_tree; @@ -121,9 +121,6 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
rd_len = rd_data_end - rd_data_start;
- alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len, - sp_limit, get_sp (), &initrd_start, &initrd_end); - #if defined(CONFIG_OF_LIBFDT) /* find flattened device tree */ alloc_current = get_fdt (alloc_current, @@ -134,7 +131,8 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, * if the user wants it (the logic is in the subroutines). */ if (of_flat_tree) { - if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) { + /* pass in dummy initrd info, we'll fix up later */ + if (fdt_chosen(of_flat_tree, rd_data_start, rd_data_end, 0) < 0) { fdt_error ("/chosen node create failed"); do_reset (cmdtp, flag, argc, argv); } @@ -157,6 +155,38 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, } #endif /* CONFIG_OF_LIBFDT */
+ alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len, + sp_limit, get_sp (), &initrd_start, &initrd_end); + +#if defined(CONFIG_OF_LIBFDT) + /* fixup the initrd now that we know where it should be */ + if ((of_flat_tree) && (initrd_start && initrd_end)) { + uint64_t addr, size; + int total = fdt_num_mem_rsv(of_flat_tree); + int j; + + /* Look for the dummy entry and delete it */ + for (j = 0; j < total; j++) { + fdt_get_mem_rsv(of_flat_tree, j, &addr, &size); + if (addr == rd_data_start) { + fdt_del_mem_rsv(of_flat_tree, j); + break; + } + } + + ret = fdt_add_mem_rsv(of_flat_tree, initrd_start, + initrd_end - initrd_start + 1); + if (ret < 0) { + printf("fdt_chosen: %s\n", fdt_strerror(ret)); + do_reset (cmdtp, flag, argc, argv); + } + + do_fixup_by_path_u32(of_flat_tree, "/chosen", + "linux,initrd-start", initrd_start, 0); + do_fixup_by_path_u32(of_flat_tree, "/chosen", + "linux,initrd-end", initrd_end, 0); + } +#endif debug ("## Transferring control to Linux (at address %08lx) ...\n", (ulong)kernel);
Kumar Gala wrote:
Doing the fdt before the ramdisk allows us to grow the fdt w/o concern however it does mean we have to go in and fixup the initrd info since we don't know where it will be.
Signed-off-by: Kumar Gala galak@kernel.crashing.org
Hi Kumar,
Fixed up the error handling. The old code tried returning an error and should have called do_reset().
lib_ppc/bootm.c | 40 +++++++++++++++++++++++++++++++++++----- 1 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 5158ccc..09e349e 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -71,7 +71,7 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, bd_t *kbd; void (*kernel)(bd_t *, ulong, ulong, ulong, ulong);
- int has_of = 0;
- int ret, has_of = 0;
I would strongly prefer to see "ret" declared on a separate line. I'm not a fan of comma declared variables and, when one is initialized and the other isn't, it makes my brain play tricks on me (thinking ret is initialized to 0 too, which it isn't).
#if defined(CONFIG_OF_LIBFDT) char *of_flat_tree; @@ -121,9 +121,6 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag,
rd_len = rd_data_end - rd_data_start;
- alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
sp_limit, get_sp (), &initrd_start, &initrd_end);#if defined(CONFIG_OF_LIBFDT) /* find flattened device tree */ alloc_current = get_fdt (alloc_current, @@ -134,7 +131,8 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, * if the user wants it (the logic is in the subroutines). */ if (of_flat_tree) {
if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
/* pass in dummy initrd info, we'll fix up later */if (fdt_chosen(of_flat_tree, rd_data_start, rd_data_end, 0) < 0) {
If you pass in 0,0 for rd_data_start, rd_data_end it will not create the ramdisk entry...
fdt_error ("/chosen node create failed"); do_reset (cmdtp, flag, argc, argv); }@@ -157,6 +155,38 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, } #endif /* CONFIG_OF_LIBFDT */
- alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
sp_limit, get_sp (), &initrd_start, &initrd_end);+#if defined(CONFIG_OF_LIBFDT)
- /* fixup the initrd now that we know where it should be */
- if ((of_flat_tree) && (initrd_start && initrd_end)) {
uint64_t addr, size;int total = fdt_num_mem_rsv(of_flat_tree);int j;/* Look for the dummy entry and delete it */for (j = 0; j < total; j++) {fdt_get_mem_rsv(of_flat_tree, j, &addr, &size);if (addr == rd_data_start) {fdt_del_mem_rsv(of_flat_tree, j);break;}}
...and the above loop and delete will then be unnecessary.
ret = fdt_add_mem_rsv(of_flat_tree, initrd_start,initrd_end - initrd_start + 1);if (ret < 0) {printf("fdt_chosen: %s\n", fdt_strerror(ret));do_reset (cmdtp, flag, argc, argv);}do_fixup_by_path_u32(of_flat_tree, "/chosen","linux,initrd-start", initrd_start, 0);do_fixup_by_path_u32(of_flat_tree, "/chosen","linux,initrd-end", initrd_end, 0);- }
+#endif debug ("## Transferring control to Linux (at address %08lx) ...\n", (ulong)kernel);
...and you will have to pass in '1' for the create (last) parameter in the do_fixup_by_path_u32() calls to create them if they don't exist.
Is that better, or just a different way to do the same thing? It seems more straight-forward to me.
Best regards, gvb
On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
Kumar Gala wrote:
Doing the fdt before the ramdisk allows us to grow the fdt w/o concern however it does mean we have to go in and fixup the initrd info since we don't know where it will be. Signed-off-by: Kumar Gala galak@kernel.crashing.org
Hi Kumar,
Fixed up the error handling. The old code tried returning an error and should have called do_reset(). lib_ppc/bootm.c | 40 +++++++++++++++++++++++++++++++++++----- 1 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 5158ccc..09e349e 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -71,7 +71,7 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, bd_t *kbd; void (*kernel)(bd_t *, ulong, ulong, ulong, ulong);
- int has_of = 0;
- int ret, has_of = 0;
I would strongly prefer to see "ret" declared on a separate line. I'm not a fan of comma declared variables and, when one is initialized and the other isn't, it makes my brain play tricks on me (thinking ret is initialized to 0 too, which it isn't).
#if defined(CONFIG_OF_LIBFDT) char *of_flat_tree; @@ -121,9 +121,6 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, rd_len = rd_data_end - rd_data_start;
- alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
sp_limit, get_sp (), &initrd_start, &initrd_end);#if defined(CONFIG_OF_LIBFDT) /* find flattened device tree */ alloc_current = get_fdt (alloc_current, @@ -134,7 +131,8 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, * if the user wants it (the logic is in the subroutines). */ if (of_flat_tree) {
if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
/* pass in dummy initrd info, we'll fix up later */if (fdt_chosen(of_flat_tree, rd_data_start, rd_data_end, 0) < 0) {If you pass in 0,0 for rd_data_start, rd_data_end it will not create the ramdisk entry...
If rd_data_start, rd_data_end are 0,0 than we don't have a ramdisk, so its ok.
fdt_error ("/chosen node create failed"); do_reset (cmdtp, flag, argc, argv); }@@ -157,6 +155,38 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, } #endif /* CONFIG_OF_LIBFDT */
- alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
sp_limit, get_sp (), &initrd_start, &initrd_end);+#if defined(CONFIG_OF_LIBFDT)
- /* fixup the initrd now that we know where it should be */
- if ((of_flat_tree) && (initrd_start && initrd_end)) {
uint64_t addr, size;int total = fdt_num_mem_rsv(of_flat_tree);int j;/* Look for the dummy entry and delete it */for (j = 0; j < total; j++) {fdt_get_mem_rsv(of_flat_tree, j, &addr, &size);if (addr == rd_data_start) {fdt_del_mem_rsv(of_flat_tree, j);break;}}...and the above loop and delete will then be unnecessary.
Again, protected by initrd_start/_end being non-zero.
ret = fdt_add_mem_rsv(of_flat_tree, initrd_start,initrd_end - initrd_start + 1);if (ret < 0) {printf("fdt_chosen: %s\n", fdt_strerror(ret));do_reset (cmdtp, flag, argc, argv);}do_fixup_by_path_u32(of_flat_tree, "/chosen","linux,initrd-start", initrd_start, 0);do_fixup_by_path_u32(of_flat_tree, "/chosen","linux,initrd-end", initrd_end, 0);- }
+#endif debug ("## Transferring control to Linux (at address %08lx) ...\n", (ulong)kernel);
...and you will have to pass in '1' for the create (last) parameter in the do_fixup_by_path_u32() calls to create them if they don't exist.
They should exist by the time we are doing the fixup.
Is that better, or just a different way to do the same thing? It seems more straight-forward to me.
Its just re-ordering the way we do things.
- k
Kumar Gala wrote:
On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
Kumar Gala wrote:
Doing the fdt before the ramdisk allows us to grow the fdt w/o concern however it does mean we have to go in and fixup the initrd info since we don't know where it will be. Signed-off-by: Kumar Gala galak@kernel.crashing.org
Hi Kumar,
Fixed up the error handling. The old code tried returning an error and should have called do_reset(). lib_ppc/bootm.c | 40 +++++++++++++++++++++++++++++++++++----- 1 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 5158ccc..09e349e 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -71,7 +71,7 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, bd_t *kbd; void (*kernel)(bd_t *, ulong, ulong, ulong, ulong);
- int has_of = 0;
- int ret, has_of = 0;
I would strongly prefer to see "ret" declared on a separate line. I'm not a fan of comma declared variables and, when one is initialized and the other isn't, it makes my brain play tricks on me (thinking ret is initialized to 0 too, which it isn't).
#if defined(CONFIG_OF_LIBFDT) char *of_flat_tree; @@ -121,9 +121,6 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, rd_len = rd_data_end - rd_data_start;
- alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
sp_limit, get_sp (), &initrd_start, &initrd_end);#if defined(CONFIG_OF_LIBFDT) /* find flattened device tree */ alloc_current = get_fdt (alloc_current, @@ -134,7 +131,8 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, * if the user wants it (the logic is in the subroutines). */ if (of_flat_tree) {
if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) <
- {
/* pass in dummy initrd info, we'll fix up later */if (fdt_chosen(of_flat_tree, rd_data_start, rd_data_end, 0)< 0) {
If you pass in 0,0 for rd_data_start, rd_data_end it will not create the ramdisk entry...
If rd_data_start, rd_data_end are 0,0 than we don't have a ramdisk, so its ok.
My point is to pass in 0,0 *so that* they are not created at all, rather than passing in dummy values, creating entries that we have to delete/rewrite later.
fdt_error ("/chosen node create failed"); do_reset (cmdtp, flag, argc, argv); }@@ -157,6 +155,38 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, } #endif /* CONFIG_OF_LIBFDT */
- alloc_current = ramdisk_high (alloc_current, rd_data_start, rd_len,
sp_limit, get_sp (), &initrd_start, &initrd_end);+#if defined(CONFIG_OF_LIBFDT)
- /* fixup the initrd now that we know where it should be */
- if ((of_flat_tree) && (initrd_start && initrd_end)) {
uint64_t addr, size;int total = fdt_num_mem_rsv(of_flat_tree);int j;/* Look for the dummy entry and delete it */for (j = 0; j < total; j++) {fdt_get_mem_rsv(of_flat_tree, j, &addr, &size);if (addr == rd_data_start) {fdt_del_mem_rsv(of_flat_tree, j);break;}}...and the above loop and delete will then be unnecessary.
Again, protected by initrd_start/_end being non-zero.
My point is to pass in 0,0 *so that* they are not created at all, rather than passing in dummy values, creating entries that we have to delete/rewrite later.
ret = fdt_add_mem_rsv(of_flat_tree, initrd_start,initrd_end - initrd_start + 1);if (ret < 0) {printf("fdt_chosen: %s\n", fdt_strerror(ret));do_reset (cmdtp, flag, argc, argv);}do_fixup_by_path_u32(of_flat_tree, "/chosen","linux,initrd-start", initrd_start, 0);do_fixup_by_path_u32(of_flat_tree, "/chosen","linux,initrd-end", initrd_end, 0);- }
+#endif debug ("## Transferring control to Linux (at address %08lx) ...\n", (ulong)kernel);
...and you will have to pass in '1' for the create (last) parameter in the do_fixup_by_path_u32() calls to create them if they don't exist.
They should exist by the time we are doing the fixup.
Is that better, or just a different way to do the same thing? It seems more straight-forward to me.
Its just re-ordering the way we do things.
- k
The patch is creating dummy initrd entries in the reserved map and in /chosen, only to work hard to delete and re-create the reserved map entries and rewrite the /chosen entries.
My counter-proposal is to not bother with dummy values. Simply pass in 0,0 which will prevent the creation of the initrd entries by fdt_chosen(). By not creating dummy entries, you can simply create the proper entries once you know what the the correct values are, rather than the more complicated rsvmap search & delete + rsvmap creation + /chosen modifications.
Best regards, gvb
On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
Kumar Gala wrote:
Doing the fdt before the ramdisk allows us to grow the fdt w/o concern however it does mean we have to go in and fixup the initrd info since we don't know where it will be. Signed-off-by: Kumar Gala galak@kernel.crashing.org
Hi Kumar,
Fixed up the error handling. The old code tried returning an error and should have called do_reset(). lib_ppc/bootm.c | 40 +++++++++++++++++++++++++++++++++++----- 1 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c index 5158ccc..09e349e 100644 --- a/lib_ppc/bootm.c +++ b/lib_ppc/bootm.c @@ -71,7 +71,7 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, bd_t *kbd; void (*kernel)(bd_t *, ulong, ulong, ulong, ulong);
- int has_of = 0;
- int ret, has_of = 0;
I would strongly prefer to see "ret" declared on a separate line. I'm not a fan of comma declared variables and, when one is initialized and the other isn't, it makes my brain play tricks on me (thinking ret is initialized to 0 too, which it isn't).
#if defined(CONFIG_OF_LIBFDT) char *of_flat_tree; @@ -121,9 +121,6 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, rd_len = rd_data_end - rd_data_start;
- alloc_current = ramdisk_high (alloc_current, rd_data_start,
rd_len,
sp_limit, get_sp (), &initrd_start, &initrd_end);#if defined(CONFIG_OF_LIBFDT) /* find flattened device tree */ alloc_current = get_fdt (alloc_current, @@ -134,7 +131,8 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, * if the user wants it (the logic is in the subroutines). */ if (of_flat_tree) {
if (fdt_chosen(of_flat_tree, initrd_start, initrd_end,
- < 0) {
/* pass in dummy initrd info, we'll fix up later */if (fdt_chosen(of_flat_tree, rd_data_start, rd_data_end,
- < 0) {
If you pass in 0,0 for rd_data_start, rd_data_end it will not create the ramdisk entry...
If rd_data_start, rd_data_end are 0,0 than we don't have a ramdisk, so its ok.
My point is to pass in 0,0 *so that* they are not created at all, rather than passing in dummy values, creating entries that we have to delete/rewrite later.
fdt_error ("/chosen node create failed"); do_reset (cmdtp, flag, argc, argv); }@@ -157,6 +155,38 @@ do_bootm_linux(cmd_tbl_t *cmdtp, int flag, } #endif /* CONFIG_OF_LIBFDT */
- alloc_current = ramdisk_high (alloc_current, rd_data_start,
rd_len,
sp_limit, get_sp (), &initrd_start, &initrd_end);+#if defined(CONFIG_OF_LIBFDT)
- /* fixup the initrd now that we know where it should be */
- if ((of_flat_tree) && (initrd_start && initrd_end)) {
uint64_t addr, size;int total = fdt_num_mem_rsv(of_flat_tree);int j;/* Look for the dummy entry and delete it */for (j = 0; j < total; j++) {fdt_get_mem_rsv(of_flat_tree, j, &addr, &size);if (addr == rd_data_start) {fdt_del_mem_rsv(of_flat_tree, j);break;}}...and the above loop and delete will then be unnecessary.
Again, protected by initrd_start/_end being non-zero.
My point is to pass in 0,0 *so that* they are not created at all, rather than passing in dummy values, creating entries that we have to delete/rewrite later.
ret = fdt_add_mem_rsv(of_flat_tree, initrd_start,initrd_end - initrd_start + 1);if (ret < 0) {printf("fdt_chosen: %s\n", fdt_strerror(ret));do_reset (cmdtp, flag, argc, argv);}do_fixup_by_path_u32(of_flat_tree, "/chosen","linux,initrd-start", initrd_start, 0);do_fixup_by_path_u32(of_flat_tree, "/chosen","linux,initrd-end", initrd_end, 0);- }
+#endif debug ("## Transferring control to Linux (at address %08lx) ... \n", (ulong)kernel);
...and you will have to pass in '1' for the create (last) parameter in the do_fixup_by_path_u32() calls to create them if they don't exist.
They should exist by the time we are doing the fixup.
Is that better, or just a different way to do the same thing? It seems more straight-forward to me.
Its just re-ordering the way we do things.
- k
The patch is creating dummy initrd entries in the reserved map and in /chosen, only to work hard to delete and re-create the reserved map entries and rewrite the /chosen entries.
My counter-proposal is to not bother with dummy values. Simply pass in 0,0 which will prevent the creation of the initrd entries by fdt_chosen(). By not creating dummy entries, you can simply create the proper entries once you know what the the correct values are, rather than the more complicated rsvmap search & delete + rsvmap creation + /chosen modifications.
Ahh, the reason I wanted them created was to ensure we have enough size for them up front rather than figuring that out later. By creating them and replacing them I will not being changing the size at all.
- k
Kumar Gala wrote:
On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
Kumar Gala wrote:
[[[[snip]]]]
The patch is creating dummy initrd entries in the reserved map and in /chosen, only to work hard to delete and re-create the reserved map entries and rewrite the /chosen entries.
My counter-proposal is to not bother with dummy values. Simply pass in 0,0 which will prevent the creation of the initrd entries by fdt_chosen(). By not creating dummy entries, you can simply create the proper entries once you know what the the correct values are, rather than the more complicated rsvmap search & delete + rsvmap creation + /chosen modifications.
Ahh, the reason I wanted them created was to ensure we have enough size for them up front rather than figuring that out later. By creating them and replacing them I will not being changing the size at all.
- k
OK, I see.
Currently this isn't an issue because our blob has a fixed size that has free space inside it, so creating the rsvmap and /chosen entries eat at the internal free space and don't change the total blob size.
People are advocating dynamically increasing the blob size, which simplifies things for blob generation (don't have to guess how big to make the blob when running the dtc to create it), but that would cause problems with my counter-proposal.
Thanks, gvb
On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
Kumar Gala wrote:
[[[[snip]]]]
The patch is creating dummy initrd entries in the reserved map and in /chosen, only to work hard to delete and re-create the reserved map entries and rewrite the /chosen entries.
My counter-proposal is to not bother with dummy values. Simply pass in 0,0 which will prevent the creation of the initrd entries by fdt_chosen(). By not creating dummy entries, you can simply create the proper entries once you know what the the correct values are, rather than the more complicated rsvmap search & delete + rsvmap creation + /chosen modifications.
Ahh, the reason I wanted them created was to ensure we have enough size for them up front rather than figuring that out later. By creating them and replacing them I will not being changing the size at all.
- k
OK, I see.
Currently this isn't an issue because our blob has a fixed size that has free space inside it, so creating the rsvmap and /chosen entries eat at the internal free space and don't change the total blob size.
People are advocating dynamically increasing the blob size, which simplifies things for blob generation (don't have to guess how big to make the blob when running the dtc to create it), but that would cause problems with my counter-proposal.
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
- k
In message 8B11A571-3BC2-42BD-9B66-DF22A736A3A7@kernel.crashing.org you wrote:
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
So what is the state of this patch now? Will it go in through the FDT custodian repo, or has it been rejected, or am I supposed to apply it directly? Or?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
In message 8B11A571-3BC2-42BD-9B66-DF22A736A3A7@kernel.crashing.org you wrote:
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
So what is the state of this patch now? Will it go in through the FDT custodian repo, or has it been rejected, or am I supposed to apply it directly? Or?
Best regards,
Wolfgang Denk
Hi Wolfgang, Kumar,
Most of the email exchanges were with me and the patch is fine IMHO.
I was expecting it to go through the "new-image" custodian, but I would be happy to apply it to the u-boot-fdt repo and push it up from there.
Preferences, Kumar?
Best regards, gvb
Jerry Van Baren wrote:
Wolfgang Denk wrote:
In message 8B11A571-3BC2-42BD-9B66-DF22A736A3A7@kernel.crashing.org you wrote:
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
So what is the state of this patch now? Will it go in through the FDT custodian repo, or has it been rejected, or am I supposed to apply it directly? Or?
[...]
Most of the email exchanges were with me and the patch is fine IMHO.
I was expecting it to go through the "new-image" custodian, but I would be happy to apply it to the u-boot-fdt repo and push it up from there.
Jerry, Kumar,
I think it would be better to have this patch (and other ones by Kumar that touch booting-related code) go through the new-image branch. Otherwise we'll have merge conflicts down the road, especially since we've got a big patch coming, that adds framework for new image handling. I'd like to have this framework patch merged first, then take care of (re-based) boot-related patches that have been posted recently.
Regards, Bartlomiej
On Feb 22, 2008, at 8:36 AM, Bartlomiej Sieka wrote:
Jerry Van Baren wrote:
Wolfgang Denk wrote:
In message <8B11A571-3BC2-42BD-9B66- DF22A736A3A7@kernel.crashing.org> you wrote:
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
So what is the state of this patch now? Will it go in through the FDT custodian repo, or has it been rejected, or am I supposed to apply it directly? Or?
[...]
Most of the email exchanges were with me and the patch is fine IMHO. I was expecting it to go through the "new-image" custodian, but I would be happy to apply it to the u-boot-fdt repo and push it up from there.
Jerry, Kumar,
I think it would be better to have this patch (and other ones by Kumar that touch booting-related code) go through the new-image branch. Otherwise we'll have merge conflicts down the road, especially since we've got a big patch coming, that adds framework for new image handling. I'd like to have this framework patch merged first, then take care of (re-based) boot-related patches that have been posted recently.
Agreed. Was off having some fun for a few days.
I think going through the new image tree is best.
- k
Kumar Gala wrote:
On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote:
Kumar Gala wrote:
[[[[snip]]]]
The patch is creating dummy initrd entries in the reserved map and in /chosen, only to work hard to delete and re-create the reserved map entries and rewrite the /chosen entries.
My counter-proposal is to not bother with dummy values. Simply pass in 0,0 which will prevent the creation of the initrd entries by fdt_chosen(). By not creating dummy entries, you can simply create the proper entries once you know what the the correct values are, rather than the more complicated rsvmap search & delete + rsvmap creation + /chosen modifications.
Ahh, the reason I wanted them created was to ensure we have enough size for them up front rather than figuring that out later. By creating them and replacing them I will not being changing the size at all.
- k
OK, I see.
Currently this isn't an issue because our blob has a fixed size that has free space inside it, so creating the rsvmap and /chosen entries eat at the internal free space and don't change the total blob size.
People are advocating dynamically increasing the blob size, which simplifies things for blob generation (don't have to guess how big to make the blob when running the dtc to create it), but that would cause problems with my counter-proposal.
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
But we don't really grow the blob, we are just allocating the space for the initrd properties - *if* the blob already has enough free space. If the blob does not have enough free space we'll hit the bottom anyway, whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't matter whether we pre-allocate space for initrd or not. Or am I missing something?
I was rather thinking of increasing the total blob size when relocating it. Currently relocation happens only when the blob is not within BOOTMAPSZ region, so we would need to always relocate the blob and figure out the size delta: (1) get it from env variable, if set (2) or use some default delta. What do you think?
Cheers, m.
Marian Balakowicz wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote: > Kumar Gala wrote:
[[[[snip]]]]
The patch is creating dummy initrd entries in the reserved map and in /chosen, only to work hard to delete and re-create the reserved map entries and rewrite the /chosen entries.
My counter-proposal is to not bother with dummy values. Simply pass in 0,0 which will prevent the creation of the initrd entries by fdt_chosen(). By not creating dummy entries, you can simply create the proper entries once you know what the the correct values are, rather than the more complicated rsvmap search & delete + rsvmap creation + /chosen modifications.
Ahh, the reason I wanted them created was to ensure we have enough size for them up front rather than figuring that out later. By creating them and replacing them I will not being changing the size at all.
- k
OK, I see.
Currently this isn't an issue because our blob has a fixed size that has free space inside it, so creating the rsvmap and /chosen entries eat at the internal free space and don't change the total blob size.
People are advocating dynamically increasing the blob size, which simplifies things for blob generation (don't have to guess how big to make the blob when running the dtc to create it), but that would cause problems with my counter-proposal.
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
But we don't really grow the blob, we are just allocating the space for the initrd properties - *if* the blob already has enough free space. If the blob does not have enough free space we'll hit the bottom anyway, whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't matter whether we pre-allocate space for initrd or not. Or am I missing something?
I was rather thinking of increasing the total blob size when relocating it. Currently relocation happens only when the blob is not within BOOTMAPSZ region, so we would need to always relocate the blob and figure out the size delta: (1) get it from env variable, if set (2) or use some default delta. What do you think?
Also, for the blob growing changes it would be nice to first merge your LMB patches to sort out the memory allocation. I didn't have a time to review your changes in detail but it's definitely step in the right direction. Current bootm allocation is pretty messy, I cleaned it a bit but didn't want to touch too many pieces at the same time. For the same reason I would like to finish the current patch I am working on that adds dual format framework before we add LMB and dynamic blob growing.
m.
Marian Balakowicz wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote: > Kumar Gala wrote:
[[[[snip]]]]
The patch is creating dummy initrd entries in the reserved map and in /chosen, only to work hard to delete and re-create the reserved map entries and rewrite the /chosen entries.
My counter-proposal is to not bother with dummy values. Simply pass in 0,0 which will prevent the creation of the initrd entries by fdt_chosen(). By not creating dummy entries, you can simply create the proper entries once you know what the the correct values are, rather than the more complicated rsvmap search & delete + rsvmap creation + /chosen modifications.
Ahh, the reason I wanted them created was to ensure we have enough size for them up front rather than figuring that out later. By creating them and replacing them I will not being changing the size at all.
- k
OK, I see.
Currently this isn't an issue because our blob has a fixed size that has free space inside it, so creating the rsvmap and /chosen entries eat at the internal free space and don't change the total blob size.
People are advocating dynamically increasing the blob size, which simplifies things for blob generation (don't have to guess how big to make the blob when running the dtc to create it), but that would cause problems with my counter-proposal.
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
But we don't really grow the blob, we are just allocating the space for the initrd properties - *if* the blob already has enough free space. If the blob does not have enough free space we'll hit the bottom anyway, whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't matter whether we pre-allocate space for initrd or not. Or am I missing something?
I was rather thinking of increasing the total blob size when relocating it. Currently relocation happens only when the blob is not within BOOTMAPSZ region, so we would need to always relocate the blob and figure out the size delta: (1) get it from env variable, if set (2) or use some default delta. What do you think?
Cheers, m.
The missing part is libfdt doesn't exactly support dynamic resizing and our current code doesn't do in-place resizing (which it could do by doing a move to the same location, but with a larger/smaller length).
Kumar is lining up the pieces to get there, but we aren't there yet...
gvb
Jerry Van Baren wrote:
Marian Balakowicz wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote:
Kumar Gala wrote: > On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote: >> Kumar Gala wrote:
[[[[snip]]]]
The patch is creating dummy initrd entries in the reserved map and in /chosen, only to work hard to delete and re-create the reserved map entries and rewrite the /chosen entries.
My counter-proposal is to not bother with dummy values. Simply pass in 0,0 which will prevent the creation of the initrd entries by fdt_chosen(). By not creating dummy entries, you can simply create the proper entries once you know what the the correct values are, rather than the more complicated rsvmap search & delete + rsvmap creation + /chosen modifications.
Ahh, the reason I wanted them created was to ensure we have enough size for them up front rather than figuring that out later. By creating them and replacing them I will not being changing the size at all.
- k
OK, I see.
Currently this isn't an issue because our blob has a fixed size that has free space inside it, so creating the rsvmap and /chosen entries eat at the internal free space and don't change the total blob size.
People are advocating dynamically increasing the blob size, which simplifies things for blob generation (don't have to guess how big to make the blob when running the dtc to create it), but that would cause problems with my counter-proposal.
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
But we don't really grow the blob, we are just allocating the space for the initrd properties - *if* the blob already has enough free space. If the blob does not have enough free space we'll hit the bottom anyway, whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't matter whether we pre-allocate space for initrd or not. Or am I missing something?
I was rather thinking of increasing the total blob size when relocating it. Currently relocation happens only when the blob is not within BOOTMAPSZ region, so we would need to always relocate the blob and figure out the size delta: (1) get it from env variable, if set (2) or use some default delta. What do you think?
The missing part is libfdt doesn't exactly support dynamic resizing and our current code doesn't do in-place resizing (which it could do by doing a move to the same location, but with a larger/smaller length).
Kumar is lining up the pieces to get there, but we aren't there yet...
I see, but how about resizing to a new location:
- err = fdt_open_into (fdt_blob, (void *)of_start, of_len); + err = fdt_open_into (fdt_blob, (void *)of_start, of_len + delta);
Should that work?
If we add LMB and rework bootm memory allocation, putting things (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting from bootm_low then we may want to always relocate fdt to avoid overlapping. And, in case of new uImage FDT blob will be embedded in a new uImage shell which is a blob itself. So, in this case in-place resizing is not really a clean option, we would need to resize the embedding new uImage blob first, and this one may have significant size, so I suspect it may impact performance.
m.
On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
Jerry Van Baren wrote:
Marian Balakowicz wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:46 PM, Jerry Van Baren wrote:
Kumar Gala wrote:
On Feb 18, 2008, at 1:15 PM, Jerry Van Baren wrote: > Kumar Gala wrote: >> On Feb 18, 2008, at 11:51 AM, Jerry Van Baren wrote: >>> Kumar Gala wrote:
[[[[snip]]]]
> The patch is creating dummy initrd entries in the reserved map > and in /chosen, only to work hard to delete and re-create the > reserved map entries and rewrite the /chosen entries. > > My counter-proposal is to not bother with dummy values. Simply > pass in 0,0 which will prevent the creation of the initrd > entries > by fdt_chosen(). By not creating dummy entries, you can simply > create the proper entries once you know what the the correct > values are, rather than the more complicated rsvmap search & > delete + rsvmap creation + /chosen modifications. Ahh, the reason I wanted them created was to ensure we have enough size for them up front rather than figuring that out later. By creating them and replacing them I will not being changing the size at all.
- k
OK, I see.
Currently this isn't an issue because our blob has a fixed size that has free space inside it, so creating the rsvmap and /chosen entries eat at the internal free space and don't change the total blob size.
People are advocating dynamically increasing the blob size, which simplifies things for blob generation (don't have to guess how big to make the blob when running the dtc to create it), but that would cause problems with my counter-proposal.
And the whole point of my patch was to enable the ability to dynamically grow the blob before we do anything w/the ramdisk.
But we don't really grow the blob, we are just allocating the space for the initrd properties - *if* the blob already has enough free space. If the blob does not have enough free space we'll hit the bottom anyway, whether in fdt_chosen() or ft_board_setup(), so it seem that it doesn't matter whether we pre-allocate space for initrd or not. Or am I missing something?
I was rather thinking of increasing the total blob size when relocating it. Currently relocation happens only when the blob is not within BOOTMAPSZ region, so we would need to always relocate the blob and figure out the size delta: (1) get it from env variable, if set (2) or use some default delta. What do you think?
The missing part is libfdt doesn't exactly support dynamic resizing and our current code doesn't do in-place resizing (which it could do by doing a move to the same location, but with a larger/smaller length).
Kumar is lining up the pieces to get there, but we aren't there yet...
I see, but how about resizing to a new location:
- err = fdt_open_into (fdt_blob, (void *)of_start, of_len);
- err = fdt_open_into (fdt_blob, (void *)of_start, of_len + delta);
Should that work?
If we add LMB and rework bootm memory allocation, putting things (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting from bootm_low then we may want to always relocate fdt to avoid overlapping. And, in case of new uImage FDT blob will be embedded in a new uImage shell which is a blob itself. So, in this case in-place resizing is not really a clean option, we would need to resize the embedding new uImage blob first, and this one may have significant size, so I suspect it may impact performance.
I felt the sequence (on PPC) is either: kernel, cmdline, kdb, initrd
or
kernel, fdt, initrd
The reason being is that initrd doesn't need to be constrained to BOOTMAPSZ but cmdline, kdb, and fdt would/should be.
- k
Kumar Gala wrote:
On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
...
If we add LMB and rework bootm memory allocation, putting things (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting from bootm_low then we may want to always relocate fdt to avoid overlapping. And, in case of new uImage FDT blob will be embedded in a new uImage shell which is a blob itself. So, in this case in-place resizing is not really a clean option, we would need to resize the embedding new uImage blob first, and this one may have significant size, so I suspect it may impact performance.
I felt the sequence (on PPC) is either: kernel, cmdline, kdb, initrd
or
kernel, fdt, initrd
The reason being is that initrd doesn't need to be constrained to BOOTMAPSZ but cmdline, kdb, and fdt would/should be.
That's right.
My point was just to have two steps:
1) Move all the stuff to the final locations (whatever the sequence)
- kernel
- fdt - *always* relocate to within BOOTMAPSZ, increase size dynamically.
fdt blob can be delivered using of the three different methods: (1) raw fdt blob, (2) fdt blob embedded in legacy format uImage, (3) fdt blob embedded in new uImage format. To simplify things always relocate it to a allocated spot within BOOTMAPSZ.
- initrd - within or outside of the BOOTMAPSZ boundaries
2) Update fdt blob, knowing we have enough free blob space: set initrd params, other fixups, etc.
With such approach we don't need special treatment for initrd,start/initrd,end (and other fixups). We assure that there is enough free space in fdt blob when we relocate it.
m.
On Feb 26, 2008, at 3:11 AM, Marian Balakowicz wrote:
Kumar Gala wrote:
On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
...
If we add LMB and rework bootm memory allocation, putting things (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting from bootm_low then we may want to always relocate fdt to avoid overlapping. And, in case of new uImage FDT blob will be embedded in a new uImage shell which is a blob itself. So, in this case in-place resizing is not really a clean option, we would need to resize the embedding new uImage blob first, and this one may have significant size, so I suspect it may impact performance.
I felt the sequence (on PPC) is either: kernel, cmdline, kdb, initrd
or
kernel, fdt, initrd
The reason being is that initrd doesn't need to be constrained to BOOTMAPSZ but cmdline, kdb, and fdt would/should be.
That's right.
My point was just to have two steps:
- Move all the stuff to the final locations (whatever the sequence)
kernel
fdt - *always* relocate to within BOOTMAPSZ, increase size
dynamically.
fdt blob can be delivered using of the three different methods: (1) raw fdt blob, (2) fdt blob embedded in legacy format uImage, (3) fdt blob embedded in new uImage format. To simplify things always relocate it to a allocated spot within BOOTMAPSZ.
- initrd - within or outside of the BOOTMAPSZ boundaries
- Update fdt blob, knowing we have enough free blob space: set initrd
params, other fixups, etc.
With such approach we don't need special treatment for initrd,start/initrd,end (and other fixups). We assure that there is enough free space in fdt blob when we relocate it.
How we get the fdt blob doesn't matter as much as its size. At this point we still don't know how large it might need to be. I think the changes I made make sense in that we'd want to process the majority of the fdt before we do anything w/the initrd.
We can cleanup the fixup once we have better support for dealing w/the sizing of the fdt.
- k
Kumar Gala wrote:
On Feb 26, 2008, at 3:11 AM, Marian Balakowicz wrote:
Kumar Gala wrote:
On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
...
If we add LMB and rework bootm memory allocation, putting things (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting from bootm_low then we may want to always relocate fdt to avoid overlapping. And, in case of new uImage FDT blob will be embedded in a new uImage shell which is a blob itself. So, in this case in-place resizing is not really a clean option, we would need to resize the embedding new uImage blob first, and this one may have significant size, so I suspect it may impact performance.
I felt the sequence (on PPC) is either: kernel, cmdline, kdb, initrd
or
kernel, fdt, initrd
The reason being is that initrd doesn't need to be constrained to BOOTMAPSZ but cmdline, kdb, and fdt would/should be.
That's right.
My point was just to have two steps:
- Move all the stuff to the final locations (whatever the sequence)
kernel
fdt - *always* relocate to within BOOTMAPSZ, increase size dynamically.
fdt blob can be delivered using of the three different methods: (1) raw fdt blob, (2) fdt blob embedded in legacy format uImage, (3) fdt blob embedded in new uImage format. To simplify things always relocate it to a allocated spot within BOOTMAPSZ.
- initrd - within or outside of the BOOTMAPSZ boundaries
- Update fdt blob, knowing we have enough free blob space: set initrd
params, other fixups, etc.
With such approach we don't need special treatment for initrd,start/initrd,end (and other fixups). We assure that there is enough free space in fdt blob when we relocate it.
How we get the fdt blob doesn't matter as much as its size. At this point we still don't know how large it might need to be. I think the changes I made make sense in that we'd want to process the majority of the fdt before we do anything w/the initrd.
Correct me if I am wrong but my understanding is that it will not help if the total size of the blob is too small.
It will just use/allocate free internal blob space - if the internal free space is sufficient. And if it is it can be used at any time. If it is not sufficient, then we need to grow the blob total size first.
m.
On Feb 27, 2008, at 5:20 AM, Marian Balakowicz wrote:
Kumar Gala wrote:
On Feb 26, 2008, at 3:11 AM, Marian Balakowicz wrote:
Kumar Gala wrote:
On Feb 22, 2008, at 11:08 AM, Marian Balakowicz wrote:
...
If we add LMB and rework bootm memory allocation, putting things (kernel, cmdline, kdb, initrd (optionally), fdt) in sequence starting from bootm_low then we may want to always relocate fdt to avoid overlapping. And, in case of new uImage FDT blob will be embedded in a new uImage shell which is a blob itself. So, in this case in-place resizing is not really a clean option, we would need to resize the embedding new uImage blob first, and this one may have significant size, so I suspect it may impact performance.
I felt the sequence (on PPC) is either: kernel, cmdline, kdb, initrd
or
kernel, fdt, initrd
The reason being is that initrd doesn't need to be constrained to BOOTMAPSZ but cmdline, kdb, and fdt would/should be.
That's right.
My point was just to have two steps:
- Move all the stuff to the final locations (whatever the sequence)
kernel
fdt - *always* relocate to within BOOTMAPSZ, increase size
dynamically.
fdt blob can be delivered using of the three different methods: (1) raw fdt blob, (2) fdt blob embedded in legacy format uImage, (3) fdt blob embedded in new uImage format. To simplify things always relocate it to a allocated spot within BOOTMAPSZ.
- initrd - within or outside of the BOOTMAPSZ boundaries
- Update fdt blob, knowing we have enough free blob space: set
initrd params, other fixups, etc.
With such approach we don't need special treatment for initrd,start/initrd,end (and other fixups). We assure that there is enough free space in fdt blob when we relocate it.
How we get the fdt blob doesn't matter as much as its size. At this point we still don't know how large it might need to be. I think the changes I made make sense in that we'd want to process the majority of the fdt before we do anything w/the initrd.
Correct me if I am wrong but my understanding is that it will not help if the total size of the blob is too small.
It will just use/allocate free internal blob space - if the internal free space is sufficient. And if it is it can be used at any time. If it is not sufficient, then we need to grow the blob total size first.
At this point in time you are correct. However I'm looking forward to having the possibility of growing things w/regards to the fdt.
- k
participants (5)
-
Bartlomiej Sieka -
Jerry Van Baren -
Kumar Gala -
Marian Balakowicz -
Wolfgang Denk