[coreboot] r593 -

[Peter Stuge]

I have issues with parts of this.


On Wed, Feb 13, 2008 at 10:00:21PM +0100, svn at coreboot.org wrote:
> M    include/device/path.h
> Add LPC path type, replacing SUPERIO path type, since SUPERIO is
> only one type of LPC. Clean up tabbing in parts of the file
> (cosmetic).

I think this needs more discussion.

Are all superios we want to support indeed LPC? Will that stay true?

Is the code generic enough to work for any LPC device?
(Flash chip, EC, custom hardware?)


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

Sorry, but I think this is bogus.

LPC can do IO, memory access, DMA and bus master transfers.

iobase seems to be a superio path, not a generic LPC path.


> M    northbridge/intel/i440bxemulation/dts
> Add ID info for the chip. 

>  {
>  	ramsize = "128";
>  	constructor = "i440bx_constructors";
> +	domainid = "0x8086, 0x7190";
>  };

Again, does this identify a new domain created by this device, or
does it identify this device within the containing domain? Can we
escape this ambiguity?


>  {
>  	ramsize = "128";
>  	constructor = "i440bx_constructors";
> +	domainid = "0x8086, 0x7190";
>  };


>  struct constructor i440bx_constructors[] = {
>  	{.id = {.type = DEVICE_ID_PCI_DOMAIN,
> +		.u = {.pci_domain = {.vendor = 0x8086,.device = 0x7190}}},


> +	 .ops = &i440bxemulation_pcidomainops},
>  	{.id = {.type = DEVICE_ID_PCI,
>  		.u = {.pci = {.vendor = 0x8086,.device = 0x7190}}},

These ids are repeated four times, that is at least two times too
many. Yes, this device id is both a PCI device and a domain, but
still.


> +	domain at 0 {

I'd kind of like to prepend pci_ here. "pci_domain"

>  		/config/("northbridge/intel/i440bxemulation/dts");
> +		bus at 0 {

And here. "pci_bus"


> +			pci at 0,0 {
> +			};
> +			pci at 1,0 {

And have these be devices of some sort so that they are not mistaken
for a peripheral component _interconnect_. (ie. the bus)


> -/*
> -#if 0

The cleanup is really great! But please make it a separate patch
next time.


//Peter