| Message ID | 20230731135608.975404-1-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | 41fbb344695f224d70d3c469ea418015279ba55e |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | [v3] x86: Change tesing logic of mtrr commit | expand |
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 My patch covers such scenario, no? > !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER); > > if (do_mtrr) { Regards, Bin
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 :-) Regards, Simon
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
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(-) > applied to u-boot-x86, thanks!
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) && !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER); if (do_mtrr) {