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

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

On Thu, Jan 02, 2014 at 07:40:14PM +0100, Andrew Jones wrote:
> On Thu, Jan 02, 2014 at 09:23:38AM -0800, Christoffer Dall wrote:
> > 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.
> 
> I'll start this work right away. Peter's import libfdt idea sounds like
> the perfect solution.
> 
Agreed.

-Christoffer