Re: [coreboot] dtc patch: make all names unique, misc. cleanup, more to come

[ron minnich]

On Wed, Sep 3, 2008 at 8:20 PM, Carl-Daniel Hailfinger
 wrote:
> > struct device dev_root = {
> >       .path = { .type = DEVICE_PATH_ROOT },
> >       .next = &dev_cpu_0,
> >       .link = {
> >               [0] = {
> >                       .dev = &dev_root,
> >                       .link = 0,
> >                       .children = &dev_cpu_0
> >               },
> >               [1] = {
> >                       .dev = &dev_root,
> >                       .link = 0,
> >                       .children = &dev_apic_0
> >               },
> >               [2] = {
> >                       .dev = &dev_root,
> >                       .link = 0,
> >                       .children = &dev_domain_0
> >               },
> >       },
> >       .links = 3,
> >       .bus = &dev_root.link[0],
> >       .next = &dev_cpu_0,
> >       .ops = &default_dev_ops_root,
> >       .dtsname = "root",
> >       .enabled = 1
> > };
> >
> > Multiple links that work. Does this work? I don't know.
>
> I'll try to modify a qemu target for testing and report back.

Thanks
> > I am not sure this it totally right yet, however:
> > struct device dev_domain_0 = {
> >       .path = {.type=DEVICE_PATH_PCI_DOMAIN,{.pci_domain={ .domain = 0x0 }}},
> >       .device_configuration = &domain_0,
> >       .ops = &geodelx_north_domain,
> >       .link = {
> >               [0] = {
> >                       .dev = &dev_domain_0,
> >                       .link = 0,
> >                       .children = &dev_pci_0_0_1_0
> >               },
> >           [1] = {
> >                   .dev = &dev_domain_0,
> >                   .link = 0,
> >                   .children = &dev_pci_0_0_f_0
> >           },
> >           [2] = {
> >                   .dev = &dev_domain_0,
> >                   .link = 0,
> >                   .children = &dev_pci_0_0_f_2
> >           },
>
> The list of links "feels" wrong. I can't yet express that feeling in
> technical terms, but I'll revisit this issue tomorrow.

it is wrong. :-)

Let's review links.

Links came in IIRC because of the k8. We needed a way to express the
HT links. It gets weird because on the k8, the HT address space is in
PCI space (this is actually a very nice thing AMD did). There are
three HT links, and they are all addressed as 0:18.0.

I never felt they way we expressed links in v2 was as clear as we
might have made them. How do you know a link is there?
It's not really explicitly named. You see this kind of comment in v2 Config.lb:

device pci 18.0 on #  northbridge
                                #  devices on link 0, link 0 == LDT 0

What would trigger that? How do we get to link 1? Turns out it happens
if we see this again:
                        device pci 18.0 on #  northbridge

config tool "knows" that it is now link 1. See tyan/s2892. It's not
just for HT either. And two children of a device in v2 having the same
path become links.

struct device _dev8 = {
        .ops = 0,
        .bus = &_dev6.link[0],
        .path = {.type=DEVICE_PATH_PCI,.u={.pci={ .devfn = PCI_DEVFN(0x18,0)}}},
        .enabled = 1,
        .on_mainboard = 1,
        .link = {
                [0] = {
                        .link = 0,
                        .dev = &_dev8,
                        .children = &_dev10,
                },
                [1] = {
                        .link = 1,
                        .dev = &_dev8,
                },
                [2] = {
                        .link = 2,
                        .dev = &_dev8,
                        .children = &_dev72,
                },
        },
        .links = 3,
        .sibling = &_dev79,
        .next=&_dev10
};

dev10 is one pci bus, dev72 is the ck804.

Again, links come into play when multiple devices have the same path,
e.g. HT links on 18:0.0 (three of them) or I2C mux (2 of them). Again,
see tyan s2892.

So in the dts, what would this look like? Well, if we had:

pci@18,0 {};
pci@18,0 {};
pci@18,0 {};

this might become three links. This is the v2 way of doing it. What I
wanted to do was something like this:
bus@0 {pci@18,0 {};};
bus@1 {pci@18,1 {};};

and so on. Really, if a bus is only ever going to be a link, maybe we
should rename "bus" to "link" and leave it at that.

So much for links.

Also, I see your point but:
struct device dev_pci_domain_0_bus_0_dev_1_fn_0

is just far more wordy than I like. Bear in mind that the primary goal
of this patch is to make the names unique.
Human readability is a secondary goal (no one ever looks at these
names anyway) and I think the long name you propose
is more than I would like to read all the time :-)

Thanks again

ron