Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35296 )
Change subject: soc/amd/picasso: Refactor AOAC enabling ......................................................................
soc/amd/picasso: Refactor AOAC enabling
Replace the raw register definitions with device numbers and macros for determining the register offsets. Rewrite the source to refer to AOAC device numbers instead of a structure.
Remove the calculated offset for the console UART. Picasso's UARTs are not contiguous so handle them separately.
Change-Id: Iffc87f39ebe38394a56d41bb0940e9701fd05db9 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/southbridge.c 2 files changed, 57 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/35296/1
diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h index 803c638..dad4358 100644 --- a/src/soc/amd/picasso/include/soc/southbridge.h +++ b/src/soc/amd/picasso/include/soc/southbridge.h @@ -206,14 +206,18 @@ #define I2C_PAD_CTRL_SPARE1 BIT(18)
/* FCH AOAC Registers 0xfed81e00 */ -#define FCH_AOAC_D3_CONTROL_CLK_GEN 0x40 -#define FCH_AOAC_D3_CONTROL_I2C2 0x4e -#define FCH_AOAC_D3_CONTROL_I2C3 0x50 -#define FCH_AOAC_D3_CONTROL_I2C4 0x52 -#define FCH_AOAC_D3_CONTROL_UART0 0x56 -#define FCH_AOAC_D3_CONTROL_UART1 0x58 -#define FCH_AOAC_D3_CONTROL_AMBA 0x62 -/* Bit definitions for all FCH_AOAC_D3_CONTROL_* Registers */ +#define AOAC_DEV_D3_CTL(device) (0x40 + device * 2) +#define AOAC_DEV_D3_STATE(device) (AOAC_DEV_D3_CTL(device) + 1) + +#define FCH_AOAC_DEV_CLK_GEN 0 +#define FCH_AOAC_DEV_I2C2 7 +#define FCH_AOAC_DEV_I2C3 8 +#define FCH_AOAC_DEV_I2C4 9 +#define FCH_AOAC_DEV_UART0 11 +#define FCH_AOAC_DEV_UART1 12 +#define FCH_AOAC_DEV_AMBA 17 + +/* Bit definitions for Device D3 Control AOACx0000[40...7E] step 2 */ #define FCH_AOAC_TARGET_DEVICE_STATE (BIT(0) + BIT(1)) #define FCH_AOAC_DEVICE_STATE BIT(2) #define FCH_AOAC_PWR_ON_DEV BIT(3) @@ -222,14 +226,7 @@ #define FCH_AOAC_SW_RST_B BIT(6) #define FCH_AOAC_IS_SW_CONTROL BIT(7)
-#define FCH_AOAC_D3_STATE_CLK_GEN 0x41 -#define FCH_AOAC_D3_STATE_I2C2 0x4f -#define FCH_AOAC_D3_STATE_I2C3 0x51 -#define FCH_AOAC_D3_STATE_I2C4 0x53 -#define FCH_AOAC_D3_STATE_UART0 0x57 -#define FCH_AOAC_D3_STATE_UART1 0x59 -#define FCH_AOAC_D3_STATE_AMBA 0x63 -/* Bit definitions for all FCH_AOAC_D3_STATE_* Registers */ +/* Bit definitions for Device D3 State AOACx0000[41...7f] step 2 */ #define FCH_AOAC_PWR_RST_STATE BIT(0) #define FCH_AOAC_RST_CLK_OK_STATE BIT(1) #define FCH_AOAC_RST_B_STATE BIT(2) @@ -307,11 +304,6 @@ /* IO 0xf0 NCP Error */ #define NCP_WARM_BOOT BIT(7) /* Write-once */
-struct picasso_aoac { - int enable; - int status; -}; - typedef struct aoac_devs { unsigned int :7; unsigned int ic2e:1; /* 7: I2C2 */ diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 582034a..e6fc0de 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -36,19 +36,25 @@ #include <soc/nvs.h> #include <types.h>
+#define FCH_AOAC_UART_FOR_CONSOLE \ + (CONFIG_UART_FOR_CONSOLE == 0 ? FCH_AOAC_DEV_UART0 \ + : CONFIG_UART_FOR_CONSOLE == 1 ? FCH_AOAC_DEV_UART1 \ + : -1) +#if FCH_AOAC_UART_FOR_CONSOLE == -1 +# error Unsupported UART_FOR_CONSOLE chosen +#endif + /* * Table of devices that need their AOAC registers enabled and waited * upon (usually about .55 milliseconds). Instead of individual delays * waiting for each device to become available, a single delay will be - * executed. + * executed. The console UART is handled separately from this table. */ -const static struct picasso_aoac aoac_devs[] = { - { (FCH_AOAC_D3_CONTROL_UART0 + CONFIG_UART_FOR_CONSOLE * 2), - (FCH_AOAC_D3_STATE_UART0 + CONFIG_UART_FOR_CONSOLE * 2) }, - { FCH_AOAC_D3_CONTROL_AMBA, FCH_AOAC_D3_STATE_AMBA }, - { FCH_AOAC_D3_CONTROL_I2C2, FCH_AOAC_D3_STATE_I2C2 }, - { FCH_AOAC_D3_CONTROL_I2C3, FCH_AOAC_D3_STATE_I2C3 }, - { FCH_AOAC_D3_CONTROL_I2C4, FCH_AOAC_D3_STATE_I2C4 } +const static int aoac_devs[] = { + FCH_AOAC_DEV_AMBA, + FCH_AOAC_DEV_I2C2, + FCH_AOAC_DEV_I2C3, + FCH_AOAC_DEV_I2C4, };
/* @@ -101,21 +107,21 @@ return irq_association; }
-static void power_on_aoac_device(int aoac_device_control_register) +static void power_on_aoac_device(int dev) { uint8_t byte;
/* Power on the UART and AMBA devices */ - byte = aoac_read8(aoac_device_control_register); + byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); byte |= FCH_AOAC_PWR_ON_DEV; - aoac_write8(aoac_device_control_register, byte); + aoac_write8(AOAC_DEV_D3_CTL(dev), byte); }
-static bool is_aoac_device_enabled(int aoac_device_status_register) +static bool is_aoac_device_enabled(int dev) { uint8_t byte;
- byte = aoac_read8(aoac_device_status_register); + 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; @@ -123,20 +129,38 @@ return false; }
+static void enable_aoac_console_uart(void) +{ + if (!CONFIG(PICASSO_UART)) + return; + + power_on_aoac_device(FCH_AOAC_UART_FOR_CONSOLE); +} + +static bool is_aoac_console_uart_enabled(void) +{ + if (!CONFIG(PICASSO_UART)) + return true; + + return is_aoac_device_enabled(FCH_AOAC_UART_FOR_CONSOLE); +} + void enable_aoac_devices(void) { bool status; int i;
for (i = 0; i < ARRAY_SIZE(aoac_devs); i++) - power_on_aoac_device(aoac_devs[i].enable); + power_on_aoac_device(aoac_devs[i]); + enable_aoac_console_uart();
/* Wait for AOAC devices to indicate power and clock OK */ do { udelay(100); status = true; for (i = 0; i < ARRAY_SIZE(aoac_devs); i++) - status &= is_aoac_device_enabled(aoac_devs[i].status); + status &= is_aoac_device_enabled(aoac_devs[i]); + status &= is_aoac_console_uart_enabled(); } while (!status); }
@@ -513,11 +537,11 @@ if (gnvs == NULL) return;
- gnvs->aoac.ic2e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_I2C2); - gnvs->aoac.ic3e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_I2C3); - gnvs->aoac.ic4e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_I2C4); - gnvs->aoac.ut0e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_UART0); - gnvs->aoac.ut1e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_UART1); + gnvs->aoac.ic2e = is_aoac_device_enabled(FCH_AOAC_DEV_I2C2); + gnvs->aoac.ic3e = is_aoac_device_enabled(FCH_AOAC_DEV_I2C3); + gnvs->aoac.ic4e = is_aoac_device_enabled(FCH_AOAC_DEV_I2C4); + gnvs->aoac.ut0e = is_aoac_device_enabled(FCH_AOAC_DEV_UART0); + gnvs->aoac.ut1e = is_aoac_device_enabled(FCH_AOAC_DEV_UART1); /* Rely on these being in sync with devicetree */ sata = pcidev_path_on_root(SATA_DEVFN); gnvs->aoac.st_e = sata && sata->enabled ? 1 : 0;
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35296 )
Change subject: soc/amd/picasso: Refactor AOAC enabling ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35296/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35296/1//COMMIT_MSG@13 PS1, Line 13: Picasso's UARTs UART0 and UART1 are... and you code enforces UART0 or UART1. If your code also accepted the other UARTs, then this would be accurate. Why not simply say that UART enabling is now controlled by a config parameter?
https://review.coreboot.org/c/coreboot/+/35296/1/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/35296/1/src/soc/amd/picasso/southbr... PS1, Line 52: */ Maybe add why? I had to look the code below to understand it had a control through config PICASSO_UART. Just a suggestion, I accept either way.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35296 )
Change subject: soc/amd/picasso: Refactor AOAC enabling ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35296/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35296/1//COMMIT_MSG@13 PS1, Line 13: Picasso's UARTs Keep in mind this is primarily affecting the copy of stoneyridge code. The patch that updates picasso for its UARTs comes later in https://review.coreboot.org/c/coreboot/+/33767/14 I chose to remove the calculation here so that the update looks cleaner. UART 0/1/2/3 are devices 11/12/16/26.
Why not simply say that UART enabling is now controlled by a config parameter?
If anything, an APU UART was being turned on incorrectly when it wasn't supposed to be. Do you want me to add a note that it corrects this condition?
https://review.coreboot.org/c/coreboot/+/35296/1/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/35296/1/src/soc/amd/picasso/southbr... PS1, Line 52: */
Maybe add why? I had to look the code below to understand it had a control through config PICASSO_UA […]
The comment is really meant to a caution to anyone who may try to add their UART to the aoac_devs list without first seeing what the code does. Reading the source below, it's pretty evident IMO that the console UART is only turned on when CONFIG(PICASSO_UART) is enabled.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35296 )
Change subject: soc/amd/picasso: Refactor AOAC enabling ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/35296/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35296/1//COMMIT_MSG@13 PS1, Line 13: Picasso's UARTs
Keep in mind this is primarily affecting the copy of stoneyridge code. […]
No need, your choice.
https://review.coreboot.org/c/coreboot/+/35296/1/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/35296/1/src/soc/amd/picasso/southbr... PS1, Line 52: */
The comment is really meant to a caution to anyone who may try to add their UART to the aoac_devs li […]
Ack
Marshall Dawson has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35296 )
Change subject: soc/amd/picasso: Refactor AOAC enabling ......................................................................
soc/amd/picasso: Refactor AOAC enabling
Replace the raw register definitions with device numbers and macros for determining the register offsets. Rewrite the source to refer to AOAC device numbers instead of a structure.
Remove the calculated offset for the console UART. Picasso's UARTs are not contiguous so handle them separately.
Change-Id: Iffc87f39ebe38394a56d41bb0940e9701fd05db9 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35296 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/picasso/include/soc/southbridge.h M src/soc/amd/picasso/southbridge.c 2 files changed, 57 insertions(+), 41 deletions(-)
Approvals: build bot (Jenkins): Verified Richard Spiegel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h index 803c638..dad4358 100644 --- a/src/soc/amd/picasso/include/soc/southbridge.h +++ b/src/soc/amd/picasso/include/soc/southbridge.h @@ -206,14 +206,18 @@ #define I2C_PAD_CTRL_SPARE1 BIT(18)
/* FCH AOAC Registers 0xfed81e00 */ -#define FCH_AOAC_D3_CONTROL_CLK_GEN 0x40 -#define FCH_AOAC_D3_CONTROL_I2C2 0x4e -#define FCH_AOAC_D3_CONTROL_I2C3 0x50 -#define FCH_AOAC_D3_CONTROL_I2C4 0x52 -#define FCH_AOAC_D3_CONTROL_UART0 0x56 -#define FCH_AOAC_D3_CONTROL_UART1 0x58 -#define FCH_AOAC_D3_CONTROL_AMBA 0x62 -/* Bit definitions for all FCH_AOAC_D3_CONTROL_* Registers */ +#define AOAC_DEV_D3_CTL(device) (0x40 + device * 2) +#define AOAC_DEV_D3_STATE(device) (AOAC_DEV_D3_CTL(device) + 1) + +#define FCH_AOAC_DEV_CLK_GEN 0 +#define FCH_AOAC_DEV_I2C2 7 +#define FCH_AOAC_DEV_I2C3 8 +#define FCH_AOAC_DEV_I2C4 9 +#define FCH_AOAC_DEV_UART0 11 +#define FCH_AOAC_DEV_UART1 12 +#define FCH_AOAC_DEV_AMBA 17 + +/* Bit definitions for Device D3 Control AOACx0000[40...7E] step 2 */ #define FCH_AOAC_TARGET_DEVICE_STATE (BIT(0) + BIT(1)) #define FCH_AOAC_DEVICE_STATE BIT(2) #define FCH_AOAC_PWR_ON_DEV BIT(3) @@ -222,14 +226,7 @@ #define FCH_AOAC_SW_RST_B BIT(6) #define FCH_AOAC_IS_SW_CONTROL BIT(7)
-#define FCH_AOAC_D3_STATE_CLK_GEN 0x41 -#define FCH_AOAC_D3_STATE_I2C2 0x4f -#define FCH_AOAC_D3_STATE_I2C3 0x51 -#define FCH_AOAC_D3_STATE_I2C4 0x53 -#define FCH_AOAC_D3_STATE_UART0 0x57 -#define FCH_AOAC_D3_STATE_UART1 0x59 -#define FCH_AOAC_D3_STATE_AMBA 0x63 -/* Bit definitions for all FCH_AOAC_D3_STATE_* Registers */ +/* Bit definitions for Device D3 State AOACx0000[41...7f] step 2 */ #define FCH_AOAC_PWR_RST_STATE BIT(0) #define FCH_AOAC_RST_CLK_OK_STATE BIT(1) #define FCH_AOAC_RST_B_STATE BIT(2) @@ -307,11 +304,6 @@ /* IO 0xf0 NCP Error */ #define NCP_WARM_BOOT BIT(7) /* Write-once */
-struct picasso_aoac { - int enable; - int status; -}; - typedef struct aoac_devs { unsigned int :7; unsigned int ic2e:1; /* 7: I2C2 */ diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 582034a..e6fc0de 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -36,19 +36,25 @@ #include <soc/nvs.h> #include <types.h>
+#define FCH_AOAC_UART_FOR_CONSOLE \ + (CONFIG_UART_FOR_CONSOLE == 0 ? FCH_AOAC_DEV_UART0 \ + : CONFIG_UART_FOR_CONSOLE == 1 ? FCH_AOAC_DEV_UART1 \ + : -1) +#if FCH_AOAC_UART_FOR_CONSOLE == -1 +# error Unsupported UART_FOR_CONSOLE chosen +#endif + /* * Table of devices that need their AOAC registers enabled and waited * upon (usually about .55 milliseconds). Instead of individual delays * waiting for each device to become available, a single delay will be - * executed. + * executed. The console UART is handled separately from this table. */ -const static struct picasso_aoac aoac_devs[] = { - { (FCH_AOAC_D3_CONTROL_UART0 + CONFIG_UART_FOR_CONSOLE * 2), - (FCH_AOAC_D3_STATE_UART0 + CONFIG_UART_FOR_CONSOLE * 2) }, - { FCH_AOAC_D3_CONTROL_AMBA, FCH_AOAC_D3_STATE_AMBA }, - { FCH_AOAC_D3_CONTROL_I2C2, FCH_AOAC_D3_STATE_I2C2 }, - { FCH_AOAC_D3_CONTROL_I2C3, FCH_AOAC_D3_STATE_I2C3 }, - { FCH_AOAC_D3_CONTROL_I2C4, FCH_AOAC_D3_STATE_I2C4 } +const static int aoac_devs[] = { + FCH_AOAC_DEV_AMBA, + FCH_AOAC_DEV_I2C2, + FCH_AOAC_DEV_I2C3, + FCH_AOAC_DEV_I2C4, };
/* @@ -101,21 +107,21 @@ return irq_association; }
-static void power_on_aoac_device(int aoac_device_control_register) +static void power_on_aoac_device(int dev) { uint8_t byte;
/* Power on the UART and AMBA devices */ - byte = aoac_read8(aoac_device_control_register); + byte = aoac_read8(AOAC_DEV_D3_CTL(dev)); byte |= FCH_AOAC_PWR_ON_DEV; - aoac_write8(aoac_device_control_register, byte); + aoac_write8(AOAC_DEV_D3_CTL(dev), byte); }
-static bool is_aoac_device_enabled(int aoac_device_status_register) +static bool is_aoac_device_enabled(int dev) { uint8_t byte;
- byte = aoac_read8(aoac_device_status_register); + 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; @@ -123,20 +129,38 @@ return false; }
+static void enable_aoac_console_uart(void) +{ + if (!CONFIG(PICASSO_UART)) + return; + + power_on_aoac_device(FCH_AOAC_UART_FOR_CONSOLE); +} + +static bool is_aoac_console_uart_enabled(void) +{ + if (!CONFIG(PICASSO_UART)) + return true; + + return is_aoac_device_enabled(FCH_AOAC_UART_FOR_CONSOLE); +} + void enable_aoac_devices(void) { bool status; int i;
for (i = 0; i < ARRAY_SIZE(aoac_devs); i++) - power_on_aoac_device(aoac_devs[i].enable); + power_on_aoac_device(aoac_devs[i]); + enable_aoac_console_uart();
/* Wait for AOAC devices to indicate power and clock OK */ do { udelay(100); status = true; for (i = 0; i < ARRAY_SIZE(aoac_devs); i++) - status &= is_aoac_device_enabled(aoac_devs[i].status); + status &= is_aoac_device_enabled(aoac_devs[i]); + status &= is_aoac_console_uart_enabled(); } while (!status); }
@@ -513,11 +537,11 @@ if (gnvs == NULL) return;
- gnvs->aoac.ic2e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_I2C2); - gnvs->aoac.ic3e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_I2C3); - gnvs->aoac.ic4e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_I2C4); - gnvs->aoac.ut0e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_UART0); - gnvs->aoac.ut1e = is_aoac_device_enabled(FCH_AOAC_D3_STATE_UART1); + gnvs->aoac.ic2e = is_aoac_device_enabled(FCH_AOAC_DEV_I2C2); + gnvs->aoac.ic3e = is_aoac_device_enabled(FCH_AOAC_DEV_I2C3); + gnvs->aoac.ic4e = is_aoac_device_enabled(FCH_AOAC_DEV_I2C4); + gnvs->aoac.ut0e = is_aoac_device_enabled(FCH_AOAC_DEV_UART0); + gnvs->aoac.ut1e = is_aoac_device_enabled(FCH_AOAC_DEV_UART1); /* Rely on these being in sync with devicetree */ sata = pcidev_path_on_root(SATA_DEVFN); gnvs->aoac.st_e = sata && sata->enabled ? 1 : 0;
-
Marshall Dawson (Code Review) -
Richard Spiegel (Code Review)