On Wed, 8 Dec 2010 10:59:44 -0800
Deepak Saxena <deepak_sax...@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_sax...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
