On Fri, Aug 17, 2012 at 05:00:46PM +0100, Peter Maydell wrote:
> 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.
True. Well, that change looks OK to me.
Anywhere we can reasonably avoid building in unnecessary constraints, we
should avoid it, I guess.
>
> >> + }
> >> }
> >> }
> >>
> >> @@ -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 :-)
Yup
> >> +
> >> + 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...
OK. It feels like they certainly should follow #address-cells. For
non-AArch64 platforms, we're dependent on at least enough low physical
RAM (or a low alias) to load the kernel -- in practice that means the
initrd should fit in it too. I guess we should just leave this patch
as-is for now.
Ultimately, for AArch64 it can make sense for there to be no RAM in the
bottom 4GB of physical address space, so I would be worried if the DT
bindings don't cope with this.
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
