[coreboot] patch: dbe62

[Carl-Daniel Hailfinger]

On 29.02.2008 17:52, ron minnich wrote:
> On Fri, Feb 29, 2008 at 5:41 AM, Peter Stuge <peter at stuge.se> wrote:
>
>   
>>  > >  > +void mainboard_pre_payload(void)
>>  > >  > +{
>>  > >  > +     geode_pre_payload();
>>  > >  > +     banner(BIOS_DEBUG, "mainboard_pre_payload: done");
>>  > >  > +}
>>  > >
>>  > >  Why do we need this mainboard code when it is only calling a
>>  > >  function that can be determined using the dts?
>>  >
>>  > Show me how. I don't know.
>>
>>  arch/x86/stage1.c currently calls mainboard_pre_payload() - for
>>  Geode LX the call has nothing to do with the mainboard and
>>  everything to do with the north.
>>
>>  geode_pre_payload() is defined in geodelx/geodelxinit.c, so one easy
>>  way is to rename it to northbridge_amd_geodelx_pre_payload() and
>>  generate the call from data in the dts.
>>
>>  Another way would be to add it to struct device_operations. I like
>>  that better actually.
>>     
>
>
> Yep, but here you've hit a limitation in the design which we never
> thought of. Device stuff is in stage 2. But this mainboard_early stuff
> is in main (btw, we're going to need to rename arch/x86/stage1.c again
> -- since it calls stage2 and stage2 returns to it, it's not really a
> stage1 at all. I believe the name change was my mistake).
>   

Perhaps call it "launcher" stage? On the other hand, it is part of the 
bootblock (so either stage0 or stage1) and it is after CAR has been 
switched on (so definitely stage1). I vote to keep the name until we 
figure out something better.

> The device tree is in stage 2. It goes away at the end of stage2 since
> we didn't see a need for it. The data that it creates is gone at that
> point.
>
> OK, it gets worse. You REALLY don't want to do this call until just
> about the last second, after payload is up and ready to go, since it
> disables ROM caching. That's a long time after stage 2. You can't do
> this call in stage2.
>
> There are several options here but most of them are not that great.
> 1. add phase7 to stage 2 in device tree and return dynamic device tree
> from stage 2 to caller.
> stage2 phase 7 is now called AFTER payload is loaded and BEFORE
> payload is jumped to. This breaks our nice stage design but that's
> life. Big problem -- where does device tree go in memory and what do
> we do if it interferes with payload location?
> 2. what we have now -- it works.
>   

What we have now is OK, but there is room for improvement.

> 3. name a pre-payload in the dts, have dtc build the
> mainboard_pre_payload function for us, call stays as it is now.
> 4. Linker sets -- Oh, yuck, a lot of work went into v3 to AVOID linker sets.
>   

5. Maybe the name of mainboard_pre_payload() is unfortunate and we 
should have pre_payload() which calls cpu_pre_payload(), 
chipset_pre_payload() and mainboard_pre_payload() in a row. That way, we 
don't have to put any GeodeLX ROM cache disabling below mainboard() and 
keep that stuff local to the southbridge code.


>>  > >  > +unsigned long write_pirq_routing_table(unsigned long addr)
>>  > >
>>  > >  What an abomination. hint hint ;)
>>  > >
>>  > >  Can this code really not live in northbridge/ ?
>>  >
>>  > no, it's a south function, with mainboard-dependent bits. I'm happy
>>  > to see a way to make it generic, I just don't know how yet.
>>
>>  Is the _code_ really mainboard dependent? I would think it's device
>>  dependent, and the board only changes input values. Am I off?
>>     
>
> Sure, but it's almost always a southbridge thing. But you need to
> rewrite the code quite a bit to move it to the south directory, and
> I'm not sure it's worth it in the end -- having it in mainboard allows
> you to deal with mainboard bugs and problems very easily.
>   

Peter's idea was probably to separate code from data and I think there 
is some potential in that idea. write_pirq_routing_table() is identical 
for alix1c and dbe62 and could in theory really live in the south 
directory. AFAICS this wouldn't even require any changes in the function.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/