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

[repository service]

Author: stuge
Date: Sat Jun  4 17:45:12 2011
New Revision: 6627
URL: https://tracker.coreboot.org/trac/coreboot/changeset/6627

Log:
Port persimmon r6584 and r6601 to e350m1: SPI prefetch early

Enable SPI cacheline prefetch 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:54 2011	(r6626)
+++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:45:12 2011	(r6627)
@@ -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) | 1 << 24);
+    }
+
   // early enable of SPI 33 MHz fast mode read
   if (boot_cpu())
     {

[Stefan Reinauer]

* repository service  [110604 17:45]:
> Author: stuge
> Date: Sat Jun  4 17:45:12 2011
> New Revision: 6627
> URL: https://tracker.coreboot.org/trac/coreboot/changeset/6627
>
> Log:
> Port persimmon r6584 and r6601 to e350m1: SPI prefetch early
>
> Enable SPI cacheline prefetch 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:54 2011	(r6626)
> +++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:45:12 2011	(r6627)
> @@ -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) | 1 << 24);
> }

This should use pci config space access functions. Also, enabling
Prefetch and 33MHz fast read mode should possibly go in the
southbridge's bootblock.c so the first cbfs scan does not run with the
slow settings.
> // early enable of SPI 33 MHz fast mode read
>    if (boot_cpu())
>      {

[Peter Stuge]

Stefan Reinauer wrote:
> > +++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:45:12 2011	(r6627)
> > @@ -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) | 1 << 24);
> > }
>
> This should use pci config space access functions. Also, enabling
> Prefetch and 33MHz fast read mode should possibly go in the
> southbridge's bootblock.c so the first cbfs scan does not run with
> the slow settings.

Good idea! Please do it! Remember to also revert the commit for
persimmon.

I think the Fusion code is a good place for such cleanups, where we
have a chance to factor out more common code before it gets too wide
spread. :)

//Peter

[Stefan Reinauer]

* Peter Stuge  [110604 18:19]:
> Stefan Reinauer wrote:
> > > +++ trunk/src/mainboard/asrock/e350m1/romstage.c	Sat Jun  4 17:45:12 2011	(r6627)
> > > @@ -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) | 1 << 24);
> > > }
> >
> > This should use pci config space access functions. Also, enabling
> > Prefetch and 33MHz fast read mode should possibly go in the
> > southbridge's bootblock.c so the first cbfs scan does not run with
> > the slow settings.
>
> Good idea! Please do it! Remember to also revert the commit for
> persimmon.

Unfortunately I have no hardware to test this.
> I think the Fusion code is a good place for such cleanups, where we
> have a chance to factor out more common code before it gets too wide
> spread. :)

Yes. Absolutely

[Scott Duplichan]

Stefan Reinauer wrote:

]Also, enabling Prefetch and 33MHz fast read mode should possibly go in the
]southbridge's bootblock.c so the first cbfs scan does not run with the
]slow settings.

Hello Stefan,

You are probably right. In fact these settings are applied fairly early
even without the patch:
tracker.coreboot.org/trac/coreboot/browser/trunk/src/vendorcode/amd/cimx/
sb800/SBPOR.c#L67 (prefetch)
tracker.coreboot.org/trac/coreboot/browser/trunk/src/vendorcode/amd/cimx/
sb800/SBPOR.c#L249 (spi speed)

The patch applies the settings even earlier for boot time reduction. The
gain is small:

---e350m1 DOS AHCI SSD boot time in ms---
early prefetch   off    on     off    on 
early 33 mhz     off    off    on     on 
                 ---    ---    ---    ---
                 686    682    736    679

Together the changes save 7 ms. The question is, where do we draw the line
on boot time reduction? I worked in a group a while back where a manager
said, "every millisecond counts". This was due to a desire to make a
customer's board boot more quickly than a board from a competitor. Certainly
no user can notice a boot time difference of a few ms. The difference is
easy to measure though, and in some cases a few ms is enough to affect who
calls their board fastest. On the other hand, coreboot+seabios is already
several thousand ms faster than UEFI, so maybe saving 7 ms is not worth the
somewhat out of place code.

Thanks,
Scott

[Kevin O'Connor]

On Sun, Jun 05, 2011 at 02:39:22AM -0500, Scott Duplichan wrote:
> Stefan Reinauer wrote:
> ]Also, enabling Prefetch and 33MHz fast read mode should possibly go in the
> ]southbridge's bootblock.c so the first cbfs scan does not run with the
> ]slow settings.

[...]
> Together the changes save 7 ms. The question is, where do we draw the line
> on boot time reduction? I worked in a group a while back where a manager
> said, "every millisecond counts". This was due to a desire to make a
> customer's board boot more quickly than a board from a competitor. Certainly
> no user can notice a boot time difference of a few ms. The difference is
> easy to measure though, and in some cases a few ms is enough to affect who
> calls their board fastest. On the other hand, coreboot+seabios is already
> several thousand ms faster than UEFI, so maybe saving 7 ms is not worth the
> somewhat out of place code.

I agree that no one would notice 7ms on it's own.  However, a handful
of 7ms improvements can be noticed.

I think of the bootblock as the place to fully map the flash device.
So, why would it be out of place to enable the faster flash accesses
in the bootblock?

-Kevin

[Scott Duplichan]

Kevin O'Connor wrote:

]I agree that no one would notice 7ms on it's own.  However, a handful
]of 7ms improvements can be noticed.
]
]I think of the bootblock as the place to fully map the flash device.
]So, why would it be out of place to enable the faster flash accesses
]in the bootblock?
]
]-Kevin

Hello Kevin,

The reason the patch code seems out of place is that the original code
enables the flash enhancements early (before memory initialization)
in sb800/sbpor.c. The patch moves this sb800 setting even earlier. That
is good, except that the sb800 setting is no longer grouped together
with other sb800 code. Instead, it is in romstage.c, which doesn't
usually embed chipset code. A better solution might be to move the flash
enhancement enabling code to an earlier position within sbpor.c. While
sbpor.c executes very early, it runs a fair amount of code before
enabling prefetch and SPI 33 MHz.

Thanks,
Scott

[Marc Jones]

On Tue, Jun 7, 2011 at 10:29 AM, Scott Duplichan  wrote:
> Kevin O'Connor wrote:
>
> ]I agree that no one would notice 7ms on it's own.  However, a handful
> ]of 7ms improvements can be noticed.
> ]
> ]I think of the bootblock as the place to fully map the flash device.
> ]So, why would it be out of place to enable the faster flash accesses
> ]in the bootblock?
> ]
> ]-Kevin
>
> Hello Kevin,
>
> The reason the patch code seems out of place is that the original code
> enables the flash enhancements early (before memory initialization)
> in sb800/sbpor.c. The patch moves this sb800 setting even earlier. That
> is good, except that the sb800 setting is no longer grouped together
> with other sb800 code. Instead, it is in romstage.c, which doesn't
> usually embed chipset code. A better solution might be to move the flash
> enhancement enabling code to an earlier position within sbpor.c. While
> sbpor.c executes very early, it runs a fair amount of code before
> enabling prefetch and SPI 33 MHz.
>
> Thanks,
> Scott

There are southbridge specific bootblock.c functions which enable
access etc., so moving the prefetch settings should be fine.

Can SPI be programmed to 33MHz always? Does it depend on the device or
platform? It may need to be a configuration option or a romstage.c
call.

Marc