[PATCH 1/5] x86: fsp: Use mtrr_set_next_var() for graphics memory
Simon Glass
sjg at chromium.org
Fri Jul 28 19:03:09 CEST 2023
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.
>
> > 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