Re: [coreboot] r593 -

[Carl-Daniel Hailfinger]

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,
> .u = {.pci = {.vendor = PCI_VENDOR_ID_AMD,
> 	      .device = PCI_DEVICE_ID_AMD_LXBRIDGE}}},
> .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