Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34914 )
Change subject: soc/amd/common: Add AcpiMmio access for SMBus PCI device ......................................................................
soc/amd/common: Add AcpiMmio access for SMBus PCI device
The PCI register space is accessible at 0xfed80000. Add functions for use as helpers.
Change-Id: Icbf5bdc449322c3f5e59e6126d709cb2808591d5 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 2 files changed, 44 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/34914/1
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c index edb3882..7fad456 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -65,7 +65,39 @@ pm_io_write16(reg + sizeof(uint16_t), value & 0xffff); }
-/* smbus pci read/write - access registers at 0xfed80000 - currently unused */ +#if SUPPORTS_ACPIMMIO_SM_PCI_BASE +/* smbus pci read/write - access registers at 0xfed80000 */ + +u8 sm_pci_read8(u8 reg) +{ + return read8((void *)(ACPIMMIO_SM_PCI_BASE + reg)); +} + +u16 sm_pci_read16(u8 reg) +{ + return read16((void *)(ACPIMMIO_SM_PCI_BASE + reg)); +} + +u32 sm_pci_read32(u8 reg) +{ + return read32((void *)(ACPIMMIO_SM_PCI_BASE + reg)); +} + +void sm_pci_write8(u8 reg, u8 value) +{ + write8((void *)(ACPIMMIO_SM_PCI_BASE + reg), value); +} + +void sm_pci_write16(u8 reg, u16 value) +{ + write16((void *)(ACPIMMIO_SM_PCI_BASE + reg), value); +} + +void sm_pci_write32(u8 reg, u32 value) +{ + write32((void *)(ACPIMMIO_SM_PCI_BASE + reg), value); +} +#endif
#if SUPPORTS_ACPIMMIO_SMI_BASE /* smi read/write - access registers at 0xfed80200 */ diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index 32da867..0913a5a 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -20,6 +20,9 @@
/* iomap.h must indicate if the device uses a block, optional if unused. */ #include <soc/iomap.h> +#ifndef SUPPORTS_ACPIMMIO_SM_PCI_BASE + #define SUPPORTS_ACPIMMIO_SM_PCI_BASE 0 +#endif #ifndef SUPPORTS_ACPIMMIO_SMI_BASE #define SUPPORTS_ACPIMMIO_SMI_BASE 0 #endif @@ -161,6 +164,14 @@ /* Enable the AcpiMmio range at 0xfed80000 */ void enable_acpimmio_decode(void);
+/* Access SMBus PCI registers at 0xfed80000 */ +uint8_t sm_pci_read8(uint8_t reg); +uint16_t sm_pci_read16(uint8_t reg); +uint32_t sm_pci_read32(uint8_t reg); +void sm_pci_write8(uint8_t reg, uint8_t value); +void sm_pci_write16(uint8_t reg, uint16_t value); +void sm_pci_write32(uint8_t reg, uint32_t value); + /* Access PM registers using IO cycles */ uint8_t pm_io_read8(uint8_t reg); uint16_t pm_io_read16(uint8_t reg);
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34914 )
Change subject: soc/amd/common: Add AcpiMmio access for SMBus PCI device ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34914/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34914/4//COMMIT_MSG@9 PS4, Line 9: 0xfed80000 0xfed80a00
https://review.coreboot.org/c/coreboot/+/34914/4/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/34914/4/src/soc/amd/common/block/ac... PS4, Line 69: 0xfed80000 0xfed80a00
https://review.coreboot.org/c/coreboot/+/34914/4/src/soc/amd/common/block/ac... PS4, Line 73: ACPIMMIO_SM_PCI_BASE Why not ACPIMMIO_SMB_PCI_BASE? "SM" could be confused with system management (SMM).
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34914 )
Change subject: soc/amd/common: Add AcpiMmio access for SMBus PCI device ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34914/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34914/5//COMMIT_MSG@7 PS5, Line 7: SMBus I'm starting to believe this commit message is misleading, as there is already SMBus access through MMIO. Also, much more then ACPI is accessed through these 256 MMIO registers... as I mentioned on another patch, USB legacy registers are also accessed. And I believe you mentioned UART can also be accessed. Please change the title.
https://review.coreboot.org/c/coreboot/+/34914/5//COMMIT_MSG@9 PS5, Line 9: PCI Which device/function?
Hello Richard Spiegel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34914
to look at the new patch set (#6).
Change subject: soc/amd/common: Add AcpiMmio access for SMBus PCI device ......................................................................
soc/amd/common: Add AcpiMmio access for SMBus PCI device
The standard PCI register space for D14F0 is accessible at 0xfed80000. Add functions for use as helpers.
Change-Id: Icbf5bdc449322c3f5e59e6126d709cb2808591d5 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 2 files changed, 44 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/34914/6
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34914 )
Change subject: soc/amd/common: Add AcpiMmio access for SMBus PCI device ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34914/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34914/4//COMMIT_MSG@9 PS4, Line 9: 0xfed80000
0xfed80a00
+a00 is MMIO for controlling the bus.
https://review.coreboot.org/c/coreboot/+/34914/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34914/5//COMMIT_MSG@7 PS5, Line 7: SMBus
SMBus access through MMIO
That's a fixed MMIO option to the controller instead of relying on the variable I/O locations. This is the PCI device itself, not the registers for talking to the bus.
And I believe you mentioned UART can also be accessed.
No, there's a register for accessing a handful of settings that apply to the UART at D14F0xFC. It's a register in the PCI device config space, not accessing a UART itself.
https://review.coreboot.org/c/coreboot/+/34914/5//COMMIT_MSG@9 PS5, Line 9: PCI
Which device/function?
Done
https://review.coreboot.org/c/coreboot/+/34914/4/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/34914/4/src/soc/amd/common/block/ac... PS4, Line 69: 0xfed80000
0xfed80a00
fed80000 is correct
https://review.coreboot.org/c/coreboot/+/34914/4/src/soc/amd/common/block/ac... PS4, Line 73: ACPIMMIO_SM_PCI_BASE
Why not ACPIMMIO_SMB_PCI_BASE? "SM" could be confused with system management (SMM).
I don't think it's very confusing. The very next group is smi_read/write/n using ACPI_SMI_BASE. I can't think that there's a PCI-based SMM bank of registers we'd be accessing.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34914 )
Change subject: soc/amd/common: Add AcpiMmio access for SMBus PCI device ......................................................................
Patch Set 6: Code-Review+2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34914 )
Change subject: soc/amd/common: Add AcpiMmio access for SMBus PCI device ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34914/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34914/5//COMMIT_MSG@7 PS5, Line 7: SMBus
SMBus access through MMIO […]
Done
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/34914 )
Change subject: soc/amd/common: Add AcpiMmio access for SMBus PCI device ......................................................................
soc/amd/common: Add AcpiMmio access for SMBus PCI device
The standard PCI register space for D14F0 is accessible at 0xfed80000. Add functions for use as helpers.
Change-Id: Icbf5bdc449322c3f5e59e6126d709cb2808591d5 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34914 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h 2 files changed, 44 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Martin Roth: 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 edb3882..7fad456 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -65,7 +65,39 @@ pm_io_write16(reg + sizeof(uint16_t), value & 0xffff); }
-/* smbus pci read/write - access registers at 0xfed80000 - currently unused */ +#if SUPPORTS_ACPIMMIO_SM_PCI_BASE +/* smbus pci read/write - access registers at 0xfed80000 */ + +u8 sm_pci_read8(u8 reg) +{ + return read8((void *)(ACPIMMIO_SM_PCI_BASE + reg)); +} + +u16 sm_pci_read16(u8 reg) +{ + return read16((void *)(ACPIMMIO_SM_PCI_BASE + reg)); +} + +u32 sm_pci_read32(u8 reg) +{ + return read32((void *)(ACPIMMIO_SM_PCI_BASE + reg)); +} + +void sm_pci_write8(u8 reg, u8 value) +{ + write8((void *)(ACPIMMIO_SM_PCI_BASE + reg), value); +} + +void sm_pci_write16(u8 reg, u16 value) +{ + write16((void *)(ACPIMMIO_SM_PCI_BASE + reg), value); +} + +void sm_pci_write32(u8 reg, u32 value) +{ + write32((void *)(ACPIMMIO_SM_PCI_BASE + reg), value); +} +#endif
#if SUPPORTS_ACPIMMIO_SMI_BASE /* smi read/write - access registers at 0xfed80200 */ diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index 59ab9e5..ca57cf5 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -21,6 +21,9 @@ #include <stdint.h> /* iomap.h must indicate if the device uses a block, optional if unused. */ #include <soc/iomap.h> +#ifndef SUPPORTS_ACPIMMIO_SM_PCI_BASE + #define SUPPORTS_ACPIMMIO_SM_PCI_BASE 0 +#endif #ifndef SUPPORTS_ACPIMMIO_SMI_BASE #define SUPPORTS_ACPIMMIO_SMI_BASE 0 #endif @@ -162,6 +165,14 @@ /* Enable the AcpiMmio range at 0xfed80000 */ void enable_acpimmio_decode(void);
+/* Access SMBus PCI registers at 0xfed80000 */ +uint8_t sm_pci_read8(uint8_t reg); +uint16_t sm_pci_read16(uint8_t reg); +uint32_t sm_pci_read32(uint8_t reg); +void sm_pci_write8(uint8_t reg, uint8_t value); +void sm_pci_write16(uint8_t reg, uint16_t value); +void sm_pci_write32(uint8_t reg, uint32_t value); + /* Access PM registers using IO cycles */ uint8_t pm_io_read8(uint8_t reg); uint16_t pm_io_read16(uint8_t reg);
-
Marshall Dawson (Code Review) -
Martin Roth (Code Review)
-
Richard Spiegel (Code Review)