[PATCH] Major cleanups of hard_reset() related code/config

[Uwe Hermann]

See patch.

I'm happy for any comments. Also, I'm not sure how all this
hard_reset() stuff is meant to work. Many southbridges have _two_
implementations of hard_reset(). Why? Some only have one.
Some have none, but I guess that's OK, as their RAM init or other
coreboot code doesn't need to reset the machine (?)

And then there are various reset.c files in the mainboard directories
which don't look board-specific at all. I propose to drop them all and
provide one global hard_reset() function somewhere. Feasible?

This is the list of reset.c files we have:

$ find . -name reset.c
./src/mainboard/tyan/s2735/reset.c
./src/mainboard/kontron/986lcd-m/reset.c
./src/mainboard/intel/eagleheights/reset.c
./src/mainboard/intel/xe7501devkit/reset.c
./src/mainboard/intel/jarrell/reset.c
./src/mainboard/dell/s1850/reset.c
./src/mainboard/supermicro/x6dhr_ig2/reset.c
./src/mainboard/supermicro/x6dhr_ig/reset.c
./src/mainboard/supermicro/x6dai_g/reset.c
./src/mainboard/supermicro/x6dhe_g/reset.c
./src/mainboard/supermicro/x6dhe_g2/reset.c

If you look at them you'll notice they're all pretty much the same,
at least the hard_reset() function in the files.

Can someone explain what the rationale is for having the board reset.c
files (and some hard_reset() implementations in auto.c instead of in
reset.c files, which is also not so nice)?

Not yet abuild-tested.

Uwe.

[PATCH] Major cleanups of hard_reset() related code/config

[Peter Stuge]

Uwe Hermann wrote:
> I'm happy for any comments.

I'm really positive, but unfortunately I can't answer the questions.
:\ I've thought the same things.

//Peter

[PATCH] Major cleanups of hard_reset() related code/config

[Joseph Smith]

> Can someone explain what the rationale is for having the board reset.c
> files (and some hard_reset() implementations in auto.c instead of in
> reset.c files, which is also not so nice)?

I know for my two boards I need the hard_reset() in auto.c otherwise
coreboot hangs at raminit after a Linux reboot because the memory is still
initialized and needs to be reset.

Hence:

if (memory_initialized()) { 
    hard_reset(); 
}

[PATCH] Major cleanups of hard_reset() related code/config

[Joseph Smith]

On Tue, 20 Oct 2009 06:53:49 -0400, Joseph Smith 
wrote:
> > Can someone explain what the rationale is for having the board reset.c
> > files (and some hard_reset() implementations in auto.c instead of in
> > reset.c files, which is also not so nice)?
>
> I know for my two boards I need the hard_reset() in auto.c otherwise
> coreboot hangs at raminit after a Linux reboot because the memory is

still
> initialized and needs to be reset.
>
> Hence:
>
> if (memory_initialized()) {
>     hard_reset();
> }

I suppose boards APCI and power state implimentations (SO -> S5) probably
don't need hard_reset() in auto.c, but the rest will need this.

[Myles Watson]

On Mon, Oct 19, 2009 at 6:15 PM, Uwe Hermann  wrote:
> See patch.
>
> I'm happy for any comments. Also, I'm not sure how all this
> hard_reset() stuff is meant to work. Many southbridges have _two_
> implementations of hard_reset(). Why? Some only have one.
> Index: src/southbridge/amd/amd8111/amd8111_reset.c
>
> --- src/southbridge/amd/amd8111/amd8111_reset.c    (Revision 4813)
> +++ src/southbridge/amd/amd8111/amd8111_reset.c    (Arbeitskopie)
> @@ -54,6 +54,7 @@
>  #include "../../../northbridge/amd/amdk8/reset_test.c"
>
> +/* FIXME: There's another implementation in amd8111_early_ctrl.c. Why? */
>  void hard_reset(void)
>  {
>      device_t dev;
> Index: src/southbridge/amd/amd8111/amd8111_early_ctrl.c
> ===================================================================
> --- src/southbridge/amd/amd8111/amd8111_early_ctrl.c    (Revision 4813)
> +++ src/southbridge/amd/amd8111/amd8111_early_ctrl.c    (Arbeitskopie)
> @@ -35,6 +35,7 @@
>      enable_cf9_x(sbbusn, sbdn);
>  }
>
> +/* FIXME: There's another implementation in amd8111_reset.c. Why? */
>  static void hard_reset(void)
>  {
>          set_bios_reset();

CAR code has its own implementation of many functions: printk, die, HT
initialization, ...

The early files are inlcuded into the cache_as_ram_auto.c, and the other
ones are used with coreboot_ram.
> Drop hard_reset() dummy from asus/a8v-e_se and asus/m2v-mx_se.
> As mentioned by ruik on IRC, the soft_reset() function already does a
> hard reset, so this function is not needed.

I think it's a bug to have soft_reset() do a hard reset.

Thanks,
Myles

[PATCH] Major cleanups of hard_reset() related code/config

[Stefan Reinauer]

Uwe Hermann wrote:
> See patch.
>
> I'm happy for any comments. Also, I'm not sure how all this
> hard_reset() stuff is meant to work. Many southbridges have _two_
> implementations of hard_reset(). Why? Some only have one.
> Some have none, but I guess that's OK, as their RAM init or other
> coreboot code doesn't need to reset the machine (?)
>
> And then there are various reset.c files in the mainboard directories
> which don't look board-specific at all. I propose to drop them all and
> provide one global hard_reset() function somewhere. Feasible?

It might silently break a lot of stuff. There's about 6 ways to reset a
machine (probably more) and it's not always trivial to see why a certain
reset method is needed at a certain time.

To your patch, I don't understand why you're suggesting to rename
hard_reset to board_name_hard_reset(). That seems like an awful idea...
Any reason for it?

Stefan

[PATCH] Major cleanups of hard_reset() related code/config, try 2

[Uwe Hermann]

On Thu, Oct 22, 2009 at 10:20:32AM +0200, Stefan Reinauer wrote:
> > And then there are various reset.c files in the mainboard directories
> > which don't look board-specific at all. I propose to drop them all and
> > provide one global hard_reset() function somewhere. Feasible?
>
> It might silently break a lot of stuff. There's about 6 ways to reset a
> machine (probably more) and it's not always trivial to see why a certain
> reset method is needed at a certain time.

OK, we should add comments for this non-obvious stuff then (where we
know the reason at least). I won't touch any of them for now without
testing, though.
> To your patch, I don't understand why you're suggesting to rename
> hard_reset to board_name_hard_reset(). That seems like an awful idea...
> Any reason for it?

It was probably a stupid idea, yes. I removed those comments. The reason
was that we have one i82801ca_hard_reset() function already in

src/southbridge/intel/i82801ca/i82801ca_reset.c

where

src/mainboard/intel/xe7501devkit/reset.c

has a hard_reset() function that is a simple wrapper around
i82801ca_hard_reset(). That seems to be the only such function, all
other southbridges name the function hard_reset() already. Maybe
we should rename i82801ca_hard_reset() to hard_reset() and then drop
src/southbridge/intel/i82801ca/i82801ca_reset.c?

Either way, this is material for another patch, maybe I'll post
something later.
For now I kept the updated hard_reset() patch as simple as possible,
in order to get a first round of cleanups in.

I also left ruik's hard_reset() functions alone for now as Myles
suggested, maybe we should rename soft_reset() to hard_reset() in
that code, not sure.

Updated patch attached.

Uwe.

[PATCH] Major cleanups of hard_reset() related code/config, try 2

[Myles Watson]

Acked-by: Myles Watson

Thanks,
Myles

[PATCH] Major cleanups of hard_reset() related code/config, try 2

[Uwe Hermann]

On Sat, Oct 24, 2009 at 09:34:41AM -0600, Myles Watson wrote:
> Acked-by: Myles Watson

Thanks, r4849.

Uwe.