Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Rizwan Qureshi, Arthur Heymans, Patrick Rudolph, EricR Lai. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55653 )
Change subject: soc/intel/common: Add cpu_fill_code_cache() to test eNEM ......................................................................
Patch Set 3:
(11 comments)
File src/cpu/intel/common/Kconfig:
https://review.coreboot.org/c/coreboot/+/55653/comment/110e1c30_323269cd PS2, Line 46: Cache
lowercase.
Ack
https://review.coreboot.org/c/coreboot/+/55653/comment/133e45ad_57f4c538 PS2, Line 48: Also, wish to run the stress test on the CAR region by filling : up the cache by a memory copy.
I'd split that functionality off? It sounds useful to print cache information regardless of CAR type […]
Ack
File src/cpu/intel/common/common_init.c:
https://review.coreboot.org/c/coreboot/+/55653/comment/e444c9fd_67812364 PS2, Line 347: fill_code_cache
Return if not ENV_CACHE_AS_RAM?
Ack
https://review.coreboot.org/c/coreboot/+/55653/comment/2535e55e_6fdf653f PS2, Line 357: fast_spi_get_bios_region
Maybe also check if the RO flash is also covered by an MTRR? Otherwise this whole operation is a noo […]
Ack
https://review.coreboot.org/c/coreboot/+/55653/comment/ee90cb66_985d0911 PS2, Line 359: : * Don't need to run this stress test if available cache size is higher than : * BIOS region size
explain why, as it's not obvious to the reader I think. […]
Ack
https://review.coreboot.org/c/coreboot/+/55653/comment/29b90c93_4ee82918 PS2, Line 363: get_cache_info() >= size
You have to account for the cache used for cache as ram too. […]
Ack
https://review.coreboot.org/c/coreboot/+/55653/comment/37a0310c_4831e880 PS2, Line 369: 4ULL * GiB - 64 * KiB
I guess you are trying to avoid the bootblock here, while you're calling this inside the bootblo […]
Ack
https://review.coreboot.org/c/coreboot/+/55653/comment/8e165264_a4f5378f PS2, Line 370: 1 * KiB
sizeof(buf)
Ack
https://review.coreboot.org/c/coreboot/+/55653/comment/4a9a0515_e44cd1e9 PS2, Line 371: "TEST : %lx %x\n", addr, buf[0]
Is printing this even useful? It does not really tell you that CAR passed or failed a stress test.
Its taking some time for memcpy hence added a printf to make sure folks understood tat something is going on and if that stops so consider as hang.
Now i have modified this a small animated circle, hopefully that is more meaning here than redundant text.
https://review.coreboot.org/c/coreboot/+/55653/comment/d5687760_8e96763d PS2, Line 371: BIOS_INFO
BIOS_SPEW.
Ack
https://review.coreboot.org/c/coreboot/+/55653/comment/c6f26f02_78ee5bff PS2, Line 372: 1 * KiB
sizeof(buf)
Ack