Hi Simon, On Mon, Jul 31, 2023 at 7:01 AM Bin Meng <bmeng...@gmail.com> wrote: > > Hi Simon, > > On Sat, Jul 29, 2023 at 1:11 AM Simon Glass <s...@chromium.org> wrote: > > > > Hi Bin, > > > > On Fri, 28 Jul 2023 at 10:44, Bin Meng <bmeng...@gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Sat, Jul 29, 2023 at 12:03 AM Simon Glass <s...@chromium.org> wrote: > > > > > > > > Hi Bin, > > > > > > > > On Fri, 28 Jul 2023 at 03:38, Bin Meng <bmeng...@gmail.com> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Thu, Jul 27, 2023 at 8:55 AM Simon Glass <s...@chromium.org> wrote: > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > On Tue, 25 Jul 2023 at 07:43, Bin Meng <bmeng...@gmail.com> wrote: > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > On Mon, Jul 24, 2023 at 6:14 AM Simon Glass <s...@chromium.org> > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > On Sun, 23 Jul 2023 at 09:50, Bin Meng <bmeng...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > On Sun, Jul 23, 2023 at 11:43 AM Simon Glass > > > > > > > > > <s...@chromium.org> wrote: > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > On Fri, 21 Jul 2023 at 10:12, Bin Meng <bmeng...@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...@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