Re: [PATCH 6/9] Introduce a simple iomap structure

[Christoffer Dall <christoffer.dall@linaro.org>]

On Thu, Jan 02, 2014 at 05:04:48PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:30:54PM -0800, Christoffer Dall wrote:
> > > +struct iomap {
> > > +	const char *type;
> > > +	const char *compats[5];
> > 
> > I would name the field compatible to be more in-line with the
> > DT-representation, but OK.
> > 
> > it looks from the above like the array must be null terminated?
> > 
> > how about #define IOMAP_MAX_COMPATS 5
> 
> defines are good.
> 
> > and then turn your loop above into:
> > 
> > for (i = 0; i < IOMAP_MAX_COMPATS; i++, m++) {
> > 	if (!m->compats[i])
> > 		break;
> 
> Doesn't improve the loop conditions much, IMO.
> 

I think it's much simpler to read a for loop of the typical form of
until "i < CONSTANT";  what you had before was a data structure with an
assumption of null-termination that could only be realized by looking at
the code that deals with it.

> > 	if (strcmp(m->compats[i], compat) == 0)
> > 		return m;
> > }
> > 
> > > +	u32 nr;
> > > +	u32 addrs[64];
> > 
> > why are we limiting ourselves to a 32 bit physical address space?
> 
> I have had this mentally noted from the beginning as something I'll
> need to deal with when getting aarch64 going. Anyway, I think a lot
> of this is going to change as I teach kvm-unit-tests more and more
> about DT.

Shouldn't be a big change to just support u64 in your iomap instead.

> 
> <snip>
> > > +print "{\n\t.type = NULL,\n},\n";
> > > +print "};\n";
> > > +exit 0;
> > 
> > This script doesn't work on either of the Ubuntu distros I run.  The
> > reasons are that the dumpfdt tool is not processing the multi-compatible
> > strings output from the dtb correctly and the fdtget utility included
> > does not yet have the -l option to list subnodes.
> > 
> > I spent a fair amount of time trying to fix this, changing dumpfdt to
> > 'dtc -I dtb -O dts', and I tried it on the newest Ubuntu distro, tried
> > compiling fdtget from the kernel sources etc. and failed miserably.
> > 
> > I think at the very least we need to check the tools available on the
> > build machine as part of the configure script or test it a little
> > broader.
> >
> 
> Drat. I've preferred the idea of using the tools to parse the DT during
> the build - avoiding the need for [cross-compiled] libfdt as a dep, but
> I guess as we need more and more from the DT, or when particular tests
> want to look at the DT itself, and there are already dependency problems,
> then it's looking more and more like we should just use libfdt.
> 
Yes, after working with this for a while that's the same conclusion I
came to.  From the ARM perspective, anything running in a VM will be
device-tree driven (this is not the time for anybody to say anything
about ACPI), so we could just feed the VM the device tree directly if we
support libfdt in the guest and avoid another point of failure.  I can
try to find some time to help you hack that up if you want.