On 17 August 2012 16:45, Dave Martin <dave.martin@xxxxxxxxxx> wrote:
> 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.
>> + if ((addrcells != 1 && addrcells != 2) ||
>> + (sizecells != 1 && sizecells != 2))
>
> We probably assume that sizecells is always == addrcells.
My intention was to make the patch handle any of the combinations,
though as you point out I missed a case [my suggested fix is below.]
>> @@ -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)
Yep, I missed this. Instead how about:
if(fdt32_to_cpu(p[addrcells]) != 0)
goto no_add_memory;
if(sizecells == 2 && fdt32_to_cpu(p[addrcells + 1]) != 0)
goto no_add_memory;
> If addr-cells is smaller than size-cells, someone needs their head
> examined. Hopefully that isn't legal in the device tree specification.
I don't think the dtb spec forbids it, partly because it's flexible
enough to allow for addresses which aren't memory addresses but
are instead weird things specifying bus addresses.
>> + }
>> }
>> }
>>
>> @@ -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.
Yes; laziness on my part but justifiable laziness :-)
>> +
>> + 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.
I have a query in with Grant to get him to make a decision here
and write some documentation...
>> if((e = fdt_setprop_cell(fdt, _chosen, "linux,initrd-start",
>> info->initrd_start)) < 0)
>> goto libfdt_error;
>> --
>> 1.7.9.5
>>
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
