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

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@gmail.com>
---

 arch/x86/lib/fsp/fsp_graphics.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Hi Bin,

On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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?

Regards,
Simon

Hi Simon,

On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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?

On the other hand, with this patch, how could the FSP graphics memory
programs a MTRR register that should be programmed by
dram_init_banksize()?

Regards,
Bin

Hi Bin,

On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.

>
> On the other hand, with this patch, how could the FSP graphics memory
> programs a MTRR register that should be programmed by
> dram_init_banksize()?

I believe this adds a new mtrr and then commits the result.

Regards,
Simon

Hi Simon,

On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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

Hi Bin,

On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.

Right, but the thing is, your first patch does not revert the
behaviour of mtrr_add_request(). It is still just adding to the end.

i.e. mtrr_commt() adds new ones but does not overwrite those at the back.

Looking at your full series, this is what I see on coral:

0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000

But I see that do_mtrr is wrong for coral in init_cache_f_r():

do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&

So with coral the mtrrs are never written?

Regards,
Simon

Hi Simon,

On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.
>
> Right, but the thing is, your first patch does not revert the
> behaviour of mtrr_add_request(). It is still just adding to the end.
>
> i.e. mtrr_commt() adds new ones but does not overwrite those at the back.
>
> Looking at your full series, this is what I see on coral:
>
> 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> 2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
>
> But I see that do_mtrr is wrong for coral in init_cache_f_r():
>
> do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&
>
> So with coral the mtrrs are never written?

Yes, it seems this place is the culprit. The comment says:

         * Note: if there is an SPL, then it has already set up MTRRs so we
         *      don't need to do that here

So on Coral, the assumption of SPL programming MTRRs is wrong.

Maybe we should do:

        bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT);

do_mtrr &= (!IS_ENABLED(CONFIG_SPL) || IS_ENABLED(CONFIG_FSP_VERSION2)) &&
        !IS_ENABLED(CONFIG_FSP_VERSION1) &&
        !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);

Will this work?

Regards,
Bin

Hi Bin,

On Fri, 28 Jul 2023 at 03:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.
> >
> > Right, but the thing is, your first patch does not revert the
> > behaviour of mtrr_add_request(). It is still just adding to the end.
> >
> > i.e. mtrr_commt() adds new ones but does not overwrite those at the back.
> >
> > Looking at your full series, this is what I see on coral:
> >
> > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > 2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> >
> > But I see that do_mtrr is wrong for coral in init_cache_f_r():
> >
> > do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&
> >
> > So with coral the mtrrs are never written?
>
> Yes, it seems this place is the culprit. The comment says:
>
>          * Note: if there is an SPL, then it has already set up MTRRs so we
>          *      don't need to do that here
>
> So on Coral, the assumption of SPL programming MTRRs is wrong.
>
> Maybe we should do:
>
>         bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT);
>
> do_mtrr &= (!IS_ENABLED(CONFIG_SPL) || IS_ENABLED(CONFIG_FSP_VERSION2)) &&
>         !IS_ENABLED(CONFIG_FSP_VERSION1) &&
>         !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
>
> Will this work?

Unfortunately not. In fact I don't think we need to change this function.

For coral the sequence is:
SPL manually messes with MTRRs to add two MTRRs for SPI flash
SPL jumps to U-Boot proper
now we start the global_data with 0 MTRR requests
fsp_init_banksize() runs and adds two MTRR requests (uncommitted)
init_cache_f_r() runs, but does not call mtrr_commit()
fsp_graphics_probe() adds one MTRR request, making 5 in total
fsp_graphics_probe() calls mtrr_commit()

So I think if we adjust fsp_graphics to either add a request and
commit (for FSP2) or just add next var (for other things) we might be
OK

Here is a run on coral with my modifications at
https://github.com/sjg20/u-boot/tree/for-bin

U-Boot 2023.10-rc1-00014-g37d6c899d4b (Jul 28 2023 - 09:58:37 -0600)

CPU:   Intel(R) Celeron(R) CPU N3450 @ 1.10GHz
DRAM:  dram
- add req
- add req
dram done
4 GiB (effective 3.9 GiB)
do_mtrr 0
Core:  67 devices, 35 uclasses, devicetree: separate
MMC:   sdmmc@1b,0: 1, emmc@1c,0: 0
Loading Environment from nowhere... OK
Video: graphics
Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
2   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
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
- add req
commit, do_caches=1
Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
0   Y     Back         0000000000000000 0000007f80000000 0000000080000000
1   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
2   Y     Back         0000000100000000 0000007f80000000 0000000080000000
3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
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
graphics done
1024x768x32 @ b0000000
Model: Google Coral
Net:         eth_initialize() No ethernet found.
SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
Hit any key to stop autoboot:  0
=> mtrr
CPU 0:
Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
0   Y     Back         0000000000000000 0000007f80000000 0000000080000000
1   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
2   Y     Back         0000000100000000 0000007f80000000 0000000080000000
3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
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
=>

Regards,
Simon

Hi Simon,

On Sat, Jul 29, 2023 at 12:03 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Fri, 28 Jul 2023 at 03:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Bin,
> > > > > > >
> > > > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.
> > >
> > > Right, but the thing is, your first patch does not revert the
> > > behaviour of mtrr_add_request(). It is still just adding to the end.
> > >
> > > i.e. mtrr_commt() adds new ones but does not overwrite those at the back.
> > >
> > > Looking at your full series, this is what I see on coral:
> > >
> > > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > > 2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > >
> > > But I see that do_mtrr is wrong for coral in init_cache_f_r():
> > >
> > > do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&
> > >
> > > So with coral the mtrrs are never written?
> >
> > Yes, it seems this place is the culprit. The comment says:
> >
> >          * Note: if there is an SPL, then it has already set up MTRRs so we
> >          *      don't need to do that here
> >
> > So on Coral, the assumption of SPL programming MTRRs is wrong.
> >
> > Maybe we should do:
> >
> >         bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT);
> >
> > do_mtrr &= (!IS_ENABLED(CONFIG_SPL) || IS_ENABLED(CONFIG_FSP_VERSION2)) &&
> >         !IS_ENABLED(CONFIG_FSP_VERSION1) &&
> >         !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
> >
> > Will this work?
>
> Unfortunately not. In fact I don't think we need to change this function.
>
> For coral the sequence is:
> SPL manually messes with MTRRs to add two MTRRs for SPI flash
> SPL jumps to U-Boot proper
> now we start the global_data with 0 MTRR requests
> fsp_init_banksize() runs and adds two MTRR requests (uncommitted)
> init_cache_f_r() runs, but does not call mtrr_commit()

But with my proposed change, mtrr_commit() should be called here. Why
does it not work?

> fsp_graphics_probe() adds one MTRR request, making 5 in total
> fsp_graphics_probe() calls mtrr_commit()
>
> So I think if we adjust fsp_graphics to either add a request and
> commit (for FSP2) or just add next var (for other things) we might be
> OK
>
> Here is a run on coral with my modifications at
> https://github.com/sjg20/u-boot/tree/for-bin
>
> U-Boot 2023.10-rc1-00014-g37d6c899d4b (Jul 28 2023 - 09:58:37 -0600)
>
> CPU:   Intel(R) Celeron(R) CPU N3450 @ 1.10GHz
> DRAM:  dram
> - add req
> - add req
> dram done
> 4 GiB (effective 3.9 GiB)
> do_mtrr 0
> Core:  67 devices, 35 uclasses, devicetree: separate
> MMC:   sdmmc@1b,0: 1, emmc@1c,0: 0
> Loading Environment from nowhere... OK
> Video: graphics
> Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> 2   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> 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
> - add req
> commit, do_caches=1
> Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> 0   Y     Back         0000000000000000 0000007f80000000 0000000080000000
> 1   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> 2   Y     Back         0000000100000000 0000007f80000000 0000000080000000
> 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> 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
> graphics done
> 1024x768x32 @ b0000000
> Model: Google Coral
> Net:         eth_initialize() No ethernet found.
> SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> Hit any key to stop autoboot:  0
> => mtrr
> CPU 0:
> Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> 0   Y     Back         0000000000000000 0000007f80000000 0000000080000000
> 1   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> 2   Y     Back         0000000100000000 0000007f80000000 0000000080000000
> 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> 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
> =>

Regards,
Bin

Hi Bin,

On Fri, 28 Jul 2023 at 10:44, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sat, Jul 29, 2023 at 12:03 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Fri, 28 Jul 2023 at 03:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Bin,
> > > > > > > >
> > > > > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.
> > > >
> > > > Right, but the thing is, your first patch does not revert the
> > > > behaviour of mtrr_add_request(). It is still just adding to the end.
> > > >
> > > > i.e. mtrr_commt() adds new ones but does not overwrite those at the back.
> > > >
> > > > Looking at your full series, this is what I see on coral:
> > > >
> > > > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > > > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > > > 2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > >
> > > > But I see that do_mtrr is wrong for coral in init_cache_f_r():
> > > >
> > > > do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&
> > > >
> > > > So with coral the mtrrs are never written?
> > >
> > > Yes, it seems this place is the culprit. The comment says:
> > >
> > >          * Note: if there is an SPL, then it has already set up MTRRs so we
> > >          *      don't need to do that here
> > >
> > > So on Coral, the assumption of SPL programming MTRRs is wrong.
> > >
> > > Maybe we should do:
> > >
> > >         bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT);
> > >
> > > do_mtrr &= (!IS_ENABLED(CONFIG_SPL) || IS_ENABLED(CONFIG_FSP_VERSION2)) &&
> > >         !IS_ENABLED(CONFIG_FSP_VERSION1) &&
> > >         !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
> > >
> > > Will this work?
> >
> > Unfortunately not. In fact I don't think we need to change this function.
> >
> > For coral the sequence is:
> > SPL manually messes with MTRRs to add two MTRRs for SPI flash
> > SPL jumps to U-Boot proper
> > now we start the global_data with 0 MTRR requests
> > fsp_init_banksize() runs and adds two MTRR requests (uncommitted)
> > init_cache_f_r() runs, but does not call mtrr_commit()
>
> But with my proposed change, mtrr_commit() should be called here. Why
> does it not work?

Firstly it is still running from SPI flash, so the commit makes
everything run very slow from this point, since it drops those two
MTRRs. So we don't want to commit the MTRRs yet.

But yes it does work, in that we end up with three MTRRs (two DRAM and
one graphics). It's just very slow getting to that point. That's why I
think we should stick with fsp_graphics_probe() doing the commit, for
FSPv2.

>
> > fsp_graphics_probe() adds one MTRR request, making 5 in total
> > fsp_graphics_probe() calls mtrr_commit()
> >
> > So I think if we adjust fsp_graphics to either add a request and
> > commit (for FSP2) or just add next var (for other things) we might be
> > OK
> >
> > Here is a run on coral with my modifications at
> > https://github.com/sjg20/u-boot/tree/for-bin
> >
> > U-Boot 2023.10-rc1-00014-g37d6c899d4b (Jul 28 2023 - 09:58:37 -0600)
> >
> > CPU:   Intel(R) Celeron(R) CPU N3450 @ 1.10GHz
> > DRAM:  dram
> > - add req
> > - add req
> > dram done
> > 4 GiB (effective 3.9 GiB)
> > do_mtrr 0
> > Core:  67 devices, 35 uclasses, devicetree: separate
> > MMC:   sdmmc@1b,0: 1, emmc@1c,0: 0
> > Loading Environment from nowhere... OK
> > Video: graphics
> > Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > 2   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > 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
> > - add req
> > commit, do_caches=1
> > Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> > 0   Y     Back         0000000000000000 0000007f80000000 0000000080000000
> > 1   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > 2   Y     Back         0000000100000000 0000007f80000000 0000000080000000
> > 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > 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
> > graphics done
> > 1024x768x32 @ b0000000
> > Model: Google Coral
> > Net:         eth_initialize() No ethernet found.
> > SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> > Hit any key to stop autoboot:  0
> > => mtrr
> > CPU 0:
> > Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> > 0   Y     Back         0000000000000000 0000007f80000000 0000000080000000
> > 1   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > 2   Y     Back         0000000100000000 0000007f80000000 0000000080000000
> > 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > 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
> > =>
>

Regards,
Simon

Hi Simon,

On Sat, Jul 29, 2023 at 1:11 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Fri, 28 Jul 2023 at 10:44, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sat, Jul 29, 2023 at 12:03 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Fri, 28 Jul 2023 at 03:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Bin,
> > > > > > >
> > > > > > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Bin,
> > > > > > > > >
> > > > > > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.
> > > > >
> > > > > Right, but the thing is, your first patch does not revert the
> > > > > behaviour of mtrr_add_request(). It is still just adding to the end.
> > > > >
> > > > > i.e. mtrr_commt() adds new ones but does not overwrite those at the back.
> > > > >
> > > > > Looking at your full series, this is what I see on coral:
> > > > >
> > > > > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > > > > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > > > > 2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > > >
> > > > > But I see that do_mtrr is wrong for coral in init_cache_f_r():
> > > > >
> > > > > do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&
> > > > >
> > > > > So with coral the mtrrs are never written?
> > > >
> > > > Yes, it seems this place is the culprit. The comment says:
> > > >
> > > >          * Note: if there is an SPL, then it has already set up MTRRs so we
> > > >          *      don't need to do that here
> > > >
> > > > So on Coral, the assumption of SPL programming MTRRs is wrong.
> > > >
> > > > Maybe we should do:
> > > >
> > > >         bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT);
> > > >
> > > > do_mtrr &= (!IS_ENABLED(CONFIG_SPL) || IS_ENABLED(CONFIG_FSP_VERSION2)) &&
> > > >         !IS_ENABLED(CONFIG_FSP_VERSION1) &&
> > > >         !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
> > > >
> > > > Will this work?
> > >
> > > Unfortunately not. In fact I don't think we need to change this function.
> > >
> > > For coral the sequence is:
> > > SPL manually messes with MTRRs to add two MTRRs for SPI flash
> > > SPL jumps to U-Boot proper
> > > now we start the global_data with 0 MTRR requests
> > > fsp_init_banksize() runs and adds two MTRR requests (uncommitted)
> > > init_cache_f_r() runs, but does not call mtrr_commit()
> >
> > But with my proposed change, mtrr_commit() should be called here. Why
> > does it not work?
>
> Firstly it is still running from SPI flash, so the commit makes
> everything run very slow from this point, since it drops those two
> MTRRs. So we don't want to commit the MTRRs yet.
>
> But yes it does work, in that we end up with three MTRRs (two DRAM and
> one graphics). It's just very slow getting to that point. That's why I
> think we should stick with fsp_graphics_probe() doing the commit, for
> FSPv2.

It's possible that someone does not include FSP_GRAPHICS on Coral so
you rely on fsp_graphics_probe() doing the commit is not going to
work.

Besides, the logic of doing mtrr commit in fsp_graphics_probe() does
not keep everything in consistency.

This Coral issue, it sounds like we should fix
arch/x86/cpu/apollolake/cpu_spl.c::arch_cpu_init_spl() to call
mtrr_commit() for the cache of SPI flash in the SPL phase.

>
> >
> > > fsp_graphics_probe() adds one MTRR request, making 5 in total
> > > fsp_graphics_probe() calls mtrr_commit()
> > >
> > > So I think if we adjust fsp_graphics to either add a request and
> > > commit (for FSP2) or just add next var (for other things) we might be
> > > OK
> > >
> > > Here is a run on coral with my modifications at
> > > https://github.com/sjg20/u-boot/tree/for-bin
> > >
> > > U-Boot 2023.10-rc1-00014-g37d6c899d4b (Jul 28 2023 - 09:58:37 -0600)
> > >
> > > CPU:   Intel(R) Celeron(R) CPU N3450 @ 1.10GHz
> > > DRAM:  dram
> > > - add req
> > > - add req
> > > dram done
> > > 4 GiB (effective 3.9 GiB)
> > > do_mtrr 0
> > > Core:  67 devices, 35 uclasses, devicetree: separate
> > > MMC:   sdmmc@1b,0: 1, emmc@1c,0: 0
> > > Loading Environment from nowhere... OK
> > > Video: graphics
> > > Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> > > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > > 2   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 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
> > > - add req
> > > commit, do_caches=1
> > > Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> > > 0   Y     Back         0000000000000000 0000007f80000000 0000000080000000
> > > 1   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > 2   Y     Back         0000000100000000 0000007f80000000 0000000080000000
> > > 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 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
> > > graphics done
> > > 1024x768x32 @ b0000000
> > > Model: Google Coral
> > > Net:         eth_initialize() No ethernet found.
> > > SF: Detected w25q128fw with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> > > Hit any key to stop autoboot:  0
> > > => mtrr
> > > CPU 0:
> > > Reg Valid Write-type   Base   ||        Mask   ||        Size   ||
> > > 0   Y     Back         0000000000000000 0000007f80000000 0000000080000000
> > > 1   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > 2   Y     Back         0000000100000000 0000007f80000000 0000000080000000
> > > 3   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 4   N     Uncacheable  0000000000000000 0000000000000000 0000008000000000
> > > 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
> > > =>
> >
>
> Regards,
> Simon

Hi Simon,

On Mon, Jul 31, 2023 at 7:01 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sat, Jul 29, 2023 at 1:11 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Fri, 28 Jul 2023 at 10:44, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sat, Jul 29, 2023 at 12:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Fri, 28 Jul 2023 at 03:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Bin,
> > > > > > > >
> > > > > > > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Bin,
> > > > > > > > > >
> > > > > > > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.
> > > > > >
> > > > > > Right, but the thing is, your first patch does not revert the
> > > > > > behaviour of mtrr_add_request(). It is still just adding to the end.
> > > > > >
> > > > > > i.e. mtrr_commt() adds new ones but does not overwrite those at the back.
> > > > > >
> > > > > > Looking at your full series, this is what I see on coral:
> > > > > >
> > > > > > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > > > > > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > > > > > 2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > > > >
> > > > > > But I see that do_mtrr is wrong for coral in init_cache_f_r():
> > > > > >
> > > > > > do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&
> > > > > >
> > > > > > So with coral the mtrrs are never written?
> > > > >
> > > > > Yes, it seems this place is the culprit. The comment says:
> > > > >
> > > > >          * Note: if there is an SPL, then it has already set up MTRRs so we
> > > > >          *      don't need to do that here
> > > > >
> > > > > So on Coral, the assumption of SPL programming MTRRs is wrong.
> > > > >
> > > > > Maybe we should do:
> > > > >
> > > > >         bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT);
> > > > >
> > > > > do_mtrr &= (!IS_ENABLED(CONFIG_SPL) || IS_ENABLED(CONFIG_FSP_VERSION2)) &&
> > > > >         !IS_ENABLED(CONFIG_FSP_VERSION1) &&
> > > > >         !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
> > > > >
> > > > > Will this work?
> > > >
> > > > Unfortunately not. In fact I don't think we need to change this function.
> > > >
> > > > For coral the sequence is:
> > > > SPL manually messes with MTRRs to add two MTRRs for SPI flash
> > > > SPL jumps to U-Boot proper
> > > > now we start the global_data with 0 MTRR requests
> > > > fsp_init_banksize() runs and adds two MTRR requests (uncommitted)
> > > > init_cache_f_r() runs, but does not call mtrr_commit()
> > >
> > > But with my proposed change, mtrr_commit() should be called here. Why
> > > does it not work?
> >
> > Firstly it is still running from SPI flash, so the commit makes
> > everything run very slow from this point, since it drops those two
> > MTRRs. So we don't want to commit the MTRRs yet.
> >
> > But yes it does work, in that we end up with three MTRRs (two DRAM and
> > one graphics). It's just very slow getting to that point. That's why I
> > think we should stick with fsp_graphics_probe() doing the commit, for
> > FSPv2.
>
> It's possible that someone does not include FSP_GRAPHICS on Coral so
> you rely on fsp_graphics_probe() doing the commit is not going to
> work.
>
> Besides, the logic of doing mtrr commit in fsp_graphics_probe() does
> not keep everything in consistency.
>
> This Coral issue, it sounds like we should fix
> arch/x86/cpu/apollolake/cpu_spl.c::arch_cpu_init_spl() to call
> mtrr_commit() for the cache of SPI flash in the SPL phase.
>

Another possible place to insert the mtrr_commit() is ich_spi_probe().

Currently it programmed the MTRR for TPL, but not for SPL.

I suspect the TPL phase is duplicate since it is already programmed in
the arch/x86/cpu/apollolake/cpu_spl.c::arch_cpu_init_tpl().

Regards,
Bin

Hi Bin,

On Sun, 30 Jul 2023 at 17:18, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Jul 31, 2023 at 7:01 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sat, Jul 29, 2023 at 1:11 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Fri, 28 Jul 2023 at 10:44, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sat, Jul 29, 2023 at 12:03 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Fri, 28 Jul 2023 at 03:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Bin,
> > > > > > >
> > > > > > > On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Bin,
> > > > > > > > >
> > > > > > > > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Simon,
> > > > > > > > > >
> > > > > > > > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Bin,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn@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@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.
> > > > > > >
> > > > > > > Right, but the thing is, your first patch does not revert the
> > > > > > > behaviour of mtrr_add_request(). It is still just adding to the end.
> > > > > > >
> > > > > > > i.e. mtrr_commt() adds new ones but does not overwrite those at the back.
> > > > > > >
> > > > > > > Looking at your full series, this is what I see on coral:
> > > > > > >
> > > > > > > 0   Y     Back         00000000fef00000 0000007ffff80000 0000000000080000
> > > > > > > 1   Y     Back         00000000fef80000 0000007ffffc0000 0000000000040000
> > > > > > > 2   Y     Combine      00000000b0000000 0000007ff0000000 0000000010000000
> > > > > > >
> > > > > > > But I see that do_mtrr is wrong for coral in init_cache_f_r():
> > > > > > >
> > > > > > > do_mtrr &= !IS_ENABLED(CONFIG_SPL) &&
> > > > > > >
> > > > > > > So with coral the mtrrs are never written?
> > > > > >
> > > > > > Yes, it seems this place is the culprit. The comment says:
> > > > > >
> > > > > >          * Note: if there is an SPL, then it has already set up MTRRs so we
> > > > > >          *      don't need to do that here
> > > > > >
> > > > > > So on Coral, the assumption of SPL programming MTRRs is wrong.
> > > > > >
> > > > > > Maybe we should do:
> > > > > >
> > > > > >         bool do_mtrr = CONFIG_IS_ENABLED(X86_32BIT_INIT);
> > > > > >
> > > > > > do_mtrr &= (!IS_ENABLED(CONFIG_SPL) || IS_ENABLED(CONFIG_FSP_VERSION2)) &&
> > > > > >         !IS_ENABLED(CONFIG_FSP_VERSION1) &&
> > > > > >         !IS_ENABLED(CONFIG_SYS_SLIMBOOTLOADER);
> > > > > >
> > > > > > Will this work?
> > > > >
> > > > > Unfortunately not. In fact I don't think we need to change this function.
> > > > >
> > > > > For coral the sequence is:
> > > > > SPL manually messes with MTRRs to add two MTRRs for SPI flash
> > > > > SPL jumps to U-Boot proper
> > > > > now we start the global_data with 0 MTRR requests
> > > > > fsp_init_banksize() runs and adds two MTRR requests (uncommitted)
> > > > > init_cache_f_r() runs, but does not call mtrr_commit()
> > > >
> > > > But with my proposed change, mtrr_commit() should be called here. Why
> > > > does it not work?
> > >
> > > Firstly it is still running from SPI flash, so the commit makes
> > > everything run very slow from this point, since it drops those two
> > > MTRRs. So we don't want to commit the MTRRs yet.
> > >
> > > But yes it does work, in that we end up with three MTRRs (two DRAM and
> > > one graphics). It's just very slow getting to that point. That's why I
> > > think we should stick with fsp_graphics_probe() doing the commit, for
> > > FSPv2.
> >
> > It's possible that someone does not include FSP_GRAPHICS on Coral so
> > you rely on fsp_graphics_probe() doing the commit is not going to
> > work.
> >
> > Besides, the logic of doing mtrr commit in fsp_graphics_probe() does
> > not keep everything in consistency.
> >
> > This Coral issue, it sounds like we should fix
> > arch/x86/cpu/apollolake/cpu_spl.c::arch_cpu_init_spl() to call
> > mtrr_commit() for the cache of SPI flash in the SPL phase.
> >
>
> Another possible place to insert the mtrr_commit() is ich_spi_probe().
>
> Currently it programmed the MTRR for TPL, but not for SPL.
>
> I suspect the TPL phase is duplicate since it is already programmed in
> the arch/x86/cpu/apollolake/cpu_spl.c::arch_cpu_init_tpl().

Yes there is probably some other stuff going on in SPL/TPL.

But with your v2 series I found that if we change the condition at the
top of init_cache_f_r() it works. I will send the updated patch.

Regards,
Simon