[commit] r6626 - trunk/src/mainboard/asrock/e350m1

[repository service]

Author: stuge
Date: Sat Jun  4 17:44:54 2011
New Revision: 6626
URL: https://tracker.coreboot.org/trac/coreboot/changeset/6626

Log:
Port persimmon r6583 to e350m1: pstate 0 early

Switch processor cores to pstate 0 early to reduce boot time.

Signed-off-by: Marshall Buschman 
Acked-by: Peter Stuge

Modified:
   trunk/src/mainboard/asrock/e350m1/romstage.c

Modified: trunk/src/mainboard/asrock/e350m1/romstage.c
==============================================================================
--- trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:44:31 2011	(r6625)
+++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:44:54 2011	(r6626)
@@ -47,6 +47,9 @@
   u32 val;
   u8 reg8;

+  // all cores: set pstate 0 (1600 MHz) early to save a few ms of boot time
+  __writemsr (0xc0010062, 0);
+
   // early enable of SPI 33 MHz fast mode read
   if (boot_cpu())
     {

[Stefan Reinauer]

* repository service  [110604 17:44]:
> Author: stuge
> Date: Sat Jun  4 17:44:54 2011
> New Revision: 6626
> URL: https://tracker.coreboot.org/trac/coreboot/changeset/6626
>
> Log:
> Port persimmon r6583 to e350m1: pstate 0 early
>
> Switch processor cores to pstate 0 early to reduce boot time.
>
> Signed-off-by: Marshall Buschman 
> Acked-by: Peter Stuge
>
> Modified:
>    trunk/src/mainboard/asrock/e350m1/romstage.c
> Modified: trunk/src/mainboard/asrock/e350m1/romstage.c
>
> --- trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:44:31 2011	(r6625)
> +++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:44:54 2011	(r6626)
> @@ -47,6 +47,9 @@
>    u32 val;
>    u8 reg8;
> // all cores: set pstate 0 (1600 MHz) early to save a few ms of boot time
> __writemsr (0xc0010062, 0);

why not use writemsr instead of __writemsr?
> // early enable of SPI 33 MHz fast mode read
>    if (boot_cpu())
>      {

[commit] r6625 - trunk/src/mainboard/asrock/e350m1

[Peter Stuge]

Stefan Reinauer wrote:
> > +++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:44:31 2011	(r6625)

..
> > __outdword (0xcf8, 0x8000a3a0);
>
> what's the reason to not use pci_read_config32() here?
> > +++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:44:54 2011	(r6626)

..
> > __writemsr (0xc0010062, 0);
>
> why not use writemsr instead of __writemsr?

This is how it looked in the previous patches committed to svn, which
were blindly applied to asrock/e350m1 after having tested that they
worked. Ie. blame Scott. :p

//Peter

[commit] r6625 - trunk/src/mainboard/asrock/e350m1

[Stefan Reinauer]

* Peter Stuge  [110604 18:16]:
> Stefan Reinauer wrote:
> > > +++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:44:31 2011	(r6625)
>
> ..
> > > __outdword (0xcf8, 0x8000a3a0);
> >
> > what's the reason to not use pci_read_config32() here?
> > > +++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:44:54 2011	(r6626)
>
> ..
> > > __writemsr (0xc0010062, 0);
> >
> > why not use writemsr instead of __writemsr?
>
> This is how it looked in the previous patches committed to svn, which
> were blindly applied to asrock/e350m1 after having tested that they
> worked. Ie. blame Scott. :p

I'm not blaming anyone, I just think this should  get fixed, or at least not
repeated ;)

Blindly copy'n'pasting code is not always the answer.

[commit] r6625 - trunk/src/mainboard/asrock/e350m1

[Peter Stuge]

Stefan Reinauer wrote:
> > previous patches committed to svn, which were blindly applied to
> > asrock/e350m1
>
> Blindly copy'n'pasting code is not always the answer.

Of course not. The situation here is that we worked toward zero
differences between a nicely formatted patch set, and the single big
that Scott was kind to provide during his work on this code. To keep
complexity of this work manageable it was important to arrive on
nearly exactly the same code in the end. I think now is a great time
to make cleanups on code common for both boards.

What we have arrived at now is coreboot trunk working brilliantly for
a very recently announced consumer mainboard with the latest AMD
technology. It was considerably more difficult to test with
amd/persimmon because of more limited availability.

//Peter

[Scott Duplichan]

Stefan Reinauer wrote:

]> +  // all cores: set pstate 0 (1600 MHz) early to save a few ms of boot
]time
]> + __writemsr (0xc0010062, 0);
]> +
]
]why not use writemsr instead of __writemsr?

Hello Stefan,

The wrmsr function is certainly OK. I am in the habit us using the
Intel/MS functions because they are built into the Microsoft compiler
and therefore portable between all UEFI vendor code bases and any
other code that uses the MS complier. But portability with UEFI is
not the reason I still sometimes use these function types. Truthfully,
the coreboot wrmsr function seems awkward to me. With the Intel/MS
__writemsr function, any 64-bit value can be written with a single
statement. Coreboot code using wrmsr is often taking 4 statements to
do the same thing. While there is probably little difference in
generated code, using the coreboot wrmsr function does require more
source code typing. I also find it less readable. For example,

// set bit 46 of NB_CFG_MSR using wrmsr function:
msr_t msr;
msr = rdmsr(NB_CFG_MSR);
msr.hi |= (1 << (46 - 32));
wrmsr(NB_CFG_MSR, msr);

// Making msr_t a union that adds a uint64_t would allow:
msr_t msr;
msr = rdmsr(NB_CFG_MSR);
msr.all |= 1 << 46;
wrmsr(NB_CFG_MSR, msr);

// The Intel/MS style uint64_t prototypes allow:
__writemsr (__readmsr (NB_CFG_MSR) | 1 << 46);

A question I have wanted to ask for a while is why coreboot
doesn't use 64 bit integers in situations like this?

Thanks,
Scott