[PATCH 0/7] x86: Various fixes to MTRR and FSP codes

[PATCH 0/7] x86: Various fixes to MTRR and FSP codes

[Bin Meng]

At present Intel Crown Bay does not boot. This was caused by various
regression issues introduced when supporting FSP2, and some flaws in
MTRR codes.

With this series, U-Boot boot again on Intel Crown Bay board.

This series is available at u-boot-x86/crownbay for testing.


Bin Meng (7):
  x86: fsp: Don't program MTRR for DRAM
  x86: mtrr: Do not clear the unused ones in mtrr_commit()
  x86: mtrr: Skip MSRs that were already programmed in mtrr_commit()
  x86: mtrr: Abort if requested size is not power of 2
  x86: cmd: hob: Fix the command usage and help messages
  x86: cmd: hob: Fix display of resource type for system memory
  x86: fsp: Only FSP2 has INIT_PHASE_END_FIRMWARE

 arch/x86/cpu/mtrr.c           | 13 ++++++++-----
 arch/x86/include/asm/mtrr.h   |  7 ++++---
 arch/x86/lib/fsp/fsp_common.c | 16 +++++++++-------
 arch/x86/lib/fsp/fsp_dram.c   | 35 ++++++++++-------------------------
 cmd/x86/hob.c                 |  9 ++++-----
 5 files changed, 35 insertions(+), 45 deletions(-)

-- 
2.25.1

[PATCH 1/7] x86: fsp: Don't program MTRR for DRAM

[Bin Meng]

This actually reverts the following 2 commits:

commit 427911001809 ("x86: Set up the MTRR for SDRAM")
commit d46c0932a9d4 ("x86: fsp: Adjust calculations for MTRR range and DRAM top")

There are several outstanding issues as to why:

* For FSP1, the system memory and reserved memory used by FSP are
  already programmed in the MTRR by FSP.
* The 'mtrr_top' mistakenly includes TSEG memory range that has the
  same RES_MEM_RESERVED resource type. Its address is programmed
  and reported by FSP to be near the top of 4 GiB space, which is
  not what we want for SDRAM.
* The call to mtrr_add_request() is not guaranteed to have its size
  to be exactly the power of 2. This causes reserved bits of the
  IA32_MTRR_PHYSMASK register to be written which generates #GP.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 arch/x86/lib/fsp/fsp_dram.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/arch/x86/lib/fsp/fsp_dram.c b/arch/x86/lib/fsp/fsp_dram.c
index 8ad9aeedac..928c5225cd 100644
--- a/arch/x86/lib/fsp/fsp_dram.c
+++ b/arch/x86/lib/fsp/fsp_dram.c
@@ -11,7 +11,6 @@
 #include <asm/e820.h>
 #include <asm/global_data.h>
 #include <asm/mrccache.h>
-#include <asm/mtrr.h>
 #include <asm/post.h>
 #include <dm/ofnode.h>
 
@@ -42,10 +41,8 @@ int fsp_scan_for_ram_size(void)
 
 int dram_init_banksize(void)
 {
-	efi_guid_t fsp = FSP_HOB_RESOURCE_OWNER_FSP_GUID;
 	const struct hob_header *hdr;
 	struct hob_res_desc *res_desc;
-	phys_addr_t mtrr_top;
 	phys_addr_t low_end;
 	uint bank;
 
@@ -53,47 +50,35 @@ int dram_init_banksize(void)
 		gd->bd->bi_dram[0].start = 0;
 		gd->bd->bi_dram[0].size = gd->ram_size;
 
-		mtrr_add_request(MTRR_TYPE_WRBACK, 0, gd->ram_size);
 		return 0;
 	}
 
-	low_end = 0;	/* top of low memory usable by U-Boot */
-	mtrr_top = 0;	/* top of low memory (even if reserved) */
+	low_end = 0;
 	for (bank = 1, hdr = gd->arch.hob_list;
 	     bank < CONFIG_NR_DRAM_BANKS && !end_of_hob(hdr);
 	     hdr = get_next_hob(hdr)) {
 		if (hdr->type != HOB_TYPE_RES_DESC)
 			continue;
 		res_desc = (struct hob_res_desc *)hdr;
-		if (!guidcmp(&res_desc->owner, &fsp))
-			low_end = res_desc->phys_start;
 		if (res_desc->type != RES_SYS_MEM &&
 		    res_desc->type != RES_MEM_RESERVED)
 			continue;
 		if (res_desc->phys_start < (1ULL << 32)) {
-			mtrr_top = max(mtrr_top,
-				       res_desc->phys_start + res_desc->len);
-		} else {
-			gd->bd->bi_dram[bank].start = res_desc->phys_start;
-			gd->bd->bi_dram[bank].size = res_desc->len;
-			mtrr_add_request(MTRR_TYPE_WRBACK, res_desc->phys_start,
-					 res_desc->len);
-			log_debug("ram %llx %llx\n",
-				  gd->bd->bi_dram[bank].start,
-				  gd->bd->bi_dram[bank].size);
+			low_end = max(low_end,
+				      res_desc->phys_start + res_desc->len);
+			continue;
 		}
+
+		gd->bd->bi_dram[bank].start = res_desc->phys_start;
+		gd->bd->bi_dram[bank].size = res_desc->len;
+		log_debug("ram %llx %llx\n", gd->bd->bi_dram[bank].start,
+			  gd->bd->bi_dram[bank].size);
 	}
 
 	/* Add the memory below 4GB */
 	gd->bd->bi_dram[0].start = 0;
 	gd->bd->bi_dram[0].size = low_end;
 
-	/*
-	 * Set up an MTRR to the top of low, reserved memory. This is necessary
-	 * for graphics to run at full speed in U-Boot.
-	 */
-	mtrr_add_request(MTRR_TYPE_WRBACK, 0, mtrr_top);
-
 	return 0;
 }
 
@@ -166,7 +151,7 @@ unsigned int install_e820_map(unsigned int max_entries,
 #if CONFIG_IS_ENABLED(HANDOFF) && IS_ENABLED(CONFIG_USE_HOB)
 int handoff_arch_save(struct spl_handoff *ho)
 {
-	ho->arch.usable_ram_top = gd->bd->bi_dram[0].size;
+	ho->arch.usable_ram_top = fsp_get_usable_lowmem_top(gd->arch.hob_list);
 	ho->arch.hob_list = gd->arch.hob_list;
 
 	return 0;
-- 
2.25.1

[PATCH 2/7] x86: mtrr: Do not clear the unused ones in mtrr_commit()

[Bin Meng]

Current mtrr_commit() logic assumes that MTRR MSRs are programmed
consecutively from index 0 to its maximum number, and whenever it
detects an unused one, it clears all other MTRRs starting from that
one. However this may not always be the case.

In fact, the clear is not much helpful because these MTRRs come out
of reset as disabled already. Drop the clear codes.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 arch/x86/cpu/mtrr.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c
index 166aff380c..73cf7bb2be 100644
--- a/arch/x86/cpu/mtrr.c
+++ b/arch/x86/cpu/mtrr.c
@@ -157,10 +157,6 @@ int mtrr_commit(bool do_caches)
 	for (i = 0; i < gd->arch.mtrr_req_count; i++, req++)
 		set_var_mtrr(i, req->type, req->start, req->size);
 
-	/* Clear the ones that are unused */
-	debug("clear\n");
-	for (; i < mtrr_get_var_count(); i++)
-		wrmsrl(MTRR_PHYS_MASK_MSR(i), 0);
 	debug("close\n");
 	mtrr_close(&state, do_caches);
 	debug("mtrr done\n");
-- 
2.25.1

[PATCH 3/7] x86: mtrr: Skip MSRs that were already programmed in mtrr_commit()

[Bin Meng]

At present mtrr_commit() programs the MTRR MSRs starting from
index 0, which may overwrite MSRs that were already programmed
by previous boot stage or FSP.

Switch to call mtrr_set_next_var() instead.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 arch/x86/cpu/mtrr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c
index 73cf7bb2be..14c644eb56 100644
--- a/arch/x86/cpu/mtrr.c
+++ b/arch/x86/cpu/mtrr.c
@@ -155,7 +155,7 @@ int mtrr_commit(bool do_caches)
 	debug("open done\n");
 	qsort(req, gd->arch.mtrr_req_count, sizeof(*req), h_comp_mtrr);
 	for (i = 0; i < gd->arch.mtrr_req_count; i++, req++)
-		set_var_mtrr(i, req->type, req->start, req->size);
+		mtrr_set_next_var(req->type, req->start, req->size);
 
 	debug("close\n");
 	mtrr_close(&state, do_caches);
-- 
2.25.1

[PATCH 4/7] x86: mtrr: Abort if requested size is not power of 2

[Bin Meng]

The size parameter of mtrr_add_request() and mtrr_set_next_var()
shall be power of 2, otherwise the logic creates a mask that does
not meet the requirement of IA32_MTRR_PHYSMASK register.

Programming such a mask value to IA32_MTRR_PHYSMASK generates #GP.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 arch/x86/cpu/mtrr.c         | 7 +++++++
 arch/x86/include/asm/mtrr.h | 7 ++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c
index 14c644eb56..260a008093 100644
--- a/arch/x86/cpu/mtrr.c
+++ b/arch/x86/cpu/mtrr.c
@@ -26,6 +26,7 @@
 #include <asm/mp.h>
 #include <asm/msr.h>
 #include <asm/mtrr.h>
+#include <linux/log2.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -179,6 +180,9 @@ int mtrr_add_request(int type, uint64_t start, uint64_t size)
 	if (!gd->arch.has_mtrr)
 		return -ENOSYS;
 
+	if (!is_power_of_2(size))
+		return -EINVAL;
+
 	if (gd->arch.mtrr_req_count == MAX_MTRR_REQUESTS)
 		return -ENOSPC;
 	req = &gd->arch.mtrr_req[gd->arch.mtrr_req_count++];
@@ -223,6 +227,9 @@ int mtrr_set_next_var(uint type, uint64_t start, uint64_t size)
 {
 	int mtrr;
 
+	if (!is_power_of_2(size))
+		return -EINVAL;
+
 	mtrr = get_free_var_mtrr();
 	if (mtrr < 0)
 		return mtrr;
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 384672e93f..d1aa86bf1d 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -119,7 +119,7 @@ void mtrr_close(struct mtrr_state *state, bool do_caches);
  *
  * @type:	Requested type (MTRR_TYPE_)
  * @start:	Start address
- * @size:	Size
+ * @size:	Size, must be power of 2
  *
  * @return:	0 on success, non-zero on failure
  */
@@ -144,8 +144,9 @@ int mtrr_commit(bool do_caches);
  *
  * @type:	Requested type (MTRR_TYPE_)
  * @start:	Start address
- * @size:	Size
- * @return 0 on success, -ENOSPC if there are no more MTRRs
+ * @size:	Size, must be power of 2
+ * @return 0 on success, -EINVAL if size is not power of 2,
+ * -ENOSPC if there are no more MTRRs
  */
 int mtrr_set_next_var(uint type, uint64_t base, uint64_t size);
 
-- 
2.25.1

[PATCH 5/7] x86: cmd: hob: Fix the command usage and help messages

[Bin Meng]

At present the hob command usage and help messages are messed up
in a single line. They should be separated.

This was a regression introduced when [seq] and [-v] were added
to the command.

Fixes: d11544dfa9f4 ("x86: hob: Add way to show a single hob entry")
Fixes: 51af144eb7a0 ("x86: Allow showing details about a HOB entry")
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 cmd/x86/hob.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/cmd/x86/hob.c b/cmd/x86/hob.c
index 01db93eb3e..71e7fcbd65 100644
--- a/cmd/x86/hob.c
+++ b/cmd/x86/hob.c
@@ -158,8 +158,7 @@ static int do_hob(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 }
 
 U_BOOT_CMD(hob, 3, 1, do_hob,
-	   "[-v] [seq]  Print Hand-Off Block (HOB) information"
-	   "   -v  - Show detailed HOB information where available"
-	   "   seq - Record # to show (all by default)",
-	   ""
+	   "[-v] [seq]  Print Hand-Off Block (HOB) information",
+	   "   -v  - Show detailed HOB information where available\n"
+	   "   seq - Record # to show (all by default)"
 );
-- 
2.25.1

[PATCH 6/7] x86: cmd: hob: Fix display of resource type for system memory

[Bin Meng]

The resource type for system memory is currently displayed as
"unknown", which is wrong.

Fixes: 51af144eb7a0 ("x86: Allow showing details about a HOB entry")
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 cmd/x86/hob.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/x86/hob.c b/cmd/x86/hob.c
index 71e7fcbd65..04d092dbe7 100644
--- a/cmd/x86/hob.c
+++ b/cmd/x86/hob.c
@@ -78,7 +78,7 @@ static void show_hob_details(const struct hob_header *hdr)
 		const struct hob_res_desc *res = ptr;
 		const char *typename;
 
-		typename = res->type > 0 && res->type <= RES_MAX_MEM_TYPE ?
+		typename = res->type >= RES_SYS_MEM && res->type <= RES_MAX_MEM_TYPE ?
 			res_type[res->type] : "unknown";
 
 		printf("     base = %08llx, len = %08llx, end = %08llx, type = %d (%s)\n\n",
-- 
2.25.1

[PATCH 7/7] x86: fsp: Only FSP2 has INIT_PHASE_END_FIRMWARE

[Bin Meng]

For FSP1, there is no such INIT_PHASE_END_FIRMWARE.

Fixes: 7c73cea44290 ("x86: Notify the FSP of the 'end firmware' event")
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

 arch/x86/lib/fsp/fsp_common.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
index 6365b0a50a..0155eaee8d 100644
--- a/arch/x86/lib/fsp/fsp_common.c
+++ b/arch/x86/lib/fsp/fsp_common.c
@@ -68,13 +68,15 @@ void board_final_cleanup(void)
 	/* TODO(sjg@chromium.org): This causes Linux to crash */
 	return;
 
-	/* call into FspNotify */
-	debug("Calling into FSP (notify phase INIT_PHASE_END_FIRMWARE): ");
-	status = fsp_notify(NULL, INIT_PHASE_END_FIRMWARE);
-	if (status)
-		debug("fail, error code %x\n", status);
-	else
-		debug("OK\n");
+	if (CONFIG_IS_ENABLED(FSP_VERSION2)) {
+		/* call into FspNotify */
+		debug("Calling into FSP (notify phase INIT_PHASE_END_FIRMWARE): ");
+		status = fsp_notify(NULL, INIT_PHASE_END_FIRMWARE);
+		if (status)
+			debug("fail, error code %x\n", status);
+		else
+			debug("OK\n");
+	}
 }
 
 int fsp_save_s3_stack(void)
-- 
2.25.1

Re: [PATCH 1/7] x86: fsp: Don't program MTRR for DRAM

[Simon Glass]

Hi Bin,

On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> This actually reverts the following 2 commits:
>
> commit 427911001809 ("x86: Set up the MTRR for SDRAM")
> commit d46c0932a9d4 ("x86: fsp: Adjust calculations for MTRR range and DRAM top")
>
> There are several outstanding issues as to why:
>
> * For FSP1, the system memory and reserved memory used by FSP are
>   already programmed in the MTRR by FSP.
> * The 'mtrr_top' mistakenly includes TSEG memory range that has the
>   same RES_MEM_RESERVED resource type. Its address is programmed
>   and reported by FSP to be near the top of 4 GiB space, which is
>   not what we want for SDRAM.
> * The call to mtrr_add_request() is not guaranteed to have its size
>   to be exactly the power of 2. This causes reserved bits of the
>   IA32_MTRR_PHYSMASK register to be written which generates #GP.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  arch/x86/lib/fsp/fsp_dram.c | 35 ++++++++++-------------------------
>  1 file changed, 10 insertions(+), 25 deletions(-)

Unfortunately this makes coral (FSP2) go *very* slowly. Can you
perhaps do this change just for FSP1?

Regards,
Simon

Re: [PATCH 2/7] x86: mtrr: Do not clear the unused ones in mtrr_commit()

[Simon Glass]

On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Current mtrr_commit() logic assumes that MTRR MSRs are programmed
> consecutively from index 0 to its maximum number, and whenever it
> detects an unused one, it clears all other MTRRs starting from that
> one. However this may not always be the case.
>
> In fact, the clear is not much helpful because these MTRRs come out
> of reset as disabled already. Drop the clear codes.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  arch/x86/cpu/mtrr.c | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
Tested-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/7] x86: mtrr: Skip MSRs that were already programmed in mtrr_commit()

[Simon Glass]

On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> At present mtrr_commit() programs the MTRR MSRs starting from
> index 0, which may overwrite MSRs that were already programmed
> by previous boot stage or FSP.
>
> Switch to call mtrr_set_next_var() instead.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  arch/x86/cpu/mtrr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
Tested-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 4/7] x86: mtrr: Abort if requested size is not power of 2

[Simon Glass]

On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> The size parameter of mtrr_add_request() and mtrr_set_next_var()
> shall be power of 2, otherwise the logic creates a mask that does
> not meet the requirement of IA32_MTRR_PHYSMASK register.
>
> Programming such a mask value to IA32_MTRR_PHYSMASK generates #GP.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  arch/x86/cpu/mtrr.c         | 7 +++++++
>  arch/x86/include/asm/mtrr.h | 7 ++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
Tested-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 5/7] x86: cmd: hob: Fix the command usage and help messages

[Simon Glass]

On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> At present the hob command usage and help messages are messed up
> in a single line. They should be separated.
>
> This was a regression introduced when [seq] and [-v] were added
> to the command.
>
> Fixes: d11544dfa9f4 ("x86: hob: Add way to show a single hob entry")
> Fixes: 51af144eb7a0 ("x86: Allow showing details about a HOB entry")
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  cmd/x86/hob.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
Tested-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 6/7] x86: cmd: hob: Fix display of resource type for system memory

[Simon Glass]

On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> The resource type for system memory is currently displayed as
> "unknown", which is wrong.
>
> Fixes: 51af144eb7a0 ("x86: Allow showing details about a HOB entry")
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  cmd/x86/hob.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
Tested-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 7/7] x86: fsp: Only FSP2 has INIT_PHASE_END_FIRMWARE

[Simon Glass]

Hi Bin,

On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> For FSP1, there is no such INIT_PHASE_END_FIRMWARE.
>
> Fixes: 7c73cea44290 ("x86: Notify the FSP of the 'end firmware' event")
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
>  arch/x86/lib/fsp/fsp_common.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
> index 6365b0a50a..0155eaee8d 100644
> --- a/arch/x86/lib/fsp/fsp_common.c
> +++ b/arch/x86/lib/fsp/fsp_common.c
> @@ -68,13 +68,15 @@ void board_final_cleanup(void)
>         /* TODO(sjg@chromium.org): This causes Linux to crash */
>         return;
>
> -       /* call into FspNotify */
> -       debug("Calling into FSP (notify phase INIT_PHASE_END_FIRMWARE): ");
> -       status = fsp_notify(NULL, INIT_PHASE_END_FIRMWARE);
> -       if (status)
> -               debug("fail, error code %x\n", status);
> -       else
> -               debug("OK\n");
> +       if (CONFIG_IS_ENABLED(FSP_VERSION2)) {
> +               /* call into FspNotify */
> +               debug("Calling into FSP (notify phase INIT_PHASE_END_FIRMWARE): ");
> +               status = fsp_notify(NULL, INIT_PHASE_END_FIRMWARE);
> +               if (status)
> +                       debug("fail, error code %x\n", status);
> +               else
> +                       debug("OK\n");
> +       }
>  }

So shouldn't we move this whole function into fsp2?

>
>  int fsp_save_s3_stack(void)
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH 2/7] x86: mtrr: Do not clear the unused ones in mtrr_commit()

[Bin Meng]

On Mon, Aug 2, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Current mtrr_commit() logic assumes that MTRR MSRs are programmed
> > consecutively from index 0 to its maximum number, and whenever it
> > detects an unused one, it clears all other MTRRs starting from that
> > one. However this may not always be the case.
> >
> > In fact, the clear is not much helpful because these MTRRs come out
> > of reset as disabled already. Drop the clear codes.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  arch/x86/cpu/mtrr.c | 4 ----
> >  1 file changed, 4 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
> Tested-by: Simon Glass <sjg@chromium.org>

applied to u-boot-x86, thanks!

Re: [PATCH 3/7] x86: mtrr: Skip MSRs that were already programmed in mtrr_commit()

[Bin Meng]

On Mon, Aug 2, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > At present mtrr_commit() programs the MTRR MSRs starting from
> > index 0, which may overwrite MSRs that were already programmed
> > by previous boot stage or FSP.
> >
> > Switch to call mtrr_set_next_var() instead.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  arch/x86/cpu/mtrr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
> Tested-by: Simon Glass <sjg@chromium.org>

applied to u-boot-x86, thanks!

Re: [PATCH 4/7] x86: mtrr: Abort if requested size is not power of 2

[Bin Meng]

On Mon, Aug 2, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > The size parameter of mtrr_add_request() and mtrr_set_next_var()
> > shall be power of 2, otherwise the logic creates a mask that does
> > not meet the requirement of IA32_MTRR_PHYSMASK register.
> >
> > Programming such a mask value to IA32_MTRR_PHYSMASK generates #GP.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  arch/x86/cpu/mtrr.c         | 7 +++++++
> >  arch/x86/include/asm/mtrr.h | 7 ++++---
> >  2 files changed, 11 insertions(+), 3 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
> Tested-by: Simon Glass <sjg@chromium.org>

applied to u-boot-x86, thanks!

Re: [PATCH 5/7] x86: cmd: hob: Fix the command usage and help messages

[Bin Meng]

On Mon, Aug 2, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > At present the hob command usage and help messages are messed up
> > in a single line. They should be separated.
> >
> > This was a regression introduced when [seq] and [-v] were added
> > to the command.
> >
> > Fixes: d11544dfa9f4 ("x86: hob: Add way to show a single hob entry")
> > Fixes: 51af144eb7a0 ("x86: Allow showing details about a HOB entry")
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  cmd/x86/hob.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
> Tested-by: Simon Glass <sjg@chromium.org>

applied to u-boot-x86, thanks!

Re: [PATCH 6/7] x86: cmd: hob: Fix display of resource type for system memory

[Bin Meng]

On Mon, Aug 2, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > The resource type for system memory is currently displayed as
> > "unknown", which is wrong.
> >
> > Fixes: 51af144eb7a0 ("x86: Allow showing details about a HOB entry")
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  cmd/x86/hob.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Tested on chromebook_coral, chromebook_samus, chromebook_link, minnowmax
> Tested-by: Simon Glass <sjg@chromium.org>

applied to u-boot-x86, thanks!

Re: [PATCH 1/7] x86: fsp: Don't program MTRR for DRAM

[Bin Meng]

Hi Simon,

On Mon, Aug 2, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > This actually reverts the following 2 commits:
> >
> > commit 427911001809 ("x86: Set up the MTRR for SDRAM")
> > commit d46c0932a9d4 ("x86: fsp: Adjust calculations for MTRR range and DRAM top")
> >
> > There are several outstanding issues as to why:
> >
> > * For FSP1, the system memory and reserved memory used by FSP are
> >   already programmed in the MTRR by FSP.
> > * The 'mtrr_top' mistakenly includes TSEG memory range that has the
> >   same RES_MEM_RESERVED resource type. Its address is programmed
> >   and reported by FSP to be near the top of 4 GiB space, which is
> >   not what we want for SDRAM.
> > * The call to mtrr_add_request() is not guaranteed to have its size
> >   to be exactly the power of 2. This causes reserved bits of the
> >   IA32_MTRR_PHYSMASK register to be written which generates #GP.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  arch/x86/lib/fsp/fsp_dram.c | 35 ++++++++++-------------------------
> >  1 file changed, 10 insertions(+), 25 deletions(-)
>
> Unfortunately this makes coral (FSP2) go *very* slowly. Can you
> perhaps do this change just for FSP1?

Sure will do that in v2.

Regards,
Bin

Re: [PATCH 7/7] x86: fsp: Only FSP2 has INIT_PHASE_END_FIRMWARE

[Bin Meng]

Hi Simon,

On Mon, Aug 2, 2021 at 3:19 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Sat, 31 Jul 2021 at 02:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > For FSP1, there is no such INIT_PHASE_END_FIRMWARE.
> >
> > Fixes: 7c73cea44290 ("x86: Notify the FSP of the 'end firmware' event")
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >
> > ---
> >
> >  arch/x86/lib/fsp/fsp_common.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
> > index 6365b0a50a..0155eaee8d 100644
> > --- a/arch/x86/lib/fsp/fsp_common.c
> > +++ b/arch/x86/lib/fsp/fsp_common.c
> > @@ -68,13 +68,15 @@ void board_final_cleanup(void)
> >         /* TODO(sjg@chromium.org): This causes Linux to crash */
> >         return;
> >
> > -       /* call into FspNotify */
> > -       debug("Calling into FSP (notify phase INIT_PHASE_END_FIRMWARE): ");
> > -       status = fsp_notify(NULL, INIT_PHASE_END_FIRMWARE);
> > -       if (status)
> > -               debug("fail, error code %x\n", status);
> > -       else
> > -               debug("OK\n");
> > +       if (CONFIG_IS_ENABLED(FSP_VERSION2)) {
> > +               /* call into FspNotify */
> > +               debug("Calling into FSP (notify phase INIT_PHASE_END_FIRMWARE): ");
> > +               status = fsp_notify(NULL, INIT_PHASE_END_FIRMWARE);
> > +               if (status)
> > +                       debug("fail, error code %x\n", status);
> > +               else
> > +                       debug("OK\n");
> > +       }
> >  }
>
> So shouldn't we move this whole function into fsp2?

Yes, I think so.

Regards,
Bin