From 41fbb344695f224d70d3c469ea418015279ba55e Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Mon, 31 Jul 2023 07:56:02 -0600 Subject: [PATCH 1/7] x86: Change testing logic of mtrr commit 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 Tweak to put back CONFIG_FSP_VERSION2 at top: Signed-off-by: Simon Glass --- 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 f33194045f9..60a2707dcf1 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) && !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER); if (do_mtrr) { From 3dfa4115016de44410f7cc58fa5277ec39dcd4e1 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Mon, 31 Jul 2023 14:01:04 +0800 Subject: [PATCH 2/7] x86: fsp: Use mtrr_set_next_var() for graphics memory At present this uses mtrr_add_request() & mtrr_commit() combination to program the MTRR for graphics memory. This usage has two major issues as below: - mtrr_commit() will re-initialize all MTRR registers from index 0, using the settings previously added by mtrr_add_request() and saved in gd->arch.mtrr_req[], which won't cause any issue but is unnecessary - The way such combination works is based on the assumption that U-Boot has full control with MTRR programming (e.g.: U-Boot without any blob that does all low-level initialization on its own, or using FSP2 which does not touch MTRR), but this is not the case with FSP. FSP programs some MTRRs during its execution but U-Boot does not have the settings saved in gd->arch.mtrr_req[] and when doing mtrr_commit() it will corrupt what was already programmed previously. Correct this to use mtrr_set_next_var() instead. Signed-off-by: Bin Meng Reviewed-by: Simon Glass --- arch/x86/lib/fsp/fsp_graphics.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/x86/lib/fsp/fsp_graphics.c b/arch/x86/lib/fsp/fsp_graphics.c index 2bcc49f6051..09d5da8c841 100644 --- a/arch/x86/lib/fsp/fsp_graphics.c +++ b/arch/x86/lib/fsp/fsp_graphics.c @@ -110,8 +110,7 @@ static int fsp_video_probe(struct udevice *dev) if (ret) goto err; - mtrr_add_request(MTRR_TYPE_WRCOMB, vesa->phys_base_ptr, 256 << 20); - mtrr_commit(true); + mtrr_set_next_var(MTRR_TYPE_WRCOMB, vesa->phys_base_ptr, 256 << 20); printf("%dx%dx%d @ %x\n", uc_priv->xsize, uc_priv->ysize, vesa->bits_per_pixel, vesa->phys_base_ptr); From b5126f26d0c8d8560081b1f36ed7aee7f083104b Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Mon, 31 Jul 2023 14:01:05 +0800 Subject: [PATCH 3/7] video: broadwell: Use mtrr_set_next_var() for graphics memory At present this uses mtrr_add_request() & mtrr_commit() combination to program the MTRR for graphics memory. This usage has two major issues as below: - mtrr_commit() will re-initialize all MTRR registers from index 0, using the settings previously added by mtrr_add_request() and saved in gd->arch.mtrr_req[], which won't cause any issue but is unnecessary - The way such combination works is based on the assumption that U-Boot has full control with MTRR programming (e.g.: U-Boot without any blob that does all low-level initialization on its own, or using FSP2 which does not touch MTRR), but this is not the case with FSP. FSP programs some MTRRs during its execution but U-Boot does not have the settings saved in gd->arch.mtrr_req[] and when doing mtrr_commit() it will corrupt what was already programmed previously. Correct this to use mtrr_set_next_var() instead. Signed-off-by: Bin Meng Reviewed-by: Simon Glass --- drivers/video/broadwell_igd.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/video/broadwell_igd.c b/drivers/video/broadwell_igd.c index 6aa4e27071d..83b6c908a8d 100644 --- a/drivers/video/broadwell_igd.c +++ b/drivers/video/broadwell_igd.c @@ -693,13 +693,9 @@ static int broadwell_igd_probe(struct udevice *dev) /* Use write-combining for the graphics memory, 256MB */ fbbase = IS_ENABLED(CONFIG_VIDEO_COPY) ? plat->copy_base : plat->base; - ret = mtrr_add_request(MTRR_TYPE_WRCOMB, fbbase, 256 << 20); - if (!ret) - ret = mtrr_commit(true); - if (ret && ret != -ENOSYS) { - printf("Failed to add MTRR: Display will be slow (err %d)\n", - ret); - } + ret = mtrr_set_next_var(MTRR_TYPE_WRCOMB, fbbase, 256 << 20); + if (ret) + printf("Failed to add MTRR: Display will be slow (err %d)\n", ret); debug("fb=%lx, size %x, display size=%d %d %d\n", plat->base, plat->size, uc_priv->xsize, uc_priv->ysize, uc_priv->bpix); From 0f497b2b8cb2c448cef70460d466c4ad1459f5ec Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Mon, 31 Jul 2023 14:01:06 +0800 Subject: [PATCH 4/7] video: ivybridge: Use mtrr_set_next_var() for graphics memory At present this uses mtrr_add_request() & mtrr_commit() combination to program the MTRR for graphics memory. This usage has two major issues as below: - mtrr_commit() will re-initialize all MTRR registers from index 0, using the settings previously added by mtrr_add_request() and saved in gd->arch.mtrr_req[], which won't cause any issue but is unnecessary - The way such combination works is based on the assumption that U-Boot has full control with MTRR programming (e.g.: U-Boot without any blob that does all low-level initialization on its own, or using FSP2 which does not touch MTRR), but this is not the case with FSP. FSP programs some MTRRs during its execution but U-Boot does not have the settings saved in gd->arch.mtrr_req[] and when doing mtrr_commit() it will corrupt what was already programmed previously. Correct this to use mtrr_set_next_var() instead. Signed-off-by: Bin Meng Reviewed-by: Simon Glass --- drivers/video/ivybridge_igd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/video/ivybridge_igd.c b/drivers/video/ivybridge_igd.c index 9264dd6770d..c2cc976618a 100644 --- a/drivers/video/ivybridge_igd.c +++ b/drivers/video/ivybridge_igd.c @@ -774,8 +774,7 @@ static int bd82x6x_video_probe(struct udevice *dev) /* Use write-combining for the graphics memory, 256MB */ fbbase = IS_ENABLED(CONFIG_VIDEO_COPY) ? plat->copy_base : plat->base; - mtrr_add_request(MTRR_TYPE_WRCOMB, fbbase, 256 << 20); - mtrr_commit(true); + mtrr_set_next_var(MTRR_TYPE_WRCOMB, fbbase, 256 << 20); return 0; } From bff002d45b0d006458613790eb43397c37f93ae8 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Mon, 31 Jul 2023 14:01:07 +0800 Subject: [PATCH 5/7] video: vesa: Use mtrr_set_next_var() for graphics memory At present this uses mtrr_add_request() & mtrr_commit() combination to program the MTRR for graphics memory. This usage has two major issues as below: - mtrr_commit() will re-initialize all MTRR registers from index 0, using the settings previously added by mtrr_add_request() and saved in gd->arch.mtrr_req[], which won't cause any issue but is unnecessary - The way such combination works is based on the assumption that U-Boot has full control with MTRR programming (e.g.: U-Boot without any blob that does all low-level initialization on its own, or using FSP2 which does not touch MTRR), but this is not the case with FSP. FSP programs some MTRRs during its execution but U-Boot does not have the settings saved in gd->arch.mtrr_req[] and when doing mtrr_commit() it will corrupt what was already programmed previously. Correct this to use mtrr_set_next_var() instead. Signed-off-by: Bin Meng Reviewed-by: Simon Glass --- drivers/video/vesa.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/video/vesa.c b/drivers/video/vesa.c index cac3bb0c331..50912c5c8bc 100644 --- a/drivers/video/vesa.c +++ b/drivers/video/vesa.c @@ -23,8 +23,7 @@ static int vesa_video_probe(struct udevice *dev) /* Use write-combining for the graphics memory, 256MB */ fbbase = IS_ENABLED(CONFIG_VIDEO_COPY) ? plat->copy_base : plat->base; - mtrr_add_request(MTRR_TYPE_WRCOMB, fbbase, 256 << 20); - mtrr_commit(true); + mtrr_set_next_var(MTRR_TYPE_WRCOMB, fbbase, 256 << 20); return 0; } From d560f7cae04128061167c1507af08293b666c766 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 31 Jul 2023 14:01:08 +0800 Subject: [PATCH 6/7] x86: Return mtrr_add_request() to its old purpose This function used to be for adding a list of requests to be actioned on relocation. Revert it back to this purpose, to avoid problems with boards which need control of their MTRRs (i.e. those which don't use FSP). The mtrr_set_next_var() function is available when the next free variable-MTRR must be set, so this can be used instead. Signed-off-by: Simon Glass Reviewed-by: Bin Meng Fixes: 3bcd6cf89ef ("x86: mtrr: Skip MSRs that were already programmed..") Fixes: 596bd0589ad ("x86: mtrr: Do not clear the unused ones..") Signed-off-by: Bin Meng --- arch/x86/cpu/mtrr.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c index d57fcface01..9c24ae984e9 100644 --- a/arch/x86/cpu/mtrr.c +++ b/arch/x86/cpu/mtrr.c @@ -166,8 +166,12 @@ 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++) - mtrr_set_next_var(req->type, req->start, req->size); + 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"); From db971a7587d04b3f1cf2e6d452f9e37f50c5b3ed Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 25 Jul 2023 15:37:06 -0600 Subject: [PATCH 7/7] x86: Add a little more info to cbsysinfo Show the number of records in the table and the total table size in bytes. Signed-off-by: Simon Glass Reviewed-by: Bin Meng --- arch/x86/include/asm/cb_sysinfo.h | 4 ++++ arch/x86/lib/coreboot/cb_sysinfo.c | 4 ++++ cmd/x86/cbsysinfo.c | 5 +++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/arch/x86/include/asm/cb_sysinfo.h b/arch/x86/include/asm/cb_sysinfo.h index 2c78b22d0d2..12fa395ffd2 100644 --- a/arch/x86/include/asm/cb_sysinfo.h +++ b/arch/x86/include/asm/cb_sysinfo.h @@ -138,6 +138,8 @@ * @rsdp: Pointer to ACPI RSDP table * @unimpl_count: Number of entries in unimpl_map[] * @unimpl: List of unimplemented IDs (bottom 8 bits only) + * @table_size: Number of bytes taken up by the sysinfo table + * @rec_count: Number of records in the sysinfo table */ struct sysinfo_t { unsigned int cpu_khz; @@ -219,6 +221,8 @@ struct sysinfo_t { void *rsdp; u32 unimpl_count; u8 unimpl[SYSINFO_MAX_UNIMPL]; + uint table_size; + uint rec_count; }; extern struct sysinfo_t lib_sysinfo; diff --git a/arch/x86/lib/coreboot/cb_sysinfo.c b/arch/x86/lib/coreboot/cb_sysinfo.c index 42cc3a128d9..dfbc80c430e 100644 --- a/arch/x86/lib/coreboot/cb_sysinfo.c +++ b/arch/x86/lib/coreboot/cb_sysinfo.c @@ -447,6 +447,8 @@ static int cb_parse_header(void *addr, int len, struct sysinfo_t *info) ptr += rec->size; } + info->table_size += (void *)ptr - (void *)header; + info->rec_count += header->table_entries; return 1; } @@ -462,6 +464,8 @@ int get_coreboot_info(struct sysinfo_t *info) addr = locate_coreboot_table(); if (addr < 0) return addr; + info->table_size = 0; + info->rec_count = 0; ret = cb_parse_header((void *)addr, 0x1000, info); if (!ret) return -ENOENT; diff --git a/cmd/x86/cbsysinfo.c b/cmd/x86/cbsysinfo.c index 2b8d3b0a435..84822a3e321 100644 --- a/cmd/x86/cbsysinfo.c +++ b/cmd/x86/cbsysinfo.c @@ -190,8 +190,9 @@ static void show_table(struct sysinfo_t *info, bool verbose) struct cb_serial *ser = info->serial; int i; - printf("Coreboot table at %lx, decoded to %p", - gd->arch.coreboot_table, info); + printf("Coreboot table at %lx, size %x, records %x (dec %d), decoded to %p", + gd->arch.coreboot_table, info->table_size, info->rec_count, + info->rec_count, info); if (info->header) printf(", forwarded to %p\n", info->header); printf("\n");