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

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