-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Wednesday, November 12, 2008 11:55 PM To: Myles Watson Cc: Coreboot Subject: Re: [coreboot] Resource allocation
ioport@2e {
/config/("superio/winbond/w83627hf/dts");
pnp@2 {
- /config/("superio/winbond/w83627hf/pnp.dts");
enabled;
io = "0x3f8";
irq = "4";
};
pnp@5 {
- /config/("superio/winbond/w83627hf/pnp.dts");
enabled;
io = "0x60";
io2 = "0x62";
irq = "1";
irq2 = "12";
};
pnp@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@2 {
/config/("superio/winbond/w83627hf/pnp.dts");
enabled;
io = "0x3f8";
irq = "4";
};
pnp@X all filled in...
pnp@5 {
/config/("superio/winbond/w83627hf/pnp.dts");
enabled;
io = "0x60";
io2 = "0x62";
irq = "1";
irq2 = "12";
};
pnp@a {
/config/("superio/winbond/w83627hf/pnp.dts");
enabled;
io = "0x290";
irq = "5";
};
then being able to say in the mainboard dts:
ioport@2e {
/config/("superio/winbond/w83627hf/dts");
pnp@2 {
enabled;
};
pnp@5 { /*KBD*/
enabled;
};
pnp@9 {
disabled;
};
pnp@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@W83627HF_KBC instead of pnp@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