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 { };
> + [EMAIL PROTECTED] {
> /config/("northbridge/amd/geodelx/apic");
> - enabled;
> };
> - domain0 {
> + [EMAIL PROTECTED] {
> /config/("northbridge/amd/geodelx/domain");
> - enabled;
> - pcidomain = "0";
> - device0,0 {
> - /config/("northbridge/amd/geodelx/pci");
> - enabled;
> - pcipath = "1,0";
> + [EMAIL PROTECTED],0 {
> + /config/("northbridge/amd/geodelx/pci");
> };
> - southbridge {
> + [EMAIL PROTECTED],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 {
> + [EMAIL PROTECTED] {
I think there is no consensus yet whether the [EMAIL PROTECTED] syntax is
unambiguous given that LPC can do much more than addressing via
iobase. Maybe change the statement to [EMAIL PROTECTED] or even
[EMAIL PROTECTED]: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/
--
coreboot mailing list
[email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
