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?
Regards, Bin