Change in coreboot[master]: [WIP] soc/amd/common/block/acpimmio: Redo acpimmio for psp_verstage a...
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: [WIP] soc/amd/common/block/acpimmio: Redo acpimmio for psp_verstage again ......................................................................
[WIP] soc/amd/common/block/acpimmio: Redo acpimmio for psp_verstage again
Change-Id: I3cb1b5a90023ebc4359835be716c5e3f9451df60 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/acpimmio/Makefile.inc M src/soc/amd/common/block/acpimmio/mmio_util.c D src/soc/amd/common/block/acpimmio/mmio_util_psp.c A src/soc/amd/common/block/acpimmio/psp_verstage_stub.c M src/soc/amd/common/block/include/amdblocks/acpimmio.h M src/soc/amd/common/block/include/amdblocks/acpimmio_map.h D src/soc/amd/common/block/include/amdblocks/acpimmio_psp.h 7 files changed, 67 insertions(+), 245 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/42523/1
diff --git a/src/soc/amd/common/block/acpimmio/Makefile.inc b/src/soc/amd/common/block/acpimmio/Makefile.inc index 13864e4..28d78d5 100644 --- a/src/soc/amd/common/block/acpimmio/Makefile.inc +++ b/src/soc/amd/common/block/acpimmio/Makefile.inc @@ -13,5 +13,5 @@ ifeq ($(CONFIG_SOC_AMD_COMMON_BLOCK_ACPIMMIO),y) verstage-$(CONFIG_ARCH_VERSTAGE_X86_32) += biosram.c verstage-$(CONFIG_ARCH_VERSTAGE_X86_32) += mmio_util.c -verstage-$(CONFIG_ARCH_VERSTAGE_ARM) += mmio_util_psp.c +verstage-$(CONFIG_ARCH_VERSTAGE_ARM) += mmio_util.c endif diff --git a/src/soc/amd/common/block/acpimmio/mmio_util.c b/src/soc/amd/common/block/acpimmio/mmio_util.c index c5f82f9..2bfa878 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -2,9 +2,12 @@
#include <types.h> #include <arch/io.h> -#include <amdblocks/acpimmio_map.h> #include <amdblocks/acpimmio.h>
+#if CONSTANT_ACPIMMIO_BASE_ADDRESS +#include <amdblocks/acpimmio_map.h> +#endif + DECLARE_ACPIMMIO(acpimmio_sm_pci, SM_PCI); DECLARE_ACPIMMIO(acpimmio_gpio_100, GPIO_100); DECLARE_ACPIMMIO(acpimmio_smi, SMI); diff --git a/src/soc/amd/common/block/acpimmio/mmio_util_psp.c b/src/soc/amd/common/block/acpimmio/mmio_util_psp.c deleted file mode 100644 index 75f71e4..0000000 --- a/src/soc/amd/common/block/acpimmio/mmio_util_psp.c +++ /dev/null @@ -1,163 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <stdint.h> -#include <arch/io.h> -#include <device/mmio.h> -#include <amdblocks/acpimmio.h> - -static uintptr_t iomux_bar; - -void iomux_set_bar(void *bar) -{ - iomux_bar = (uintptr_t)bar; -} - -u8 iomux_read8(u8 reg) -{ - return read8((void *)(iomux_bar + reg)); -} - -u16 iomux_read16(u8 reg) -{ - return read16((void *)(iomux_bar + reg)); -} - -u32 iomux_read32(u8 reg) -{ - return read32((void *)(iomux_bar + reg)); -} - -void iomux_write8(u8 reg, u8 value) -{ - write8((void *)(iomux_bar + reg), value); -} - -void iomux_write16(u8 reg, u16 value) -{ - write16((void *)(iomux_bar + reg), value); -} - -void iomux_write32(u8 reg, u32 value) -{ - write32((void *)(iomux_bar + reg), value); -} - -static uintptr_t misc_bar; - -void misc_set_bar(void *bar) -{ - misc_bar = (uintptr_t)bar; -} - -u8 misc_read8(u8 reg) -{ - return read8((void *)(misc_bar + reg)); -} - -u16 misc_read16(u8 reg) -{ - return read16((void *)(misc_bar + reg)); -} - -u32 misc_read32(u8 reg) -{ - return read32((void *)(misc_bar + reg)); -} - -void misc_write8(u8 reg, u8 value) -{ - write8((void *)(misc_bar + reg), value); -} - -void misc_write16(u8 reg, u16 value) -{ - write16((void *)(misc_bar + reg), value); -} - -void misc_write32(u8 reg, u32 value) -{ - write32((void *)(misc_bar + reg), value); -} - -static uintptr_t gpio_bar; - -void gpio_set_bar(void *bar) -{ - gpio_bar = (uintptr_t)bar; -} - -void *gpio_get_bar(void) -{ - return (void *)gpio_bar; -} - -static uintptr_t aoac_bar; - -void aoac_set_bar(void *bar) -{ - aoac_bar = (uintptr_t)bar; -} - -u8 aoac_read8(u8 reg) -{ - return read8((void *)(aoac_bar + reg)); -} - -void aoac_write8(u8 reg, u8 value) -{ - write8((void *)(aoac_bar + reg), value); -} - -static uintptr_t io_bar; - -void io_set_bar(void *bar) -{ - io_bar = (uintptr_t)bar; -} - -u8 io_read8(u16 reg) -{ - return read8((void *)(io_bar + reg)); -} - -void io_write8(u16 reg, u8 value) -{ - write8((void *)(io_bar + reg), value); -} - -/* PM registers are accessed a byte at a time via CD6/CD7 */ -uint8_t pm_io_read8(uint8_t reg) -{ - outb(reg, PM_INDEX); - return inb(PM_DATA); -} - -uint16_t pm_io_read16(uint8_t reg) -{ - return (pm_io_read8(reg + sizeof(uint8_t)) << 8) | pm_io_read8(reg); -} - -uint32_t pm_io_read32(uint8_t reg) -{ - return (pm_io_read16(reg + sizeof(uint16_t)) << 16) | pm_io_read16(reg); -} - -void pm_io_write8(uint8_t reg, uint8_t value) -{ - outb(reg, PM_INDEX); - outb(value, PM_DATA); -} - -void pm_io_write16(uint8_t reg, uint16_t value) -{ - pm_io_write8(reg, value & 0xff); - value >>= 8; - pm_io_write8(reg + sizeof(uint8_t), value & 0xff); -} - -void pm_io_write32(uint8_t reg, uint32_t value) -{ - pm_io_write16(reg, value & 0xffff); - value >>= 16; - pm_io_write16(reg + sizeof(uint16_t), value & 0xffff); -} diff --git a/src/soc/amd/common/block/acpimmio/psp_verstage_stub.c b/src/soc/amd/common/block/acpimmio/psp_verstage_stub.c new file mode 100644 index 0000000..9c74f8f --- /dev/null +++ b/src/soc/amd/common/block/acpimmio/psp_verstage_stub.c @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <acpimmio.h> + +static void iomux_set_bar(void *bar) +{ + acpimmio_iomux_bar = bar; +} + +static void misc_set_bar(void *bar) +{ + acpimmio_misc_bar = bar; +} + +static void gpio_set_bar(void *bar) +{ + acpimmio_gpio0_bar = bar; +} + +static void aoac_set_bar(void *bar) +{ + acpimmio_aoac_bar = bar; +} + +static void io_set_bar(void *bar) +{ + acpimmio_io_bar = bar; +} diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio.h b/src/soc/amd/common/block/include/amdblocks/acpimmio.h index 3caed11..3f00ae2 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio.h @@ -6,6 +6,40 @@ #include <device/mmio.h> #include <types.h>
+/* 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 +#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 + +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_xhci_pm; +extern uint8_t *MAYBE_CONST acpimmio_acdc_tmr; +extern uint8_t *MAYBE_CONST acpimmio_aoac; + +#undef MAYBE_CONST
/* Enable the AcpiMmio range at 0xfed80000 */
@@ -23,13 +57,6 @@ void pm_io_write16(uint8_t reg, uint16_t value); void pm_io_write32(uint8_t reg, uint32_t value);
-#if !ENV_X86 - -#include <amdblocks/acpimmio_psp.h> - -#else - -#include <amdblocks/acpimmio_map.h>
static inline uint8_t sm_pci_read8(uint8_t reg) { @@ -384,6 +411,4 @@ write8(acpimmio_aoac + reg, value); }
-#endif /* ENV_X86 */ - #endif /* __AMDBLOCKS_ACPIMMIO_H__ */ 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 912e891..5d71287 100644 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h +++ b/src/soc/amd/common/block/include/amdblocks/acpimmio_map.h @@ -112,20 +112,8 @@ * across family/model products. */
-/* For x86 base is constant, while PSP does mapping runtime. */ -#define CONSTANT_ACPIMMIO_BASE_ADDRESS ENV_X86 - #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__
/* ASL fails on additions. */ @@ -161,29 +149,6 @@ #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 *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_xhci_pm; -extern uint8_t *MAYBE_CONST acpimmio_acdc_tmr; -extern uint8_t *MAYBE_CONST acpimmio_aoac; - -#undef MAYBE_CONST - #endif
#endif /* __AMDBLOCKS_ACPIMMIO_MAP_H__ */ diff --git a/src/soc/amd/common/block/include/amdblocks/acpimmio_psp.h b/src/soc/amd/common/block/include/amdblocks/acpimmio_psp.h deleted file mode 100644 index b8ca7b5..0000000 --- a/src/soc/amd/common/block/include/amdblocks/acpimmio_psp.h +++ /dev/null @@ -1,36 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-or-later */ - -#ifndef __AMDBLOCKS_ACPIMMIO_PSP_H__ -#define __AMDBLOCKS_ACPIMMIO_PSP_H__ - -#include <device/mmio.h> -#include <types.h> - -void iomux_set_bar(void *bar); -void *iomux_get_bar(void); -void misc_set_bar(void *bar); -void *misc_get_bar(void); -void gpio_set_bar(void *bar); -void *gpio_get_bar(void); -void aoac_set_bar(void *bar); -void *aoac_get_bar(void); -void io_set_bar(void *bar); -u8 io_read8(u16 reg); -void io_write8(u16 reg, u8 value); - -u8 iomux_read8(u8 reg); -u16 iomux_read16(u8 reg); -u32 iomux_read32(u8 reg); -void iomux_write8(u8 reg, u8 value); -void iomux_write16(u8 reg, u16 value); -void iomux_write32(u8 reg, u32 value); -u8 misc_read8(u8 reg); -u16 misc_read16(u8 reg); -u32 misc_read32(u8 reg); -void misc_write8(u8 reg, u8 value); -void misc_write16(u8 reg, u16 value); -void misc_write32(u8 reg, u32 value); -u8 aoac_read8(u8 reg); -void aoac_write8(u8 reg, u8 value); - -#endif
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: [WIP] soc/amd/common: Place guards for ACPIMMIO on ENV_ARM ......................................................................
Set Ready For Review
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42523
to look at the new patch set (#7).
Change subject: [WIP] soc/amd/common: Place guards for ACPIMMIO on ENV_ARM ......................................................................
[WIP] soc/amd/common: Place guards for ACPIMMIO on ENV_ARM
TBD: <arch/io.h> missing TBD: <soc/iomap.h> has x86-only sections TBD: Add mockup build-testing for ENV_VERSTAGE_ARMxx
Change-Id: I3cb1b5a90023ebc4359835be716c5e3f9451df60 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c A src/soc/amd/common/block/acpimmio/psp_verstage_stub.c M src/soc/amd/picasso/i2c.c M src/soc/amd/picasso/include/soc/iomap.h 4 files changed, 50 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/42523/7
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: [WIP] soc/amd/common: Place guards for ACPIMMIO on ENV_ARM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c@5... PS7, Line 55: switch (dev->path.mmio.addr) { If we continue with the ideological view that no x86 memory space symbols should appear in ARCH_VERSTAGE_ARMxx environment, what is the plan on these MMIO device addresses. To get past this for amd/picasso I think this ENV_X86 guard here would be option than splitting the file.
IMHO there is something else fundamentally wrong when we do this reverse-mapping from MMIO address to device node names. If there is better solution for this, the x86-centric identifiers APU_I2Cx_BASE might just disappear here.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: [WIP] soc/amd/common: Place guards for ACPIMMIO on ENV_ARM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c@5... PS7, Line 55: switch (dev->path.mmio.addr) {
If we continue with the ideological view that no x86 memory space symbols should appear in ARCH_VERS […]
You could just use the i2c_bus_address[x] values? I don't think those case statements will hold up, but that approach should work.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: [WIP] soc/amd/common: Place guards for ACPIMMIO on ENV_ARM ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/common/block/ac... PS7, Line 32: DECLARE_ACPIMMIO_BANK No reason to use DECLARE_ACPIMMIO_BANK in this block.
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/psp_verstage_stub.c:
PS7: I would just remove these since we can directly set acpimmio_iomux from psp_verstage.
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c@5... PS7, Line 55: switch (dev->path.mmio.addr) {
You could just use the i2c_bus_address[x] values? I don't think those case statements will hold up, […]
This is also why I split it off into a different file. It's only needed when generating ACPI tables, which verstage doesn't do.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: [WIP] soc/amd/common: Place guards for ACPIMMIO on ENV_ARM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c@5... PS7, Line 55: switch (dev->path.mmio.addr) {
This is also why I split it off into a different file. […]
Yes, but it won't be called. garbage collection will remove them.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42523
to look at the new patch set (#8).
Change subject: [WIP,RFC] soc/amd/common: Place some ENV_X86 guards ......................................................................
[WIP,RFC] soc/amd/common: Place some ENV_X86 guards
Base address symbols for ACPIMMIO banks that would not get assigned at runtime must not resolve at linker-stage either.
The build of PSP-verstage should pass without the preprocessor macros that have x86-centric view of memory space.
Change-Id: I3cb1b5a90023ebc4359835be716c5e3f9451df60 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/picasso/include/soc/iomap.h 2 files changed, 16 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/42523/8
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: [WIP,RFC] soc/amd/common: Place some ENV_X86 guards ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c@5... PS7, Line 55: switch (dev->path.mmio.addr) {
Yes, but it won't be called. garbage collection will remove them.
I did not look deep into this yet but looks to me that i2c controller enablement does not respond the excepted way when some i2c controllers are disabled in devicetree.
In similar fashion, the SOC code makes assumptions for pads, configuring pins to i2c use and excepts platform code to override those. Shouldn't it be the other way around such that mainboard/variant has (full) control of the pads?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42523
to look at the new patch set (#9).
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
soc/amd/common,picasso: Place some ENV_X86 guards
Base address symbols for ACPIMMIO banks that would not get assigned at runtime must not resolve at linker-stage either.
The build of PSP-verstage should pass without the preprocessor macros that have x86-centric view of memory space.
Change-Id: I3cb1b5a90023ebc4359835be716c5e3f9451df60 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/picasso/include/soc/iomap.h 2 files changed, 20 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/42523/9
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/common/block/ac... PS7, Line 32: DECLARE_ACPIMMIO_BANK
No reason to use DECLARE_ACPIMMIO_BANK in this block.
In the lack of psp-verstage, I had experimented with the combination of ENV_X86 && !CONSTANT_ACPIMMIO_BASE_ADDRESS to see possible impacts on code size and aliasing MMIO writes.
Without const qualifier I would need to drop value assignment, else it needs .data section. So MAYBE_CONST alone is not a solution here either.
If new mappings appear to PSP servicecalls we would need to add these back one-by-one.
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/psp_verstage_stub.c:
PS7:
I would just remove these since we can directly set acpimmio_iomux from psp_verstage.
It was just a stub anyways. Seemed like psp_verstage is architected such that a static function needs to be added for simple variable assignments.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 9:
(1 comment)
Furquan, patchset #7 had GPIO / PADs related question.
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/picasso/i2c.c@5... PS7, Line 55: switch (dev->path.mmio.addr) {
I did not look deep into this yet but looks to me that i2c controller enablement does not respond th […]
This patchset no longer touches this file.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 11:
Matt, a series of changes has been submitted to soc/amd/common. Could you check with some stoneyridge chromebook against possible regressions. Or if you at Google do so, please mention it.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/11/src/soc/amd/common/block/a... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42523/11/src/soc/amd/common/block/a... PS11, Line 11: CONSTANT_ACPIMMIO_BASE_ADDRESS These two are redundant. How about we remove CONSTANT_ACPIMMIO_BASE_ADDRESS ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/11/src/soc/amd/common/block/a... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42523/11/src/soc/amd/common/block/a... PS11, Line 11: CONSTANT_ACPIMMIO_BASE_ADDRESS
These two are redundant. […]
ENV_X86 comes from the need of ACPIMMIO_BASE() below and CONSTANT_xx comes from use of const below.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/common/block/ac... File src/soc/amd/common/block/acpimmio/mmio_util.c:
https://review.coreboot.org/c/coreboot/+/42523/7/src/soc/amd/common/block/ac... PS7, Line 32: DECLARE_ACPIMMIO_BANK
In the lack of psp-verstage, I had experimented with the combination of ENV_X86 && !CONSTANT_ACPIMMI […]
Ack
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Abandoned
Aaron Durbin has restored this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Restored
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 13: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG@7 PS13, Line 7: soc/amd/common,picasso: Place some ENV_X86 guards Rebase to current master before submit. Lots of moving parts there.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG@7 PS13, Line 7: soc/amd/common,picasso: Place some ENV_X86 guards
Rebase to current master before submit. Lots of moving parts there.
Are you going to do that? Or was that a suggestion to myself?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG@7 PS13, Line 7: soc/amd/common,picasso: Place some ENV_X86 guards
Are you going to do that? Or was that a suggestion to myself?
I won't work on this. You'll likely want CB:42894 and dependencies submitted for a clean rebase and additionally CB:43307 fixed or CB:43328 restored. And I think AOAC is now also mapped for psp-verstage.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG@7 PS13, Line 7: soc/amd/common,picasso: Place some ENV_X86 guards
I won't work on this. […]
OK. Lemme take a look.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 13: Code-Review+2
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Abandoned
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Restored
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42523/13//COMMIT_MSG@7 PS13, Line 7: soc/amd/common,picasso: Place some ENV_X86 guards
OK. Lemme take a look.
all dependencies have landed, so i'll mark this as resolved and submit the patch
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42523 )
Change subject: soc/amd/common,picasso: Place some ENV_X86 guards ......................................................................
soc/amd/common,picasso: Place some ENV_X86 guards
Base address symbols for ACPIMMIO banks that would not get assigned at runtime must not resolve at linker-stage either.
The build of PSP-verstage should pass without the preprocessor macros that have x86-centric view of memory space.
Change-Id: I3cb1b5a90023ebc4359835be716c5e3f9451df60 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42523 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/soc/amd/common/block/acpimmio/mmio_util.c M src/soc/amd/picasso/include/soc/iomap.h 2 files changed, 20 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: 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 b2df8e7..9e8342b 100644 --- a/src/soc/amd/common/block/acpimmio/mmio_util.c +++ b/src/soc/amd/common/block/acpimmio/mmio_util.c @@ -2,16 +2,25 @@
#include <types.h> #include <arch/io.h> -#include <amdblocks/acpimmio_map.h> #include <amdblocks/acpimmio.h>
-#if CONSTANT_ACPIMMIO_BASE_ADDRESS +#if ENV_X86 +#include <amdblocks/acpimmio_map.h> +#endif + +#if ENV_X86 && CONSTANT_ACPIMMIO_BASE_ADDRESS #define DECLARE_ACPIMMIO(ptr, bank) \ uint8_t *const ptr = (void *)(uintptr_t)ACPIMMIO_BASE(bank) #else #define DECLARE_ACPIMMIO(ptr, bank) uint8_t *ptr #endif
+DECLARE_ACPIMMIO(acpimmio_aoac, AOAC); +DECLARE_ACPIMMIO(acpimmio_iomux, IOMUX); +DECLARE_ACPIMMIO(acpimmio_gpio0, GPIO0); +DECLARE_ACPIMMIO(acpimmio_misc, MISC); + +#if ENV_X86 DECLARE_ACPIMMIO(acpimmio_sm_pci, SM_PCI); DECLARE_ACPIMMIO(acpimmio_gpio_100, GPIO_100); DECLARE_ACPIMMIO(acpimmio_smi, SMI); @@ -25,15 +34,12 @@ 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); +#endif
#undef DECLARE_ACPIMMIO
diff --git a/src/soc/amd/picasso/include/soc/iomap.h b/src/soc/amd/picasso/include/soc/iomap.h index 43e5658..0296c87 100644 --- a/src/soc/amd/picasso/include/soc/iomap.h +++ b/src/soc/amd/picasso/include/soc/iomap.h @@ -3,6 +3,8 @@ #ifndef AMD_PICASSO_IOMAP_H #define AMD_PICASSO_IOMAP_H
+#if ENV_X86 + /* MMIO Ranges */ /* IO_APIC_ADDR defined in arch/x86 0xfec00000 */ #define GNB_IO_APIC_ADDR 0xfec01000 @@ -22,6 +24,8 @@
/* Reserved 0xfecd1000-0xfedc3fff */
+#endif /* ENV_X86 */ + /* * Picasso/Dali have I2C0 and I2C1 wired to the Sensor Fusion Hub (SFH/MP2). * The controllers are not directly accessible via the x86. @@ -37,6 +41,8 @@ #define I2C_MASTER_START_INDEX 2 #define I2C_SLAVE_DEV_COUNT 1
+#if ENV_X86 + #define APU_I2C2_BASE 0xfedc4000 #define APU_I2C3_BASE 0xfedc5000 #define APU_I2C4_BASE 0xfedc6000 @@ -62,6 +68,8 @@
#define FLASH_BASE_ADDR ((0xffffffff - CONFIG_ROM_SIZE) + 1)
+#endif /* ENV_X86 */ + /* I/O Ranges */ #define ACPI_SMI_CTL_PORT 0xb2 #define PICASSO_ACPI_IO_BASE CONFIG_PICASSO_ACPI_IO_BASE
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Kyösti Mälkki (Code Review)
-
Raul Rangel (Code Review)