[PATCH 6/8] x86: broadwell: Set up MTRRs

[Simon Glass]

Hi Bin,

On Mon, 4 Sept 2023 at 03:22, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Thu, Aug 24, 2023 at 2:48 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > The current condition does not handle the samus_tpl case where it sets
> > up the RAM in SPL but needs to commit the MTRRs in U-Boot proper.
> >
> > Add another case to handle this and update the comment.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >  arch/x86/lib/init_helpers.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c
> > index 60a2707dcf1..bf0c921577d 100644
> > --- a/arch/x86/lib/init_helpers.c
> > +++ b/arch/x86/lib/init_helpers.c
> > @@ -15,7 +15,8 @@ DECLARE_GLOBAL_DATA_PTR;
> >  int init_cache_f_r(void)
> >  {
> >         bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT) ||
> > -                IS_ENABLED(CONFIG_FSP_VERSION2);
> > +                IS_ENABLED(CONFIG_FSP_VERSION2) ||
> > +                (IS_ENABLED(CONFIG_TPL) && IS_ENABLED(CONFIG_HAVE_MRC));
>
> So this combination is only for Samus TPL case, not a generic one, right?

It is certainly targeted at samus TPL, but in a way it is applicable
generically. When TPL is in use, we do the memory init in SPL and pass
the RAM into to U-Boot proper, so it sets up the MTRRs. This is the
same as the only other TPL board (coral). So samus and coral are in
common in this sense...with the different being that samus uses MRC
but coral usese FSP2.

>
> I hate this as it's easy to break. Maybe we should be calling an API
> from the board specific codes to determine whether we should do the
> MTRR programming. Thoughts?

I am not sure how we would do that. I did suggest a special Kconfig a
while back, so that boards can overwrite the setting if needed. But in
any case we would want a sensible default for that Kconfig.

>
> >         int ret;
> >
> >         /*
> > @@ -23,11 +24,9 @@ int init_cache_f_r(void)
> >          *
> >          * 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 with FSPv2 or MRC - MTRRs must be set here
> >          * booting from coreboot - in this case there is no SPL, so we set up
> >          *      the MTRRs here
> > -        * 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_FSP_VERSION1) &&
> >                 !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
> > --
>

At least until we figure out something better, this seems clear enough.

Regards,
Simon