[PATCH v2 1/6] x86: Change tesing logic of mtrr commit
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
---
Changes in v2: - new patch: "x86: Change tesing logic of mtrr commit"
arch/x86/lib/init_helpers.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c index f33194045f..a2535e4468 100644 --- a/arch/x86/lib/init_helpers.c +++ b/arch/x86/lib/init_helpers.c @@ -14,15 +14,13 @@ DECLARE_GLOBAL_DATA_PTR;
int init_cache_f_r(void) { - bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT) || - IS_ENABLED(CONFIG_FSP_VERSION2); + bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT); int ret;
/* * 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,7 +28,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) && + do_mtrr &= (!IS_ENABLED(CONFIG_SPL) || IS_ENABLED(CONFIG_FSP_VERSION2)) && !IS_ENABLED(CONFIG_FSP_VERSION1) && !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com ---
(no changes since v1)
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 2bcc49f605..09d5da8c84 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);
Hi Simon,
On Mon, Jul 31, 2023 at 2:02 PM Bin Meng bmeng@tinylab.org wrote:
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com
(no changes since v1)
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 2bcc49f605..09d5da8c84 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);--
Would you give a Rb/Tb tag, if you will?
Regards, Bin
On Mon, 31 Jul 2023 at 00:02, Bin Meng bmeng@tinylab.org wrote:
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com
(no changes since v1)
arch/x86/lib/fsp/fsp_graphics.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com ---
(no changes since v1)
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 6aa4e27071..83b6c908a8 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);
On Mon, 31 Jul 2023 at 00:03, Bin Meng bmeng@tinylab.org wrote:
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com
(no changes since v1)
drivers/video/broadwell_igd.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com ---
(no changes since v1)
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 9264dd6770..c2cc976618 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; }
On Mon, 31 Jul 2023 at 00:04, Bin Meng bmeng@tinylab.org wrote:
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com
(no changes since v1)
drivers/video/ivybridge_igd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com ---
(no changes since v1)
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 cac3bb0c33..50912c5c8b 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; }
On Mon, 31 Jul 2023 at 00:05, Bin Meng bmeng@tinylab.org wrote:
From: Bin Meng bmeng.cn@gmail.com
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 bmeng.cn@gmail.com
(no changes since v1)
drivers/video/vesa.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
From: Simon Glass sjg@chromium.org
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 sjg@chromium.org Reviewed-by: Bin Meng bmeng.cn@gmail.com 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 bmeng.cn@gmail.com
---
(no changes since v1)
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 d57fcface0..9c24ae984e 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");
On Mon, Jul 31, 2023 at 2:01 PM Bin Meng bmeng@tinylab.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
Changes in v2:
- new patch: "x86: Change tesing logic of mtrr commit"
arch/x86/lib/init_helpers.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
This patch is superceded by https://patchwork.ozlabs.org/project/uboot/patch/20230731135608.975404-1-sjg...
The rest of the patches are applied to u-boot-x86, thanks!
Regards, Bin
participants (3)
-
Bin Meng -
Bin Meng -
Simon Glass