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

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

On Wed, Dec 04, 2013 at 05:42:54PM +0100, Andrew Jones wrote:
> Add a simple structure and search function to the common code that
> can be used for looking up a set of addresses by type. The user
> must supply the iomaps[] table, e.g.
> 
> struct iomap iomaps[] = {
> {
>     .type = "virtio_mmio",
>     .nr = 4,
>     .addrs = { 0x2000000, 0x2000200, 0x2000400, 0x2000600, },
> },
> {
>     .type = NULL,
> },
> };
> 
> Also add a script scripts/gen-devtree-iomaps.pl that can generate
> the iomaps table from an fdt, e.g.
> 
> fdtdump dtb | scripts/gen-devtree-iomaps.pl - virtio_mmio
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> ---
> v2:
>   - switch to kernel coding style
>   - rework fdt parsing to enable extraction of properties. Extract
>     the 'compatible' property.
>   - add iomaps_find_compatible() to allow searching for iomaps by
>     its 'compatible' property.
> ---
>  README                        |   1 +
>  lib/iomaps.c                  |  31 +++++++++++++
>  lib/iomaps.h                  |  14 ++++++
>  scripts/gen-devtree-iomaps.pl | 105 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 151 insertions(+)
>  create mode 100644 lib/iomaps.c
>  create mode 100644 lib/iomaps.h
>  create mode 100755 scripts/gen-devtree-iomaps.pl
> 
> diff --git a/README b/README
> index 0174679c05021..f834c61dc6112 100644
> --- a/README
> +++ b/README
> @@ -20,6 +20,7 @@ Directory structure:
>  .:		Makefile and config files for the tests
>  ./config:	config files for the tests
>  ./docs:		documentation files
> +./scripts:	misc helper scripts
>  ./lib:		general services for the tests
>  ./lib/<ARCH>:	architecture dependent services for the tests
>  ./<ARCH>:	the sources of the tests and the created objects/images
> diff --git a/lib/iomaps.c b/lib/iomaps.c
> new file mode 100644
> index 0000000000000..f2263b79dce96
> --- /dev/null
> +++ b/lib/iomaps.c
> @@ -0,0 +1,31 @@
> +#include "libcflat.h"
> +#include "iomaps.h"
> +
> +extern const struct iomap iomaps[];
> +
> +const struct iomap *iomaps_find_type(const char *type)
> +{
> +	const struct iomap *m = &iomaps[0];
> +
> +	while (m->type) {
> +		if (strcmp(m->type, type) == 0)
> +			return m;
> +		++m;
> +	}
> +	return NULL;
> +}
> +
> +const struct iomap *iomaps_find_compatible(const char *compat)
> +{
> +	const struct iomap *m = &iomaps[0];
> +	const char *c;
> +	int i;
> +
> +	while (m->type) {
> +		for (i = 0, c = m->compats[0]; c != NULL; c = m->compats[++i])
> +			if (strcmp(c, compat) == 0)
> +				return m;
> +		++m;
> +	}
> +	return NULL;
> +}
> diff --git a/lib/iomaps.h b/lib/iomaps.h
> new file mode 100644
> index 0000000000000..76a1aa4720337
> --- /dev/null
> +++ b/lib/iomaps.h
> @@ -0,0 +1,14 @@
> +#ifndef _IOMAPS_H_
> +#define _IOMAPS_H_
> +#include "libcflat.h"
> +
> +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
and then turn your loop above into:

for (i = 0; i < IOMAP_MAX_COMPATS; i++, m++) {
	if (!m->compats[i])
		break;
	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?

> +};
> +
> +const struct iomap *iomaps_find_type(const char *type);
> +const struct iomap *iomaps_find_compatible(const char *compat);
> +#endif
> diff --git a/scripts/gen-devtree-iomaps.pl b/scripts/gen-devtree-iomaps.pl
> new file mode 100755
> index 0000000000000..b48e85e48ab34
> --- /dev/null
> +++ b/scripts/gen-devtree-iomaps.pl
> @@ -0,0 +1,105 @@
> +#!/usr/bin/perl -w
> +use strict;
> +use File::Temp qw/:POSIX/;
> +
> +my $dts = shift @ARGV;
> +my @types = @ARGV;
> +my $max_nr_addrs = 64;
> +my $max_nr_compats = 4;
> +
> +if (!defined $dts || $#types < 0) {
> +	print STDERR "Usage: gen-devtree-iomaps ".
> +		"<dts-file|-> <addr-type> [addr-types...]\n";
> +	exit 1;
> +}
> +
> +my $dtb = tmpnam();
> +system "dtc -I dts -O dtb $dts -o $dtb";
> +
> +my $g = join '|', map { $_ . '@' } @types;
> +my @devs = grep { /$g/ } `fdtget -l $dtb / 2>/dev/null`;
> +
> +my %iomaps;
> +foreach my $dev (@devs) {
> +
> +	chomp($dev);
> +	my ($type, $addr) = split /@/, $dev;
> +
> +	if (!exists $iomaps{$type}) {
> +
> +		my $compatible = `fdtget $dtb /$dev compatible 2>/dev/null`;
> +		chomp($compatible);
> +		my @compats = split ' ', $compatible;
> +
> +		$iomaps{$type}{compats} = \@compats;
> +		$iomaps{$type}{addrs} = [$addr];
> +	} else {
> +		push @{ $iomaps{$type}{addrs} }, $addr;
> +	}
> +}
> +unlink $dtb;
> +
> +print <<EOF;
> +/*
> + * Generated file. See gen-devtree-iomaps.pl
> + */
> +#include "iomaps.h"
> +EOF
> +print "\nconst struct iomap iomaps[] = {\n";
> +foreach my $type (keys %iomaps) {
> +
> +	my $compats = $iomaps{$type}{compats};
> +	my $addrs = $iomaps{$type}{addrs};
> +
> +	my $nr_compats = $#{ $compats } + 1;
> +	if ($nr_compats > $max_nr_compats) {
> +		print STDERR "$type has $nr_compats compats, but iomaps can ".
> +			"only support up to $max_nr_compats.\n";
> +		splice @{ $compats }, $max_nr_compats;
> +	}
> +
> +	@{ $addrs } = sort @{ $addrs };
> +
> +	my $nr = $#{ $addrs } + 1;
> +	if ($nr > $max_nr_addrs) {
> +		print STDERR "$type has $nr addrs, but iomaps can ".
> +			"only support up to $max_nr_addrs.\n";
> +		$nr = $max_nr_addrs;
> +		splice @{ $addrs }, $nr;
> +	}
> +
> +	@{ $addrs } = map { $_ = sprintf '0x%.8x', hex($_) } @{ $addrs };
> +
> +	print "{\n";
> +	print "\t.type = \"$type\",\n";
> +
> +	print "\t.compats = {";
> +	foreach my $compat (@{ $compats }) {
> +		print " \"$compat\",";
> +	}
> +	print " NULL, },\n";
> +
> +	print "\t.nr = $nr,\n";
> +	print "\t.addrs = {";
> +	if ($nr < 5) {
> +		print ' ';
> +		print join ', ', @{ $addrs };
> +		print ", },\n";
> +	} else {
> +		print "\n";
> +		for (my $i = 0; $i < $nr; $i += 5) {
> +			print "\t\t";
> +			my $j = $i;
> +			while ($j < $i + 4 && $j < $nr - 1) {
> +				print $addrs->[$j] . ", ";
> +				++$j;
> +			}
> +			print $addrs->[$j] . ",\n";
> +		}
> +		print "\t},\n";
> +	}
> +	print "},\n";
> +}
> +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.

Thanks,
-- 
Christoffer