Patchwork mainboard.c: init() still being called?

login
register
about
Submitter Myles Watson
Date 2010-08-16 15:11:41
Message ID <AANLkTi=G0_HjgVg7FTofRLqAyR7pL4t6X4Q7q7iyq0cz@mail.gmail.com>
Download mbox | patch
Permalink /patch/1746/
State Accepted
Commit r5698
Headers show

Comments

Myles Watson - 2010-08-16 15:11:41
On Mon, Aug 16, 2010 at 7:57 AM, Jens Rottmann
<JRottmann@lippertembedded.de> wrote:
> Hi all,
>
> in Nov 2008 (around rev. 3760) I had done Coreboot support for 2 of our boards
> (LiPPERT RoadRunner-LX and SpaceRunner-LX). Now I'd like to add support for 2
> more, based on the code for the previous ones as the hardware is quite similar.
>
> However in the past 2 years / 2000 revs Coreboot has evolved quite a lot! Even
> though SpaceRunner support still compiles, it doesn't quite seem to work as it
> did back then. Looks like I have to try and bring the 2 older board's code up to
> scratch first before I can base anything on it.

Sorry about the breakage.  Things have changed quite a bit.

> The old code defined a config byte (for some GPIOs) in struct
> mainboard_lippert_spacerunner_lx_config (chip.h), which used to be set in
> Config.lb with a simple register "..." = "..." statement and used in
> mainboard.c's init() by casting dev->chip_info to struct
> mainboard_lippert_spacerunner_lx_config.
>
> The current code still has the struct (renamed to mainboard_config), but while
> changing Config.lb to devicetree.cb the register statement got dropped so
> there's no longer any value assigned to the config byte. I tried to re-add the
> register line where it was before (before chip northbridge/...) but this seems
> no longer to be allowed. And more importantly, init() which uses the config byte
> is still there but does not seem to be executed any more. At least I couldn't
> see its debug output.
I think that happened when the devicetree.cb files were created
automatically.  There weren't many configuration values for the
mainboard.

> I tried to look at the AMD DB800, which had served as my example at the time,
> but the DB800 code hasn't changed, so probably its init() isn't called now
> either. (Only the DB800 doesn't do anything important there.)
The attached patch calls init() again.  It makes it match the comments again.

Signed-off-by: Myles Watson <mylesgw@gmail.com>

I think that the value that was left out of devicetree.cb should move
to the mainboard init() or into the device that's being configured
(cs5536.)  It makes sense to use values in the device tree to
parameterize device code for specific mainboards, but I don't see the
point of parameterizing mainboard-specific code from the device tree.

Thanks,
Myles
Peter Stuge - 2010-08-16 15:41:52
Myles Watson wrote:
> The attached patch calls init() again.  It makes it match the comments again.
> 
> Signed-off-by: Myles Watson <mylesgw@gmail.com>

Acked-by: Peter Stuge <peter@stuge.se>


> It makes sense to use values in the device tree to parameterize
> device code for specific mainboards, but I don't see the point of
> parameterizing mainboard-specific code from the device tree.

It's no secret that I'd like coreboot to have zero mainboard-specific
code, and allow parameters to be used to completely describe a board.


//Peter
Myles Watson - 2010-08-16 15:54:15
> Myles Watson wrote:
> > The attached patch calls init() again.  It makes it match the comments
> again.
> >
> > Signed-off-by: Myles Watson <mylesgw@gmail.com>
> 
> Acked-by: Peter Stuge <peter@stuge.se>
> 
> 
> > It makes sense to use values in the device tree to parameterize
> > device code for specific mainboards, but I don't see the point of
> > parameterizing mainboard-specific code from the device tree.
> 
> It's no secret that I'd like coreboot to have zero mainboard-specific
> code, and allow parameters to be used to completely describe a board.

In that case, are you sure you want to ack it?  Right now there are very few
boards that have mainboard init functions ( < 20).  I think now is the time
to migrate away from it if it's a bad idea.

I don't think it would take very long to help Jens change the code so that
the initialization that was done by the mainboard code gets done in the
cs5536 code where it belongs.

Thanks,
Myles
Myles Watson - 2010-08-16 16:07:11
>> > It makes sense to use values in the device tree to parameterize
>> > device code for specific mainboards, but I don't see the point of
>> > parameterizing mainboard-specific code from the device tree.
>>
>> It's no secret that I'd like coreboot to have zero mainboard-specific
>> code, and allow parameters to be used to completely describe a board.
>
> In that case, are you sure you want to ack it?  Right now there are very few
> boards that have mainboard init functions ( < 20).  I think now is the time
> to migrate away from it if it's a bad idea.
I just checked, and the number of boards is 31 counting all the ones
that use enable_dev for their mainboards.

Thanks,
Myles
Stefan Reinauer - 2010-08-16 16:11:58
On 8/16/10 5:11 PM, Myles Watson wrote:
> I think that the value that was left out of devicetree.cb should move
> to the mainboard init() or into the device that's being configured
> (cs5536.)  It makes sense to use values in the device tree to
> parameterize device code for specific mainboards, but I don't see the
> point of parameterizing mainboard-specific code from the device tree.
>
> Thanks,
> Myles
> Index: src/devices/device.c
> ===================================================================
> --- src/devices/device.c	(revision 5692)
> +++ src/devices/device.c	(working copy)
> @@ -1104,6 +1104,9 @@
>  
>  	printk(BIOS_INFO, "Initializing devices...\n");
>  
> +	/* First call the mainboard init. */
> +	init_dev(&dev_root);
> +
>  	/* now initialize everything. */
>  	for (link = dev_root.link_list; link; link = link->next)
>  		init_link(link);

Acked-by: Stefan Reinauer <stepan@coresystems.de>

Thanks for fixing coreboot. mainboard specific setup code is critical
for many boards.

Stefan
Stefan Reinauer - 2010-08-16 16:15:05
On 8/16/10 6:11 PM, Stefan Reinauer wrote:
> On 8/16/10 5:11 PM, Myles Watson wrote:
>> Index: src/devices/device.c
>> ===================================================================
>> --- src/devices/device.c	(revision 5692)
>> +++ src/devices/device.c	(working copy)
>> @@ -1104,6 +1104,9 @@
>>  
>>  	printk(BIOS_INFO, "Initializing devices...\n");
>>  
>> +	/* First call the mainboard init. */
>> +	init_dev(&dev_root);
>> +
>>  	/* now initialize everything. */
>>  	for (link = dev_root.link_list; link; link = link->next)
>>  		init_link(link);
>
> Acked-by: Stefan Reinauer <stepan@coresystems.de>
>
> Thanks for fixing coreboot. mainboard specific setup code is critical
> for many boards.
>
> Stefan
Do we need a similar workaround for the enable_dev() call?
Myles Watson - 2010-08-16 16:28:03
On Mon, Aug 16, 2010 at 10:15 AM, Stefan Reinauer
<stefan.reinauer@coresystems.de> wrote:
> On 8/16/10 6:11 PM, Stefan Reinauer wrote:
>
> On 8/16/10 5:11 PM, Myles Watson wrote:
>
> Index: src/devices/device.c
> ===================================================================
> --- src/devices/device.c	(revision 5692)
> +++ src/devices/device.c	(working copy)
> @@ -1104,6 +1104,9 @@
>
>  	printk(BIOS_INFO, "Initializing devices...\n");
>
> +	/* First call the mainboard init. */
> +	init_dev(&dev_root);
> +
>  	/* now initialize everything. */
>  	for (link = dev_root.link_list; link; link = link->next)
>  		init_link(link);
>
> Acked-by: Stefan Reinauer <stepan@coresystems.de>
Rev 5698.

> Do we need a similar workaround for the enable_dev() call?
It's already there.  I think init() just got missed because so few
boards use it.

Thanks,
Myles
Peter Stuge - 2010-08-16 16:45:11
Stefan Reinauer wrote:
> Thanks for fixing coreboot.

This was also why I acked. I think even if it's not what I would
like, it's how things work now, so better fix it. :)


> mainboard specific setup code is critical for many boards.

What does it do? Is 986lcd-m a good place to find complicated
mb-specific setup code?


//Peter
Stefan Reinauer - 2010-08-16 17:01:23
On 8/16/10 6:45 PM, Peter Stuge wrote:
> Stefan Reinauer wrote:
>> Thanks for fixing coreboot.
> This was also why I acked. I think even if it's not what I would
> like, it's how things work now, so better fix it. :)
>
>
>> mainboard specific setup code is critical for many boards.
> What does it do? Is 986lcd-m a good place to find complicated
> mb-specific setup code?
>
On some boards it sets up the fans. On others it makes sure the display
brightness is initialized correctly.
Sometimes it installs board specific interrupt handlers for oprom init.

Stefan
Peter Stuge - 2010-08-16 17:11:07
Stefan Reinauer wrote:
> >> mainboard specific setup code is critical for many boards.
> >
> > What does it do?
> 
> On some boards it sets up the fans.
> On others it makes sure the display brightness is initialized
> correctly.

What determines the correct way to do those tasks? Is it really
mainboard specific; ie. circuit design? Even if there is a unique
supporting circuit on the boards, the chips that we talk to from
software will only have a finite ways they can work, so we should
always be able to use parameters..


> Sometimes it installs board specific interrupt handlers for oprom
> init.

Thanks, I "forgot" about that one. :) What makes those interrupt
handlers board specific though? What they return might also be
easy to control by simple parameters.


//Peter
Stefan Reinauer - 2010-08-16 18:14:12
On 8/16/10 7:11 PM, Peter Stuge wrote:
> Stefan Reinauer wrote:
>>>> mainboard specific setup code is critical for many boards.
>>> What does it do?
>> On some boards it sets up the fans.
>> On others it makes sure the display brightness is initialized
>> correctly.
> What determines the correct way to do those tasks? Is it really
> mainboard specific; ie. circuit design?
Yes. Hence they're done in mainboard.c

Some of them _might_ live in mainboard.c because creating a new
framework like a "display brightness framework" into coreboot and
writing a driver for whatever actual hardware controls brightness just
for getting one value written into one register didn't seem appropriate
at the time. So in the end it makes life easier for porters and readers
than having to cope with yet another framework.

> Even if there is a unique
> supporting circuit on the boards, the chips that we talk to from
> software will only have a finite ways they can work, so we should
> always be able to use parameters..
Yes. But you have to weigh things up. While there might be a "formula
for everything" it might not be the most effective way to solve one
given problem, even if the model sounds nicer in theory.

>> Sometimes it installs board specific interrupt handlers for oprom
>> init.
> Thanks, I "forgot" about that one. :) What makes those interrupt
> handlers board specific though? What they return might also be
> easy to control by simple parameters.
That might be very different things, like how wide the graphics bus is
or how fast it can be clocked based on how the mainboard designed layed
out the wires. Or whether the board has a TV-Out that should be selected.

I agree a lot of this _could_ go into the config / device tree, but
right now there are bigger drawbacks one could fight if going for a
sconfig overhaul.
Such as putting interrupt routing information in there.
Maybe even dropping the idea of "register" in sconfig completely and
coming up with something better.

Stefan

Patch

Index: src/devices/device.c
===================================================================
--- src/devices/device.c	(revision 5692)
+++ src/devices/device.c	(working copy)
@@ -1104,6 +1104,9 @@ 
 
 	printk(BIOS_INFO, "Initializing devices...\n");
 
+	/* First call the mainboard init. */
+	init_dev(&dev_root);
+
 	/* now initialize everything. */
 	for (link = dev_root.link_list; link; link = link->next)
 		init_link(link);