Change in ...coreboot[master]: soc/intel/cannonlake: Pass more SPI options to FSP.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32791

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

soc/intel/cannonlake: Pass more SPI options to FSP.

Add the ability for the device tree to set CsEnable, CsPolarity, and
DefaultCsOutput parameters to the FSP.

BUG=b:130329260
BRANCh=none
TEST=Compiles.

Change-Id: Ic816395b7d198a52c704e6cabcb56889150b741c
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/include/soc/serialio.h
3 files changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32791/1

diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h
index 2b2a51f..c3f3023 100644
--- a/src/soc/intel/cannonlake/chip.h
+++ b/src/soc/intel/cannonlake/chip.h
@@ -338,6 +338,17 @@
     */
    uint8_t SerialIoDevMode[PchSerialIoIndexMAX];

+	/* 0 = Active low CS, 1 = Active high CS */
+	uint8_t SerialIoSpi0CsPolarity[2];
+	uint8_t SerialIoSpi1CsPolarity[2];
+	uint8_t SerialIoSpi2CsPolarity[2];
+	/* 0 = GPIO, 1 = Chip select */
+	uint8_t SerialIoSpi0CsEnable[2];
+	uint8_t SerialIoSpi1CsEnable[2];
+	uint8_t SerialIoSpi2CsEnable[2];
+	/* Default CS as output; 0 = CS0, 1 = CS1 */
+	uint8_t SerialIoSpiDefaultCsOutput[3];
+
    /* GPIO SD card detect pin */
    unsigned int sdcard_cd_gpio;

diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index cc01d10..dd5e2c4 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -86,6 +86,20 @@
    	params->SerialIoUartMode[i] =
    			get_param_value(config, dev_offset);
    }
+
+	for (i = 0; i < 2; i++) {
+		params->SerialIoSpi0CsPolarity[i] =
+			config->SerialIoSpi0CsPolarity[i];
+		params->SerialIoSpi1CsPolarity[i] =
+			config->SerialIoSpi1CsPolarity[i];
+		params->SerialIoSpi2CsPolarity[i] =
+			config->SerialIoSpi2CsPolarity[i];
+	}
+
+	for (i = 0; i < 3; i++) {
+		params->SerialIoSpiDefaultCsOutput[i] =
+			config->SerialIoSpiDefaultCsOutput[i];
+	}
 }
 #else
 static void parse_devicetree_param(const config_t *config, FSP_S_CONFIG *params)
diff --git a/src/soc/intel/cannonlake/include/soc/serialio.h b/src/soc/intel/cannonlake/include/soc/serialio.h
index c92bd2d..5ce4a80 100644
--- a/src/soc/intel/cannonlake/include/soc/serialio.h
+++ b/src/soc/intel/cannonlake/include/soc/serialio.h
@@ -54,4 +54,19 @@
    PchSerialIoIndexMAX
 } PCH_SERIAL_IO_CONTROLLER;

+typedef enum {
+	PchSerialIoCsActiveLow,
+	PchSerialIoCsActiveHigh,
+} PCH_SPI_CS_ACTIVE_MODE;
+
+typedef enum {
+	PchSerialIoCsGpio,
+	PchSerialIoCsEnable,
+} PCH_SPI_CS_ENABLE_MODE;
+
+typedef enum {
+	PchSerialIoCS0,
+	PchSerialIoCS1,
+} PCH_SPI_CS_SELECT;
+
 #endif

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Patch Set 1:

(1 comment)

These settings are just tweaking configuration int he designware ip block, no? Why are we putting in code to feed this into FSP when we already have code which configures everything?

https://review.coreboot.org/#/c/32791/1/src/soc/intel/cannonlake/chip.h 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/#/c/32791/1/src/soc/intel/cannonlake/chip.h@342 
PS1, Line 342: 2
This should be based on a macro, and the bounds in the code should use ARRAY_SIZE() -- not open coding indicies and sizing.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (1 comment)
>
> These settings are just tweaking configuration int he designware ip block, no? Why are we putting in code to feed this into FSP when we already have code which configures everything?

Unfortunately, FSP ends up configuring these based on the default values in the UPDs. Ideally, FSP should not configure any of this. But currently there is no option to say skip all this programming.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Abandoned

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has restored this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Restored

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Paul Fagerburg, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32791

to look at the new patch set (#2).

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

soc/intel/cannonlake: Pass more SPI options to FSP.

Add the ability for the device tree to set CsEnable, CsPolarity, and
DefaultCsOutput parameters to the FSP.

BUG=b:130329260
BRANCh=none
TEST=Compiles.

Change-Id: Ic816395b7d198a52c704e6cabcb56889150b741c
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/include/soc/serialio.h
3 files changed, 47 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32791/2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32791/1/src/soc/intel/cannonlake/chip.h 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/#/c/32791/1/src/soc/intel/cannonlake/chip.h@342 
PS1, Line 342: 2
> This should be based on a macro, and the bounds in the code should use ARRAY_SIZE() -- not open codi […]

Done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > (1 comment)
> >
> > These settings are just tweaking configuration int he designware ip block, no? Why are we putting in code to feed this into FSP when we already have code which configures everything?
>
> Unfortunately, FSP ends up configuring these based on the default values in the UPDs. Ideally, FSP should not configure any of this. But currently there is no option to say skip all this programming.

So, looking at this change, I don't think we need to set the options for all the SPI buses. We probably just care about the buses that BIOS is actually using e.g. TPM bus on hatch. All other buses are not used by BIOS and so even if FSP ends up configuring them differently, OS will re-configure them anyways.

Based on this, we should be able to just use gpsi structure within common_soc_config to determine if a bus is being used in coreboot (speed_mhz is a good way to check that) and then call gspi_get_soc_spi_cfg to get bus params and setup the required FSP UPDs. Thoughts?

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/32791/2/src/soc/intel/cannonlake/chip.h 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/#/c/32791/2/src/soc/intel/cannonlake/chip.h@41 
PS2, Line 41: #define SOC_INTEL_CML_SPI_DEV_MAX 3
What does this represent? Generic SPI devices? PchSerialIoSpiMAX == 2 but this macro is 3. Why is there a difference? Such discrepancies should be commented accordingly here. There seems to be an issue in the upd settings. Has intel provide an explanation for 3 vs 2 array sizes?

https://review.coreboot.org/#/c/32791/2/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/2/src/soc/intel/cannonlake/fsp_params.... 
PS2, Line 96: 			config->SerialIoSpi2CsPolarity[i];
What is utilizing the SerialIoSpiXCsEnable fields?

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32791/2/src/soc/intel/cannonlake/chip.h 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/#/c/32791/2/src/soc/intel/cannonlake/chip.h@41 
PS2, Line 41: #define SOC_INTEL_CML_SPI_DEV_MAX 3
> What does this represent? Generic SPI devices? PchSerialIoSpiMAX == 2 but this macro is 3. […]

CsPolarity and CsEnable are 2-field arrays where each field indicates each Chip select i.e. CS0 and CS1.

SerialIoSpiDefaultCsOutput selects the default chip select for each spi device i.e. SPI0, SPI1 and SPI2.

So, basically there are 3 GSPI buses and 2 Chip selects for each bus.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Pass more SPI options to FSP.
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32791/2/src/soc/intel/cannonlake/chip.h 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/#/c/32791/2/src/soc/intel/cannonlake/chip.h@41 
PS2, Line 41: #define SOC_INTEL_CML_SPI_DEV_MAX 3
> CsPolarity and CsEnable are 2-field arrays where each field indicates each Chip select i.e. […]

I see. They are controller based. Why aren't we grouping our options (chip.h) into a controller orientated format so it's clearer? We don't have to mirror the decisions in the UPD header. Similarly, PchSerialIoSpiMAX is a terrible name and doesn't convey what it's trying to represent. That is clearly an Intel name -- why carry over the confusion?

Change in ...coreboot[master]: soc/intel/cannonlake: Use CS0 for GSPI0 in FSP parameters.

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Paul Fagerburg, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32791

to look at the new patch set (#3).

Change subject: soc/intel/cannonlake: Use CS0 for GSPI0 in FSP parameters.
......................................................................

soc/intel/cannonlake: Use CS0 for GSPI0 in FSP parameters.

Ensure that the FSP chooses CS0 for GSPI0.

BUG=b:130329260
BRANCh=none
TEST=Compiles.

Change-Id: Ic816395b7d198a52c704e6cabcb56889150b741c
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32791/3

Change in ...coreboot[master]: soc/intel/cannonlake: Use CS0 for GSPI0 in FSP parameters.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Use CS0 for GSPI0 in FSP parameters.
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/32791/3/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/3/src/soc/intel/cannonlake/fsp_params.... 
PS3, Line 369: 		params->SerialIoSpi0CsEnable[0] = 1;
You shouldn't bake in board specific attributes in non-board specific places. This is the soc directory and should work for all uses of the SoC.

Change in ...coreboot[master]: soc/intel/cannonlake: Use CS0 for GSPI0 in FSP parameters.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Use CS0 for GSPI0 in FSP parameters.
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/32791/3/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/3/src/soc/intel/cannonlake/fsp_params.... 
PS3, Line 368: params->SerialIoSpi0CsPolarity[0] = 0;
             : 		params->SerialIoSpi0CsEnable[0] = 1;
Sorry, if this was not clear from my earlier comment. What I meant was:
1. Run a loop to decide if BIOS cares about a particular GSPI bus
   Check common_soc_config.gspi[i].speed_mhz --> where i goes from 0 to CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX

2. If speed_mhz for any bus is non-zero, set the FSP params for that bus:

a. Call gspi_get_soc_spi_cfg(i, &cfg)
 b. Use cfg.cs_polarity to set SerialIoSpiXCsPolarity
 c. Set SerialIoSpiXCsEnable[0] to 1 always because GSPI driver assumes that CS0 is used in BIOS
 d. Set SerialIoSpiDefaultCsOutput[i] to 0 because GSPI driver assumes that CS0 is used in BIOS.

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Paul Fagerburg, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32791

to look at the new patch set (#4).

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

When FSP UPD parameters are configured, also configure the GSPI CS lines
appropriately.  GSPI driver assumes CS0 is the CS signal to use.

BUG=b:130329260
BRANCH=None
TEST=Boot Kohaku, TPM communcation still functional.

Change-Id: Ic816395b7d198a52c704e6cabcb56889150b741c
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 56 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32791/4

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/32791/4/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/4/src/soc/intel/cannonlake/fsp_params.... 
PS4, Line 132: 			if (cfg.cs_polarity == SPI_POLARITY_LOW) {
braces {} are not necessary for any arm of this statement

https://review.coreboot.org/#/c/32791/4/src/soc/intel/cannonlake/fsp_params.... 
PS4, Line 414: 		configure_gspi_cs(i, config, &(params->SerialIoSpiCsPolarity[0]));
line over 80 characters

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Paul Fagerburg, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32791

to look at the new patch set (#5).

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

When FSP UPD parameters are configured, also configure the GSPI CS lines
appropriately.  GSPI driver assumes CS0 is the CS signal to use.

BUG=b:130329260
BRANCH=None
TEST=Boot Kohaku, TPM communcation still functional.

Change-Id: Ic816395b7d198a52c704e6cabcb56889150b741c
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 56 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32791/5

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Paul Fagerburg (Code Review)]

Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 5: Code-Review+1

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/#/c/32791/5/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/5/src/soc/intel/cannonlake/fsp_params.... 
PS5, Line 137: 			/* GSPI driver assumes CS0 is used */
Is this why we're only passing index 0 for the CsEnable and CsOutput? If so, please add a comment below at the call site as well for the assumptions being made.

https://review.coreboot.org/#/c/32791/5/src/soc/intel/cannonlake/fsp_params.... 
PS5, Line 403: (
fwiw, the parens aren't needed

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Tim Wawrzynczak (Code Review)]

Hello Patrick Rudolph, Paul Fagerburg, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32791

to look at the new patch set (#6).

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

When FSP UPD parameters are configured, also configure the GSPI CS lines
appropriately.  GSPI driver assumes CS0 is the CS signal to use.

BUG=b:130329260
BRANCH=None
TEST=Boot Kohaku, TPM communcation still functional.

Change-Id: Ic816395b7d198a52c704e6cabcb56889150b741c
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 59 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32791/6

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/#/c/32791/5/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/5/src/soc/intel/cannonlake/fsp_params.... 
PS5, Line 137: 			/* GSPI driver assumes CS0 is used */
> Is this why we're only passing index 0 for the CsEnable and CsOutput? If so, please add a comment be […]

Yes, will do.

https://review.coreboot.org/#/c/32791/5/src/soc/intel/cannonlake/fsp_params.... 
PS5, Line 403: (
> fwiw, the parens aren't needed

True, I just know that sometimes I forget the precedence and that keeps things clear.  If it's confusing, I'll remove them.

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 6: Code-Review+1

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Paul Fagerburg (Code Review)]

Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/32791/5/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/5/src/soc/intel/cannonlake/fsp_params.... 
PS5, Line 403: (
> True, I just know that sometimes I forget the precedence and that keeps things clear. […]

I think parens improve readability in this case, and would favor keeping them.

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 6: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/32791/6/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/6/src/soc/intel/cannonlake/fsp_params.... 
PS6, Line 137: 			*defaultcs = 0;
             : 			*enable = 1;
Add NULL-checks and you can use the same function in both cases

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/32791/6/src/soc/intel/cannonlake/fsp_params.... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/#/c/32791/6/src/soc/intel/cannonlake/fsp_params.... 
PS6, Line 137: 			*defaultcs = 0;
             : 			*enable = 1;
> Add NULL-checks and you can use the same function in both cases

Done

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Tim Wawrzynczak (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Paul Fagerburg, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32791

to look at the new patch set (#7).

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

When FSP UPD parameters are configured, also configure the GSPI CS lines
appropriately.  GSPI driver assumes CS0 is the CS signal to use.

BUG=b:130329260
BRANCH=None
TEST=Boot Kohaku, TPM communcation still functional.

Change-Id: Ic816395b7d198a52c704e6cabcb56889150b741c
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 43 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/32791/7

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 7: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Paul Fagerburg (Code Review)]

Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

Patch Set 7: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32791 )

Change subject: soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.
......................................................................

soc/intel/cannonlake: Configure SPI CS parameters in FSP UPD.

When FSP UPD parameters are configured, also configure the GSPI CS lines
appropriately.  GSPI driver assumes CS0 is the CS signal to use.

BUG=b:130329260
BRANCH=None
TEST=Boot Kohaku, TPM communcation still functional.

Change-Id: Ic816395b7d198a52c704e6cabcb56889150b741c
Signed-off-by: Tim Wawrzynczak 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32791
Reviewed-by: Patrick Georgi 
Reviewed-by: Paul Fagerburg 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 43 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Paul Fagerburg: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index cc01d10..dd93882 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -119,6 +119,28 @@
    write8(pmcbase + LTR_IGN, reg8);
 }

+static void configure_gspi_cs(int idx, const config_t *config,
+			      uint8_t *polarity, uint8_t *enable,
+			      uint8_t *defaultcs)
+{
+	struct spi_cfg cfg;
+
+	/* If speed_mhz is set, infer that the port should be configured */
+	if (config->common_soc_config.gspi[idx].speed_mhz != 0) {
+		if (gspi_get_soc_spi_cfg(idx, &cfg) == 0) {
+			if (cfg.cs_polarity == SPI_POLARITY_LOW)
+				*polarity = 0;
+			else
+				*polarity = 1;
+
+			if (defaultcs != NULL)
+				*defaultcs = 0;
+			if (enable != NULL)
+				*enable = 1;
+		}
+	}
+}
+
 /* UPD parameters to be initialized before SiliconInit */
 void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
 {
@@ -357,6 +379,27 @@

/* Unlock all GPIO pads */
    tconfig->PchUnlockGpioPads = config->PchUnlockGpioPads;
+
+	/*
+	 * GSPI Chip Select parameters
+	 * The GSPI driver assumes that CS0 is the used chip-select line,
+	 * therefore only CS0 is configured below.
+	 */
+#if CONFIG(SOC_INTEL_COMETLAKE)
+	configure_gspi_cs(0, config, &params->SerialIoSpi0CsPolarity[0],
+			&params->SerialIoSpi0CsEnable[0],
+			&params->SerialIoSpiDefaultCsOutput[0]);
+	configure_gspi_cs(1, config, &params->SerialIoSpi1CsPolarity[0],
+			&params->SerialIoSpi1CsEnable[0],
+			&params->SerialIoSpiDefaultCsOutput[1]);
+	configure_gspi_cs(2, config, &params->SerialIoSpi2CsPolarity[0],
+			&params->SerialIoSpi2CsEnable[0],
+			&params->SerialIoSpiDefaultCsOutput[2]);
+#else
+	for (i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++)
+		configure_gspi_cs(i, config,
+				&params->SerialIoSpiCsPolarity[0], NULL, NULL);
+#endif
 }

/* Mainboard GPIO Configuration */