[PATCH] Simplify device enabling and initialization

[Myles Watson]

If we initialize and enable devices in the order that they are found
in the tree, instead of the order that they were added to the list, it
simplifies the code.  It also makes it so that removing a device from
the devicetree.cb file won't change when its resources are enabled.

I had the devices initialize depth first, but changed it to breadth
first.  I think it makes more sense to have all the children of a
device initialized together.  I could be talked out of that.

This patch breaks the s2881, which was doing some odd acrobatics in
order to get a device initialized after its parent.  It should be an
easy fix to do it correctly now, but I don't have an s2881 to test on.
 Ward?

Changes:
1. Remove global enable_resources(child) and calls to it from bridges
2. Make root_dev_* functions local to the root_device
3. Make ".init=init_func," consistent.  Some were using ".init=&init_func,"
4. Remove comments about indirect mutual recursion since it isn't anymore

Signed-off-by: Myles Watson

Thanks,
Myles

[Stefan Reinauer]

Hi Myles,

I like the patch, but it looks like you're wiping calls to adt7463
initialization on the s2881 completely with no replacement. is that on
purpose?

Stefan

On 6/16/10 10:50 PM, Myles Watson wrote:
> Index: svn/src/mainboard/tyan/s2881/mainboard.c
>
> --- svn.orig/src/mainboard/tyan/s2881/mainboard.c
> +++ svn/src/mainboard/tyan/s2881/mainboard.c
> @@ -98,56 +98,6 @@ static void adt7463_init(device_t dev)
>    printk(BIOS_DEBUG, "ADT7463 properly initialized\n");
>  }
>
> -static void dummy_noop(device_t dummy)
> -{
> -}
>
> -static struct device_operations dummy_operations = {
> .read_resources		= dummy_noop,
> .set_resources		= dummy_noop,
> .enable_resources	= dummy_noop,
> .init			= adt7463_init,
>
> -};
>
> -static unsigned int scan_root_bus(device_t root, unsigned int max)
> -{
> struct device_path path;
> device_t dummy;
> max = root_dev_scan_bus(root, max);
> printk(BIOS_DEBUG, "scan_root_bus ok\n");
> /* The following is a little silly. We need a hook into the boot
> * process *after* the ADT7463 device has been initialized. So we
> * create this dummy device, and we put the ADT7463 S2881 specific
> * settings in its init function, which gets called
> * as the last device to be initialized.
> */
> path.type = DEVICE_PATH_PNP;
> path.pnp.port = 0;
> path.pnp.device = 0;
> dummy = alloc_dev(root->link_list, &path);
> dummy->ops = &dummy_operations;
> return max;
>
> -}
>
> -static struct device_operations mainboard_operations = {
> .read_resources		= root_dev_read_resources,
> .set_resources		= root_dev_set_resources,
> .enable_resources	= root_dev_enable_resources,
> .init			= root_dev_init,
> .scan_bus		= scan_root_bus,
>
> -};
>
> -static void enable_dev(struct device *dev)
> -{
> dev->ops = &mainboard_operations;
>
> -}
>
> struct chip_operations mainboard_ops = {
>    CHIP_NAME("Tyan S2881 Mainboard")
> .enable_dev = enable_dev,
>
> };

[Myles Watson]

On Wed, Jun 16, 2010 at 4:13 PM, Stefan Reinauer  wrote:
> Hi Myles,
>
> I like the patch, but it looks like you're wiping calls to adt7463
> initialization on the s2881 completely with no replacement. is that on
> purpose?

Yes.  I know it breaks the s2881, but I'm not sure where to put the
code and I have no way to test it.  I was hoping Ward would still have
one.

Thanks,
Myles

[Ward Vandewege]

On Wed, Jun 16, 2010 at 02:50:42PM -0600, Myles Watson wrote:
> This patch breaks the s2881, which was doing some odd acrobatics in
> order to get a device initialized after its parent.  It should be an
> easy fix to do it correctly now, but I don't have an s2881 to test on.
>  Ward?

Yep, I've got (the guts) of an s2881 lying on my desk here, and can test any
patches you throw at me :)

Thanks,
Ward.

[Myles Watson]

On Wed, Jun 16, 2010 at 6:22 PM, Ward Vandewege  wrote:
> On Wed, Jun 16, 2010 at 02:50:42PM -0600, Myles Watson wrote:
> > This patch breaks the s2881, which was doing some odd acrobatics in
> > order to get a device initialized after its parent.  It should be an
> > easy fix to do it correctly now, but I don't have an s2881 to test on.
> >  Ward?
>
> Yep, I've got (the guts) of an s2881 lying on my desk here, and can test any
> patches you throw at me :)

Great.  Here are two patches.

The first one enables the driver exactly like it was before, only as a driver.

The second one tries to make it less board specific.  Putting the fan
settings in the device tree would complete that effort, I think.

Suggestions welcome.  Thanks for testing!

Signed-off-by: Myles Watson

Thanks,
Myles

[Ward Vandewege]

Hi Myles,

Everything seems fine with either patch - but there are some differences in
the boot output.

I also ran the 'sensors' command.

Output here:

http://ward.vandewege.net/coreboot/s2881/20100617-myles/

I ran 4 tests: stock r5635 (head), stock r5632 (revision prior to this
changeset), r5635 + patch 1 and r5635 + patch 2.

Let me know if you need anything else...

Thanks,
Ward.

[Myles Watson]

On Sun, Jun 20, 2010 at 8:11 PM, Ward Vandewege  wrote:
> Hi Myles,
>
> Everything seems fine with either patch - but there are some differences in
> the boot output.
>
> I also ran the 'sensors' command.
>
> Output here:
>
> http://ward.vandewege.net/coreboot/s2881/20100617-myles/
>
> I ran 4 tests: stock r5635 (head), stock r5632 (revision prior to this
> changeset), r5635 + patch 1 and r5635 + patch 2.

Thanks for testing.

It looks like only 5632 has the "ADT7463 properly initialized"
message.  One problem is that patch 2 was meant to be applied after
patch 1, so the device didn't end up in the tree for that run.  I'll
have to think about why the only message from the new device with
patch 1 is "I2C: 00:d0 missing read_resources"  For some reason it
doesn't look like it got the correct ops.

I wonder why the temperature values look right in all cases.  Does it
need to be cold booted in order for the initialization to be needed?

Thanks,
Myles

Thanks,
Myles

[Myles Watson]

On Mon, Jun 21, 2010 at 5:59 AM, Myles Watson  wrote:
> On Sun, Jun 20, 2010 at 8:11 PM, Ward Vandewege  wrote:
> > Hi Myles,
> >
> > Everything seems fine with either patch - but there are some differences in
> > the boot output.
> >
> > I also ran the 'sensors' command.
> >
> > Output here:
> >
> > http://ward.vandewege.net/coreboot/s2881/20100617-myles/
> >
> > I ran 4 tests: stock r5635 (head), stock r5632 (revision prior to this
> > changeset), r5635 + patch 1 and r5635 + patch 2.
>
> Thanks for testing.
>
> It looks like only 5632 has the "ADT7463 properly initialized"
> message.  One problem is that patch 2 was meant to be applied after
> patch 1, so the device didn't end up in the tree for that run.  I'll
> have to think about why the only message from the new device with
> patch 1 is "I2C: 00:d0 missing read_resources"  For some reason it
> doesn't look like it got the correct ops.
>
> I wonder why the temperature values look right in all cases.  Does it
> need to be cold booted in order for the initialization to be needed?

The ADM1027 doesn't expect to have children, so it has no scan_bus
method.  I had thought that the ADM1027 was some kind of a controller
for the ADT4763, but it looks like the same type of device.  Is there
really an ADM1027 on your board?  I don't see it in your sensors
output.

So...  the first two patches are the same as before.  The third patch
adds a scan_bus method to the ADM1027 so that the ADT4763 can be
initialized, and the fourth patch replaces the ADM1027 with the
ADT4763 in the device tree, and removes the third patch.

I'd be interested in head + 1 + 2 + 3, and head + 1 + 2 + 3 + 4.

There may be some initialization that was done by the ADM1027 code
that needs to be moved to the ADT7463 code, since both were being run.

So 1 + 2 + 3 should give behavior that is closer to the original code,
but if there is no ADM1027, the right thing to do is 1 + 2 + 3 + 4 +
some ADM1027 code if necessary.

Signed-off-by: Myles Watson

Thanks,
Myles

[Myles Watson]

If there is no adm1027, then the adt4763 needs to be initialized, not
its parent.  Updated patch 4 attached.

Signed-off-by: Myles Watson

Thanks,
Myles

[Ward Vandewege]

On Mon, Jun 21, 2010 at 07:36:39AM -0600, Myles Watson wrote:
> On Mon, Jun 21, 2010 at 5:59 AM, Myles Watson  wrote:
> > On Sun, Jun 20, 2010 at 8:11 PM, Ward Vandewege  wrote:
> > > Hi Myles,
> > >
> > > Everything seems fine with either patch - but there are some differences in
> > > the boot output.
> > >
> > > I also ran the 'sensors' command.
> > >
> > > Output here:
> > >
> > > http://ward.vandewege.net/coreboot/s2881/20100617-myles/
> > >
> > > I ran 4 tests: stock r5635 (head), stock r5632 (revision prior to this
> > > changeset), r5635 + patch 1 and r5635 + patch 2.
> >
> > Thanks for testing.
> >
> > It looks like only 5632 has the "ADT7463 properly initialized"
> > message.  One problem is that patch 2 was meant to be applied after
> > patch 1, so the device didn't end up in the tree for that run.  I'll
> > have to think about why the only message from the new device with
> > patch 1 is "I2C: 00:d0 missing read_resources"  For some reason it
> > doesn't look like it got the correct ops.
> >
> > I wonder why the temperature values look right in all cases.  Does it
> > need to be cold booted in order for the initialization to be needed?
>
> The ADM1027 doesn't expect to have children, so it has no scan_bus
> method.  I had thought that the ADM1027 was some kind of a controller
> for the ADT4763, but it looks like the same type of device.  Is there
> really an ADM1027 on your board?  I don't see it in your sensors
> output.
>
> So...  the first two patches are the same as before.  The third patch
> adds a scan_bus method to the ADM1027 so that the ADT4763 can be
> initialized, and the fourth patch replaces the ADM1027 with the
> ADT4763 in the device tree, and removes the third patch.
>
> I'd be interested in head + 1 + 2 + 3, and head + 1 + 2 + 3 + 4.

See

http://ward.vandewege.net/coreboot/s2881/20100621-myles/

The temperature differences (higher readouts in head + 1 + 2 + 3 + 4 seem to
be the consequence of less than optimal cooling of the board - and the fact
that it was entirely cooled off before the first boot (head + 1 + 2 + 3)
only.

Thanks,
Ward.

[Myles Watson]

> > The ADM1027 doesn't expect to have children, so it has no scan_bus
> > method.  I had thought that the ADM1027 was some kind of a controller
> > for the ADT4763, but it looks like the same type of device.  Is there
> > really an ADM1027 on your board?  I don't see it in your sensors
> > output.
> >
> > So...  the first two patches are the same as before.  The third patch
> > adds a scan_bus method to the ADM1027 so that the ADT4763 can be
> > initialized, and the fourth patch replaces the ADM1027 with the
> > ADT4763 in the device tree, and removes the third patch.
> >
> > I'd be interested in head + 1 + 2 + 3, and head + 1 + 2 + 3 + 4.
>
> See
>
> http://ward.vandewege.net/coreboot/s2881/20100621-myles/

Thanks for testing Ward!  As far as I can see, both worked, but 1+2+3+4 is
cleaner.  It doesn't look like there is an ADM1027 on your board.

Is there something missing before an Ack & commit?

Thanks,
Myles

[Ward Vandewege]

On Tue, Jun 22, 2010 at 01:30:45PM -0600, Myles Watson wrote:
> > > The ADM1027 doesn't expect to have children, so it has no scan_bus
> > > method.  I had thought that the ADM1027 was some kind of a controller
> > > for the ADT4763, but it looks like the same type of device.  Is there
> > > really an ADM1027 on your board?  I don't see it in your sensors
> > > output.
> > >
> > > So...  the first two patches are the same as before.  The third patch
> > > adds a scan_bus method to the ADM1027 so that the ADT4763 can be
> > > initialized, and the fourth patch replaces the ADM1027 with the
> > > ADT4763 in the device tree, and removes the third patch.
> > >
> > > I'd be interested in head + 1 + 2 + 3, and head + 1 + 2 + 3 + 4.
> >
> > See
> >
> > http://ward.vandewege.net/coreboot/s2881/20100621-myles/
>
> Thanks for testing Ward!  As far as I can see, both worked, but 1+2+3+4 is
> cleaner.  It doesn't look like there is an ADM1027 on your board.
>
> Is there something missing before an Ack & commit?

I think it's good. Thanks for writing the patches!

Acked-by: Ward Vandewege

Thanks,
Ward.

[Myles Watson]

> I think it's good. Thanks for writing the patches!

No problem.  I've wanted the initialization order to be fixed for a long
time.  It's good to have it done.
> Acked-by: Ward Vandewege

Rev 5641.

Thanks,
Myles

[Ward Vandewege]

On Mon, Jun 21, 2010 at 05:59:52AM -0600, Myles Watson wrote:
> It looks like only 5632 has the "ADT7463 properly initialized"
> message.  One problem is that patch 2 was meant to be applied after
> patch 1,

Ah, whoops, I was wondering if I was doing the right thing there...
> so the device didn't end up in the tree for that run.  I'll
> have to think about why the only message from the new device with
> patch 1 is "I2C: 00:d0 missing read_resources"  For some reason it
> doesn't look like it got the correct ops.
>
> I wonder why the temperature values look right in all cases.  Does it
> need to be cold booted in order for the initialization to be needed?

All boots were cold.

Thanks,
Ward.

[Myles Watson]

On Mon, Jun 21, 2010 at 8:59 AM, Ward Vandewege  wrote:
> On Mon, Jun 21, 2010 at 05:59:52AM -0600, Myles Watson wrote:
> > It looks like only 5632 has the "ADT7463 properly initialized"
> > message.  One problem is that patch 2 was meant to be applied after
> > patch 1,
>
> Ah, whoops, I was wondering if I was doing the right thing there...

I should have been clearer.
> > so the device didn't end up in the tree for that run.  I'll
> > have to think about why the only message from the new device with
> > patch 1 is "I2C: 00:d0 missing read_resources"  For some reason it
> > doesn't look like it got the correct ops.
> >
> > I wonder why the temperature values look right in all cases.  Does it
> > need to be cold booted in order for the initialization to be needed?
>
> All boots were cold.

OK.  Thanks for doing that.

I guess I don't know what I'm looking at in the sensors output to see
if the device initialization worked.

Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> If we initialize and enable devices in the order that they are found
> in the tree, instead of the order that they were added to the list, it
> simplifies the code.  It also makes it so that removing a device from
> the devicetree.cb file won't change when its resources are enabled.

..
> Signed-off-by: Myles Watson

Acked-by: Peter Stuge
> This patch breaks the s2881, which was doing some odd acrobatics in
> order to get a device initialized after its parent.

If neccessary then I would be in favor of mainboard-specific hooks
from coreboot, that are separate from the device tree. But maybe the
init for s2881 can be done using a coreboot "driver" ?

//Peter

[Myles Watson]

On Thu, Jun 17, 2010 at 2:04 AM, Peter Stuge  wrote:
> Myles Watson wrote:
> > If we initialize and enable devices in the order that they are found
> > in the tree, instead of the order that they were added to the list, it
> > simplifies the code.  It also makes it so that removing a device from
> > the devicetree.cb file won't change when its resources are enabled.
>
> ..
> > Signed-off-by: Myles Watson
>
> Acked-by: Peter Stuge

Rev 5633.

I copied the s2881 code into a driver stub, since Ward said he's
willing to test.
> > This patch breaks the s2881, which was doing some odd acrobatics in
> > order to get a device initialized after its parent.
>
> If neccessary then I would be in favor of mainboard-specific hooks
> from coreboot, that are separate from the device tree. But maybe the
> init for s2881 can be done using a coreboot "driver" ?

That could work.  I think it might get abused, though.

Thanks,
Myles