[coreboot] Resource allocation

[Myles Watson]

> -----Original Message-----
> From: ron minnich [mailto:rminnich at gmail.com]
> Sent: Wednesday, November 12, 2008 11:55 PM
> To: Myles Watson
> Cc: Coreboot
> Subject: Re: [coreboot] Resource allocation
> 
> +				ioport at 2e {
> +
/config/("superio/winbond/w83627hf/dts");
> +					pnp at 2 {
> +
> 	/config/("superio/winbond/w83627hf/pnp.dts");
> +						enabled;
> +						io = "0x3f8";
> +						irq = "4";
> +					};
> +					pnp at 5 {
> +
> 	/config/("superio/winbond/w83627hf/pnp.dts");
> +						enabled;
> +						io = "0x60";
> +						io2 = "0x62";
> +						irq = "1";
> +						irq2 = "12";
> +					};
> +					pnp at a {
> +
> 	/config/("superio/winbond/w83627hf/pnp.dts");
> +						enabled;
> +						io = "0x290";
> +						irq = "5";
> +					};
> +				};
>  			};
> 
> 
> A few comments here. First, per the discussion in the other thread,
> it's not clear that people want to see all this dts stuff in the
> mainboard for each and every individual device.

I agree that having this in the w83627hf/dts would be better, but dtc
doesn't support that, right?  The best case for me would be having all of
the PNP structures in the SIO dts:

> +		pnp at 2 {
> +		 	/config/("superio/winbond/w83627hf/pnp.dts");
> +			enabled;
> +			io = "0x3f8";
> +			irq = "4";
> +		};
pnp at X all filled in...
> +		pnp at 5 {
> +			/config/("superio/winbond/w83627hf/pnp.dts");
> +			enabled;
> +			io = "0x60";
> +			io2 = "0x62";
> +			irq = "1";
> +			irq2 = "12";
> +		};
> +		pnp at a {
> +		 	/config/("superio/winbond/w83627hf/pnp.dts");
> +			enabled;
> +			io = "0x290";
> +			irq = "5";
> +		};

then being able to say in the mainboard dts:

> +				ioport at 2e {
> +
/config/("superio/winbond/w83627hf/dts");
> +					pnp at 2 {
> +						enabled;
> +					};
> +					pnp at 5 { /*KBD*/
> +						enabled;
> +					};
> +					pnp at 9 {
> +						disabled;
> +					};
> +					pnp at a {
> +						enabled;
> +					};


That makes it clear which devices get created (all the ones mentioned in the
dts.)  Then the SIO code can take care of special cases like devices that
need to be set even when they're disabled.

While I'm wishing I'd like to use pnp at W83627HF_KBC instead of pnp at 5 and have
that just work.  I think it might not be too hard, but it's a syntax error
now.  It would definitely reduce the chance for mistakes.

> Second, every device has an io and io2 now --even those that don't
> support it?

That's right.  The problem is that there needs to be some generic way to
pass this information to the resource code.  Right now it allocates a new
device for each of the SuperIO PNP functions, so there are dynamic devices
for all of them.  I think that there should only be dynamic devices for
things that get plugged in.

> Third, it's not clear that having a "generic" pnp is going
> to capture all the possible combinations; we tried this on
> v2 and it did not work out that well.

It would be easy to see a pnp structure that would work for a single
SuperIO.  It would also be easy for me to imagine a specific structure which
allowed setting one specific value in a device, and having the SuperIO code
populate that to pass into the PNP code.

> Fourth, I find this less
> readable than the old pnp:

I agree that this is more readable.  I don't think the code that associates
this information with the correct devices will be.  In the current code,
this information is all ignored, which simplifies it considerably.

> -	/* Floppy */
> -	floppydev = "0x0";
> -	floppyenable = "0";
> -	floppyio = "0x3f0";
> -	floppyirq = "0x60";
> -	floppydrq = "0x02";
> -
> -	/* Parallel port */
> -	ppdev = "2";
> -	ppenable = "0";
> -	ppio = "0x378";
> -	ppirq = "7";
> -
> -	/* COM1 */
> -	com1dev = "2";
> -	com1enable = "0";
> -	com1io = "0x3f8";
> -	com1irq = "4";
> -
> -	/* COM2 */
> -	com2dev = "3";
> -	com2enable = "0";
> -	com2io = "0x2f8";
> -	com2irq = "3";
> -
> -	/* Keyboard */
> -	kbdev = "5";
> -	kbenable = "0";
> -	kbio = "0x60";
> -	kbio2 = "0x62";
> -	kbirq = "1";
> -	kbirq2 = "12";
> -
> -	/* Consumer IR */
> -	cirdev = "6";
> -	cirenable = "0";
> -
> -	/* Game port */
> -	gamedev = "7";
> -	gameenable = "0";
> -	gameio = "0x220";
> -	gameio2 = "0x400";
> -	gameirq = "9";
> -
> -	/* GPIO2 */
> -	gpio2dev = "8";
> -	gpio2enable = "0";
> -
> -	/* GPIO3 */
> -	gpio3dev = "9";
> -	gpio3enable = "0";
> -
> -	/* ACPI */
> -	acpidev = "0xa";
> -	acpienable = "0";
> -
> -	/* Hardware Monitor */
> -	hwmdev = "0xb";
> -	hwmenable = "0";
> -	hwmio = "0x290";
> -	hwmirq = "5";
> 
> The naming is a lot more meaningful for me than the generic
> one-size-fits-all pnp stuff.

Yes.  It's very human readable.

> Also, note from the other thread on the 82801gx south, it almost seems
> people would prefer this one-dts-has-it-all form over the other form.

But it breaks the 1-to-1 dts to device model.

> Lotsa confusion out here with the dts right now.

Yes.

I appreciate the review.

Thanks,
Myles