Change in coreboot[master]: soc/mediatek/mt8183: Skip fast calibration in recovery mode

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36089 )

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

soc/mediatek/mt8183: Skip fast calibration in recovery mode

According MediaTek, they have seen some extreme situations where the
fast calibration passed complex memory test but there were sill memory
issues after kernel is up. Therefore, they suggest skipping fast
calibration in recovery mode. Also, the calculated parameters will not
be saved to the flash.

BRANCH=kukui
BUG=b:139099592
TEST=emerge-kukui coreboot

Change-Id: I29e0df71dc3357462e15ce8fc2ba02f21b54ed33
Signed-off-by: Yu-Ping Wu 
---
M src/soc/mediatek/mt8183/memory.c
1 file changed, 23 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36089/1

diff --git a/src/soc/mediatek/mt8183/memory.c b/src/soc/mediatek/mt8183/memory.c
index b657708..5c10f49 100644
--- a/src/soc/mediatek/mt8183/memory.c
+++ b/src/soc/mediatek/mt8183/memory.c
@@ -17,6 +17,7 @@
 #include <cbfs.h>
 #include <console/console.h>
 #include <ip_checksum.h>
+#include <security/vboot/vboot_common.h>
 #include <soc/dramc_param.h>
 #include <soc/dramc_pi_api.h>
 #include <soc/emi.h>
@@ -163,17 +164,22 @@
    if (CONFIG(MT8183_DRAM_EMCP))
    	config |= DRAMC_CONFIG_EMCP;

+	const bool recovery_mode = vboot_recovery_mode_enabled();
+
    /* Load calibration params from flash and run fast calibration */
-	if (dparam_ops->read_from_flash(dparam)) {
-		if (dram_run_fast_calibration(dparam, config) == 0) {
-			printk(BIOS_INFO,
-			       "DRAM calibraion params loaded from flash\n");
-			if (mt_set_emi(dparam) == 0 && mt_mem_test() == 0)
-				return;
+	if (!recovery_mode) {
+		if (dparam_ops->read_from_flash(dparam)) {
+			if (dram_run_fast_calibration(dparam, config) == 0) {
+				printk(BIOS_INFO,
+				       "Calibraion params loaded from flash\n");
+				if (mt_set_emi(dparam) == 0 &&
+				    mt_mem_test() == 0)
+					return;
+			}
+		} else {
+			printk(BIOS_WARNING,
+			       "Failed to read calibration data from flash\n");
    	}
-	} else {
-		printk(BIOS_WARNING,
-		       "Failed to read calibration data from flash\n");
    }

/* Run full calibration */
@@ -181,12 +187,14 @@
    if (err == 0) {
    	printk(BIOS_INFO, "Successfully loaded DRAM blobs and "
    	       "ran DRAM calibration\n");
-		set_source_to_flash(dparam->freq_params);
-		dparam->header.checksum = compute_checksum(dparam);
-		dparam_ops->write_to_flash(dparam);
-		printk(BIOS_DEBUG, "Calibration params saved to flash: "
-		       "version=%#x, size=%#x\n",
-		       dparam->header.version, dparam->header.size);
+		if (!recovery_mode) {
+			set_source_to_flash(dparam->freq_params);
+			dparam->header.checksum = compute_checksum(dparam);
+			dparam_ops->write_to_flash(dparam);
+			printk(BIOS_DEBUG, "Calibration params saved to flash: "
+			       "version=%#x, size=%#x\n",
+			       dparam->header.version, dparam->header.size);
+		}
    	return;
    }

[Yu-Ping Wu (Code Review)]

Hello Nicolas Boichat, Julius Werner, Hung-Te Lin,

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

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

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

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

soc/mediatek/mt8183: Skip fast calibration in recovery mode

According MediaTek, they have seen some extreme situations where the
fast calibration passed complex memory test but there were sill memory
issues after kernel is up. Therefore, they suggest skipping fast
calibration in recovery mode. Also, the calculated parameters will not
be saved to the flash.

BRANCH=kukui
BUG=b:139099592
TEST=emerge-kukui coreboot

Change-Id: I29e0df71dc3357462e15ce8fc2ba02f21b54ed33
Signed-off-by: Yu-Ping Wu 
---
M src/soc/mediatek/mt8183/memory.c
1 file changed, 23 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36089/2

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/36089/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36089/2//COMMIT_MSG@8 
PS2, Line 8: 
           : According MediaTek
According to MediaTek

https://review.coreboot.org/c/coreboot/+/36089/2//COMMIT_MSG@10 
PS2, Line 10: sill
still

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
File src/soc/mediatek/mt8183/memory.c:

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
PS2, Line 167: vboot_recovery_mode_enabled
I'd recommend passing this from board file (romstage), since in SOC implementation we should not rely on vboot (i.e., for non-chromeos, or for devices that do not want to enable vboot).

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
PS2, Line 170: if (!recovery_mode) {
change to

if (recovery_mode) {
    printk(BIOS_WARNING, "Skipped loading cached calibration params in recovery mode.\n");
 } else if (dparam...) {
   ...
 } else {
   ...
 }

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded a new patch set (#3) to the change originally created by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/36089 )

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

soc/mediatek/mt8183: Skip fast calibration in recovery mode

SOC vendor suggested to always run full calibration mode in recovery
mode because it is possible to get unstable memory even if the complex
memory test has been passed.

Also, since the recovery mode runs from RO and we only have training
data cache for RW, the trained calibration data can't be saved since RO
and RW may be running different firmware.

BRANCH=kukui
BUG=b:139099592
TEST=emerge-kukui coreboot

Change-Id: I29e0df71dc3357462e15ce8fc2ba02f21b54ed33
Signed-off-by: Yu-Ping Wu 
---
M src/mainboard/google/kukui/romstage.c
M src/soc/mediatek/mt8183/include/soc/emi.h
M src/soc/mediatek/mt8183/memory.c
3 files changed, 27 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36089/3

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
File src/soc/mediatek/mt8183/memory.c:

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
PS2, Line 170: if (!recovery_mode) {
> change to […]

Done

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36089/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36089/2//COMMIT_MSG@8 
PS2, Line 8: 
           : According MediaTek
> According to MediaTek

Ack

https://review.coreboot.org/c/coreboot/+/36089/2//COMMIT_MSG@10 
PS2, Line 10: sill
> still

Ack

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
File src/soc/mediatek/mt8183/memory.c:

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
PS2, Line 167: vboot_recovery_mode_enabled
> I'd recommend passing this from board file (romstage), since in SOC implementation we should not rel […]

Done

Change in coreboot[master]: mb/google/kukui: Skip fast calibration in recovery mode

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded a new patch set (#4) to the change originally created by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/36089 )

Change subject: mb/google/kukui: Skip fast calibration in recovery mode
......................................................................

mb/google/kukui: Skip fast calibration in recovery mode

SOC vendor suggested to always run full calibration mode in recovery
mode because it is possible to get unstable memory even if the complex
memory test has been passed.

Since the recovery mode runs from RO and we only have training data
cache for RW, the trained calibration data can't be saved since RO and
RW may be running different firmware.

Also revised few message to make it more clear for what calibration mode
(fast, full, or partial) has been executed.

BRANCH=kukui
BUG=b:139099592
TEST=emerge-kukui coreboot

Change-Id: I29e0df71dc3357462e15ce8fc2ba02f21b54ed33
Signed-off-by: Yu-Ping Wu 
---
M src/mainboard/google/kukui/romstage.c
M src/soc/mediatek/mt8183/include/soc/emi.h
M src/soc/mediatek/mt8183/memory.c
3 files changed, 19 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36089/4

Change in coreboot[master]: mb/google/kukui: Skip fast calibration in recovery mode

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded a new patch set (#5) to the change originally created by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/36089 )

Change subject: mb/google/kukui: Skip fast calibration in recovery mode
......................................................................

mb/google/kukui: Skip fast calibration in recovery mode

SOC vendor suggested to always run full calibration mode in recovery
mode because it is possible to get unstable memory even if the complex
memory test has been passed.

Since the recovery mode runs from RO and we only have training data
cache for RW, the trained calibration data can't be saved since RO and
RW may be running different firmware.

Also revised few message to make it more clear for what calibration mode
(fast, full, or partial) has been executed.

BRANCH=kukui
BUG=b:139099592
TEST=emerge-kukui coreboot

Change-Id: I29e0df71dc3357462e15ce8fc2ba02f21b54ed33
Signed-off-by: Yu-Ping Wu 
---
M src/mainboard/google/kukui/romstage.c
M src/soc/mediatek/mt8183/include/soc/emi.h
M src/soc/mediatek/mt8183/memory.c
3 files changed, 18 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36089/5

Change in coreboot[master]: mb/google/kukui: Skip fast calibration in recovery mode

[Julius Werner (Code Review)]

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

Change subject: mb/google/kukui: Skip fast calibration in recovery mode
......................................................................

Patch Set 5: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/36089/5/src/soc/mediatek/mt8183/mem... 
File src/soc/mediatek/mt8183/memory.c:

https://review.coreboot.org/c/coreboot/+/36089/5/src/soc/mediatek/mt8183/mem... 
PS5, Line 192: 			return;
nit: I got a bit confused about the code flow in all paths here, I think this would be easier to read as

if (try_fastk) {
   ...do the write back stuff...
 }
 return;

Maybe add a comment as to why you're not writing back when try_fastk was false, too.

Change in coreboot[master]: mb/google/kukui: Skip fast calibration in recovery mode

[Julius Werner (Code Review)]

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

Change subject: mb/google/kukui: Skip fast calibration in recovery mode
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
File src/soc/mediatek/mt8183/memory.c:

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
PS2, Line 167: vboot_recovery_mode_enabled
> Done

This is already handled -- vboot_recovery_mode_enabled() and similar functions just always return 0 when vboot is not enabled. You don't need to worry about this... x86 uses it this was as well (checking those functions from right within their SoC memory training code). I think putting them here makes more sense.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded a new patch set (#6) to the change originally created by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/36089 )

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

soc/mediatek/mt8183: Skip fast calibration in recovery mode

SOC DRAM team suggested to always run full calibration mode in recovery
mode because it is possible to get unstable memory even if the complex
memory test has been passed.

Since the recovery mode runs from RO and we only have training data
cache for RW, the trained calibration data can't be saved since RO and
RW may be running different firmware.

Also revised few message to make it more clear for what calibration mode
(fast, full, or partial) has been executed.

BRANCH=kukui
BUG=b:139099592
TEST=emerge-kukui coreboot

Change-Id: I29e0df71dc3357462e15ce8fc2ba02f21b54ed33
Signed-off-by: Yu-Ping Wu 
---
M src/mainboard/google/kukui/romstage.c
M src/soc/mediatek/mt8183/memory.c
2 files changed, 26 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36089/6

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
File src/soc/mediatek/mt8183/memory.c:

https://review.coreboot.org/c/coreboot/+/36089/2/src/soc/mediatek/mt8183/mem... 
PS2, Line 167: vboot_recovery_mode_enabled
> This is already handled -- vboot_recovery_mode_enabled() and similar functions just always return 0  […]

Yes I know the APIs will return zero when not enabled, but was thinking that having these isolated should be more clear since the concept only lives per board, not SOC.

But if you also agree putting that there would make more sense, I can revert it back.

https://review.coreboot.org/c/coreboot/+/36089/5/src/soc/mediatek/mt8183/mem... 
File src/soc/mediatek/mt8183/memory.c:

https://review.coreboot.org/c/coreboot/+/36089/5/src/soc/mediatek/mt8183/mem... 
PS5, Line 192: 			return;
> nit: I got a bit confused about the code flow in all paths here, I think this would be easier to rea […]

Done

[Yu-Ping Wu (Code Review)]

Hello Nicolas Boichat, Julius Werner, Hung-Te Lin, build bot (Jenkins),

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

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

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

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

soc/mediatek/mt8183: Skip fast calibration in recovery mode

SoC DRAM team suggested always running full calibration mode in recovery
mode because it is possible to get unstable memory even if the complex
memory test has been passed.

Since the recovery mode runs from RO and we only have training data
cache for RW, the trained calibration data can't be saved since RO and
RW may be running different firmware.

Also revised few message to make it more clear for what calibration mode
(fast, full, or partial) has been executed.

BRANCH=kukui
BUG=b:139099592
TEST=emerge-kukui coreboot

Change-Id: I29e0df71dc3357462e15ce8fc2ba02f21b54ed33
Signed-off-by: Yu-Ping Wu 
---
M src/soc/mediatek/mt8183/memory.c
1 file changed, 25 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/36089/7

[Hung-Te Lin (Code Review)]

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

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

Patch Set 7: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36089 )

Change subject: soc/mediatek/mt8183: Skip fast calibration in recovery mode
......................................................................

soc/mediatek/mt8183: Skip fast calibration in recovery mode

SoC DRAM team suggested always running full calibration mode in recovery
mode because it is possible to get unstable memory even if the complex
memory test has been passed.

Since the recovery mode runs from RO and we only have training data
cache for RW, the trained calibration data can't be saved since RO and
RW may be running different firmware.

Also revised few message to make it more clear for what calibration mode
(fast, full, or partial) has been executed.

BRANCH=kukui
BUG=b:139099592
TEST=emerge-kukui coreboot

Change-Id: I29e0df71dc3357462e15ce8fc2ba02f21b54ed33
Signed-off-by: Yu-Ping Wu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36089
Tested-by: build bot (Jenkins) 
Reviewed-by: Hung-Te Lin 
---
M src/soc/mediatek/mt8183/memory.c
1 file changed, 25 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  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 b657708..2e63913 100644
--- a/src/soc/mediatek/mt8183/memory.c
+++ b/src/soc/mediatek/mt8183/memory.c
@@ -17,6 +17,7 @@
 #include <cbfs.h>
 #include <console/console.h>
 #include <ip_checksum.h>
+#include <security/vboot/vboot_common.h>
 #include <soc/dramc_param.h>
 #include <soc/dramc_pi_api.h>
 #include <soc/emi.h>
@@ -163,13 +164,21 @@
    if (CONFIG(MT8183_DRAM_EMCP))
    	config |= DRAMC_CONFIG_EMCP;

+	const bool recovery_mode = vboot_recovery_mode_enabled();
+
    /* Load calibration params from flash and run fast calibration */
-	if (dparam_ops->read_from_flash(dparam)) {
+	if (recovery_mode) {
+		printk(BIOS_WARNING, "Skip loading cached calibration data\n");
+	} else if (dparam_ops->read_from_flash(dparam)) {
+		printk(BIOS_INFO, "DRAM-K: Fast Calibration\n");
    	if (dram_run_fast_calibration(dparam, config) == 0) {
    		printk(BIOS_INFO,
-			       "DRAM calibraion params loaded from flash\n");
+			       "Calibration params loaded from flash\n");
    		if (mt_set_emi(dparam) == 0 && mt_mem_test() == 0)
    			return;
+		} else {
+			printk(BIOS_ERR,
+			       "Failed to apply cached calibration data\n");
    	}
    } else {
    	printk(BIOS_WARNING,
@@ -177,16 +186,23 @@
    }

/* Run full calibration */
+	printk(BIOS_INFO, "DRAM-K: Full Calibration\n");
    int err = dram_run_full_calibration(dparam, config);
    if (err == 0) {
    	printk(BIOS_INFO, "Successfully loaded DRAM blobs and "
    	       "ran DRAM calibration\n");
-		set_source_to_flash(dparam->freq_params);
-		dparam->header.checksum = compute_checksum(dparam);
-		dparam_ops->write_to_flash(dparam);
-		printk(BIOS_DEBUG, "Calibration params saved to flash: "
-		       "version=%#x, size=%#x\n",
-		       dparam->header.version, dparam->header.size);
+		/*
+		 * In recovery mode the system boots in RO but the flash params
+		 * should be calibrated for RW so we can't mix them up.
+		 */
+		if (!recovery_mode) {
+			set_source_to_flash(dparam->freq_params);
+			dparam->header.checksum = compute_checksum(dparam);
+			dparam_ops->write_to_flash(dparam);
+			printk(BIOS_DEBUG, "Calibration params saved to flash: "
+			       "version=%#x, size=%#x\n",
+			       dparam->header.version, dparam->header.size);
+		}
    	return;
    }

@@ -194,6 +210,7 @@
           "falling back to load default sdram param\n", err);

/* Init params from sdram configs and run partial calibration */
+	printk(BIOS_INFO, "DRAM-K: Partial Calibration\n");
    init_sdram_params(dparam->freq_params, get_sdram_config());
    if (mt_set_emi(dparam) != 0)
    	die("Set emi failed with params from sdram config\n");