Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32934
Change subject: soc/amd/common: Add errors for invalid AcpiMmio access ......................................................................
soc/amd/common: Add errors for invalid AcpiMmio access
Print an error when accessing a reserved or unexpected AcpiMmio register block. At a minimum, require the soc to indicate in its iomap.h file which register blocks it allows coreboot to use. It may optionally indicate all that are supported/documented or may treat unused blocks as reserved.
This patch attempts to dissuade accessing unsupported blocks without requiring the complexity of structures or reinitializing at the beginning of a new stage.
TEST=inspect grunt build with objdump with supported and unsupported blocks BUG=b:131682806
Change-Id: I2121df108fd3caf07e5588bc3201bcdd8dcaaa00 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/stoneyridge/include/soc/iomap.h 3 files changed, 185 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/32934/1
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c index c8c6988..89edf396 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -16,6 +16,7 @@ #include <types.h> #include <arch/io.h> #include <device/mmio.h> +#include <console/console.h> #include <amdblocks/acpimmio_map.h> #include <amdblocks/acpimmio.h>
@@ -65,37 +66,55 @@ pm_io_write16(reg + sizeof(uint16_t), value & 0xffff); }
+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); +} + /* smbus pci read/write - access registers at 0xfed80000 - currently unused */
/* smi read/write - access registers at 0xfed80200 */
uint8_t smi_read8(uint8_t reg) { + if (!SUPPORTS_ACPIMMIO_SMI_BASE) + print_error(ACPIMMIO_SMI_BASE, reg); return read8((void *)(ACPIMMIO_SMI_BASE + reg)); }
uint16_t smi_read16(uint8_t reg) { + if (!SUPPORTS_ACPIMMIO_SMI_BASE) + print_error(ACPIMMIO_SMI_BASE, reg); return read16((void *)(ACPIMMIO_SMI_BASE + reg)); }
uint32_t smi_read32(uint8_t reg) { + if (!SUPPORTS_ACPIMMIO_SMI_BASE) + print_error(ACPIMMIO_SMI_BASE, reg); return read32((void *)(ACPIMMIO_SMI_BASE + reg)); }
void smi_write8(uint8_t reg, uint8_t value) { + if (!SUPPORTS_ACPIMMIO_SMI_BASE) + print_error(ACPIMMIO_SMI_BASE, reg); write8((void *)(ACPIMMIO_SMI_BASE + reg), value); }
void smi_write16(uint8_t reg, uint16_t value) { + if (!SUPPORTS_ACPIMMIO_SMI_BASE) + print_error(ACPIMMIO_SMI_BASE, reg); write16((void *)(ACPIMMIO_SMI_BASE + reg), value); }
void smi_write32(uint8_t reg, uint32_t value) { + if (!SUPPORTS_ACPIMMIO_SMI_BASE) + print_error(ACPIMMIO_SMI_BASE, reg); write32((void *)(ACPIMMIO_SMI_BASE + reg), value); }
@@ -103,31 +122,43 @@
u8 pm_read8(u8 reg) { + if (!SUPPORTS_ACPIMMIO_PMIO_BASE) + print_error(ACPIMMIO_PMIO_BASE, reg); return read8((void *)(ACPIMMIO_PMIO_BASE + reg)); }
u16 pm_read16(u8 reg) { + if (!SUPPORTS_ACPIMMIO_PMIO_BASE) + print_error(ACPIMMIO_PMIO_BASE, reg); return read16((void *)(ACPIMMIO_PMIO_BASE + reg)); }
u32 pm_read32(u8 reg) { + if (!SUPPORTS_ACPIMMIO_PMIO_BASE) + print_error(ACPIMMIO_PMIO_BASE, reg); return read32((void *)(ACPIMMIO_PMIO_BASE + reg)); }
void pm_write8(u8 reg, u8 value) { + if (!SUPPORTS_ACPIMMIO_PMIO_BASE) + print_error(ACPIMMIO_PMIO_BASE, reg); write8((void *)(ACPIMMIO_PMIO_BASE + reg), value); }
void pm_write16(u8 reg, u16 value) { + if (!SUPPORTS_ACPIMMIO_PMIO_BASE) + print_error(ACPIMMIO_PMIO_BASE, reg); write16((void *)(ACPIMMIO_PMIO_BASE + reg), value); }
void pm_write32(u8 reg, u32 value) { + if (!SUPPORTS_ACPIMMIO_PMIO_BASE) + print_error(ACPIMMIO_PMIO_BASE, reg); write32((void *)(ACPIMMIO_PMIO_BASE + reg), value); }
@@ -137,6 +168,8 @@
uint8_t biosram_read8(uint8_t reg) { + if (!SUPPORTS_ACPIMMIO_BIOSRAM_BASE) + print_error(ACPIMMIO_BIOSRAM_BASE, reg); /* could see 1/2/4x */ return read8((void *)(ACPIMMIO_BIOSRAM_BASE + reg)); }
@@ -153,6 +186,8 @@
void biosram_write8(uint8_t reg, uint8_t value) { + if (!SUPPORTS_ACPIMMIO_BIOSRAM_BASE) + print_error(ACPIMMIO_BIOSRAM_BASE, reg); /* could see 1/2/4x */ write8((void *)(ACPIMMIO_BIOSRAM_BASE + reg), value); }
@@ -178,31 +213,43 @@
u8 acpi_read8(u8 reg) { + if (!SUPPORTS_ACPIMMIO_ACPI_BASE) + print_error(ACPIMMIO_ACPI_BASE, reg); return read8((void *)(ACPIMMIO_ACPI_BASE + reg)); }
u16 acpi_read16(u8 reg) { + if (!SUPPORTS_ACPIMMIO_ACPI_BASE) + print_error(ACPIMMIO_ACPI_BASE, reg); return read16((void *)(ACPIMMIO_ACPI_BASE + reg)); }
u32 acpi_read32(u8 reg) { + if (!SUPPORTS_ACPIMMIO_ACPI_BASE) + print_error(ACPIMMIO_ACPI_BASE, reg); return read32((void *)(ACPIMMIO_ACPI_BASE + reg)); }
void acpi_write8(u8 reg, u8 value) { + if (!SUPPORTS_ACPIMMIO_ACPI_BASE) + print_error(ACPIMMIO_ACPI_BASE, reg); write8((void *)(ACPIMMIO_ACPI_BASE + reg), value); }
void acpi_write16(u8 reg, u16 value) { + if (!SUPPORTS_ACPIMMIO_ACPI_BASE) + print_error(ACPIMMIO_ACPI_BASE, reg); write16((void *)(ACPIMMIO_ACPI_BASE + reg), value); }
void acpi_write32(u8 reg, u32 value) { + if (!SUPPORTS_ACPIMMIO_ACPI_BASE) + print_error(ACPIMMIO_ACPI_BASE, reg); write32((void *)(ACPIMMIO_ACPI_BASE + reg), value); }
@@ -210,21 +257,29 @@
u8 asf_read8(u8 reg) { + if (!SUPPORTS_ACPIMMIO_ASF_BASE) + print_error(ACPIMMIO_ASF_BASE, reg); return read8((void *)(ACPIMMIO_ASF_BASE + reg)); }
u16 asf_read16(u8 reg) { + if (!SUPPORTS_ACPIMMIO_ASF_BASE) + print_error(ACPIMMIO_ASF_BASE, reg); return read16((void *)(ACPIMMIO_ASF_BASE + reg)); }
void asf_write8(u8 reg, u8 value) { + if (!SUPPORTS_ACPIMMIO_ASF_BASE) + print_error(ACPIMMIO_ASF_BASE, reg); write8((void *)(ACPIMMIO_ASF_BASE + reg), value); }
void asf_write16(u8 reg, u16 value) { + if (!SUPPORTS_ACPIMMIO_ASF_BASE) + print_error(ACPIMMIO_ASF_BASE, reg); write16((void *)(ACPIMMIO_ASF_BASE + reg), value); }
@@ -232,21 +287,29 @@
u8 smbus_read8(u8 reg) { + if (!SUPPORTS_ACPIMMIO_SMBUS_BASE) + print_error(ACPIMMIO_SMBUS_BASE, reg); return read8((void *)(ACPIMMIO_SMBUS_BASE + reg)); }
u16 smbus_read16(u8 reg) { + if (!SUPPORTS_ACPIMMIO_SMBUS_BASE) + print_error(ACPIMMIO_SMBUS_BASE, reg); return read16((void *)(ACPIMMIO_SMBUS_BASE + reg)); }
void smbus_write8(u8 reg, u8 value) { + if (!SUPPORTS_ACPIMMIO_SMBUS_BASE) + print_error(ACPIMMIO_SMBUS_BASE, reg); write8((void *)(ACPIMMIO_SMBUS_BASE + reg), value); }
void smbus_write16(u8 reg, u16 value) { + if (!SUPPORTS_ACPIMMIO_SMBUS_BASE) + print_error(ACPIMMIO_SMBUS_BASE, reg); write16((void *)(ACPIMMIO_SMBUS_BASE + reg), value); }
@@ -258,31 +321,43 @@
u8 iomux_read8(u8 reg) { + if (!SUPPORTS_ACPIMMIO_IOMUX_BASE) + print_error(ACPIMMIO_IOMUX_BASE, reg); return read8((void *)(ACPIMMIO_IOMUX_BASE + reg)); }
u16 iomux_read16(u8 reg) { + if (!SUPPORTS_ACPIMMIO_IOMUX_BASE) + print_error(ACPIMMIO_IOMUX_BASE, reg); return read16((void *)(ACPIMMIO_IOMUX_BASE + reg)); }
u32 iomux_read32(u8 reg) { + if (!SUPPORTS_ACPIMMIO_IOMUX_BASE) + print_error(ACPIMMIO_IOMUX_BASE, reg); return read32((void *)(ACPIMMIO_IOMUX_BASE + reg)); }
void iomux_write8(u8 reg, u8 value) { + if (!SUPPORTS_ACPIMMIO_IOMUX_BASE) + print_error(ACPIMMIO_IOMUX_BASE, reg); write8((void *)(ACPIMMIO_IOMUX_BASE + reg), value); }
void iomux_write16(u8 reg, u16 value) { + if (!SUPPORTS_ACPIMMIO_IOMUX_BASE) + print_error(ACPIMMIO_IOMUX_BASE, reg); write16((void *)(ACPIMMIO_IOMUX_BASE + reg), value); }
void iomux_write32(u8 reg, u32 value) { + if (!SUPPORTS_ACPIMMIO_IOMUX_BASE) + print_error(ACPIMMIO_IOMUX_BASE, reg); write32((void *)(ACPIMMIO_IOMUX_BASE + reg), value); }
@@ -290,31 +365,43 @@
u8 misc_read8(u8 reg) { + if (!SUPPORTS_ACPIMMIO_MISC_BASE) + print_error(ACPIMMIO_MISC_BASE, reg); return read8((void *)(ACPIMMIO_MISC_BASE + reg)); }
u16 misc_read16(u8 reg) { + if (!SUPPORTS_ACPIMMIO_MISC_BASE) + print_error(ACPIMMIO_MISC_BASE, reg); return read16((void *)(ACPIMMIO_MISC_BASE + reg)); }
u32 misc_read32(u8 reg) { + if (!SUPPORTS_ACPIMMIO_MISC_BASE) + print_error(ACPIMMIO_MISC_BASE, reg); return read32((void *)(ACPIMMIO_MISC_BASE + reg)); }
void misc_write8(u8 reg, u8 value) { + if (!SUPPORTS_ACPIMMIO_MISC_BASE) + print_error(ACPIMMIO_MISC_BASE, reg); write8((void *)(ACPIMMIO_MISC_BASE + reg), value); }
void misc_write16(u8 reg, u16 value) { + if (!SUPPORTS_ACPIMMIO_MISC_BASE) + print_error(ACPIMMIO_MISC_BASE, reg); write16((void *)(ACPIMMIO_MISC_BASE + reg), value); }
void misc_write32(u8 reg, u32 value) { + if (!SUPPORTS_ACPIMMIO_MISC_BASE) + print_error(ACPIMMIO_MISC_BASE, reg); write32((void *)(ACPIMMIO_MISC_BASE + reg), value); }
@@ -328,31 +415,43 @@
uint8_t xhci_pm_read8(uint8_t reg) { + if (!SUPPORTS_ACPIMMIO_XHCIPM_BASE) + print_error(ACPIMMIO_XHCIPM_BASE, reg); return read8((void *)(ACPIMMIO_XHCIPM_BASE + reg)); }
uint16_t xhci_pm_read16(uint8_t reg) { + if (!SUPPORTS_ACPIMMIO_XHCIPM_BASE) + print_error(ACPIMMIO_XHCIPM_BASE, reg); return read16((void *)(ACPIMMIO_XHCIPM_BASE + reg)); }
uint32_t xhci_pm_read32(uint8_t reg) { + if (!SUPPORTS_ACPIMMIO_XHCIPM_BASE) + print_error(ACPIMMIO_XHCIPM_BASE, reg); return read32((void *)(ACPIMMIO_XHCIPM_BASE + reg)); }
void xhci_pm_write8(uint8_t reg, uint8_t value) { + if (!SUPPORTS_ACPIMMIO_XHCIPM_BASE) + print_error(ACPIMMIO_XHCIPM_BASE, reg); write8((void *)(ACPIMMIO_XHCIPM_BASE + reg), value); }
void xhci_pm_write16(uint8_t reg, uint16_t value) { + if (!SUPPORTS_ACPIMMIO_XHCIPM_BASE) + print_error(ACPIMMIO_XHCIPM_BASE, reg); write16((void *)(ACPIMMIO_XHCIPM_BASE + reg), value); }
void xhci_pm_write32(uint8_t reg, uint32_t value) { + if (!SUPPORTS_ACPIMMIO_XHCIPM_BASE) + print_error(ACPIMMIO_XHCIPM_BASE, reg); write32((void *)(ACPIMMIO_XHCIPM_BASE + reg), value); }
@@ -362,10 +461,14 @@
u8 aoac_read8(u8 reg) { + if (!SUPPORTS_ACPIMMIO_AOAC_BASE) + print_error(ACPIMMIO_AOAC_BASE, reg); return read8((void *)(ACPIMMIO_AOAC_BASE + reg)); }
void aoac_write8(u8 reg, u8 value) { + if (!SUPPORTS_ACPIMMIO_AOAC_BASE) + print_error(ACPIMMIO_AOAC_BASE, reg); write8((void *)(ACPIMMIO_AOAC_BASE + reg), value); } diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index 6be856b..32da867 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -18,6 +18,69 @@ #ifndef __AMDBLOCKS_ACPIMMIO_H__ #define __AMDBLOCKS_ACPIMMIO_H__
+/* iomap.h must indicate if the device uses a block, optional if unused. */ +#include <soc/iomap.h> +#ifndef SUPPORTS_ACPIMMIO_SMI_BASE + #define SUPPORTS_ACPIMMIO_SMI_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_PMIO_BASE + #define SUPPORTS_ACPIMMIO_PMIO_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_PMIO2_BASE + #define SUPPORTS_ACPIMMIO_PMIO2_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_BIOSRAM_BASE + #define SUPPORTS_ACPIMMIO_BIOSRAM_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_CMOSRAM_BASE + #define SUPPORTS_ACPIMMIO_CMOSRAM_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_CMOS_BASE + #define SUPPORTS_ACPIMMIO_CMOS_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_ACPI_BASE + #define SUPPORTS_ACPIMMIO_ACPI_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_ASF_BASE + #define SUPPORTS_ACPIMMIO_ASF_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_SMBUS_BASE + #define SUPPORTS_ACPIMMIO_SMBUS_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_WDT_BASE + #define SUPPORTS_ACPIMMIO_WDT_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_HPET_BASE + #define SUPPORTS_ACPIMMIO_HPET_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_IOMUX_BASE + #define SUPPORTS_ACPIMMIO_IOMUX_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_MISC_BASE + #define SUPPORTS_ACPIMMIO_MISC_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_DPVGA_BASE + #define SUPPORTS_ACPIMMIO_DPVGA_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_GPIO0_BASE + #define SUPPORTS_ACPIMMIO_GPIO0_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_GPIO1_BASE + #define SUPPORTS_ACPIMMIO_GPIO1_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_GPIO2_BASE + #define SUPPORTS_ACPIMMIO_GPIO2_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_XHCIPM_BASE + #define SUPPORTS_ACPIMMIO_XHCIPM_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_ACDCTMR_BASE + #define SUPPORTS_ACPIMMIO_ACDCTMR_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_AOAC_BASE + #define SUPPORTS_ACPIMMIO_AOAC_BASE 0 +#endif + /* * The following AcpiMmio register block mapping represents definitions * that have been documented in AMD publications. All blocks aren't diff --git a/src/soc/amd/stoneyridge/include/soc/iomap.h b/src/soc/amd/stoneyridge/include/soc/iomap.h index 3856a22..da8906a 100644 --- a/src/soc/amd/stoneyridge/include/soc/iomap.h +++ b/src/soc/amd/stoneyridge/include/soc/iomap.h @@ -22,8 +22,26 @@ #define SPI_BASE_ADDRESS 0xfec10000 #define IO_APIC2_ADDR 0xfec20000
-/* AcpiMmio blocks are at fixed offsets from FED8_0000h, enabled in PMx04[1] */ +/* + * AcpiMmio blocks are at fixed offsets from FED8_0000h, enabled in PMx04[1]. + * All ranges not specified as supported below may, or may not, be listed in + * any documentation but should be considered reserved through FED8_1FFFh. + */ #include <amdblocks/acpimmio_map.h> +#define SUPPORTS_ACPIMMIO_SMI_BASE 1 /* 0xfed80100 */ +#define SUPPORTS_ACPIMMIO_PMIO_BASE 1 /* 0xfed80300 */ +#define SUPPORTS_ACPIMMIO_PMIO2_BASE 1 /* 0xfed80400 */ +#define SUPPORTS_ACPIMMIO_BIOSRAM_BASE 1 /* 0xfed80500 */ +#define SUPPORTS_ACPIMMIO_ACPI_BASE 1 /* 0xfed80800 */ +#define SUPPORTS_ACPIMMIO_ASF_BASE 1 /* 0xfed80900 */ +#define SUPPORTS_ACPIMMIO_SMBUS_BASE 1 /* 0xfed80a00 */ +#define SUPPORTS_ACPIMMIO_IOMUX_BASE 1 /* 0xfed80d00 */ +#define SUPPORTS_ACPIMMIO_MISC_BASE 1 /* 0xfed80e00 */ +#define SUPPORTS_ACPIMMIO_GPIO0_BASE 1 /* 0xfed81500 */ +#define SUPPORTS_ACPIMMIO_GPIO1_BASE 1 /* 0xfed81800 */ +#define SUPPORTS_ACPIMMIO_GPIO2_BASE 1 /* 0xfed81700 */ +#define SUPPORTS_ACPIMMIO_XHCIPM_BASE 1 /* 0xfed81c00 */ +#define SUPPORTS_ACPIMMIO_AOAC_BASE 1 /* 0xfed81e00 */
#define ALINK_AHB_ADDRESS 0xfedc0000
Marshall Dawson 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 1:
Furquan, I've tried to address your concerns about access to unsupported functions (https://review.coreboot.org/c/coreboot/+/32649), but do it as cleanly as possible. I'm not sure I like this solution a whole lot, so I'm open to suggestions.
Adding Kconfig symbols and using #if CONFIG() to remove functions is still an option, however that's ~20 additional symbols (which seems to rub some people the wrong way). I could define custom CONFIG_ values in iomap.h and still use CONFIG() to break the build in mmio_util.c vs. printing an error.
Paul Menzel 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
Richard Spiegel 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+2
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?
Hello Richard Spiegel, Paul Menzel, build bot (Jenkins), Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32934
to look at the new patch set (#4).
Change subject: soc/amd/common: Add errors for invalid AcpiMmio access ......................................................................
soc/amd/common: Add errors for invalid AcpiMmio access
Add a method for the soc/amd/<product> to indicate what AcpiMmio ranges are supported. Induce a build error if soc or mainboard code is added which attempts to use an unsupported block.
This patch attempts to dissuade accessing unsupported blocks without requiring the complexity of structures or reinitializing at the beginning of a new stage.
TEST=boot grunt, force build errors by removing blocks in iomap.h BUG=b:131682806
Change-Id: I2121df108fd3caf07e5588bc3201bcdd8dcaaa00 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/stoneyridge/include/soc/iomap.h 3 files changed, 133 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/32934/4
build bot (Jenkins) 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 4:
(7 comments)
https://review.coreboot.org/#/c/32934/4/src/soc/amd/common/block/acpimmio/mm... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/#/c/32934/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 139: /* pm2 read/write - access registers at 0xfed80400 - currently unused by any soc */ line over 80 characters
https://review.coreboot.org/#/c/32934/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 182: /* cmosram read/write - access registers at 0xfed80600 - currently unused by any soc */ line over 80 characters
https://review.coreboot.org/#/c/32934/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 186: /* cmos read/write - access registers at 0xfed80700 - currently unused by any soc */ line over 80 characters
https://review.coreboot.org/#/c/32934/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 272: /* wdt read/write - access registers at 0xfed80b00 - not currently used by any soc */ line over 80 characters
https://review.coreboot.org/#/c/32934/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 276: /* hpet read/write - access registers at 0xfed80c00 - not currently used by any soc */ line over 80 characters
https://review.coreboot.org/#/c/32934/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 348: /* dpvga read/write - access registers at 0xfed81400 - not currently used by any soc */ line over 80 characters
https://review.coreboot.org/#/c/32934/4/src/soc/amd/common/block/acpimmio/mm... PS4, Line 398: /* acdc_tmr read/write - access registers at 0xfed81d00 - not currently used by any soc */ line over 80 characters
Marshall Dawson 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 4:
(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. […]
Done, no problem.
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?
Was a good idea. N/A now.
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 4: Code-Review+2
Richard Spiegel 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 4: Code-Review+2
I see Martin's point. If you try to build without serial it's even worst. As you said, was a good idea.
Marshall Dawson has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32934 )
Change subject: soc/amd/common: Add errors for invalid AcpiMmio access ......................................................................
soc/amd/common: Add errors for invalid AcpiMmio access
Add a method for the soc/amd/<product> to indicate what AcpiMmio ranges are supported. Induce a build error if soc or mainboard code is added which attempts to use an unsupported block.
This patch attempts to dissuade accessing unsupported blocks without requiring the complexity of structures or reinitializing at the beginning of a new stage.
TEST=boot grunt, force build errors by removing blocks in iomap.h BUG=b:131682806
Change-Id: I2121df108fd3caf07e5588bc3201bcdd8dcaaa00 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32934 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/stoneyridge/include/soc/iomap.h 3 files changed, 133 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Richard Spiegel: Looks good to me, approved
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c index c8c6988..edb3882 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -67,6 +67,7 @@
/* smbus pci read/write - access registers at 0xfed80000 - currently unused */
+#if SUPPORTS_ACPIMMIO_SMI_BASE /* smi read/write - access registers at 0xfed80200 */
uint8_t smi_read8(uint8_t reg) @@ -98,7 +99,9 @@ { write32((void *)(ACPIMMIO_SMI_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_SMI_BASE */
+#if SUPPORTS_ACPIMMIO_PMIO_BASE /* pm read/write - access registers at 0xfed80300 */
u8 pm_read8(u8 reg) @@ -130,9 +133,13 @@ { write32((void *)(ACPIMMIO_PMIO_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_PMIO_BASE */
-/* pm2 read/write - access registers at 0xfed80400 - currently unused */ +#if SUPPORTS_ACPIMMIO_PMIO2_BASE +/* pm2 read/write - access registers at 0xfed80400 - currently unused by any soc */ +#endif
+#if SUPPORTS_ACPIMMIO_BIOSRAM_BASE /* biosram read/write - access registers at 0xfed80500 */
uint8_t biosram_read8(uint8_t reg) @@ -169,11 +176,17 @@ value >>= 16; biosram_write16(reg + sizeof(uint16_t), value & 0xffff); } +#endif /* SUPPORTS_ACPIMMIO_BIOSRAM_BASE */
-/* cmosram read/write - access registers at 0xfed80600 - currently unused */ +#if SUPPORTS_ACPIMMIO_CMOSRAM_BASE +/* cmosram read/write - access registers at 0xfed80600 - currently unused by any soc */ +#endif
-/* cmos read/write - access registers at 0xfed80700 - currently unused */ +#if SUPPORTS_ACPIMMIO_CMOS_BASE +/* cmos read/write - access registers at 0xfed80700 - currently unused by any soc */ +#endif
+#if SUPPORTS_ACPIMMIO_ACPI_BASE /* acpi read/write - access registers at 0xfed80800 */
u8 acpi_read8(u8 reg) @@ -205,7 +218,9 @@ { write32((void *)(ACPIMMIO_ACPI_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_ACPI_BASE */
+#if SUPPORTS_ACPIMMIO_ASF_BASE /* asf read/write - access registers at 0xfed80900 */
u8 asf_read8(u8 reg) @@ -227,7 +242,9 @@ { write16((void *)(ACPIMMIO_ASF_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_ASF_BASE */
+#if SUPPORTS_ACPIMMIO_SMBUS_BASE /* smbus read/write - access registers at 0xfed80a00 */
u8 smbus_read8(u8 reg) @@ -249,11 +266,17 @@ { write16((void *)(ACPIMMIO_SMBUS_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_SMBUS_BASE */
-/* wdt read/write - access registers at 0xfed80b00 - not currently used */ +#if SUPPORTS_ACPIMMIO_WDT_BASE +/* wdt read/write - access registers at 0xfed80b00 - not currently used by any soc */ +#endif
-/* hpet read/write - access registers at 0xfed80c00 - not currently used */ +#if SUPPORTS_ACPIMMIO_HPET_BASE +/* hpet read/write - access registers at 0xfed80c00 - not currently used by any soc */ +#endif
+#if SUPPORTS_ACPIMMIO_IOMUX_BASE /* iomux read/write - access registers at 0xfed80d00 */
u8 iomux_read8(u8 reg) @@ -285,7 +308,9 @@ { write32((void *)(ACPIMMIO_IOMUX_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_IOMUX_BASE */
+#if SUPPORTS_ACPIMMIO_MISC_BASE /* misc read/write - access registers at 0xfed80e00 */
u8 misc_read8(u8 reg) @@ -317,13 +342,25 @@ { write32((void *)(ACPIMMIO_MISC_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_MISC_BASE */
-/* dpvga read/write - access registers at 0xfed81400 - not currently used */ +#if SUPPORTS_ACPIMMIO_DPVGA_BASE +/* dpvga read/write - access registers at 0xfed81400 - not currently used by any soc */ +#endif
-/* gpio bk 0 read/write - access registers at 0xfed81500 - not currently used */ -/* gpio bk 1 read/write - access registers at 0xfed81600 - not currently used */ -/* gpio bk 2 read/write - access registers at 0xfed81700 - not currently used */ +#if SUPPORTS_ACPIMMIO_GPIO0_BASE || SUPPORTS_ACPIMMIO_GPIO1_BASE \ + || SUPPORTS_ACPIMMIO_GPIO2_BASE +/* + * No helpers are currently in use however common/block//gpio.c accesses + * the registers directly. + */
+/* gpio bk 0 read/write - access registers at 0xfed81500 */ +/* gpio bk 1 read/write - access registers at 0xfed81600 */ +/* gpio bk 2 read/write - access registers at 0xfed81700 */ +#endif + +#if SUPPORTS_ACPIMMIO_XHCIPM_BASE /* xhci_pm read/write - access registers at 0xfed81c00 */
uint8_t xhci_pm_read8(uint8_t reg) @@ -355,9 +392,13 @@ { write32((void *)(ACPIMMIO_XHCIPM_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_XHCIPM_BASE */
-/* acdc_tmr read/write - access registers at 0xfed81d00 - not currently used */ +#if SUPPORTS_ACPIMMIO_ACDCTMR_BASE +/* acdc_tmr read/write - access registers at 0xfed81d00 - not currently used by any soc */ +#endif
+#if SUPPORTS_ACPIMMIO_AOAC_BASE /* aoac read/write - access registers at 0xfed81e00 */
u8 aoac_read8(u8 reg) @@ -369,3 +410,4 @@ { write8((void *)(ACPIMMIO_AOAC_BASE + reg), value); } +#endif /* SUPPORTS_ACPIMMIO_AOAC_BASE */ diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index 6be856b..32da867 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -18,6 +18,69 @@ #ifndef __AMDBLOCKS_ACPIMMIO_H__ #define __AMDBLOCKS_ACPIMMIO_H__
+/* iomap.h must indicate if the device uses a block, optional if unused. */ +#include <soc/iomap.h> +#ifndef SUPPORTS_ACPIMMIO_SMI_BASE + #define SUPPORTS_ACPIMMIO_SMI_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_PMIO_BASE + #define SUPPORTS_ACPIMMIO_PMIO_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_PMIO2_BASE + #define SUPPORTS_ACPIMMIO_PMIO2_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_BIOSRAM_BASE + #define SUPPORTS_ACPIMMIO_BIOSRAM_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_CMOSRAM_BASE + #define SUPPORTS_ACPIMMIO_CMOSRAM_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_CMOS_BASE + #define SUPPORTS_ACPIMMIO_CMOS_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_ACPI_BASE + #define SUPPORTS_ACPIMMIO_ACPI_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_ASF_BASE + #define SUPPORTS_ACPIMMIO_ASF_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_SMBUS_BASE + #define SUPPORTS_ACPIMMIO_SMBUS_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_WDT_BASE + #define SUPPORTS_ACPIMMIO_WDT_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_HPET_BASE + #define SUPPORTS_ACPIMMIO_HPET_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_IOMUX_BASE + #define SUPPORTS_ACPIMMIO_IOMUX_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_MISC_BASE + #define SUPPORTS_ACPIMMIO_MISC_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_DPVGA_BASE + #define SUPPORTS_ACPIMMIO_DPVGA_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_GPIO0_BASE + #define SUPPORTS_ACPIMMIO_GPIO0_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_GPIO1_BASE + #define SUPPORTS_ACPIMMIO_GPIO1_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_GPIO2_BASE + #define SUPPORTS_ACPIMMIO_GPIO2_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_XHCIPM_BASE + #define SUPPORTS_ACPIMMIO_XHCIPM_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_ACDCTMR_BASE + #define SUPPORTS_ACPIMMIO_ACDCTMR_BASE 0 +#endif +#ifndef SUPPORTS_ACPIMMIO_AOAC_BASE + #define SUPPORTS_ACPIMMIO_AOAC_BASE 0 +#endif + /* * The following AcpiMmio register block mapping represents definitions * that have been documented in AMD publications. All blocks aren't diff --git a/src/soc/amd/stoneyridge/include/soc/iomap.h b/src/soc/amd/stoneyridge/include/soc/iomap.h index 3856a22..612b6e8 100644 --- a/src/soc/amd/stoneyridge/include/soc/iomap.h +++ b/src/soc/amd/stoneyridge/include/soc/iomap.h @@ -22,8 +22,25 @@ #define SPI_BASE_ADDRESS 0xfec10000 #define IO_APIC2_ADDR 0xfec20000
-/* AcpiMmio blocks are at fixed offsets from FED8_0000h, enabled in PMx04[1] */ +/* + * AcpiMmio blocks are at fixed offsets from FED8_0000h and enabled in PMx04[1]. + * All ranges not specified as supported below may, or may not, be listed in + * any documentation but should be considered reserved through FED8_1FFFh. + */ #include <amdblocks/acpimmio_map.h> +#define SUPPORTS_ACPIMMIO_SMI_BASE 1 /* 0xfed80100 */ +#define SUPPORTS_ACPIMMIO_PMIO_BASE 1 /* 0xfed80300 */ +#define SUPPORTS_ACPIMMIO_BIOSRAM_BASE 1 /* 0xfed80500 */ +#define SUPPORTS_ACPIMMIO_ACPI_BASE 1 /* 0xfed80800 */ +#define SUPPORTS_ACPIMMIO_ASF_BASE 1 /* 0xfed80900 */ +#define SUPPORTS_ACPIMMIO_SMBUS_BASE 1 /* 0xfed80a00 */ +#define SUPPORTS_ACPIMMIO_IOMUX_BASE 1 /* 0xfed80d00 */ +#define SUPPORTS_ACPIMMIO_MISC_BASE 1 /* 0xfed80e00 */ +#define SUPPORTS_ACPIMMIO_GPIO0_BASE 1 /* 0xfed81500 */ +#define SUPPORTS_ACPIMMIO_GPIO1_BASE 1 /* 0xfed81800 */ +#define SUPPORTS_ACPIMMIO_GPIO2_BASE 1 /* 0xfed81700 */ +#define SUPPORTS_ACPIMMIO_XHCIPM_BASE 1 /* 0xfed81c00 */ +#define SUPPORTS_ACPIMMIO_AOAC_BASE 1 /* 0xfed81e00 */
#define ALINK_AHB_ADDRESS 0xfedc0000
-
build bot (Jenkins) (Code Review) -
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Richard Spiegel (Code Review)