Change in coreboot[master]: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options

[Christian Walter (Code Review)]

Christian Walter has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41081 )

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options

Intel introduced VrPowerDeliveryDesign with Cannonlake. The BIOS needs
to program VrPowerDeliverDesign configuration per platform according to
the platform capabilities to avoid incorrect electrial/power parameters.

Refer to document 599797 for more details.

Change-Id: I89b8dceb40fa6a9dc67b218e91bf728ff928b5a0
Signed-off-by: Christian Walter 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/41081/1

diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h
index d4d76cd..0c946cb 100644
--- a/src/soc/intel/cannonlake/chip.h
+++ b/src/soc/intel/cannonlake/chip.h
@@ -414,6 +414,10 @@
    uint8_t LanWakeFromDeepSx;
    uint8_t WolEnableOverride;

+#if !CONFIG(SOC_INTEL_COMETLAKE)
+	uint32_t VrPowerDeliveryDesign;
+#endif
+
    /*
     * Override GPIO PM configuration:
     * 0: Use FSP default GPIO PM program,
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 434bd2b..abffa04 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -460,6 +460,10 @@
 #endif
    }

+#if !CONFIG(SOC_INTEL_COMETLAKE)
+	params->VrPowerDeliveryDesign = config->VrPowerDeliveryDesign;
+#endif
+
    dev = pcidev_path_on_root(SA_DEVFN_IGD);
    if (CONFIG(RUN_FSP_GOP) && dev && dev->enabled)
    	params->PeiGraphicsPeimInit = 1;

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/41081/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41081/1//COMMIT_MSG@9 
PS1, Line 9: Cannonlake
Cannon Lake

https://review.coreboot.org/c/coreboot/+/41081/1//COMMIT_MSG@9 
PS1, Line 9: VrPowerDeliveryDesign
the UPD VrPowerDeliveryDesign

https://review.coreboot.org/c/coreboot/+/41081/1//COMMIT_MSG@12 
PS1, Line 12: 
As the code is shared with Comet Lake, only add it for Cannon Lake.

https://review.coreboot.org/c/coreboot/+/41081/1/src/soc/intel/cannonlake/ch... 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/c/coreboot/+/41081/1/src/soc/intel/cannonlake/ch... 
PS1, Line 417: #if !CONFIG(SOC_INTEL_COMETLAKE)
Why not positive list?

#if CONFIG(SOC_INTEL_CANNONLAKE)

[Christian Walter (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options

Intel introduced the UPD VrPowerDeliveryDesign with Cannon Lake. The
BIOS needs to program VrPowerDeliverDesign configuration per platform
according to the platform capabilities to avoid incorrect
electrial/power parameters.

Refer to document 599797 for more details.

Change-Id: I89b8dceb40fa6a9dc67b218e91bf728ff928b5a0
Signed-off-by: Christian Walter 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/41081/2

[Christian Walter (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options

Intel introduced the UPD VrPowerDeliveryDesign with Cannon Lake. The
BIOS needs to program VrPowerDeliverDesign configuration per platform
according to the platform capabilities to avoid incorrect
electrial/power parameters. This is only added for Cannon Lake.

Refer to document 599797 for more details.

Change-Id: I89b8dceb40fa6a9dc67b218e91bf728ff928b5a0
Signed-off-by: Christian Walter 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/41081/3

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41081 )

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/c/coreboot/+/41081/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41081/1//COMMIT_MSG@9 
PS1, Line 9: Cannonlake
> Cannon Lake

Ack

https://review.coreboot.org/c/coreboot/+/41081/1//COMMIT_MSG@9 
PS1, Line 9: VrPowerDeliveryDesign
> the UPD VrPowerDeliveryDesign

Ack

https://review.coreboot.org/c/coreboot/+/41081/1//COMMIT_MSG@12 
PS1, Line 12:
> As the code is shared with Comet Lake, only add it for Cannon Lake.

Ack

https://review.coreboot.org/c/coreboot/+/41081/1/src/soc/intel/cannonlake/ch... 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/c/coreboot/+/41081/1/src/soc/intel/cannonlake/ch... 
PS1, Line 417: #if !CONFIG(SOC_INTEL_COMETLAKE)
> Why not positive list? […]

I felt like this is the general rule in cannonlakefolder - to exclude COMETLAKE instead of including CANNONLAKE. in soc/cannonlake/fsp_params and soc/cannonlake/romstage/fsp_params you can find the same things - so I adopted it from there.

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41081 )

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41081/1/src/soc/intel/cannonlake/ch... 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/c/coreboot/+/41081/1/src/soc/intel/cannonlake/ch... 
PS1, Line 417: #if !CONFIG(SOC_INTEL_COMETLAKE)
> I felt like this is the general rule in cannonlakefolder - to exclude COMETLAKE instead of including […]

I just mark this to be resolved if thats fine w/ you.

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41081/3/src/soc/intel/cannonlake/fs... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41081/3/src/soc/intel/cannonlake/fs... 
PS3, Line 464: VrPowerDeliveryDesign
maybe it's better to keep the default and only overwrite it when config->VrPowerDeliveryDesign != 0

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded a new patch set (#4) to the change originally created by Christian Walter. ( https://review.coreboot.org/c/coreboot/+/41081 )

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options

Intel introduced the UPD VrPowerDeliveryDesign with Cannon Lake. The
BIOS needs to program VrPowerDeliverDesign configuration per platform
according to the platform capabilities to avoid incorrect
electrial/power parameters. This is only added for Cannon Lake.

Refer to document 599797 for more details.

Change-Id: I89b8dceb40fa6a9dc67b218e91bf728ff928b5a0
Signed-off-by: Christian Walter 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/41081/4

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41081/3/src/soc/intel/cannonlake/fs... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/41081/3/src/soc/intel/cannonlake/fs... 
PS3, Line 464: VrPowerDeliveryDesign
> maybe it's better to keep the default and only overwrite it when config->VrPowerDeliveryDesign != 0

Done

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41081 )

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 4: Code-Review+1

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41081 )

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 4: Code-Review+2

[Christian Walter (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Rudolph, Patrick Rudolph,

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

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

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

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options

Intel introduced the UPD VrPowerDeliveryDesign with Cannon Lake. The
BIOS needs to program VrPowerDeliverDesign configuration per platform
according to the platform capabilities to avoid incorrect
electrial/power parameters. This is only added for Cannon Lake.

Refer to document 599797 for more details.

Change-Id: I89b8dceb40fa6a9dc67b218e91bf728ff928b5a0
Signed-off-by: Christian Walter 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/41081/5

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41081 )

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 7: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options

Intel introduced the UPD VrPowerDeliveryDesign with Cannon Lake. The
BIOS needs to program VrPowerDeliverDesign configuration per platform
according to the platform capabilities to avoid incorrect
electrial/power parameters. This is only added for Cannon Lake.

Refer to document 599797 for more details.

Change-Id: I89b8dceb40fa6a9dc67b218e91bf728ff928b5a0
Signed-off-by: Christian Walter 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41081
Reviewed-by: Philipp Deppenwiese 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 8 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Philipp Deppenwiese: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h
index 4f015a7..f6ec7ce 100644
--- a/src/soc/intel/cannonlake/chip.h
+++ b/src/soc/intel/cannonlake/chip.h
@@ -413,6 +413,10 @@
    uint8_t LanWakeFromDeepSx;
    uint8_t WolEnableOverride;

+#if !CONFIG(SOC_INTEL_COMETLAKE)
+	uint32_t VrPowerDeliveryDesign;
+#endif
+
    /*
     * Override GPIO PM configuration:
     * 0: Use FSP default GPIO PM program,
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index b432087..b7e9ad8 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -459,6 +459,10 @@
 #endif
    }

+#if !CONFIG(SOC_INTEL_COMETLAKE)
+	params->VrPowerDeliveryDesign = config->VrPowerDeliveryDesign;
+#endif
+
    dev = pcidev_path_on_root(SA_DEVFN_IGD);
    if (CONFIG(RUN_FSP_GOP) && dev && dev->enabled)
    	params->PeiGraphicsPeimInit = 1;

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41081 )

Change subject: soc/intel/cannonlake: Add VrPowerDeliveryDesign to chip options
......................................................................

Patch Set 8:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4031
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4030
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4029
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4028

Please note: This test is under development and might not be accurate at all!