Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
[WIP] soc/amd/common: Avoid MMIO aliasing on SMBus
Change-Id: I5e0ebd7609c5c83d0e443ffba74dae68017d3ebc Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/smbus/smbus.c M src/soc/amd/stoneyridge/southbridge.c 3 files changed, 20 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/42074/1
diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index b95a347..ac48538 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -250,46 +250,6 @@ write32(acpimmio_acpi + reg, value); }
-static inline uint8_t asf_read8(uint8_t reg) -{ - return read8(acpimmio_asf + reg); -} - -static inline uint16_t asf_read16(uint8_t reg) -{ - return read16(acpimmio_asf + reg); -} - -static inline void asf_write8(uint8_t reg, uint8_t value) -{ - write8(acpimmio_asf + reg, value); -} - -static inline void asf_write16(uint8_t reg, uint16_t value) -{ - write16(acpimmio_asf + reg, value); -} - -static inline uint8_t smbus_read8(uint8_t reg) -{ - return read8(acpimmio_smbus + reg); -} - -static inline uint16_t smbus_read16(uint8_t reg) -{ - return read16(acpimmio_smbus + reg); -} - -static inline void smbus_write8(uint8_t reg, uint8_t value) -{ - write8(acpimmio_smbus + reg, value); -} - -static inline void smbus_write16(uint8_t reg, uint16_t value) -{ - write16(acpimmio_smbus + reg, value); -} - static inline uint8_t iomux_read8(uint8_t reg) { return read8(acpimmio_iomux + reg); diff --git a/src/soc/amd/common/block/smbus/smbus.c b/src/soc/amd/common/block/smbus/smbus.c index b5db56b..f8bab62 100644 --- a/src/soc/amd/common/block/smbus/smbus.c +++ b/src/soc/amd/common/block/smbus/smbus.c @@ -3,7 +3,7 @@ #include <stdint.h> #include <console/console.h> #include <device/smbus_host.h> -#include <amdblocks/acpimmio.h> +#include <amdblocks/acpimmio_map.h> #include <soc/southbridge.h>
/* @@ -12,37 +12,31 @@ */ #define SMBUS_TIMEOUT (100 * 1000 * 10)
-static u8 controller_read8(uintptr_t base, u8 reg) +union reg_bank { + uint8_t reg8[0x100]; + uint16_t reg16[0x100 / sizeof(uint16_t)]; +}; + +static __always_inline u8 controller_read8(const u32 base, const u8 reg) { - switch (base) { - case ACPIMMIO_SMBUS_BASE: - return smbus_read8(reg); - case ACPIMMIO_ASF_BASE: - return asf_read8(reg); - default: - printk(BIOS_ERR, "Error attempting to read SMBus at address 0x%lx\n", - base); - } - return 0xff; + volatile union reg_bank *controller = (void *)(uintptr_t)base; + return controller->reg8[reg]; }
-static void controller_write8(uintptr_t base, u8 reg, u8 val) +static __always_inline void controller_write8(const u32 base, const u8 reg, const u8 val) { - switch (base) { - case ACPIMMIO_SMBUS_BASE: - smbus_write8(reg, val); - break; - case ACPIMMIO_ASF_BASE: - asf_write8(reg, val); - break; - default: - printk(BIOS_ERR, "Error attempting to write SMBus at address 0x%lx\n", - base); - } + volatile union reg_bank *controller = (void *)(uintptr_t)base; + controller->reg8[reg] = val; }
static int smbus_wait_until_ready(uintptr_t mmio) { + if ((mmio != (uintptr_t)acpimmio_smbus) && + (mmio != (uintptr_t)acpimmio_asf)) { + printk(BIOS_ERR, "Invalid SMBus or ASF base %#zx\n", mmio); + return -1; + } + u32 loops; loops = SMBUS_TIMEOUT; do { diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index e90fe1b..7bdc4a4 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -366,6 +366,7 @@
static void fch_smbus_init(void) { +#if 0 /* 400 kHz smbus speed. */ const uint8_t smbus_speed = (66000000 / (400000 * 4));
@@ -376,6 +377,7 @@ smbus_write8(SMBSLVSTAT, SMBSLV_STAT_CLEAR); asf_write8(SMBHSTSTAT, SMBHST_STAT_CLEAR); asf_write8(SMBSLVSTAT, SMBSLV_STAT_CLEAR); +#endif }
/* Before console init */
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42074
to look at the new patch set (#2).
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
[WIP] soc/amd/common: Avoid MMIO aliasing on SMBus
Change-Id: I5e0ebd7609c5c83d0e443ffba74dae68017d3ebc Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/smbus/smbus.c M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 22 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/42074/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... File src/soc/amd/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... PS2, Line 20: u32 make this a uintptr_t from the get-go?
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... PS2, Line 23: return controller->reg8[reg]; What's the reasoning for not using read8()? pointer aliasing rules?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... File src/soc/amd/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... PS2, Line 23: return controller->reg8[reg];
What's the reasoning for not using read8()? pointer aliasing rules?
Aliasing rules hit on every writeX() but for symmetry I avoid readX() here. Sequences where one does multiple MMIO writes with small offsets from same base produce a lot prettier code without writeX().
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... File src/soc/amd/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... PS2, Line 23: return controller->reg8[reg];
Aliasing rules hit on every writeX() but for symmetry I avoid readX() here. […]
Got it. Thanks.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42074
to look at the new patch set (#3).
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
[WIP] soc/amd/common: Avoid MMIO aliasing on SMBus
Change-Id: I5e0ebd7609c5c83d0e443ffba74dae68017d3ebc Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/smbus/smbus.c M src/soc/amd/picasso/southbridge.c M src/soc/amd/stoneyridge/southbridge.c 4 files changed, 22 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/42074/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
Set Ready For Review
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... File src/soc/amd/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... PS10, Line 15: union reg_bank { : uint8_t reg8[0x100]; : uint16_t reg16[0x100 / sizeof(uint16_t)]; : }; I'm clearly missing something. What aliasing? This seems really strange as well.
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... PS10, Line 23: controller->reg8[reg]; read8()
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... PS10, Line 29: controller->reg8[reg] = val write8()
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: [WIP] soc/amd/common: Avoid MMIO aliasing on SMBus ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... File src/soc/amd/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... PS10, Line 15: union reg_bank { : uint8_t reg8[0x100]; : uint16_t reg16[0x100 / sizeof(uint16_t)]; : };
I'm clearly missing something. What aliasing? This seems really strange as well.
It's a strange compiler optimisation topic indeed.
When we do a write8() the assembly "injector" decides the write could target anything in memory, it cannot determine the target is actually MMIO in this case. If we had previously loaded a variable from stack to register, it will have to be reloaded after write8().
It's not that bad with SMBus, but gets pretty bad with PCI MMCONF in ramstage.
Similarly with write32(), except that it now knows only variables of uint32_t may have been the target. So such variables on the stack will need to be reloaded to registers.
Also, this reg8[0x100] could be reg[0x20] or [0x40] to match how many registers the controller actually decodes. Compiler should be able to detect bad register indexing buildtime already.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42074
to look at the new patch set (#12).
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
soc/amd/common: Refactor SMBus base arguments
Replace SMBus base addresses with proper symbols.
SMBus controller has byte-wide registers. Remove the word accessors.
Change-Id: I5e0ebd7609c5c83d0e443ffba74dae68017d3ebc Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/include/amdblocks/acpimmio.h 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/stoneyridge/smbus_spd.c 5 files changed, 19 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/42074/12
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... File src/soc/amd/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... PS10, Line 15: union reg_bank { : uint8_t reg8[0x100]; : uint16_t reg16[0x100 / sizeof(uint16_t)]; : };
It's a strange compiler optimisation topic indeed. […]
With the base address passed on the stack now, this does not seem to happen.
Previously, the code accessed global acpimmio_smbus and _asf so it happened.
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... PS10, Line 23: controller->reg8[reg];
read8()
Done
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... PS10, Line 29: controller->reg8[reg] = val
write8()
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 12: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... File src/soc/amd/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/42074/10/src/soc/amd/common/block/s... PS10, Line 15: union reg_bank { : uint8_t reg8[0x100]; : uint16_t reg16[0x100 / sizeof(uint16_t)]; : };
With the base address passed on the stack now, this does not seem to happen. […]
Ack
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... File src/soc/amd/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/42074/2/src/soc/amd/common/block/sm... PS2, Line 20: u32
make this a uintptr_t from the get-go?
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 16: Code-Review+2
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Abandoned
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Restored
Hello build bot (Jenkins), Raul Rangel, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42074
to look at the new patch set (#17).
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
soc/amd/common: Refactor SMBus base arguments
Replace SMBus base addresses with proper symbols.
SMBus controller has byte-wide registers. Remove the word accessors.
Change-Id: I5e0ebd7609c5c83d0e443ffba74dae68017d3ebc Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/include/amdblocks/acpimmio.h 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/stoneyridge/smbus_spd.c 5 files changed, 20 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/42074/17
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 17:
it might be good if you split this into the word accessor removal part in a separate patch; that's the part i'd +2 right away; the rest needs some testing to verify that it won't break anything before i'd give it a +2 and i'm not sure when i'll get around to do that
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 17: Code-Review+1
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/17/src/soc/amd/common/block/s... File src/soc/amd/common/block/smbus/sm.c:
https://review.coreboot.org/c/coreboot/+/42074/17/src/soc/amd/common/block/s... PS17, Line 28: res = find_resource(pbus->dev, 0x90); I saw 0:14.0 PCI config register 0x90 defined in SB600 and SB700, dropped from SB800 documentation.
Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Marshall Dawson, Angel Pons, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42074
to look at the new patch set (#18).
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
soc/amd/common: Refactor SMBus base arguments
Replace SMBus base addresses with proper symbols.
Change-Id: I5e0ebd7609c5c83d0e443ffba74dae68017d3ebc Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- 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/stoneyridge/smbus_spd.c 4 files changed, 20 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/42074/18
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 18: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/17/src/soc/amd/common/block/s... File src/soc/amd/common/block/smbus/sm.c:
https://review.coreboot.org/c/coreboot/+/42074/17/src/soc/amd/common/block/s... PS17, Line 28: res = find_resource(pbus->dev, 0x90);
I saw 0:14.0 PCI config register 0x90 defined in SB600 and SB700, dropped from SB800 documentation.
just had a look and that register is neither defined in the stoneyridge bkdg not in the picasso ppr. as far as i've looked this function seems to be rather wrong to me
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/17/src/soc/amd/common/block/s... File src/soc/amd/common/block/smbus/sm.c:
https://review.coreboot.org/c/coreboot/+/42074/17/src/soc/amd/common/block/s... PS17, Line 28: res = find_resource(pbus->dev, 0x90);
just had a look and that register is neither defined in the stoneyridge bkdg not in the picasso ppr. […]
AFAIR, I had a revert for the copy-paste of this file, but Marshall blocked it in Jan2020.
Revisit CB:29258. AFAICS pbus->dev is always the same PCI device so I don't understand how the resource could resolve to two different addresses. But if the register 0x90 was already broken since sb800 in 2011..
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
Patch Set 19: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42074/17/src/soc/amd/common/block/s... File src/soc/amd/common/block/smbus/sm.c:
https://review.coreboot.org/c/coreboot/+/42074/17/src/soc/amd/common/block/s... PS17, Line 28: res = find_resource(pbus->dev, 0x90);
AFAIR, I had a revert for the copy-paste of this file, but Marshall blocked it in Jan2020. […]
after thinking a bit about this, i'd say that the only useful solution here would be always returning (uintptr_t)acpimmio_smbus for this pci device and when someone wants/needs to access asf, they need to use a corresponding mmio device and not the pci device. this is out of scope for this patch though
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42074 )
Change subject: soc/amd/common: Refactor SMBus base arguments ......................................................................
soc/amd/common: Refactor SMBus base arguments
Replace SMBus base addresses with proper symbols.
Change-Id: I5e0ebd7609c5c83d0e443ffba74dae68017d3ebc Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42074 Reviewed-by: Felix Held felix-coreboot@felixheld.de Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- 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/stoneyridge/smbus_spd.c 4 files changed, 20 insertions(+), 34 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
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 abf014c..69cc57a 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -126,10 +126,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
#endif /* AMD_BLOCK_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..28e662c 100644 --- a/src/soc/amd/common/block/smbus/sm.c +++ b/src/soc/amd/common/block/smbus/sm.c @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <amdblocks/acpimmio_map.h> +#include <amdblocks/acpimmio.h> #include <device/device.h> #include <device/pci.h> #include <device/pci_ids.h> @@ -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 (uintptr_t)acpimmio_smbus;
- return ACPIMMIO_ASF_BASE; + return (uintptr_t)acpimmio_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 4fb68d4..9fd18c5 100644 --- a/src/soc/amd/common/block/smbus/smbus.c +++ b/src/soc/amd/common/block/smbus/smbus.c @@ -4,8 +4,8 @@ #include <console/console.h> #include <device/smbus_host.h> #include <amdblocks/acpimmio.h> -#include <amdblocks/acpimmio_map.h> #include <amdblocks/smbus.h> +#include <soc/southbridge.h>
/* * Between 1-10 seconds, We should never timeout normally @@ -13,37 +13,27 @@ */ #define SMBUS_TIMEOUT (100 * 1000 * 10)
-static u8 controller_read8(uintptr_t base, u8 reg) +/* 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(const uintptr_t base, const u8 reg) { - switch (base) { - case ACPIMMIO_SMBUS_BASE: - return smbus_read8(reg); - case ACPIMMIO_ASF_BASE: - return asf_read8(reg); - default: - printk(BIOS_ERR, "Error attempting to read SMBus at address 0x%lx\n", - base); - } - return 0xff; + return read8((void *)(base + reg)); }
-static void controller_write8(uintptr_t base, u8 reg, u8 val) +static void controller_write8(const uintptr_t base, const u8 reg, const u8 val) { - switch (base) { - case ACPIMMIO_SMBUS_BASE: - smbus_write8(reg, val); - break; - case ACPIMMIO_ASF_BASE: - asf_write8(reg, val); - break; - default: - printk(BIOS_ERR, "Error attempting to write SMBus at address 0x%lx\n", - base); - } + write8((void *)(base + reg), val); }
static int smbus_wait_until_ready(uintptr_t mmio) { + if ((mmio != (uintptr_t)acpimmio_smbus) && + (mmio != (uintptr_t)acpimmio_asf)) { + printk(BIOS_ERR, "Invalid SMBus or ASF base %#zx\n", mmio); + return -1; + } + u32 loops; loops = SMBUS_TIMEOUT; do { diff --git a/src/soc/amd/stoneyridge/smbus_spd.c b/src/soc/amd/stoneyridge/smbus_spd.c index 7c54d8d..773aad1 100644 --- a/src/soc/amd/stoneyridge/smbus_spd.c +++ b/src/soc/amd/stoneyridge/smbus_spd.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <amdblocks/agesawrapper.h> -#include <amdblocks/acpimmio_map.h> +#include <amdblocks/acpimmio.h> #include <console/console.h> #include <device/pci_def.h> #include <device/device.h> @@ -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((uintptr_t)acpimmio_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((uintptr_t)acpimmio_smbus, dev_addr); if (error < 0) { printk(BIOS_ERR, "-------------SPD READ ERROR-----------\n"); return error;
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Kyösti Mälkki (Code Review)
-
Raul Rangel (Code Review)