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(a)chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/…
Reviewed-by: Furquan Shaikh <furquan(a)chromium.org>
Commit-Queue: Aaron Durbin <adurbin(a)google.com>
Tested-by: Aaron Durbin <adurbin(a)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)
--
To view, visit https://review.coreboot.org/c/coreboot/+/40869
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I60ba852070dd218c4ac071b6c1cfcde2df8e5dce
Gerrit-Change-Number: 40869
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-MessageType: newchange
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41029 )
Change subject: soc/intel/jasperlake: Allow SataEnable to be filled from devicetree
......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41029/4//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/41029/4//COMMIT_MSG@11
PS4, Line 11: it can be set as per each board's requirement.
> 1. Why can’t this be detected at run-time? […]
here in this we are changing FSP UPD value as per board's requirement. It will override FSP UPD default value. FSP will try to initialize SATA controller if this UPD is set to 1. if board is not using SATA then it is better to make this UPD off so FSP will not try to initialize SATA controller.
--
To view, visit https://review.coreboot.org/c/coreboot/+/41029
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4f7e7508b8cd483508293ee3e7b760574d8f025f
Gerrit-Change-Number: 41029
Gerrit-PatchSet: 5
Gerrit-Owner: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 05 May 2020 12:41:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: comment
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41028 )
Change subject: soc/intel/jasperlake: Correct the EMMC PCR Port ID
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/41028
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I17d4e7b7e0fe5e0b18867b6481b5bc9227ae19e3
Gerrit-Change-Number: 41028
Gerrit-PatchSet: 4
Gerrit-Owner: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 05 May 2020 08:21:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41029 )
Change subject: soc/intel/jasperlake: Allow SataEnable to be filled from devicetree
......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41029/4//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/41029/4//COMMIT_MSG@9
PS4, Line 9: it
> it to 1
Done
https://review.coreboot.org/c/coreboot/+/41029/4//COMMIT_MSG@10
PS4, Line 10: controller
> Dot/period after *controller*.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/41029
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4f7e7508b8cd483508293ee3e7b760574d8f025f
Gerrit-Change-Number: 41029
Gerrit-PatchSet: 5
Gerrit-Owner: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 05 May 2020 05:45:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: comment
Hello build bot (Jenkins), Patrick Georgi, Maulik V Vaghela, Subrata Banik, Aamir Bohra, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41029
to look at the new patch set (#5).
Change subject: soc/intel/jasperlake: Allow SataEnable to be filled from devicetree
......................................................................
soc/intel/jasperlake: Allow SataEnable to be filled from devicetree
SataEnable is UPD used to enable SATA. Setting it to 1 will enable SATA
controller. This patch will allow to control it from devicetree so that
it can be set as per each board's requirement.
BUG=b:155595624
BRANCH=None
TEST=Build, boot JSLRVP, Verified UPD value from FSP log
Change-Id: I4f7e7508b8cd483508293ee3e7b760574d8f025f
Signed-off-by: Ronak Kanabar <ronak.kanabar(a)intel.com>
---
M src/soc/intel/jasperlake/fsp_params.c
1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/41029/5
--
To view, visit https://review.coreboot.org/c/coreboot/+/41029
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4f7e7508b8cd483508293ee3e7b760574d8f025f
Gerrit-Change-Number: 41029
Gerrit-PatchSet: 5
Gerrit-Owner: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40415 )
Change subject: mb/google/puff: add a region to cache SPD data
......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40415/5/src/mainboard/google/hatch…
File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/40415/5/src/mainboard/google/hatch…
PS5, Line 47: if (need_update_cache)
> how about this? […]
Thinking of this, dimm_changed default true then
1. if you don't touch dimm_changed = check_if_dimm_changed(spd_cache, &blk); you need update
2. if you touch dimm_changed = check_if_dimm_changed(spd_cache, &blk); And still true,you need update. If false, you can't reach the else statement.
--
To view, visit https://review.coreboot.org/c/coreboot/+/40415
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8d07fddf113a767d62394cb31e33b56f22f74351
Gerrit-Change-Number: 40415
Gerrit-PatchSet: 5
Gerrit-Owner: Jamie Chen <jamie.chen(a)intel.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Kane Chen <kane.chen(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 05 May 2020 04:24:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kane Chen <kane.chen(a)intel.com>
Gerrit-MessageType: comment
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40415 )
Change subject: mb/google/puff: add a region to cache SPD data
......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40415/5/src/mainboard/google/hatch…
File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/40415/5/src/mainboard/google/hatch…
PS5, Line 28: SPD_CACHE
You can remove the underline here.
`Invalid SPD cache`
https://review.coreboot.org/c/coreboot/+/40415/5/src/mainboard/google/hatch…
PS5, Line 47: if (need_update_cache)
how about this?
if (need_update_cache && update_spd_cache(&blk) == CB_ERR) {
printk(BIOS_WARNING, "Fail to update SPD cache\n");
}
--
To view, visit https://review.coreboot.org/c/coreboot/+/40415
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8d07fddf113a767d62394cb31e33b56f22f74351
Gerrit-Change-Number: 40415
Gerrit-PatchSet: 5
Gerrit-Owner: Jamie Chen <jamie.chen(a)intel.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Kane Chen <kane.chen(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: EricR Lai <ericr_lai(a)compal.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Tue, 05 May 2020 04:01:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment