[U-Boot] [PATCH] Honor /memory/reg node in DTB files

Scott Wood
2010-12-08T19:11:12Z

On Wed, 8 Dec 2010 10:59:44 -0800
Deepak Saxena <deepak_saxena@mentor.com> wrote:

> On 12/07/2010 01:22 PM, Scott Wood wrote:
> > On Mon, 6 Dec 2010 16:56:26 -0800
> > Deepak Saxena <deepak_saxena@mentor.com> wrote:
> >
> >> +/*
> >> + * Check to see if an valid memory/reg property exists
> >> + * in the fdt. If so, we do not overwrite it with what's
> >> + * been scanned.
> >> + *
> >> + * Valid mean all the following:
> >> + *
> >> + * - Memory node has a device-type of "memory"
> >> + * - A reg property exists which:
> >> + * + has exactly as many cells as #address-cells + #size-cells
> >> + * + provides a range that is within [bi_memstart, bi_memstart +
> >> bi_memsize]
> >> + */
> >
> > This will get false positives -- a lot of existing device tree
> > templates have something like this:
>
> ACK. The code in the patch actually checks for this, just didn't point
> it out in the comments.

Ah, I looked for that and missed it. But there are also some boards
that have socketed RAM, but for some reason have a real memory size in
the device tree (corresponding to what the board ships with,
probably). I think this is something that should have to be selectable
by the board config (for image size reasons as well).

Also some nits:

> +static int ft_validate_memory(void *blob, bd_t *bd)
> +{
> + int nodeoffset;
> + u32 *addrcell = (u32*)fdt_getprop(blob, 0, "#address-cells", NULL);
> + u32 *sizecell = (u32*)fdt_getprop(blob, 0, "#size-cells", NULL);

const u32 *, and eliminate the cast.

> + u64 reg_base, reg_size;
> + void *reg, *dtype;
> + int len;
> +
> + if ((nodeoffset = fdt_path_offset(blob, "/memory")) >= 0)
> + {

Brace on same line as if

Don't put the assignment inside the if statement.

> + dtype = fdt_getprop(blob, nodeoffset, "device_type", &len);
> + if (!dtype || (strcmp(dtype, "memory") != 0))
> + return 0;
> +
> + reg = fdt_getprop(blob, nodeoffset, "reg", &len);
> + if (reg && len == ((*addrcell + *sizecell) * 4)) {
> + if (*addrcell == 2) {
> + reg_base = ((u64*)reg)[0];
> + reg_size = ((u64*)reg)[1];
> + } else {
> + reg_base = ((u32*)reg)[0];
> + reg_size = ((u32*)reg)[1];
> + }

This only works on big-endian.

> + if ((reg_size) &&
> + (reg_base >= (u64)bd->bi_memstart) &&
> + ((reg_size + reg_base)
> + <= ((u64)bd->bi_memstart + (u64)bd->bi_memsize)))
> + return 1;
> + }

Probably want to complain to the user if reg is invalid and not zero/missing.

-Scott