I want to reenable the "unwanted VPCI" support, but the dtc does not like me. Regardless of what I try to store in the dts, the device tree compiler will barf.
An ideal solution would be the ability to not only specify array contents, but also array size in the dts. Overriding ability for both would be even better.
Suggestions?
Regards, Carl-Daniel
Index: southbridge/amd/cs5536/cs5536.c =================================================================== --- southbridge/amd/cs5536/cs5536.c (Revision 590) +++ southbridge/amd/cs5536/cs5536.c (Arbeitskopie) @@ -608,16 +608,13 @@ if (sb->enable_ide) ide_init(dev);
-#warning Add back in unwanted VPCI support -#if 0 /* Disable unwanted virtual PCI devices. */ - for (i = 0; (i < MAX_UNWANTED_VPCI) && (0 != sb->unwanted_vpci[i]); i++) { + for (i = 0; (i < MAX_UNWANTED_VPCI) && sb->unwanted_vpci[i]; i++) { printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n", sb->unwanted_vpci[i]); outl(sb->unwanted_vpci[i] + 0x7C, 0xCF8); outl(0xDEADBEEF, 0xCFC); } -#endif printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__); }
Index: southbridge/amd/cs5536/dts =================================================================== --- southbridge/amd/cs5536/dts (Revision 590) +++ southbridge/amd/cs5536/dts (Arbeitskopie) @@ -57,4 +57,7 @@ com2_enable = "0"; com2_address = "0x2f8"; com2_irq = "3"; + + /* Disable unwanted virtualized PCI devices */ + unwanted_vpci[0] = "0"; };
OK, I can look at this, but I want to get my path changes wrapped up and committed.
This will take a bit of thinking, but let's do one dtc change set at a time :-)
Can we wrap up paths, then do this?
ron
I think the way to do this is make a node.
vpci { v1 = "0x1"; v2 = "0x2";};
Then in flattree.c I can turn that into an array that is 0-terminated.
ron
On 13.02.2008 17:52, ron minnich wrote:
I think the way to do this is make a node.
vpci { v1 = "0x1"; v2 = "0x2";};
Then in flattree.c I can turn that into an array that is 0-terminated.
Sounds like a good plan. But I don't see a way to have the mainboard dts override the settings in the chipset dts, at least when we emit a fixed-size array.
Regards, Carl-Daniel
On Feb 13, 2008 9:09 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 13.02.2008 17:52, ron minnich wrote:
I think the way to do this is make a node.
vpci { v1 = "0x1"; v2 = "0x2";};
Then in flattree.c I can turn that into an array that is 0-terminated.
Sounds like a good plan. But I don't see a way to have the mainboard dts override the settings in the chipset dts, at least when we emit a fixed-size array.
Well, hang on, let me see.
thanks
ron
Carl-Daniel Hailfinger wrote:
Index: southbridge/amd/cs5536/dts
--- southbridge/amd/cs5536/dts (Revision 590) +++ southbridge/amd/cs5536/dts (Arbeitskopie) @@ -57,4 +57,7 @@ com2_enable = "0"; com2_address = "0x2f8"; com2_irq = "3";
- /* Disable unwanted virtualized PCI devices */
- unwanted_vpci[0] = "0";
};
I am blissfully ignorant on the topic, but the first thing I wondered:
Should a virtual device not be disabled in the same way as a physical one?
pci@0,0 { disabled; }
?
Stefan
Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
Index: southbridge/amd/cs5536/dts
--- southbridge/amd/cs5536/dts (Revision 590) +++ southbridge/amd/cs5536/dts (Arbeitskopie) @@ -57,4 +57,7 @@ com2_enable = "0"; com2_address = "0x2f8"; com2_irq = "3";
- /* Disable unwanted virtualized PCI devices */
- unwanted_vpci[0] = "0";
};
I am blissfully ignorant on the topic, but the first thing I wondered:
Should a virtual device not be disabled in the same way as a physical one?
pci@0,0 { disabled; }
?
Stefan
This is an interesting idea but the devices are CS5536 specific and I don't think we want to make dts CS5536 aware. I think it better use the device ID rather than the device location to do the disable.
Marc
On Thu, Feb 14, 2008 at 2:16 PM, Marc Jones Marc.Jones@amd.com wrote:
Stefan Reinauer wrote:
Should a virtual device not be disabled in the same way as a physical one?
pci@0,0 { disabled; }
?
Stefan
This is an interesting idea but the devices are CS5536 specific and I don't think we want to make dts CS5536 aware. I think it better use the device ID rather than the device location to do the disable.
yeah, stefan's idea is an 11 on a scale of 10 for cool. But, stefan, to disable, you have to make a VSA call. ow ow ow ow ow ... for putting that into dts :-)
ron
* ron minnich rminnich@gmail.com [080214 23:18]:
pci@0,0 { disabled; }
This is an interesting idea but the devices are CS5536 specific and I don't think we want to make dts CS5536 aware. I think it better use the device ID rather than the device location to do the disable.
But, stefan, to disable, you have to make a VSA call. ow ow ow ow ow ... for putting that into dts :-)
But that VSA call is made no matter how we describe it in the dts.
The CS5536 code should know what to do if a dts asks it to disable a device or the other. Not sure.
On 14.02.2008 23:25, Stefan Reinauer wrote:
- ron minnich rminnich@gmail.com [080214 23:18]:
pci@0,0 { disabled; }
This is an interesting idea but the devices are CS5536 specific and I don't think we want to make dts CS5536 aware. I think it better use the device ID rather than the device location to do the disable.
But, stefan, to disable, you have to make a VSA call. ow ow ow ow ow ... for putting that into dts :-)
But that VSA call is made no matter how we describe it in the dts.
The CS5536 code should know what to do if a dts asks it to disable a device or the other. Not sure.
Indeed. That would solve the problem very nicely without putting hacks into the dts.
Regards, Carl-Daniel
On Thu, Feb 14, 2008 at 2:32 PM, Carl-Daniel Hailfinger
Indeed. That would solve the problem very nicely without putting hacks into the dts.
OK, so we put nodes under the 5536, and specify 'disabled', and the 5536 code knows to actually call VSA when it sees that they are disabled. This is nice. It would use the device tree in a sensible way.
ron
On 14.02.2008 23:32, ron minnich wrote:
On Thu, Feb 14, 2008 at 2:32 PM, Carl-Daniel Hailfinger
Indeed. That would solve the problem very nicely without putting hacks into the dts.
OK, so we put nodes under the 5536, and specify 'disabled', and the 5536 code knows to actually call VSA when it sees that they are disabled. This is nice. It would use the device tree in a sensible way.
Factor out Geode LX VPCI device disabling into a separate function which consumes one device at a time. This helps avoid array handling in the dts and allows us to use generic disabling infrastructure.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c =================================================================== --- LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c (Revision 599) +++ LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c (Arbeitskopie) @@ -570,6 +570,14 @@ }
+static void disable_vpci(u32 vpci) +{ + /* Disable unwanted virtual PCI device. */ + printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n", vpci); + outl(vpci + 0x7C, 0xCF8); + outl(0xDEADBEEF, 0xCFC); +} + /** * TODO. * @@ -608,16 +616,6 @@ if (sb->enable_ide) ide_init(dev);
-#warning Add back in unwanted VPCI support -#if 0 - /* Disable unwanted virtual PCI devices. */ - for (i = 0; (i < MAX_UNWANTED_VPCI) && (0 != sb->unwanted_vpci[i]); i++) { - printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n", - sb->unwanted_vpci[i]); - outl(sb->unwanted_vpci[i] + 0x7C, 0xCF8); - outl(0xDEADBEEF, 0xCFC); - } -#endif printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__); }
Acked-by: Ronald G. Minnich rminnich@gmail.com
On Thu, Feb 14, 2008 at 2:55 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 14.02.2008 23:32, ron minnich wrote:
On Thu, Feb 14, 2008 at 2:32 PM, Carl-Daniel Hailfinger
Indeed. That would solve the problem very nicely without putting hacks into the dts.
OK, so we put nodes under the 5536, and specify 'disabled', and the 5536 code knows to actually call VSA when it sees that they are disabled. This is nice. It would use the device tree in a sensible way.
Factor out Geode LX VPCI device disabling into a separate function which consumes one device at a time. This helps avoid array handling in the dts and allows us to use generic disabling infrastructure.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c
--- LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c (Revision 599) +++ LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c (Arbeitskopie) @@ -570,6 +570,14 @@ }
+static void disable_vpci(u32 vpci) +{
/* Disable unwanted virtual PCI device. */printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n", vpci);outl(vpci + 0x7C, 0xCF8);outl(0xDEADBEEF, 0xCFC);+}
/**
- TODO.
@@ -608,16 +616,6 @@ if (sb->enable_ide) ide_init(dev);
-#warning Add back in unwanted VPCI support -#if 0
/* Disable unwanted virtual PCI devices. */for (i = 0; (i < MAX_UNWANTED_VPCI) && (0 != sb->unwanted_vpci[i]); i++) {printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n",sb->unwanted_vpci[i]);outl(sb->unwanted_vpci[i] + 0x7C, 0xCF8);outl(0xDEADBEEF, 0xCFC);}-#endif printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__); }
Ron: I'm stealing your ack although the patch has slightly changed function naming. Corey/Tom: Does this name match what you had in mind?
Factor out Geode LX VPCI device disabling into a separate function which consumes one device at a time. This helps avoid array handling in the dts and allows us to use generic disabling infrastructure.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Ronald G. Minnich rminnich@gmail.com
Index: LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c =================================================================== --- LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c (Revision 599) +++ LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c (Arbeitskopie) @@ -570,6 +570,15 @@ }
+static void hide_vpci(u32 vpci_devid) +{ + /* Hide unwanted virtual PCI device. */ + printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X\n", + vpci_devid); + outl(vpci_devid + 0x7C, 0xCF8); + outl(0xDEADBEEF, 0xCFC); +} + /** * TODO. * @@ -608,16 +617,6 @@ if (sb->enable_ide) ide_init(dev);
-#warning Add back in unwanted VPCI support -#if 0 - /* Disable unwanted virtual PCI devices. */ - for (i = 0; (i < MAX_UNWANTED_VPCI) && (0 != sb->unwanted_vpci[i]); i++) { - printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n", - sb->unwanted_vpci[i]); - outl(sb->unwanted_vpci[i] + 0x7C, 0xCF8); - outl(0xDEADBEEF, 0xCFC); - } -#endif printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__); }
On Thu, Feb 14, 2008 at 8:19 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Ron: I'm stealing your ack although the patch has slightly changed function naming. Corey/Tom: Does this name match what you had in mind?
Factor out Geode LX VPCI device disabling into a separate function which consumes one device at a time. This helps avoid array handling in the dts and allows us to use generic disabling infrastructure.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Ronald G. Minnich rminnich@gmail.com
Index: LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c
--- LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c (Revision 599) +++ LinuxBIOSv3-unwantedvpci/southbridge/amd/cs5536/cs5536.c (Arbeitskopie) @@ -570,6 +570,15 @@ }
+static void hide_vpci(u32 vpci_devid) +{
/* Hide unwanted virtual PCI device. */printk(BIOS_DEBUG, "Hiding VPCI device: 0x%08X\n",vpci_devid);outl(vpci_devid + 0x7C, 0xCF8);outl(0xDEADBEEF, 0xCFC);+}
/**
- TODO.
@@ -608,16 +617,6 @@ if (sb->enable_ide) ide_init(dev);
-#warning Add back in unwanted VPCI support -#if 0
/* Disable unwanted virtual PCI devices. */for (i = 0; (i < MAX_UNWANTED_VPCI) && (0 !=sb->unwanted_vpci[i]); i++) {
printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n",sb->unwanted_vpci[i]);outl(sb->unwanted_vpci[i] + 0x7C, 0xCF8);outl(0xDEADBEEF, 0xCFC);}-#endif printk(BIOS_SPEW, "cs5536: %s() Exit\n", __FUNCTION__); }
Yeah, that works Acked-by: Corey Osgood corey.osgood@gmail.com
On 15.02.2008 02:23, Corey Osgood wrote:
On Thu, Feb 14, 2008 at 8:19 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Ron: I'm stealing your ack although the patch has slightly changed function naming. Corey/Tom: Does this name match what you had in mind?
Factor out Geode LX VPCI device disabling into a separate function which consumes one device at a time. This helps avoid array handling in the dts and allows us to use generic disabling infrastructure.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Ronald G. Minnich rminnich@gmail.com
Yeah, that works Acked-by: Corey Osgood corey.osgood@gmail.com
Thanks, r600.
Regards, Carl-Daniel
ron minnich wrote:
On Thu, Feb 14, 2008 at 2:32 PM, Carl-Daniel Hailfinger
Indeed. That would solve the problem very nicely without putting hacks into the dts.
OK, so we put nodes under the 5536, and specify 'disabled', and the 5536 code knows to actually call VSA when it sees that they are disabled. This is nice. It would use the device tree in a sensible way.
Hm, hold on. I think there is a little bit of confusion with the word "disabled" here. For a Geode VPCI device, "disabling" it means completely hiding the header from config space. If you "disable" it with VPCI, and try to do config cycles to that device, VSA will respond as if it does not exist. (returning all 1's) That behavior continues forever, unless you "enable" the device again using VSA. (you wouldn't ever do that, in general)
What does "disabled" mean for a real hardware device in the dts? I am pretty sure it does not mean "hide the header" (that is not possible in general, it is a VSA-ism).
What if I want the VPCI device to exist, but want it "disabled" in the normal dts sense of "disable"? Overloading "disabled" in the dts makes that not possible (and is also *really* confusing)
Maybe a better idea would be to just change:
/* Disable unwanted virtualized PCI devices */
to
/* Hide headers for unwanted virtualized PCI devices */
since the original words seem to have lead to this confusion.
On Thu, Feb 14, 2008 at 6:35 PM, Tom Sylla tsylla@gmail.com wrote:
ron minnich wrote:
On Thu, Feb 14, 2008 at 2:32 PM, Carl-Daniel Hailfinger
Indeed. That would solve the problem very nicely without putting hacks into the dts.
OK, so we put nodes under the 5536, and specify 'disabled', and the 5536 code knows to actually call VSA when it sees that they are disabled. This is nice. It would use the device tree in a sensible way.
Hm, hold on. I think there is a little bit of confusion with the word "disabled" here. For a Geode VPCI device, "disabling" it means completely hiding the header from config space. If you "disable" it with VPCI, and try to do config cycles to that device, VSA will respond as if it does not exist. (returning all 1's) That behavior continues forever, unless you "enable" the device again using VSA. (you wouldn't ever do that, in general)
What does "disabled" mean for a real hardware device in the dts? I am pretty sure it does not mean "hide the header" (that is not possible in general, it is a VSA-ism).
What if I want the VPCI device to exist, but want it "disabled" in the normal dts sense of "disable"? Overloading "disabled" in the dts makes that not possible (and is also *really* confusing)
Maybe a better idea would be to just change:
/* Disable unwanted virtualized PCI devices */
to
/* Hide headers for unwanted virtualized PCI devices */
since the original words seem to have lead to this confusion.
Keeping in mind that I'm no expert or really even informed with the v3/dts internals or Geode, but similar behavior is seen on the intel ICH-series and via vt8237r, would it work to add another option, call it "hidden", to the device status, and then another stage to v3? We'd do whatever stage scans the pci bridge to find all the devices, then whatever devices are marked hidden have whatever mechanism that hides them run, and v3 "forgets" the devices ever existed. i82801xx (ICH) in v2 already works to hide these devices, but I can't really remember how it works. This is just a general idea, I figure you guys have a much better idea then I do if it'd work/be possible or not.
-Corey
On Thu, Feb 14, 2008 at 06:35:35PM -0500, Tom Sylla wrote:
What does "disabled" mean for a real hardware device in the dts?
Spot on.
I am pretty sure it does not mean "hide the header" (that is not possible in general, it is a VSA-ism).
It might. ATA devices, USB, 1394 etc could be disabled => hidden in factory BIOSes.
What if I want the VPCI device to exist, but want it "disabled" in the normal dts sense of "disable"?
Why would you? And what would you expect 'the normal dts sense of "disable"' to mean in that case?
Overloading "disabled" in the dts makes that not possible (and is also *really* confusing)
Personally I would kind-of like the hiding to happen for all disabled devices.
Am I way off track?
//Peter
* Marc Jones Marc.Jones@amd.com [080214 23:16]:
+++ southbridge/amd/cs5536/dts (Arbeitskopie) @@ -57,4 +57,7 @@ com2_enable = "0"; com2_address = "0x2f8"; com2_irq = "3";
- /* Disable unwanted virtualized PCI devices */
- unwanted_vpci[0] = "0";
};
I am blissfully ignorant on the topic, but the first thing I wondered:
Should a virtual device not be disabled in the same way as a physical one?
pci@0,0 { disabled; }
?
Stefan
This is an interesting idea but the devices are CS5536 specific and I don't think we want to make dts CS5536 aware.
Yes, that's what I think, too.
We should use the standard pci mechanism rather than extending the dts with unwanted_vpci[]
I think it better use the device ID rather than the device location to do the disable.
Yes. This would be interesting for other platforms, too.
On Thu, Feb 14, 2008 at 03:16:24PM -0700, Marc Jones wrote:
This is an interesting idea but the devices are CS5536 specific and I don't think we want to make dts CS5536 aware.
I would like nothing more than to make coreboot understand the Geode architecture natively. Maybe a goal for v4, once we have truly gotten our heads around PCI. :)
I think it better use the device ID rather than the device location to do the disable.
Makes sense to me too.
Does the VSA have any default policy by the way?
//Peter
Peter Stuge wrote:
Does the VSA have any default policy by the way?
Peter,
The devices are present and available to the system. Is that what you were asking?
Marc
On Fri, Feb 15, 2008 at 11:43:48AM -0700, Marc Jones wrote:
Does the VSA have any default policy by the way?
The devices are present and available to the system. Is that what you were asking?
Right - so by default all virtual PCI devices are present and available, and any unwanted ones should be explicitly disabled.
Fits well with the new dts policy too. :)
//Peter
-
Carl-Daniel Hailfinger -
Corey Osgood -
Marc Jones -
Marc Jones
-
Peter Stuge -
ron minnich -
Stefan Reinauer -
Tom Sylla