[coreboot] r593 -

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Fri Feb 15 08:31:42 CET 2008


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 at 0 {
>  		/config/("northbridge/amd/geodelx/apic");
> -		enabled;
>  	};
> -	domain0 {
> +	domain at 0 {
>  		/config/("northbridge/amd/geodelx/domain");
> -		enabled;
> -		pcidomain = "0";
> -		device0,0 {
> -		/config/("northbridge/amd/geodelx/pci");
> -			enabled;
> -			pcipath = "1,0";
> +		pci at 1,0 {
> +			/config/("northbridge/amd/geodelx/pci");
>  		};
> -		southbridge {
> +		pci at 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 at 46 {

I think there is no consensus yet whether the lpc at addr syntax is
unambiguous given that LPC can do much more than addressing via
iobase. Maybe change the statement to superio at 46 or even
superio at 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

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list