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 hereSo 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