Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
[WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address
Change-Id: I8d50de60bb1ea1b3a521ab535a5637c4de8c3559 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h 2 files changed, 86 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/42073/1
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c index b3d3332..9be9bcb 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -5,28 +5,56 @@ #include <amdblocks/acpimmio_map.h> #include <amdblocks/acpimmio.h>
-uint8_t *const acpimmio_sm_pci = ACPIMMIO_BASE(SM_PCI); -uint8_t *const acpimmio_gpio_100 = ACPIMMIO_BASE(GPIO_100); -uint8_t *const acpimmio_smi = ACPIMMIO_BASE(SMI); -uint8_t *const acpimmio_pmio = ACPIMMIO_BASE(PMIO); -uint8_t *const acpimmio_pmio2 = ACPIMMIO_BASE(PMIO2); -uint8_t *const acpimmio_biosram = ACPIMMIO_BASE(BIOSRAM); -uint8_t *const acpimmio_cmosram = ACPIMMIO_BASE(CMOSRAM); -uint8_t *const acpimmio_cmos = ACPIMMIO_BASE(CMOS); -uint8_t *const acpimmio_acpi = ACPIMMIO_BASE(ACPI); -uint8_t *const acpimmio_asf = ACPIMMIO_BASE(ASF); -uint8_t *const acpimmio_smbus = ACPIMMIO_BASE(SMBUS); -uint8_t *const acpimmio_wdt = ACPIMMIO_BASE(WDT); -uint8_t *const acpimmio_hpet = ACPIMMIO_BASE(HPET); -uint8_t *const acpimmio_iomux = ACPIMMIO_BASE(IOMUX); -uint8_t *const acpimmio_misc = ACPIMMIO_BASE(MISC); -uint8_t *const acpimmio_dpvga = ACPIMMIO_BASE(DPVGA); -uint8_t *const acpimmio_gpio0 = ACPIMMIO_BASE(GPIO0); -uint8_t *const acpimmio_gpio1 = ACPIMMIO_BASE(GPIO1); -uint8_t *const acpimmio_gpio2 = ACPIMMIO_BASE(GPIO2); -uint8_t *const acpimmio_xhci_pm = ACPIMMIO_BASE(XHCIPM); -uint8_t *const acpimmio_acdc_tmr = ACPIMMIO_BASE(ACDCTMR); -uint8_t *const acpimmio_aoac = ACPIMMIO_BASE(AOAC); +DECLARE_ACPIMMIO(acpimmio_sm_pci, SM_PCI); +DECLARE_ACPIMMIO(acpimmio_gpio_100, GPIO_100); +DECLARE_ACPIMMIO(acpimmio_smi, SMI); +DECLARE_ACPIMMIO(acpimmio_pmio, PMIO); +DECLARE_ACPIMMIO(acpimmio_pmio2, PMIO2); +DECLARE_ACPIMMIO(acpimmio_biosram, BIOSRAM); +DECLARE_ACPIMMIO(acpimmio_cmosram, CMOSRAM); +DECLARE_ACPIMMIO(acpimmio_cmos, CMOS); +DECLARE_ACPIMMIO(acpimmio_acpi, ACPI); +DECLARE_ACPIMMIO(acpimmio_asf, ASF); +DECLARE_ACPIMMIO(acpimmio_smbus, SMBUS); +DECLARE_ACPIMMIO(acpimmio_wdt, WDT); +DECLARE_ACPIMMIO(acpimmio_hpet, HPET); +DECLARE_ACPIMMIO(acpimmio_iomux, IOMUX); +DECLARE_ACPIMMIO(acpimmio_misc, MISC); +DECLARE_ACPIMMIO(acpimmio_dpvga, DPVGA); +DECLARE_ACPIMMIO(acpimmio_gpio0, GPIO0); +DECLARE_ACPIMMIO(acpimmio_gpio1, GPIO1); +DECLARE_ACPIMMIO(acpimmio_gpio2, GPIO2); +DECLARE_ACPIMMIO(acpimmio_xhci_pm, XHCIPM); +DECLARE_ACPIMMIO(acpimmio_acdc_tmr, ACDCTMR); +DECLARE_ACPIMMIO(acpimmio_aoac, AOAC); + +#if !CONSTANT_ACPIMMIO_BASE_ADDRESS +static void __unused update_acpimmio_base(uintptr_t base) +{ + acpimmio_sm_pci = ACPIMMIO_BASE(base, SM_PCI); + acpimmio_gpio_100 = ACPIMMIO_BASE(base, GPIO_100); + acpimmio_smi = ACPIMMIO_BASE(base, SMI); + acpimmio_pmio = ACPIMMIO_BASE(base, PMIO); + acpimmio_pmio2 = ACPIMMIO_BASE(base, PMIO2); + acpimmio_biosram = ACPIMMIO_BASE(base, BIOSRAM); + acpimmio_cmosram = ACPIMMIO_BASE(base, CMOSRAM); + acpimmio_cmos = ACPIMMIO_BASE(base, CMOS); + acpimmio_acpi = ACPIMMIO_BASE(base, ACPI); + acpimmio_asf = ACPIMMIO_BASE(base, ASF); + acpimmio_smbus = ACPIMMIO_BASE(base, SMBUS); + acpimmio_wdt = ACPIMMIO_BASE(base, WDT); + acpimmio_hpet = ACPIMMIO_BASE(base, HPET); + acpimmio_iomux = ACPIMMIO_BASE(base, IOMUX); + acpimmio_misc = ACPIMMIO_BASE(base, MISC); + acpimmio_dpvga = ACPIMMIO_BASE(base, DPVGA); + acpimmio_gpio0 = ACPIMMIO_BASE(base, GPIO0); + acpimmio_gpio1 = ACPIMMIO_BASE(base, GPIO1); + acpimmio_gpio2 = ACPIMMIO_BASE(base, GPIO2); + acpimmio_xhci_pm = ACPIMMIO_BASE(base, XHCIPM); + acpimmio_acdc_tmr = ACPIMMIO_BASE(base, ACDCTMR); + acpimmio_aoac = ACPIMMIO_BASE(base, AOAC); +} +#endif
void enable_acpimmio_decode_pm24(void) { diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h index 3d3c359..9cd6281 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -35,8 +35,19 @@ * across family/model products. */
+#define CONSTANT_ACPIMMIO_BASE_ADDRESS 0 + #define AMD_SB_ACPI_MMIO_ADDR 0xfed80000
+#if CONSTANT_ACPIMMIO_BASE_ADDRESS +#define MAYBE_CONST const +#define DECLARE_ACPIMMIO(ptr, bank) \ + uint8_t *const ptr = (void *)(AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ ## bank ## _BANK) +#else +#define MAYBE_CONST +#define DECLARE_ACPIMMIO(ptr, bank) uint8_t *ptr +#endif + #ifdef __ACPI__
/* Damn, ASL fails on additions. */ @@ -68,35 +79,36 @@ #define ACPIMMIO_ACDCTMR_BANK 0x1d00 #define ACPIMMIO_AOAC_BANK 0x1e00
-#define ACPIMMIO_BASE(x) \ - (void *)(AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ ## x ## _BANK) +#define ACPIMMIO_BASE(base, x) (void *)(base + ACPIMMIO_ ## x ## _BANK)
/* FIXME: Passing host base for SMBUS is not long-term solution. */ #define ACPIMMIO_ASF_BASE (AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ASF_BANK) #define ACPIMMIO_SMBUS_BASE (AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_SMBUS_BANK)
-extern uint8_t *const acpimmio_gpio_100; -extern uint8_t *const acpimmio_sm_pci; -extern uint8_t *const acpimmio_smi; -extern uint8_t *const acpimmio_pmio; -extern uint8_t *const acpimmio_pmio2; -extern uint8_t *const acpimmio_biosram; -extern uint8_t *const acpimmio_cmosram; -extern uint8_t *const acpimmio_cmos; -extern uint8_t *const acpimmio_acpi; -extern uint8_t *const acpimmio_asf; -extern uint8_t *const acpimmio_smbus; -extern uint8_t *const acpimmio_wdt; -extern uint8_t *const acpimmio_hpet; -extern uint8_t *const acpimmio_iomux; -extern uint8_t *const acpimmio_misc; -extern uint8_t *const acpimmio_dpvga; -extern uint8_t *const acpimmio_gpio0; -extern uint8_t *const acpimmio_gpio1; -extern uint8_t *const acpimmio_gpio2; -extern uint8_t *const acpimmio_xhci_pm; -extern uint8_t *const acpimmio_acdc_tmr; -extern uint8_t *const acpimmio_aoac; +extern uint8_t *MAYBE_CONST acpimmio_gpio_100; +extern uint8_t *MAYBE_CONST acpimmio_sm_pci; +extern uint8_t *MAYBE_CONST acpimmio_smi; +extern uint8_t *MAYBE_CONST acpimmio_pmio; +extern uint8_t *MAYBE_CONST acpimmio_pmio2; +extern uint8_t *MAYBE_CONST acpimmio_biosram; +extern uint8_t *MAYBE_CONST acpimmio_cmosram; +extern uint8_t *MAYBE_CONST acpimmio_cmos; +extern uint8_t *MAYBE_CONST acpimmio_acpi; +extern uint8_t *MAYBE_CONST acpimmio_asf; +extern uint8_t *MAYBE_CONST acpimmio_smbus; +extern uint8_t *MAYBE_CONST acpimmio_wdt; +extern uint8_t *MAYBE_CONST acpimmio_hpet; +extern uint8_t *MAYBE_CONST acpimmio_iomux; +extern uint8_t *MAYBE_CONST acpimmio_misc; +extern uint8_t *MAYBE_CONST acpimmio_dpvga; +extern uint8_t *MAYBE_CONST acpimmio_gpio0; +extern uint8_t *MAYBE_CONST acpimmio_gpio1; +extern uint8_t *MAYBE_CONST acpimmio_gpio2; +extern uint8_t *MAYBE_CONST acpimmio_xhci_pm; +extern uint8_t *MAYBE_CONST acpimmio_acdc_tmr; +extern uint8_t *MAYBE_CONST acpimmio_aoac; + +#undef MAYBE_CONST
#endif
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42073/1/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42073/1/src/soc/amd/common/block/ac... PS1, Line 32: update_acpimmio_base The psp maps each block individually. See https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42073/1/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42073/1/src/soc/amd/common/block/ac... PS1, Line 32: update_acpimmio_base
The psp maps each block individually. See https://source.chromium. […]
I am not setup to build for PSP and I don't have documentation access either.
Not sure if there is some actual IOMMU, and each bank mapping eats a resource from that. Maybe there is some security implication why they did not allow mapping the entire ACPIMMIO space as one 8 kB block. Or then it was just never requested?
If you come up with a need for verstage to access some new bank, you are looking at changes to PSP kernel to implmement additional service calls for the new mapping...
Hello build bot (Jenkins), Raul Rangel, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42073
to look at the new patch set (#2).
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
[WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address
Change-Id: I8d50de60bb1ea1b3a521ab535a5637c4de8c3559 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h 2 files changed, 59 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/42073/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42073/1/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42073/1/src/soc/amd/common/block/ac... PS1, Line 32: update_acpimmio_base
I am not setup to build for PSP and I don't have documentation access either. […]
Removed, although 8 kB block would have been my preferred choice here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 2:
While this is under soc/amd it would affect pcengines/apu2 and lenovo/g505s too. At the moment I have no facility to run tests on these. Generated code size would also be of interest here. Need to cherry-pick parent commit too for testing.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 47: #else Are you thinking we would be directly assigning to the pointer objects for each mapping when running on psp?
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 83: #define ACPIMMIO_BASE(base, x) (void *)(base + ACPIMMIO_ ## x ## _BANK) Why add the 'base' here? It also doesn't appear this macro is used any more? I think adding a comment providing expectations of use would be helpful.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 47: #else
Are you thinking we would be directly assigning to the pointer objects for each mapping when running […]
I have no docs and did not search for the code in psp-verstage application how it is done there. But yes, I was thinking a one shot function early in the application to assign all the pointers.
I know some syscall like interface is there to set up the mapping and my interpretation was banks would have a constant address through the lifetime of the application.
I was also wondering how atomicity of read-modify-write ops to those banks is guaranteed.
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 83: #define ACPIMMIO_BASE(base, x) (void *)(base + ACPIMMIO_ ## x ## _BANK)
Why add the 'base' here? It also doesn't appear this macro is used any more? I think adding a commen […]
I had assumed the entire ACPIMMIO block of 8 KB would be accessible for psp-verstage application as a one mapping. It's one of the comments in patchset #1.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 47: #else
I have no docs and did not search for the code in psp-verstage application how it is done there. […]
Basically, the interface is that one has to request each bank individually: https://review.coreboot.org/c/coreboot/+/42060/4/src/vendorcode/amd/fsp/pica...
The ids for that svc call are in that same header: typedef enum FCH_IO_DEVICE { FCH_IO_DEVICE_SPI, FCH_IO_DEVICE_I2C, FCH_IO_DEVICE_GPIO, FCH_IO_DEVICE_ESPI, FCH_IO_DEVICE_IOMUX, FCH_IO_DEVICE_MISC, FCH_IO_DEVICE_AOAC, FCH_IO_DEVICE_IOPORT,
FCH_IO_DEVICE_END, } FCH_IO_DEVICE;
I was hoping the mapping would be consistent as well, but it truly is putting each mapping into its own page, and it's not consistent. It's based on the ordering of the mapping at runtime. Once a mapping is performed it doesn't change.
atomicity of rmw with a single instruction is typically not supported anywhere on MMIO. It's not something we can generally count on, but that's true for all archs I know (not saying it's impossible, but its' not something to expect).
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 83: #define ACPIMMIO_BASE(base, x) (void *)(base + ACPIMMIO_ ## x ## _BANK)
I had assumed the entire ACPIMMIO block of 8 KB would be accessible for psp-verstage application as […]
Which ones were you referring to? The supported mappings for service call are not all the mmio bases as far as I know:
typedef enum FCH_IO_DEVICE { FCH_IO_DEVICE_SPI, FCH_IO_DEVICE_I2C, FCH_IO_DEVICE_GPIO, FCH_IO_DEVICE_ESPI, FCH_IO_DEVICE_IOMUX, FCH_IO_DEVICE_MISC, FCH_IO_DEVICE_AOAC, FCH_IO_DEVICE_IOPORT,
FCH_IO_DEVICE_END, } FCH_IO_DEVICE;
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 47: #else
Basically, the interface is that one has to request each bank individually: https://review.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 83: #define ACPIMMIO_BASE(base, x) (void *)(base + ACPIMMIO_ ## x ## _BANK)
Which ones were you referring to? The supported mappings for service call are not all the mmio bases […]
All the banks together would have been one 8 KB mapping. I'll just remove ACPIMMIO_BASE() with next rebase.
https://review.coreboot.org/c/coreboot/+/42073/1/src/soc/amd/common/block/ac...
Hello Mike Banon, build bot (Jenkins), Raul Rangel, Michał Żygowski, Krystian Hebel, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42073
to look at the new patch set (#3).
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
[WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address
Change-Id: I8d50de60bb1ea1b3a521ab535a5637c4de8c3559 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h 2 files changed, 58 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/42073/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: [WIP] sb,soc/amd: Allow dynamic ACPIMMIO base address ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42073/2/src/soc/amd/common/block/in... PS2, Line 83: #define ACPIMMIO_BASE(base, x) (void *)(base + ACPIMMIO_ ## x ## _BANK)
All the banks together would have been one 8 KB mapping. […]
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
Set Ready For Review
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42073/9/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42073/9/src/soc/amd/common/block/ac... PS9, Line 10: uint8_t *const ptr = (void *)(AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ ## bank ## _BANK) cast through uintptr_t
Hello Mike Banon, build bot (Jenkins), Raul Rangel, Michał Żygowski, Krystian Hebel, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42073
to look at the new patch set (#10).
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
soc/amd/common: Allow runtime mapping of ACPIMMIO banks
Future implementation of verstage running on PSP will have access to some of the ACPIMMIO banks, but banks will be mapped runtime at non-deterministic addresses. Provide preprocessor helpers to accomplish this.
Change-Id: I8d50de60bb1ea1b3a521ab535a5637c4de8c3559 Signed-off-by: Kyösti Mälkki kyosti.malkki@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/common/block/include/amdblocks/acpimmio_map.h 3 files changed, 67 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/42073/10
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
Patch Set 10: Code-Review+1
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42073/10/src/soc/amd/common/block/a... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42073/10/src/soc/amd/common/block/a... PS10, Line 15: bank Not a fan of how this reads. You need to pay close attention that the bank parameter is dropped if !CONSTANT_ACPIMMIO_BASE_ADDRESS.
How about:
uint8_t *MAYBE_CONST acpimmio_gpio_100 = POSSIBLE_BASE(GPIO_100);
Hello Mike Banon, build bot (Jenkins), Raul Rangel, Martin Roth, Michał Żygowski, Krystian Hebel, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42073
to look at the new patch set (#12).
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
soc/amd/common: Allow runtime mapping of ACPIMMIO banks
Future implementation of verstage running on PSP will have access to some of the ACPIMMIO banks, but banks will be mapped runtime at non-deterministic addresses. Provide preprocessor helpers to accomplish this.
Change-Id: I8d50de60bb1ea1b3a521ab535a5637c4de8c3559 Signed-off-by: Kyösti Mälkki kyosti.malkki@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/common/block/include/amdblocks/acpimmio_map.h 3 files changed, 67 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/42073/12
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42073/9/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42073/9/src/soc/amd/common/block/ac... PS9, Line 10: uint8_t *const ptr = (void *)(AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ ## bank ## _BANK)
cast through uintptr_t
Done
https://review.coreboot.org/c/coreboot/+/42073/10/src/soc/amd/common/block/a... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42073/10/src/soc/amd/common/block/a... PS10, Line 15: bank
Not a fan of how this reads. […]
I would rather exclude the entire <amdblocks/acpimmio_map.h> with ENV_ARM, propose a solution that works for that too.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
Patch Set 12: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42073/10/src/soc/amd/common/block/a... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42073/10/src/soc/amd/common/block/a... PS10, Line 15: bank
I would rather exclude the entire <amdblocks/acpimmio_map. […]
I think it's fine for now. Would rather see this merged.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
Patch Set 12: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42073 )
Change subject: soc/amd/common: Allow runtime mapping of ACPIMMIO banks ......................................................................
soc/amd/common: Allow runtime mapping of ACPIMMIO banks
Future implementation of verstage running on PSP will have access to some of the ACPIMMIO banks, but banks will be mapped runtime at non-deterministic addresses. Provide preprocessor helpers to accomplish this.
Change-Id: I8d50de60bb1ea1b3a521ab535a5637c4de8c3559 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42073 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com 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 M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h 3 files changed, 67 insertions(+), 47 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, but someone else must approve Furquan Shaikh: Looks good to me, approved Raul Rangel: 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 b3d3332..5084672 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -5,28 +5,40 @@ #include <amdblocks/acpimmio_map.h> #include <amdblocks/acpimmio.h>
-uint8_t *const acpimmio_sm_pci = ACPIMMIO_BASE(SM_PCI); -uint8_t *const acpimmio_gpio_100 = ACPIMMIO_BASE(GPIO_100); -uint8_t *const acpimmio_smi = ACPIMMIO_BASE(SMI); -uint8_t *const acpimmio_pmio = ACPIMMIO_BASE(PMIO); -uint8_t *const acpimmio_pmio2 = ACPIMMIO_BASE(PMIO2); -uint8_t *const acpimmio_biosram = ACPIMMIO_BASE(BIOSRAM); -uint8_t *const acpimmio_cmosram = ACPIMMIO_BASE(CMOSRAM); -uint8_t *const acpimmio_cmos = ACPIMMIO_BASE(CMOS); -uint8_t *const acpimmio_acpi = ACPIMMIO_BASE(ACPI); -uint8_t *const acpimmio_asf = ACPIMMIO_BASE(ASF); -uint8_t *const acpimmio_smbus = ACPIMMIO_BASE(SMBUS); -uint8_t *const acpimmio_wdt = ACPIMMIO_BASE(WDT); -uint8_t *const acpimmio_hpet = ACPIMMIO_BASE(HPET); -uint8_t *const acpimmio_iomux = ACPIMMIO_BASE(IOMUX); -uint8_t *const acpimmio_misc = ACPIMMIO_BASE(MISC); -uint8_t *const acpimmio_dpvga = ACPIMMIO_BASE(DPVGA); -uint8_t *const acpimmio_gpio0 = ACPIMMIO_BASE(GPIO0); -uint8_t *const acpimmio_gpio1 = ACPIMMIO_BASE(GPIO1); -uint8_t *const acpimmio_gpio2 = ACPIMMIO_BASE(GPIO2); -uint8_t *const acpimmio_xhci_pm = ACPIMMIO_BASE(XHCIPM); -uint8_t *const acpimmio_acdc_tmr = ACPIMMIO_BASE(ACDCTMR); -uint8_t *const acpimmio_aoac = ACPIMMIO_BASE(AOAC); +#define ACPI_BANK_PTR(bank) \ + (void *)(uintptr_t)(AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ ## bank ## _BANK) + +#if CONSTANT_ACPIMMIO_BASE_ADDRESS +#define DECLARE_ACPIMMIO(ptr, bank) \ + uint8_t *const ptr = ACPI_BANK_PTR(bank) +#else +#define DECLARE_ACPIMMIO(ptr, bank) uint8_t *ptr +#endif + +DECLARE_ACPIMMIO(acpimmio_sm_pci, SM_PCI); +DECLARE_ACPIMMIO(acpimmio_gpio_100, GPIO_100); +DECLARE_ACPIMMIO(acpimmio_smi, SMI); +DECLARE_ACPIMMIO(acpimmio_pmio, PMIO); +DECLARE_ACPIMMIO(acpimmio_pmio2, PMIO2); +DECLARE_ACPIMMIO(acpimmio_biosram, BIOSRAM); +DECLARE_ACPIMMIO(acpimmio_cmosram, CMOSRAM); +DECLARE_ACPIMMIO(acpimmio_cmos, CMOS); +DECLARE_ACPIMMIO(acpimmio_acpi, ACPI); +DECLARE_ACPIMMIO(acpimmio_asf, ASF); +DECLARE_ACPIMMIO(acpimmio_smbus, SMBUS); +DECLARE_ACPIMMIO(acpimmio_wdt, WDT); +DECLARE_ACPIMMIO(acpimmio_hpet, HPET); +DECLARE_ACPIMMIO(acpimmio_iomux, IOMUX); +DECLARE_ACPIMMIO(acpimmio_misc, MISC); +DECLARE_ACPIMMIO(acpimmio_dpvga, DPVGA); +DECLARE_ACPIMMIO(acpimmio_gpio0, GPIO0); +DECLARE_ACPIMMIO(acpimmio_gpio1, GPIO1); +DECLARE_ACPIMMIO(acpimmio_gpio2, GPIO2); +DECLARE_ACPIMMIO(acpimmio_xhci_pm, XHCIPM); +DECLARE_ACPIMMIO(acpimmio_acdc_tmr, ACDCTMR); +DECLARE_ACPIMMIO(acpimmio_aoac, AOAC); + +#undef DECLARE_ACPIMMIO
void enable_acpimmio_decode_pm24(void) { diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index 3d0cf06..d3deff1 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -23,28 +23,39 @@ #define PM_04_BIOSRAM_DECODE_EN BIT(0) #define PM_04_ACPIMMIO_DECODE_EN BIT(1)
-extern uint8_t *const acpimmio_gpio_100; -extern uint8_t *const acpimmio_sm_pci; -extern uint8_t *const acpimmio_smi; -extern uint8_t *const acpimmio_pmio; -extern uint8_t *const acpimmio_pmio2; -extern uint8_t *const acpimmio_biosram; -extern uint8_t *const acpimmio_cmosram; -extern uint8_t *const acpimmio_cmos; -extern uint8_t *const acpimmio_acpi; -extern uint8_t *const acpimmio_asf; -extern uint8_t *const acpimmio_smbus; -extern uint8_t *const acpimmio_wdt; -extern uint8_t *const acpimmio_hpet; -extern uint8_t *const acpimmio_iomux; -extern uint8_t *const acpimmio_misc; -extern uint8_t *const acpimmio_dpvga; -extern uint8_t *const acpimmio_gpio0; -extern uint8_t *const acpimmio_gpio1; -extern uint8_t *const acpimmio_gpio2; -extern uint8_t *const acpimmio_xhci_pm; -extern uint8_t *const acpimmio_acdc_tmr; -extern uint8_t *const acpimmio_aoac; +/* For x86 base is constant, while PSP does mapping runtime. */ +#define CONSTANT_ACPIMMIO_BASE_ADDRESS ENV_X86 + +#if CONSTANT_ACPIMMIO_BASE_ADDRESS +#define MAYBE_CONST const +#else +#define MAYBE_CONST +#endif + +extern uint8_t *MAYBE_CONST acpimmio_gpio_100; +extern uint8_t *MAYBE_CONST acpimmio_sm_pci; +extern uint8_t *MAYBE_CONST acpimmio_smi; +extern uint8_t *MAYBE_CONST acpimmio_pmio; +extern uint8_t *MAYBE_CONST acpimmio_pmio2; +extern uint8_t *MAYBE_CONST acpimmio_biosram; +extern uint8_t *MAYBE_CONST acpimmio_cmosram; +extern uint8_t *MAYBE_CONST acpimmio_cmos; +extern uint8_t *MAYBE_CONST acpimmio_acpi; +extern uint8_t *MAYBE_CONST acpimmio_asf; +extern uint8_t *MAYBE_CONST acpimmio_smbus; +extern uint8_t *MAYBE_CONST acpimmio_wdt; +extern uint8_t *MAYBE_CONST acpimmio_hpet; +extern uint8_t *MAYBE_CONST acpimmio_iomux; +extern uint8_t *MAYBE_CONST acpimmio_misc; +extern uint8_t *MAYBE_CONST acpimmio_dpvga; +extern uint8_t *MAYBE_CONST acpimmio_gpio0; +extern uint8_t *MAYBE_CONST acpimmio_gpio1; +extern uint8_t *MAYBE_CONST acpimmio_gpio2; +extern uint8_t *MAYBE_CONST acpimmio_xhci_pm; +extern uint8_t *MAYBE_CONST acpimmio_acdc_tmr; +extern uint8_t *MAYBE_CONST acpimmio_aoac; + +#undef MAYBE_CONST
/* For older discrete FCHs */ void enable_acpimmio_decode_pm24(void); diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h index d230d54..758a5562 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -126,9 +126,6 @@ #define ACPIMMIO_ACDCTMR_BANK 0x1d00 #define ACPIMMIO_AOAC_BANK 0x1e00
-#define ACPIMMIO_BASE(x) \ - (void *)(AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ ## x ## _BANK) - /* FIXME: Passing host base for SMBUS is not long-term solution. */ #define ACPIMMIO_ASF_BASE (AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ASF_BANK) #define ACPIMMIO_SMBUS_BASE (AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_SMBUS_BANK)
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)