| 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 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 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
>> > 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
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
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?
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
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
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
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
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);