Change in coreboot[master]: Revert "soc/mediatek/mt8183: Force retraining memory if requested"

[Julius Werner (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Paul Menzel, Yu-Ping Wu,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/40623

to review the following change.

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Revert "soc/mediatek/mt8183: Force retraining memory if requested"

This reverts commit 285975dbba8c7f3bbb9f9950e79a30bb983d5123.

Reason for revert: VB2_RECOVERY_TRAIN_AND_REBOOT was never meant to have
any special effect on memory training behavior. It was just supposed to
be a "reboot automatically after reaching kernel verification" recovery
reason. On x86 devices this was used to prime the separate recovery
MRC cache in the factory (make sure it is initialized before shipping).

This isn't used on Kukui anyway, but in order to make sure nobody copies
this code and keep the behavior consistent between platforms, let's
remove it.

Change-Id: I5df5e00526e90cb573131de3c8bac9f85f4e3a5f
---
M src/soc/mediatek/mt8183/memory.c
1 file changed, 1 insertion(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/40623/1

diff --git a/src/soc/mediatek/mt8183/memory.c b/src/soc/mediatek/mt8183/memory.c
index b9ed619..2a4ebbd 100644
--- a/src/soc/mediatek/mt8183/memory.c
+++ b/src/soc/mediatek/mt8183/memory.c
@@ -169,8 +169,7 @@
    /* 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() ||
-		    vboot_check_recovery_request() == VB2_RECOVERY_TRAIN_AND_REBOOT) {
+		if (vboot_recovery_mode_memory_retrain()) {
    		printk(BIOS_WARNING, "Retrain memory in next boot\n");
    		/* Use 0xFF as erased flash data. */
    		memset(dparam, 0xff, sizeof(*dparam));

[Julius Werner (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Paul Menzel, Yu-Ping Wu,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40623

to look at the new patch set (#2).

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Revert "soc/mediatek/mt8183: Force retraining memory if requested"

This reverts commit 285975dbba8c7f3bbb9f9950e79a30bb983d5123.

Reason for revert: VB2_RECOVERY_TRAIN_AND_REBOOT was never meant to have
any special effect on memory training behavior. It was just supposed to
be a "reboot automatically after reaching kernel verification" recovery
reason. On x86 devices this was used to prime the separate recovery
MRC cache in the factory (make sure it is initialized before shipping).

This isn't used on Kukui anyway, but in order to make sure nobody copies
this code and keep the behavior consistent between platforms, let's
remove it.

Change-Id: I5df5e00526e90cb573131de3c8bac9f85f4e3a5f
Signed-off-by: Julius Werner 
---
M src/soc/mediatek/mt8183/memory.c
1 file changed, 1 insertion(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/40623/2

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 2:

Hi Julius, I think VB2_RECOVERY_TRAIN_AND_REBOOT is needed. It is the equivalent command to 'crossystem recovery_request=0xc4', which we used in factory process to enforce DRAM training ( https://chromium.googlesource.com/chromiumos/platform/factory/+/HEAD/py/test... ), for both x86 and ARM.

Without this, we can't enforce re-training in manufacturing process.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Hi Julius, I think VB2_RECOVERY_TRAIN_AND_REBOOT is needed. It is the equivalent command to 'crossystem recovery_request=0xc4', which we used in factory process to enforce DRAM training ( https://chromium.googlesource.com/chromiumos/platform/factory/+/HEAD/py/test... ), for both x86 and ARM.
>
> Without this, we can't enforce re-training in manufacturing process.

It is not as much to force retraining. This command was added on x86 to ensure that recovery mode training data is generated and saved for future uses. If that data already exists, there is no need to force training. That is the primary reason why Julius pushed this revert.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 2:
> > Without this, we can't enforce re-training in manufacturing process.
>
> It is not as much to force retraining. This command was added on x86 to ensure that recovery mode training data is generated and saved for future uses.

Well... I agree we may have slightly changed its usage.
> If that data already exists, there is no need to force training. That is the primary reason why Julius pushed this revert.

Then can we have another flag to enforce re-training?
How does x86 deal with that?

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 2:
> Patch Set 2:
> > > Without this, we can't enforce re-training in manufacturing process.
> >
> > It is not as much to force retraining. This command was added on x86 to ensure that recovery mode training data is generated and saved for future uses.
>
> Well... I agree we may have slightly changed its usage.
> > If that data already exists, there is no need to force training. That is the primary reason why Julius pushed this revert.
>
> Then can we have another flag to enforce re-training?
> How does x86 deal with that?

We have not really encountered any case where we had to force retraining in factory. Is that what is required on non-x86 platforms? Can the saved training data be simply erased by factory scripts?

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 2:
> We have not really encountered any case where we had to force retraining in factory. 
> Is that what is required on non-x86 platforms?

I think we should enable this on all platforms with training data cache.
The concern is few ODMs are using copy machine to preflash - and they do it like

1. Install the images on one DUT.
2. Verify the DUT is working all fine (and boots).
3. Unmount the SPI flash / eMMC and use that as golden sample of copy source.

So it's always better to ensure DRAM cache is cleared.
It's also very helpful to debug memory issues with one simple command.
> Can the saved training data be simply erased by factory scripts?

It is possible, but we think it may be easier to have a unified way across different platforms, especially in firmware.

For example, on x86 there may be different cache FMAP names. Even on ARM, different SOCs may have different designs (e.g. kukui does not have RO but trogdor may have).

If we have to make this a script in the end, can we have it on all images and not just factory?

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 2:

Move discussion to b:154782084?

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 2:

*ping*

It sounds like we had consensus on doing this in the internal discussion (if I judged that right), so anyone want to give me a +2?

[Julius Werner (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Joel Kitching, Furquan Shaikh, Patrick Georgi, Paul Menzel, Yu-Ping Wu,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40623

to look at the new patch set (#3).

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Revert "soc/mediatek/mt8183: Force retraining memory if requested"

This reverts commit 285975dbba8c7f3bbb9f9950e79a30bb983d5123.

Reason for revert: VB2_RECOVERY_TRAIN_AND_REBOOT was never meant to have
any special effect on memory training behavior. It was just supposed to
be a "reboot automatically after reaching kernel verification" recovery
reason. On x86 devices this was used to prime the separate recovery
MRC cache in the factory (make sure it is initialized before shipping).

This isn't used on Kukui anyway, but in order to make sure nobody copies
this code and keep the behavior consistent between platforms, let's
remove it.

Change-Id: I5df5e00526e90cb573131de3c8bac9f85f4e3a5f
Signed-off-by: Julius Werner 
---
M src/soc/mediatek/mt8183/memory.c
1 file changed, 1 insertion(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/40623/3

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 3: Code-Review+2

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 3: Code-Review+2

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Revert "soc/mediatek/mt8183: Force retraining memory if requested"

This reverts commit 285975dbba8c7f3bbb9f9950e79a30bb983d5123.

Reason for revert: VB2_RECOVERY_TRAIN_AND_REBOOT was never meant to have
any special effect on memory training behavior. It was just supposed to
be a "reboot automatically after reaching kernel verification" recovery
reason. On x86 devices this was used to prime the separate recovery
MRC cache in the factory (make sure it is initialized before shipping).

This isn't used on Kukui anyway, but in order to make sure nobody copies
this code and keep the behavior consistent between platforms, let's
remove it.

Change-Id: I5df5e00526e90cb573131de3c8bac9f85f4e3a5f
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40623
Reviewed-by: Furquan Shaikh 
Reviewed-by: Hung-Te Lin 
Tested-by: build bot (Jenkins) 
---
M src/soc/mediatek/mt8183/memory.c
1 file changed, 1 insertion(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Hung-Te Lin: Looks good to me, approved

diff --git a/src/soc/mediatek/mt8183/memory.c b/src/soc/mediatek/mt8183/memory.c
index 53763fd..aa8f7d2 100644
--- a/src/soc/mediatek/mt8183/memory.c
+++ b/src/soc/mediatek/mt8183/memory.c
@@ -163,8 +163,7 @@
    /* Load calibration params from flash and run fast calibration */
    if (recovery_mode) {
    	printk(BIOS_WARNING, "Skip loading cached calibration data\n");
-		if (get_recovery_mode_retrain_switch() ||
-		    vboot_check_recovery_request() == VB2_RECOVERY_TRAIN_AND_REBOOT) {
+		if (get_recovery_mode_retrain_switch()) {
    		printk(BIOS_WARNING, "Retrain memory in next boot\n");
    		/* Use 0xFF as erased flash data. */
    		memset(dparam, 0xff, sizeof(*dparam));

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40623 )

Change subject: Revert "soc/mediatek/mt8183: Force retraining memory if requested"
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2952
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2951
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2950
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2949

Please note: This test is under development and might not be accurate at all!