Change in coreboot[master]: soc/intel/jasperlake: Enable Intel FIVR RFI settings

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

soc/intel/jasperlake: Enable Intel FIVR RFI settings

We already have RFI settings UPD to mitigate RFI noise issues in
platform. These UPDs were not getting filled via devicetree but
needed to be filled from fsp_params.c

Exporting these UPDs to chip.h will allow OEM/ODMs to fill it
directly from devicetree and also allow us to control it based
on boards instead of keeping it common across SoCs.

BUG=b:171683785
BRANCH=None
TEST=Compilation works and we're able to fill UPD from devicetree.Value
gets reflected in FSP UPDs.

Change-Id: I495cd2294368e6b3035c48b9556a83418d5632de
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/jasperlake/chip.h
M src/soc/intel/jasperlake/fsp_params.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47286/1

diff --git a/src/soc/intel/jasperlake/chip.h b/src/soc/intel/jasperlake/chip.h
index 5e90530..16712c7 100644
--- a/src/soc/intel/jasperlake/chip.h
+++ b/src/soc/intel/jasperlake/chip.h
@@ -342,6 +342,25 @@
     *  - PM_CFG.SLP_LAN_MIN_ASST_WDTH
     */
    uint8_t PchPmPwrCycDur;
+
+	/* 
+	 * FIVR RFI Frequency
+	 * PCODE MMIO Mailbox: Set the desired RFI frequency, in increments of 100KHz.
+	 * 0: Auto. 
+	 * Range varies based on XTAL clock: 
+	 *    0-1918 (Up to 191.8HMz) for 24MHz clock;
+	 *    0-1535 (Up to 153.5MHz) for 19MHz clock.
+	 */
+	uint16_t FivrRfiFrequency;
+
+	/* 
+	 * FIVR RFI Spread Spectrum
+	 * Set the Spread Spectrum Range. <b>1.5%</b>; 
+	 * Range: 0.5%, 1%, 1.5%, 2%, 3%, 4%, 5%,6%. 
+	 * Each Range is translated to an encoded value for FIVR register. 0.5% = 0, 1%
+	 * = 3, 1.5% = 8, 2% = 18, 3% = 28, 4% = 34, 5% = 39, 6% = 44.
+	 */
+	uint8_t FivrSpreadSpectrum;
 };

typedef struct soc_intel_jasperlake_config config_t;
diff --git a/src/soc/intel/jasperlake/fsp_params.c b/src/soc/intel/jasperlake/fsp_params.c
index db27234..92c35c6 100644
--- a/src/soc/intel/jasperlake/fsp_params.c
+++ b/src/soc/intel/jasperlake/fsp_params.c
@@ -216,6 +216,10 @@
    /* Provide correct UART number for FSP debug logs */
    params->SerialIoDebugUartNumber = CONFIG_UART_FOR_CONSOLE;

+	/* Configure FIVR RFI related settings */
+	params->FivrRfiFrequency = config->FivrRfiFrequency;
+	params->FivrSpreadSpectrum = config->FivrSpreadSpectrum;
+
    /* Apply minimum assertion width settings if non-zero */
    if (config->PchPmSlpS3MinAssert)
    	params->PchPmSlpS3MinAssert = config->PchPmSlpS3MinAssert;

[build bot (Jenkins) (Code Review)]

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

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 1:

(6 comments)

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

https://review.coreboot.org/c/coreboot/+/47286/1/src/soc/intel/jasperlake/ch... 
PS1, Line 346: 	/* 
trailing whitespace

https://review.coreboot.org/c/coreboot/+/47286/1/src/soc/intel/jasperlake/ch... 
PS1, Line 349: 	 * 0: Auto. 
trailing whitespace

https://review.coreboot.org/c/coreboot/+/47286/1/src/soc/intel/jasperlake/ch... 
PS1, Line 350: 	 * Range varies based on XTAL clock: 
trailing whitespace

https://review.coreboot.org/c/coreboot/+/47286/1/src/soc/intel/jasperlake/ch... 
PS1, Line 356: 	/* 
trailing whitespace

https://review.coreboot.org/c/coreboot/+/47286/1/src/soc/intel/jasperlake/ch... 
PS1, Line 358: 	 * Set the Spread Spectrum Range. <b>1.5%</b>; 
trailing whitespace

https://review.coreboot.org/c/coreboot/+/47286/1/src/soc/intel/jasperlake/ch... 
PS1, Line 359: 	 * Range: 0.5%, 1%, 1.5%, 2%, 3%, 4%, 5%,6%. 
trailing whitespace

[Maulik V Vaghela (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

soc/intel/jasperlake: Enable Intel FIVR RFI settings

We already have RFI settings UPD to mitigate RFI noise issues in
platform. These UPDs were not getting filled via devicetree but
needed to be filled from fsp_params.c

Exporting these UPDs to chip.h will allow OEM/ODMs to fill it
directly from devicetree and also allow us to control it based
on boards instead of keeping it common across SoCs.

BUG=b:171683785
BRANCH=None
TEST=Compilation works and we're able to fill UPD from devicetree.Value
gets reflected in FSP UPDs.

Change-Id: I495cd2294368e6b3035c48b9556a83418d5632de
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/jasperlake/chip.h
M src/soc/intel/jasperlake/fsp_params.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47286/2

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
File src/soc/intel/jasperlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
PS2, Line 220: 	params->FivrRfiFrequency = config->FivrRfiFrequency;
Please add null check. if (config->Fivr....)

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
File src/soc/intel/jasperlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
PS2, Line 220: 	params->FivrRfiFrequency = config->FivrRfiFrequency;
> Please add null check. if (config->Fivr.... […]

But this can set 0 right? I think FSP default is 0, so I think it is okay to check non-zero.

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
File src/soc/intel/jasperlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
PS2, Line 220: 	params->FivrRfiFrequency = config->FivrRfiFrequency;
> But this can set 0 right? I think FSP default is 0, so I think it is okay to check non-zero.

HI Eric,

If we don't fill this via devicetree, default value will be 0 in config. Shouldn't this be fine?

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
File src/soc/intel/jasperlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
PS2, Line 220: 	params->FivrRfiFrequency = config->FivrRfiFrequency;
> HI Eric, […]

If FivrSpreadSpectrum default is 0, this should be fine. In general, it better to honor FSP setting if device tree is NULL. I have no concern here, your call.

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/47286/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47286/2//COMMIT_MSG@9 
PS2, Line 9: settings UPD
"UPD settings"

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/ch... 
File src/soc/intel/jasperlake/chip.h:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/ch... 
PS2, Line 360:  Each Range is translated to an encoded value for FIVR register. 0.5% = 0, 1%
             : 	 * = 3, 1.5% = 8, 2% = 18, 3% = 28, 4% = 34, 5% = 39, 6% = 44.
These encodings are a little odd, adding an enum or #defines for those would be helpful to people setting this option later

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
File src/soc/intel/jasperlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
PS2, Line 220: 	params->FivrRfiFrequency = config->FivrRfiFrequency;
> If FivrSpreadSpectrum default is 0, this should be fine. […]

Is this overriding the FSP default?

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
File src/soc/intel/jasperlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
PS2, Line 220: 	params->FivrRfiFrequency = config->FivrRfiFrequency;
> Is this overriding the FSP default?

Just checked, both are 0 in FSP.

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/ch... 
File src/soc/intel/jasperlake/chip.h:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/ch... 
PS2, Line 360:  Each Range is translated to an encoded value for FIVR register. 0.5% = 0, 1%
             : 	 * = 3, 1.5% = 8, 2% = 18, 3% = 28, 4% = 34, 5% = 39, 6% = 44.
> These encodings are a little odd, adding an enum or #defines for those would be helpful to people se […]

I think this can change to the FSP comment:
  # !BSF NAME:{FIVR RFI Spread Spectrum}
  # !BSF TYPE:{EditNum, HEX, (0x0,0xFF)}
  # !BSF HELP:{PCODE MMIO Mailbox: FIVR RFI Spread Spectrum, in 0.1% increments. <b>0: 0%</b>; Range: 0.0% to 10.0% (0-100).}
  gPlatformFspPkgTokenSpaceGuid.FivrSpreadSpectrum          | * | 0x01 | 0x00

This is clear 0.1 increments.

[Maulik V Vaghela (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Ronak Kanabar, Aamir Bohra, Patrick Rudolph, EricR Lai,

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

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

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

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

soc/intel/jasperlake: Enable Intel FIVR RFI settings

We already have RFI settings UPD to mitigate RFI noise issues in
platform. These UPDs were not getting filled via devicetree but
needed to be filled from fsp_params.c

Exporting these UPDs to chip.h will allow OEM/ODMs to fill it
directly from devicetree and also allow us to control it based
on boards instead of keeping it common across SoCs.

BUG=b:171683785
BRANCH=None
TEST=Compilation works and we're able to fill UPD from devicetree.Value
gets reflected in FSP UPDs.

Change-Id: I495cd2294368e6b3035c48b9556a83418d5632de
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/jasperlake/chip.h
M src/soc/intel/jasperlake/fsp_params.c
2 files changed, 22 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47286/3

[build bot (Jenkins) (Code Review)]

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

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47286/3/src/soc/intel/jasperlake/ch... 
File src/soc/intel/jasperlake/chip.h:

https://review.coreboot.org/c/coreboot/+/47286/3/src/soc/intel/jasperlake/ch... 
PS3, Line 359: 	 * FIVR RFI Spread Spectrum, in 0.1% increments. 
trailing whitespace

[Maulik V Vaghela (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Ronak Kanabar, Aamir Bohra, Patrick Rudolph, EricR Lai,

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

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

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

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

soc/intel/jasperlake: Enable Intel FIVR RFI settings

We already have RFI settings UPD to mitigate RFI noise issues in
platform. These UPDs were not getting filled via devicetree but
needed to be filled from fsp_params.c

Exporting these UPDs to chip.h will allow OEM/ODMs to fill it
directly from devicetree and also allow us to control it based
on boards instead of keeping it common across SoCs.

BUG=b:171683785
BRANCH=None
TEST=Compilation works and we're able to fill UPD from devicetree.Value
gets reflected in FSP UPDs.

Change-Id: I495cd2294368e6b3035c48b9556a83418d5632de
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/jasperlake/chip.h
M src/soc/intel/jasperlake/fsp_params.c
2 files changed, 22 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47286/4

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/ch... 
File src/soc/intel/jasperlake/chip.h:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/ch... 
PS2, Line 360:  Each Range is translated to an encoded value for FIVR register. 0.5% = 0, 1%
             : 	 * = 3, 1.5% = 8, 2% = 18, 3% = 28, 4% = 34, 5% = 39, 6% = 44.
> I think this can change to the FSP comment: […]

Ack

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
File src/soc/intel/jasperlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/47286/2/src/soc/intel/jasperlake/fs... 
PS2, Line 220: 	params->FivrRfiFrequency = config->FivrRfiFrequency;
> Just checked, both are 0 in FSP.

Yes Eric/Tim, Since FSP defaults are 0, we can safely fill it from devicetree. If someone doesn't set value from devicetree then also, config value will be 0 which aligns with FSP default.

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 4: Code-Review+2

LGTM, thanks!

[Patrick Georgi (Code Review)]

Patrick Georgi has uploaded a new patch set (#5) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/47286 )

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

soc/intel/jasperlake: Enable Intel FIVR RFI settings

We already have RFI UPD settings to mitigate RFI noise issues in
platform. These UPDs were not getting filled via devicetree but
needed to be filled from fsp_params.c

Exporting these UPDs to chip.h will allow OEM/ODMs to fill it
directly from devicetree and also allow us to control it based
on boards instead of keeping it common across SoCs.

BUG=b:171683785
BRANCH=None
TEST=Compilation works and we're able to fill UPD from devicetree.Value
gets reflected in FSP UPDs.

Change-Id: I495cd2294368e6b3035c48b9556a83418d5632de
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/jasperlake/chip.h
M src/soc/intel/jasperlake/fsp_params.c
2 files changed, 22 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/47286/5

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47286/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47286/2//COMMIT_MSG@9 
PS2, Line 9: settings UPD
> "UPD settings"

Done

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/jasperlake: Enable Intel FIVR RFI settings
......................................................................

soc/intel/jasperlake: Enable Intel FIVR RFI settings

We already have RFI UPD settings to mitigate RFI noise issues in
platform. These UPDs were not getting filled via devicetree but
needed to be filled from fsp_params.c

Exporting these UPDs to chip.h will allow OEM/ODMs to fill it
directly from devicetree and also allow us to control it based
on boards instead of keeping it common across SoCs.

BUG=b:171683785
BRANCH=None
TEST=Compilation works and we're able to fill UPD from devicetree.Value
gets reflected in FSP UPDs.

Change-Id: I495cd2294368e6b3035c48b9556a83418d5632de
Signed-off-by: Maulik V Vaghela 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47286
Reviewed-by: EricR Lai 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/jasperlake/chip.h
M src/soc/intel/jasperlake/fsp_params.c
2 files changed, 22 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  EricR Lai: Looks good to me, approved

diff --git a/src/soc/intel/jasperlake/chip.h b/src/soc/intel/jasperlake/chip.h
index f157f92..0ed4205 100644
--- a/src/soc/intel/jasperlake/chip.h
+++ b/src/soc/intel/jasperlake/chip.h
@@ -341,6 +341,24 @@
     *  - PM_CFG.SLP_LAN_MIN_ASST_WDTH
     */
    uint8_t PchPmPwrCycDur;
+
+	/*
+	 * FIVR RFI Frequency
+	 * PCODE MMIO Mailbox: Set the desired RFI frequency, in increments of 100KHz.
+	 * 0: Auto.
+	 * Range varies based on XTAL clock:
+	 *    0-1918 (Up to 191.8HMz) for 24MHz clock;
+	 *    0-1535 (Up to 153.5MHz) for 19MHz clock.
+	 */
+	uint16_t FivrRfiFrequency;
+
+	/*
+	 * FIVR RFI Spread Spectrum
+	 * Set the Spread Spectrum Range. <b>0: 0%</b>;
+	 * FIVR RFI Spread Spectrum, in 0.1% increments.
+	 * Range: 0.0% to 10.0% (0-100)
+	 */
+	uint8_t FivrSpreadSpectrum;
 };

typedef struct soc_intel_jasperlake_config config_t;
diff --git a/src/soc/intel/jasperlake/fsp_params.c b/src/soc/intel/jasperlake/fsp_params.c
index db27234..92c35c6 100644
--- a/src/soc/intel/jasperlake/fsp_params.c
+++ b/src/soc/intel/jasperlake/fsp_params.c
@@ -216,6 +216,10 @@
    /* Provide correct UART number for FSP debug logs */
    params->SerialIoDebugUartNumber = CONFIG_UART_FOR_CONSOLE;

+	/* Configure FIVR RFI related settings */
+	params->FivrRfiFrequency = config->FivrRfiFrequency;
+	params->FivrSpreadSpectrum = config->FivrSpreadSpectrum;
+
    /* Apply minimum assertion width settings if non-zero */
    if (config->PchPmSlpS3MinAssert)
    	params->PchPmSlpS3MinAssert = config->PchPmSlpS3MinAssert;