Hi Bin, On Mon, 8 May 2023 at 08:51, Bin Meng <bmeng...@gmail.com> wrote: > > On Mon, May 8, 2023 at 10:48 PM Bin Meng <bmeng...@gmail.com> wrote: > > > > Hi Simon, > > > > On Fri, May 5, 2023 at 6:51 AM Simon Glass <s...@chromium.org> wrote: > > > > > > 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 <s...@chromium.org> > > > Fixes: 3bcd6cf89ef ("x86: mtrr: Skip MSRs that were already programmed..") > > > Fixes: 596bd0589ad ("x86: mtrr: Do not clear the unused ones..") > > > --- > > > > > > Changes in v3: > > > - Fix invalid commit IDs obtained from another branch > > > > > > 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 e69dfb552b1..c174dd9b3ad 100644 > > > --- a/arch/x86/cpu/mtrr.c > > > +++ b/arch/x86/cpu/mtrr.c > > > @@ -156,8 +156,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); > > > > This change actually reverts 3bcd6cf89ef ("x86: mtrr: Skip MSRs that > > were already programmed.."), but as 3bcd6cf89ef commit message says: > > > > 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. > > > > So this change will cause regression. > > > > > > > > + /* Clear the ones that are unused */ > > > + debug("clear\n"); > > > + for (; i < mtrr_get_var_count(); i++) > > > + wrmsrl(MTRR_PHYS_MASK_MSR(i), 0); > > > > This change actually reverts 596bd0589ad ("x86: mtrr: Do not clear the > > unused ones.."), which will also create regression unfortunately.. > > > > > debug("close\n"); > > > mtrr_close(&state, do_caches); > > > debug("mtrr done\n"); > > > -- > > The regression mentioned above will cause Intel Crown Bay fail to > boot. See > https://lore.kernel.org/u-boot/20210731084529.7524-1-bmeng...@gmail.com/
Can that board perhaps use the other functoin to set MTRRs? It is mtrr_set_next_var() The change in the API has broken two of the Chromebooks which is why I am trying to go back to the way it was. Does this board set up the MTRRs in FSP? If so, perhaps we need a Kconfig for that? It is not what the newer FSPs do, but perhaps we could use that behaviour for FSPv1? Regards, Simon