I have issues with parts of this.
On Wed, Feb 13, 2008 at 10:00:21PM +0100, svn@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},
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@0 {
I'd kind of like to prepend pci_ here. "pci_domain"
/config/("northbridge/intel/i440bxemulation/dts");
bus@0 {
And here. "pci_bus"
pci@0,0 {};pci@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
Peter Stuge wrote:
I have issues with parts of this.
On Wed, Feb 13, 2008 at 10:00:21PM +0100, svn@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?)
You are right. I think it should stay SuperIO.
{ 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?
I don't even think this should be in the dts at all. The driver code should know which pci ids to bind to. And we are doing this already:
.ops = &i440bxemulation_pcidomainops},
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.
Fully agreed.
Stefan
On Thu, Feb 14, 2008 at 1:10 PM, Stefan Reinauer stepan@coresystems.de wrote:
Peter Stuge wrote:
You are right. I think it should stay SuperIO.
I'll try to put it back tomorrow.
{ 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?
I don't even think this should be in the dts at all. The driver code should know which pci ids to bind to. And we are doing this already:
This identifies the device id of this device. That's got nothing to do with the containing domain. It's the type of device. The containing domain is an instance of this type.
Any suggestions for lessening confusion? I don't know what to do here.
.ops = &i440bxemulation_pcidomainops},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.
Fully agreed.
I'll look at it tomorrow and see what I can do. Until then, ideas welcome.
We have 6 weeks until Denver, it would be nice to lock this set of issues down.
ron
Try this, simplifies the dts, removes redundant ids.
might even allow us to totally yank the pciid, domainid, etc. keywords
ron
On Thu, Feb 14, 2008 at 04:25:39PM -0800, ron minnich wrote:
Basically, this patch gets rid of the constructor arrays in the various .c files, and seperates them into individual structs.
Yes, I quite like this!
Why were there arrays in the first place? Was that introduces for the LX which has multiple devices? This new approach is definately the right thing then!
I actually thought this was the way it worked already. :p
//Peter
On Thu, Feb 14, 2008 at 4:39 PM, Peter Stuge peter@stuge.se wrote:
Why were there arrays in the first place? Was that introduces for the LX which has multiple devices? This new approach is definately the right thing then!
The arrays were introduced by me over a year ago and were probably a mistake. I was trying to avoid the craziness in v2 of using the linker to build arrays of structs -- it confused people all the time to have a struct referenced in the code that was not actually declared anywhere.
I actually thought this was the way it worked already. :p
It will soon.
ron
On 15.02.2008 01:25, ron minnich wrote:
Try this, simplifies the dts, removes redundant ids.
might even allow us to totally yank the pciid, domainid, etc. keywords
I like it.
Index: northbridge/amd/geodelx/geodelx.c
--- northbridge/amd/geodelx/geodelx.c (revision 592) +++ northbridge/amd/geodelx/geodelx.c (working copy) @@ -354,24 +354,23 @@
- The constructor for the device.
- Domain ops and APIC cluster ops and PCI device ops are different.
*/ -struct constructor geodelx_north_constructors[] = { +struct constructor geodelx_north_domain = { /* Northbridge running a PCI domain. */
- {.id = {.type = DEVICE_ID_PCI_DOMAIN,
- .id = {.type = DEVICE_ID_PCI_DOMAIN, .u = {.pci_domain = {.vendor = PCI_VENDOR_ID_AMD, .device = PCI_DEVICE_ID_AMD_LXBRIDGE}}}, .ops = &geodelx_pcidomain_ops},
- geodelx_north_apic = {
+ struct constructor geodelx_north_apic = {
/* Northbridge running an APIC cluster. */
- {.id = {.type = DEVICE_ID_APIC_CLUSTER,
.id = {.type = DEVICE_ID_APIC_CLUSTER, .u = {.apic_cluster = {.vendor = PCI_VENDOR_ID_AMD, .device = PCI_DEVICE_ID_AMD_LXBRIDGE}}}, .ops = &geodelx_apic_ops},
/* Northbridge running a PCI device. */
- {.id = {.type = DEVICE_ID_PCI,
- geodelx_north_pci = {
.id = {.type = DEVICE_ID_PCI,
.ops = &geodelx_pci_ops},- {.ops = 0},
-};
.ops = &geodelx_pci_ops};
Two comments about the style while it is not yet carved in stone. - Can you prefix every constructor with "struct constructor"? - Can we possibly use anonymous unions to get rid of that obnoxious .u? (separate patch)
Regards, Carl-Daniel
On Thu, Feb 14, 2008 at 5:31 PM, Carl-Daniel Hailfinger
Two comments about the style while it is not yet carved in stone.
- Can you prefix every constructor with "struct constructor"?
sure.
- Can we possibly use anonymous unions to get rid of that obnoxious .u?
(separate patch)
seperately.
Note one thing the arrays in the individual files got you. You could drag along ALL types of a (e.g.) the intel southbridge even when you only specified one of them in a dts. Now, to get all the seperate constructors, you're going to have to specify that you use all those parts.
That's why I did the array, I now recall, but it probably confused everyone, as did this explanation. So never mind.
It's possible we should just go back to linker sets :-) ewwww.
ron
On Thu, Feb 14, 2008 at 05:34:04PM -0800, ron minnich wrote:
Note one thing the arrays in the individual files got you. You could drag along ALL types of a (e.g.) the intel southbridge even when you only specified one of them in a dts.
*nods* Hopefully we can be smart about this also in the future.
Now, to get all the seperate constructors, you're going to have to specify that you use all those parts.
Not so elegant. But thinking about it a bit, this comes back to the dts. The dts syntax doesn't have a concept of a composite device, so maybe we should not even try to hack around it?
That's why I did the array, I now recall, but it probably confused everyone, as did this explanation. So never mind.
I agree completely with the concept, just not that implementation. :)
It's possible we should just go back to linker sets :-) ewwww.
Anything linker may turn out to be troublesome?
//Peter
OK, here is try 2, with changes per your comments.
qemu boots fine. I appreciate all the comments!
I think we're close.
Thanks
ron
On Thu, Feb 14, 2008 at 05:48:01PM -0800, ron minnich wrote:
This is not signed off yet, but is close.
It also boots qemu just fine, which is a good sign.
Great!
Note that from now on, to pull a constructor into the coreboot image and make it available, some dts somewhere has to name it.
I think this is a very safe and reasonable behavior. :)
I am almost certain we can now completely remove all the domainid, pciid, and such ugly stuff. Comments welcome.
Please do those removes in the same patch. (And some are..)
+++ southbridge/amd/cs5536/dts (working copy) @@ -19,8 +19,7 @@ */
{
- constructor = "cs5536_constructors";
- pciid = "PCI_VENDOR_ID_AMD,PCI_DEVICE_ID_AMD_CS5536_ISA";
..like here. \o/
+++ northbridge/amd/geodelx/domain (working copy) @@ -19,7 +19,6 @@ */
{
- constructor = "geodelx_north_constructors";
- domainid = "PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_LXBRIDGE";
..and here.
+++ northbridge/intel/i440bxemulation/domain (working copy) @@ -20,6 +20,6 @@
{ ramsize = "128";
- constructor = "i440bx_constructors";
- constructor = "i440bx_domain"; domainid = "0x8086, 0x7190";
..but not here.
if (!strncmp(tree->name, "lpc", 3)){
fprintf(f, "\t.path = {.type=DEVICE_PATH_SUPERIO,.u={.superio={.iobase=%s}}},\n",
fprintf(f, "\t.path = {.type=DEVICE_PATH_LPC,.u={.lpc={.iobase=%s}}},\n", path);
So the LPC/SUPERIO thing got in the middle of this.
Please have those changes be a separate patch.
Looks very good!
//Peter
On 15.02.2008 02:48, ron minnich wrote:
OK, here is try 2, with changes per your comments.
qemu boots fine. I appreciate all the comments!
I think we're close.
southbridge/intel/i82371eb/ide and northbridge/intel/i440bxemulation/domain do not exist in my local checkout. No idea how they appear to be checked in in your copy of the tree. Can you please check whether they were ever committed?
If you have a working rom with a Linux kernel, can you post both boot logs with and without the patch against svn HEAD? Thanks.
Regards, Carl-Daniel
On Thu, Feb 14, 2008 at 6:06 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
southbridge/intel/i82371eb/ide and northbridge/intel/i440bxemulation/domain do not exist in my local checkout. No idea how they appear to be checked in in your copy of the tree. Can you please check whether they were ever committed?
They are created by svn mv but that is pending on the commit.
If you have a working rom with a Linux kernel, can you post both boot logs with and without the patch against svn HEAD? Thanks.
Hmm. How badly do you need this one ... I can do it but ...
thanks
ron
On 15.02.2008 04:56, ron minnich wrote:
On Thu, Feb 14, 2008 at 6:06 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
southbridge/intel/i82371eb/ide and northbridge/intel/i440bxemulation/domain do not exist in my local checkout. No idea how they appear to be checked in in your copy of the tree. Can you please check whether they were ever committed?
They are created by svn mv but that is pending on the commit.
OK, no problem then. I was just not sure whether your tree diverged (which should be difficult with svn).
If you have a working rom with a Linux kernel, can you post both boot logs with and without the patch against svn HEAD? Thanks.
Hmm. How badly do you need this one ... I can do it but ...
It was just to compare compilation and boot logs with and without the patch. I always try to explain any differences in the boot log to make sure the changes did what I want. So if you are sure this does not break qemu at all and are willing to fix up alix1c no objections/demands from my side (no new patch needed).
Regards, Carl-Daniel
On 15.02.2008 02:48, ron minnich wrote:
OK, here is try 2, with changes per your comments.
qemu boots fine. I appreciate all the comments!
I think we're close.
Indeed. A few more comments to make this the perfect patch ;-)
This is not signed off yet, but is close.
It also boots qemu just fine, which is a good sign.
Note that from now on, to pull a constructor into the coreboot image and make it available, some dts somewhere has to name it.
I am almost certain we can now completely remove all the domainid, pciid, and such ugly stuff. Comments welcome.
Index: mainboard/pcengines/alix1c/dts
--- mainboard/pcengines/alix1c/dts (revision 592) +++ mainboard/pcengines/alix1c/dts (working copy) @@ -22,26 +22,17 @@ enabled; mainboard-vendor = "PC Engines"; mainboard-name = "ALIX1.C";
- cpus {
enabled;- };
- apic {
- cpus { };
- apic@0 { /config/("northbridge/amd/geodelx/apic");
enabled;- domain0 {
- domain@0 { /config/("northbridge/amd/geodelx/domain");
enabled;pcidomain = "0";device0,0 {/config/("northbridge/amd/geodelx/pci");enabled;pcipath = "1,0";
pci@1,0 {/config/("northbridge/amd/geodelx/pci");
southbridge {
pci@15,0 { /config/("southbridge/amd/cs5536/dts");
pcipath = "0xf,0";enabled; enable_ide = "1"; /* Interrupt enables for LPC bus. * Each bit is an IRQ 0-15. */@@ -54,7 +45,7 @@ * See virtual PIC spec. */ enable_gpio_int_route = "0x0D0C0700"; };
superio {
lpc@46 {
I think there is no consensus yet whether the lpc@addr syntax is unambiguous given that LPC can do much more than addressing via iobase. Maybe change the statement to superio@46 or even superio@46:ioport?
/config/("superio/winbond/w83627hf/dts"); com1enable = "1"; };Index: northbridge/amd/geodelx/geodelx.c
--- northbridge/amd/geodelx/geodelx.c (revision 592) +++ northbridge/amd/geodelx/geodelx.c (working copy) @@ -354,24 +354,23 @@
- The constructor for the device.
- Domain ops and APIC cluster ops and PCI device ops are different.
*/ -struct constructor geodelx_north_constructors[] = { +struct constructor geodelx_north_domain = { /* Northbridge running a PCI domain. */
- {.id = {.type = DEVICE_ID_PCI_DOMAIN,
- .id = {.type = DEVICE_ID_PCI_DOMAIN, .u = {.pci_domain = {.vendor = PCI_VENDOR_ID_AMD, .device = PCI_DEVICE_ID_AMD_LXBRIDGE}}}, .ops = &geodelx_pcidomain_ops},
- geodelx_north_apic = {
struct constructor geodelx_north_apic = {
/* Northbridge running an APIC cluster. */
- {.id = {.type = DEVICE_ID_APIC_CLUSTER,
.id = {.type = DEVICE_ID_APIC_CLUSTER, .u = {.apic_cluster = {.vendor = PCI_VENDOR_ID_AMD, .device = PCI_DEVICE_ID_AMD_LXBRIDGE}}}, .ops = &geodelx_apic_ops},
/* Northbridge running a PCI device. */
- {.id = {.type = DEVICE_ID_PCI,
- geodelx_north_pci = {
ditto.
.id = {.type = DEVICE_ID_PCI,
.ops = &geodelx_pci_ops},- {.ops = 0},
-};
.ops = &geodelx_pci_ops};
[...]
Index: util/dtc/flattree.c
--- util/dtc/flattree.c (revision 593) +++ util/dtc/flattree.c (working copy) @@ -556,7 +556,7 @@ path); } if (!strncmp(tree->name, "lpc", 3)){
fprintf(f, "\t.path = {.type=DEVICE_PATH_SUPERIO,.u={.superio={.iobase=%s}}},\n",
fprintf(f, "\t.path = {.type=DEVICE_PATH_LPC,.u={.lpc={.iobase=%s}}},\n",
See above for the "LPC can do much more than ioports" argument.
path); }} @@ -727,13 +727,13 @@ /* find any/all properties with the name constructor */ for_each_config(tree, prop) { if (streq(prop->name, "constructor")){
printf("\t%s,\n", prop->val.val);
printf("\t&%s,\n", prop->val.val);} }
for_each_property(tree, prop) { if (streq(prop->name, "constructor")){
printf("\t%s,\n", prop->val.val);
printf("\t&%s,\n", prop->val.val);@@ -790,13 +790,13 @@ for_each_config(tree, prop) { if (! streq(prop->name, "constructor")) /* this is special */ continue;
fprintf(f, "extern struct constructor %s[];\n", prop->val.val);
fprintf(f, "extern struct constructor %s;\n", prop->val.val);}
for_each_property(tree, prop) { if (! streq(prop->name, "constructor")) /* this is special */ continue;
fprintf(f, "extern struct constructor %s[];\n", prop->val.val);
fprintf(f, "extern struct constructor %s;\n", prop->val.val);}
for_each_property(tree, prop) {
Regards, Carl-Daniel
I will change lpc to superio in a follow on patch, don't worry. My mistake.
thanks
ron
-
Carl-Daniel Hailfinger -
Peter Stuge -
ron minnich -
Stefan Reinauer