Rob Barnes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44565 )
Change subject: mb/google/zork: Always populate 2 channels in Trembyle APCB ......................................................................
mb/google/zork: Always populate 2 channels in Trembyle APCB
Trembyle based boards should always populate both memory channels in the APCB. AGPIO84 selects 1 or 2 channels at runtime.
Dalboz based boards only support 1 channel an do not strap AGPIO84.
BUG=164675027 TEST=Boot Ezkinil, confirm dmidecode -t 17 is correct
Change-Id: Ibfbc4816571c424e5efd8bb8d90707235d3c1754 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/mainboard/google/zork/variants/baseboard/Makefile.inc M src/soc/amd/picasso/Makefile.inc 2 files changed, 9 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/44565/1
diff --git a/src/mainboard/google/zork/variants/baseboard/Makefile.inc b/src/mainboard/google/zork/variants/baseboard/Makefile.inc index c9e3657..d7aa3d3 100644 --- a/src/mainboard/google/zork/variants/baseboard/Makefile.inc +++ b/src/mainboard/google/zork/variants/baseboard/Makefile.inc @@ -44,16 +44,21 @@ # GPIO_NUMBER: FCH GPIO number # GPIO_IO_MUX: Value write to IOMUX to configure this GPIO # GPIO_BANK_CTL: Value write to GPIOBankCtl[23:16] to configure this GPIO +# APCB_POPULATE_2ND_CHANNEL: Populates 2nd memory channel in APCB when true. +# Trembyle based boards select 1 or 2 channels based on AGPIO84 +# Dalboz based boards only support 1 channel ifeq ($(CONFIG_BOARD_GOOGLE_BASEBOARD_TREMBYLE),y) APCB_BOARD_ID_GPIO0 = 121 1 0 APCB_BOARD_ID_GPIO1 = 120 1 0 APCB_BOARD_ID_GPIO2 = 131 3 0 APCB_BOARD_ID_GPIO3 = 116 1 0 +APCB_POPULATE_2ND_CHANNEL = true else ifeq ($(CONFIG_BOARD_GOOGLE_BASEBOARD_DALBOZ),y) APCB_BOARD_ID_GPIO0 = 132 1 0 APCB_BOARD_ID_GPIO1 = 90 1 0 APCB_BOARD_ID_GPIO2 = 86 3 0 APCB_BOARD_ID_GPIO3 = 69 1 0 +APCB_POPULATE_2ND_CHANNEL = false else $(error Undefined APCB selection GPIOS for Zork baseboard) endif #($(CONFIG_BOARD_GOOGLE_BASEBOARD_TREMBYLE),y) diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index bbb064f..c5a8261 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -197,7 +197,7 @@ #
# type = 0x60 -PSP_APCB_FILES=$(foreach f, $(APCB_SOURCES), $(obj)/APCB_$(f).bin) +PSP_APCB_FILES=$(foreach f, $(APCB_SOURCES), $(obj)/APCB_$(f:_x1=_x2).bin)
# type = 0x61 PSP_APOB_BASE=$(CONFIG_PSP_APOB_DRAM_ADDRESS) @@ -395,7 +395,8 @@ --board_id_gpio2 $(APCB_BOARD_ID_GPIO2) \ --board_id_gpio3 $(APCB_BOARD_ID_GPIO3)
-$(obj)/APCB_%_x1.bin: $$(SPD_SOURCES_DIR)/%.spd.hex \ +$(obj)/APCB_%_x2.bin: \ + $$(SPD_SOURCES_DIR)/%.spd.hex \ $(APCB_EDIT_TOOL) \ $(APCB_MAGIC_BLOB) $(APCB_EDIT_TOOL) \ @@ -404,21 +405,7 @@ --hex \ --strip_manufacturer_information \ --spd_0_0 $< \ - --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ - --board_id_gpio1 $(APCB_BOARD_ID_GPIO1) \ - --board_id_gpio2 $(APCB_BOARD_ID_GPIO2) \ - --board_id_gpio3 $(APCB_BOARD_ID_GPIO3) - -$(obj)/APCB_%_x2.bin: $$(SPD_SOURCES_DIR)/%.spd.hex \ - $(APCB_EDIT_TOOL) \ - $(APCB_MAGIC_BLOB) - $(APCB_EDIT_TOOL) \ - $(APCB_MAGIC_BLOB) \ - $@ \ - --hex \ - --strip_manufacturer_information \ - --spd_0_0 $< \ - --spd_1_0 $< \ + $$([ $(APCB_POPULATE_2ND_CHANNEL) = true ] && echo "--spd_1_0 $<") \ --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ --board_id_gpio1 $(APCB_BOARD_ID_GPIO1) \ --board_id_gpio2 $(APCB_BOARD_ID_GPIO2) \
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44565 )
Change subject: mb/google/zork: Always populate 2 channels in Trembyle APCB ......................................................................
Patch Set 1:
This change will be refactored when zork is integrated with spd_tools, but it would be good to get a build in with just this change first to lower the risk of the spd_tool changes.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44565 )
Change subject: mb/google/zork: Always populate 2 channels in Trembyle APCB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44565/1/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/44565/1/src/soc/amd/picasso/Makefil... PS1, Line 398: x2 When APCB_POPULATE_2ND_CHANNEL != true, won't this make an SPD with just 1 channel? Isn't it misleading to call it _x2?
Should we just get rid of the suffix all together? The suffix was needed when the APCBs were generated by amd-picasso-fsp so we could pick the correct one. Now that we generate it as part of the build, I think we can remove the suffix.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Martin Roth, Eric Peers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44565
to look at the new patch set (#2).
Change subject: mb/google/zork: Always populate 2 channels in Trembyle APCB ......................................................................
mb/google/zork: Always populate 2 channels in Trembyle APCB
Trembyle based boards should always populate both memory channels in the APCB. AGPIO84 selects 1 or 2 channels at runtime.
Dalboz based boards only support 1 channel an do not strap AGPIO84.
BUG=164675027 TEST=Boot Ezkinil and Dalboz, confirm dmidecode -t 17 is correct
Change-Id: Ibfbc4816571c424e5efd8bb8d90707235d3c1754 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/mainboard/google/zork/spd/Makefile.inc M src/mainboard/google/zork/variants/baseboard/Makefile.inc M src/mainboard/google/zork/variants/berknip/spd/Makefile.inc M src/mainboard/google/zork/variants/dalboz/spd/Makefile.inc M src/mainboard/google/zork/variants/dirinboz/spd/Makefile.inc M src/mainboard/google/zork/variants/ezkinil/spd/Makefile.inc M src/mainboard/google/zork/variants/morphius/spd/Makefile.inc M src/mainboard/google/zork/variants/vilboz/spd/Makefile.inc M src/mainboard/google/zork/variants/woomax/spd/Makefile.inc M src/soc/amd/picasso/Makefile.inc 10 files changed, 72 insertions(+), 91 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/44565/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44565 )
Change subject: mb/google/zork: Always populate 2 channels in Trembyle APCB ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44565/2/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/44565/2/src/soc/amd/picasso/Makefil... PS2, Line 410: $$([ $(APCB_POPULATE_2ND_CHANNEL) = true ] && echo "--spd_1_0 $<") Can you evaluate at Make time instead of bash?
$(if $(APCB_POPULATE_2ND_CHANNEL), "--spd_1_0 $<", "")
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44565 )
Change subject: mb/google/zork: Always populate 2 channels in Trembyle APCB ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44565/2/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/44565/2/src/soc/amd/picasso/Makefil... PS2, Line 410: $$([ $(APCB_POPULATE_2ND_CHANNEL) = true ] && echo "--spd_1_0 $<")
Can you evaluate at Make time instead of bash? […]
This feedback was added to https://review.coreboot.org/c/coreboot/+/44480
Rob Barnes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44565 )
Change subject: mb/google/zork: Always populate 2 channels in Trembyle APCB ......................................................................
Abandoned
Superseded by https://review.coreboot.org/c/coreboot/+/44480
-
Raul Rangel (Code Review) -
Rob Barnes (Code Review)