Subtractive Resources

[Myles Watson]

I'm running into a problem when resources are read.

The amd8111 adds two subtractive resources on link 0.  The problem is that
it has no link 0.

static void amd8111_lpc_read_resources(struct device * dev)
{
    struct resource *res;

printk(BIOS_DEBUG,"%s calls read_resources with %s bus %s \n",
            __func__, dev->dtsname, dev->bus? dev->bus->dev->dtsname:
"NULL");
    /* Get the normal pci resources of this device */
    pci_dev_read_resources(dev);

/* Add an extra subtractive resource for both memory and I/O */
    res = new_resource(dev, IOINDEX_SUBTRACTIVE(0, 0));
    res->flags = IORESOURCE_IO | IORESOURCE_SUBTRACTIVE |
IORESOURCE_ASSIGNED;

res = new_resource(dev, IOINDEX_SUBTRACTIVE(1, 0));
    res->flags = IORESOURCE_MEM | IORESOURCE_SUBTRACTIVE |
IORESOURCE_ASSIGNED;
    printk(BIOS_DEBUG,"%s after read_resources\n", __func__);
}

Either of these changes fix it, but I'd like a little more understanding of
why we're trying to do this.

I like the first change better because it quits trying sooner.

Index: device/device.c
===================================================================
--- device/device.c     (revision 984)
+++ device/device.c     (working copy)
@@ -311,7 +311,7 @@

/* Read in subtractive resources behind the current device.
*/
                links = 0;
-               for (i = 0; i < curdev->resources; i++) {
+               for (i = 0; i < curdev->resources && curdev->links>0; i++) {
                        struct resource *resource;
                        unsigned int link;
                        resource = &curdev->resource[i];
@@ -326,7 +326,8 @@
                        }
                        if (!(links & (1 << link))) {
                                links |= (1 << link);
-                               read_resources(&curdev->link[link]);
+                               if (curdev->link[link].dev)
+                                       read_resources(&curdev->link[link]);
                        }
                }
        }

Signed-off-by: Myles Watson

Thanks,
Myles

[ron minnich]

On Fri, Nov 7, 2008 at 7:18 AM, Myles Watson  wrote:
> I'm running into a problem when resources are read.
>
> The amd8111 adds two subtractive resources on link 0.  The problem is that
> it has no link 0.

This is usually a sign that I got the dtc wrong somehow.

Here is v2:
struct device _dev15 = {
        .ops = 0,
        .bus = &_dev8.link[0],
        .path = {.type=DEVICE_PATH_PCI,.u={.pci={ .devfn = PCI_DEVFN(0x0,0)}}},
        .enabled = 1,
        .on_mainboard = 1,
        .link = {
                [0] = {
                        .link = 0,
                        .dev = &_dev15,
                        .children = &_dev16,
                },
        },
        .links = 1,
        .sibling = &_dev20,
        .chip_ops = &southbridge_amd_amd8111_ops,
        .chip_info = &southbridge_amd_amd8111_info_14,
        .next=&_dev16
};

and

struct device _dev16 = {
        .ops = 0,
        .bus = &_dev15.link[0],
        .path = {.type=DEVICE_PATH_PCI,.u={.pci={ .devfn = PCI_DEVFN(0x0,0)}}},
        .enabled = 1,
        .on_mainboard = 1,
        .link = {
        },
        .links = 0,
        .sibling = &_dev17,
        .chip_ops = &southbridge_amd_amd8111_ops,
        .chip_info = &southbridge_amd_amd8111_info_14,
        .next=&_dev17
};

So the amd8111 is a bridge that needs resources.

That said, you said your change fixes it. Does that mean serengeti
works? It's hard to see how given that this is a bridge.

ron

[Myles Watson]

> -----Original Message-----
> From: ron minnich [mailto:[e-mail]]
> Sent: Friday, November 07, 2008 9:24 AM
> To: Myles Watson
> Cc: Coreboot
> Subject: Re: Subtractive Resources
>
> On Fri, Nov 7, 2008 at 7:18 AM, Myles Watson  wrote:
> > I'm running into a problem when resources are read.
> >
> > The amd8111 adds two subtractive resources on link 0.  The problem is
>
> that
> > it has no link 0.
>
> This is usually a sign that I got the dtc wrong somehow.
>
> Here is v2:
> struct device _dev15 = {
>         .ops = 0,
>         .bus = &_dev8.link[0],
>         .path = {.type=DEVICE_PATH_PCI,.u={.pci={ .devfn =
> PCI_DEVFN(0x0,0)}}},
>         .enabled = 1,
>         .on_mainboard = 1,
>         .link = {
>                 [0] = {
>                         .link = 0,
>                         .dev = &_dev15,
>                         .children = &_dev16,
>                 },
>         },
>         .links = 1,
>         .sibling = &_dev20,
>         .chip_ops = &southbridge_amd_amd8111_ops,
>         .chip_info = &southbridge_amd_amd8111_info_14,
>         .next=&_dev16
> };
>
> and
>
> struct device _dev16 = {
>         .ops = 0,
>         .bus = &_dev15.link[0],
>         .path = {.type=DEVICE_PATH_PCI,.u={.pci={ .devfn =
> PCI_DEVFN(0x0,0)}}},
>         .enabled = 1,
>         .on_mainboard = 1,
>         .link = {
>         },
>         .links = 0,
>         .sibling = &_dev17,
>         .chip_ops = &southbridge_amd_amd8111_ops,
>         .chip_info = &southbridge_amd_amd8111_info_14,
>         .next=&_dev17
> };
>
> So the amd8111 is a bridge that needs resources.

My understanding is that the amd8111 _has_ a bridge, but the device that is
causing the problem is lpc, which is not a bridge, nor behind the amd8111's
bridge.  So when its resources are read and it is found to have subtractive
resources, the code tries to descend.  It doesn't have a bus, and this
fails.
> That said, you said your change fixes it. Does that mean serengeti
> works? It's hard to see how given that this is a bridge.

Sorry I wasn't more clear.  I meant that the fix no longer tries to call
functions with NULL pointers.  I don't think the dts is wrong, but I think
that there are a lot fewer intermediaries in v3 then there were in v2.

Thanks,
Myles

[ron minnich]

On Fri, Nov 7, 2008 at 8:46 AM, Myles Watson  wrote:
> My understanding is that the amd8111 _has_ a bridge, but the device that is
> causing the problem is lpc, which is not a bridge, nor behind the amd8111's
> bridge.  So when its resources are read and it is found to have subtractive
> resources, the code tries to descend.  It doesn't have a bus, and this
> fails.

you are right. I missed it. That's a good catch.

Do you need those resources added for correct operation?

Even if we had a link, does it make sense to descend the link to read
subtractive resources?

I like patch 1 but at the same time it feels like maybe we're not
getting at the right problem. If we're that point in the code, and
reading links, why are the links not there?
> Sorry I wasn't more clear.  I meant that the fix no longer tries to call
> functions with NULL pointers.  I don't think the dts is wrong, but I think
> that there are a lot fewer intermediaries in v3 then there were in v2.

Well, that I  like to hear.

Marc made the case that things such as superio should not even be
"under" the lpc in the dts, since they stand "outside" the tree in
some sense. He argued that we should instead put them at top level.
There is merit to his argument. This would simplify the lpc code as
well.

Still, this is starting to converge to a more sensible block of code.

ron

[Myles Watson]

> -----Original Message-----
> From: ron minnich [mailto:[e-mail]]
> Sent: Friday, November 07, 2008 9:55 AM
> To: Myles Watson
> Cc: Coreboot
> Subject: Re: Subtractive Resources
>
> On Fri, Nov 7, 2008 at 8:46 AM, Myles Watson  wrote:
> > My understanding is that the amd8111 _has_ a bridge, but the device that
>
> is
> > causing the problem is lpc, which is not a bridge, nor behind the
>
> amd8111's
> > bridge.  So when its resources are read and it is found to have
>
> subtractive
> > resources, the code tries to descend.  It doesn't have a bus, and this
> > fails.
>
> you are right. I missed it. That's a good catch.
>
> Do you need those resources added for correct operation?

They get added fine.  That's why I wanted to know why that code was there.
> Even if we had a link, does it make sense to descend the link to read
> subtractive resources?

I don't know.
> I like patch 1 but at the same time it feels like maybe we're not
> getting at the right problem. If we're that point in the code, and
> reading links, why are the links not there?

I agree.  It does happen when it goes through the domain, which I think is
correct, and has no problems.
> > Sorry I wasn't more clear.  I meant that the fix no longer tries to call
> > functions with NULL pointers.  I don't think the dts is wrong, but I
>
> think
> > that there are a lot fewer intermediaries in v3 then there were in v2.
>
> Well, that I  like to hear.
>
> Marc made the case that things such as superio should not even be
> "under" the lpc in the dts, since they stand "outside" the tree in
> some sense. He argued that we should instead put them at top level.
> There is merit to his argument. This would simplify the lpc code as
> well.

I think it's fine to have the lpc define resources since that's who
implements them, but I like the idea of the resource being visible from the
domain (maybe not outside the tree.)

Thanks,
Myles

[Marc Jones]

----- Original Message ----
> From: Myles Watson
> > -----Original Message-----
> > From: ron minnich [mailto:[e-mail]]
> > On Fri, Nov 7, 2008 at 8:46 AM, Myles Watson wrote:

.....
> > Marc made the case that things such as superio should not even be
> > "under" the lpc in the dts, since they stand "outside" the tree in
> > some sense. He argued that we should instead put them at top level.
> > There is merit to his argument. This would simplify the lpc code as
> > well.
>
> I think it's fine to have the lpc define resources since that's who
> implements them, but I like the idea of the resource being visible from the
> domain (maybe not outside the tree.)

I think you either have a separate legacy/subtractive/isa/lpc domain or it needs to be under the isa/lpc pci device. What I didn't really like was that it was uncontained in the domain. It was just sitting at the top level pci bus. BUT that is just a nit. Documented correctly etc I could live with it.

Marc

[Myles Watson]

> -----Original Message-----
> From: Marc Jones [mailto:[e-mail]]
> Sent: Friday, November 07, 2008 11:43 AM
> To: Myles Watson; ron minnich
> Cc: Coreboot
> Subject: Re: [coreboot] Subtractive Resources
>
> ----- Original Message ----
> > From: Myles Watson
> > > -----Original Message-----
> > > From: ron minnich [mailto:[e-mail]]
> > > On Fri, Nov 7, 2008 at 8:46 AM, Myles Watson wrote:
>
> .....
> > > Marc made the case that things such as superio should not even be
> > > "under" the lpc in the dts, since they stand "outside" the tree in
> > > some sense. He argued that we should instead put them at top level.
> > > There is merit to his argument. This would simplify the lpc code as
> > > well.
> >
> > I think it's fine to have the lpc define resources since that's who
> > implements them, but I like the idea of the resource being visible from
>
> the
> > domain (maybe not outside the tree.)
>
> I think you either have a separate legacy/subtractive/isa/lpc domain or it
> needs to be under the isa/lpc pci device. What I didn't really like was
> that it was uncontained in the domain.

I agree.
> It was just sitting at the top
> level pci bus. BUT that is just a nit. Documented correctly etc I could
> live with it.

Let's try to put it in the right place.  I don't think we want to wait until
v4 :)

Thanks,
Myles