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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, May 27, 2025 at 10:34:58AM +0200, Michael Tretter wrote:
> 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.

I was not aiming for merging the data in the nodes, more like: If that
node already exists, re-use it and overwrite the content with the data
passed to this function.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |