V Sowmya has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Enable the SkipCpuReplacementCheck ......................................................................
soc/intel/jasperlake: Enable the SkipCpuReplacementCheck
This patch enable the fspm upd to SkipCpuReplacementCheck.
Change-Id: I63fcdab3686322406cf7c24fc26cbb535cc58c8d Signed-off-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/jasperlake/romstage/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/42453/1
diff --git a/src/soc/intel/jasperlake/romstage/fsp_params.c b/src/soc/intel/jasperlake/romstage/fsp_params.c index d9063b0..dbfb9b5 100644 --- a/src/soc/intel/jasperlake/romstage/fsp_params.c +++ b/src/soc/intel/jasperlake/romstage/fsp_params.c @@ -113,6 +113,9 @@ ARRAY_SIZE(config->PchHdaAudioLinkSndwEnable), "copy buffer overflow!"); memcpy(m_cfg->PchHdaAudioLinkSndwEnable, config->PchHdaAudioLinkSndwEnable, sizeof(config->PchHdaAudioLinkSndwEnable)); + + /* Setting this option to skip CPU replacement check */ + m_cfg->SkipCpuReplacementCheck = 1; }
void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version)
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Set Ready For Review
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/2/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/2/src/soc/intel/jasperlake/ch... PS2, Line 287: Skip CPU replacement check Can you add more info here as similar to commit message?
Hello build bot (Jenkins), Rizwan Qureshi, Krishna P Bhat D, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42453
to look at the new patch set (#3).
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration
Add SkipCpuReplacementCheck config to control the FSPM UPD used for skipping the CPU replacementment check to avoid the forced MRC training for the platforms with soldered down SOC.
BUG=b:160201335 TEST=Build and verified on waddledoo with CSE Lite SKU.
Change-Id: I63fcdab3686322406cf7c24fc26cbb535cc58c8d Signed-off-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/jasperlake/chip.h M src/soc/intel/jasperlake/romstage/fsp_params.c 2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/42453/3
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/2/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/2/src/soc/intel/jasperlake/ch... PS2, Line 287: Skip CPU replacement check
Can you add more info here as similar to commit message?
Done
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... PS4, Line 293: uint8_t SkipCpuReplacementCheck; I believe that setting this UPD means MRC training is skipped even if there is no cached training data. If my understanding is correct, what is the impact to the system? Please correct me if I am wrong.
Also is it a requirement for CSE Lite SKU? If so, I am not seeing other SoCs like TGL or CML setting this UPD. I can see CSE Lite SKU is enabled in puff. Why is it different for JSL compared to CML.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... PS4, Line 293: uint8_t SkipCpuReplacementCheck;
I believe that setting this UPD means MRC training is skipped even if there is no cached training da […]
No, this isn't possible. One can't skip memory training and successfully boot when there's no training data at all. This is about reusing existing memory training data when the CPU might have been replaced.
The CPU replacement check is something MRC does before reusing training data. On SKL, it consists of asking the ME/CSE about it. If the CPU has been replaced, then MRC will discard the training data and force a full memory training. SPS on SKL does not support CPU replacement Check, so Kabylake FSP forces a full memory retrain on such platforms.
I think a similar story happens with CSE Lite, but I've no idea. In any case, it's pretty useless for soldered-on platforms, as one does not simply swap BGA chips back and forth.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... PS4, Line 293: uint8_t SkipCpuReplacementCheck;
No, this isn't possible. […]
Thank you Angel Pons for sharing the bigger picture and educating me.
Looking at CML FSP, it seems there is no UPD to skip the CPU Replacement Check. Probably that is the reason it is not set in that platform. But the question still remains as to why this UPD needs to be set if the SoC is soldered down.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... PS4, Line 293: uint8_t SkipCpuReplacementCheck;
Thank you Angel Pons for sharing the bigger picture and educating me. […]
Setting this flag ensures that MRC re-training is not forced even if the CPU replacement flag is set. This is required for CSE Lite because with CSE Lite a device can downgrade to an older version of the firmware. In this case, coreboot clears all CSE data to avoid incompatibilities. This results in CPU replacement flag being set. However, we don't really want to force a memory retraining in this condition. Hence, the UPD is set in case of CSE Lite. This isn't yet set for CML or TGL because the CSE downgrade changes are not yet merged.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... PS4, Line 293: uint8_t SkipCpuReplacementCheck;
Setting this flag ensures that MRC re-training is not forced even if the CPU replacement flag is set […]
Thanks Furquan for the explanation.
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... PS4, Line 293: uint8_t SkipCpuReplacementCheck;
Thanks Furquan for the explanation.
This UPD is valid, since on Intel RVP's we can still swap the silicon and in that case CSE reports that CPU is being replaced and a full MRC training will done. Like Furquan mentioned, we have to set this UPD for the CSE Lite to support the downgrade feature and this change has been pushed for CML and TGL also. Here is the crossbug that captures this discussion for CSE Lite BUG=b:154801654
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... PS4, Line 293: uint8_t SkipCpuReplacementCheck;
This UPD is valid, since on Intel RVP's we can still swap the silicon and in that case CSE reports t […]
Oh right, I forget that Intel RVPs have that clamp-like thingy that allows one to easily replace BGA things:
https://images.anandtech.com/doci/6993/DSC_0332.jpg https://images.anandtech.com/doci/6993/DSC_0343.jpg
Both the CPU and the PCH of this Crystalwell CRB can be replaced.
Meh, now I want one of these CRBs 😭
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... File src/soc/intel/jasperlake/chip.h:
https://review.coreboot.org/c/coreboot/+/42453/4/src/soc/intel/jasperlake/ch... PS4, Line 293: uint8_t SkipCpuReplacementCheck;
Oh right, I forget that Intel RVPs have that clamp-like thingy that allows one to easily replace BGA […]
Yup, but now we don't have that clamp-like lock but still are really fun to work with 😊👍
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
Can you please rebase the changes, fix the dependency chain and mark all the comments as resolved.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4:
Patch Set 4:
Can you please rebase the changes, fix the dependency chain and mark all the comments as resolved.
Also can either the owner or the uploader add Cq-Depend: chrome-internal:3142530 in all the CLs, such that when the changes are downstreamed, continuous integration can check for the state of the external dependencies.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 4: Code-Review+2
(5 comments)
https://review.coreboot.org/c/coreboot/+/42453/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42453/1//COMMIT_MSG@9 PS1, Line 9: enable
enables
Done
https://review.coreboot.org/c/coreboot/+/42453/1//COMMIT_MSG@9 PS1, Line 9: fspm
FSP-M
Done
https://review.coreboot.org/c/coreboot/+/42453/1//COMMIT_MSG@9 PS1, Line 9: upd
UPD
Done
https://review.coreboot.org/c/coreboot/+/42453/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42453/4//COMMIT_MSG@16 PS4, Line 16: Change-Id: I63fcdab3686322406cf7c24fc26cbb535cc58c8d Add:
Cq-Depend: chrome-internal:3142530
https://review.coreboot.org/c/coreboot/+/42453/1/src/soc/intel/jasperlake/ro... File src/soc/intel/jasperlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42453/1/src/soc/intel/jasperlake/ro... PS1, Line 117: /* Setting this option to skip CPU replacement check */
Why are we skipping the CPU replacement check? I would prefer having a comment that explains this
Done
Hello build bot (Jenkins), Furquan Shaikh, Rizwan Qureshi, Angel Pons, Krishna P Bhat D, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42453
to look at the new patch set (#5).
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration
Add SkipCpuReplacementCheck config to control the FSPM UPD used for skipping the CPU replacementment check to avoid the forced MRC training for the platforms with soldered down SOC.
BUG=b:160201335 TEST=Build and verify CSE Lite SKU on Waddleddo. Cq-Depend: chrome-internal:3142530
Change-Id: I63fcdab3686322406cf7c24fc26cbb535cc58c8d Signed-off-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/jasperlake/chip.h M src/soc/intel/jasperlake/romstage/fsp_params.c 2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/42453/5
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42453/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42453/4//COMMIT_MSG@16 PS4, Line 16: Change-Id: I63fcdab3686322406cf7c24fc26cbb535cc58c8d
Add: […]
Done
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 5: Code-Review+2
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 5: Code-Review+1
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
Patch Set 5: Code-Review+2
Karthik Ramasubramanian has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42453 )
Change subject: soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration ......................................................................
soc/intel/jasperlake: Add the SkipCpuReplacementCheck configuration
Add SkipCpuReplacementCheck config to control the FSPM UPD used for skipping the CPU replacementment check to avoid the forced MRC training for the platforms with soldered down SOC.
BUG=b:160201335 TEST=Build and verify CSE Lite SKU on Waddleddo. Cq-Depend: chrome-internal:3142530
Change-Id: I63fcdab3686322406cf7c24fc26cbb535cc58c8d Signed-off-by: V Sowmya v.sowmya@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42453 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com Reviewed-by: Sridhar Siricilla sridhar.siricilla@intel.com Reviewed-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/jasperlake/chip.h M src/soc/intel/jasperlake/romstage/fsp_params.c 2 files changed, 10 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Rizwan Qureshi: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved Sridhar Siricilla: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/jasperlake/chip.h b/src/soc/intel/jasperlake/chip.h index d8ea560..e5e10e3 100644 --- a/src/soc/intel/jasperlake/chip.h +++ b/src/soc/intel/jasperlake/chip.h @@ -284,6 +284,13 @@ */ uint8_t cpu_ratio_override;
+ /* Skip CPU replacement check + * 0: disable + * 1: enable + * Setting this option to skip CPU replacement check to avoid the forced MRC training + * for the platforms with soldered down SOC. + */ + uint8_t SkipCpuReplacementCheck; };
typedef struct soc_intel_jasperlake_config config_t; diff --git a/src/soc/intel/jasperlake/romstage/fsp_params.c b/src/soc/intel/jasperlake/romstage/fsp_params.c index 6d4055a..0688eea 100644 --- a/src/soc/intel/jasperlake/romstage/fsp_params.c +++ b/src/soc/intel/jasperlake/romstage/fsp_params.c @@ -114,6 +114,9 @@ ARRAY_SIZE(config->PchHdaAudioLinkSndwEnable), "copy buffer overflow!"); memcpy(m_cfg->PchHdaAudioLinkSndwEnable, config->PchHdaAudioLinkSndwEnable, sizeof(config->PchHdaAudioLinkSndwEnable)); + + /* Skip the CPU replacement check */ + m_cfg->SkipCpuReplacementCheck = config->SkipCpuReplacementCheck; }
void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version)
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sridhar Siricilla (Code Review)
-
V Sowmya (Code Review)