| Message ID | 1308094074-24898-1-git-send-email-davidb@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 14 Jun 2011, David Brown wrote: > Some targets have multiple memory regions. Parse up to 8 of these > when converting the atags to fdt. > > Signed-off-by: David Brown <davidb@codeaurora.org> I've added your patch to my zImage patch queue. > With this change, I am able to boot MSM8x60 combining ATAGS and my DT. > It seems to work as long as my device tree has placeholders for all of > the properties I need. I think those missing nodes should simply be created if missing. I however don't see any function in the libfdt API to do that -- there is fdt_add_subnode() but no fdt_add_node(). Any DT expert please? Also, should the DTB be enlarged in order to add new nodes, or even modify existing ones with larger properties? In other words, should something like fdt_move(fdt, fdt, fdt_totalsize(fdt)+HEADROOM) be used beforehand? Nicolas
On Wed, Jun 15 2011, Nicolas Pitre wrote: > On Tue, 14 Jun 2011, David Brown wrote: > >> Some targets have multiple memory regions. Parse up to 8 of these >> when converting the atags to fdt. >> >> Signed-off-by: David Brown <davidb@codeaurora.org> > > I've added your patch to my zImage patch queue. > >> With this change, I am able to boot MSM8x60 combining ATAGS and my DT. >> It seems to work as long as my device tree has placeholders for all of >> the properties I need. > > I think those missing nodes should simply be created if missing. I > however don't see any function in the libfdt API to do that -- there is > fdt_add_subnode() but no fdt_add_node(). Any DT expert please? It does seem to only work if I already have nodes that are large enough to hold the properties coming from the ATAG. For example, I have to have an array of memory reg values large enough, as well as a command line large enough to hold the one passed from the bootloader. If the DTB's command line is smaller than the one from ATAG, it just silently leaves the DTB one in place. David
On Wed, 15 Jun 2011, David Brown wrote: > On Wed, Jun 15 2011, Nicolas Pitre wrote: > > > On Tue, 14 Jun 2011, David Brown wrote: > > > >> Some targets have multiple memory regions. Parse up to 8 of these > >> when converting the atags to fdt. > >> > >> Signed-off-by: David Brown <davidb@codeaurora.org> > > > > I've added your patch to my zImage patch queue. > > > >> With this change, I am able to boot MSM8x60 combining ATAGS and my DT. > >> It seems to work as long as my device tree has placeholders for all of > >> the properties I need. > > > > I think those missing nodes should simply be created if missing. I > > however don't see any function in the libfdt API to do that -- there is > > fdt_add_subnode() but no fdt_add_node(). Any DT expert please? > > It does seem to only work if I already have nodes that are large enough > to hold the properties coming from the ATAG. For example, I have to > have an array of memory reg values large enough, as well as a command > line large enough to hold the one passed from the bootloader. > > If the DTB's command line is smaller than the one from ATAG, it just > silently leaves the DTB one in place. That I can explain, and that looks easy to fix. Nicolas
On Wed, Jun 15, 2011 at 03:50:37PM -0400, Nicolas Pitre wrote: > On Tue, 14 Jun 2011, David Brown wrote: > > > Some targets have multiple memory regions. Parse up to 8 of these > > when converting the atags to fdt. > > > > Signed-off-by: David Brown <davidb@codeaurora.org> > > I've added your patch to my zImage patch queue. > > > With this change, I am able to boot MSM8x60 combining ATAGS and my DT. > > It seems to work as long as my device tree has placeholders for all of > > the properties I need. > > I think those missing nodes should simply be created if missing. I > however don't see any function in the libfdt API to do that -- there is > fdt_add_subnode() but no fdt_add_node(). Any DT expert please? fdt_add_subnode() will create a new node as you require. The "subnode" is just supposed to indicate that the parameters are in the form of the offset of the parent node and the new node's immediate name, rather than taking a full path name for the new node. > Also, should the DTB be enlarged in order to add new nodes, or even > modify existing ones with larger properties? In other words, > should something like fdt_move(fdt, fdt, fdt_totalsize(fdt)+HEADROOM) be > used beforehand? Yes, you will need to do this, fdt_open_into() is the function you want. Without making room first, adding nodes or expanding existing properties will return -FDT_ERR_NOSPACE. Once you've made whatever edits you need, you can use fdt_pack() to collapse the tree back to its minimum size. Although this is somewhat awkward, this approach is a deliberate design decision, to avoid making libfdt dependent on a working general purpose allocator, which may not be available when libfdt is used in very limited environments such as a firmware/bootloader. If you do find things you need to do which libfdt doesn't support, I'm more than happy to extend it as necessary.
On Wed, Jun 15 2011, Nicolas Pitre wrote: > I think those missing nodes should simply be created if missing. I > however don't see any function in the libfdt API to do that -- there is > fdt_add_subnode() but no fdt_add_node(). Any DT expert please? > > Also, should the DTB be enlarged in order to add new nodes, or even > modify existing ones with larger properties? In other words, > should something like fdt_move(fdt, fdt, fdt_totalsize(fdt)+HEADROOM) be > used beforehand? I've tried adding some padding to my DTB by passing '-p 1024'. It does seem to try and do something, but I'm getting a fault in memmove() from fdt_splice() that I'm still trying to track down. It looks like the expansion is possible, though, at least once everything is right. David
On Thu, 16 Jun 2011, David Gibson wrote: > On Wed, Jun 15, 2011 at 03:50:37PM -0400, Nicolas Pitre wrote: > > On Tue, 14 Jun 2011, David Brown wrote: > > > > > Some targets have multiple memory regions. Parse up to 8 of these > > > when converting the atags to fdt. > > > > > > Signed-off-by: David Brown <davidb@codeaurora.org> > > > > I've added your patch to my zImage patch queue. > > > > > With this change, I am able to boot MSM8x60 combining ATAGS and my DT. > > > It seems to work as long as my device tree has placeholders for all of > > > the properties I need. > > > > I think those missing nodes should simply be created if missing. I > > however don't see any function in the libfdt API to do that -- there is > > fdt_add_subnode() but no fdt_add_node(). Any DT expert please? > > fdt_add_subnode() will create a new node as you require. The > "subnode" is just supposed to indicate that the parameters are in the > form of the offset of the parent node and the new node's immediate > name, rather than taking a full path name for the new node. Sure... but I'm still a total nincompoop with regard to FDT. Let's suppose I do: offset = fdt_path_offset(fdt, "/memory"); but that returns -FDT_ERR_NOTFOUND. Now what? If I look at the documentation for fdt_add_subnode(): /** * fdt_add_subnode - creates a new node * @fdt: pointer to the device tree blob * @parentoffset: structure block offset of a node * @name: name of the subnode to locate * [...] I have no node offset, and can't find the offset for the parent of an nonexistent node. Should I use 0 for parentoffset here? I'm guessing that "/memory" is at the top level so there is just no parent. > > Also, should the DTB be enlarged in order to add new nodes, or even > > modify existing ones with larger properties? In other words, > > should something like fdt_move(fdt, fdt, fdt_totalsize(fdt)+HEADROOM) be > > used beforehand? > > Yes, you will need to do this, fdt_open_into() is the function you > want. Without making room first, adding nodes or expanding existing > properties will return -FDT_ERR_NOSPACE. Once you've made whatever > edits you need, you can use fdt_pack() to collapse the tree back to > its minimum size. Excellent! FRom a quick code inspection, fdt_open_into() appears to be fine with both the source and destination pointers being the same, right? > Although this is somewhat awkward, this approach is a deliberate > design decision, to avoid making libfdt dependent on a working general > purpose allocator, which may not be available when libfdt is used in > very limited environments such as a firmware/bootloader. This is perfect. The environment where I want to use this code is fairly limited indeed. Nicolas
On Mon, Jun 20, 2011 at 12:03:21AM -0400, Nicolas Pitre wrote: > On Thu, 16 Jun 2011, David Gibson wrote: > > > On Wed, Jun 15, 2011 at 03:50:37PM -0400, Nicolas Pitre wrote: > > > On Tue, 14 Jun 2011, David Brown wrote: > > > > > > > Some targets have multiple memory regions. Parse up to 8 of these > > > > when converting the atags to fdt. > > > > > > > > Signed-off-by: David Brown <davidb@codeaurora.org> > > > > > > I've added your patch to my zImage patch queue. > > > > > > > With this change, I am able to boot MSM8x60 combining ATAGS and my DT. > > > > It seems to work as long as my device tree has placeholders for all of > > > > the properties I need. > > > > > > I think those missing nodes should simply be created if missing. I > > > however don't see any function in the libfdt API to do that -- there is > > > fdt_add_subnode() but no fdt_add_node(). Any DT expert please? > > > > fdt_add_subnode() will create a new node as you require. The > > "subnode" is just supposed to indicate that the parameters are in the > > form of the offset of the parent node and the new node's immediate > > name, rather than taking a full path name for the new node. > > Sure... but I'm still a total nincompoop with regard to FDT. > > Let's suppose I do: > > offset = fdt_path_offset(fdt, "/memory"); > > but that returns -FDT_ERR_NOTFOUND. Now what? > > If I look at the documentation for fdt_add_subnode(): > > /** > * fdt_add_subnode - creates a new node > * @fdt: pointer to the device tree blob > * @parentoffset: structure block offset of a node > * @name: name of the subnode to locate > * [...] > > I have no node offset, and can't find the offset for the parent of an > nonexistent node. Should I use 0 for parentoffset here? I'm guessing > that "/memory" is at the top level so there is just no parent. Ah! I see the source of your confusion. In this case the parent node is the root node, and the root node always has offset 0. I guess this needs to be better documented, although I'm not immediately sure where. > > > Also, should the DTB be enlarged in order to add new nodes, or even > > > modify existing ones with larger properties? In other words, > > > should something like fdt_move(fdt, fdt, fdt_totalsize(fdt)+HEADROOM) be > > > used beforehand? > > > > Yes, you will need to do this, fdt_open_into() is the function you > > want. Without making room first, adding nodes or expanding existing > > properties will return -FDT_ERR_NOSPACE. Once you've made whatever > > edits you need, you can use fdt_pack() to collapse the tree back to > > its minimum size. > > Excellent! > > FRom a quick code inspection, fdt_open_into() appears to be fine with > both the source and destination pointers being the same, right? Yes, that should be fine. It should also be fine with the new buffer partially overlapping the existing tree. > > Although this is somewhat awkward, this approach is a deliberate > > design decision, to avoid making libfdt dependent on a working general > > purpose allocator, which may not be available when libfdt is used in > > very limited environments such as a firmware/bootloader. > > This is perfect. The environment where I want to use this code is > fairly limited indeed. *smiles smugly*
diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c index 11c1a88..ac93e28 100644 --- a/arch/arm/boot/compressed/atags_to_fdt.c +++ b/arch/arm/boot/compressed/atags_to_fdt.c @@ -31,6 +31,8 @@ static int setprop_cell(void *fdt, const char *node_path, int atags_to_fdt(void *dt, void *atag_list) { struct tag *atag = atag_list; + uint32_t mem_reg_property[16]; + int memcount = 0; /* make sure we've got an aligned pointer */ if ((u32)atag_list & 0x3) @@ -51,11 +53,10 @@ int atags_to_fdt(void *dt, void *atag_list) setprop_string(dt, "/chosen", "bootargs", atag->u.cmdline.cmdline); } else if (atag->hdr.tag == ATAG_MEM) { - uint32_t mem_reg_property[2]; - mem_reg_property[0] = cpu_to_fdt32(atag->u.mem.start); - mem_reg_property[1] = cpu_to_fdt32(atag->u.mem.size); - setprop(dt, "/memory", "reg", mem_reg_property, - sizeof(mem_reg_property)); + if (memcount >= sizeof(mem_reg_property)/sizeof(uint32_t)) + continue; + mem_reg_property[memcount++] = cpu_to_fdt32(atag->u.mem.start); + mem_reg_property[memcount++] = cpu_to_fdt32(atag->u.mem.size); } else if (atag->hdr.tag == ATAG_INITRD2) { uint32_t initrd_start, initrd_size; initrd_start = atag->u.initrd.start; @@ -67,5 +68,10 @@ int atags_to_fdt(void *dt, void *atag_list) } } + if (memcount) { + setprop(dt, "/memory", "reg", mem_reg_property, + 4*memcount); + } + return 0; }
Some targets have multiple memory regions. Parse up to 8 of these when converting the atags to fdt. Signed-off-by: David Brown <davidb@codeaurora.org> --- With this change, I am able to boot MSM8x60 combining ATAGS and my DT. It seems to work as long as my device tree has placeholders for all of the properties I need. It still seems rather clunky, especially that it requires bootimg from a zImage. David arch/arm/boot/compressed/atags_to_fdt.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)