[PATCH 1/5] x86: fsp: Use mtrr_set_next_var() for graphics memory
Bin Meng
bmeng.cn at gmail.com
Mon Jul 31 01:01:57 CEST 2023
Hi Simon,
On Sat, Jul 29, 2023 at 1:11 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Fri, 28 Jul 2023 at 10:44, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sat, Jul 29, 2023 at 12:03 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Fri, 28 Jul 2023 at 03:38, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Bin,
> > > > > > >
> > > > > > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Bin,
> > > > > > > > >
> > > > > > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > At present this uses mtrr_add_request() & mtrr_commit() combination
> > > > > > > > > > to program the MTRR for graphics memory. This usage has two major
> > > > > > > > > > issues as below:
> > > > > > > > > >
> > > > > > > > > > - mtrr_commit() will re-initialize all MTRR registers from index 0,
> > > > > > > > > > using the settings previously added by mtrr_add_request() and saved
> > > > > > > > > > in gd->arch.mtrr_req[], which won't cause any issue but is unnecessary
> > > > > > > > > > - The way such combination works is based on the assumption that U-Boot
> > > > > > > > > > has full control with MTRR programming (e.g.: U-Boot without any blob
> > > > > > > > > > that does all low-level initialization on its own, or using FSP2 which
> > > > > > > > > > does not touch MTRR), but this is not the case with FSP. FSP programs
> > > > > > > > > > some MTRRs during its execution but U-Boot does not have the settings
> > > > > > > > > > saved in gd->arch.mtrr_req[] and when doing mtrr_commit() it will
> > > > > > > > > > corrupt what was already programmed previously.
> > > > > > > > > >
> > > > > > > > > > Correct this to use mtrr_set_next_var() instead.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > arch/x86/lib/fsp/fsp_graphics.c | 3 +--
> > > > > > > > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > Thanks for looking into this. The series works fine on link. I suspect
> > > > > > > > > it will be find on samus too, but I cannot test right now. Sadly
> > > > > > > > > minnowmax is also dead right now but I hope to fix it soon. I don't
> > > > > > > > > expect any problems there.
> > > > > > > > >
> > > > > > > > > However, for coral, this first patch breaks the mtrrs. With master we get:
> > > > > > > > >
> > > > > > > > > => mtrr
> > > > > > > > > CPU 0:
> > > > > > > > > Reg Valid Write-type Base || Mask || Size ||
> > > > > > > > > 0 Y Back 00000000fef00000 0000007ffff80000 0000000000080000
> > > > > > > > > 1 Y Back 00000000fef80000 0000007ffffc0000 0000000000040000
> > > > > > > > > 2 Y Back 0000000000000000 0000007f80000000 0000000080000000
> > > > > > > > > 3 Y Combine 00000000b0000000 0000007ff0000000 0000000010000000
> > > > > > > > > 4 Y Back 0000000100000000 0000007f80000000 0000000080000000
> > > > > > > > > 5 N Uncacheable 0000000000000000 0000000000000000 0000008000000000
> > > > > > > > > 6 N Uncacheable 0000000000000000 0000000000000000 0000008000000000
> > > > > > > > > 7 N Uncacheable 0000000000000000 0000000000000000 0000008000000000
> > > > > > > > > 8 N Uncacheable 0000000000000000 0000000000000000 0000008000000000
> > > > > > > > > 9 N Uncacheable 0000000000000000 0000000000000000 0000008000000000
> > > > > > > > >
> > > > > > > > > with this patch on coral we get:
> > > > > > > > >
> > > > > > > > > => mtrr
> > > > > > > > > CPU 0:
> > > > > > > > > Reg Valid Write-type Base || Mask || Size ||
> > > > > > > > > 0 Y Back 00000000fef00000 0000007ffff80000 0000000000080000
> > > > > > > > > 1 Y Back 00000000fef80000 0000007ffffc0000 0000000000040000
> > > > > > > > > 2 Y Combine 00000000b0000000 0000007ff0000000 0000000010000000
> > > > > > > > > 3 N Uncacheable 0000000000000000 0000000000000000 0000008000000000
> > > > > > > > >
> > > > > > > > > At present coral expects to handle the MTRRs itself, and it seems that
> > > > > > > > > perhaps the APL FSPv2 does not? Do we need a new Kconfig for dealing
> > > > > > > > > with FSPv2 perhaps?
> > > > > > > >
> > > > > > > > I am a little bit confused. The comment in arch/x86/lib/fsp/fsp_dram.c::
> > > > > > > > dram_init_banksize() says:
> > > > > > > >
> > > > > > > > /*
> > > > > > > > * For FSP1, the system memory and reserved memory used by FSP are
> > > > > > > > * already programmed in the MTRR by FSP. Also it is observed that
> > > > > > > > * FSP on Intel Queensbay platform reports the TSEG memory range
> > > > > > > > * that has the same RES_MEM_RESERVED resource type whose address
> > > > > > > > * is programmed by FSP to be near the top of 4 GiB space, which is
> > > > > > > > * not what we want for DRAM.
> > > > > > > > *
> > > > > > > > * However it seems FSP2's behavior is different. We need to add the
> > > > > > > > * DRAM range in MTRR otherwise the boot process goes very slowly,
> > > > > > > > * which was observed on Chromebook Coral with FSP2.
> > > > > > > > */
> > > > > > > >
> > > > > > > > So on Coral with FSP2, U-Boot programs the MTTR by itself.
> > > > > > > >
> > > > > > > > In this dram_init_banksize(), it calls mtrr_add_request() 3 times, 2
> > > > > > > > of which should be what you were seeing as 2 and 4 below:
> > > > > > > >
> > > > > > > > > 2 Y Back 0000000000000000 0000007f80000000 0000000080000000
> > > > > > > > > 3 Y Combine 00000000b0000000 0000007ff0000000 0000000010000000
> > > > > > > > > 4 Y Back 0000000100000000 0000007f80000000 0000000080000000
> > > > > > > >
> > > > > > > > The #3 should be the FSP graphics frame buffer. But I failed to
> > > > > > > > understand how the FSP graphics programs a MTRR register in between
> > > > > > > > the 2 memory regions programmed by dram_init_banksize() on
> > > > > > > > u-boot/master, how could that happen?
> > > > > > >
> > > > > > > Remember that the MTRRs are sorted, so the order or mtrr_add_request()
> > > > > > > calls does not matter.
> > > > > > >
> > > > > >
> > > > > > Still cannot explain.
> > > > > >
> > > > > > 0 Y Back 00000000fef00000 0000007ffff80000 0000000000080000
> > > > > > 1 Y Back 00000000fef80000 0000007ffffc0000 0000000000040000
> > > > > > 2 Y Back 0000000000000000 0000007f80000000 0000000080000000
> > > > > > 3 Y Combine 00000000b0000000 0000007ff0000000 0000000010000000
> > > > > > 4 Y Back 0000000100000000 0000007f80000000 0000000080000000
> > > > > >
> > > > > > After we sort the mtrr memory range, #2 whose base is 0x0 should have
> > > > > > been put to the first entry, then followed by #3 whose base is
> > > > > > 0xb0000000.
> > > > >
> > > > > 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 at 1b,0: 1, emmc at 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
More information about the U-Boot
mailing list