Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44196 )
Change subject: mrc_cache: Update mrc_cache data in romstage ......................................................................
Patch Set 22: Code-Review+1
(5 comments)
Thanks, this looks good to me in general now. Just a couple of nits, and I'm not quite sure why those Kconfigs need to go there but that's more of an x86-internal matter of opinion. I'll leave the +2 to Furquan.
https://review.coreboot.org/c/coreboot/+/44196/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44196/8//COMMIT_MSG@16 PS8, Line 16: Updating spi driver Kconfig to set : BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY=y if both : BOOT_DEVICE_SPI_FLASH_RW_NOMMAP=y and BOOT_DEVICE_SPI_FLASH=y to bring : the appropriate libraries into romstage.
This has been removed and put in the individual cpu Kconfig files.
Why? I think in general it's better to make the thing you normally want be the default and not push the burden on every single platform to select a bunch of boilerplate options.
Does anyone know why this is a separate Kconfig at all? Doesn't it just enable code that would be eliminated by the linker if it isn't used anyway? For those cases I usually don't see why there needs to be a Kconfig... either the platform has code that does RW flash accesses and then it needs to have that code, or it doesn't and the code is automatically eliminated. Why bother the user with that decision if it can be done automatically?
https://review.coreboot.org/c/coreboot/+/44196/22/src/drivers/mrc_cache/mrc_... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/44196/22/src/drivers/mrc_cache/mrc_... PS22, Line 326: BIOS_INFO nit: still not BIOS_ERR... (and actually, on second thought, might be a good idea to start the line with "ERROR: MRC: " or something for visibility, because this is something that can happen when CBFS_CACHE size isn't big enough and we'd want people to notice that problem)
https://review.coreboot.org/c/coreboot/+/44196/22/src/drivers/mrc_cache/mrc_... PS22, Line 442: BIOS_DEBUG Here too, errors like these should really all use BIOS_ERR.
https://review.coreboot.org/c/coreboot/+/44196/22/src/drivers/mrc_cache/mrc_... PS22, Line 593: (void *) nit: unnecessary cast, cbmem_entry_start() already returns (void *)
https://review.coreboot.org/c/coreboot/+/44196/22/src/drivers/mrc_cache/mrc_... PS22, Line 594: nit: bad alignment? (don't use spaces unless you're aligning it to something specific, which I don't think you are here?)