[PATCH v3] x86: Change tesing logic of mtrr commit

[Bin Meng]

Hi Simon,

On Tue, Aug 1, 2023 at 12:13 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 31 Jul 2023 at 08:58, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Jul 31, 2023 at 9:56 PM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > From: Bin Meng <bmeng.cn at 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 at gmail.com>
> > > Tweak to put back CONFIG_FSP_VERSION2 at top:
> > > Signed-off-by: Simon Glass <sjg at 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