[coreboot] patch: more path support

Peter Stuge peter at stuge.se
Tue Feb 12 14:50:15 CET 2008


On Mon, Feb 11, 2008 at 08:03:11PM -0800, ron minnich wrote:
> So, instead of what we had before, a pathname property in 
> a node, e.g.
> pcipath = "0xf, 1";
> Now you label the node with the path, e.g.
> pci at 0xf,1
> 
> This results in a far cleaner and simpler dts.

I still think it is very confusing.

The path describes where within the parent node space this new node
is located, but the path is written "within" the new node.
(No, not within {} but after the node is created.)

This is confusing because the rest of the dts is hierarchical (good!)
(ordered top-down) and here the order is suddenly backwards;
information related to a higher level comes later in the file.


> +struct superio_path
> +{
> +	unsigned iobase;
> +};

I would also like to indicate the path type in the path somehow, to
avoid confusion. This will also allow higher resolution syntax
checking of dts files.

Maybe:

devfn:0,0 pci {}

io:2e superio {}

?

The paths are still somewhat confusing. It is not clear how to reach
the new node. devfn is particularly confusing since it might be
expected to only exist _within_ the PCI.


> +++ mainboard/emulation/qemu-x86/dts	(working copy)
> @@ -23,22 +23,15 @@
>  	mainboard-name = "QEMU x86";
>  	enabled;

Is this enable still required?


> +	domain at 0 {
>  		/config/("northbridge/intel/i440bxemulation/dts");
> +		bus at 0 {
> +			pci at 0,0 {
> +			};
> +			pci at 1,0 {
> +				/config/("southbridge/intel/i82371eb/dts");
> +			};
>  		};
>  	};
>  };

I like how this is turning out. Just that path thing..


> +++ northbridge/intel/i440bxemulation/dts	(working copy)
> @@ -21,4 +21,5 @@
>  {
>  	ramsize = "128";
>  	constructor = "i440bx_constructors";
> +	domainid = "0x8086, 0x7190";

This is the id of this component within a domain, or does it mean
that this component creates a new domain at that PCI id?

I really want to avoid this ambiguity.


> +	path = index(tree->name, '@');
> +	if (path && path[1]) {
> +		path++;
> +		if (!strncmp(tree->name, "cpu", 3)){

I would prefer comparing (path-tree->name) bytes instead of 3 to make
sure we can have both "bus" and "busmaster" nodes. (stupid example,
but you get the idea)


> +		if (!strncmp(tree->name, "lpc", 3)){
> +			fprintf(f, "\t.path = {.type=DEVICE_PATH_SUPERIO,.u={.superio={.iobase=%s}}},\n", 

Now this one isn't very clean. Which is it? lpc or superio? I don't
think all superios are on LPC.


The cleanup and the enabled/disabled change are great!


//Peter




More information about the coreboot mailing list