On Tue, Nov 11, 2008 at 3:08 PM, Myles Watson mylesgw@gmail.com wrote:
resources. For example:
PNP 2e.0: size 8 align 3 gran 3 limit 7ff flags 100 index 60which is the floppy device has this resource definition { &w83627hf_ops, W83627HF_FDC, PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, { 0x07f8, 0}, }, and this dts entry /* Floppy */ floppydev = "0x0"; floppyenable = "0"; floppyio = "0x3f0"; floppyirq = "0x60"; floppydrq = "0x02";
So you're saying it should be a fixed resource size 8 base 0x3f0?
Yes, if it were enabled. I don't know what the device code does if it is disabled.
Good point. I looked, and this doesn't ever get checked. All these devices show up as dynamic devices and lose this information. I guess some phase3 scan function needs to be implemented that takes this into account.
Thanks, Myles
static void phase3_chip_setup_dev(struct device *dev) { - pnp_enable_devices(dev, &w83627hf_ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info); + struct superio_winbond_w83627hf_dts_config *conf; + conf = dev->device_configuration; + + if (conf->com1enable) + pnp_enable_devices(dev, &w83627hf_ops, 1, &pnp_dev_info[2]); + if (conf->com2enable) + pnp_enable_devices(dev, &w83627hf_ops, 1, &pnp_dev_info[3]); + if (conf->kbenable) + pnp_enable_devices(dev, &w83627hf_ops, 1, &pnp_dev_info[4]); + if (conf->hwmenable) + pnp_enable_devices(dev, &w83627hf_ops, 1, &pnp_dev_info[9]); }
Are we thinking something like this? Something different? I don't understand how we were hoping those dts values would make it into devices.
Thanks, Myles
resources. For example: PNP 2e.0: size 8 align 3 gran 3 limit 7ff flags 100 index 60 which is the floppy device has this resource definition { &w83627hf_ops, W83627HF_FDC, PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, { 0x07f8, 0}, }, and this dts entry /* Floppy */ floppydev = "0x0"; floppyenable = "0"; floppyio = "0x3f0"; floppyirq = "0x60"; floppydrq = "0x02";
So you're saying it should be a fixed resource size 8 base 0x3f0?
Yes, if it were enabled. I don't know what the device code does if it is disabled.
Good point. I looked, and this doesn't ever get checked. All these devices show up as dynamic devices and lose this information. I guess some phase3 scan function needs to be implemented that takes this into account.
static void phase3_chip_setup_dev(struct device *dev) {
pnp_enable_devices(dev, &w83627hf_ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info);
struct superio_winbond_w83627hf_dts_config *conf;conf = dev->device_configuration;if (conf->com1enable)pnp_enable_devices(dev, &w83627hf_ops, 1, &pnp_dev_info[2]);if (conf->com2enable)pnp_enable_devices(dev, &w83627hf_ops, 1, &pnp_dev_info[3]);if (conf->kbenable)pnp_enable_devices(dev, &w83627hf_ops, 1, &pnp_dev_info[4]);if (conf->hwmenable)pnp_enable_devices(dev, &w83627hf_ops, 1, &pnp_dev_info[9]);}
Are we thinking something like this? Something different? I don't understand how we were hoping those dts values would make it into devices.
This would be a good reason to have a generic enable for each device like in v2. Then the enable function can handle anything. I am not sure how or why this changed in v3.
Marc
On Tue, Nov 11, 2008 at 2:58 PM, Marc Jones marcj303@yahoo.com wrote:
This would be a good reason to have a generic enable for each device like in v2. Then the enable function can handle anything. I am not sure how or why this changed in v3.
I thought you and I just finished getting this right. It's called phase3_setup_chip_dev. Is this not right? It does what the v2 thing did IIRC.
ron
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Tuesday, November 11, 2008 4:13 PM To: Marc Jones Cc: Myles Watson; Coreboot Subject: Re: [coreboot] Resource allocation
On Tue, Nov 11, 2008 at 2:58 PM, Marc Jones marcj303@yahoo.com wrote:
This would be a good reason to have a generic enable for each device
like in v2. Then the enable function can handle anything. I am not sure how or why this changed in v3.
I thought you and I just finished getting this right. It's called phase3_setup_chip_dev. Is this not right? It does what the v2 thing did IIRC.
It goes through and allocates new devices and resources for each PNP device. It doesn't read any of the dts information. That's the missing part.
I think I could add more fields to the struct that gets passed to the chip enable and populate them from the dts. I just wasn't sure how you were thinking it would work.
Thanks, Myles
On Tue, Nov 11, 2008 at 8:24 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Tuesday, November 11, 2008 4:13 PM To: Marc Jones Cc: Myles Watson; Coreboot Subject: Re: [coreboot] Resource allocation
On Tue, Nov 11, 2008 at 2:58 PM, Marc Jones marcj303@yahoo.com wrote:
This would be a good reason to have a generic enable for each device
like in v2. Then the enable function can handle anything. I am not sure how or why this changed in v3.
I thought you and I just finished getting this right. It's called phase3_setup_chip_dev. Is this not right? It does what the v2 thing did IIRC.
It goes through and allocates new devices and resources for each PNP device. It doesn't read any of the dts information. That's the missing part.
I think I could add more fields to the struct that gets passed to the chip enable and populate them from the dts. I just wasn't sure how you were thinking it would work.
The other way to do it would be to add the PNP devices back into the dts. I personally think that's the way to go. If we put PNP devices around the configuration it won't be so huge, and the old PNP functions will work better again.
Was it deliberate that the devices disappeared?
Thanks,
Myles
On Tue, Nov 11, 2008 at 7:40 PM, Myles Watson mylesgw@gmail.com wrote:
Was it deliberate that the devices disappeared?
I am missing context again. Do you mean that the pnp devices disappeared from dts or ...
ron
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Tuesday, November 11, 2008 9:12 PM To: Myles Watson Cc: Marc Jones; Coreboot Subject: Re: [coreboot] Resource allocation
On Tue, Nov 11, 2008 at 7:40 PM, Myles Watson mylesgw@gmail.com wrote:
Was it deliberate that the devices disappeared?
I am missing context again. Do you mean that the pnp devices disappeared from dts or ...
Yes. Instead of having each PNP device in the SuperIO be its own device in the dts, there is just one ioport device. That's why all of the PNP devices get created as dynamic devices.
Thanks, Myles
Myles Watson wrote:
Yes. Instead of having each PNP device in the SuperIO be its own device in the dts,
In which dts?
there is just one ioport device. That's why all of the PNP devices get created as dynamic devices.
I would like the superio dts to express the hardware and have all LDNs. For simplicity I think it makes sense to have a complete static definition of superios in dts and not have to do any dynamic probing of them in a chip that will never change anyway.
The board dts must then say which of these LDNs are actually implemented. alix1c has no parallell port for example, but has GPIOs. Desktop boards have some other ports, but not GPIOs.
Dynamic superio probing only makes any sense to me if it can be used verbatim on each and every superio, which I don't think is the case.. Is that false?
//Peter
On Wed, Nov 12, 2008 at 7:39 AM, Peter Stuge peter@stuge.se wrote:
Myles Watson wrote:
Yes. Instead of having each PNP device in the SuperIO be its own device in the dts,
In which dts?
there is just one ioport device. That's why all of the PNP devices get created as dynamic devices.
I would like the superio dts to express the hardware and have all LDNs. For simplicity I think it makes sense to have a complete static definition of superios in dts and not have to do any dynamic probing of them in a chip that will never change anyway.
The board dts must then say which of these LDNs are actually implemented. alix1c has no parallell port for example, but has GPIOs. Desktop boards have some other ports, but not GPIOs.
Dynamic superio probing only makes any sense to me if it can be used verbatim on each and every superio, which I don't think is the case.. Is that false?
yes, but having a single superio device structure in the static tree is what requires dynamic devices. We're not probing. We are actually just creating devices on demand. Those are two different things.
Plus, the weird thing on a superio: even if you don't want a function (e.g. serial) it's important to make sure you turn certain pnp functions off in some cases -- so you may want that pnp function controlled, even if you do not use it.
Remember the rule: one dts -> one device.
So we have one superio device. To create a real device for each of the superio pnp functions, the code does this: void pnp_enable_devices(struct device *base_dev, struct device_operations *ops, unsigned int functions, struct pnp_info *info) { struct device_path path; struct device_id id = {.type = DEVICE_ID_PNP }; struct device *dev; int i;
path.type = DEVICE_PATH_PNP; path.pnp.port = base_dev->path.pnp.port;
/* Setup the ops and resources on the newly allocated devices. */ for (i = 0; i < functions; i++) { path.pnp.device = info[i].function; dev = alloc_find_dev(base_dev->bus, &path, &id);
so we get a new dynamic device for each pnp function.
Making one device for each pnp function, we will need one dts for each pnp function. This is very easy to do, although it will be a bit of work. But now is the time to do it if we want to do it. I am not convinced we want to do it.
I'd rather put the effort into getting the k8 to work, but I don't see how this is blocking the k8.
ron
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of ron minnich Sent: Wednesday, November 12, 2008 9:12 AM To: Coreboot Subject: Re: [coreboot] Resource allocation
On Wed, Nov 12, 2008 at 7:39 AM, Peter Stuge peter@stuge.se wrote:
Myles Watson wrote:
Yes. Instead of having each PNP device in the SuperIO be its own device in the dts,
In which dts?
there is just one ioport device. That's why all of the PNP devices get created as dynamic devices.
I would like the superio dts to express the hardware and have all LDNs. For simplicity I think it makes sense to have a complete static definition of superios in dts and not have to do any dynamic probing of them in a chip that will never change anyway.
The board dts must then say which of these LDNs are actually implemented. alix1c has no parallell port for example, but has GPIOs. Desktop boards have some other ports, but not GPIOs.
Dynamic superio probing only makes any sense to me if it can be used verbatim on each and every superio, which I don't think is the case.. Is that false?
yes, but having a single superio device structure in the static tree is what requires dynamic devices. We're not probing. We are actually just creating devices on demand. Those are two different things.
Plus, the weird thing on a superio: even if you don't want a function (e.g. serial) it's important to make sure you turn certain pnp functions off in some cases -- so you may want that pnp function controlled, even if you do not use it.
Remember the rule: one dts -> one device.
I forgot the rule. I'm down the path of creating static devices for each PNP device. :(
So we have one superio device. To create a real device for each of the superio pnp functions, the code does this: void pnp_enable_devices(struct device *base_dev, struct device_operations *ops, unsigned int functions, struct pnp_info *info) { struct device_path path; struct device_id id = {.type = DEVICE_ID_PNP }; struct device *dev; int i;
path.type = DEVICE_PATH_PNP; path.pnp.port = base_dev->path.pnp.port; /* Setup the ops and resources on the newly allocated devices. */ for (i = 0; i < functions; i++) { path.pnp.device = info[i].function; dev = alloc_find_dev(base_dev->bus, &path, &id);so we get a new dynamic device for each pnp function.
Which has absolutely no knowledge of the dts. We could, however, create an initialization function that copies all of the information from the dts into the specific device. It just has to be done before resource allocation, which means that pnp_enable_devices needs to be split and renamed.
Making one device for each pnp function, we will need one dts for each pnp function. This is very easy to do, although it will be a bit of work. But now is the time to do it if we want to do it. I am not convinced we want to do it.
I have a generic pnp dts which has members io,irq,io2,irq2,drq
I'd rather put the effort into getting the k8 to work, but I don't see how this is blocking the k8.
The problem is that since these resources don't get matched with the dts, they don't get set to the right values. The SuperIO doesn't get configured at all correctly. Since they are dynamic resources, the mess up the resource allocation algorithm and make it so that the resources can't be allocated.
Although it is possible that something else is blocking k8, this seems like a bad thing to me. It will be a lot easier to see what's wrong with the rest when this is fixed.
Thanks, Myles
On Wed, Nov 12, 2008 at 8:25 AM, Myles Watson mylesgw@gmail.com wrote:
I forgot the rule. I'm down the path of creating static devices for each PNP device. :(
if you make a dts per PNP device, the dtc will do that for you.
don't go too far without a sanity test with me or you will go insane :-)
Which has absolutely no knowledge of the dts. We could, however, create an initialization function that copies all of the information from the dts into the specific device. It just has to be done before resource allocation, which means that pnp_enable_devices needs to be split and renamed.
as marc pointed out, the phase3_chip_setup_dev does have dts knowledge.
I'm seeing the problem more now. As you have pointed out, the pnp stuff has no good connection to anything else, and this needs to be fixed.
I have a generic pnp dts which has members io,irq,io2,irq2,drq
can I see it?
ron
On Wed, Nov 12, 2008 at 10:37 AM, ron minnich rminnich@gmail.com wrote:
On Wed, Nov 12, 2008 at 8:25 AM, Myles Watson mylesgw@gmail.com wrote:
I forgot the rule. I'm down the path of creating static devices for each PNP device. :(
if you make a dts per PNP device, the dtc will do that for you.
don't go too far without a sanity test with me or you will go insane :-)
Which has absolutely no knowledge of the dts. We could, however, create
an
initialization function that copies all of the information from the dts
into
the specific device. It just has to be done before resource allocation, which means that pnp_enable_devices needs to be split and renamed.
as marc pointed out, the phase3_chip_setup_dev does have dts knowledge.
I'm seeing the problem more now. As you have pointed out, the pnp stuff has no good connection to anything else, and this needs to be fixed.
I have a generic pnp dts which has members io,irq,io2,irq2,drq
can I see it?
Shield your eyes from the ugliness :)
This patch creates a superio/winbond/*/pnp.dts which is generic enough that it probably should be extended if needed and moved to device/pnp.dts or superio/pnp.dts or something like that.
I wish that I could put pnp@W83267HF_KBC and have the define come out in the statictree.c, but it doesn't like that, so I have to duplicate the information.
Right now it does pnp@5 as a static child of ioport@2e. I have to fix that up in the code by assigning the pnp device's port from its parents. That could move.
In the pnp code, I changed it so that it doesn't allocate a pnp device if it doesn't find it, because that would mean that all the values were wrong for the ports, irqs, etc.
Signed-off-by: Myles Watson mylesgw@gmail.com
I signed it off in case I'm hit by a truck, for Uwe, but it shouldn't be committed.
Thanks, Myles
Here's the resource tree with my patch, note that it's not perfect yet.
domain_0(PCI_DOMAIN: 0000) assign_resources done, bus 0 link: 0 root(Root Device) assign_resources done, bus 0 link: 0 Root Device resource base 1000 size 2000 align c gran 0 limit ffff flags 40000100 index 0 Root Device resource base fc000000 size 1200000 align 18 gran 0 limit febfffff flags 40000200 index 1 PCI_DOMAIN: 0000 resource base 400 size 0 align 0 gran 0 limit ffff flags 40040100 index 10000000 PCI_DOMAIN: 0000 resource base 0 size 0 align 0 gran 0 limit fcffffffff flags 40040200 index 10000100 PCI_DOMAIN: 0000 resource base 0 size a0000 align 0 gran 0 limit 0 flags e0004200 index 10 PCI_DOMAIN: 0000 resource base c0000 size ff40000 align 0 gran 0 limit 0 flags e0004200 index 20 PCI: 00:18.0 resource base fd200000 size 0 align 14 gran 14 limit ffffffffff flags 60001200 index 1b8 PCI: 00:18.0 resource base 1000 size 2000 align c gran c limit ffff flags 60000100 index 1c0 PCI: 00:18.0 resource base fc000000 size 1200000 align 18 gran 14 limit febfffff flags 60000200 index 1b0 PCI: 00:18.0 resource base a0000 size 20000 align 0 gran 0 limit 0 flags 0 index 1a8 PCI: 00:06.0 resource base 1000 size 1000 align c gran c limit ffff flags 60000102 index 1c PCI: 00:06.0 resource base fc000000 size 1100000 align 18 gran 14 limit febfffff flags 60000202 index 20 PCI: 01:00.0 resource base fd050000 size 1000 align c gran c limit ffffffff flags 60000200 index 10 PCI: 01:00.1 resource base fd051000 size 1000 align c gran c limit ffffffff flags 60000200 index 10 PCI: 01:04.0 resource base fc000000 size 1000000 align 18 gran 18 limit ffffffff flags 60000200 index 10 PCI: 01:04.0 resource base fd053000 size 100 align 8 gran 8 limit ffffffff flags 60000200 index 14 PCI: 01:04.0 resource base 1000 size 100 align 8 gran 8 limit ffff flags 60000100 index 18 PCI: 01:04.0 resource base fd040000 size 10000 align 10 gran 10 limit ffffffff flags 60002200 index 30 PCI: 01:05.0 resource base fd000000 size 20000 align 11 gran 11 limit ffffffff flags 60000200 index 10 PCI: 01:05.0 resource base fd020000 size 20000 align 11 gran 11 limit ffffffff flags 60000200 index 14 PCI: 01:05.0 resource base 1400 size 40 align 6 gran 6 limit ffff flags 60000100 index 18 PCI: 01:05.0 resource base fd052000 size 800 align b gran b limit ffffffff flags 60002200 index 30 PCI: 00:07.0 resource base 0 size 0 align 0 gran 0 limit 0 flags 40040100 index 10000000 PCI: 00:07.0 resource base 0 size 0 align 0 gran 0 limit 0 flags 40040200 index 10000100 PNP: 002e.2 resource base 0 size 8 align 3 gran 3 limit 7ff flags c0000100 index 60 PNP: 002e.2 resource base 0 size 1 align 0 gran 0 limit 0 flags 400 index 70 PNP: 002e.5 resource base 0 size 1 align 0 gran 0 limit ffffffff flags c0000100 index 60 PNP: 002e.5 resource base 0 size 1 align 0 gran 0 limit ffffffff flags c0000100 index 62 PNP: 002e.5 resource base 0 size 1 align 0 gran 0 limit 0 flags 400 index 70 PNP: 002e.5 resource base 0 size 1 align 0 gran 0 limit 0 flags 400 index 72 PCI: 00:07.1 resource base 2020 size 10 align 4 gran 4 limit ffff flags 60000100 index 20 PCI: 00:07.2 resource base 2000 size 20 align 5 gran 5 limit ffff flags 60000100 index 10 PCI: 00:0a.1 resource base fd100000 size 1000 align c gran c limit ffffffffffffffff flags 60000201 index 10 PCI: 00:0b.1 resource base fd101000 size 1000 align c gran c limit fffffffffffffff
On Wed, Nov 12, 2008 at 10:59 AM, Myles Watson mylesgw@gmail.com wrote:
Here's the resource tree with my patch, note that it's not perfect yet.
domain_0(PCI_DOMAIN: 0000) assign_resources done, bus 0 link: 0 root(Root Device) assign_resources done, bus 0 link: 0
Here's the first problem: The root device only has 1 IO resource
Root Device resource base 1000 size 2000 align c gran 0 limit ffff flags 40000100 index 0 Root Device resource base fc000000 size 1200000 align 18 gran 0 limit febfffff flags 40000200 index 1
But the domain has multiple, and they don't get configured correctly. I think resource allocation should be done per domain. I haven't implemented it yet, though.
Thanks, Myles
PCI_DOMAIN: 0000 resource base 400 size 0 align 0 gran 0 limit ffff flags 40040100 index 10000000 PCI_DOMAIN: 0000 resource base 0 size 0 align 0 gran 0 limit fcffffffff flags 40040200 index 10000100 PCI_DOMAIN: 0000 resource base 0 size a0000 align 0 gran 0 limit 0 flags e0004200 index 10 PCI_DOMAIN: 0000 resource base c0000 size ff40000 align 0 gran 0 limit 0 flags e0004200 index 20 PCI: 00:18.0 resource base fd200000 size 0 align 14 gran 14 limit ffffffffff flags 60001200 index 1b8 PCI: 00:18.0 resource base 1000 size 2000 align c gran c limit ffff flags 60000100 index 1c0 PCI: 00:18.0 resource base fc000000 size 1200000 align 18 gran 14 limit febfffff flags 60000200 index 1b0 PCI: 00:18.0 resource base a0000 size 20000 align 0 gran 0 limit 0 flags 0 index 1a8 PCI: 00:06.0 resource base 1000 size 1000 align c gran c limit ffff flags 60000102 index 1c PCI: 00:06.0 resource base fc000000 size 1100000 align 18 gran 14 limit febfffff flags 60000202 index 20 PCI: 01:00.0 resource base fd050000 size 1000 align c gran c limit ffffffff flags 60000200 index 10 PCI: 01:00.1 resource base fd051000 size 1000 align c gran c limit ffffffff flags 60000200 index 10 PCI: 01:04.0 resource base fc000000 size 1000000 align 18 gran 18 limit ffffffff flags 60000200 index 10 PCI: 01:04.0 resource base fd053000 size 100 align 8 gran 8 limit ffffffff flags 60000200 index 14 PCI: 01:04.0 resource base 1000 size 100 align 8 gran 8 limit ffff flags 60000100 index 18 PCI: 01:04.0 resource base fd040000 size 10000 align 10 gran 10 limit ffffffff flags 60002200 index 30 PCI: 01:05.0 resource base fd000000 size 20000 align 11 gran 11 limit ffffffff flags 60000200 index 10 PCI: 01:05.0 resource base fd020000 size 20000 align 11 gran 11 limit ffffffff flags 60000200 index 14 PCI: 01:05.0 resource base 1400 size 40 align 6 gran 6 limit ffff flags 60000100 index 18 PCI: 01:05.0 resource base fd052000 size 800 align b gran b limit ffffffff flags 60002200 index 30 PCI: 00:07.0 resource base 0 size 0 align 0 gran 0 limit 0 flags 40040100 index 10000000 PCI: 00:07.0 resource base 0 size 0 align 0 gran 0 limit 0 flags 40040200 index 10000100 PNP: 002e.2 resource base 0 size 8 align 3 gran 3 limit 7ff flags c0000100 index 60 PNP: 002e.2 resource base 0 size 1 align 0 gran 0 limit 0 flags 400 index 70 PNP: 002e.5 resource base 0 size 1 align 0 gran 0 limit ffffffff flags c0000100 index 60 PNP: 002e.5 resource base 0 size 1 align 0 gran 0 limit ffffffff flags c0000100 index 62 PNP: 002e.5 resource base 0 size 1 align 0 gran 0 limit 0 flags 400 index 70 PNP: 002e.5 resource base 0 size 1 align 0 gran 0 limit 0 flags 400 index 72 PCI: 00:07.1 resource base 2020 size 10 align 4 gran 4 limit ffff flags 60000100 index 20 PCI: 00:07.2 resource base 2000 size 20 align 5 gran 5 limit ffff flags 60000100 index 10 PCI: 00:0a.1 resource base fd100000 size 1000 align c gran c limit ffffffffffffffff flags 60000201 index 10 PCI: 00:0b.1 resource base fd101000 size 1000 align c gran c limit fffffffffffffff
+ 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. Second, every device has an io and io2 now --even those that don't support it? 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. Fourth, I find this less readable than the old pnp:
- /* 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.
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.
Lotsa confusion out here with the dts right now.
ron
-----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
On Thu, Nov 13, 2008 at 6:03 AM, Myles Watson mylesgw@gmail.com wrote:
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:
doesn't support what? A hierarchy? It will support what we want.
But how do you propose to modify properties in the dts as we do now?
example
here is the (shortened for brevity) winbond/w83627 dts:
/{ keyboard { io0="0x62"; io2 = "0x64"; /* whatever*/ }; };
And in the mainboard
ioport@2e { /config/("superio/winbond/w83627hf/pnp.dts"); keyboard/io0 = "0x44"; };
Would that work for everyone?
It still leaves other issues but ...
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.
yes, but this is *exactly* the model that people are objecting to in the other thread. We kind of need to make up our mind here.
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.
I would rather not do this.
That's right. The problem is that there needs to be some generic way to pass this information to the resource code.
This is actually not even v2, it's v1. It did not work because you have resources in that struct that don't exist on many devices, and there are always new devices with new resource types that break the model.
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.
I am still not convinced this really matters.
ron
On Thu, Nov 13, 2008 at 8:58 AM, ron minnich rminnich@gmail.com wrote:
On Thu, Nov 13, 2008 at 6:03 AM, Myles Watson mylesgw@gmail.com wrote:
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.
yes, but this is *exactly* the model that people are objecting to in the other thread. We kind of need to make up our mind here.
How about we get something working, then improve on it. Here's my latest try.
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.
I would rather not do this.
OK. It's bitten me once, but I'll be more careful.
That's right. The problem is that there needs to be some generic way to pass this information to the resource code.
This is actually not even v2, it's v1. It did not work because you have resources in that struct that don't exist on many devices, and there are always new devices with new resource types that break the model.
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.
I am still not convinced this really matters.
I'm attaching two logs. One is without the attached patch, the other with. It fixes resource allocation and makes the logs much cleaner, but it doesn't make the box work correctly. It's just hard to debug too many things at once. I'd like to get the SuperIO working, then resource allocation, then VGA if its still broken, then HT ...
The patch won't apply for you because it depends on other code in the tree. There's just too much to send now, and I haven't gotten organized enough to use Jordan's script yet.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
I forgot to say what was interesting in the logs:
I like to do side by side diff, but to each their own.
no_pnp is the log without my patch. 1. Notice that it creates an enabled dynamic device for each function, even the disabled ones. 2. Notice the resource allocation bases. Because the PNP devices get included in resource allocation, the IO base and limit are broken. 3. There are lots of incorrect resources, because no values are taken from the dts.
Thanks, Myles
Myles,
I think that this is going to right direction. There are a couple things that I don't understand.
Why is there a w83627hf_pnp_ops and w83627hf_ops? Can they be combined?
And this is why there are two dts files, pnp.dts and dts?
I don't think each device should have io, io1, io2, etc. They only need whatever is required to fill the entry in pnp_dev_info[].
And so there isn't a stock pnp dts entry. Each should be specific for the device (kbc, uart, etc) but generic names.
Thanks for the clarification.
Marc -- http://marcjstuff.blogspot.com/
________________________________ From: Myles Watson mylesgw@gmail.com To: ron minnich rminnich@gmail.com Cc: Coreboot coreboot@coreboot.org Sent: Thursday, November 13, 2008 10:29:08 AM Subject: Re: [coreboot] Resource allocation
On Thu, Nov 13, 2008 at 8:58 AM, ron minnich rminnich@gmail.com wrote:
On Thu, Nov 13, 2008 at 6:03 AM, Myles Watson mylesgw@gmail.com wrote:
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.
yes, but this is *exactly* the model that people are objecting to in the other thread. We kind of need to make up our mind here.
How about we get something working, then improve on it. Here's my latest try.
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.
I would rather not do this.
OK. It's bitten me once, but I'll be more careful.
That's right. The problem is that there needs to be some generic way to pass this information to the resource code.
This is actually not even v2, it's v1. It did not work because you have resources in that struct that don't exist on many devices, and there are always new devices with new resource types that break the model.
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.
I am still not convinced this really matters.
I'm attaching two logs. One is without the attached patch, the other with. It fixes resource allocation and makes the logs much cleaner, but it doesn't make the box work correctly. It's just hard to debug too many things at once. I'd like to get the SuperIO working, then resource allocation, then VGA if its still broken, then HT ...
The patch won't apply for you because it depends on other code in the tree. There's just too much to send now, and I haven't gotten organized enough to use Jordan's script yet.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
{ device_operations = "w83627thg_ops"; /* To override any of these, put the over-ride in mainboard dts. */
/* COM1 */ pnp@2{ com1dev = "2"; com1enable = "0"; com1io = "0x3f8"; com1irq = "4"; };
Questions I have no answer to: Before I put the pnp2 in , we got this:
struct superio_winbond_w83627thg_dts_config domain_0_ioport_2e = { .com1dev = 0x2, .com1enable = 0x0, .com1io = 0x3f8,
etc.
What should we get now?
Should pnp2 be a device? Child or sibling of w83627thg? What device_operations should it have? One possibiltiy: if no device_operations property in the pnp@2 node, inherit from parent.
What do you want to see?
ron
On Thu, Nov 13, 2008 at 12:14 PM, ron minnich rminnich@gmail.com wrote:
{ device_operations = "w83627thg_ops"; /* To override any of these, put the over-ride in mainboard dts. */
/* COM1 */ pnp@2{ com1dev = "2"; com1enable = "0"; com1io = "0x3f8"; com1irq = "4"; };Questions I have no answer to: Before I put the pnp2 in , we got this:
struct superio_winbond_w83627thg_dts_config domain_0_ioport_2e = { .com1dev = 0x2, .com1enable = 0x0, .com1io = 0x3f8,
etc.
What should we get now?
struct superio_common_pnp domain_0_ioport_2e_pnp_2 = { .enable = 0x0, .io = 3f8,
struct superio_common_pnp domain_0_ioport_2e_pnp_5 = { .enable = 0x0, .io = 3f8, etc.
Should pnp2 be a device?
That's the way it is now. I'm just making them static instead of dynamic.
Child or sibling of w83627thg?
Child? That's how I did it.
What device_operations should it have?
Enable and set.
One possibiltiy: if no device_operations property in the pnp@2 node, inherit from parent.
That's how I did it the first time, but then I saw that you were already passing it into the PNP code in the info structure, so I took it back out.
What do you want to see?
I'm not picky if it works. I could go back and implement it with it all inside the device. I would just have to change the PNP code a _lot_ more than I did. There would have to be more passes so that you could get the devices set up, then go back and initialize them with resources. I was trying to minimize changes.
Thanks, Myles
On Thu, Nov 13, 2008 at 1:01 PM, Myles Watson mylesgw@gmail.com wrote:
struct superio_common_pnp domain_0_ioport_2e_pnp_2 = { .enable = 0x0, .io = 3f8,
struct superio_common_pnp domain_0_ioport_2e_pnp_5 = { .enable = 0x0, .io = 3f8, etc.
This makes sense to me. The we will have one superio dts with kids. I will try to do this -- tomorrow is my day off hence it is coreboot day.
If this works it will also apply to southbridge parts etc.
ron
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Thursday, November 13, 2008 3:38 PM To: Myles Watson Cc: Marc Jones; Coreboot Subject: Re: [coreboot] Resource allocation
On Thu, Nov 13, 2008 at 1:01 PM, Myles Watson mylesgw@gmail.com wrote:
struct superio_common_pnp domain_0_ioport_2e_pnp_2 = { .enable = 0x0, .io = 3f8,
struct superio_common_pnp domain_0_ioport_2e_pnp_5 = { .enable = 0x0, .io = 3f8, etc.
This makes sense to me. The we will have one superio dts with kids. I will try to do this -- tomorrow is my day off hence it is coreboot day.
This patch should apply. It is based off the old dts. That was the only part that wouldn't apply from the old one.
I agree with Marc that we could put the ops back together.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
On Thu, Nov 13, 2008 at 5:02 PM, Myles Watson mylesgw@gmail.com wrote:
This patch should apply. It is based off the old dts. That was the only part that wouldn't apply from the old one.
I agree with Marc that we could put the ops back together.
Signed-off-by: Myles Watson mylesgw@gmail.com
But this does not actually work does it? Or does it? How far does it get? Or is this just the notion of what we want?
ron
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Thursday, November 13, 2008 6:28 PM To: Myles Watson Cc: Marc Jones; Coreboot Subject: Re: [coreboot] Resource allocation
On Thu, Nov 13, 2008 at 5:02 PM, Myles Watson mylesgw@gmail.com wrote:
This patch should apply. It is based off the old dts. That was the
only
part that wouldn't apply from the old one.
I agree with Marc that we could put the ops back together.
Signed-off-by: Myles Watson mylesgw@gmail.com
But this does not actually work does it? Or does it? How far does it get? Or is this just the notion of what we want?
It's most of the way there. The devices show up and get initialized. I'm not sure what else is supposed to happen with them.
Thanks, Myles
OK but what about the superio/common/pnp.dts. Do you need to do an svn add?
ron
OK but what about the superio/common/pnp.dts. Do you need to do an svn add?
Yes. I remembered to add it on one box, but forgot on my laptop when I generated the last patch. I don't know why I always expect that to happen automatically when I apply a patch.
Tomorrow I'll unify the ops again and send a new patch.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
OK, to use this I still see a problem.
In order to make the pnp devices not be dynamic, we have lost the ability to have default superio settings that don't have to be done in the mainboard.
In other words, in the current system, we have this *in the superio*:
- /* 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";
now, for each and every *mainboard* that uses that superio, we have to set these defaults in the mainboard.
That's not good.
The problem is easily solved: create files such as superio/common/com1.dts, and use them instead of the pnp.dts which has non-optimal default settings. Or, just create superio/winbond/whatever/comport.dts, keyboard.dts, etc. and use them.
I think this is closer but maybe still not right. This could be the template for all other dts setups if we get it right.
ron
On Fri, Nov 14, 2008 at 9:43 AM, ron minnich rminnich@gmail.com wrote:
OK, to use this I still see a problem.
In order to make the pnp devices not be dynamic, we have lost the ability to have default superio settings that don't have to be done in the mainboard.
It could be done in superio.c where the mask is being set.
In other words, in the current system, we have this *in the superio*:
/* Floppy */floppydev = "0x0";now, for each and every *mainboard* that uses that superio, we have to set these defaults in the mainboard.
That's not good.
That was one of my questions. Aren't these settings mainboard specific? In that case we want to set them in each mainboard. Otherwise it'll bite lots of people.
The problem is easily solved: create files such as superio/common/com1.dts, and use them instead of the pnp.dts which has non-optimal default settings. Or, just create superio/winbond/whatever/comport.dts, keyboard.dts, etc. and use them.
If we're going that way, I'd rather just use what we have and change the PNP code to match. (Let it create dynamic devices but pass more information in the info structure so that the enables get set, etc.) The hierarchy buys us nothing if we just make lots of little files and types.
BTW: Does anyone see PNP devices besides the SuperIOs? I.E., will the PNP code ever get used for anything else?
I think this is closer but maybe still not right. This could be the
template for all other dts setups if we get it right.
Yes. Let's get it right.
Thanks, Myles
On Fri, Nov 14, 2008 at 8:50 AM, Myles Watson mylesgw@gmail.com wrote:
It could be done in superio.c where the mask is being set.
I think it should be done in dts, but this *is* an option. If the dts setting is zero pick a reasonable default.
That was one of my questions. Aren't these settings mainboard specific? In that case we want to set them in each mainboard. Otherwise it'll bite lots of people.
They are usually generic, and only rarely need to change. This supports your idea of having defaults in .c files. This makes the defaults harder to find, but it would work. However, I like your idea below a little better:
If we're going that way, I'd rather just use what we have and change the PNP code to match. (Let it create dynamic devices but pass more information in the info structure so that the enables get set, etc.) The hierarchy buys us nothing if we just make lots of little files and types.
I think this makes the most sense.
BTW: Does anyone see PNP devices besides the SuperIOs? I.E., will the PNP code ever get used for anything else?
not yet. I hope never. PNP is a mess.
ron
On Fri, Nov 14, 2008 at 9:55 AM, ron minnich rminnich@gmail.com wrote:
On Fri, Nov 14, 2008 at 8:50 AM, Myles Watson mylesgw@gmail.com wrote:
If we're going that way, I'd rather just use what we have and change the
PNP
code to match. (Let it create dynamic devices but pass more information
in
the info structure so that the enables get set, etc.) The hierarchy buys
us
nothing if we just make lots of little files and types.
I think this makes the most sense.
I'll implement this one and send a patch. I don't like it being labeled dynamic, but I will live (or else I'll change it.)
Thanks, Myles
On Fri, Nov 14, 2008 at 9:58 AM, Myles Watson mylesgw@gmail.com wrote:
On Fri, Nov 14, 2008 at 9:55 AM, ron minnich rminnich@gmail.com wrote:
On Fri, Nov 14, 2008 at 8:50 AM, Myles Watson mylesgw@gmail.com wrote:
If we're going that way, I'd rather just use what we have and change the
PNP
code to match. (Let it create dynamic devices but pass more information
in
the info structure so that the enables get set, etc.) The hierarchy
buys us
nothing if we just make lots of little files and types.
I think this makes the most sense.
I'll implement this one and send a patch. I don't like it being labeled dynamic, but I will live (or else I'll change it.)
I changed it so that the names look like this: ioport_2e(IOPORT: 2e): enabled 1 have_resources 0 devfn ff ioport_2e_pnp_child_0(PNP: 002e.0): enabled 0 have_resources 0 devfn 0 ioport_2e_pnp_child_0(PNP: 002e.1): enabled 0 have_resources 0 devfn 1 ioport_2e_pnp_child_0(PNP: 002e.2): enabled 1 have_resources 0 devfn 2 ioport_2e_pnp_child_0(PNP: 002e.3): enabled 0 have_resources 0 devfn 3 ioport_2e_pnp_child_0(PNP: 002e.5): enabled 1 have_resources 0 devfn 5 ioport_2e_pnp_child_0(PNP: 002e.6): enabled 0 have_resources 0 devfn 6 ioport_2e_pnp_child_0(PNP: 002e.7): enabled 0 have_resources 0 devfn 7 ioport_2e_pnp_child_0(PNP: 002e.8): enabled 0 have_resources 0 devfn 8 ioport_2e_pnp_child_0(PNP: 002e.9): enabled 0 have_resources 0 devfn 9 ioport_2e_pnp_child_0(PNP: 002e.a): enabled 0 have_resources 0 devfn a ioport_2e_pnp_child_0(PNP: 002e.b): enabled 1 have_resources 0 devfn b
This patch changes PNP support for devices so that the dts values get passed in. Build and run-tested only on Serengeti. It will probably require the rework of other SuperIOs.
include/device/pnp.h: Add enable, val, and irq & drq structs.
superio/winbond/w83627hf/superio.c: Change functions to operate on children. Add device ID to ops. Add enables to pnp_dev_info table. Fill in dts values.
superio/winbond/w83627hf/dts: Get rid of device number parameters. Add config parameters so we know when they're set.
device/pnp_device.c: Allocate devices as children to SuperIO.
device/pnp_device.c: Allocate devices as children to SuperIO.
mainboard/amd/serengeti/dts: Move ioport so it's found. (Not its permanent resting place I hope.) Add enables for KBC, SP1, and HWM to show it off.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
OK, this is very, very close.
But: you don't need these I think: + floppyconf = "0";
Why? because the dt compiler merges the values of the mainboard dts and the superio dts to produce the statictree.c In other words, if you don't set anything in the mainboard dts entry for ioport@2e, then the struct will get the default values. Any values over-ridden by the mainboard dts get changed, but nothing else does.
So, given that you can set all these variables in the superio dts, or leave them at default, there is no need to say "I am setting these in the mainboard dts".
You can thus simplify the patch a good deal by yanking the *conf stuff out, in both the dts and the .c files.
thanks
ron p.s. it's really close, and it's really better, so hang in there.
On Fri, Nov 14, 2008 at 2:14 PM, ron minnich rminnich@gmail.com wrote:
OK, this is very, very close.
But: you don't need these I think:
floppyconf = "0";Why? because the dt compiler merges the values of the mainboard dts and the superio dts to produce the statictree.c In other words, if you don't set anything in the mainboard dts entry for ioport@2e, then the struct will get the default values. Any values over-ridden by the mainboard dts get changed, but nothing else does.
Given sane defaults this probably makes sense. In my mind I was seeing someone wanting to set the port address but not enable the device, and someone else not wanting to set the port address. I realize that my code didn't make this happen, but it would be easy to add, and I meant to.
If that case is not important, then I'll rip out the conf stuff.
Thanks, Myles
On Fri, Nov 14, 2008 at 1:20 PM, Myles Watson mylesgw@gmail.com wrote:
Given sane defaults this probably makes sense. In my mind I was seeing someone wanting to set the port address but not enable the device,
That works now, or should.
You can set any one of those settings and leave the others at default.
The Right Things should happen. What doesn't happen?
thanks
ron
On Fri, Nov 14, 2008 at 2:29 PM, ron minnich rminnich@gmail.com wrote:
On Fri, Nov 14, 2008 at 1:20 PM, Myles Watson mylesgw@gmail.com wrote:
Given sane defaults this probably makes sense. In my mind I was seeing someone wanting to set the port address but not enable the device,
That works now, or should.
You can set any one of those settings and leave the others at default.
The Right Things should happen. What doesn't happen?
There's no way not to use the default. In other words, there's always a device created for each PNP function. My original intent was that if conf was not set, no device needed to be created.
Thanks, Myles
On Fri, Nov 14, 2008 at 2:31 PM, Myles Watson mylesgw@gmail.com wrote:
On Fri, Nov 14, 2008 at 2:29 PM, ron minnich rminnich@gmail.com wrote:
On Fri, Nov 14, 2008 at 1:20 PM, Myles Watson mylesgw@gmail.com wrote:
Given sane defaults this probably makes sense. In my mind I was seeing someone wanting to set the port address but not enable the device,
That works now, or should.
You can set any one of those settings and leave the others at default.
The Right Things should happen. What doesn't happen?
There's no way not to use the default. In other words, there's always a device created for each PNP function. My original intent was that if conf was not set, no device needed to be created.
This patch rips out the conf variables and changes the debug printing a little, in addition to the old patch.
Thanks, Myles
Acked-by: Ronald G. Minnich rminnich@gmail.com
I don't think this breaks anything else, does it?
ron
On Fri, Nov 14, 2008 at 2:37 PM, ron minnich rminnich@gmail.com wrote:
Acked-by: Ronald G. Minnich rminnich@gmail.com
I don't think this breaks anything else, does it?
I'm not sure. It shouldn't break anything that was getting values from the dts, since that wasn't working. Since nothing else uses PNP code...
It probably breaks the other SuperIOs, since I changed the definition of the pnp_info struct. I'll fix that next as long as I have testers.
Thanks, Myles
On Fri, Nov 14, 2008 at 1:40 PM, Myles Watson mylesgw@gmail.com wrote:
On Fri, Nov 14, 2008 at 2:37 PM, ron minnich rminnich@gmail.com wrote:
Acked-by: Ronald G. Minnich rminnich@gmail.com
I don't think this breaks anything else, does it?
I'm not sure. It shouldn't break anything that was getting values from the dts, since that wasn't working. Since nothing else uses PNP code...
It probably breaks the other SuperIOs, since I changed the definition of the pnp_info struct. I'll fix that next as long as I have testers.
I can test some platforms.
ron
On Fri, Nov 14, 2008 at 2:37 PM, ron minnich rminnich@gmail.com wrote:
Acked-by: Ronald G. Minnich rminnich@gmail.com
Rev 1027.
Thanks, Myles
Tested with etherboot and booting to linux with dbe62, but that doesn't have a superio, really.
But still. With all the changes that have gone in lately, I am glad to see my old friend the dbe62 still boots!
ron
On Thu, Nov 13, 2008 at 11:30 AM, Marc Jones marcj303@yahoo.com wrote:
Myles,
I think that this is going to right direction. There are a couple things that I don't understand.
Why is there a w83627hf_pnp_ops and w83627hf_ops? Can they be combined?
The problem is that one is the parent, and one is for the children. The parent's ops just iterates through the children and calls the correct functions. If you don't have that, then the childrens' functions never get called.
And this is why there are two dts files, pnp.dts and dts?
I wanted to simplify the code that took the information from the dts and passed it to the PNP code. If you just have a big list of attributes, you have to figure out which attribute goes with which function number and pass them to the PNP code. Having devices already created with the correct function numbers seems less error-prone to me.
I don't think each device should have io, io1, io2, etc. They only need whatever is required to fill the entry in pnp_dev_info[].
I agree. I just didn't want to create w83627hf_kbd.dts and friends, because then I'd be at the same place with lots of code to pass the arguments.
And so there isn't a stock pnp dts entry. Each should be specific for the device (kbc, uart, etc) but generic names.
I don't understand what you mean.
Thanks for the clarification.
Thanks for questioning. It helps.
Myles
Marc
http://marcjstuff.blogspot.com/
*From:* Myles Watson mylesgw@gmail.com *To:* ron minnich rminnich@gmail.com *Cc:* Coreboot coreboot@coreboot.org *Sent:* Thursday, November 13, 2008 10:29:08 AM *Subject:* Re: [coreboot] Resource allocation
On Thu, Nov 13, 2008 at 8:58 AM, ron minnich rminnich@gmail.com wrote:
On Thu, Nov 13, 2008 at 6:03 AM, Myles Watson mylesgw@gmail.com wrote:
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.
yes, but this is *exactly* the model that people are objecting to in the other thread. We kind of need to make up our mind here.
How about we get something working, then improve on it. Here's my latest try.
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.
I would rather not do this.
OK. It's bitten me once, but I'll be more careful.
That's right. The problem is that there needs to be some generic way
to
pass this information to the resource code.
This is actually not even v2, it's v1. It did not work because you have resources in that struct that don't exist on many devices, and there are always new devices with new resource types that break the model.
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.
I am still not convinced this really matters.
I'm attaching two logs. One is without the attached patch, the other with. It fixes resource allocation and makes the logs much cleaner, but it doesn't make the box work correctly. It's just hard to debug too many things at once. I'd like to get the SuperIO working, then resource allocation, then VGA if its still broken, then HT ...
The patch won't apply for you because it depends on other code in the tree. There's just too much to send now, and I haven't gotten organized enough to use Jordan's script yet.
Signed-off-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
-----Original Message----- From: ron minnich [mailto:rminnich@gmail.com] Sent: Wednesday, November 12, 2008 10:38 AM To: Myles Watson Cc: Coreboot Subject: Re: [coreboot] Resource allocation
On Wed, Nov 12, 2008 at 8:25 AM, Myles Watson mylesgw@gmail.com wrote:
I forgot the rule. I'm down the path of creating static devices for
each
PNP device. :(
if you make a dts per PNP device, the dtc will do that for you.
don't go too far without a sanity test with me or you will go insane :-)
I think I'm there now.
I got to the point where I wanted to access the device_configuration struct in the pnp_device code, and that's not pretty. I guess I need to take a step back.
Thanks, Myles
On Wed, Nov 12, 2008 at 2:02 PM, Myles Watson mylesgw@gmail.com wrote:
I got to the point where I wanted to access the device_configuration struct in the pnp_device code, and that's not pretty. I guess I need to take a step back.
yes. Let's take this more slowly. I think it's easy to go off the rails.
ron
ron minnich wrote:
Dynamic superio probing only makes any sense to me if it can be used verbatim on each and every superio
yes, but having a single superio device structure in the static tree is what requires dynamic devices. We're not probing. We are actually just creating devices on demand. Those are two different things.
Right. Who knows what should be demanded? I would like the mainboard dts to be the source.
Plus, the weird thing on a superio: even if you don't want a function (e.g. serial) it's important to make sure you turn certain pnp functions off in some cases -- so you may want that pnp function controlled, even if you do not use it.
Yes, that's why I think all functions of the chip should be listed somewhere (superio dts) but the ones that actually are enabled somewhere else (mainboard dts)..
Remember the rule: one dts -> one device.
I tend to like one dts -> one chip. But as long as dtses can pull in other dtses we both win.
..
so we get a new dynamic device for each pnp function.
Making one device for each pnp function, we will need one dts for each pnp function. This is very easy to do, although it will be a bit of work. But now is the time to do it if we want to do it. I am not convinced we want to do it.
It will end up being a lot of files, which is a little clumsy. I'd like to roll it into one file..
I guess we've gone back and forth on this. I guess dtc is the bottleneck because it only deals with one struct per input file?
I'd rather put the effort into getting the k8 to work,
Or GeodeLX, C7, or Core2. There's plenty of unfinished stuff in v3 now. :) SCNR.
//Peter
On Wed, Nov 12, 2008 at 8:46 AM, Peter Stuge peter@stuge.se wrote:
Remember the rule: one dts -> one device.
I tend to like one dts -> one chip. But as long as dtses can pull in other dtses we both win.
this decision was taken some time ago and I would rather not revisit it.
It will end up being a lot of files, which is a little clumsy. I'd like to roll it into one file..
I guess we've gone back and forth on this. I guess dtc is the bottleneck because it only deals with one struct per input file?
it's complicated. I think myles is on the right track at this point.
ron
On Wed, Nov 12, 2008 at 11:46 AM, Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
Dynamic superio probing only makes any sense to me if it can be used verbatim on each and every superio
yes, but having a single superio device structure in the static tree is what requires dynamic devices. We're not probing. We are actually just creating devices on demand. Those are two different things.
Right. Who knows what should be demanded? I would like the mainboard dts to be the source.
Plus, the weird thing on a superio: even if you don't want a function (e.g. serial) it's important to make sure you turn certain pnp functions off in some cases -- so you may want that pnp function controlled, even if you do not use it.
Yes, that's why I think all functions of the chip should be listed somewhere (superio dts) but the ones that actually are enabled somewhere else (mainboard dts)..
Remember the rule: one dts -> one device.
I tend to like one dts -> one chip. But as long as dtses can pull in other dtses we both win.
..
so we get a new dynamic device for each pnp function.
Making one device for each pnp function, we will need one dts for each pnp function. This is very easy to do, although it will be a bit of work. But now is the time to do it if we want to do it. I am not convinced we want to do it.
It will end up being a lot of files, which is a little clumsy. I'd like to roll it into one file..
I guess we've gone back and forth on this. I guess dtc is the bottleneck because it only deals with one struct per input file?
I'd rather put the effort into getting the k8 to work,
Or GeodeLX, C7, or Core2. There's plenty of unfinished stuff in v3 now. :) SCNR.
C7 I'll be getting back to ASAP, I've got lots of other things on my plate right this moment though. It's mostly done, just needs a few more bugs tracked down and squashed. Also, for some reason, doing early_mtrr_init() like via epia-cn does in v3 completely breaks booting.
-Corey
-
Corey Osgood -
Marc Jones -
Myles Watson -
Peter Stuge -
ron minnich