Hi Simon,

On Tue, Aug 1, 2023 at 12:13 AM Simon Glass sjg@chromium.org wrote:

Hi Bin,

On Mon, 31 Jul 2023 at 08:58, Bin Meng bmeng.cn@gmail.com wrote:

...

Hi Simon,

On Mon, Jul 31, 2023 at 9:56 PM Simon Glass sjg@chromium.org wrote:

...

From: Bin Meng bmeng.cn@gmail.com

On Coral U-Boot SPL programs some MTRRs and FSPv2 in U-Boot proper needs to program MTRRs too. With current testing logic of mtrr commit in init_cache_f_r(), the mtrr commit is skipped which won't work as the queued mtrr requests include setup for DRAM regions.

Change the logic to allow such configuration.

Signed-off-by: Bin Meng bmeng.cn@gmail.com Tweak to put back CONFIG_FSP_VERSION2 at top: Signed-off-by: Simon Glass sjg@chromium.org

---

Changes in v3:

• Allow committing MTRRs with FSP_VERSION2

Changes in v2:

• new patch: "x86: Change tesing logic of mtrr commit"

arch/x86/lib/init_helpers.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c index f33194045f9e..60a2707dcf1b 100644 --- a/arch/x86/lib/init_helpers.c +++ b/arch/x86/lib/init_helpers.c @@ -21,8 +21,7 @@ int init_cache_f_r(void) /* * Supported configurations: *

•    * booting from slimbootloader - in that case the MTRRs are already set
  

•    *      up
  

•    * booting from slimbootloader - MTRRs are already set up
     * booting with FSPv1 - MTRRs are already set up
     * booting with FSPv2 - MTRRs must be set here
     * booting from coreboot - in this case there is no SPL, so we set up
  

@@ -30,8 +29,7 @@ int init_cache_f_r(void) * Note: if there is an SPL, then it has already set up MTRRs so we * don't need to do that here */

•   do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&
  

•           !IS_ENABLED(CONFIG_FSP_VERSION1) &&
  

•   do_mtrr &= !IS_ENABLED(CONFIG_FSP_VERSION1) &&
  

The drop of IS_ENABLED(CONFIG_SPL) here will cause:

1. SPL programs the MTRR
2. U-Boot proper programs the MTRR again, which is not we want

I believe it is what we want since we want to drop the SPI things and add the SDRAM ones which U-Boot proper has added.

...

My patch covers such scenario, no?

...

            !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);

    if (do_mtrr) {

No, with your patch do_mtrr is set to false at the top, so the &= does nothing. I promise you, if it worked I would say so, but it doesn't :-)

Great!

Regards, Bin