Change in coreboot[master]: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support
Hello Rob Barnes,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40421
to review the following change.
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
soc/amd/picasso: Add SPI speed devicetree settings and EM100 support
The EM100 SPI flash emulator has a limited SPI frequency support, so ignore higher frequency setting in the devicetree in the case that EM100 usage is selected in Kconfig.
BUG=b:147758054 BUG=b:153675510
Change-Id: I24c27ec39101c7c07bedc27056f690cf2cc54951 Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://chromium-review.googlesource.com/2045134 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/southbridge.c 3 files changed, 33 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40421/1
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index d9211b4..b5e9c25 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -32,6 +32,7 @@ select GENERIC_GPIO_LIB select IOAPIC select HAVE_USBDEBUG_OPTIONS + select HAVE_EM100_SUPPORT select TSC_MONOTONIC_TIMER select SOC_AMD_COMMON_BLOCK_SPI select TSC_SYNC_LFENCE diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index 53c0329..90d0af5 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -9,6 +9,7 @@ #include <commonlib/helpers.h> #include <drivers/i2c/designware/dw_i2c.h> #include <soc/i2c.h> +#include <soc/southbridge.h> #include <arch/acpi_device.h>
#define PICASSO_I2C_DEV_MAX 4 @@ -32,6 +33,14 @@ I2S_PINS_I2S_TDM = 4, I2S_PINS_UNCONF = 7, /* All pads will be input mode */ } acp_pin_cfg; + + /* SPI Config */ + bool spi_override_defaults; + uint32_t spi_normal_speed; + uint32_t spi_fast_speed; + uint32_t spi_altio_speed; + uint32_t spi_tpm_speed; + uint32_t spi_read_mode; };
typedef struct soc_amd_picasso_config config_t; diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index 6bedab0..4c463ad 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -238,6 +238,28 @@ & ~SPI_READ_MODE_MASK) | mode); }
+static void sb_spi_init(void) +{ + lpc_enable_spi_prefetch(); + sb_init_spi_base(); + sb_disable_4dw_burst(); + + const config_t *config = config_of_soc(); + if(!CONFIG(EM100) && config != NULL && config->spi_override_defaults) { + sb_read_mode(config->spi_read_mode); + sb_set_spi100(config->spi_normal_speed, + config->spi_fast_speed, + config->spi_altio_speed, + config->spi_tpm_speed); + } else { + sb_read_mode(SPI_READ_MODE_NOM); + sb_set_spi100(SPI_SPEED_16M, /* Normal */ + SPI_SPEED_16M, /* Fast */ + SPI_SPEED_16M, /* AltIO */ + SPI_SPEED_16M); /* TPM */ + } +} + static void fch_smbus_init(void) { /* 400 kHz smbus speed. */ @@ -263,11 +285,7 @@ if (CONFIG(POST_IO) && (CONFIG_POST_IO_PORT == 0x80) && CONFIG(PICASSO_LPC_IOMUX)) lpc_enable_port80(); - lpc_enable_spi_prefetch(); - sb_init_spi_base(); - sb_disable_4dw_burst(); - sb_set_spi100(SPI_SPEED_33M, SPI_SPEED_33M, - SPI_SPEED_16M, SPI_SPEED_16M); + sb_spi_init(); enable_acpimmio_decode_pm04(); fch_smbus_init(); sb_enable_cf9_io();
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40421/1/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/40421/1/src/soc/amd/picasso/southbr... PS1, Line 248: if(!CONFIG(EM100) && config != NULL && config->spi_override_defaults) { space required before the open parenthesis '('
Hello build bot (Jenkins), Raul Rangel, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40421
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
soc/amd/picasso: Add SPI speed devicetree settings and EM100 support
The EM100 SPI flash emulator has a limited SPI frequency support, so ignore higher frequency setting in the devicetree in the case that EM100 usage is selected in Kconfig.
BUG=b:147758054 BUG=b:153675510
Change-Id: I24c27ec39101c7c07bedc27056f690cf2cc54951 Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://chromium-review.googlesource.com/2045134 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/southbridge.c 3 files changed, 33 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40421/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
Patch Set 3:
Jenkins had some network problems; seems to be on the coreboot Gerrit side though, since I also had problems accessing coreboot Gerrit for a short time
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/chip.h@... PS3, Line 39: uint32_t spi_normal_speed; It would be helpful to add a comment here indicating what macros can be used for speed and read mode. In fact, I think we should change those to enum in southbridge.h? Then we can simply use the enum type here which will make it pretty clear.
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... PS3, Line 248: && config != NULL This check is redundant. config_of_soc() takes care of ensuring config is not NULL.
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... PS3, Line 288: sb_spi_init I think this function is executed too late. Looking at the current series of patches, my understanding is that picasso will be having a bootblock. So, these settings should be done in bootblock_soc_init() or its equivalent for picasso.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
Patch Set 3: Code-Review-2
(1 comment)
needs some rework; will look into that tomorrow
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/chip.h@... PS3, Line 39: uint32_t spi_normal_speed;
It would be helpful to add a comment here indicating what macros can be used for speed and read mode […]
good point; will do that tomorrow
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... PS3, Line 288: sb_spi_init
I think this function is executed too late. […]
fch_pre_init() is called as part of bootblock as per https://review.coreboot.org/c/coreboot/+/37490/13/src/soc/amd/picasso/bootbl.... So this should still work fine.
Furquan Shaikh has uploaded a new patch set (#4) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
soc/amd/picasso: Allow mainboard to configure SPI settings
This change adds options to allow mainboard to configure SPI speed for different modes as well as the SPI read mode.
BUG=b:153675510,b:147758054 BRANCH=trembyle-bringup TEST=Verified that SPI settings are configured correctly for trembyle.
Change-Id: I24c27ec39101c7c07bedc27056f690cf2cc54951 Signed-off-by: Furquan Shaikh furquan@google.com Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/southbridge.c 2 files changed, 23 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40421/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40421/4/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/4/src/soc/amd/picasso/chip.h@... PS4, Line 78: * Default values if not overriden by mainboard: 'overriden' may be misspelled - perhaps 'overridden'?
Furquan Shaikh has uploaded a new patch set (#5) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
soc/amd/picasso: Allow mainboard to configure SPI settings
This change adds options to allow mainboard to configure SPI speed for different modes as well as the SPI read mode.
BUG=b:153675510,b:147758054 BRANCH=trembyle-bringup TEST=Verified that SPI settings are configured correctly for trembyle.
Change-Id: I24c27ec39101c7c07bedc27056f690cf2cc54951 Signed-off-by: Furquan Shaikh furquan@google.com Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/southbridge.c 2 files changed, 23 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40421/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40421/4/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/4/src/soc/amd/picasso/chip.h@... PS4, Line 78: * Default values if not overriden by mainboard:
'overriden' may be misspelled - perhaps 'overridden'?
Done
Furquan Shaikh has uploaded a new patch set (#6) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
soc/amd/picasso: Allow mainboard to configure SPI settings
This change adds options to allow mainboard to configure SPI speed for different modes as well as the SPI read mode.
BUG=b:153675510,b:147758054 BRANCH=trembyle-bringup TEST=Verified that SPI settings are configured correctly for trembyle.
Change-Id: I24c27ec39101c7c07bedc27056f690cf2cc54951 Signed-off-by: Furquan Shaikh furquan@google.com Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/southbridge.c 2 files changed, 22 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/40421/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Patch Set 7: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/chip.h@... PS3, Line 39: uint32_t spi_normal_speed;
good point; will do that tomorrow
Done
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... PS3, Line 248: && config != NULL
This check is redundant. config_of_soc() takes care of ensuring config is not NULL.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40421/8/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/8/src/soc/amd/picasso/chip.h@... PS8, Line 84: spi_read_mode Instead of placing all these in the main picasso config, would it be cleaner if we defined a SPI chip in the device tree?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40421/8/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/8/src/soc/amd/picasso/chip.h@... PS8, Line 84: spi_read_mode
Instead of placing all these in the main picasso config, would it be cleaner if we defined a SPI chi […]
Interesting idea. I went with this because this is what we have been doing for all platforms (Intel as well). And for other IPs too: Example I2C config on lines 25-26.
Also, we don't really have a SPI device in the devicetree. In my opinion, let's continue with this for now and we can look at all the IPs together and see if adding chip drivers for each separately makes sense.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
What do we do about Felix's -2? He's OOO. Should we push a new patch?
https://review.coreboot.org/c/coreboot/+/40421/8/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/8/src/soc/amd/picasso/chip.h@... PS8, Line 84: spi_read_mode
Interesting idea. […]
Ack
Aaron Durbin has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Removed Code-Review-2 by Felix Held felix-coreboot@felixheld.de
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
Patch Set 8:
Patch Set 8: Code-Review+2
(1 comment)
What do we do about Felix's -2? He's OOO. Should we push a new patch?
I just removed Felix as a reviewer given the OOO situation.
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Allow mainboard to configure SPI settings ......................................................................
soc/amd/picasso: Allow mainboard to configure SPI settings
This change adds options to allow mainboard to configure SPI speed for different modes as well as the SPI read mode.
BUG=b:153675510,b:147758054 BRANCH=trembyle-bringup TEST=Verified that SPI settings are configured correctly for trembyle.
Change-Id: I24c27ec39101c7c07bedc27056f690cf2cc54951 Signed-off-by: Furquan Shaikh furquan@google.com Signed-off-by: Rob Barnes robbarnes@google.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/40421 Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/southbridge.c 2 files changed, 22 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index 4cc10ef..edb1b69 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -10,6 +10,7 @@ #include <drivers/i2c/designware/dw_i2c.h> #include <soc/i2c.h> #include <soc/iomap.h> +#include <soc/southbridge.h> #include <arch/acpi_device.h>
struct soc_amd_picasso_config { @@ -70,6 +71,21 @@ uint8_t core_dldo_bypass; uint8_t min_soc_vid_offset; uint8_t aclk_dpm0_freq_400MHz; + + /* + * 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 spi_read_mode; + enum spi100_speed spi_normal_speed; + enum spi100_speed spi_fast_speed; + enum spi100_speed spi_altio_speed; + enum spi100_speed spi_tpm_speed; };
typedef struct soc_amd_picasso_config config_t; diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c index c38f373..4f25802 100644 --- a/src/soc/amd/picasso/southbridge.c +++ b/src/soc/amd/picasso/southbridge.c @@ -23,6 +23,7 @@ #include <soc/pci_devs.h> #include <soc/nvs.h> #include <types.h> +#include "chip.h"
#define FCH_AOAC_UART_FOR_CONSOLE \ (CONFIG_UART_FOR_CONSOLE == 0 ? FCH_AOAC_DEV_UART0 \ @@ -237,8 +238,11 @@
static void sb_spi_config_modes(void) { - sb_set_spi100(SPI_SPEED_33M, SPI_SPEED_33M, - SPI_SPEED_16M, SPI_SPEED_16M); + const struct soc_amd_picasso_config *cfg = config_of_soc(); + + sb_read_mode(cfg->spi_read_mode); + sb_set_spi100(cfg->spi_normal_speed, cfg->spi_fast_speed, cfg->spi_altio_speed, + cfg->spi_tpm_speed); }
static void sb_spi_init(void)
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Raul Rangel (Code Review)