[U-Boot] [PATCH 2/3] libfdt: make fdt_increase_size() available to everyone
The function fdt_increase_size() increases the size of the device tree by the given amount. This is useful for any code that wants to add a node or large property, to avoid the possibility of running out of space. It's much smarter to have U-Boot increase the size of device tree when it knows it's going to add data, instead of hoping that the DTS was compiled with the right -p value.
Signed-off-by: Timur Tabi timur@freescale.com ---
common/fdt_support.c | 20 ++++++++++---------- include/fdt_support.h | 1 + 2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c index b6f252a..01d635b 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -645,6 +645,16 @@ int fdt_resize(void *blob) return actualsize; }
+int fdt_increase_size(void *fdt, int add_len) +{ + int newlen; + + newlen = fdt_totalsize(fdt) + add_len; + + /* Open in place with a new len */ + return fdt_open_into(fdt, fdt, newlen); +} + #ifdef CONFIG_PCI #define CONFIG_SYS_PCI_NR_INBOUND_WIN 4
@@ -792,16 +802,6 @@ int fdt_del_subnodes(const void *blob, int parent_offset) return 0; }
-int fdt_increase_size(void *fdt, int add_len) -{ - int newlen; - - newlen = fdt_totalsize(fdt) + add_len; - - /* Open in place with a new len */ - return fdt_open_into(fdt, fdt, newlen); -} - int fdt_del_partitions(void *blob, int parent_offset) { const void *prop; diff --git a/include/fdt_support.h b/include/fdt_support.h index 9a453af..d70627d 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -78,6 +78,7 @@ void ft_pci_setup(void *blob, bd_t *bd);
void set_working_fdt_addr(void *addr); int fdt_resize(void *blob); +int fdt_increase_size(void *fdt, int add_len);
int fdt_fixup_nor_flash_size(void *blob, int cs, u32 size);
On Mon, May 10, 2010 at 4:26 PM, Timur Tabi timur@freescale.com wrote:
The function fdt_increase_size() increases the size of the device tree by the given amount. This is useful for any code that wants to add a node or large property, to avoid the possibility of running out of space. It's much smarter to have U-Boot increase the size of device tree when it knows it's going to add data, instead of hoping that the DTS was compiled with the right -p value.
Signed-off-by: Timur Tabi timur@freescale.com
Please ignore the "2/3". This patch is not part of a series.
Wolfgang, we have code (not yet ready to be published) that needs this change. Please apply if you don't have any objections.
On May 10, 2010, at 4:26 PM, Timur Tabi wrote:
The function fdt_increase_size() increases the size of the device tree by the given amount. This is useful for any code that wants to add a node or large property, to avoid the possibility of running out of space. It's much smarter to have U-Boot increase the size of device tree when it knows it's going to add data, instead of hoping that the DTS was compiled with the right -p value.
Signed-off-by: Timur Tabi timur@freescale.com
Jerry, any comments on this?
- k
common/fdt_support.c | 20 ++++++++++---------- include/fdt_support.h | 1 + 2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c index b6f252a..01d635b 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -645,6 +645,16 @@ int fdt_resize(void *blob) return actualsize; }
+int fdt_increase_size(void *fdt, int add_len) +{
- int newlen;
- newlen = fdt_totalsize(fdt) + add_len;
- /* Open in place with a new len */
- return fdt_open_into(fdt, fdt, newlen);
+}
#ifdef CONFIG_PCI #define CONFIG_SYS_PCI_NR_INBOUND_WIN 4
@@ -792,16 +802,6 @@ int fdt_del_subnodes(const void *blob, int parent_offset) return 0; }
-int fdt_increase_size(void *fdt, int add_len) -{
- int newlen;
- newlen = fdt_totalsize(fdt) + add_len;
- /* Open in place with a new len */
- return fdt_open_into(fdt, fdt, newlen);
-}
int fdt_del_partitions(void *blob, int parent_offset) { const void *prop; diff --git a/include/fdt_support.h b/include/fdt_support.h index 9a453af..d70627d 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -78,6 +78,7 @@ void ft_pci_setup(void *blob, bd_t *bd);
void set_working_fdt_addr(void *addr); int fdt_resize(void *blob); +int fdt_increase_size(void *fdt, int add_len);
int fdt_fixup_nor_flash_size(void *blob, int cs, u32 size);
-- 1.6.5
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
The function fdt_increase_size() increases the size of the device tree by the given amount. This is useful for any code that wants to add a node or large property, to avoid the possibility of running out of space. It's much smarter to have U-Boot increase the size of device tree when it knows it's going to add data, instead of hoping that the DTS was compiled with the right -p value.
Improve the size increase algorithm by malloc()ing the necessary space. This avoids potential problems with the FDT blob overwriting things that follow it in memory.
Signed-off-by: Timur Tabi timur@freescale.com Signed-off-by: Gerald Van Baren vanbaren@cideas.com ---
Hi Timur, Kumar,
This had me scratching my head briefly. I first thought you were creating a new function, but you just exposed the existing function in fdt_support.h and moved it to its logical position, given its new status.
The code has one pre-existing weakness that bothers me: if there is something following the FDT blob, it will get overwritten by the increased blob. One way around this would be to malloc() a new memory space and move and expand the blob to the new space. The first time this was done, the original blob should not be free()ed since it wasn't malloc()ed, but the second and subsequent times it should be free()ed.
I've added this to your patch, but have *NOT* execution tested it. Does this addition (a) make sense and (b) work?
Thanks, gvb
common/fdt_support.c | 35 +++++++++++++++++++++++++---------- include/fdt_support.h | 1 + 2 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c index b6f252a..6bd03d6 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -645,6 +645,31 @@ int fdt_resize(void *blob) return actualsize; }
+/* Keep track of malloc()ed blobs for freeing */ +static void *fdt_malloc_blob = 0; + +int fdt_increase_size(void *fdt, int add_len) +{ + int newlen; + void *newblob; + int ret; + + newlen = fdt_totalsize(fdt) + add_len; + newblob = malloc(newlen); + + /* Copy the blob and expand it */ + ret = fdt_open_into(fdt, newblob, newlen); + if (!ret) { + free(newblob); + return ret; + } + + if (!fdt_malloc_blob) + free(fdt_malloc_blob); + fdt_malloc_blob = newblob; + return 0; +} + #ifdef CONFIG_PCI #define CONFIG_SYS_PCI_NR_INBOUND_WIN 4
@@ -792,16 +817,6 @@ int fdt_del_subnodes(const void *blob, int parent_offset) return 0; }
-int fdt_increase_size(void *fdt, int add_len) -{ - int newlen; - - newlen = fdt_totalsize(fdt) + add_len; - - /* Open in place with a new len */ - return fdt_open_into(fdt, fdt, newlen); -} - int fdt_del_partitions(void *blob, int parent_offset) { const void *prop; diff --git a/include/fdt_support.h b/include/fdt_support.h index 9a453af..d70627d 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -78,6 +78,7 @@ void ft_pci_setup(void *blob, bd_t *bd);
void set_working_fdt_addr(void *addr); int fdt_resize(void *blob); +int fdt_increase_size(void *fdt, int add_len);
int fdt_fixup_nor_flash_size(void *blob, int cs, u32 size);
On Sat, May 15, 2010 at 9:11 PM, Gerald Van Baren gvb.uboot@gmail.com wrote:
The code has one pre-existing weakness that bothers me: if there is something following the FDT blob, it will get overwritten by the increased blob. One way around this would be to malloc() a new memory space and move and expand the blob to the new space. The first time this was done, the original blob should not be free()ed since it wasn't malloc()ed, but the second and subsequent times it should be free()ed.
But then how does the caller know where the new blob is? When I call fdt_increase_size(), I pass it the address of a blob that I'm modifying. After the function returns, my value of 'fdt' is no longer valid.
I've added this to your patch, but have *NOT* execution tested it. Does this addition (a) make sense and (b) work?
I was expecting the caller of fdt_increase_size() to know that the space after the fdt is available.
Timur Tabi wrote:
On Sat, May 15, 2010 at 9:11 PM, Gerald Van Baren gvb.uboot@gmail.com wrote:
The code has one pre-existing weakness that bothers me: if there is something following the FDT blob, it will get overwritten by the increased blob. One way around this would be to malloc() a new memory space and move and expand the blob to the new space. The first time this was done, the original blob should not be free()ed since it wasn't malloc()ed, but the second and subsequent times it should be free()ed.
But then how does the caller know where the new blob is? When I call fdt_increase_size(), I pass it the address of a blob that I'm modifying. After the function returns, my value of 'fdt' is no longer valid.
Oops, yes, that is a pretty fatal flaw.
I've added this to your patch, but have *NOT* execution tested it. Does this addition (a) make sense and (b) work?
I was expecting the caller of fdt_increase_size() to know that the space after the fdt is available.
OK. I'll <acked-by> the original patch. The best way to incorporate the patch would be for Kumar (I assume) to pick up the libfdt change with the Freescale change that needs it so that the changes are inherently coordinated.
gvb
Dear Timur Tabi,
In message AANLkTilPrgpUwL630lzSmzKt5bBXFiq6zDPIiR3pQcwa@mail.gmail.com you wrote:
I've added this to your patch, but have *NOT* execution tested it. Does this addition (a) make sense and (b) work?
I was expecting the caller of fdt_increase_size() to know that the space after the fdt is available.
So where is the code that makes sure that there is sufficient space available?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
I was expecting the caller of fdt_increase_size() to know that the space after the fdt is available.
So where is the code that makes sure that there is sufficient space available?
Well, if I load the fdt to address c0000, and it's 10KB in size, I'm pretty sure I can expand it to 16KB if I know that there's nothing else between c0000 and c4000.
If you're asking about what code do I have that actually calls fdt_increase_size(), that code is currently used for a new board that we're not yet ready to publish. I'd like my patch for fdt_increase_size() included in the next merge window, so that when rebase our internal repository on the next release, this patch will already be available. It makes our internal development easier.
Dear Timur Tabi,
In message 4BF14FBF.3040900@freescale.com you wrote:
So where is the code that makes sure that there is sufficient space available?
Well, if I load the fdt to address c0000, and it's 10KB in size, I'm pretty sure I can expand it to 16KB if I know that there's nothing else between c0000 and c4000.
Assume the case that the DTB is stored in NOR flash, and I pass the NOR flash address to the bootm command...
I'm not sure if there is any guarantee for free room behind the DTB in this case.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Assume the case that the DTB is stored in NOR flash, and I pass the NOR flash address to the bootm command...
I'm not sure if there is any guarantee for free room behind the DTB in this case.
We can never guarantee this. The code that calls fdt_increase_size() will just have to ensure that there is enough room.
If the fdt is in NOR flash, then boot_relocate_fdt() will copy it to RAM via lmb_alloc_base(), which uses CONFIG_SYS_FDT_PAD to determine how much extra room is needed.
So it appear to me that there are two ways the fdt is "allocated" in memory:
1) It is copied to a location allocated via lmb_alloc_base() by boot_relocate_fdt().
2) It is simply placed somewhere in memory (via tftp) and left there.
I don't believe we ever use malloc() to allocate the memory for the fdt, so these two should cover 100% of all cases.
So for case #1, we can use CONFIG_SYS_FDT_PAD. For case #2, we just have to assume that when the user did "tftp c0000 my.dtb", that he knows what he's doing.
I assume that fdt_increase_size() will only be used to increase the available space by a significant amount. One example of this (and the reason I posted this patch in the first place), is to embed firmware inside the device tree. A new binding for Freescale QE firmware allows for this, and I have code in-house which implements this.
So I say that a particular board will know whether it needs to significantly expand the available amount of RAM beyond the default CONFIG_SYS_FDT_PAD. Therefore, we can define a new value of CONFIG_SYS_FDT_PAD in the board header files for those boards that need it.
I believe that this should cover everything, and that my patch is okay and should be merged in.
Dear Timur Tabi,
In message 4BF29FE9.1070305@freescale.com you wrote:
Wolfgang Denk wrote:
Assume the case that the DTB is stored in NOR flash, and I pass the NOR flash address to the bootm command...
I'm not sure if there is any guarantee for free room behind the DTB in this case.
We can never guarantee this. The code that calls fdt_increase_size() will just have to ensure that there is enough room.
Such an "ensure that there is enough room" is exactly what I'm asking for.
If the fdt is in NOR flash, then boot_relocate_fdt() will copy it to RAM via lmb_alloc_base(), which uses CONFIG_SYS_FDT_PAD to determine how much extra room is needed.
Hm... it seems that not a single board uses this setting, which also happens to be completely undocumented.
So it appear to me that there are two ways the fdt is "allocated" in memory:
- It is copied to a location allocated via lmb_alloc_base() by
boot_relocate_fdt().
- It is simply placed somewhere in memory (via tftp) and left there.
I don't believe we ever use malloc() to allocate the memory for the fdt, so these two should cover 100% of all cases.
For me lmb_alloc_base() is just another form of malloc()...
So for case #1, we can use CONFIG_SYS_FDT_PAD. For case #2, we just have to assume that when the user did "tftp c0000 my.dtb", that he knows what he's doing.
I'm not sure if we have the same intentions here.
I think, that for case #1 the available size is known, so we can check if we exceed the limits. And this is what we should do then.
I assume that fdt_increase_size() will only be used to increase the available space by a significant amount. One example of this (and the reason I posted this patch in the first place), is to embed firmware inside the device tree. A new binding for Freescale QE firmware allows for this, and I have code in-house which implements this.
If we are talking about such substantial increments then it is all the more important to check for available room.
So I say that a particular board will know whether it needs to significantly expand the available amount of RAM beyond the default CONFIG_SYS_FDT_PAD. Therefore, we can define a new value of CONFIG_SYS_FDT_PAD in the board header files for those boards that need it.
No. We should check if the programmed value is sufficient.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
We can never guarantee this. The code that calls fdt_increase_size() will just have to ensure that there is enough room.
Such an "ensure that there is enough room" is exactly what I'm asking for.
Maybe I don't understand what you're getting at. My point is that, whenever someone writes code that calls fdt_increase_size(), *that* person needs to provide the assurance, not me. Within fdt_increase_size(), there is no way to ensure anything. And since my patch only deals with fdt_increase_size() itself, I don't understand what you want from *me* within the context of *this* patch.
If the fdt is in NOR flash, then boot_relocate_fdt() will copy it to RAM via lmb_alloc_base(), which uses CONFIG_SYS_FDT_PAD to determine how much extra room is needed.
Hm... it seems that not a single board uses this setting,
That's because the default has been sufficient so far.
which also happens to be completely undocumented.
That's got nothing to do with me. I didn't write the code that uses CONFIG_SYS_FDT_PAD.
For me lmb_alloc_base() is just another form of malloc()...
True, but it's not the same as malloc(). For example, I cannot use realloc() to make the allocation bigger. If all fdts were allocated via malloc(), then I could modify fdt_increase_size() to call realloc().
So for case #1, we can use CONFIG_SYS_FDT_PAD. For case #2, we just have to assume that when the user did "tftp c0000 my.dtb", that he knows what he's doing.
I'm not sure if we have the same intentions here.
I think, that for case #1 the available size is known, so we can check if we exceed the limits. And this is what we should do then.
But within fdt_increase_size(), how do I know if the memory is allocated via lmb_alloc_base()? The heap management data structure for an lmb is managed external to the heap itself.
I assume that fdt_increase_size() will only be used to increase the available space by a significant amount. One example of this (and the reason I posted this patch in the first place), is to embed firmware inside the device tree. A new binding for Freescale QE firmware allows for this, and I have code in-house which implements this.
If we are talking about such substantial increments then it is all the more important to check for available room.
And again, the point *I* am trying to make is that it's okay to put the onus of that check on the *caller* of fdt_increase_size(), and not on fdt_increase_size() itself.
So I say that a particular board will know whether it needs to significantly expand the available amount of RAM beyond the default CONFIG_SYS_FDT_PAD. Therefore, we can define a new value of CONFIG_SYS_FDT_PAD in the board header files for those boards that need it.
No. We should check if the programmed value is sufficient.
But that is only meaningful if the fdt is allocated via an lmb, which is not true in case #2. In case #2, there is no allocation of memory, so there's no way to know within fdt_increase_size()!
Dear Timur Tabi,
In message 4BF2B302.2030909@freescale.com you wrote:
We can never guarantee this. The code that calls fdt_increase_size() will just have to ensure that there is enough room.
Such an "ensure that there is enough room" is exactly what I'm asking for.
Maybe I don't understand what you're getting at. My point is that, whenever someone writes code that calls fdt_increase_size(), *that* person needs to provide the assurance, not me. Within fdt_increase_size(), there is no way to ensure anything. And since my patch only deals with fdt_increase_size() itself, I don't understand what you want from *me* within the context of *this* patch.
Your problem is that you are too much focussed on "your patch" only. You need to keep your eyes open for the bigger whole. Your patch has the potential of causing nasty crashes, and I ask you to prevent this. This may require changes outside the current scope of "your patch".
If the fdt is in NOR flash, then boot_relocate_fdt() will copy it to RAM via lmb_alloc_base(), which uses CONFIG_SYS_FDT_PAD to determine how much extra room is needed.
Hm... it seems that not a single board uses this setting,
That's because the default has been sufficient so far.
Right. But your patch is supposed to change this.
which also happens to be completely undocumented.
That's got nothing to do with me. I didn't write the code that uses CONFIG_SYS_FDT_PAD.
No. But you are trying to get code into mainline where the current code is insufficient. So you also got to extend the code to make yourpatch acceptable.
I think, that for case #1 the available size is known, so we can check if we exceed the limits. And this is what we should do then.
But within fdt_increase_size(), how do I know if the memory is allocated via lmb_alloc_base()? The heap management data structure for an lmb is managed external to the heap itself.
If you don't know this in fdt_increase_size(), then either make it known there (for example by extending other code to pass on such information), or add such checking to other parts of the code in the call chain.
If we are talking about such substantial increments then it is all the more important to check for available room.
And again, the point *I* am trying to make is that it's okay to put the onus of that check on the *caller* of fdt_increase_size(), and not on fdt_increase_size() itself.
OK. I have no problem with that. I am just missing this other part of the required changes in your patch.
No. We should check if the programmed value is sufficient.
But that is only meaningful if the fdt is allocated via an lmb, which is not true in case #2. In case #2, there is no allocation of memory, so there's no way to know within fdt_increase_size()!
Maybe. But there is still case #1, where we have a real problem, and I will not accept your patch without seing this problem properly addressed.
Best regards,
Wolfgang Denk
On Tue, May 18, 2010 at 5:20 PM, Wolfgang Denk wd@denx.de wrote:
And again, the point *I* am trying to make is that it's okay to put the onus of that check on the *caller* of fdt_increase_size(), and not on fdt_increase_size() itself.
OK. I have no problem with that. I am just missing this other part of the required changes in your patch.
I'm not ready to submit any code that calls fdt_increase_size() yet. I'm just trying to create the infrastructure here so that I can be sure that my in-house code is correct. If this patch is rejected because there's a better way to increase the size of an fdt, then I need to know that *now*.
But that is only meaningful if the fdt is allocated via an lmb, which is not true in case #2. In case #2, there is no allocation of memory, so there's no way to know within fdt_increase_size()!
Maybe. But there is still case #1, where we have a real problem, and I will not accept your patch without seing this problem properly addressed.
And I said that we'll handle this by setting a new value of CONFIG_SYS_FDT_PAD. This will ensure that the initial memory allocation is large enough.
Technically speaking, even without my patch we already have the problem that bothers you. When boot_relocate_fdt() allocates the LMB, it gives it an additional buffer of 12KB. There's nothing stopping some code from calling fdt_setprop() and/or fdt_add_subnode() a thousand times, which will cause the fdt to grow outside of the allocated area.
Dear Timur Tabi,
In message AANLkTimFF-MM8jLicndervkWwRVpMQEeEuVd_RNTMxrY@mail.gmail.com you wrote:
And again, the point *I* am trying to make is that it's okay to put the onus of that check on the *caller* of fdt_increase_size(), and not on fdt_increase_size() itself.
OK. I have no problem with that. I am just missing this other part of the required changes in your patch.
I'm not ready to submit any code that calls fdt_increase_size() yet. I'm just trying to create the infrastructure here so that I can be sure that my in-house code is correct. If this patch is rejected because there's a better way to increase the size of an fdt, then I need to know that *now*.
I reject your patch because it introduces the risk of crashing the system and it appears you do not want to be bothered fixing this.
Maybe. But there is still case #1, where we have a real problem, and I will not accept your patch without seing this problem properly addressed.
And I said that we'll handle this by setting a new value of CONFIG_SYS_FDT_PAD. This will ensure that the initial memory allocation is large enough.
And I said that this is not sufficient. Static buffers have always been a bad idea as there will always be a user who needs more space. In this case, where we know the amount needed, we can as well arrange for it. You may have to add some code to implemenmt this, butthen adding this code has to be part of your patch submission.
Technically speaking, even without my patch we already have the problem that bothers you. When boot_relocate_fdt() allocates the LMB, it gives it an additional buffer of 12KB. There's nothing stopping some code from calling fdt_setprop() and/or fdt_add_subnode() a thousand times, which will cause the fdt to grow outside of the allocated area.
Right. We don't provide safety belts against doing stupid things. But you mentioned your extension is needed to insert firmware blobs which can take "significant size" - you cannot estimate in advace (at compile time) which size the end user may need. So using a static buffer size is a stupid thing too do as is is bound to fail rather sooner than later.
I will not accept code that relies on such compile time assumptions about dynamically inserted data.
[Note that it does not help that *you* *shout* *at* *me* all the time that you need this *now*. This does not make your patch any better.]
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
I reject your patch because it introduces the risk of crashing the system and it appears you do not want to be bothered fixing this.
But I believe I have already fixed it by stating that any users of fdt_increase_size() must ensure that the new size fits in the allocated area. The same rules apply to functions like strcat(), so I don't understand why you are so concerned.
And I said that this is not sufficient. Static buffers have always been a bad idea as there will always be a user who needs more space. In this case, where we know the amount needed, we can as well arrange for it. You may have to add some code to implemenmt this, butthen adding this code has to be part of your patch submission.
But there is no way to ensure that every user of fdt_increase_size() will follow those rules.
I could, for instance, add an lmb parameter to fdt_increase_size(), but this will only apply in instances where the fdt exists inside an lmb. There is no lmb_realloc() function, so the most I can do is return an error if the lmb isn't big enough.
The problem is that by the time the code needs to resize the fdt, it has long since forgotten about the lmb. We would need to modify ft_board_setup() to take an lmb as a parameter, but that would require changes to almost 30 boards!
And then, that doesn't help the situation where the fdt is just dumped in memory somewhere, like this:
tftp 0xc0000 my.dtb bootm xxx xxx 0xc0000
In this situation, no lmb is created, so there's no way to know if there's anything at address 0xc4000 that could be overwritten.
So the best I can do is this:
int fdt_increase_size(struct lmb *lmb, void *fdt, int add_len) { int newlen;
newlen = fdt_totalsize(fdt) + add_len;
if (lmb) { int lmb_size = lmb_size(lmb);
if (newlen > lmb_size) return -ENOMEM; }
/* Open in place with a new len */ return fdt_open_into(fdt, fdt, newlen); }
So if an lmb is provided, we can check for overwrites, but we can't require the lmb.
The only problem with the above function is that lmb_size() doesn't exist, and I'm not sure how to implement it yet. There doesn't appear to be any useful support functions for the lmb code at the moment.
Right. We don't provide safety belts against doing stupid things. But you mentioned your extension is needed to insert firmware blobs which can take "significant size" - you cannot estimate in advace (at compile time) which size the end user may need.
Actually, I can. For firmware, at least, I have macro called CONFIG_SYS_FSL_FW_MAXSIZE that defines the largest size that firmware can be. This is used not only to specify the new size of the lmb (if applicable), but to also ensure limits on the firmware itself. This would be a board-specific setting, since each board knows what kind of firmware it needs. And then I have this:
#ifndef CONFIG_SYS_FDT_PAD_DFLT #define CONFIG_SYS_FDT_PAD_DFLT 0x3000 #endif
#ifndef CONFIG_SYS_FMAN_FW_MAXSIZE #define CONFIG_SYS_FMAN_FW_MAXSIZE 0 #endif
#ifndef CONFIG_SYS_FDT_PAD #define CONFIG_SYS_FDT_PAD (CONFIG_SYS_FDT_PAD_DFLT + CONFIG_SYS_FMAN_FW_MAXSIZE) #endif
Thus, each board can define its own settings, and boot_relocate_fdt() will honor them.
So using a static buffer size is a stupid thing too do as is is bound to fail rather sooner than later.
AFAIK, lmb regions cannot be resized, so once we create an lmb region for the fdt, we're stuck with it. That's why I've enhanced boot_relocate_fdt() to ensure there's enough room in the lmb region to hold the firmware. I just haven't posted *that* code yet, since it's part of the P4080DS board support that isn't ready yet.
I will not accept code that relies on such compile time assumptions about dynamically inserted data.
I don't see any way around that.
The firmware is inserted after the lmb is created, and the code that creates the lmb region has no knowledge of the firmware (or any other board-specific entities that are going to be inserted into the fdt). The best I can do is force the firmware to be smaller than a compile-time limit, and ensure the lmb region is expanded by that value.
[Note that it does not help that *you* *shout* *at* *me* all the time that you need this *now*. This does not make your patch any better.]
Shouting is done with ALL CAPS. Framing words with asterisks is used to designate emphasis.
Dear Timur Tabi,
In message 4BF3FC6D.4000605@freescale.com you wrote:
But I believe I have already fixed it by stating that any users of fdt_increase_size() must ensure that the new size fits in the allocated area. The same rules apply to functions like strcat(), so I don't understand why you are so concerned.
I think the places where strcat() is used actually try and make sure to have enough room in the target buffer; also, almost all of themn appent static strigs with well-known sizes only.
You talked about inserting user-supplied data of unknown size.
This is a completely different thing.
But there is no way to ensure that every user of fdt_increase_size() will follow those rules.
There is always a way if you really try.
I could, for instance, add an lmb parameter to fdt_increase_size(), but this will only apply in instances where the fdt exists inside an lmb. There is no lmb_realloc() function, so the most I can do is return an error if the lmb isn't big enough.
You could add a lmb_realloc() function...
The problem is that by the time the code needs to resize the fdt, it has long since forgotten about the lmb. We would need to modify
You could add code so that this information gets stored in a suitable way.
ft_board_setup() to take an lmb as a parameter, but that would require changes to almost 30 boards!
There have been changes in the past that required changes to many, many more boards. What exactly is the problem with doing this?
And then, that doesn't help the situation where the fdt is just dumped in memory somewhere, like this:
Right. In this case the user is responsible. But that's an unrelated use case.
So if an lmb is provided, we can check for overwrites, but we can't require the lmb.
Eventually you can do something like lmb_realloc() as well.
The only problem with the above function is that lmb_size() doesn't exist,
Then add it?
and I'm not sure how to implement it yet. There doesn't appear to be any useful support functions for the lmb code at the moment.
Probably there was no need for these yet. Now there is.
Right. We don't provide safety belts against doing stupid things. But you mentioned your extension is needed to insert firmware blobs which can take "significant size" - you cannot estimate in advace (at compile time) which size the end user may need.
Actually, I can. For firmware, at least, I have macro called CONFIG_SYS_FSL_FW_MAXSIZE that defines the largest size that firmware can
Why would you add another static buffer size to excuse the limitations of another one?
Please expect me to reject a patch that adds CONFIG_SYS_FSL_FW_MAXSIZE with the same argumets we're discussing here.
applicable), but to also ensure limits on the firmware itself. This would be a board-specific setting, since each board knows what kind of firmware it needs. And then I have this:
A "board" cannot ever know what the user will be doing.
Thus, each board can define its own settings, and boot_relocate_fdt() will honor them.
This makes little or no sense to me.
When we load such firmware from some form of image, we know exactly what size if has. And we have mechanisms in place to use dynamically sized buffers. Use these, please.
I will not accept code that relies on such compile time assumptions about dynamically inserted data.
I don't see any way around that.
You know my opinion.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
I think the places where strcat() is used actually try and make sure to have enough room in the target buffer; also, almost all of themn appent static strigs with well-known sizes only.
You talked about inserting user-supplied data of unknown size.
Yes and no. The data is supplied by the user, but it will also be limited in size by a compile-time constant. In this case, the firmware should come only from Freescale, and the limit is double the size of current firmware, so that should be enough to handle all situations, now and in the future.
I could, for instance, add an lmb parameter to fdt_increase_size(), but this will only apply in instances where the fdt exists inside an lmb. There is no lmb_realloc() function, so the most I can do is return an error if the lmb isn't big enough.
You could add a lmb_realloc() function...
I'm not sure that would work in the context of the way lmb regions are currently being used. reallocating an lmb will probably require moving it as well, and the caller of lmb_realloc() might not be able to handle updated pointers.
The problem is that by the time the code needs to resize the fdt, it has long since forgotten about the lmb. We would need to modify
You could add code so that this information gets stored in a suitable way.
You make a lot of statements saying that I can do this and I can do that, but you provide no examples. Have you even looked at the code to see what such a change would require?
I believe that modifying the parameters for fdt_board_setup() is too intrusive of a change. So the only available spot where we could put the lmb pointers is in the bd_t structure. And my understanding is that this structure is already too fragile and should not be modified.
ft_board_setup() to take an lmb as a parameter, but that would require changes to almost 30 boards!
There have been changes in the past that required changes to many, many more boards. What exactly is the problem with doing this?
It's not worth it. It would be easier for me to take the code of fdt_increase_size() and just copy it into boot_relocate_fdt().
In fact, I think that's what I will do. I thought I was being polite by simply re-using an existing function, but I'd rather just copy the code into boot_relocate_fdt() if it means I don't have to argue with you about it.
Dear Timur Tabi,
In message 4BF4051A.1090202@freescale.com you wrote:
You make a lot of statements saying that I can do this and I can do that, but you provide no examples. Have you even looked at the code to see what such a change would require?
No, I haven't. I don't have to look at that code to see that what you are trying to do is wrong. It's not my task to provide a solution to ypur problem; all I want to make sure is we do not add code that is known to have problems.
In fact, I think that's what I will do. I thought I was being polite by simply re-using an existing function, but I'd rather just copy the code into boot_relocate_fdt() if it means I don't have to argue with you about it.
What makes you think such a patch would be accepted?
Don't try to wiggle out of the symptoms. Please solve the problem at the root.
I think he have sufficiently discussed this topic now?
Best regards,
Wolfgang Denk
So I tried to read this whole thread and got lost in the discussion. I'm wondering of something along the following lines would address your concerns:
#define CONFIG_SYS_FDT_PAD 0x3000
/* Allow for arch specific config before we boot */ int __fdt_board_size(void) { return CONFIG_SYS_FDT_PAD; } int fdt_board_size(void) __attribute__((weak, alias("__ fdt_board_size")));
int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base, char **of_flat_tree, ulong *of_size) { ...
if ((fdt_blob + *of_size + fdt_board_size()) >= ((char *)CONFIG_SYS_BOOTMAPSZ + bootmap_base)) relocate = 1;
... }
Than board specific code knows what fix ups might happen and can implement dynamic behavior and do something like:
int fdt_board_size(void) { determine_my_firmsize() + __fdt_board_size() }
- k
Dear Kumar Gala,
In message 52E9D06A-E721-4907-9024-11BDC8D006E0@kernel.crashing.org you wrote:
So I tried to read this whole thread and got lost in the discussion. I'm wondering of something along the following lines would address your concerns:
My concerns are simple: if we need to increase the size of the FDT because we pull in some random amountof data, we should make sure to have enough room for this, or fail with a clear error message.
And we should not try to use fixed sized buffers, but instead adapt to the actual amount required by the end user.
Timur assumes that all such code and it's sizess will be known in advance, and I disagree - other users will have other ideas what they can do with this, and his assumptions will break.
#define CONFIG_SYS_FDT_PAD 0x3000
Where exactly is this 0x3000 coming from?
/* Allow for arch specific config before we boot */ int __fdt_board_size(void) { return CONFIG_SYS_FDT_PAD; } int fdt_board_size(void) __attribute__((weak, alias("__ fdt_board_size")));
int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base, char **of_flat_tree, ulong *of_size) { ...
if ((fdt_blob + *of_size + fdt_board_size()) >= ((char *)CONFIG_SYS_BOOTMAPSZ + bootmap_base)) relocate = 1;... }
Than board specific code knows what fix ups might happen and can implement dynamic behavior and do something like:
If any board specific code can determine the needed sizes, then how would such code differ from one board to another?
Why would this in any way be a board specific implementation? This makes no sense to me. The feature to include some binary data into the DTB is IMO in no way dependent on or specific to a certain board.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Why would this in any way be a board specific implementation? This makes no sense to me. The feature to include some binary data into the DTB is IMO in no way dependent on or specific to a certain board.
The data I'm trying to embed is firmware for various devices on some of our SOCs, such as the QE on the MPC8360. Only boards with SOCs that have these devices come with firmware, and not all of them require the firmware to be passed to Linux.
Please note that fdt_increase_size() is just a front-end to fdt_open_into(), so technically I don't need to fdt_increase_size(). However, you said you would reject any patch that uses fdt_open_into() in this manner, so we're back to square one.
Dear Timur Tabi,
In message 4BF4623B.1080109@freescale.com you wrote:
Why would this in any way be a board specific implementation? This makes no sense to me. The feature to include some binary data into the DTB is IMO in no way dependent on or specific to a certain board.
The data I'm trying to embed is firmware for various devices on some of our SOCs, such as the QE on the MPC8360. Only boards with SOCs that have these devices come with firmware, and not all of them require the firmware to be passed to Linux.
Yes, I know all of this. This is your specific use case. But maybe you can take the blinkers off for a moment, and face up to other potential use cases as well?
User A might want to ambed a FPGA bit stream, user B something we don't even dream of yet.
Instead of implementing this feature in a way that makes it restricted to your current use case only we can as well make it generic enough so others can use it as well.
Please note that fdt_increase_size() is just a front-end to fdt_open_into(), so technically I don't need to fdt_increase_size(). However, you said you would reject any patch that uses fdt_open_into() in this manner, so we're back to square one.
Back to square one? I did not realize you ever left that position ;-)
Best regards,
Wolfgang Denk
On Thu, May 20, 2010 at 3:28 AM, Wolfgang Denk wd@denx.de wrote:
Dear Timur Tabi,
In message 4BF4623B.1080109@freescale.com you wrote:
Why would this in any way be a board specific implementation? This makes no sense to me. The feature to include some binary data into the DTB is IMO in no way dependent on or specific to a certain board.
The data I'm trying to embed is firmware for various devices on some of our SOCs, such as the QE on the MPC8360. Only boards with SOCs that have these devices come with firmware, and not all of them require the firmware to be passed to Linux.
Yes, I know all of this. This is your specific use case. But maybe you can take the blinkers off for a moment, and face up to other potential use cases as well?
Come on, Wolfgang. Why do you think I posted this patch in the first place? I even said so in the title of patch: "make fdt_increase_size() available to everyone".
You asked why the information would be board-specific, and I answered that question.
Now I believe you're intentionally trying to be difficult.
User A might want to ambed a FPGA bit stream, user B something we don't even dream of yet.
Which is exactly why I want fdt_increase_size() to be available to everyone.
Instead of implementing this feature in a way that makes it restricted to your current use case only we can as well make it generic enough so others can use it as well.
Exactly! And the best way to do that is to make the function that adds space to the fdt available to everyone.
Please note that fdt_increase_size() is just a front-end to fdt_open_into(), so technically I don't need to fdt_increase_size(). However, you said you would reject any patch that uses fdt_open_into() in this manner, so we're back to square one.
Back to square one? I did not realize you ever left that position ;-)
How silly of me. For a moment, I forgot that I was trying to improve U-Boot.
On Thu, May 20, 2010 at 3:28 AM, Wolfgang Denk wd@denx.de wrote:
Instead of implementing this feature in a way that makes it restricted to your current use case only we can as well make it generic enough so others can use it as well.
Wolfgang, can you tell me what method you would like me to use to pass the address and/or size of the firmware FIT image to boot_relocate_fdt()? Obviously, you don't like CONFIG_SYS_FDT_PAD, but I want to know what direction to go in before I start coding.
Dear Timur Tabi,
In message AANLkTikGuHbXqZYwCup04C2SCuB6Uof75zdoH-boZzUX@mail.gmail.com you wrote:
On Thu, May 20, 2010 at 3:28 AM, Wolfgang Denk wd@denx.de wrote:
Instead of implementing this feature in a way that makes it restricted to your current use case only we can as well make it generic enough so others can use it as well.
Wolfgang, can you tell me what method you would like me to use to pass the address and/or size of the firmware FIT image to boot_relocate_fdt()? Obviously, you don't like CONFIG_SYS_FDT_PAD, but I want to know what direction to go in before I start coding.
I'm not sure if I understand what you are trying to ask.
If you have an address and a size and want to pass these to a function, you can of course use two arguments to that function, say "phys_addr_t addr, size_t size".
But as this seems obvious I feel you might have something else in mind, yet I cannot figure out what it might be.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
But as this seems obvious I feel you might have something else in mind, yet I cannot figure out what it might be.
How would you prefer the user to be able to specify the address of the firmware FIT image, when he wants to boot Linux? I think I remember Kumar saying something about an environment variable, but I can't find that email any more.
Dear Timur Tabi,
In message 4BFC1075.5010508@freescale.com you wrote:
But as this seems obvious I feel you might have something else in mind, yet I cannot figure out what it might be.
How would you prefer the user to be able to specify the address of the firmware FIT image, when he wants to boot Linux? I think I remember Kumar saying something about an environment variable, but I can't find that email any more.
That was on IRC; here the relevant snippet:
(17:09:35) wdenk_: If the feature is enabled for a board, and if there is an entry "insert blob type xxx here", then common code should do that. (17:10:10) wdenk_: in plain old style it could be a 4th argument, or with FIT images it could be part of the image description. (17:11:46) wdenk_: or it could be an env variable, or ... (17:12:05) wdenk_: this is a design question then, and probably easy to solve. (17:12:45) galak: are we ok w/the use of an env variable to point to the blob? (17:13:16) galak: manly I'm trying to avoid moving to FIT images as that's a big churn on the user community (17:13:24) galak: s/manly/mainly/ (17:14:09) TimurTabi: it would be nice if we didn't have to wrap everything in a FIT image in order for U-boot to be able to use it. (17:14:52) TimurTabi: the QE firmware, for instance, already has a documented header that includes its size (17:15:06) galak: I'd say there is a point at which FIT images make sense (17:15:22) TimurTabi: as a hard requirement for this feature? (17:15:39) galak: not for this feature, but in general (17:16:08) galak: we need to be doing some tooling to make FIT usage as seamless as "old style/mkimage" is today (17:16:43) galak: I'd like to try and separate the two issues, and thus asking if an env var pointing to the address of the qe firmware in this case would be acceptable to wdenk1 (17:16:52) TimurTabi: how do we make the "add a blob" feature generic without using FIT images? (17:18:26) wdenk_: galak: please omit the "qe" part :-) It would be the address of a IH_TYPE_FIRMWARE image then, right? (17:20:11) galak: wdenk1: I'd say the address can point to that, but I think its still a getenv ("qe_fw_addr") (17:20:49) wdenk_: NAK. I want a _generic_ implementation, and "qe" is highly special. (17:21:32) wdenk_: I don't want to use "qe_something" for an Altera FPGA bitstream. (17:21:38) galak: wdenk1: agreed (17:22:02) wdenk_: fdt_fw_addr or so might be better. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (17:22:21) galak: I'd be ok with that ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (17:22:23) TimurTabi: wdenk1: so you want all such firmware to be wrapped in an IH_TYPE_FIRMWARE image so that we can know generically how large that image is, even thought we need board-specific code to embed that image into the fdt? (17:22:52) wdenk_: TimurTabi: we do not need any board-specific code for that. (17:23:12) TimurTabi: wdenk1: yes we do. There is a specific device tree binding for embedding QE firmware. (17:23:22) galak: its not board specific (17:23:35) wdenk_: if a board enables the feature in it's configuration, and if the variable is found in the env, then generic code will do what needs to be done. (17:26:03) galak: its firmware type specific, do we have anyway in the image hdr to determine QE firmware from Altera FPGA firmware? (17:26:56) wdenk_: TimurTabi: That was a silly thing to add to the DT spec. Now nobody else can use it. If I have a MPC823E and I need to load it with CPM microcode I cannot use this, because CPM microcode is not QEFW. If I have a FPGA bitstream I cannot use it because it is not QEFW. If I have a firmware blob for FOO I cannot use it because it's not QEFW. (17:28:00) TimurTabi: wdenk1: I don't think it's silly at all. The compatible property tells the linux driver whether the embedded firmware is for it or for something else (17:28:07) galak: there needs to be some bit of information to convey the intended purpose of the FW (17:28:28) wdenk_: galak: guess we don't have yet, but adding one specific type of firmware image and making it so special that nobody else who needs the same feature is just stupid. (17:29:15) TimurTabi: wdenk1: generic firmware really doesn't make much sense, to me. (17:29:58) galak: I agree, however there are few items that are specific. I think the vast majority of the code can be used by any firmware but we do need something to convey the intended user of the firmware (FPGA, QE, usb dev, etc.) (17:35:32) TimurTabi: wdenk1: so qe firmware needs to be embedded in the device tree differently than other firmware (17:37:05) wdenk_: add a type identifier, then (17:37:27) TimurTabi: wdenk1: I have. It's called the "compatible" property (17:38:25) TimurTabi: U-boot also needs to locate all the QE nodes in the device tree, and put pointers from those nodes to the firmware node (17:38:35) TimurTabi: so this can't be done generically (17:38:58) galak: thus my question of type or use of name in the env var to imply type (17:39:06) TimurTabi: this is the reason I posted the patch "libfdt: introduce function fdt_get_max_phandle" (17:39:32) wdenk_: you and your "can't be done". (17:39:43) TimurTabi: ok, "can't be done in a reasonable manner" (17:40:15) wdenk_: use the name field in the image, for example (17:40:45) TimurTabi: wdenk1: how do I know which nodes in the device tree should be updated? (17:40:56) TimurTabi: also, the binding says that the qe firmware node should be located inside the first qe node (17:41:17) TimurTabi: and that the other qe nodes should have phandles pointing to the firmware node in the first qe node (17:41:53) wdenk_: TimurTabi: I do not care about QE... (17:42:22) TimurTabi: wdenk1: I'm giving you an example of why we can't treat embedded firmware blobs in the device tree in a completely generic manner (17:42:49) TimurTabi: the process of putting the firmware in the device tree is specific to the type of firmware itself (17:43:20) wdenk_: sorry, gotta run now (17:43:30) TimurTabi: ok
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
That was on IRC; here the relevant snippet:
Thanks. Just to be clear, do you expect fdt_fw_addr always to point to a FIT-wrapped firmware binary?
(17:40:56) TimurTabi: also, the binding says that the qe firmware node should be located inside the first qe node (17:41:17) TimurTabi: and that the other qe nodes should have phandles pointing to the firmware node in the first qe node (17:41:53) wdenk_: TimurTabi: I do not care about QE... (17:42:22) TimurTabi: wdenk1: I'm giving you an example of why we can't treat embedded firmware blobs in the device tree in a completely generic manner (17:42:49) TimurTabi: the process of putting the firmware in the device tree is specific to the type of firmware itself (17:43:20) wdenk_: sorry, gotta run now (17:43:30) TimurTabi: ok
We never finished this discussion. My point was that even if the firmware is wrapped in a FIT image, the process by which the firmware is actually inserted into the device tree is specific to the actual firmware. You could even say it's board-specific.
In contrast, you want the fdt relocation code to be able to increase the size of the fdt without knowing any details about the firmware itself. Therefore, there will be two pieces of code that references fdt_fw_addr. The first is in boot_relocate_fdt(), which will extract the size information from the FIT image that fdt_fw_addr points to. The second is the QE code which extracts the firmware from the FIT image and embeds it into the device tree, in a QE-specific way.
I just want to make sure that we're on the same page.
Dear Timur Tabi,
In message 4BFC24DB.8070805@freescale.com you wrote:
Wolfgang Denk wrote:
That was on IRC; here the relevant snippet:
Thanks. Just to be clear, do you expect fdt_fw_addr always to point to a FIT-wrapped firmware binary?
Please re-read the IRC log. Kumar explicitly stated he was trying to avoid making FIT images mandatory, at least for now. And I explicitly wrote that it should be "the address of a IH_TYPE_FIRMWARE image then".
We never finished this discussion. My point was that even if the firmware is wrapped in a FIT image, the process by which the firmware is actually inserted into the device tree is specific to the actual firmware. You could even say it's board-specific.
You could say that. You could also say that 2+2=5.
I will argue in both cases.
In contrast, you want the fdt relocation code to be able to increase the size of the fdt without knowing any details about the firmware itself.
That's not correct. At least we know the address and the size. And if you follow my recommendation of using the name entry for type information (or come up with a better proposal) we also know the type.
Therefore, there will be two pieces of code that references fdt_fw_addr. The first is in boot_relocate_fdt(), which will extract the size information from the FIT image that fdt_fw_addr points to. The second is the QE code which extracts the firmware from the FIT image and embeds it into the device tree, in a QE-specific way.
I see no inherent problems with having a generic, common part for all systems enabling this feature, plus eventually hooks for (additional) customized processing of certain firmware image types.
Of course one can argue that making the decision on the type based on the name entry is a stupid thing, and come up for example with additional IH_TYPE entries; or even try to define subtypes. I think I'll leave this as an exercise to you :-)
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Thanks. Just to be clear, do you expect fdt_fw_addr always to point to a FIT-wrapped firmware binary?
Please re-read the IRC log. Kumar explicitly stated he was trying to avoid making FIT images mandatory, at least for now.
And he proposed a board-specific function that would allow this to work, but you rejected it. So I don't still know how to implement what you want.
And I explicitly wrote that it should be "the address of a IH_TYPE_FIRMWARE image then".
So you're saying fdt_fw_addr should pointer to either a FIT image or the older image type? What's the point in supporting the older type? Isn't it deprecated?
But either way, the firmware needs to be wrapped inside an image object. I think Kumar was implying that he didn't want to make *any* image type (FIT or legacy) mandatory.
We never finished this discussion. My point was that even if the firmware is wrapped in a FIT image, the process by which the firmware is actually inserted into the device tree is specific to the actual firmware. You could even say it's board-specific.
You could say that. You could also say that 2+2=5.
I will argue in both cases.
I don't understand your position. The method by which firmware is to be embedded in the device tree *is* specific to the kind of firmware in question, and therefore requires knowledge of the kind of firmware. A QE firmware is not embedded in the device tree the same way an FPGA firmware is. This is just a fact.
You keep telling me that there's a counter argument to this statement, but I don't know what it is. You just tell me you disagree. In effect, you are the one saying that 2+2=5.
In contrast, you want the fdt relocation code to be able to increase the size of the fdt without knowing any details about the firmware itself.
That's not correct. At least we know the address and the size.
Address and size is *not* details about the firmware itself. When I say "details about the firmware itself", I mean stuff like what kind of firmware it is, what chip it's for, what it's supposed to do, etc.
And if you follow my recommendation of using the name entry for type information (or come up with a better proposal) we also know the type.
Are you talking about the ih_name field in the image_header_t structure? So for instance, if the ih_name field says "QE Firmware", we can assume assume that it's a QE firmware, and the generic code should have something like this in it:
#include "qe.h"
if (strcmp(image->ih_name, "QE Firmware") == 0) { /* * Embed the firmware in the device tree using the binding * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe * /qe.txt */ }
Doesn't that seem really clunky to you? That requires the generic code to have knowledge of every type of firmware. Wouldn't it be simpler if we just followed Kumar's recommendation to have a board-specific __weak__ function that handled this code?
Therefore, there will be two pieces of code that references fdt_fw_addr. The first is in boot_relocate_fdt(), which will extract the size information from the FIT image that fdt_fw_addr points to. The second is the QE code which extracts the firmware from the FIT image and embeds it into the device tree, in a QE-specific way.
I see no inherent problems with having a generic, common part for all systems enabling this feature, plus eventually hooks for (additional) customized processing of certain firmware image types.
So you agree with Kumar's idea of having a weak function that embeds the firmware into the device tree, but the firmware must always be wrapped in an image format?
Of course one can argue that making the decision on the type based on the name entry is a stupid thing, and come up for example with additional IH_TYPE entries; or even try to define subtypes. I think I'll leave this as an exercise to you :-)
I'd rather not use subtypes, because I don't think we want anything like this:
if (is_qe_firmware()) { /* embed QE firmware */ } else if (is_amd_fpga_firmware()) { /* embed AMD fpga firmware */ } ...
Dear Timur Tabi,
In message 4BFD3A39.4090209@freescale.com you wrote:
Please re-read the IRC log. Kumar explicitly stated he was trying to avoid making FIT images mandatory, at least for now.
And he proposed a board-specific function that would allow this to work, but you rejected it. So I don't still know how to implement what you want.
Well, in a way that may be image-type dependent, but that is not board-specific.
And I explicitly wrote that it should be "the address of a IH_TYPE_FIRMWARE image then".
So you're saying fdt_fw_addr should pointer to either a FIT image or the older image type? What's the point in supporting the older type? Isn't it deprecated?
No, not really. It works fine for the intended purpose. Actually I still prefer it in a lot of cases because we have checksum protection of the header information, while you can have a totally corrupted DTB without really being able to detect it.
But either way, the firmware needs to be wrapped inside an image object. I think Kumar was implying that he didn't want to make *any* image type (FIT or legacy) mandatory.
And where would you then get type and size information from?
I don't understand your position. The method by which firmware is to be embedded in the device tree *is* specific to the kind of firmware in question, and therefore requires knowledge of the kind of firmware. A QE firmware is not embedded in the device tree the same way an FPGA firmware is. This is just a fact.
I also said that I see no problems with ading type specific hooks.
You keep telling me that there's a counter argument to this statement, but I don't know what it is. You just tell me you disagree. In effect, you are the one saying that 2+2=5.
Really?
In contrast, you want the fdt relocation code to be able to increase the size of the fdt without knowing any details about the firmware itself.
That's not correct. At least we know the address and the size.
Address and size is *not* details about the firmware itself. When I say
No, but they are important properties, for example when it comes to find out by how much the DT needs to be grown.
"details about the firmware itself", I mean stuff like what kind of firmware it is, what chip it's for, what it's supposed to do, etc.
Maybe we can abstrct off most of this, and/or leave it to image type specific handlers?
Are you talking about the ih_name field in the image_header_t structure? So for instance, if the ih_name field says "QE Firmware", we can assume assume that it's a QE firmware, and the generic code should have something like this in it:
#include "qe.h"
No. There would be no "qe.h" needed in that generic code.
if (strcmp(image->ih_name, "QE Firmware") == 0) { /* * Embed the firmware in the device tree using the binding * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe * /qe.txt */ }
No. More like
if (strcmp(image->ih_name, "QE Firmware") == 0) handle_fdt_fw_qe(image);
or similar. Probably #ifdef'ed for machines that have enabled QE support in their board config, or with a weak handle_fdt_fw_qe() that gets filled in for QE aware boards only so the compiler optimizes away that call everywhere else.
Doesn't that seem really clunky to you? That requires the generic code to have knowledge of every type of firmware. Wouldn't it be simpler if we just followed Kumar's recommendation to have a board-specific __weak__ function that handled this code?
D*mn. Don't you get it. There is NOT BOARD-SPECIFIC CODE anywhere.
It is feature specific. Either you enable QE support or not, but then the same code will be used for all boards enabling this feature.
I see no inherent problems with having a generic, common part for all systems enabling this feature, plus eventually hooks for (additional) customized processing of certain firmware image types.
So you agree with Kumar's idea of having a weak function that embeds the firmware into the device tree, but the firmware must always be wrapped in an image format?
Yes. Note that there is NOT any board-specific code.
Of course one can argue that making the decision on the type based on the name entry is a stupid thing, and come up for example with additional IH_TYPE entries; or even try to define subtypes. I think I'll leave this as an exercise to you :-)
I'd rather not use subtypes, because I don't think we want anything like this:
if (is_qe_firmware()) { /* embed QE firmware */ } else if (is_amd_fpga_firmware()) { /* embed AMD fpga firmware */ } ...
In which way would that be worse compared to
if (strcmp(image->ih_name, "QE Firmware") == 0) handle_fdt_fw_qe(image); else if (strcmp(image->ih_name, "AMD FPGA Firmware") == 0) handle_fdt_fw_amd(image); ... ?
Actually it would be easier to read...
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
And he proposed a board-specific function that would allow this to work, but you rejected it. So I don't still know how to implement what you want.
Well, in a way that may be image-type dependent, but that is not board-specific.
Technically, that's true, but in most cases the function that returns the address/size of the firmware would exist in board code.
For example, the MPC8323 has a bug in its QE UART support, so if you want to use the QE as a UART, you need to download a special QE firmware to work-around the bug. So even though all MPC8323 boards have a QE that can run in UART mode, only those boards that have a QE wired to an RS232 port need to enable the UART work-around, so only those boards would embed the firmware in the device tree.
So you're saying fdt_fw_addr should pointer to either a FIT image or the older image type? What's the point in supporting the older type? Isn't it deprecated?
No, not really. It works fine for the intended purpose. Actually I still prefer it in a lot of cases because we have checksum protection of the header information, while you can have a totally corrupted DTB without really being able to detect it.
Ok.
But either way, the firmware needs to be wrapped inside an image object. I think Kumar was implying that he didn't want to make *any* image type (FIT or legacy) mandatory.
And where would you then get type and size information from?
For example, QE firmware comes with its own header that includes size information, as well as a magic number and a CRC. See struct qe_firmware in qe.h. Since we distribute the QE firmware on our boards as-is, it would be nice if we didn't need to wrap those binaries inside a legacy/FIT image. So on an MPC8323E-MDS board, for example, the board-specific implementation of fdt_board_size() could support having fdt_fw_addr points directly to a QE firmware binary. We already have code that validates a QE firmware (see function qe_upload_firmware()), so this is safe.
For those firmware types that don't have such a header built-in, they would need to be wrapped in a legacy/FIT image.
I don't understand your position. The method by which firmware is to be embedded in the device tree *is* specific to the kind of firmware in question, and therefore requires knowledge of the kind of firmware. A QE firmware is not embedded in the device tree the same way an FPGA firmware is. This is just a fact.
I also said that I see no problems with ading type specific hooks.
Ok.
That's not correct. At least we know the address and the size.
Address and size is *not* details about the firmware itself. When I say
No, but they are important properties, for example when it comes to find out by how much the DT needs to be grown.
I agree, although I would say that only "size" is necessary.
"details about the firmware itself", I mean stuff like what kind of firmware it is, what chip it's for, what it's supposed to do, etc.
Maybe we can abstrct off most of this, and/or leave it to image type specific handlers?
I'm not sure that's a good idea. We can't predict what kind of firmware we'll need to support in the future, and a board-specific fdt_board_size() function is, in my opinion, a more elegant way to solve the problem.
#include "qe.h"
No. There would be no "qe.h" needed in that generic code.
if (strcmp(image->ih_name, "QE Firmware") == 0) { /* * Embed the firmware in the device tree using the binding * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe * /qe.txt */ }
No. More like
if (strcmp(image->ih_name, "QE Firmware") == 0) handle_fdt_fw_qe(image);
Where is the function prototype for handle_fdt_fw_qe()? Wouldn't it be in qe.h?
Also, I really think the strcmp is a bad idea, because then we have to put a specific string in the ih_name field, not something unique to the actual firmware in the image.
or similar. Probably #ifdef'ed for machines that have enabled QE support in their board config, or with a weak handle_fdt_fw_qe() that gets filled in for QE aware boards only so the compiler optimizes away that call everywhere else.
So it would look like board_init_r()?
Doesn't that seem really clunky to you? That requires the generic code to have knowledge of every type of firmware. Wouldn't it be simpler if we just followed Kumar's recommendation to have a board-specific __weak__ function that handled this code?
D*mn. Don't you get it. There is NOT BOARD-SPECIFIC CODE anywhere.
It is feature specific. Either you enable QE support or not, but then the same code will be used for all boards enabling this feature.
Ok.
I see no inherent problems with having a generic, common part for all systems enabling this feature, plus eventually hooks for (additional) customized processing of certain firmware image types.
So you agree with Kumar's idea of having a weak function that embeds the firmware into the device tree, but the firmware must always be wrapped in an image format?
Yes. Note that there is NOT any board-specific code.
Kumar's function would be defined in a board-specific source file. Using the QE firmware example for the 8323 above, I would add this code to board/freescale/mpc832xemds/mpc832xemds.c:
#include "qe.h"
int fdt_board_size(void) { struct qe_firmware *qefw = getenv("fdt_fw_addr");
if (!qefw) return 0;
if (!is_valid_qe_firmware(qefw)) return 0;
return qefw->header.length; }
I'd rather not use subtypes, because I don't think we want anything like this:
if (is_qe_firmware()) { /* embed QE firmware */ } else if (is_amd_fpga_firmware()) { /* embed AMD fpga firmware */ } ...
In which way would that be worse compared to
if (strcmp(image->ih_name, "QE Firmware") == 0) handle_fdt_fw_qe(image); else if (strcmp(image->ih_name, "AMD FPGA Firmware") == 0) handle_fdt_fw_amd(image); ... ?
It wouldn't be. I don't like either method.
I believe we should have a board-specific function that figures out how much extra space is needed, and just returns a single integer that the boot_relocate_fdt() uses to pad the FDT when it relocates it.
That board-specific function can do whatever it needs to do to figure out which firmware binaries need to be embedded, where they are, how big they are, and more importantly, what format they're in.
Later on, function ft_board_setup() will be the function that actually embeds the firmware into the device tree.
On Wed, May 26, 2010 at 11:38:27AM -0500, Timur Tabi wrote:
I believe we should have a board-specific function that figures out how much extra space is needed, and just returns a single integer that the boot_relocate_fdt() uses to pad the FDT when it relocates it.
Why don't we just grow the FDT on demand, after we make sure that it always lives someplace that is safely growable?
Or if we absolutely must resize it all at once, have a variable that contains the size required, which gets increased by whatever init code determines a reason for it (whether it be QE firmware in this environment variable, some other firmware in that environment variable, just a bunch of nodes that u-boot creates on this platform, etc).
-Scott
Scott Wood wrote:
On Wed, May 26, 2010 at 11:38:27AM -0500, Timur Tabi wrote:
I believe we should have a board-specific function that figures out how much extra space is needed, and just returns a single integer that the boot_relocate_fdt() uses to pad the FDT when it relocates it.
Why don't we just grow the FDT on demand, after we make sure that it always lives someplace that is safely growable?
We use lmb regions to allocate space for the fdt in the bootmap, so we need to know how big the lmb region should be before we allocate it. Resizing an lmb region will probably require moving it, and that might confuse the upper-level functions that expect the fdt pointer to remain constant.
Or if we absolutely must resize it all at once, have a variable that contains the size required, which gets increased by whatever init code determines a reason for it (whether it be QE firmware in this environment variable, some other firmware in that environment variable, just a bunch of nodes that u-boot creates on this platform, etc).
The issue is that how those functions will look. Kumar and I are advocating a board-specific weak function that figures it all out at once and returns a single number.
On 05/26/2010 12:56 PM, Timur Tabi wrote:
Scott Wood wrote:
On Wed, May 26, 2010 at 11:38:27AM -0500, Timur Tabi wrote:
I believe we should have a board-specific function that figures out how much extra space is needed, and just returns a single integer that the boot_relocate_fdt() uses to pad the FDT when it relocates it.
Why don't we just grow the FDT on demand, after we make sure that it always lives someplace that is safely growable?
We use lmb regions to allocate space for the fdt in the bootmap, so we need to know how big the lmb region should be before we allocate it.
But you can reasonably allocate significantly more than you'll need without actually causing the fdt to get that big. The actual cap could be a board specific magic number (like CONFIG_SYS_MALLOC_LEN), or we could cap it at something based on the amount of RAM.
Resizing an lmb region will probably require moving it, and that might confuse the upper-level functions that expect the fdt pointer to remain constant.
Or if we absolutely must resize it all at once, have a variable that contains the size required, which gets increased by whatever init code determines a reason for it (whether it be QE firmware in this environment variable, some other firmware in that environment variable, just a bunch of nodes that u-boot creates on this platform, etc).
The issue is that how those functions will look.
What functions? I'm just talking about arbitrary code that runs before the fdt is resized, and after relocation (or even before if we make it a gd_t variable), and adds a certain amount to a variable.
Kumar and I are advocating a board-specific weak function that figures it all out at once and returns a single number.
That seems an awkward combination of centralization (you need to combine every fdt-expanding feature found on a board into the board file) and decentralization (if such a feature is added or changed you may need to update a bunch of board files).
-Scott
Scott Wood wrote:
But you can reasonably allocate significantly more than you'll need without actually causing the fdt to get that big. The actual cap could be a board specific magic number (like CONFIG_SYS_MALLOC_LEN), or we could cap it at something based on the amount of RAM.
We have something like that already:
#ifndef CONFIG_SYS_FDT_PAD #define CONFIG_SYS_FDT_PAD 0x3000 #endif
And Wolfgang doesn't like it.
I was going to do this in my board header file:
#define CONFIG_SYS_MAX_QE_FW_SIZE 0x4000
#define CONFIG_SYS_FDT_PAD (0x3000 + CONFIG_SYS_MAX_QE_FW_SIZE)
And I was even going to add code to the QE firmware functions to verify that the firmware is not larger than CONFIG_SYS_MAX_QE_FW_SIZE.
The issue is that how those functions will look.
What functions? I'm just talking about arbitrary code that runs before the fdt is resized, and after relocation (or even before if we make it a gd_t variable), and adds a certain amount to a variable.
Actually, that would be *during* relocation because we relocate and resize the fdt in the same function: fdt_open_into().
Depending on the nature of the firmware, we may not know anything about the firmware until we try to boot Linux, so we need to decide where this arbitrary code is supposed to exist.
Kumar and I are advocating a board-specific weak function that figures it all out at once and returns a single number.
That seems an awkward combination of centralization (you need to combine every fdt-expanding feature found on a board into the board file) and decentralization (if such a feature is added or changed you may need to update a bunch of board files).
We could have feature-specific functions that do the work of handling the fdt size for a specific feature. So qe_firmware_size(void *) would tell you the size of the QE firmware, and your board's fdt_board_size() would just be a front-end to qe_firmware_size().
Are you suggesting that we implement some user interface where the user, on the command line, can specify any number and type of firmware binaries to get embedded in the device tree?
Dear Timur Tabi,
In message 4BFD6704.2040800@freescale.com you wrote:
We have something like that already:
#ifndef CONFIG_SYS_FDT_PAD #define CONFIG_SYS_FDT_PAD 0x3000 #endif
And Wolfgang doesn't like it.
Because nobody can explain where this magic number 0x3000 is coming from or why it is supposed to be sufficient.
I was going to do this in my board header file:
#define CONFIG_SYS_MAX_QE_FW_SIZE 0x4000
And I rejected this because it is pecific to just one use case (QE support) while I want a generic solution. Also, compile time size limits are always a bad thing if we can have run-time computed limits instead.
Are you suggesting that we implement some user interface where the user, on the command line, can specify any number and type of firmware binaries to get embedded in the device tree?
No. Such information should be part of the firmware image itself.
Best regards,
Wolfgang Denk
Dear Timur Tabi,
In message 4BFD4E83.2080202@freescale.com you wrote:
Well, in a way that may be image-type dependent, but that is not board-specific.
Technically, that's true, but in most cases the function that returns the address/size of the firmware would exist in board code.
For example, the MPC8323 has a bug in its QE UART support, so if you want to use the QE as a UART, you need to download a special QE firmware to work-around the bug. So even though all MPC8323 boards have a QE that can run in UART mode, only those boards that have a QE wired to an RS232 port need to enable the UART work-around, so only those boards would embed the firmware in the device tree.
Right. Then these boards can enable the feature in their board config files, and get what they want. But it's still generic code, and not spoecific to any special board.
But either way, the firmware needs to be wrapped inside an image object. I think Kumar was implying that he didn't want to make *any* image type (FIT or legacy) mandatory.
And where would you then get type and size information from?
For example, QE firmware comes with its own header that includes size information, as well as a magic number and a CRC. See struct qe_firmware in
But we want a generic solution, and this includes firmware blobs that may not contain such information. Ergo we use what we use elsewhere in U-Boot for the very purpose: U-Boot image files.
For those firmware types that don't have such a header built-in, they would need to be wrapped in a legacy/FIT image.
And for the sake of using common code we will do this always.
Maybe we can abstrct off most of this, and/or leave it to image type specific handlers?
I'm not sure that's a good idea. We can't predict what kind of firmware we'll need to support in the future, and a board-specific fdt_board_size() function is, in my opinion, a more elegant way to solve the problem.
You completely fail to understand what "board-specific" means. After so many rounds of discussion and so many attempts to explain this, you still don't get it. I'm on the verge of giving up.
This code is in no way board-specific, so board-specific functions make no sense.
Just swallow and accept this if you cannot understand it.
#include "qe.h"
No. There would be no "qe.h" needed in that generic code.
if (strcmp(image->ih_name, "QE Firmware") == 0) { /* * Embed the firmware in the device tree using the binding * definition in Documentation/powerpc/dts-bindings/fsl/cpm_qe * /qe.txt */ }
No. More like
if (strcmp(image->ih_name, "QE Firmware") == 0) handle_fdt_fw_qe(image);
Where is the function prototype for handle_fdt_fw_qe()? Wouldn't it be in qe.h?
Grrrrrghhhh!!!!!
Please open your eyes and start reading!!!
Just 15 lines above I wrote: "There would be no "qe.h" needed in that generic code." Then what good couldit do if we add a prototype to that header file that we need in common code? Nothing!
Obviously we need that prototype in a header file included by that common code, and this has NOTHING to do with any *qe*.h files.
Also, I really think the strcmp is a bad idea, because then we have to put a specific string in the ih_name field, not something unique to the actual firmware in the image.
Well, I said that this might need discussion - I offered image subtypes as an alternative, maybe there are other/better ideas. Input welcome.
or similar. Probably #ifdef'ed for machines that have enabled QE support in their board config, or with a weak handle_fdt_fw_qe() that gets filled in for QE aware boards only so the compiler optimizes away that call everywhere else.
So it would look like board_init_r()?
Not really. board_init_r() is still architecture specific - ARM and PowerPC and MIPS and ... use different implementations.
Here this is feature dependent, not architecture or CPU or board dependent code.
So you agree with Kumar's idea of having a weak function that embeds the firmware into the device tree, but the firmware must always be wrapped in an image format?
Yes. Note that there is NOT any board-specific code.
Kumar's function would be defined in a board-specific source file. Using
I don;t think so. It would not be accepted for mainline then.
the QE firmware example for the 8323 above, I would add this code to board/freescale/mpc832xemds/mpc832xemds.c:
Do you see my NAK? NAK!!!
In which way is this specific to the mpc832xemds board?
Would my custom 8323 board not need the same thing? Do you expect us to duplicate this code to all boards which need this QE feature?
In which way would that be worse compared to
if (strcmp(image->ih_name, "QE Firmware") == 0) handle_fdt_fw_qe(image); else if (strcmp(image->ih_name, "AMD FPGA Firmware") == 0) handle_fdt_fw_amd(image); ... ?
It wouldn't be. I don't like either method.
I'll happily accept more elegant / powerful approaches.
I believe we should have a board-specific function that figures out how much extra space is needed, and just returns a single integer that the boot_relocate_fdt() uses to pad the FDT when it relocates it.
That board-specific function can do whatever it needs to do to figure out which firmware binaries need to be embedded, where they are, how big they are, and more importantly, what format they're in.
Later on, function ft_board_setup() will be the function that actually embeds the firmware into the device tree.
Seems you really cannot look beyond your own nose, it seems.
Can't you ask somebody else to implement this for you?
Best regards,
Wolfgang Denk
On May 19, 2010, at 5:06 PM, Wolfgang Denk wrote:
Dear Kumar Gala,
In message 52E9D06A-E721-4907-9024-11BDC8D006E0@kernel.crashing.org you wrote:
So I tried to read this whole thread and got lost in the discussion. I'm wondering of something along the following lines would address your concerns:
My concerns are simple: if we need to increase the size of the FDT because we pull in some random amountof data, we should make sure to have enough room for this, or fail with a clear error message.
And we should not try to use fixed sized buffers, but instead adapt to the actual amount required by the end user.
Timur assumes that all such code and it's sizess will be known in advance, and I disagree - other users will have other ideas what they can do with this, and his assumptions will break.
#define CONFIG_SYS_FDT_PAD 0x3000
Where exactly is this 0x3000 coming from?
I think this was done to manage some growth of the dtb based on board fix ups. However, not sure where the number came from.
/* Allow for arch specific config before we boot */ int __fdt_board_size(void) { return CONFIG_SYS_FDT_PAD; } int fdt_board_size(void) __attribute__((weak, alias("__ fdt_board_size")));
int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base, char **of_flat_tree, ulong *of_size) { ...
if ((fdt_blob + *of_size + fdt_board_size()) >= ((char *)CONFIG_SYS_BOOTMAPSZ + bootmap_base)) relocate = 1;... }
Than board specific code knows what fix ups might happen and can implement dynamic behavior and do something like:
If any board specific code can determine the needed sizes, then how would such code differ from one board to another?
Why would this in any way be a board specific implementation? This makes no sense to me. The feature to include some binary data into the DTB is IMO in no way dependent on or specific to a certain board.
I agree this isn't 100% board specific, but my intent was to leave it under the board control. One assumes the board knows if it needs firmware blob A in dtb or not.
It follows the same logic as why we have ft_board_setup.
- k
Kumar Gala wrote:
int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base, char **of_flat_tree, ulong *of_size) { ...
if ((fdt_blob + *of_size + fdt_board_size()) >= ((char *)CONFIG_SYS_BOOTMAPSZ + bootmap_base)) relocate = 1;
Kumar, what do you think about having boot_relocate_fdt() always relocate the fdt, even if it is already in the bootmap? That would ensure that it gets resized and put into an lmb region.
Timur Tabi wrote:
The function fdt_increase_size() increases the size of the device tree by the given amount. This is useful for any code that wants to add a node or large property, to avoid the possibility of running out of space. It's much smarter to have U-Boot increase the size of device tree when it knows it's going to add data, instead of hoping that the DTS was compiled with the right -p value.
Signed-off-by: Timur Tabi timur@freescale.com
Acked-by: Gerald Van Baren vanbaren@cideas.com
common/fdt_support.c | 20 ++++++++++---------- include/fdt_support.h | 1 + 2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/common/fdt_support.c b/common/fdt_support.c index b6f252a..01d635b 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -645,6 +645,16 @@ int fdt_resize(void *blob) return actualsize; }
+int fdt_increase_size(void *fdt, int add_len) +{
- int newlen;
- newlen = fdt_totalsize(fdt) + add_len;
- /* Open in place with a new len */
- return fdt_open_into(fdt, fdt, newlen);
+}
#ifdef CONFIG_PCI #define CONFIG_SYS_PCI_NR_INBOUND_WIN 4
@@ -792,16 +802,6 @@ int fdt_del_subnodes(const void *blob, int parent_offset) return 0; }
-int fdt_increase_size(void *fdt, int add_len) -{
- int newlen;
- newlen = fdt_totalsize(fdt) + add_len;
- /* Open in place with a new len */
- return fdt_open_into(fdt, fdt, newlen);
-}
int fdt_del_partitions(void *blob, int parent_offset) { const void *prop; diff --git a/include/fdt_support.h b/include/fdt_support.h index 9a453af..d70627d 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -78,6 +78,7 @@ void ft_pci_setup(void *blob, bd_t *bd);
void set_working_fdt_addr(void *addr); int fdt_resize(void *blob); +int fdt_increase_size(void *fdt, int add_len);
int fdt_fixup_nor_flash_size(void *blob, int cs, u32 size);
participants (6)
-
Gerald Van Baren -
Jerry Van Baren
-
Kumar Gala -
Scott Wood -
Timur Tabi -
Wolfgang Denk