Change in coreboot[master]: mb/google/asurada: Init dram in romstage

[CK HU (Code Review)]

Hello Duan huayang,

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

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

to review the following change.

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

mb/google/asurada: Init dram in romstage

Signed-off-by: Huayang Duan 
Change-Id: Ied350570a695cca1424a6562e41120bcaf467797
---
M src/mainboard/google/asurada/Makefile.inc
A src/mainboard/google/asurada/romstage.c
2 files changed, 50 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/44568/1

diff --git a/src/mainboard/google/asurada/Makefile.inc b/src/mainboard/google/asurada/Makefile.inc
index 8395e4a..c742539 100644
--- a/src/mainboard/google/asurada/Makefile.inc
+++ b/src/mainboard/google/asurada/Makefile.inc
@@ -8,6 +8,7 @@

romstage-y += memlayout.ld
 romstage-y += boardid.c
+romstage-y += romstage.c
 romstage-y += sdram_configs.c

ramstage-y += memlayout.ld
diff --git a/src/mainboard/google/asurada/romstage.c b/src/mainboard/google/asurada/romstage.c
new file mode 100644
index 0000000..8fa2f38
--- /dev/null
+++ b/src/mainboard/google/asurada/romstage.c
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <arch/stages.h>
+#include <console/console.h>
+#include <fmap.h>
+#include <soc/dramc_param.h>
+#include <soc/emi.h>
+#include <soc/mmu_operations.h>
+
+/* This must be defined in chromeos.fmd in same name and size. */
+#define CALIBRATION_REGION		"RW_DDR_TRAINING"
+#define CALIBRATION_REGION_SIZE		0x2000
+
+_Static_assert(sizeof(struct dramc_param) <= CALIBRATION_REGION_SIZE,
+	       "sizeof(struct dramc_param) exceeds " CALIBRATION_REGION);
+
+static bool read_calibration_data_from_flash(struct dramc_param *dparam)
+{
+	const size_t length = sizeof(*dparam);
+	size_t ret = fmap_read_area(CALIBRATION_REGION, dparam, length);
+	printk(BIOS_DEBUG, "read data from flash, ret=%#lx, length=%#lx\n", ret, length);
+
+	return ret == length;
+}
+
+static bool write_calibration_data_to_flash(const struct dramc_param *dparam)
+{
+	const size_t length = sizeof(*dparam);
+	size_t ret = fmap_overwrite_area(CALIBRATION_REGION, dparam, length);
+	printk(BIOS_DEBUG, "write data from flash, ret=%#lx, length=%#lx\n", ret, length);
+
+	return ret == length;
+}
+
+/* dramc_param is ~2K and too large to fit in stack. */
+static struct dramc_param dramc_parameter = {0x0};
+
+static struct dramc_param_ops dparam_ops = {
+	.param = &dramc_parameter,
+	.read_from_flash = &read_calibration_data_from_flash,
+	.write_to_flash = &write_calibration_data_to_flash,
+};
+
+void platform_romstage_main(void)
+{
+	mt_mem_init(&dparam_ops);
+	mtk_mmu_after_dram();
+	printk(BIOS_WARNING, "after mt_mem_init\n");
+}

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44568/1/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/1/src/mainboard/google/asurad... 
PS1, Line 48: BIOS_WARNING
BIOS_INFO, or just remove this.

[Duan huayang (Code Review)]

Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44568/1/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/1/src/mainboard/google/asurad... 
PS1, Line 48: BIOS_WARNING
> BIOS_INFO, or just remove this.

Done

[CK HU (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Yu-Ping Wu, Duan huayang,

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

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

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

mb/google/asurada: Init dram in romstage

Signed-off-by: Huayang Duan 
Change-Id: Ied350570a695cca1424a6562e41120bcaf467797
---
M src/mainboard/google/asurada/Makefile.inc
A src/mainboard/google/asurada/romstage.c
2 files changed, 49 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/44568/2

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 2: Code-Review-1

src/mainboard/google/asurada/romstage.c:46:2: error: implicit declaration of function 'mt_mem_init'; did you mean 'mtk_mmu_init'? [-Werror=implicit-function-declaration]

[Duan huayang (Code Review)]

Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 3:
> Patch Set 2: Code-Review-1
>
> src/mainboard/google/asurada/romstage.c:46:2: error: implicit declaration of function 'mt_mem_init'; did you mean 'mtk_mmu_init'? [-Werror=implicit-function-declaration]

adjust the depend order of patch list, all change can build pass now.

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 3:

please rebase, it can build I think.

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 5: -Code-Review

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 5:

(4 comments)

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
PS5, Line 21: #lx
"#zx" for size_t

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
PS5, Line 21: #lx
"#zx" for size_t

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
PS5, Line 30: 	printk(BIOS_DEBUG, "write data from flash, ret=%#lx, length=%#lx\n", ret, length);
Same. "#zx" for size_t

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
PS5, Line 36:  = {0x0}
I think there's no need to explicitly initialize this to 0.

[CK HU (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Yu-Ping Wu, Duan huayang,

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

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

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

mb/google/asurada: Init dram in romstage

Signed-off-by: Huayang Duan 
Change-Id: Ied350570a695cca1424a6562e41120bcaf467797
---
M src/mainboard/google/asurada/Makefile.inc
A src/mainboard/google/asurada/romstage.c
2 files changed, 49 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/44568/6

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 6:

(4 comments)

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
PS5, Line 21: #lx
> "#zx" for size_t

Ack

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
PS5, Line 21: #lx
> "#zx" for size_t

Ack

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
PS5, Line 30: 	printk(BIOS_DEBUG, "write data from flash, ret=%#lx, length=%#lx\n", ret, length);
> Same. […]

Ack

https://review.coreboot.org/c/coreboot/+/44568/5/src/mainboard/google/asurad... 
PS5, Line 36:  = {0x0}
> I think there's no need to explicitly initialize this to 0.

Ack

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 6: Code-Review+2

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 6:

(4 comments)

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
PS6, Line 21: %zx
%#zx

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
PS6, Line 21: %zx
%#zx

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
PS6, Line 30: %zx
%#zx

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
PS6, Line 30: %zx
%#zx

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 6: -Code-Review

oops

[Yidi Lin (Code Review)]

Yidi Lin has uploaded a new patch set (#9) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

mb/google/asurada: Init dram in romstage

Signed-off-by: Huayang Duan 
Signed-off-by: Yidi Lin 
Change-Id: Ied350570a695cca1424a6562e41120bcaf467797
---
M src/mainboard/google/asurada/Makefile.inc
A src/mainboard/google/asurada/romstage.c
2 files changed, 49 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/44568/9

[Yidi Lin (Code Review)]

Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 9:

(4 comments)

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
PS6, Line 21: %zx
> %#zx

Ack

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
PS6, Line 21: %zx
> %#zx

Ack

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
PS6, Line 30: %zx
> %#zx

Ack

https://review.coreboot.org/c/coreboot/+/44568/6/src/mainboard/google/asurad... 
PS6, Line 30: %zx
> %#zx

Ack

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 9: Code-Review+2

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 10: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44568/11/src/mainboard/google/asura... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/11/src/mainboard/google/asura... 
PS11, Line 17: static bool read_calibration_data_from_flash(struct dramc_param *dparam)
             : {
             : 	const size_t length = sizeof(*dparam);
             : 	size_t ret = fmap_read_area(CALIBRATION_REGION, dparam, length);
             : 	printk(BIOS_DEBUG, "read data from flash, ret=%#zx, length=%#zx\n", ret, length);
             : 
             : 	return ret == length;
             : }
             : 
             : static bool write_calibration_data_to_flash(const struct dramc_param *dparam)
             : {
             : 	const size_t length = sizeof(*dparam);
             : 	size_t ret = fmap_overwrite_area(CALIBRATION_REGION, dparam, length);
             : 	printk(BIOS_DEBUG, "write data from flash, ret=%#zx, length=%#zx\n", ret, length);
             : 
             : 	return ret == length;
             : }
Why can’t these be SoC functions?

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44568/11/src/mainboard/google/asura... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/11/src/mainboard/google/asura... 
PS11, Line 17: static bool read_calibration_data_from_flash(struct dramc_param *dparam)
             : {
             : 	const size_t length = sizeof(*dparam);
             : 	size_t ret = fmap_read_area(CALIBRATION_REGION, dparam, length);
             : 	printk(BIOS_DEBUG, "read data from flash, ret=%#zx, length=%#zx\n", ret, length);
             : 
             : 	return ret == length;
             : }
             : 
             : static bool write_calibration_data_to_flash(const struct dramc_param *dparam)
             : {
             : 	const size_t length = sizeof(*dparam);
             : 	size_t ret = fmap_overwrite_area(CALIBRATION_REGION, dparam, length);
             : 	printk(BIOS_DEBUG, "write data from flash, ret=%#zx, length=%#zx\n", ret, length);
             : 
             : 	return ret == length;
             : }
> Why can’t these be SoC functions?

The SoC may have different approaches for storing calibration data, especially there is no guarantee the region name will be the same across different boards (in fact, we do plan to change that soon).

I think it should be fine to keep current version until we have finished the migration to unified mrc_cache and then it can live in common driver, or as SoC functions.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

mb/google/asurada: Init dram in romstage

Signed-off-by: Huayang Duan 
Signed-off-by: Yidi Lin 
Change-Id: Ied350570a695cca1424a6562e41120bcaf467797
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44568
Reviewed-by: Yu-Ping Wu 
Reviewed-by: Hung-Te Lin 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/asurada/Makefile.inc
A src/mainboard/google/asurada/romstage.c
2 files changed, 49 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Hung-Te Lin: Looks good to me, approved
  Yu-Ping Wu: Looks good to me, approved

diff --git a/src/mainboard/google/asurada/Makefile.inc b/src/mainboard/google/asurada/Makefile.inc
index 8395e4a..c742539 100644
--- a/src/mainboard/google/asurada/Makefile.inc
+++ b/src/mainboard/google/asurada/Makefile.inc
@@ -8,6 +8,7 @@

romstage-y += memlayout.ld
 romstage-y += boardid.c
+romstage-y += romstage.c
 romstage-y += sdram_configs.c

ramstage-y += memlayout.ld
diff --git a/src/mainboard/google/asurada/romstage.c b/src/mainboard/google/asurada/romstage.c
new file mode 100644
index 0000000..a0e0818
--- /dev/null
+++ b/src/mainboard/google/asurada/romstage.c
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <arch/stages.h>
+#include <console/console.h>
+#include <fmap.h>
+#include <soc/dramc_param.h>
+#include <soc/emi.h>
+#include <soc/mmu_operations.h>
+
+/* This must be defined in chromeos.fmd in same name and size. */
+#define CALIBRATION_REGION		"RW_DDR_TRAINING"
+#define CALIBRATION_REGION_SIZE		0x2000
+
+_Static_assert(sizeof(struct dramc_param) <= CALIBRATION_REGION_SIZE,
+	       "sizeof(struct dramc_param) exceeds " CALIBRATION_REGION);
+
+static bool read_calibration_data_from_flash(struct dramc_param *dparam)
+{
+	const size_t length = sizeof(*dparam);
+	size_t ret = fmap_read_area(CALIBRATION_REGION, dparam, length);
+	printk(BIOS_DEBUG, "read data from flash, ret=%#zx, length=%#zx\n", ret, length);
+
+	return ret == length;
+}
+
+static bool write_calibration_data_to_flash(const struct dramc_param *dparam)
+{
+	const size_t length = sizeof(*dparam);
+	size_t ret = fmap_overwrite_area(CALIBRATION_REGION, dparam, length);
+	printk(BIOS_DEBUG, "write data from flash, ret=%#zx, length=%#zx\n", ret, length);
+
+	return ret == length;
+}
+
+/* dramc_param is ~2K and too large to fit in stack. */
+static struct dramc_param dramc_parameter;
+
+static struct dramc_param_ops dparam_ops = {
+	.param = &dramc_parameter,
+	.read_from_flash = &read_calibration_data_from_flash,
+	.write_to_flash = &write_calibration_data_to_flash,
+};
+
+void platform_romstage_main(void)
+{
+	mt_mem_init(&dparam_ops);
+	mtk_mmu_after_dram();
+}

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44568 )

Change subject: mb/google/asurada: Init dram in romstage
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44568/12/src/mainboard/google/asura... 
File src/mainboard/google/asurada/romstage.c:

https://review.coreboot.org/c/coreboot/+/44568/12/src/mainboard/google/asura... 
PS12, Line 30: from
This should be "to"