Change in coreboot[master]: soc/intel/tigerlake: Configure RP setting

[Wonkyu Kim (Code Review)]

Wonkyu Kim has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

soc/intel/tigerlake: Configure RP setting

Configure RP LTR and AER

BUG=b:151166040
TEST= build and boot volteer and check LTR and AER value
from FSP log

Signed-off-by: Wonkyu Kim 
Change-Id: I668f2e5fea15019a9e5ae06fb4d55fa2aea69e8a
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40262/1

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index e105061..259ebbe 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -118,6 +118,12 @@
    	L1_SS_L1_2,
    } PcieRpL1Substates[CONFIG_MAX_ROOT_PORTS];

+	/* PCIe LTR : Disable(1) / Enable(0) */
+	uint8_t	PcieRpLtrDisable[CONFIG_MAX_ROOT_PORTS];
+
+	/* PCIE RP Advanced Error Report: Enable(1) / Disable (1) */
+	uint8_t PcieRpAdvancedErrorReporting[CONFIG_MAX_ROOT_PORTS];
+
    /* SMBus */
    uint8_t SmbusEnable;

diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index ff6d3a9..da07d5f 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -122,10 +122,13 @@
    }

/* RP Configs */
-	for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++)
+	for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++) {
    	params->PcieRpL1Substates[i] =
    		get_l1_substate_control(config->PcieRpL1Substates[i]);
-
+		params->PcieRpLtrEnable[i] = !config->PcieRpLtrDisable[i];
+		params->PcieRpAdvancedErrorReporting[i] =
+			config->PcieRpAdvancedErrorReporting[i];
+	}
    /* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_on_root(PCH_DEV_SLOT_XHCI, 1);
    if (dev) {

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

soc/intel/tigerlake: Configure RP setting

Configure RP LTR and AER

BUG=b:151166040
TEST= build and boot volteer and check LTR and AER value
from FSP log

Signed-off-by: Wonkyu Kim 
Change-Id: I668f2e5fea15019a9e5ae06fb4d55fa2aea69e8a
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40262/2

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

soc/intel/tigerlake: Configure RP setting

Configure RP LTR and AER

BUG=b:151166040
TEST= build and boot volteer and check LTR and AER value
from FSP log

Signed-off-by: Wonkyu Kim 
Change-Id: I668f2e5fea15019a9e5ae06fb4d55fa2aea69e8a
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40262/3

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 3: Code-Review+1

[Alex Levin (Code Review)]

Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40262/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40262/3//COMMIT_MSG@9 
PS3, Line 9: Configure RP LTR and AER
Would suggest a rephrase in the spirit of:

Adding LTR and AER configuration to the root ports config

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 128: 		params->PcieRpLtrEnable[i] = !config->PcieRpLtrDisable[i];
why is the opposite of the config setting? why not keep it uniform? i.e. use LTR enable for both or LTR disable for both?

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 128: 		params->PcieRpLtrEnable[i] = !config->PcieRpLtrDisable[i];
> why is the opposite of the config setting? why not keep it uniform? i.e. […]

Yes please. Let's keep this as PcieRpLtrEnable.

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

soc/intel/tigerlake: Configure RP setting

Adding LTR and AER configuration to the root ports config

BUG=b:151166040
TEST= build and boot volteer and check LTR and AER value
from FSP log

Signed-off-by: Wonkyu Kim 
Change-Id: I668f2e5fea15019a9e5ae06fb4d55fa2aea69e8a
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40262/4

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40262/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40262/3//COMMIT_MSG@9 
PS3, Line 9: Configure RP LTR and AER
> Would suggest a rephrase in the spirit of: […]

Ack

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 128: 		params->PcieRpLtrEnable[i] = !config->PcieRpLtrDisable[i];
> Yes please. Let's keep this as PcieRpLtrEnable.

That's because we want to have PpLtrEnable by default. Otherwise devicetree in each mainboard should enable it.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 128: 		params->PcieRpLtrEnable[i] = !config->PcieRpLtrDisable[i];
> That's because we want to have PpLtrEnable by default. […]

If that is the case, I would recommend skipping setting of this UPD completely. If in the future there is a need for any mainboard to disable LTR, then it can be added as required. This change can just allow setting of AER.

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 128: 		params->PcieRpLtrEnable[i] = !config->PcieRpLtrDisable[i];
> If that is the case, I would recommend skipping setting of this UPD completely. […]

We still need to set it as FSP default value is LTR disabled but we want to enable LTR by default(without adding in devicetree) for enabling ASPM fully.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 128: 		params->PcieRpLtrEnable[i] = !config->PcieRpLtrDisable[i];
> We still need to set it as FSP default value is LTR disabled but we want to enable LTR by default(wi […]

Aah I see. In that case, I think we should just use the config for enable. It is confusing that some parameters for PCIe are positive and others are negative. Also, on previous Intel platforms, it looks like we are already using PcieRpLtrEnable[] that the mainboard sets. It would be good to be consistent.

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

soc/intel/tigerlake: Configure RP setting

Adding LTR and AER configuration to the root ports config

BUG=b:151166040
TEST= build and boot volteer and check LTR and AER value
from FSP log

Signed-off-by: Wonkyu Kim 
Change-Id: I668f2e5fea15019a9e5ae06fb4d55fa2aea69e8a
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40262/5

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/40262/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 128: 		params->PcieRpLtrEnable[i] = !config->PcieRpLtrDisable[i];
> Aah I see. In that case, I think we should just use the config for enable. […]

Done

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 5: Code-Review+1

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 5: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/40262/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40262/5//COMMIT_MSG@9 
PS5, Line 9: Adding
Add

https://review.coreboot.org/c/coreboot/+/40262/5//COMMIT_MSG@9 
PS5, Line 9: Adding LTR and AER configuration to the root ports config
Please add a dot/period at the end

[Wonkyu Kim (Code Review)]

Hello build bot (Jenkins), Shaunak Saha, Furquan Shaikh, Caveh Jalali, Paul Menzel, Nick Vaccaro, Srinidhi N Kaushik, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

soc/intel/tigerlake: Configure RP setting

Add LTR and AER configuration to the root ports config.

BUG=b:151166040
TEST= build and boot volteer and check LTR and AER value
from FSP log

Signed-off-by: Wonkyu Kim 
Change-Id: I668f2e5fea15019a9e5ae06fb4d55fa2aea69e8a
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40262/6

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 6:

(2 comments)
> Patch Set 5: Code-Review+1
>
> (1 comment)

https://review.coreboot.org/c/coreboot/+/40262/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40262/5//COMMIT_MSG@9 
PS5, Line 9: Adding
> Add

Done

https://review.coreboot.org/c/coreboot/+/40262/5//COMMIT_MSG@9 
PS5, Line 9: Adding LTR and AER configuration to the root ports config
> Please add a dot/period at the end

Done

[Srinidhi N Kaushik (Code Review)]

Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

Patch Set 6: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40262 )

Change subject: soc/intel/tigerlake: Configure RP setting
......................................................................

soc/intel/tigerlake: Configure RP setting

Add LTR and AER configuration to the root ports config.

BUG=b:151166040
TEST= build and boot volteer and check LTR and AER value
from FSP log

Signed-off-by: Wonkyu Kim 
Change-Id: I668f2e5fea15019a9e5ae06fb4d55fa2aea69e8a
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40262
Reviewed-by: Srinidhi N Kaushik 
Reviewed-by: Nick Vaccaro 
Reviewed-by: Paul Menzel 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 11 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Nick Vaccaro: Looks good to me, but someone else must approve
  Srinidhi N Kaushik: Looks good to me, approved
  Wonkyu Kim: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index e105061..bc6c3db 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -118,6 +118,12 @@
    	L1_SS_L1_2,
    } PcieRpL1Substates[CONFIG_MAX_ROOT_PORTS];

+	/* PCIe LTR: Enable (1) / Disable (0) */
+	uint8_t PcieRpLtrEnable[CONFIG_MAX_ROOT_PORTS];
+
+	/* PCIE RP Advanced Error Report: Enable (1) / Disable (0) */
+	uint8_t PcieRpAdvancedErrorReporting[CONFIG_MAX_ROOT_PORTS];
+
    /* SMBus */
    uint8_t SmbusEnable;

diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index ff6d3a9..231399c 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -122,10 +122,13 @@
    }

/* RP Configs */
-	for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++)
+	for (i = 0; i < CONFIG_MAX_ROOT_PORTS; i++) {
    	params->PcieRpL1Substates[i] =
    		get_l1_substate_control(config->PcieRpL1Substates[i]);
-
+		params->PcieRpLtrEnable[i] = config->PcieRpLtrEnable[i];
+		params->PcieRpAdvancedErrorReporting[i] =
+			config->PcieRpAdvancedErrorReporting[i];
+	}
    /* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_on_root(PCH_DEV_SLOT_XHCI, 1);
    if (dev) {