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

[Scott Wood]

On Wed, 8 Dec 2010 10:59:44 -0800
Deepak Saxena <deepak_saxena at 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 at 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