Matt Papageorge has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from Kconfig options ......................................................................
soc/amd/common: Use SPI settings from Kconfig options
Since SPI speeds and mode are now set in Picasso Kconfig for EFS, remove duplicate values from device tree and instead harvest the values from Kconfig.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morphius.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com --- M src/mainboard/google/zork/variants/baseboard/devicetree.cb M src/soc/amd/common/block/include/amdblocks/chip.h M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c 4 files changed, 34 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/43303/1
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree.cb b/src/mainboard/google/zork/variants/baseboard/devicetree.cb index 9db0d23..71c5b29 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree.cb @@ -138,15 +138,6 @@ .tx_res_tune = 0x01, }"
- # SPI Configuration - register "common_config.spi_config" = "{ - .normal_speed = SPI_SPEED_66M, /* MHz */ - .fast_speed = SPI_SPEED_66M, /* MHz */ - .altio_speed = SPI_SPEED_66M, /* MHz */ - .tpm_speed = SPI_SPEED_66M, /* MHz */ - .read_mode = SPI_READ_MODE_DUAL112, - }" - # eSPI Configuration register "common_config.espi_config" = "{ .std_io_decode_bitmap = ESPI_DECODE_IO_0x80_EN | ESPI_DECODE_IO_0X60_0X64_EN, diff --git a/src/soc/amd/common/block/include/amdblocks/chip.h b/src/soc/amd/common/block/include/amdblocks/chip.h index d150464..8835152 100644 --- a/src/soc/amd/common/block/include/amdblocks/chip.h +++ b/src/soc/amd/common/block/include/amdblocks/chip.h @@ -7,17 +7,6 @@ #include <amdblocks/spi.h>
struct soc_amd_common_config { - /* - * SPI configuration - * Default values if not overridden by mainboard: - * Read mode - Normal 33MHz - * Normal speed - 66MHz - * Fast speed - 66MHz - * Alt speed - 66MHz - * TPM speed - 66MHz - */ - struct spi_config spi_config; - /* eSPI configuration */ struct espi_config espi_config; }; diff --git a/src/soc/amd/common/block/include/amdblocks/spi.h b/src/soc/amd/common/block/include/amdblocks/spi.h index d226e0c..9773465 100644 --- a/src/soc/amd/common/block/include/amdblocks/spi.h +++ b/src/soc/amd/common/block/include/amdblocks/spi.h @@ -58,22 +58,6 @@ #define SPI_FIFO_LAST_BYTE 0xc7 #define SPI_FIFO_DEPTH (SPI_FIFO_LAST_BYTE - SPI_FIFO)
-struct spi_config { - /* - * Default values if not overridden by mainboard: - * Read mode - Normal 33MHz - * Normal speed - 66MHz - * Fast speed - 66MHz - * Alt speed - 66MHz - * TPM speed - 66MHz - */ - enum spi_read_mode read_mode; - enum spi100_speed normal_speed; - enum spi100_speed fast_speed; - enum spi100_speed altio_speed; - enum spi100_speed tpm_speed; -}; - /* * Perform early SPI initialization: * 1. Sets SPI ROM base and enables SPI ROM diff --git a/src/soc/amd/common/block/spi/fch_spi.c b/src/soc/amd/common/block/spi/fch_spi.c index bf64c3f..5a1c945 100644 --- a/src/soc/amd/common/block/spi/fch_spi.c +++ b/src/soc/amd/common/block/spi/fch_spi.c @@ -49,16 +49,43 @@
static void fch_spi_config_mb_modes(void) { - const struct soc_amd_common_config *cfg = soc_get_common_config(); + uint8_t spi_speed = 0xff; + uint8_t spi_read_mode = 0xff;
- if (!cfg) - die("Common config structure is NULL!\n"); + if (CONFIG(SPI_SPEED_66M)) + spi_speed = SPI_SPEED_66M; + if (CONFIG(SPI_SPEED_33M)) + spi_speed = SPI_SPEED_33M; + if (CONFIG(SPI_SPEED_22M)) + spi_speed = SPI_SPEED_22M; + if (CONFIG(SPI_SPEED_16M)) + spi_speed = SPI_SPEED_16M; + if (CONFIG(SPI_SPEED_100M)) + spi_speed = SPI_SPEED_100M; + if (CONFIG(SPI_SPEED_800K)) + spi_speed = SPI_SPEED_800K;
- const struct spi_config *spi_cfg = &cfg->spi_config; + if (CONFIG(SPI_READ_MODE_NORMAL_33M)) + spi_read_mode = SPI_READ_MODE_NORMAL33M; + if (CONFIG(SPI_READ_MODE_DUAL_IO_112)) + spi_read_mode = SPI_READ_MODE_DUAL112; + if (CONFIG(SPI_READ_MODE_QUAD_IO_114)) + spi_read_mode = SPI_READ_MODE_QUAD114; + if (CONFIG(SPI_READ_MODE_DUAL_IO_122)) + spi_read_mode = SPI_READ_MODE_DUAL122; + if (CONFIG(SPI_READ_MODE_QUAD_IO_144)) + spi_read_mode = SPI_READ_MODE_QUAD144; + if (CONFIG(SPI_READ_MODE_NORMAL_66M)) + spi_read_mode = SPI_READ_MODE_NORMAL66M; + if (CONFIG(SPI_READ_MODE_FAST_READ)) + spi_read_mode = SPI_READ_MODE_FAST_READ;
- fch_spi_set_read_mode(spi_cfg->read_mode); - fch_spi_set_spi100(spi_cfg->normal_speed, spi_cfg->fast_speed, - spi_cfg->altio_speed, spi_cfg->tpm_speed); + if ((spi_speed == 0xff) || (spi_read_mode == 0xff)) + die("SPI configuration not properly set!\n"); + + fch_spi_set_read_mode(spi_read_mode); + fch_spi_set_spi100(spi_speed, spi_speed, + spi_speed, spi_speed); }
static void fch_spi_config_em100_modes(void)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from Kconfig options ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/chip.h:
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/in... PS1, Line 7: #include <amdblocks/spi.h> Remove this include?
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/sp... PS1, Line 57: if (CONFIG(SPI_SPEED_33M)) Should all these be else if? compiler will do dead code optimization regardless and just leave the assignment, but these options are mutually exclusive. We should treat them as such. Same for the modes below.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from Kconfig options ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43303/1//COMMIT_MSG@9 PS1, Line 9: Picasso Kconfig Yes, but you are editing common code.. not just picasso code.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from Kconfig options ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/sp... PS1, Line 52: uint8_t spi_speed = 0xff; I was actually thinking that coreboot can read out these values from EFS. That allows us to configure the SPI settings in one place. With that, we can also ensure that em100 works correctly even with PSP using the configuration from EFS.
Kconfigs --> Use AMDFWTOOL to configure the SPI settings correctly in EFS --> SPI speed can be overwritten in a generated EFS for special modes like em100.
PSP --> Uses EFS SPI settings to configure the SPI mode/speed coreboot --> Uses EFS SPI settings to configure the SPI mode/speed
Thus, if em100 is being used, only change required would be to set the EFS fields differently without even having to re-build the entire image.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43303
to look at the new patch set (#2).
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
soc/amd/common: Use SPI settings from EFS
Since SPI speeds and mode are now set in Picasso Embedded Firmware Structure (EFS), remove these settings from devicetree and instead harvest them from the EFS.
BUG=b:158755102 TEST=Boot test on Tremblye and Morphius. Boot times remain the same.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com --- M src/mainboard/google/zork/variants/baseboard/devicetree.cb M src/soc/amd/common/block/include/amdblocks/chip.h M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c 4 files changed, 44 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/43303/2
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43303/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43303/1//COMMIT_MSG@9 PS1, Line 9: Picasso Kconfig
Yes, but you are editing common code.. not just picasso code.
Ack
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/chip.h:
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/in... PS1, Line 7: #include <amdblocks/spi.h>
Remove this include?
Done
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/sp... PS1, Line 52: uint8_t spi_speed = 0xff;
I was actually thinking that coreboot can read out these values from EFS. […]
Good idea. new patch implements this method
https://review.coreboot.org/c/coreboot/+/43303/1/src/soc/amd/common/block/sp... PS1, Line 57: if (CONFIG(SPI_SPEED_33M))
Should all these be else if? compiler will do dead code optimization regardless and just leave the a […]
No longer applicable with the new patch but will keep it in mind for the future
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... PS4, Line 101: fch_spi_config_modes Can we just delete all of this now?
Justin Frodsham has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 4: Code-Review+1
Justin Frodsham has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Removed Code-Review+1 by Justin Frodsham justin.frodsham@amd.corp-partner.google.com
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... PS4, Line 92: *spi_speed, *spi_speed); I might not agree about using same max spi_speed for all fast,norm,alt and tpm here. But I don't have BOM or necessary datasheets to give any more accurate comments that I already have placed in CB:43306.
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... PS4, Line 101: fch_spi_config_modes
Can we just delete all of this now?
If you do, please verify if you need to configure different norm and fast spi_speed parameters based on the BOM. AFAIR one cannot parse devicetree parameters into commandline arguments to be passed for amdfwtool.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... PS4, Line 92: *spi_speed, *spi_speed);
I might not agree about using same max spi_speed for all fast,norm,alt and tpm here. […]
using the same value for all four is very likely wrong as the EFS only contains information on the SPI flash frequency and mode, but not about the other (e)SPI frequencies
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... PS4, Line 92: *spi_speed, *spi_speed);
using the same value for all four is very likely wrong as the EFS only contains information on the S […]
The specific SPI chip we use on Trembyle: Clock frequency for Read Data instruction (03h): 50 MHz Clock frequency for QPI Read instructions (0Bh, EBh, 0Ch), with different dummy clocks: 104 MHz Clock frequency for all other SPI/QPI instructions: 104 MHz
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 4:
(1 comment)
Looks like SPI settings have recently been refactored to be platform specific instead of the one in baseboard devicetree. Martin also has a patch up right now for moving spi speed configuration earlier.
So I will need to rebase this change. But before then we need to decide how to use to harvested the EFS speeds (if at all). Any ideas are welcome
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... PS4, Line 92: *spi_speed, *spi_speed);
The specific SPI chip we use on Trembyle: […]
So should we move to just configuring the SPI Fast speed from EFS and leave everything else the same? Seems like that is the best of both worlds, but much more complex.
Hello Justin Frodsham, build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Paul Menzel, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43303
to look at the new patch set (#5).
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
soc/amd/common: Use SPI settings from EFS
Use the new Picasso SPI Kconfig options for setting device tree SPI fast speed and mode.
BUG=b:158755102 TEST=Boot test on Tremblye and Morphius. 1st x86 timestamp improves by over a second. Verify correct values with a hex editor.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/soc/amd/common/block/include/amdblocks/spi.h 3 files changed, 63 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/43303/5
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43303/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb:
https://review.coreboot.org/c/coreboot/+/43303/5/src/mainboard/google/zork/v... PS5, Line 135: SPI_SPEED_66M I'm all for deleting all this if EFS takes care of setting it up.
https://review.coreboot.org/c/coreboot/+/43303/5/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/spi.h:
https://review.coreboot.org/c/coreboot/+/43303/5/src/soc/amd/common/block/in... PS5, Line 61: EFS_SPI_ST_READ_MODE where is this used?
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Set Ready For Review
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/5/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/spi.h:
https://review.coreboot.org/c/coreboot/+/43303/5/src/soc/amd/common/block/in... PS5, Line 61: EFS_SPI_ST_READ_MODE
where is this used?
Yea I guess we dont need this anymore
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/43303/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb:
https://review.coreboot.org/c/coreboot/+/43303/9/src/mainboard/google/zork/v... PS9, Line 135: .altio_speed = SPI_SPEED_66M, /* MHz */ : .tpm_speed = SPI_SPEED_66M, /* MHz */ Can't we just drop these two entries from trembyle and dalboz devicetree? None of them are really used on zork family. Not for this change, but probably as a follow-up.
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/spi.h:
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/in... PS9, Line 64: Read mode - Normal 33MHz This needs to be dropped too.
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/sp... PS9, Line 59: SPI_SPEED_16M, SPI_SPEED_16M Thinking about this again. I don't think these really matter for em100. All it cares about is normal read speed here. Not for this change, but probably we should fix this.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 9:
Please add to commit message that you have tested stoneyridge builds to not change.
While stoneyridge selects SOC_AMD_COMMON_BLOCK_SPI it does not actually call fch_spi_early_init() and the Makefiles were not modified to fill EFS. Even though support for fam15h was added to util/amdfwtool.
Hello Justin Frodsham, build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson, Paul Menzel, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43303
to look at the new patch set (#10).
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
soc/amd/common: Use SPI settings from EFS
Remove SPI fast speed and mode settings from the device tree. Also remove AMD common code for setting fast speed and mode. The previous EFS change makes setting this speed unnecessary as it should already be set much earlier during PSP execution.
BUG=b:158755102 TEST=Boot test on Tremblye and Morphius. 1st x86 timestamp improves by over a second. Verify correct values with a hex editor.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/soc/amd/common/block/include/amdblocks/spi.h M src/soc/amd/common/block/spi/fch_spi.c 4 files changed, 8 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/43303/10
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: soc/amd/common: Use SPI settings from EFS ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43303/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb:
https://review.coreboot.org/c/coreboot/+/43303/5/src/mainboard/google/zork/v... PS5, Line 135: SPI_SPEED_66M
I'm all for deleting all this if EFS takes care of setting it up.
Done for the two fields being utilized. Furquan has suggested removing this entire thing as well and we should accomplish this in a future change.
https://review.coreboot.org/c/coreboot/+/43303/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb:
https://review.coreboot.org/c/coreboot/+/43303/9/src/mainboard/google/zork/v... PS9, Line 135: .altio_speed = SPI_SPEED_66M, /* MHz */ : .tpm_speed = SPI_SPEED_66M, /* MHz */
Can't we just drop these two entries from trembyle and dalboz devicetree? None of them are really us […]
Ack
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/spi.h:
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/in... PS9, Line 64: Read mode - Normal 33MHz
This needs to be dropped too.
Done
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/sp... PS9, Line 59: SPI_SPEED_16M, SPI_SPEED_16M
Thinking about this again. I don't think these really matter for em100. […]
Agreed
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 16:
Patch Set 9:
Please add to commit message that you have tested stoneyridge builds to not change.
While stoneyridge selects SOC_AMD_COMMON_BLOCK_SPI it does not actually call fch_spi_early_init() and the Makefiles were not modified to fill EFS. Even though support for fam15h was added to util/amdfwtool.
I have refactored the change as follows. 1. Kconfig changes 2. Build changes for picasso platforms 3. Build changes for stoney ridge platforms
An optional future 4th change would be to address the duplicate definition of this information in mainboard/google/kahlee/bootblock/bootblock.c mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/4/src/soc/amd/common/block/sp... PS4, Line 101: fch_spi_config_modes
If you do, please verify if you need to configure different norm and fast spi_speed parameters based […]
Yes. Postponing this until a future change to get EFS in for now
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43303/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb:
https://review.coreboot.org/c/coreboot/+/43303/5/src/mainboard/google/zork/v... PS5, Line 135: SPI_SPEED_66M
Agreed.
Done
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/43303/9/src/soc/amd/common/block/sp... PS9, Line 59: SPI_SPEED_16M, SPI_SPEED_16M
I'm pretty sure that the em100 needs the fast read speeds changed too.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 16: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/google/zork/... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/google/zork/... PS16, Line 231: 2 On Ezkinil, I tested this at 4 with and without an em100. So I think we can change the default in a followup. I would rather get this submitted soon.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/amd/mandolin... File src/mainboard/amd/mandolin/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/amd/mandolin... PS16, Line 110: default 6 : default 0 if EM100 These are reversed for all 4 with an EM100. Kconfig only pays attention to the FIRST default it reads.
I think we should just make the em100 Kconfig get loaded first though. I'll discuss that with you on chat.
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/google/zork/... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/google/zork/... PS16, Line 232: default 0 if EM100 also needs to go first.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/amd/mandolin... File src/mainboard/amd/mandolin/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/amd/mandolin... PS16, Line 110: default 6 : default 0 if EM100
These are reversed for all 4 with an EM100. […]
+1 to default for EM100 to be placed first. Also, I think this can live in the soc/common/block/spi/Kconfig or at least at the Picasso SoC level.
Hello Justin Frodsham, build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Patrick Georgi, Marshall Dawson, Paul Menzel, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43303
to look at the new patch set (#17).
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
mb/amd,google/mandolin,zork: Set EFS SPI platform config
Set platform defaults for SPI settings in Kconfig for EFS.
BUG=b:158755102 TEST=Build and boot test on Tremblye and Morphius. Verify values in output image in a hex editor. Measure 1st x86 timestamp, perf improves by over a second.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com --- M src/mainboard/amd/mandolin/Kconfig M src/mainboard/google/zork/Kconfig M src/soc/amd/picasso/Makefile.inc 3 files changed, 35 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/43303/17
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/amd/mandolin... File src/mainboard/amd/mandolin/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/amd/mandolin... PS16, Line 110: default 6 : default 0 if EM100
+1 to default for EM100 to be placed first. […]
Done. For some reason I was thinking it was the other way around. I have learned a lot about kconfig while adding this feature, hopefully my future changes will benefit :)
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/google/zork/... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/16/src/mainboard/google/zork/... PS16, Line 232: default 0 if EM100
also needs to go first.
Done
Hello Justin Frodsham, build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Patrick Georgi, Marshall Dawson, Paul Menzel, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43303
to look at the new patch set (#18).
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
mb/amd,google/mandolin,zork: Set EFS SPI platform config
Set platform defaults for SPI settings in Kconfig for EFS.
BUG=b:158755102 TEST=Build and boot test on Tremblye and Morphius. Verify values in output image in a hex editor. Measure 1st x86 timestamp, perf improves by over a second.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com --- M src/mainboard/amd/mandolin/Kconfig M src/mainboard/google/zork/Kconfig M src/soc/amd/picasso/Makefile.inc 3 files changed, 35 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/43303/18
Hello Justin Frodsham, build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Patrick Georgi, Marshall Dawson, Paul Menzel, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43303
to look at the new patch set (#19).
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
mb/amd,google/mandolin,zork: Set EFS SPI platform config
Set platform defaults for SPI settings in Kconfig for EFS.
BUG=b:158755102 TEST=Build and boot test on Tremblye and Morphius. Verify values in output image in a hex editor. Measure 1st x86 timestamp, perf improves by over a second.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com --- M src/mainboard/amd/mandolin/Kconfig M src/mainboard/google/zork/Kconfig M src/soc/amd/picasso/Makefile.inc 3 files changed, 35 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/43303/19
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 19: Code-Review+2
Hello Justin Frodsham, build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Patrick Georgi, Marshall Dawson, Paul Menzel, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43303
to look at the new patch set (#20).
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
mb/amd,google/mandolin,zork: Set EFS SPI platform config
Set platform defaults for SPI settings in Kconfig for EFS.
BUG=b:158755102 TEST=Build and boot test on Tremblye and Morphius. Verify values in output image in a hex editor. Measure 1st x86 timestamp, perf improves by over a second.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com --- M src/mainboard/amd/mandolin/Kconfig M src/mainboard/google/zork/Kconfig M src/soc/amd/picasso/Makefile.inc 3 files changed, 35 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/43303/20
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 20:
Only difference with this patch is a re base and changing the Micron default (Trembyle uses Winbond)
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 20: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/20/src/mainboard/google/zork/... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/20/src/mainboard/google/zork/... PS20, Line 241: 0 Have you checked all the variants of Zork to ensure this is not required for any?
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/20/src/mainboard/google/zork/... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/20/src/mainboard/google/zork/... PS20, Line 241: 0
Have you checked all the variants of Zork to ensure this is not required for any?
Yes: Dalboz, Tremblye, Ezkinil and Morphius all use Winbond parts.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43303/20/src/mainboard/google/zork/... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43303/20/src/mainboard/google/zork/... PS20, Line 241: 0
Yes: Dalboz, Tremblye, Ezkinil and Morphius all use Winbond parts.
Thanks Matt!
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43303 )
Change subject: mb/amd,google/mandolin,zork: Set EFS SPI platform config ......................................................................
mb/amd,google/mandolin,zork: Set EFS SPI platform config
Set platform defaults for SPI settings in Kconfig for EFS.
BUG=b:158755102 TEST=Build and boot test on Tremblye and Morphius. Verify values in output image in a hex editor. Measure 1st x86 timestamp, perf improves by over a second.
Change-Id: I765dada14700f4800263d2d3844af07fad0e5b71 Signed-off-by: Matt Papageorge matthewpapa07@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43303 Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/amd/mandolin/Kconfig M src/mainboard/google/zork/Kconfig M src/soc/amd/picasso/Makefile.inc 3 files changed, 35 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/mainboard/amd/mandolin/Kconfig b/src/mainboard/amd/mandolin/Kconfig index 0913c15..03cbd4b 100644 --- a/src/mainboard/amd/mandolin/Kconfig +++ b/src/mainboard/amd/mandolin/Kconfig @@ -105,4 +105,18 @@ string default "3rdparty/amd_blobs/picasso/PicassoGenericVbios.bin" if BOARD_AMD_MANDOLIN
+config EFS_SPI_READ_MODE + int + default 0 if EM100 + default 5 + +config EFS_SPI_SPEED + int + default 3 if EM100 + default 0 + +config EFS_SPI_MICRON_FLAG + int + default 0 + endif # BOARD_AMD_MANDOLIN diff --git a/src/mainboard/google/zork/Kconfig b/src/mainboard/google/zork/Kconfig index b6ad7e0..6160021 100644 --- a/src/mainboard/google/zork/Kconfig +++ b/src/mainboard/google/zork/Kconfig @@ -226,4 +226,18 @@ bootblock. This implies that a static VBOOT2_WORK() buffer must be allocated in memlayout.
+config EFS_SPI_READ_MODE + int + default 4 if EM100 + default 2 + +config EFS_SPI_SPEED + int + default 3 if EM100 + default 0 + +config EFS_SPI_MICRON_FLAG + int + default 0 + endif # BOARD_GOOGLE_BASEBOARD_TREMBYLE || BOARD_GOOGLE_BASEBOARD_DALBOZ diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 4c1d726..bbb064f 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -322,6 +322,9 @@ OPT_PSP_SHAREDMEM_SIZE=$(call add_opt_prefix, $(PSP_SHAREDMEM_SIZE), --sharedmem-size) OPT_APOB_NV_SIZE=$(call add_opt_prefix, $(APOB_NV_SIZE), --apob-nv-size) OPT_APOB_NV_BASE=$(call add_opt_prefix, $(APOB_NV_BASE),--apob-nv-base) +OPT_EFS_SPI_READ_MODE=$(call add_opt_prefix, $(CONFIG_EFS_SPI_READ_MODE), --spi-read-mode) +OPT_EFS_SPI_SPEED=$(call add_opt_prefix, $(CONFIG_EFS_SPI_SPEED), --spi-speed) +OPT_EFS_SPI_MICRON_FLAG=$(call add_opt_prefix, $(CONFIG_EFS_SPI_MICRON_FLAG), --spi-micron-flag)
AMDFW_COMMON_ARGS=$(OPT_AMD_PUBKEY_FILE) \ $(OPT_PSPBTLDR_FILE) \ @@ -370,6 +373,10 @@ $(OPT_PSP_SHAREDMEM_SIZE) \ --combo-capable \ $(OPT_TOKEN_UNLOCK) \ + $(OPT_EFS_SPI_READ_MODE) \ + $(OPT_EFS_SPI_SPEED) \ + $(OPT_EFS_SPI_MICRON_FLAG) \ + --soc-name "Picasso" \ --flashsize $(CONFIG_ROM_SIZE)
# Copy prebuild APCBs if they exist
-
Aaron Durbin (Code Review) -
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Justin Frodsham (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Matt Papageorge (Code Review)
-
Raul Rangel (Code Review)