[PATCH 1/5] x86: fsp: Use mtrr_set_next_var() for graphics memory

[Bin Meng]

Hi Simon,

On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > 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 at gmail.com>
> > > > ---
> > > >
> > > >  arch/x86/lib/fsp/fsp_graphics.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > Thanks for looking into this. The series works fine on link. I suspect
> > > it will be find on samus too, but I cannot test right now. Sadly
> > > minnowmax is also dead right now but I hope to fix it soon. I don't
> > > expect any problems there.
> > >
> > > However, for coral, this first patch breaks the mtrrs. With master we get:
> > >
> > > => mtrr
> > > CPU 0:
> > > Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> > > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > > 2   Y     Back         0000000000000000 0000007f80000000 0000000080000000
> > > 3   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > 4   Y     Back         0000000100000000 0000007f80000000 0000000080000000
> > > 5   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 6   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 7   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 8   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 9   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > >
> > > with this patch on coral we get:
> > >
> > > => mtrr
> > > CPU 0:
> > > Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> > > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > > 2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > >
> > > At present coral expects to handle the MTRRs itself, and it seems that
> > > perhaps the APL FSPv2 does not? Do we need a new Kconfig for dealing
> > > with FSPv2 perhaps?
> >
> > I am a little bit confused. The comment in arch/x86/lib/fsp/fsp_dram.c::
> > dram_init_banksize() says:
> >
> >         /*
> >          * For FSP1, the system memory and reserved memory used by FSP are
> >          * already programmed in the MTRR by FSP. Also it is observed that
> >          * FSP on Intel Queensbay platform reports the TSEG memory range
> >          * that has the same RES_MEM_RESERVED resource type whose address
> >          * is programmed by FSP to be near the top of 4 GiB space, which is
> >          * not what we want for DRAM.
> >          *
> >          * However it seems FSP2's behavior is different. We need to add the
> >          * DRAM range in MTRR otherwise the boot process goes very slowly,
> >          * which was observed on Chromebook Coral with FSP2.
> >          */
> >
> > So on Coral with FSP2, U-Boot programs the MTTR by itself.
> >
> > In this dram_init_banksize(), it calls mtrr_add_request() 3 times, 2
> > of which should be what you were seeing as 2 and 4 below:
> >
> > > 2   Y     Back         0000000000000000 0000007f80000000 0000000080000000
> > > 3   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > 4   Y     Back         0000000100000000 0000007f80000000 0000000080000000
> >
> > The #3 should be the FSP graphics frame buffer. But I failed to
> > understand how the FSP graphics programs a MTRR register in between
> > the 2 memory regions programmed by dram_init_banksize() on
> > u-boot/master, how could that happen?
>
> Remember that the MTRRs are sorted, so the order or mtrr_add_request()
> calls does not matter.
>

Still cannot explain.

0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
2   Y     Back         0000000000000000 0000007f80000000 0000000080000000
3   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
4   Y     Back         0000000100000000 0000007f80000000 0000000080000000

After we sort the mtrr memory range, #2 whose base is 0x0 should have
been put to the first entry, then followed by #3 whose base is
0xb0000000.

Regards,
Bin