Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40869
to review the following change.
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
soc/amd/picasso: add Kconfig option to disable rom sharing
Add a knob for mainboards to request disablement of the SPI flash ROM sharing in the chipset. The chipset allows the board to share the SPI flash bus and needs a pin to perform the request. If the board design does not employ SPI flash ROM sharing then it's imperative to ensure this option is selected, especially if the pin is being utilized by something else in the board design.
BUG=b:153502861
Change-Id: I60ba852070dd218c4ac071b6c1cfcde2df8e5dce Signed-off-by: Aaron Durbin adurbin@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Reviewed-by: Furquan Shaikh furquan@chromium.org Commit-Queue: Aaron Durbin adurbin@google.com Tested-by: Aaron Durbin adurbin@google.com --- M src/soc/amd/common/block/include/amdblocks/lpc.h M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/southbridge.c 3 files changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/40869/1
diff --git a/src/soc/amd/common/block/include/amdblocks/lpc.h b/src/soc/amd/common/block/include/amdblocks/lpc.h index dc33073..2fabac4 100644 --- a/src/soc/amd/common/block/include/amdblocks/lpc.h +++ b/src/soc/amd/common/block/include/amdblocks/lpc.h @@ -10,6 +10,8 @@ /* PCI registers for D14F3 */ #define LPC_PCI_CONTROL 0x40 #define LEGACY_DMA_EN BIT(2) +#define VW_ROM_SHARING_EN BIT(3) +#define EXT_ROM_SHARING_EN BIT(4)
#define LPC_IO_PORT_DECODE_ENABLE 0x44 #define DECODE_ENABLE_PARALLEL_PORT0 BIT(0) diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index a37f543..a54a5f3 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -208,6 +208,14 @@ Picasso's LPC bus signals are MUXed with some of the EMMC signals. Select this option if LPC signals are required.
+config DISABLE_SPI_FLASH_ROM_SHARING + def_bool n + help + Instruct the chipset to not honor the EGPIO67_SPI_ROM_REQ pin + which indicates a board level ROM transaction request. This + removes arbitration with board and assumes the chipset controls + the SPI flash bus entirely. + config MAINBOARD_POWER_RESTORE def_bool n help diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 6bedab0..dd88ec9 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -330,11 +330,24 @@ printk(BIOS_DEBUG, "\n"); }
+static void disable_rom_sharing(void) +{ + u8 byte; + + byte = pci_read_config8(SOC_LPC_DEV, LPC_PCI_CONTROL); + byte &= ~VW_ROM_SHARING_EN; + byte &= ~EXT_ROM_SHARING_EN; + pci_write_config8(SOC_LPC_DEV, LPC_PCI_CONTROL, byte); +} + /* After console init */ void fch_early_init(void) { sb_print_pmxc0_status(); i2c_soc_early_init(); + + if (CONFIG(DISABLE_SPI_FLASH_ROM_SHARING)) + disable_rom_sharing(); }
void sb_enable(struct device *dev)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... PS1, Line 13: #define VW_ROM_SHARING_EN BIT(3) : #define EXT_ROM_SHARING_EN BIT(4) This applies to both Stoneyridge and Picasso?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... PS1, Line 13: #define VW_ROM_SHARING_EN BIT(3) : #define EXT_ROM_SHARING_EN BIT(4)
This applies to both Stoneyridge and Picasso?
3 and 4 are reserved for stoney. Do you think I should move them?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... PS1, Line 13: #define VW_ROM_SHARING_EN BIT(3) : #define EXT_ROM_SHARING_EN BIT(4)
3 and 4 are reserved for stoney. […]
Well, I guess we could add a PROVIDES_ROM_SHARING Kconfig that picasso selects. Then take appropriate action based on both Kconfigs.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... PS1, Line 13: #define VW_ROM_SHARING_EN BIT(3) : #define EXT_ROM_SHARING_EN BIT(4)
Well, I guess we could add a PROVIDES_ROM_SHARING Kconfig that picasso selects. […]
In that case we should also move the disable_rom_sharing() to common/block/lpc/lpc.c
Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40869
to look at the new patch set (#2).
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
soc/amd/picasso: add Kconfig option to disable rom sharing
Add a knob for mainboards to request disablement of the SPI flash ROM sharing in the chipset. The chipset allows the board to share the SPI flash bus and needs a pin to perform the request. If the board design does not employ SPI flash ROM sharing then it's imperative to ensure this option is selected, especially if the pin is being utilized by something else in the board design.
BUG=b:153502861
Change-Id: I60ba852070dd218c4ac071b6c1cfcde2df8e5dce Signed-off-by: Aaron Durbin adurbin@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Reviewed-by: Furquan Shaikh furquan@chromium.org Commit-Queue: Aaron Durbin adurbin@google.com Tested-by: Aaron Durbin adurbin@google.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/southbridge.c 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/40869/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Uploaded patch set 2.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... PS1, Line 13: #define VW_ROM_SHARING_EN BIT(3) : #define EXT_ROM_SHARING_EN BIT(4)
In that case we should also move the disable_rom_sharing() to common/block/lpc/lpc. […]
I added a PROVIDES_ROM_SHARING Kconfig and also moved the function.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 2: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... PS1, Line 13: #define VW_ROM_SHARING_EN BIT(3) : #define EXT_ROM_SHARING_EN BIT(4)
In that case we should also move the disable_rom_sharing() to common/block/lpc/lpc. […]
I feel better that I didn't add these defines here. I guess that seems like the proper place along w/ the proper guards.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... PS1, Line 13: #define VW_ROM_SHARING_EN BIT(3) : #define EXT_ROM_SHARING_EN BIT(4)
I feel better that I didn't add these defines here. […]
Are you asking for me to change something?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/lpc.h:
https://review.coreboot.org/c/coreboot/+/40869/1/src/soc/amd/common/block/in... PS1, Line 13: #define VW_ROM_SHARING_EN BIT(3) : #define EXT_ROM_SHARING_EN BIT(4)
Are you asking for me to change something?
No. These were some non-published comments. You can ignore.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Uploaded patch set 3: Patch Set 2 was rebased.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
Uploaded patch set 4: Patch Set 3 was rebased.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40869 )
Change subject: soc/amd/picasso: add Kconfig option to disable rom sharing ......................................................................
soc/amd/picasso: add Kconfig option to disable rom sharing
Add a knob for mainboards to request disablement of the SPI flash ROM sharing in the chipset. The chipset allows the board to share the SPI flash bus and needs a pin to perform the request. If the board design does not employ SPI flash ROM sharing then it's imperative to ensure this option is selected, especially if the pin is being utilized by something else in the board design.
BUG=b:153502861
Change-Id: I60ba852070dd218c4ac071b6c1cfcde2df8e5dce Signed-off-by: Aaron Durbin adurbin@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Reviewed-by: Furquan Shaikh furquan@chromium.org Commit-Queue: Aaron Durbin adurbin@google.com Tested-by: Aaron Durbin adurbin@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40869 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/southbridge.c 2 files changed, 12 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 3d69966..a42629b 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -40,6 +40,7 @@ select SOC_AMD_COMMON_BLOCK_SATA select SOC_AMD_COMMON_BLOCK_SMBUS select SOC_AMD_COMMON_BLOCK_PSP_GEN2 + select PROVIDES_ROM_SHARING select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH select PARALLEL_MP @@ -216,6 +217,14 @@ Picasso's LPC bus signals are MUXed with some of the EMMC signals. Select this option if LPC signals are required.
+config DISABLE_SPI_FLASH_ROM_SHARING + def_bool n + help + Instruct the chipset to not honor the EGPIO67_SPI_ROM_REQ pin + which indicates a board level ROM transaction request. This + removes arbitration with board and assumes the chipset controls + the SPI flash bus entirely. + config MAINBOARD_POWER_RESTORE def_bool n help diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 0d54294..d742038 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -360,6 +360,9 @@ { sb_print_pmxc0_status(); i2c_soc_early_init(); + + if (CONFIG(DISABLE_SPI_FLASH_ROM_SHARING)) + lpc_disable_spi_rom_sharing(); }
void sb_enable(struct device *dev)