Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32934 )
Change subject: soc/amd/common: Add errors for invalid AcpiMmio access ......................................................................
Patch Set 3: Code-Review-1
(2 comments)
https://review.coreboot.org/#/c/32934/3/src/soc/amd/common/block/acpimmio/mm... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/#/c/32934/3/src/soc/amd/common/block/acpimmio/mm... PS3, Line 69: static void print_error(uintptr_t base, uint8_t reg) : { : printk(BIOS_ERR, "Error: attempting to access offset 0x%x of unsupported AcpiMmio at 0x%lx\n", : reg, base); : } : I'm not crazy about this because it relies on somebody seeing the issue in the logfile.
I'd really prefer a build-time error if it's not supported. What about just surrounding the functions with #ifdef SUPPORTS_ACPIMMIO_XXX_BASE? That'd at least give a linker error if something in the code is calling it.
If we can't do that, could we add an assert so we can make the errors fatal?
https://review.coreboot.org/#/c/32934/3/src/soc/amd/common/block/acpimmio/mm... PS3, Line 82: print_error(ACPIMMIO_SMI_BASE, reg); could we pass __FUNCTION__ to the print_error so that it's easy to see what's being called?