Change in coreboot[master]: mb/google/dedede: re-arrange SPD indexes to keep all variants the same.
Hello Marco Chen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41723
to review the following change.
Change subject: mb/google/dedede: re-arrange SPD indexes to keep all variants the same. ......................................................................
mb/google/dedede: re-arrange SPD indexes to keep all variants the same.
Starting from proto 1 of Waddledee, the spd indexes of all variants will be the same and this is updated to newest schematic already.
On the other hand, proto 0 of Waddledee shipped with micron part which is moved to index 1 from 0. As a result, we need to handle this transition in memory.c of variant - waddledee based on board version.
BUG=b:152277273 BRANCH=none TEST=make sure waddledee proto 0 can boot up correctly with this new FW.
Change-Id: I1bf6f1b775a41a4d5b3963e3bda52e566f05a1c4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/dedede/variants/waddledee/Makefile.inc M src/mainboard/google/dedede/variants/waddledee/memory.c M src/mainboard/google/dedede/variants/wheelie/Makefile.inc 3 files changed, 28 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/41723/1
diff --git a/src/mainboard/google/dedede/variants/waddledee/Makefile.inc b/src/mainboard/google/dedede/variants/waddledee/Makefile.inc index dfb97ba..922c314 100644 --- a/src/mainboard/google/dedede/variants/waddledee/Makefile.inc +++ b/src/mainboard/google/dedede/variants/waddledee/Makefile.inc @@ -1,6 +1,6 @@ ## SPDX-License-Identifier: GPL-2.0-or-later
-SPD_SOURCES = SPD_LPDDR4X_200b_8Gb_4267_DDP_1x16 #0b0000 -SPD_SOURCES += empty #0b0001 +SPD_SOURCES = empty #0b0000 +SPD_SOURCES += SPD_LPDDR4X_200b_8Gb_4267_DDP_1x16 #0b0001
romstage-y += memory.c diff --git a/src/mainboard/google/dedede/variants/waddledee/memory.c b/src/mainboard/google/dedede/variants/waddledee/memory.c index 2c81650..754e12e 100644 --- a/src/mainboard/google/dedede/variants/waddledee/memory.c +++ b/src/mainboard/google/dedede/variants/waddledee/memory.c @@ -2,6 +2,7 @@
#include <baseboard/variants.h> #include <baseboard/gpio.h> +#include <console/console.h> #include <ec/google/chromeec/ec.h> #include <gpio.h>
@@ -15,3 +16,26 @@
return false; } + +int variant_memory_sku(void) +{ + gpio_t spd_gpios[] = { + GPIO_MEM_CONFIG_0, + GPIO_MEM_CONFIG_1, + GPIO_MEM_CONFIG_2, + GPIO_MEM_CONFIG_3, + }; + uint32_t spd_index = gpio_base2_value(spd_gpios, ARRAY_SIZE(spd_gpios)); + uint32_t board_ver; + + /* Starting from proto 1, we re-arrange the spd indexes. */ + if (!google_chromeec_get_board_version(&board_ver) && board_ver != 0) + return spd_index; + + /* In proto 0, the spd index 0 is the only SKU and indicates to 1 now. */ + if (spd_index != 0) + printk(BIOS_ERR, "spd index %d is not valid in proto 0.\n", + spd_index); + + return 1; +} diff --git a/src/mainboard/google/dedede/variants/wheelie/Makefile.inc b/src/mainboard/google/dedede/variants/wheelie/Makefile.inc index 6c8c913..e1aeeae 100644 --- a/src/mainboard/google/dedede/variants/wheelie/Makefile.inc +++ b/src/mainboard/google/dedede/variants/wheelie/Makefile.inc @@ -1,3 +1,4 @@ ## SPDX-License-Identifier: GPL-2.0-or-later
-SPD_SOURCES = Micron_MT53E512M32D2NP_2GB #0b0000 +SPD_SOURCES = empty #0b0000 +SPD_SOURCES += SPD_LPDDR4X_200b_8Gb_4267_DDP_1x16 #0b0001
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41723 )
Change subject: mb/google/dedede: re-arrange SPD indexes to keep all variants the same. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41723/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/wheelie/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41723/1/src/mainboard/google/dedede... PS1, Line 4: SPD_SOURCES += SPD_LPDDR4X_200b_8Gb_4267_DDP_1x16 #0b0001 The schematics is not reflecting this. Sorry if it has been addressed already.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41723 )
Change subject: mb/google/dedede: re-arrange SPD indexes to keep all variants the same. ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41723/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41723/1//COMMIT_MSG@9 PS1, Line 9: the spd indexes of all variants will : be the same and this is updated to newest schematic already. I don't think we need to do this. Variants are going to diverge from the reference designs at some point and so they don't really need to follow the same IDs.
https://review.coreboot.org/c/coreboot/+/41723/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/waddledee/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41723/1/src/mainboard/google/dedede... PS1, Line 3: empty Why? Let's not use empty IDs. Also these IDs were already configured in hardware for the current build, right?
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41723 )
Change subject: mb/google/dedede: re-arrange SPD indexes to keep all variants the same. ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41723/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41723/1//COMMIT_MSG@9 PS1, Line 9: the spd indexes of all variants will : be the same and this is updated to newest schematic already.
I don't think we need to do this. […]
Yes, that is true about variants can have their own spd index. But the CL here just follow the newest schematic of Waddledoo and Waddledee. Please check the newest schematic.
https://review.coreboot.org/c/coreboot/+/41723/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/waddledee/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41723/1/src/mainboard/google/dedede... PS1, Line 3: empty
Why? Let's not use empty IDs. […]
Please refer to Waddledee schematic in 2020-05-20 version and this was phased in the Waddledee proto 1 build which SMT date would be soon. And we should provide FW in 5/28.
https://review.coreboot.org/c/coreboot/+/41723/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/wheelie/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41723/1/src/mainboard/google/dedede... PS1, Line 4: SPD_SOURCES += SPD_LPDDR4X_200b_8Gb_4267_DDP_1x16 #0b0001
The schematics is not reflecting this. Sorry if it has been addressed already.
Wheelie is just start to do gerber out review and committed to follow updated schematic version (0520 update here) so Wheelie would be updated as well.
- peichao is in the review list - will double check in weekly meeting.
Marco Chen has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41723 )
Change subject: mb/google/dedede: re-arrange SPD indexes to keep all variants the same. ......................................................................
Abandoned
Team decides not to change the SPD index so abandon this one.
-
Furquan Shaikh (Code Review) -
Karthik Ramasubramanian (Code Review)
-
Marco Chen (Code Review)