On Fri, Aug 17, 2012 at 04:22:03PM +0100, Peter Maydell wrote:
> ARM board device tree blobs have moved to specifying addresses and
> sizes as 64 bit values (2 cells) rather than 32 bit (1 cell); update
> bootwrapper so it can handle these rather than stopping with an error.
Aha, I was just asking Pawel whether he had this already :)
Looks about right, with a couple of niggles -- comments below.
Akira ran into this exact problem. We solved it temporarily by hacking
the device tree down to 32 bits, and so might be interested in testing
the finished patch with the original 64-bit device tree.
Cheers
---Dave
>
> Signed-off-by: Peter Maydell <peter.maydell@xxxxxxxxxx>
> ---
> semi_loader.c | 41 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/semi_loader.c b/semi_loader.c
> index 6677527..0237a44 100644
> --- a/semi_loader.c
> +++ b/semi_loader.c
> @@ -103,6 +103,7 @@ static void update_fdt(void **dest, struct loader_info *info)
> int _chosen;
> void *fdt;
> uint32_t const *p;
> + int addrcells, sizecells;
>
> if(!info->fdt_start)
> return;
> @@ -113,17 +114,21 @@ static void update_fdt(void **dest, struct loader_info *info)
>
> /*
> * Sanity-check address sizes, since addresses and sizes which do
> - * not take up exactly 4 bytes are not supported.
> + * not take up exactly 4 or 8 bytes are not supported.
> */
> {
> if(!(p = fdt_getprop(fdt, 0, "#address-cells", &e)))
> goto libfdt_error;
> - else if(e != 4 || fdt32_to_cpu(*p) != 1)
> + else if(e != 4)
> goto size_error;
> -
> + addrcells = fdt32_to_cpu(*p);
> if(!(p = fdt_getprop(fdt, 0, "#size-cells", &e)))
> goto libfdt_error;
> - else if(e != 4 || fdt32_to_cpu(*p) != 1)
> + else if(e != 4)
> + goto size_error;
> + sizecells = fdt32_to_cpu(*p);
> + if ((addrcells != 1 && addrcells != 2) ||
> + (sizecells != 1 && sizecells != 2))
We probably assume that sizecells is always == addrcells.
It looks like the code below assumes this. sizecells < addrcells
doesn't feel totally out of the question, but probably won't normally
happen. It would imply that you might need to slice up a nice big >=4GB
slab of RAM into little pieces, which would seem unfortunate.
If so, this should be changed to:
addrcells != 1 || addrcells != 2 || addrcells != sizecells
In this case, we should clarify the size_error error text to something like
"Unexpected/mismatched ..."
> goto size_error;
> }
>
> @@ -137,7 +142,7 @@ static void update_fdt(void **dest, struct loader_info *info)
> {
> int offset, depth = 0;
> int _memory;
> - uint32_t reg[2];
> + uint32_t reg[4];
>
> for(offset = fdt_next_node(fdt, 0, &depth); offset >= 0;
> offset = fdt_next_node(fdt, offset, &depth)) {
> @@ -153,8 +158,13 @@ static void update_fdt(void **dest, struct loader_info *info)
> if(e < 0)
> goto libfdt_error;
>
> - if(fdt32_to_cpu(p[1]) != 0)
> - goto no_add_memory;
> + if (sizecells == 1) {
> + if(fdt32_to_cpu(p[1]) != 0)
> + goto no_add_memory;
(assumes that addrcells == 1)
> + } else {
> + if(fdt32_to_cpu(p[2]) != 0 || fdt32_to_cpu(p[3]) != 0)
> + goto no_add_memory;
(assumes that addrcells == 2)
If addr-cells is smaller than size-cells, someone needs their head
examined. Hopefully that isn't legal in the device tree specification.
> + }
> }
> }
>
> @@ -162,9 +172,15 @@ static void update_fdt(void **dest, struct loader_info *info)
> goto libfdt_error;
> _memory = e;
>
> - reg[0] = cpu_to_fdt32(PHYS_OFFSET);
> - reg[1] = cpu_to_fdt32(PHYS_SIZE);
> - if((e = fdt_setprop(fdt, _memory, "reg", ®, sizeof reg)) < 0)
> + /* This assumes PHYS_OFFSET and PHYS_SIZE are 32 bits, though
> + * the fdt cells we put them in may not be.
> + */
> + reg[0] = reg[1] = reg[2] = reg[3] = 0;
> + reg[addrcells - 1] = cpu_to_fdt32(PHYS_OFFSET);
> + reg[addrcells + sizecells - 1] = cpu_to_fdt32(PHYS_SIZE);
I wonder wether we'll have to change that at some point... for now it
seems OK, though.
> +
> + if((e = fdt_setprop(fdt, _memory, "reg", ®,
> + sizeof(reg[0]) * (addrcells + sizecells))) < 0)
> goto libfdt_error;
>
> if((e = fdt_setprop_string(fdt, _memory, "device_type",
> @@ -186,7 +202,10 @@ no_add_memory:
>
> if(info->initrd_start) {
> uint32_t initrd_end = info->initrd_start + info->initrd_size;
> -
> + /* It's not documented whether these cells should honour
> + * #address-cells. Currently the kernel accepts them as being
> + * addresses of either size, so we leave them as 32 bits for now.
> + */
Interesting... OK.
> if((e = fdt_setprop_cell(fdt, _chosen, "linux,initrd-start",
> info->initrd_start)) < 0)
> goto libfdt_error;
> --
> 1.7.9.5
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
