dtc patch: make all names unique, misc. cleanup, more to come

[ron minnich]

This resolves the "unique name" problem. It sets up for k8 CPUs with
multiple busses per chip.

Comments welcome. I realize this code is not ideal :-)

ron

dtc patch: make all names unique, misc. cleanup, more to come

[Carl-Daniel Hailfinger]

On 04.09.2008 02:30, ron minnich wrote:
> This resolves the "unique name" problem. It sets up for k8 CPUs with
> multiple busses per chip.
>
> Comments welcome. I realize this code is not ideal :-)

Forgive me, but I'd like to review this in depth and that might take
some time. I'm also doubting that the new scheme is really more readable
than my proposal from a few days ago.

Regards,
Carl-Daniel

[ron minnich]

On Wed, Sep 3, 2008 at 5:34 PM, Carl-Daniel Hailfinger
 wrote:
> Forgive me, but I'd like to review this in depth and that might take
> some time. I'm also doubting that the new scheme is really more readable
> than my proposal from a few days ago.

ok, but I hope you'll see it is. The names of structs now have the
path in them. Create statictree.[ch] and you'll see what I mean.

But let me know.

Go ahead and resend your one from a few days ago and I'll look at it.
Keep in mind that this new stuff I'm doing is also there to support
multiple links per chip -- that's going in now. Currently we only
support one link per chip. We do need the inherited domain/bus/dev/fn
that I put in the dtc node struct (see dtc.h) to make this bus stuff
work.

ron

[ron minnich]

btw, I'm pretty happy with how this previous patch is working. It took
me about 5 mins to make the last mod and now I have this:

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 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
    	},
    },
    .links = 3,
    .bus = &dev_root.link[2],
    .dtsname = "domain_0",
    .enabled = 1
};

Anyway, no further progess is possible on k8 until this is working as
the v2 config tool did for multiple links. It's the current
showstopper. So we need to solve it. I'm totally out of the loop
starting, well, very soon now and for next 10 days, so those with a
better idea are welcome to implement it :-)

ron

dtc patch: make all names unique, misc. cleanup, more to come

[Carl-Daniel Hailfinger]

On 04.09.2008 02:59, ron minnich wrote:
> btw, I'm pretty happy with how this previous patch is working. It took
> me about 5 mins to make the last mod and now I have this:

Could you please resend your latest patch? Thanks.
> 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.
> 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.
One thing would be that PCI devices are now direct children of the
domain, not of the bus. That may cause problems.
> },
>    .links = 3,
>    .bus = &dev_root.link[2],
>    .dtsname = "domain_0",
>    .enabled = 1
> };
>
> Anyway, no further progess is possible on k8 until this is working as
> the v2 config tool did for multiple links. It's the current
> showstopper. So we need to solve it. I'm totally out of the loop
> starting, well, very soon now and for next 10 days, so those with a
> better idea are welcome to implement it :-)

I'll look at v2 again and try to compare.

Regards,
Carl-Daniel

[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

dtc patch: make all names unique, misc. cleanup, more to come

[Carl-Daniel Hailfinger]

On 04.09.2008 17:22, ron minnich wrote:
> On Wed, Sep 3, 2008 at 8:20 PM, Carl-Daniel Hailfinger
>  wrote:
> > > 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.

Yes, and that implicit magic is something I want to avoid in v3. Sure,
it's clever, but how many people will ever understand it?
> 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.

You correctly identified the biggest question from a conceptual POV. Is
a bus only a link? Or is a bus a device that hangs off its parent and
has its own children?

My current idea is:
Treat every dts node except "ht" normally. Nodes below a "ht" node don't
get added as multiple siblings below a single link/bus, but they each
end up as single sibling, each on its own link. The dts bus number is
treated as link number in generated code
I'll have to revisit that idea, though. It feels not quite right.
> 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.

Yes.
> 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 :-)

True. If we don't have a solution by Tuesday, I'll change my patch to
fit your naming taste.
The dtsname printed during coreboot bootup needs vast improvements,
though. It is barely readable both for current svn and any of our
proposals. I propose to offer specifying it in the dts. Having print
coreboot "First RTL8139 Network Card on bottom of board (PCI: 00:01.1)"
instead of "pci_0_0_1_1(PCI: 00:01.1)" is the difference between easy
and cryptic.

Regards,
Carl-Daniel

dtc patch: make all names unique, misc. cleanup, more to come

[Carl-Daniel Hailfinger]

On 04.09.2008 02:30, ron minnich wrote:
> This resolves the "unique name" problem. It sets up for k8 CPUs with
> multiple busses per chip.
>
> Comments welcome. I realize this code is not ideal :-)
>
> Cleanup. Not totally yet. But it's better and it works with dbe62.
>
> Basically, variables such as domain, bus, etc. are now inherited from the parent.
>
> Label_tree has been fixed to do lots of tree annotation and cleanup.
>
> One whole un-needed function is gone.

But the patch added 90 lines of code in total. We may need to trim that
down.
> We're going to soon be able to do stuff like bus@3{} bus@2{} etc. which we 
> need to support k8.
>
> This code "needs more love" as ward puts it but this is an improvement.
>
> Also, names are now unique:
> struct device dev_pci_0_0_1_0
>
> which is domain 0, bus 0, dev 1, fn 0.

My personal preferences for the name would be:
struct device dev_pci_domain_0_bus_0_dev_1_fn_0
or
struct device dev_pci_domain_0_bus_0_devfn_1_0
or
struct device dev_pci_domain0_bus0_dev1_fn0

Basically, pci_0_0_1_0 is not that intuitive.
> Signed-off-by: Ronald G. Minnich

I have not reviewed the code itself yet, but the output (except problems
with lots of additional linebreaks in inopportune places) is definitely
an improvement.
It would be cool if you could write in the changelog that your patch
initializes the dev.path for CPU devices (was zero before).

Once I fully understand your patch and its side effects, I'll probably ack.

Regards,
Carl-Daniel