Re: [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes

[Michael Tretter <m.tretter@pengutronix.de>]

On Tue, 27 May 2025 08:22:14 +0200, Sascha Hauer wrote:
> On Mon, May 26, 2025 at 04:38:12PM +0200, Michael Tretter wrote:
> > Board code in the PBL may use fdt_fixup_mem() to write the base
> > addresses and sizes of detected SDRAM to the fdt before passing the fdt
> > to other software like the TF-A and OP-TEE.
> > 
> > This is implemented on the fdt to avoid that the PBL needs to unpack the
> > device tree, use the of function for manipulation and repack the device
> > tree.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  include/pbl.h |  1 +
> >  pbl/fdt.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> > 
> > diff --git a/include/pbl.h b/include/pbl.h
> > index abac3458593af2cec972a54ce9fb69e344179670..b330010562c4aba5fccbb4c421bb95291fa0bea1 100644
> > --- a/include/pbl.h
> > +++ b/include/pbl.h
> > @@ -17,6 +17,7 @@ extern unsigned long free_mem_end_ptr;
> >  void pbl_barebox_uncompress(void *dest, void *compressed_start, unsigned int len);
> >  
> >  void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsize);
> > +int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num);
> >  
> >  struct fdt_device_id {
> >  	const char *compatible;
> > diff --git a/pbl/fdt.c b/pbl/fdt.c
> > index 3b1783152a8cb81f3eda1187b2f7bf998c4addf4..40564952acfcd88bef050966720e76b7378bc720 100644
> > --- a/pbl/fdt.c
> > +++ b/pbl/fdt.c
> > @@ -2,6 +2,7 @@
> >  #include <linux/libfdt.h>
> >  #include <pbl.h>
> >  #include <linux/printk.h>
> > +#include <stdio.h>
> >  
> >  static const __be32 *fdt_parse_reg(const __be32 *reg, uint32_t n,
> >  				   uint64_t *val)
> > @@ -73,6 +74,76 @@ void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsiz
> >  	while (1);
> >  }
> >  
> > +int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num)
> > +{
> > +	int na, ns;
> > +	int node, root;
> > +	int err;
> > +	int i;
> > +
> > +	err = fdt_check_header(fdt);
> > +	if (err != 0) {
> > +		pr_err("Invalid device tree blob\n");
> > +		return err;
> > +	}
> > +
> > +	root = fdt_path_offset(fdt, "/");
> > +	if (root < 0) {
> > +		pr_err("Cannot find root node: %s\n", fdt_strerror(root));
> > +		return root;
> > +	}
> > +
> > +	na = fdt_address_cells(fdt, root);
> > +	if (na < 0) {
> > +		pr_err("Cannot find #address-cells property: %s\n",
> > +		       fdt_strerror(na));
> > +		return na;
> > +	}
> > +
> > +	ns = fdt_size_cells(fdt, root);
> > +	if (ns < 0) {
> > +		pr_err("Cannot find #size-cells property: %s\n",
> > +				fdt_strerror(ns));
> > +		return ns;
> > +	}
> 
> fdt_appendprop_addrrange() handles #address-cells and #size-cells
> internally, consequently na and ns are not used in this function.

That's a leftover of a previous implementation. I'll drop it.

> 
> > +
> > +	for (i = 0; i < num; i++) {
> > +		char name[32];
> > +
> > +		if (membase[i] == 0 || memsize[i] == 0)
> > +			continue;
> > +
> > +		snprintf(name, sizeof(name), "memory@%lx", membase[i]);
> > +		node = fdt_add_subnode(fdt, root, name);
> 
> There are some pitfalls in this function when the device tree already
> has memory nodes. It seems fdt_add_subnode() returns -FDT_ERR_EXISTS
> in case the node already exists. You should likely check that.

In this case, the error handling gets even more complex. If the device
tree node already exists and the size specified in the node doesn't
match the size passed to the function, the function has to decide which
value should get precedence. Probably the value passed to the function.

In this case, fdt_fixup_mem() will be able to fixup existing nodes and
add new nodes if the nodes do not exist.

If fdt_fixup_mem() is able to fixup existing nodes, it should really try
hard to find the correct existing node, which will add more complexity.
This needs some tests.

However, I'm not sure if we really want this complexity in the PBL.

> 
> Some device trees have a plain "memory" node without the @address
> postfix. These should be deleted before adding other memory nodes.

OK. That's similar to what of_memory_fixup is doing. I'll add this.

> 
> > +		if (node < 0) {
> > +			pr_err("Cannot to add node %s: %s\n",
> > +			       name, fdt_strerror(node));
> > +			err = node;
> > +			break;
> > +		}
> > +
> > +		pr_debug("Add memory node %s (0x%lx, size 0x%lx)\n",
> > +			 name, membase[i], memsize[i]);
> > +
> > +		err = fdt_setprop(fdt, node,
> > +				  "device_type", "memory", sizeof("memory"));
> > +		if (err < 0) {
> > +			pr_err("%s: Cannot set device_type: %s\n",
> > +			       name, fdt_strerror(err));
> > +			return err;
> > +		}
> > +
> > +		err = fdt_appendprop_addrrange(fdt, root, node, "reg", membase[i], memsize[i]);
> 
> This sounds like it appends the range to ranges already existing in the
> node. Do you have to call fdt_delprop(..., "reg") beforehand?

Right now, this is not necessary, because the node is a newly created
node and doesn't have the "reg" property. If the function is changed to
update existing nodes, the property probably has to be cleared.

Michael