Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
soc/amd/common: Redo ACPIMMIO_BASE and _BANK
Change-Id: I31f2d04d9fc8bdd9e270fb3cb48d71f215999a50 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 M src/soc/amd/common/block/smbus/sm.c M src/soc/amd/common/block/smbus/smbus.c M src/soc/amd/picasso/acpi/aoac.asl M src/soc/amd/stoneyridge/smbus_spd.c 6 files changed, 16 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42894/1
diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c index 5084672..8acd27f 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -5,12 +5,9 @@ #include <amdblocks/acpimmio_map.h> #include <amdblocks/acpimmio.h>
-#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) + uint8_t *const ptr = (void *)(uintptr_t)ACPIMMIO_BASE(bank) #else #define DECLARE_ACPIMMIO(ptr, bank) uint8_t *ptr #endif 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 758a5562..e5caefc 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -95,13 +95,10 @@ #define AMD_SB_ACPI_MMIO_ADDR 0xfed80000
#ifdef __ACPI__ - -/* ASL fails on additions. */ +/* ASL MemoryFixed32() fails if these are additions. */ #define ACPIMMIO_MISC_BASE 0xfed80e00 #define ACPIMMIO_GPIO0_BASE 0xfed81500 -#define ACPIMMIO_AOAC_BASE 0xfed81e00 - -#else +#endif
#define ACPIMMIO_SM_PCI_BANK 0x0000 #define ACPIMMIO_GPIO_100_BANK 0x0100 @@ -126,10 +123,6 @@ #define ACPIMMIO_ACDCTMR_BANK 0x1d00 #define ACPIMMIO_AOAC_BANK 0x1e00
-/* 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) - -#endif +#define ACPIMMIO_BASE(bank) (AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ ## bank ## _BANK)
#endif /* __AMDBLOCKS_ACPIMMIO_MAP_H__ */ diff --git a/src/soc/amd/common/block/smbus/sm.c b/src/soc/amd/common/block/smbus/sm.c index c5c1ed8..f540e97 100644 --- a/src/soc/amd/common/block/smbus/sm.c +++ b/src/soc/amd/common/block/smbus/sm.c @@ -27,9 +27,9 @@ pbus = get_pbus_smbus(dev); res = find_resource(pbus->dev, 0x90); if (res->base == SMB_BASE_ADDR) - return ACPIMMIO_SMBUS_BASE; + return ACPIMMIO_BASE(SMBUS);
- return ACPIMMIO_ASF_BASE; + return ACPIMMIO_BASE(ASF); }
static int lsmbus_recv_byte(struct device *dev) diff --git a/src/soc/amd/common/block/smbus/smbus.c b/src/soc/amd/common/block/smbus/smbus.c index e304483..fe1862f 100644 --- a/src/soc/amd/common/block/smbus/smbus.c +++ b/src/soc/amd/common/block/smbus/smbus.c @@ -13,12 +13,15 @@ */ #define SMBUS_TIMEOUT (100 * 1000 * 10)
+/* FIXME: Passing host base for SMBUS is not long-term solution. + It is possible to have multiple buses behind same host. */ + static u8 controller_read8(uintptr_t base, u8 reg) { switch (base) { - case ACPIMMIO_SMBUS_BASE: + case ACPIMMIO_BASE(SMBUS): return smbus_read8(reg); - case ACPIMMIO_ASF_BASE: + case ACPIMMIO_BASE(ASF): return asf_read8(reg); default: printk(BIOS_ERR, "Error attempting to read SMBus at address 0x%lx\n", @@ -30,10 +33,10 @@ static void controller_write8(uintptr_t base, u8 reg, u8 val) { switch (base) { - case ACPIMMIO_SMBUS_BASE: + case ACPIMMIO_BASE(SMBUS): smbus_write8(reg, val); break; - case ACPIMMIO_ASF_BASE: + case ACPIMMIO_BASE(ASF): asf_write8(reg, val); break; default: diff --git a/src/soc/amd/picasso/acpi/aoac.asl b/src/soc/amd/picasso/acpi/aoac.asl index f26f85f..1b9e255 100644 --- a/src/soc/amd/picasso/acpi/aoac.asl +++ b/src/soc/amd/picasso/acpi/aoac.asl @@ -4,7 +4,7 @@
#define AOAC_DEVICE(DEV_NAME, DEV_ID, SX) \ PowerResource(DEV_NAME, SX, 0) { \ - OperationRegion (AOAC, SystemMemory, ACPIMMIO_AOAC_BASE + 0x40 + (DEV_ID << 1), 2) \ + OperationRegion (AOAC, SystemMemory, ACPIMMIO_BASE(AOAC) + 0x40 + (DEV_ID << 1), 2) \ Field (AOAC, ByteAcc, NoLock, Preserve) { \ /* \ * Target Device State \ diff --git a/src/soc/amd/stoneyridge/smbus_spd.c b/src/soc/amd/stoneyridge/smbus_spd.c index 7c54d8d..38aac76 100644 --- a/src/soc/amd/stoneyridge/smbus_spd.c +++ b/src/soc/amd/stoneyridge/smbus_spd.c @@ -34,7 +34,7 @@ dev_addr = (SmbusSlaveAddress >> 1);
/* Read the first SPD byte */ - error = do_smbus_read_byte(ACPIMMIO_SMBUS_BASE, dev_addr, 0); + error = do_smbus_read_byte(ACPIMMIO_BASE(SMBUS), dev_addr, 0); if (error < 0) { printk(BIOS_ERR, "-------------SPD READ ERROR-----------\n"); return error; @@ -44,7 +44,7 @@
/* Read the remaining SPD bytes using do_smbus_recv_byte for speed */ for (index = 1 ; index < count ; index++) { - error = do_smbus_recv_byte(ACPIMMIO_SMBUS_BASE, dev_addr); + error = do_smbus_recv_byte(ACPIMMIO_BASE(SMBUS), dev_addr); if (error < 0) { printk(BIOS_ERR, "-------------SPD READ ERROR-----------\n"); return error;
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Patch Set 1:
There as at least _BANK -> _OFFSET request, anything else that could go here?
Hello build bot (Jenkins), Raul Rangel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42894
to look at the new patch set (#2).
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
soc/amd/common: Redo ACPIMMIO_BASE and _BANK
Change-Id: I31f2d04d9fc8bdd9e270fb3cb48d71f215999a50 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 M src/soc/amd/common/block/smbus/sm.c M src/soc/amd/common/block/smbus/smbus.c M src/soc/amd/picasso/acpi/aoac.asl M src/soc/amd/stoneyridge/smbus_spd.c 6 files changed, 17 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42894/2
Hello build bot (Jenkins), Raul Rangel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42894
to look at the new patch set (#3).
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
soc/amd/common: Redo ACPIMMIO_BASE and _BANK
Change-Id: I31f2d04d9fc8bdd9e270fb3cb48d71f215999a50 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 M src/soc/amd/picasso/acpi/aoac.asl 3 files changed, 5 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42894/3
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Patch Set 3: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Patch Set 7: Code-Review+2
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Abandoned
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Restored
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... PS8, Line 97: #ifdef __ACPI__ the ifdef isn't strictly necessary here. undecided if it should stay in there or not; for now i'm ok with it
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... PS8, Line 97: #ifdef __ACPI__
the ifdef isn't strictly necessary here. […]
It's there to prevent them from being used outside ASL. But the comment below actully became obsolete with some iasl update and some followup removes the couple lines here.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... PS8, Line 97: #ifdef __ACPI__
It's there to prevent them from being used outside ASL. […]
ah, ok, that's probably a good thing then
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... PS8, Line 97: #ifdef __ACPI__
ah, ok, that's probably a good thing then
A bit more background; all addresses here represent the x86-centric view of the memory map and should have ENV_X86 guard. CB:42523
Seems like I got tired of waiting for a developer to submit the works and after sitting with +2's for two weeks I finally had abandoned these.
Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Marshall Dawson, Angel Pons, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42894
to look at the new patch set (#9).
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
soc/amd/common: Redo ACPIMMIO_BASE and _BANK
Change-Id: I31f2d04d9fc8bdd9e270fb3cb48d71f215999a50 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 M src/soc/amd/picasso/acpi/aoac.asl 3 files changed, 5 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/42894/9
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpimmio_map.h:
https://review.coreboot.org/c/coreboot/+/42894/8/src/soc/amd/common/block/in... PS8, Line 97: #ifdef __ACPI__
A bit more background; all addresses here represent the x86-centric view of the memory map and shoul […]
ASL change CB:42944 does not build so the comment below still applies.
Memory32Fixed (ReadWrite, 0xfed81500, 0x400) vs. Memory32Fixed (ReadWrite, (0xfed80000 + 0x1500), 0x400)
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42894 )
Change subject: soc/amd/common: Redo ACPIMMIO_BASE and _BANK ......................................................................
soc/amd/common: Redo ACPIMMIO_BASE and _BANK
Change-Id: I31f2d04d9fc8bdd9e270fb3cb48d71f215999a50 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42894 Reviewed-by: Felix Held felix-coreboot@felixheld.de Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Raul Rangel rrangel@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h M src/soc/amd/picasso/acpi/aoac.asl 3 files changed, 5 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved Raul Rangel: Looks good to me, approved Angel Pons: 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 6231d49..b2df8e7 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -5,12 +5,9 @@ #include <amdblocks/acpimmio_map.h> #include <amdblocks/acpimmio.h>
-#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) + uint8_t *const ptr = (void *)(uintptr_t)ACPIMMIO_BASE(bank) #else #define DECLARE_ACPIMMIO(ptr, bank) uint8_t *ptr #endif 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 69cc57a..176dc2b 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -95,13 +95,10 @@ #define AMD_SB_ACPI_MMIO_ADDR 0xfed80000
#ifdef __ACPI__ - -/* ASL fails on additions. */ +/* ASL MemoryFixed32() fails if these are additions. */ #define ACPIMMIO_MISC_BASE 0xfed80e00 #define ACPIMMIO_GPIO0_BASE 0xfed81500 -#define ACPIMMIO_AOAC_BASE 0xfed81e00 - -#else +#endif
#define ACPIMMIO_SM_PCI_BANK 0x0000 #define ACPIMMIO_GPIO_100_BANK 0x0100 @@ -126,6 +123,6 @@ #define ACPIMMIO_ACDCTMR_BANK 0x1d00 #define ACPIMMIO_AOAC_BANK 0x1e00
-#endif +#define ACPIMMIO_BASE(bank) (AMD_SB_ACPI_MMIO_ADDR + ACPIMMIO_ ## bank ## _BANK)
#endif /* AMD_BLOCK_ACPIMMIO_MAP_H */ diff --git a/src/soc/amd/picasso/acpi/aoac.asl b/src/soc/amd/picasso/acpi/aoac.asl index ffdfcd4..461b78c 100644 --- a/src/soc/amd/picasso/acpi/aoac.asl +++ b/src/soc/amd/picasso/acpi/aoac.asl @@ -4,7 +4,7 @@
#define AOAC_DEVICE(DEV_NAME, DEV_ID, SX) \ PowerResource(DEV_NAME, SX, 0) { \ - OperationRegion (AOAC, SystemMemory, ACPIMMIO_AOAC_BASE + 0x40 + (DEV_ID << 1), 2) \ + OperationRegion (AOAC, SystemMemory, ACPIMMIO_BASE(AOAC) + 0x40 + (DEV_ID << 1), 2) \ Field (AOAC, ByteAcc, NoLock, Preserve) { \ /* \ * Target Device State \
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Kyösti Mälkki (Code Review)
-
Raul Rangel (Code Review)