[commit] r6584 - trunk/src/mainboard/amd/persimmon

[repository service]

Author: mjones
Date: Sun May 15 23:54:04 2011
New Revision: 6584
URL: https://tracker.coreboot.org/trac/coreboot/changeset/6584

Log:
Enable SPI cacheline prefetch early to reduce boot time.

Signed-off-by: Scott Duplichan 
Acked-by: Marc Jones

Modified:
   trunk/src/mainboard/amd/persimmon/romstage.c

Modified: trunk/src/mainboard/amd/persimmon/romstage.c
==============================================================================
--- trunk/src/mainboard/amd/persimmon/romstage.c	Sun May 15 23:51:31 2011	(r6583)
+++ trunk/src/mainboard/amd/persimmon/romstage.c	Sun May 15 23:54:04 2011	(r6584)
@@ -50,6 +50,13 @@
   // all cores: set pstate 0 (1600 MHz) early to save a few ms of boot time
   __writemsr (0xc0010062, 0);

+  // early enable of PrefetchEnSPIFromHost
+  if (boot_cpu())
+    {
+    __outdword (0xcf8, 0x8000a3b8);
+    __outdword (0xcfc, __indword (0xcfc) | 0 << 24);
+    }
+
   // early enable of SPI 33 MHz fast mode read
   if (boot_cpu())
     {

[Peter Stuge]

repository service wrote:
> +++ trunk/src/mainboard/amd/persimmon/romstage.c	Sun May 15 23:54:04 2011	(r6584)

..
> // early enable of PrefetchEnSPIFromHost
> if (boot_cpu())
> {
> __outdword (0xcf8, 0x8000a3b8);
> __outdword (0xcfc, __indword (0xcfc) | 0 << 24);
> }

PCI function? And maybe this, as well as the 33MHz setup, is good to
have in the chipset code, as opposed to duplicated per mainboard?

//Peter

[Stefan Reinauer]

On 5/15/11 3:57 PM, Peter Stuge wrote:
> repository service wrote:
> > +++ trunk/src/mainboard/amd/persimmon/romstage.c	Sun May 15 23:54:04 2011	(r6584)
>
> ..
> > // early enable of PrefetchEnSPIFromHost
> > if (boot_cpu())
> > {
> > __outdword (0xcf8, 0x8000a3b8);
> > __outdword (0xcfc, __indword (0xcfc) | 0<<  24);
> > }
>
> PCI function? And maybe this, as well as the 33MHz setup, is good to
> have in the chipset code, as opposed to duplicated per mainboard?

and in coreboot there already is a function to do a dword IO access: 
outl(). At least while not under vendorcode/ that should be used (if 
it's not a PCI access in which case pci_config... should be used)

It took me a long time to get rid of 6 instances of printk. I don't 
really want to have to do the same again for outb/outw/outl ;-)

Stefan

[Scott Duplichan]

Peter Stuge wrote:

]> +    __outdword (0xcf8, 0x8000a3b8);
]> +    __outdword (0xcfc, __indword (0xcfc) | 0 << 24);
]> +    }
]
]PCI function? And maybe this, as well as the 33MHz setup, is good to
]have in the chipset code, as opposed to duplicated per mainboard?

Hello Peter,

I will try improve this and some of the others, hopefully this week.
Yes, the code above looks odd, so I should at least explain why. This
and the other newly added early settings indeed duplicate settings made
later. The justification is/was 'early as possible' for boot time
reduction. With another change or two, both persimmon and asrock e350m1
boot to DOS time drop to 600 ms.

Maybe a bigger question is why the unusual pci config write coding?
Certainly a function call is just as good (and I will submit a change).
The code is a quick and dirty way to paste in a pci config write and
have it work for amd agesa as well as for all phases of UEFI. I used to
get frustrated over how complicated UEFI makes something as simple as a
write to base pci config space. A single function call? More like a
couple dozen lines of code: Call the OpenProtocol function pointer from
a global structure, passing half a dozen items including the GUID for PCI
protocol. Check for errors then call the write() member of the structure
it returns. Of course integer constants are often passed by address, so
separate allocation is needed for those. Check for and handle any error
returned by the write function, then call another function to close the
protocol. The code sequence above was my silent protest over this ordeal,
and it ended up in coreboot. I/O functions named like __outdword are 
supported by the Microsoft compilers used for UEFI, and can even be used
with no prototype.

Thanks,
Scott

[Mark Marshall]

On 15/05/2011 22:54, repository service wrote:
> Author: mjones
> Date: Sun May 15 23:54:04 2011
> New Revision: 6584
> URL: https://tracker.coreboot.org/trac/coreboot/changeset/6584
>
> Log:
> Enable SPI cacheline prefetch early to reduce boot time.
>
> Signed-off-by: Scott Duplichan
> Acked-by: Marc Jones
>
> Modified:
>     trunk/src/mainboard/amd/persimmon/romstage.c
> Modified: trunk/src/mainboard/amd/persimmon/romstage.c
>
> --- trunk/src/mainboard/amd/persimmon/romstage.c	Sun May 15 23:51:31 2011	(r6583)
> +++ trunk/src/mainboard/amd/persimmon/romstage.c	Sun May 15 23:54:04 2011	(r6584)
> @@ -50,6 +50,13 @@
>     // all cores: set pstate 0 (1600 MHz) early to save a few ms of boot time
>     __writemsr (0xc0010062, 0);
> // early enable of PrefetchEnSPIFromHost
> if (boot_cpu())
> {
> __outdword (0xcf8, 0x8000a3b8);
> __outdword (0xcfc, __indword (0xcfc) | 0<<  24);
> }
> // early enable of SPI 33 MHz fast mode read
>   if (boot_cpu())
>     {

Isn't this a no-op (or'ing 0 into the read value and writing it back)?

MM

[Scott Duplichan]

Mark Marshall wrote:

]> +    __outdword (0xcf8, 0x8000a3b8);
]> +    __outdword (0xcfc, __indword (0xcfc) | 0<<  24);
]
]Isn't this a no-op (or'ing 0 into the read value and writing it back)?
]
]MM

Hello Mark,

Thanks for point this out. You are certainly correct. This
was left over from a test of its effectiveness and not restored.
I repeated the test with the corrected code and it cuts boot
time by 4 ms. Revision 6601 corrects it.

Still needed is a change to replace these in line code sequences
with calls to existing PCI config functions.

Thanks,
Scott