Attention is currently required from: Sridhar Siricilla, Arthur Heymans.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63734 )
Change subject: soc/intel/common: Fix buggy code tries to access DESC region
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> SI_DESC is going to be remain in future platforms as well. So, reference to SI_DESC which get defined in the FMAP should be fine , IMO.
The default generated x86 FMAP does not have this. This means that you always need to manually write an FMAP yourself for this feature. Another point is that this pushes problems to the runtime, because there is no buildtime guarantee that this FMAP section will be there.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63734
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib144cc0845b7527e5a3032529b0802f961944b87
Gerrit-Change-Number: 63734
Gerrit-PatchSet: 4
Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 21 Apr 2022 13:28:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63734 )
Change subject: soc/intel/common: Fix buggy code tries to access DESC region
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> boot_device_spi_flash ought to be used instead of boot_device_ro. […]
SI_DESC is going to be remain in future platforms as well. So, reference to SI_DESC which get defined in the FMAP should be fine , IMO.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63734
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib144cc0845b7527e5a3032529b0802f961944b87
Gerrit-Change-Number: 63734
Gerrit-PatchSet: 4
Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 21 Apr 2022 13:20:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment
Attention is currently required from: Marc Jones, Subrata Banik, Tim Wawrzynczak, Arthur Heymans, Eric Lai.
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63630 )
Change subject: soc/intel/cmn/pch/lockdown: Implement LPC lock down configuration
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/63630
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3e49b783e5db0ff40238e6e9259e48a4ecca66f8
Gerrit-Change-Number: 63630
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 21 Apr 2022 13:09:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/62913 )
Change subject: mb/intel/adlrvp: Set half_populated true for ADL-N
......................................................................
mb/intel/adlrvp: Set half_populated true for ADL-N
Alder Lake-N has single memory controller with 64-bit bus width. Alder
Lake common meminit block driver considers bus width to be 128-bit and
populates the meminit data accordingly. By setting half_populated to
true, only the bottom half is populated.
Ideally, half_populated is used in platforms with multiple channels to
enable only one half of the channel. Alder Lake N has single channel,
and it would require for new structures to be defined in meminit block
driver for LPx memory configurations. In order to avoid adding new
structures, set half_populated to true. This has the same effect as
having single channel with 64-bit width.
BRANCH=NONE
TEST=Build and boot ADL-N RVP.
Signed-off-by: Usha P <usha.p(a)intel.com>
Change-Id: I2ecc3018a1ab039990ba47898ff0e0e2ede695cc
Reviewed-on: https://review.coreboot.org/c/coreboot/+/62913
Reviewed-by: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M src/mainboard/intel/adlrvp/romstage_fsp_params.c
1 file changed, 10 insertions(+), 1 deletion(-)
Approvals:
build bot (Jenkins): Verified
Rizwan Qureshi: Looks good to me, approved
diff --git a/src/mainboard/intel/adlrvp/romstage_fsp_params.c b/src/mainboard/intel/adlrvp/romstage_fsp_params.c
index e29b7d4..90d8302 100644
--- a/src/mainboard/intel/adlrvp/romstage_fsp_params.c
+++ b/src/mainboard/intel/adlrvp/romstage_fsp_params.c
@@ -47,7 +47,16 @@
FSP_M_CONFIG *m_cfg = &memupd->FspmConfig;
const struct mb_cfg *mem_config = variant_memory_params();
int board_id = get_board_id();
- const bool half_populated = false;
+
+ /*
+ * Alder Lake common meminit block driver considers bus width to be 128-bit and
+ * populates the meminit data accordingly. Alder Lake-N has single memory controller
+ * with 64-bit bus width. By setting half_populated to true, only the bottom half is
+ * populated.
+ * TODO: Implement __weak variant_is_half_populated(void) function.
+ */
+ const bool half_populated = (CONFIG(BOARD_INTEL_ADLRVP_N_EXT_EC)
+ || CONFIG(BOARD_INTEL_ADLRVP_N));
const struct mem_spd memory_down_spd_info = {
.topo = MEM_TOPO_MEMORY_DOWN,
--
To view, visit https://review.coreboot.org/c/coreboot/+/62913
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I2ecc3018a1ab039990ba47898ff0e0e2ede695cc
Gerrit-Change-Number: 62913
Gerrit-PatchSet: 15
Gerrit-Owner: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Kangheui Won <khwon(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: merged
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/63708 )
Change subject: soc/intel/common/block/cse: Simplify CSE final ops
......................................................................
soc/intel/common/block/cse: Simplify CSE final ops
Looks like the `notify_data` struct array idea comes from the FSP 2.0
notify driver, which has a similar struct but with several additional
fields. However, there's no need for this mechanism in the CSE driver
because the struct only contains a condition (boolean) and a function
to execute, which can be expressed as a regular if-block.
Change-Id: I65fcb2fc02ea16b37c764f1fd69bdff3382fad18
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/63708
Reviewed-by: Subrata Banik <subratabanik(a)google.com>
Reviewed-by: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 5 insertions(+), 20 deletions(-)
Approvals:
build bot (Jenkins): Verified
Subrata Banik: Looks good to me, approved
Eric Lai: Looks good to me, approved
diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c
index 2444cee..64fd041 100644
--- a/src/soc/intel/common/block/cse/cse.c
+++ b/src/soc/intel/common/block/cse/cse.c
@@ -1195,11 +1195,6 @@
me_reset_with_count();
}
-struct cse_notify_phase_data {
- bool skip;
- void (*notify_func)(void);
-};
-
/*
* `cse_final_ready_to_boot` function is native implementation of equivalent events
* performed by FSP NotifyPhase(Ready To Boot) API invocations.
@@ -1235,27 +1230,17 @@
heci_set_to_d0i3();
}
-static const struct cse_notify_phase_data notify_data[] = {
- {
- .skip = CONFIG(USE_FSP_NOTIFY_PHASE_READY_TO_BOOT),
- .notify_func = cse_final_ready_to_boot,
- },
- {
- .skip = CONFIG(USE_FSP_NOTIFY_PHASE_END_OF_FIRMWARE),
- .notify_func = cse_final_end_of_firmware,
- },
-};
-
/*
* `cse_final` function is native implementation of equivalent events performed by
* each FSP NotifyPhase() API invocations.
*/
static void cse_final(struct device *dev)
{
- for (size_t i = 0; i < ARRAY_SIZE(notify_data); i++) {
- if (!notify_data[i].skip)
- notify_data[i].notify_func();
- }
+ if (!CONFIG(USE_FSP_NOTIFY_PHASE_READY_TO_BOOT))
+ cse_final_ready_to_boot();
+
+ if (!CONFIG(USE_FSP_NOTIFY_PHASE_END_OF_FIRMWARE))
+ cse_final_end_of_firmware();
}
static struct device_operations cse_ops = {
--
To view, visit https://review.coreboot.org/c/coreboot/+/63708
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I65fcb2fc02ea16b37c764f1fd69bdff3382fad18
Gerrit-Change-Number: 63708
Gerrit-PatchSet: 2
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged