Change in coreboot[master]: mainboard/google: Allow Hatch variants to read SPD data over SMBus
Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
mainboard/google: Allow Hatch variants to read SPD data over SMBus
All Hatch variants so far embed static SPD data encoded within the firmware image. However we wish the flexibility for romstage implementations that allow for reading the SPD data dynamically over SMBus. This romstage variant allows for reading the SPD data over SMBus.
BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I3516a46b91840a9f6d1f4cffb2553d939d79cda2 Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc A src/mainboard/google/hatch/romstage_spd_smbus.c 3 files changed, 63 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/36449/1
diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index 219be22..e339693 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -60,7 +60,11 @@
config ROMSTAGE_SPD_CBFS bool - default y + default y if !ROMSTAGE_SPD_SMBUS + +config ROMSTAGE_SPD_SMBUS + bool + default n
config DRIVER_TPM_SPI_BUS default 0x1 diff --git a/src/mainboard/google/hatch/Makefile.inc b/src/mainboard/google/hatch/Makefile.inc index 3ed82e7..0740c08 100644 --- a/src/mainboard/google/hatch/Makefile.inc +++ b/src/mainboard/google/hatch/Makefile.inc @@ -21,6 +21,7 @@ ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC) += ec.c
romstage-$(CONFIG_ROMSTAGE_SPD_CBFS) += romstage_spd_cbfs.c +romstage-$(CONFIG_ROMSTAGE_SPD_SMBUS) += romstage_spd_smbus.c romstage-$(CONFIG_CHROMEOS) += chromeos.c
verstage-$(CONFIG_CHROMEOS) += chromeos.c diff --git a/src/mainboard/google/hatch/romstage_spd_smbus.c b/src/mainboard/google/hatch/romstage_spd_smbus.c new file mode 100644 index 0000000..245b61d --- /dev/null +++ b/src/mainboard/google/hatch/romstage_spd_smbus.c @@ -0,0 +1,57 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <baseboard/variants.h> +#include <soc/cnl_memcfg_init.h> +#include <soc/romstage.h> +#include <variant/gpio.h> +#include <spd_bin.h> +#include <gpio.h> + +/* + * GPIO_MEM_CH_SEL is set to 1 for single channel skus + * and 0 for dual channel skus. + */ +#define GPIO_MEM_CH_SEL GPP_F2 + +void mainboard_memory_init_params(FSPM_UPD *memupd) +{ + int is_single_ch_mem; + struct cnl_mb_cfg memcfg; + variant_memory_params(&memcfg); + + /* + * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single + * channel skus and 0 for dual channel skus. + */ + is_single_ch_mem = gpio_get(GPIO_MEM_CH_SEL); + + /* Read spd block to get memory config */ + struct spd_block blk = { + .addr_map = { 0x50, 0x52 }, + }; + + memcfg.dq_pins_interleaved = 1; + get_spd_smbus(&blk); + memcfg.spd[0].read_type = READ_SMBUS; + memcfg.spd[0].spd_spec.spd_smbus_address = (uintptr_t)blk.spd_array[0]; + if (!is_single_ch_mem) { + memcfg.spd[1].read_type = READ_SMBUS; + memcfg.spd[1].spd_spec.spd_smbus_address = (uintptr_t)blk.spd_array[1]; + } + dump_spd_info(&blk); + + cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); +}
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 1:
I would still rather see one file with just the memcfg.spd parts in an if/else block.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 1:
Patch Set 1:
I would still rather see one file with just the memcfg.spd parts in an if/else block.
Tim, the reasoning not using if/else blocks is because its isn't just a field set difference. The implementations are different. variant_memory_sku() in the base assumes variants define GPIO #defines which doesn't make sense for Puff so that ends up being wrapped as well. Further, mainboard_get_dram_part_num() isn't needed so that winds up being wrapped so we are just left with mainboard_memory_init_params() and diff'ing them reveals that although they are structurally similar they are semantically different as the configure FSP's struct is substantiated with totally different values.
The long story shorts is that it degenerates into having two files in one wrapped in large if-else blocks, with a tentative weak symbol in one path.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I would still rather see one file with just the memcfg.spd parts in an if/else block.
Tim, the reasoning not using if/else blocks is because its isn't just a field set difference. The implementations are different. variant_memory_sku() in the base assumes variants define GPIO #defines which doesn't make sense for Puff so that ends up being wrapped as well. Further, mainboard_get_dram_part_num() isn't needed so that winds up being wrapped so we are just left with mainboard_memory_init_params() and diff'ing them reveals that although they are structurally similar they are semantically different as the configure FSP's struct is substantiated with totally different values.
The long story shorts is that it degenerates into having two files in one wrapped in large if-else blocks, with a tentative weak symbol in one path.
Is Puff considered a Hatch variant?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
I would still rather see one file with just the memcfg.spd parts in an if/else block.
Tim, the reasoning not using if/else blocks is because its isn't just a field set difference. The implementations are different. variant_memory_sku() in the base assumes variants define GPIO #defines which doesn't make sense for Puff so that ends up being wrapped as well. Further, mainboard_get_dram_part_num() isn't needed so that winds up being wrapped so we are just left with mainboard_memory_init_params() and diff'ing them reveals that although they are structurally similar they are semantically different as the configure FSP's struct is substantiated with totally different values.
The long story shorts is that it degenerates into having two files in one wrapped in large if-else blocks, with a tentative weak symbol in one path.
Is Puff considered a Hatch variant?
We can fold https://review.coreboot.org/c/coreboot/+/36065 in with these changes and for the purposes of AP firmware it then is.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 48: memcfg.spd[0].read_type = READ_SMBUS; Why are we marking this is as READ_SMBUS when get_spd_smbus() is doing the reading and filling in the data contents?
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 49: memcfg.spd[0].spd_spec.spd_smbus_address = (uintptr_t)blk.spd_array[0]; spd_smbus_address is expecting an smbus address. 0xa0 (0x50 shifted) or 0xa4 (0x54 shifted). It's not expecting the contents themselves.
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 52: 1 I'm not sure this should be 1. There are some bizarre encodings in that I think you need to make this 2 because I believe these entries map to chip selects and for dual rank parts the 2 chip selects will be routed to a single dimm.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 48: memcfg.spd[0].read_type = READ_SMBUS;
Why are we marking this is as READ_SMBUS when get_spd_smbus() is doing the reading and filling in th […]
So I realised now that FSP has a parallel mechanism internally for reading SPD data aside from that of the native coreboot version of get_spd_smbus(). I have conflated them while looking at other Intel boards and I should choice just the one mechanism.
Thanks for catching this, I wonder if we should make a patch to ensure other platforms do this in a consistent manner as a separate thing?
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 49: memcfg.spd[0].spd_spec.spd_smbus_address = (uintptr_t)blk.spd_array[0];
spd_smbus_address is expecting an smbus address. 0xa0 (0x50 shifted) or 0xa4 (0x54 shifted). […]
Your absolutely correct, I mixed this up with 'MemorySpdPtr00' too much CamelCase for my dyslexic brain thanks for the catch!!
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 52: 1
I'm not sure this should be 1. […]
Interesting, noted thanks. I will rework this implementation based on this and the above. Thanks very much for looking it over as this is my first FSP experience so its very helpful for the second more informed eye.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 27: GPIO_MEM_CH_SEL I don't think we need this for boards reading SPD via SMBus.
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 48: memcfg.spd[0].read_type = READ_SMBUS; yes, I think we should just use get_spd_smbus() and pass in the data into FSP.
Thanks for catching this, I wonder if we should make a patch to ensure other platforms do this in a consistent manner as a separate thing?
That can be done as a follow-up.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36449
to look at the new patch set (#3).
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
mainboard/google: Allow Hatch variants to read SPD data over SMBus
All Hatch variants so far embed static SPD data encoded within the firmware image. However we wish the flexibility for romstage implementations that allow for reading the SPD data dynamically over SMBus. This romstage variant allows for reading the SPD data over SMBus.
BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I3516a46b91840a9f6d1f4cffb2553d939d79cda2 Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc A src/mainboard/google/hatch/romstage_spd_smbus.c 3 files changed, 54 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/36449/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36449/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/3/src/mainboard/google/hatch/... PS3, Line 26: struct spd_info spd[NUM_DIMM_SLOT] = { : { : .read_type = READ_SMBUS, : .spd_spec = {.spd_smbus_address = 0xa0}, : }, : { : .read_type = NOT_EXISTING, : }, : { : .read_type = READ_SMBUS, : .spd_spec = {.spd_smbus_address = 0xa4}, : }, : { : .read_type = NOT_EXISTING, : }, : }; : memcpy(memcfg.spd, spd, sizeof(memcfg.spd)); Could this be directly assigned to memcfg.spd instead?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36449
to look at the new patch set (#4).
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
mainboard/google: Allow Hatch variants to read SPD data over SMBus
All Hatch variants so far embed static SPD data encoded within the firmware image. However we wish the flexibility for romstage implementations that allow for reading the SPD data dynamically over SMBus. This romstage variant allows for reading the SPD data over SMBus.
V.2: Dispence with memcpy().
BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I3516a46b91840a9f6d1f4cffb2553d939d79cda2 Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc A src/mainboard/google/hatch/romstage_spd_smbus.c 3 files changed, 51 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/36449/4
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36449/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/3/src/mainboard/google/hatch/... PS3, Line 26: struct spd_info spd[NUM_DIMM_SLOT] = { : { : .read_type = READ_SMBUS, : .spd_spec = {.spd_smbus_address = 0xa0}, : }, : { : .read_type = NOT_EXISTING, : }, : { : .read_type = READ_SMBUS, : .spd_spec = {.spd_smbus_address = 0xa4}, : }, : { : .read_type = NOT_EXISTING, : }, : }; : memcpy(memcfg.spd, spd, sizeof(memcfg.spd));
Could this be directly assigned to memcfg. […]
memcpy() removed.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 4: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 25: READ_SMBUS Did you decide to not use get_spd_smbus()?
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 42: variant_memory_params Wouldn't the above configuration get overwritten by the weak implementation of variant_memory_params() in baseboard/memory.c.
Are you intending to provide a stronger implementation for boards using SMBus?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 25: READ_SMBUS
Did you decide to not use get_spd_smbus()?
I ended up going with FSP instead of a mix and match as it is a big awkward. This way the struct is just filled and your done.
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 42: variant_memory_params
Wouldn't the above configuration get overwritten by the weak implementation of variant_memory_params […]
Actually you are right. This is why I used the memcpy before this patch however it wasn't liked, can we just take the previous version and get this stuff moving forwards?
Frankly I just want us to get a building image of Puff then reshuffle things once the use-case is clear by letting the working code direct the design instead of designing too much ahead.
Hello Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36449
to look at the new patch set (#6).
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
mainboard/google: Allow Hatch variants to read SPD data over SMBus
All Hatch variants so far embed static SPD data encoded within the firmware image. However we wish the flexibility for romstage implementations that allow for reading the SPD data dynamically over SMBus. This romstage variant allows for reading the SPD data over SMBus.
V.2: Dispence with memcpy(). V.3: Revert back to previous patch with memcpy().
BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I3516a46b91840a9f6d1f4cffb2553d939d79cda2 Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc A src/mainboard/google/hatch/romstage_spd_smbus.c 3 files changed, 54 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/36449/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 25: READ_SMBUS
I ended up going with FSP instead of a mix and match as it is a big awkward. […]
Wouldn't it be something like: void mainboard_memory_init_params(FSPM_UPD *memupd) { » struct cnl_mb_cfg memcfg; » struct spd_block blk = { 0x50, 0x52 };
» variant_memory_params(&memcfg);
» get_spd_smbus(&blk); » memcfg.spd[0].read_type = READ_SPD_MEMPTR; » memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; » memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = blk.spd_array[0];
» memcfg.spd[1].read_type = NOT_EXISTING;
» memcfg.spd[2].read_type = READ_SPD_MEMPTR; » memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; » memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = blk.spd_array[1];
» memcfg.spd[3].read_type = NOT_EXISTING; }
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 25: READ_SMBUS
Wouldn't it be something like: […]
It could be done that way as well, its all a matter of taste I suppose. I can do it this way if you wish?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 25: READ_SMBUS
It could be done that way as well, its all a matter of taste I suppose. […]
I think it would be better to use get_spd_smbus() rather than relying on FSP to do the SMBus read.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 25: READ_SMBUS
I think it would be better to use get_spd_smbus() rather than relying on FSP to do the SMBus read.
Understood.
https://review.coreboot.org/c/coreboot/+/36449/5/src/mainboard/google/hatch/... PS5, Line 42: variant_memory_params
Actually you are right. […]
Done
Hello Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36449
to look at the new patch set (#7).
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
mainboard/google: Allow Hatch variants to read SPD data over SMBus
All Hatch variants so far embed static SPD data encoded within the firmware image. However we wish the flexibility for romstage implementations that allow for reading the SPD data dynamically over SMBus. This romstage variant allows for reading the SPD data over SMBus.
V.2: Dispence with memcpy(). V.3: Revert back to previous patch with memcpy(). V.4: Rewrite again to avoid memcpy().
BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I3516a46b91840a9f6d1f4cffb2553d939d79cda2 Signed-off-by: Edward O'Callaghan quasisec@chromium.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc A src/mainboard/google/hatch/romstage_spd_smbus.c 3 files changed, 56 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/36449/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/36449/8/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/8/src/mainboard/google/hatch/... PS8, Line 47: memcfg.vref_ca_config = 2; This overrides whatever the variant is setting. Just a note of caution in case variant decides to handle it differently.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36449/8/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/8/src/mainboard/google/hatch/... PS8, Line 47: memcfg.vref_ca_config = 2;
This overrides whatever the variant is setting. […]
Ack, I have ideas how to tidy things up going forwards however I am avoiding any pre-mature optimizations until I see things working. I'll make a bug for this and in general.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/romstage_spd_smbus.c:
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 27: GPIO_MEM_CH_SEL
I don't think we need this for boards reading SPD via SMBus.
Done
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 48: memcfg.spd[0].read_type = READ_SMBUS;
yes, I think we should just use get_spd_smbus() and pass in the data into FSP. […]
Done
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 49: memcfg.spd[0].spd_spec.spd_smbus_address = (uintptr_t)blk.spd_array[0];
Your absolutely correct, I mixed this up with 'MemorySpdPtr00' too much CamelCase for my dyslexic br […]
Done
https://review.coreboot.org/c/coreboot/+/36449/2/src/mainboard/google/hatch/... PS2, Line 52: 1
Interesting, noted thanks. I will rework this implementation based on this and the above. […]
Done
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
mainboard/google: Allow Hatch variants to read SPD data over SMBus
All Hatch variants so far embed static SPD data encoded within the firmware image. However we wish the flexibility for romstage implementations that allow for reading the SPD data dynamically over SMBus. This romstage variant allows for reading the SPD data over SMBus.
V.2: Dispence with memcpy(). V.3: Revert back to previous patch with memcpy(). V.4: Rewrite again to avoid memcpy().
BRANCH=none BUG=b:143134702 TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I3516a46b91840a9f6d1f4cffb2553d939d79cda2 Signed-off-by: Edward O'Callaghan quasisec@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/36449 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/Makefile.inc A src/mainboard/google/hatch/romstage_spd_smbus.c 3 files changed, 56 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index 219be22..e339693 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -60,7 +60,11 @@
config ROMSTAGE_SPD_CBFS bool - default y + default y if !ROMSTAGE_SPD_SMBUS + +config ROMSTAGE_SPD_SMBUS + bool + default n
config DRIVER_TPM_SPI_BUS default 0x1 diff --git a/src/mainboard/google/hatch/Makefile.inc b/src/mainboard/google/hatch/Makefile.inc index a226bd6..7ad7849 100644 --- a/src/mainboard/google/hatch/Makefile.inc +++ b/src/mainboard/google/hatch/Makefile.inc @@ -21,6 +21,7 @@ ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC) += ec.c
romstage-$(CONFIG_ROMSTAGE_SPD_CBFS) += romstage_spd_cbfs.c +romstage-$(CONFIG_ROMSTAGE_SPD_SMBUS) += romstage_spd_smbus.c romstage-$(CONFIG_CHROMEOS) += chromeos.c
verstage-$(CONFIG_CHROMEOS) += chromeos.c diff --git a/src/mainboard/google/hatch/romstage_spd_smbus.c b/src/mainboard/google/hatch/romstage_spd_smbus.c new file mode 100644 index 0000000..74d59a5 --- /dev/null +++ b/src/mainboard/google/hatch/romstage_spd_smbus.c @@ -0,0 +1,50 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <baseboard/variants.h> +#include <soc/cnl_memcfg_init.h> +#include <soc/romstage.h> +#include <spd_bin.h> + +void mainboard_memory_init_params(FSPM_UPD *memupd) +{ + struct cnl_mb_cfg memcfg; + variant_memory_params(&memcfg); + + /* Read spd block to get memory config */ + struct spd_block blk = { + .addr_map = { 0x50, 0x52, }, + }; + + /* Access memory info through SMBUS. */ + get_spd_smbus(&blk); + memcfg.spd[0].read_type = READ_SPD_MEMPTR; + memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; + memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[0]; + + memcfg.spd[1].read_type = NOT_EXISTING; + + memcfg.spd[2].read_type = READ_SPD_MEMPTR; + memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; + memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[1]; + + memcfg.spd[3].read_type = NOT_EXISTING; + dump_spd_info(&blk); + + /* set to 2 VREF_CA goes to CH_A and VREF_DQ_B goes to CH_B. */ + memcfg.vref_ca_config = 2; + + cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); +}
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36449 )
Change subject: mainboard/google: Allow Hatch variants to read SPD data over SMBus ......................................................................
Patch Set 9:
I just stumbled across this. If you introduce some logic for board variants to select between CBFS/SMBUS source for SPD, I strongly feel it should not be implemented under mainboard/. Maybe not even under cannonlake/ or soc/intel but lib/. The requirement seems very much vendor-agnostic to me.
-
Aaron Durbin (Code Review) -
Edward O'Callaghan (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Tim Wawrzynczak (Code Review)