Change in coreboot[master]: soc/amd: factor out common AOAC device enable and status query functions
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48183 )
Change subject: soc/amd: factor out common AOAC device enable and status query functions ......................................................................
soc/amd: factor out common AOAC device enable and status query functions
The code on Stoneyridge didn't set the FCH_AOAC_TARGET_DEVICE_STATE bits to FCH_AOAC_D0_INITIALIZED like the code for Picasso does, but that is the default value after reset for those bits on both platforms.
Change-Id: I7cae23257ae54da73b713fe88aca5edfa4656754 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- A src/soc/amd/common/block/aoac/Kconfig A src/soc/amd/common/block/aoac/Makefile.inc A src/soc/amd/common/block/aoac/aoac.c M src/soc/amd/common/block/include/amdblocks/aoac.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/aoac.c M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/uart.c M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/southbridge.c 10 files changed, 60 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/48183/1
diff --git a/src/soc/amd/common/block/aoac/Kconfig b/src/soc/amd/common/block/aoac/Kconfig new file mode 100644 index 0000000..361a46e --- /dev/null +++ b/src/soc/amd/common/block/aoac/Kconfig @@ -0,0 +1,6 @@ +config SOC_AMD_COMMON_BLOCK_AOAC + bool + default n + help + Select this option to add the common functions for the AOAC block to + the build. diff --git a/src/soc/amd/common/block/aoac/Makefile.inc b/src/soc/amd/common/block/aoac/Makefile.inc new file mode 100644 index 0000000..4debc55 --- /dev/null +++ b/src/soc/amd/common/block/aoac/Makefile.inc @@ -0,0 +1,8 @@ +ifeq ($(CONFIG_SOC_AMD_COMMON_BLOCK_AOAC),y) + +bootblock-y += aoac.c +romstage-y += aoac.c +verstage-y += aoac.c +ramstage-y += aoac.c + +endif # CONFIG_SOC_AMD_COMMON_BLOCK_AOAC diff --git a/src/soc/amd/common/block/aoac/aoac.c b/src/soc/amd/common/block/aoac/aoac.c new file mode 100644 index 0000000..25f4642 --- /dev/null +++ b/src/soc/amd/common/block/aoac/aoac.c @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <stdint.h> +#include <amdblocks/acpimmio.h> +#include <amdblocks/aoac.h> + +void power_on_aoac_device(unsigned int dev) +{ + uint8_t byte; + + /* Power on the UART and AMBA devices */ + byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); + byte |= FCH_AOAC_PWR_ON_DEV; + byte &= ~FCH_AOAC_TARGET_DEVICE_STATE; + byte |= FCH_AOAC_D0_INITIALIZED; + aoac_write8(AOAC_DEV_D3_CTL(dev), byte); +} + +void power_off_aoac_device(unsigned int dev) +{ + uint8_t byte; + + /* Power off the UART and AMBA devices */ + byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); + byte &= ~FCH_AOAC_PWR_ON_DEV; + aoac_write8(AOAC_DEV_D3_CTL(dev), byte); +} + +bool is_aoac_device_enabled(unsigned int dev) +{ + uint8_t byte; + + byte = aoac_read8(AOAC_DEV_D3_STATE(dev)); + byte &= (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE); + if (byte == (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE)) + return true; + else + return false; +} diff --git a/src/soc/amd/common/block/include/amdblocks/aoac.h b/src/soc/amd/common/block/include/amdblocks/aoac.h index 81390b5..ea4da61 100644 --- a/src/soc/amd/common/block/include/amdblocks/aoac.h +++ b/src/soc/amd/common/block/include/amdblocks/aoac.h @@ -32,4 +32,8 @@ #define FCH_AOAC_STAT0 BIT(6) #define FCH_AOAC_STAT1 BIT(7)
+bool is_aoac_device_enabled(unsigned int dev); +void power_on_aoac_device(unsigned int dev); +void power_off_aoac_device(unsigned int dev); + #endif /* AMD_BLOCK_AOAC_H */ diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index c3995c8..9e9c99a 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -35,6 +35,7 @@ select SOC_AMD_COMMON_BLOCK_ACPIMMIO select SOC_AMD_COMMON_BLOCK_BANKED_GPIOS select SOC_AMD_COMMON_BLOCK_ACPI + select SOC_AMD_COMMON_BLOCK_AOAC select SOC_AMD_COMMON_BLOCK_GRAPHICS select SOC_AMD_COMMON_BLOCK_LPC select SOC_AMD_COMMON_BLOCK_PCI diff --git a/src/soc/amd/picasso/aoac.c b/src/soc/amd/picasso/aoac.c index 21031f7..7fd839b 100644 --- a/src/soc/amd/picasso/aoac.c +++ b/src/soc/amd/picasso/aoac.c @@ -30,40 +30,6 @@ FCH_AOAC_DEV_ESPI, };
-void power_on_aoac_device(unsigned int dev) -{ - uint8_t byte; - - /* Power on the UART and AMBA devices */ - byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); - byte |= FCH_AOAC_PWR_ON_DEV; - byte &= ~FCH_AOAC_TARGET_DEVICE_STATE; - byte |= FCH_AOAC_D0_INITIALIZED; - aoac_write8(AOAC_DEV_D3_CTL(dev), byte); -} - -void power_off_aoac_device(unsigned int dev) -{ - uint8_t byte; - - /* Power off the UART and AMBA devices */ - byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); - byte &= ~FCH_AOAC_PWR_ON_DEV; - aoac_write8(AOAC_DEV_D3_CTL(dev), byte); -} - -bool is_aoac_device_enabled(unsigned int dev) -{ - uint8_t byte; - - byte = aoac_read8(AOAC_DEV_D3_STATE(dev)); - byte &= (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE); - if (byte == (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE)) - return true; - else - return false; -} - void wait_for_aoac_enabled(unsigned int dev) { while (!is_aoac_device_enabled(dev)) diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h index 8a2ae49..809eb97 100644 --- a/src/soc/amd/picasso/include/soc/southbridge.h +++ b/src/soc/amd/picasso/include/soc/southbridge.h @@ -247,9 +247,6 @@ } __packed aoac_devs_t;
void enable_aoac_devices(void); -bool is_aoac_device_enabled(unsigned int dev); -void power_on_aoac_device(unsigned int dev); -void power_off_aoac_device(unsigned int dev); void wait_for_aoac_enabled(unsigned int dev); void sb_clk_output_48Mhz(void); void sb_enable(struct device *dev); diff --git a/src/soc/amd/picasso/uart.c b/src/soc/amd/picasso/uart.c index 1aa42ef..a71a0e9 100644 --- a/src/soc/amd/picasso/uart.c +++ b/src/soc/amd/picasso/uart.c @@ -6,6 +6,7 @@ #include <device/mmio.h> #include <amdblocks/gpio_banks.h> #include <amdblocks/acpimmio.h> +#include <amdblocks/aoac.h> #include <soc/southbridge.h> #include <soc/gpio.h> #include <soc/uart.h> diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index 86df361..f24d202 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -27,6 +27,7 @@ select SOC_AMD_COMMON_BLOCK_ACPIMMIO select SOC_AMD_COMMON_BLOCK_BANKED_GPIOS select SOC_AMD_COMMON_BLOCK_ACPI + select SOC_AMD_COMMON_BLOCK_AOAC select SOC_AMD_COMMON_BLOCK_LPC select SOC_AMD_COMMON_BLOCK_PCI select SOC_AMD_COMMON_BLOCK_HDA diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index b427c18..f411179 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -146,28 +146,6 @@ return irq_association; }
-static void power_on_aoac_device(unsigned int dev) -{ - uint8_t byte; - - /* Power on the UART and AMBA devices */ - byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); - byte |= FCH_AOAC_PWR_ON_DEV; - aoac_write8(AOAC_DEV_D3_CTL(dev), byte); -} - -static bool is_aoac_device_enabled(unsigned int dev) -{ - uint8_t byte; - - byte = aoac_read8(AOAC_DEV_D3_STATE(dev)); - byte &= (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE); - if (byte == (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE)) - return true; - else - return false; -} - void enable_aoac_devices(void) { bool status;
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48183 )
Change subject: soc/amd: factor out common AOAC device enable and status query functions ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48183/1/src/soc/amd/common/block/ao... File src/soc/amd/common/block/aoac/aoac.c:
https://review.coreboot.org/c/coreboot/+/48183/1/src/soc/amd/common/block/ao... PS1, Line 6: Is it worth a comment that the device isn't guaranteed to be enabled upon exit? And can use is_aoac_device_enabled() to determine if it's up?
https://review.coreboot.org/c/coreboot/+/48183/1/src/soc/amd/common/block/ao... PS1, Line 11: UART I'm not sure why picasso mentioned UART in the comment. Probably left over from some older change. It seems out of place here. For that matter, the entire "power on..." comment seems redundant in a function named power_on...(). Same below.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48183
to look at the new patch set (#2).
Change subject: soc/amd: factor out common AOAC device enable and status query functions ......................................................................
soc/amd: factor out common AOAC device enable and status query functions
The code on Stoneyridge didn't set the FCH_AOAC_TARGET_DEVICE_STATE bits to FCH_AOAC_D0_INITIALIZED like the code for Picasso does, but that is the default value after reset for those bits on both platforms.
Change-Id: I7cae23257ae54da73b713fe88aca5edfa4656754 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- A src/soc/amd/common/block/aoac/Kconfig A src/soc/amd/common/block/aoac/Makefile.inc A src/soc/amd/common/block/aoac/aoac.c M src/soc/amd/common/block/include/amdblocks/aoac.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/aoac.c M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/uart.c M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/southbridge.c 10 files changed, 53 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/48183/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48183 )
Change subject: soc/amd: factor out common AOAC device enable and status query functions ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48183/1/src/soc/amd/common/block/ao... File src/soc/amd/common/block/aoac/aoac.c:
https://review.coreboot.org/c/coreboot/+/48183/1/src/soc/amd/common/block/ao... PS1, Line 6:
Is it worth a comment that the device isn't guaranteed to be enabled upon exit? And can use is_aoac […]
Done
https://review.coreboot.org/c/coreboot/+/48183/1/src/soc/amd/common/block/ao... PS1, Line 11: UART
I'm not sure why picasso mentioned UART in the comment. Probably left over from some older change. […]
Done
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48183 )
Change subject: soc/amd: factor out common AOAC device enable and status query functions ......................................................................
Patch Set 2: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48183 )
Change subject: soc/amd: factor out common AOAC device enable and status query functions ......................................................................
soc/amd: factor out common AOAC device enable and status query functions
The code on Stoneyridge didn't set the FCH_AOAC_TARGET_DEVICE_STATE bits to FCH_AOAC_D0_INITIALIZED like the code for Picasso does, but that is the default value after reset for those bits on both platforms.
Change-Id: I7cae23257ae54da73b713fe88aca5edfa4656754 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/48183 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- A src/soc/amd/common/block/aoac/Kconfig A src/soc/amd/common/block/aoac/Makefile.inc A src/soc/amd/common/block/aoac/aoac.c M src/soc/amd/common/block/include/amdblocks/aoac.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/aoac.c M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/uart.c M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/southbridge.c 10 files changed, 53 insertions(+), 59 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/src/soc/amd/common/block/aoac/Kconfig b/src/soc/amd/common/block/aoac/Kconfig new file mode 100644 index 0000000..361a46e --- /dev/null +++ b/src/soc/amd/common/block/aoac/Kconfig @@ -0,0 +1,6 @@ +config SOC_AMD_COMMON_BLOCK_AOAC + bool + default n + help + Select this option to add the common functions for the AOAC block to + the build. diff --git a/src/soc/amd/common/block/aoac/Makefile.inc b/src/soc/amd/common/block/aoac/Makefile.inc new file mode 100644 index 0000000..4debc55 --- /dev/null +++ b/src/soc/amd/common/block/aoac/Makefile.inc @@ -0,0 +1,8 @@ +ifeq ($(CONFIG_SOC_AMD_COMMON_BLOCK_AOAC),y) + +bootblock-y += aoac.c +romstage-y += aoac.c +verstage-y += aoac.c +ramstage-y += aoac.c + +endif # CONFIG_SOC_AMD_COMMON_BLOCK_AOAC diff --git a/src/soc/amd/common/block/aoac/aoac.c b/src/soc/amd/common/block/aoac/aoac.c new file mode 100644 index 0000000..c5f161b --- /dev/null +++ b/src/soc/amd/common/block/aoac/aoac.c @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <stdint.h> +#include <amdblocks/acpimmio.h> +#include <amdblocks/aoac.h> + +/* This initiates the power on sequence, but doesn't wait for the device to be powered on. */ +void power_on_aoac_device(unsigned int dev) +{ + uint8_t byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); + byte |= FCH_AOAC_PWR_ON_DEV; + byte &= ~FCH_AOAC_TARGET_DEVICE_STATE; + byte |= FCH_AOAC_D0_INITIALIZED; + aoac_write8(AOAC_DEV_D3_CTL(dev), byte); +} + +void power_off_aoac_device(unsigned int dev) +{ + uint8_t byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); + byte &= ~FCH_AOAC_PWR_ON_DEV; + aoac_write8(AOAC_DEV_D3_CTL(dev), byte); +} + +bool is_aoac_device_enabled(unsigned int dev) +{ + uint8_t byte = aoac_read8(AOAC_DEV_D3_STATE(dev)); + byte &= (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE); + if (byte == (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE)) + return true; + else + return false; +} diff --git a/src/soc/amd/common/block/include/amdblocks/aoac.h b/src/soc/amd/common/block/include/amdblocks/aoac.h index f4f58e6..ff5d0db 100644 --- a/src/soc/amd/common/block/include/amdblocks/aoac.h +++ b/src/soc/amd/common/block/include/amdblocks/aoac.h @@ -32,4 +32,8 @@ #define FCH_AOAC_STAT0 BIT(6) #define FCH_AOAC_STAT1 BIT(7)
+bool is_aoac_device_enabled(unsigned int dev); +void power_on_aoac_device(unsigned int dev); +void power_off_aoac_device(unsigned int dev); + #endif /* AMD_BLOCK_AOAC_H */ diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 7f32c3c..e2feebd 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -35,6 +35,7 @@ select SOC_AMD_COMMON_BLOCK_ACPIMMIO select SOC_AMD_COMMON_BLOCK_BANKED_GPIOS select SOC_AMD_COMMON_BLOCK_ACPI + select SOC_AMD_COMMON_BLOCK_AOAC select SOC_AMD_COMMON_BLOCK_GRAPHICS select SOC_AMD_COMMON_BLOCK_LPC select SOC_AMD_COMMON_BLOCK_PCI diff --git a/src/soc/amd/picasso/aoac.c b/src/soc/amd/picasso/aoac.c index 21031f7..7fd839b 100644 --- a/src/soc/amd/picasso/aoac.c +++ b/src/soc/amd/picasso/aoac.c @@ -30,40 +30,6 @@ FCH_AOAC_DEV_ESPI, };
-void power_on_aoac_device(unsigned int dev) -{ - uint8_t byte; - - /* Power on the UART and AMBA devices */ - byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); - byte |= FCH_AOAC_PWR_ON_DEV; - byte &= ~FCH_AOAC_TARGET_DEVICE_STATE; - byte |= FCH_AOAC_D0_INITIALIZED; - aoac_write8(AOAC_DEV_D3_CTL(dev), byte); -} - -void power_off_aoac_device(unsigned int dev) -{ - uint8_t byte; - - /* Power off the UART and AMBA devices */ - byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); - byte &= ~FCH_AOAC_PWR_ON_DEV; - aoac_write8(AOAC_DEV_D3_CTL(dev), byte); -} - -bool is_aoac_device_enabled(unsigned int dev) -{ - uint8_t byte; - - byte = aoac_read8(AOAC_DEV_D3_STATE(dev)); - byte &= (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE); - if (byte == (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE)) - return true; - else - return false; -} - void wait_for_aoac_enabled(unsigned int dev) { while (!is_aoac_device_enabled(dev)) diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h index 8a2ae49..809eb97 100644 --- a/src/soc/amd/picasso/include/soc/southbridge.h +++ b/src/soc/amd/picasso/include/soc/southbridge.h @@ -247,9 +247,6 @@ } __packed aoac_devs_t;
void enable_aoac_devices(void); -bool is_aoac_device_enabled(unsigned int dev); -void power_on_aoac_device(unsigned int dev); -void power_off_aoac_device(unsigned int dev); void wait_for_aoac_enabled(unsigned int dev); void sb_clk_output_48Mhz(void); void sb_enable(struct device *dev); diff --git a/src/soc/amd/picasso/uart.c b/src/soc/amd/picasso/uart.c index 1aa42ef..a71a0e9 100644 --- a/src/soc/amd/picasso/uart.c +++ b/src/soc/amd/picasso/uart.c @@ -6,6 +6,7 @@ #include <device/mmio.h> #include <amdblocks/gpio_banks.h> #include <amdblocks/acpimmio.h> +#include <amdblocks/aoac.h> #include <soc/southbridge.h> #include <soc/gpio.h> #include <soc/uart.h> diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index 86df361..f24d202 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -27,6 +27,7 @@ select SOC_AMD_COMMON_BLOCK_ACPIMMIO select SOC_AMD_COMMON_BLOCK_BANKED_GPIOS select SOC_AMD_COMMON_BLOCK_ACPI + select SOC_AMD_COMMON_BLOCK_AOAC select SOC_AMD_COMMON_BLOCK_LPC select SOC_AMD_COMMON_BLOCK_PCI select SOC_AMD_COMMON_BLOCK_HDA diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c index b427c18..f411179 100644 --- a/src/soc/amd/stoneyridge/southbridge.c +++ b/src/soc/amd/stoneyridge/southbridge.c @@ -146,28 +146,6 @@ return irq_association; }
-static void power_on_aoac_device(unsigned int dev) -{ - uint8_t byte; - - /* Power on the UART and AMBA devices */ - byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); - byte |= FCH_AOAC_PWR_ON_DEV; - aoac_write8(AOAC_DEV_D3_CTL(dev), byte); -} - -static bool is_aoac_device_enabled(unsigned int dev) -{ - uint8_t byte; - - byte = aoac_read8(AOAC_DEV_D3_STATE(dev)); - byte &= (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE); - if (byte == (FCH_AOAC_PWR_RST_STATE | FCH_AOAC_RST_CLK_OK_STATE)) - return true; - else - return false; -} - void enable_aoac_devices(void) { bool status;
-
Felix Held (Code Review) -
Marshall Dawson (Code Review)