Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36251 )
Change subject: soc/mediatek/mt8183: Force retraining memory if requested ......................................................................
soc/mediatek/mt8183: Force retraining memory if requested
To allow retraining memory without hotkey (for example in manufacturing process), we want to enforce re-training when the recovery reason is set to VB2_RECOVERY_TRAIN_AND_REBOOT (which can be done by running "crossystem recovery_request=0xc4").
On x86 (MRC cache) this is primarily used for ensuring RO calibration data is filled; and on MT8183 we have only RW calibration, but it seems totally fine to extend that to RW.
BRANCH=kukui BUG=None TEST=boots; crossystem recovery_reason=0xc4; reboot
Change-Id: Iaa5275f0e0eb90f6ab3a7d4579977a6655d59bd9 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/soc/mediatek/mt8183/memory.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36251/1
diff --git a/src/soc/mediatek/mt8183/memory.c b/src/soc/mediatek/mt8183/memory.c index 2a4ebbd..b9ed619 100644 --- a/src/soc/mediatek/mt8183/memory.c +++ b/src/soc/mediatek/mt8183/memory.c @@ -169,7 +169,8 @@ /* Load calibration params from flash and run fast calibration */ if (recovery_mode) { printk(BIOS_WARNING, "Skip loading cached calibration data\n"); - if (vboot_recovery_mode_memory_retrain()) { + if (vboot_recovery_mode_memory_retrain() || + vboot_check_recovery_request() == VB2_RECOVERY_TRAIN_AND_REBOOT) { printk(BIOS_WARNING, "Retrain memory in next boot\n"); /* Use 0xFF as erased flash data. */ memset(dparam, 0xff, sizeof(*dparam));
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36251 )
Change subject: soc/mediatek/mt8183: Force retraining memory if requested ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36251 )
Change subject: soc/mediatek/mt8183: Force retraining memory if requested ......................................................................
Patch Set 1: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36251 )
Change subject: soc/mediatek/mt8183: Force retraining memory if requested ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/36251/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36251/1//COMMIT_MSG@14 PS1, Line 14: On x86 (MRC cache) this is primarily used for ensuring RO calibration I don't actually see any other coreboot code referencing this reason. Where is that implemented on x86? Is this actually used anywhere?
https://review.coreboot.org/c/coreboot/+/36251/1/src/soc/mediatek/mt8183/mem... File src/soc/mediatek/mt8183/memory.c:
https://review.coreboot.org/c/coreboot/+/36251/1/src/soc/mediatek/mt8183/mem... PS1, Line 173: vboot_check_recovery_request() == VB2_RECOVERY_TRAIN_AND_REBOOT) { When it says TRAIN_AND_REBOOT, should it actually reboot afterwards? What are the expected semantics for this?
Hello Yu-Ping Wu, Julius Werner, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36251
to look at the new patch set (#2).
Change subject: soc/mediatek/mt8183: Force retraining memory if requested ......................................................................
soc/mediatek/mt8183: Force retraining memory if requested
To allow retraining memory without hotkey (for example in manufacturing process), we want to enforce re-training when the recovery reason is set to VB2_RECOVERY_TRAIN_AND_REBOOT (which can be done by running "crossystem recovery_request=0xc4").
The special reason was created for X86 MRC cache, for ensuring RO calibration data is filled (the underlying implementation was in vboot, not coreboot); and on MT8183 we have only RW calibration, but it seems totally fine to extend that for RW.
BRANCH=kukui BUG=None TEST=boots; crossystem recovery_reason=0xc4; reboot
Change-Id: Iaa5275f0e0eb90f6ab3a7d4579977a6655d59bd9 Signed-off-by: Hung-Te Lin hungte@chromium.org --- M src/soc/mediatek/mt8183/memory.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36251/2
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36251 )
Change subject: soc/mediatek/mt8183: Force retraining memory if requested ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36251/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36251/1//COMMIT_MSG@14 PS1, Line 14: On x86 (MRC cache) this is primarily used for ensuring RO calibration
I don't actually see any other coreboot code referencing this reason. […]
That is in vboot_reference. see firmware/lib/vboot_api_kernel.c#L305.
Maybe I should clarify: it was made for MRC cache, although the real implementation was inside vboot.
https://review.coreboot.org/c/coreboot/+/36251/1/src/soc/mediatek/mt8183/mem... File src/soc/mediatek/mt8183/memory.c:
https://review.coreboot.org/c/coreboot/+/36251/1/src/soc/mediatek/mt8183/mem... PS1, Line 173: vboot_check_recovery_request() == VB2_RECOVERY_TRAIN_AND_REBOOT) {
When it says TRAIN_AND_REBOOT, should it actually reboot afterwards? What are the expected semantics […]
vboot will reboot after seeing that.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36251 )
Change subject: soc/mediatek/mt8183: Force retraining memory if requested ......................................................................
soc/mediatek/mt8183: Force retraining memory if requested
To allow retraining memory without hotkey (for example in manufacturing process), we want to enforce re-training when the recovery reason is set to VB2_RECOVERY_TRAIN_AND_REBOOT (which can be done by running "crossystem recovery_request=0xc4").
The special reason was created for X86 MRC cache, for ensuring RO calibration data is filled (the underlying implementation was in vboot, not coreboot); and on MT8183 we have only RW calibration, but it seems totally fine to extend that for RW.
BRANCH=kukui BUG=None TEST=boots; crossystem recovery_reason=0xc4; reboot
Change-Id: Iaa5275f0e0eb90f6ab3a7d4579977a6655d59bd9 Signed-off-by: Hung-Te Lin hungte@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/36251 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Yu-Ping Wu yupingso@google.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Julius Werner jwerner@chromium.org --- M src/soc/mediatek/mt8183/memory.c 1 file changed, 2 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Julius Werner: Looks good to me, approved Yu-Ping Wu: Looks good to me, but someone else must approve
diff --git a/src/soc/mediatek/mt8183/memory.c b/src/soc/mediatek/mt8183/memory.c index 2a4ebbd..b9ed619 100644 --- a/src/soc/mediatek/mt8183/memory.c +++ b/src/soc/mediatek/mt8183/memory.c @@ -169,7 +169,8 @@ /* Load calibration params from flash and run fast calibration */ if (recovery_mode) { printk(BIOS_WARNING, "Skip loading cached calibration data\n"); - if (vboot_recovery_mode_memory_retrain()) { + if (vboot_recovery_mode_memory_retrain() || + vboot_check_recovery_request() == VB2_RECOVERY_TRAIN_AND_REBOOT) { printk(BIOS_WARNING, "Retrain memory in next boot\n"); /* Use 0xFF as erased flash data. */ memset(dparam, 0xff, sizeof(*dparam));
Julius Werner has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/36251 )
Change subject: soc/mediatek/mt8183: Force retraining memory if requested ......................................................................
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)