Change in coreboot[master]: soc/intel/tigerlake: Enable CNVi Mode

[Srinidhi N Kaushik (Code Review)]

Srinidhi N Kaushik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39317 )

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

soc/intel/tigerlake: Enable CNVi Mode

Add configs to enable CNVi mode and CNViBtCore.

BUG=none
BRANCH=none
TEST=Build and boot tglrvp

Signed-off-by: Srinidhi N Kaushik 
Change-Id: Ic372348a1409b2594a85b71b2fc742be96b84b87
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params_tgl.c
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/39317/1

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index e57abe8..e0ab421 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -208,6 +208,10 @@
    /* Enable Pch iSCLK */
    uint8_t pch_isclk;

+	/* CNVi */
+	uint8_t CnviMode;
+	uint8_t CnviBtCore;
+	
    /* CNVi BT Audio Offload: Enable/Disable BT Audio Offload. */
    enum {
    	FORCE_DISABLE,
diff --git a/src/soc/intel/tigerlake/fsp_params_tgl.c b/src/soc/intel/tigerlake/fsp_params_tgl.c
index 0587b88..152257a 100644
--- a/src/soc/intel/tigerlake/fsp_params_tgl.c
+++ b/src/soc/intel/tigerlake/fsp_params_tgl.c
@@ -145,6 +145,10 @@
    else
    	params->PchLanEnable = dev->enabled;

+	/* CNVi */
+	params->CnviMode = config->CnviMode;
+	params->CnviBtCore = config->CnviBtCore;
+
    /* Legacy 8254 timer support */
    params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER;
    params->Enable8254ClockGatingOnS3 = !CONFIG_USE_LEGACY_8254_TIMER;

[build bot (Jenkins) (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39317/1/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/39317/1/src/soc/intel/tigerlake/chi... 
PS1, Line 214: 	
trailing whitespace

[Srinidhi N Kaushik (Code Review)]

Hello Shaunak Saha, build bot (Jenkins), Wonkyu Kim, Raj Astekar, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

soc/intel/tigerlake: Enable CNVi Mode

Add configs to enable CNVi mode and CNViBtCore.

BUG=none
BRANCH=none
TEST=Build and boot tglrvp

Signed-off-by: Srinidhi N Kaushik 
Change-Id: Ic372348a1409b2594a85b71b2fc742be96b84b87
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params_tgl.c
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/39317/2

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 2: Code-Review+1

[Nick Vaccaro (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 2: Code-Review+2

[caveh jalali (Code Review)]

caveh jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39317 )

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 2: Code-Review+1

[Wonkyu Kim (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

soc/intel/tigerlake: Enable CNVi Mode

Add configs to enable CNVi mode and CNViBtCore.

BUG=none
BRANCH=none
TEST=Build and boot tglrvp

Signed-off-by: Srinidhi N Kaushik 
Change-Id: Ic372348a1409b2594a85b71b2fc742be96b84b87
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39317
Reviewed-by: Nick Vaccaro 
Reviewed-by: caveh jalali 
Reviewed-by: Wonkyu Kim 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params_tgl.c
2 files changed, 8 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index e57abe8..a6bcf08 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -208,6 +208,10 @@
    /* Enable Pch iSCLK */
    uint8_t pch_isclk;

+	/* CNVi */
+	uint8_t CnviMode;
+	uint8_t CnviBtCore;
+
    /* CNVi BT Audio Offload: Enable/Disable BT Audio Offload. */
    enum {
    	FORCE_DISABLE,
diff --git a/src/soc/intel/tigerlake/fsp_params_tgl.c b/src/soc/intel/tigerlake/fsp_params_tgl.c
index 9e22b58..0dae0fe 100644
--- a/src/soc/intel/tigerlake/fsp_params_tgl.c
+++ b/src/soc/intel/tigerlake/fsp_params_tgl.c
@@ -149,6 +149,10 @@
    else
    	params->PchLanEnable = dev->enabled;

+	/* CNVi */
+	params->CnviMode = config->CnviMode;
+	params->CnviBtCore = config->CnviBtCore;
+
    /* Legacy 8254 timer support */
    params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER;
    params->Enable8254ClockGatingOnS3 = !CONFIG_USE_LEGACY_8254_TIMER;

[9elements QA (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1152
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1151
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1150

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

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39317/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 153: params->CnviMode = config->CnviMode;
             : 	params->CnviBtCore = config->CnviBtCore;
Sorry about the delayed review. I have 2 questions:

a) Can't we use the state of CNVi device in device tree to set this param? Then, we won't need the additional config for CnviMode and CnviBtCore.

b) CNVi is enabled when x86 comes out of reset. So, what is FSP doing with these UPDs? If it is just configuring GPIOs, I think we should be able to handle them in coreboot.

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39317/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 153: params->CnviMode = config->CnviMode;
             : 	params->CnviBtCore = config->CnviBtCore;
> Sorry about the delayed review. I have 2 questions: […]

Furquan, for a) do you want me to use pcidev_on_root(PCH_DEV_SLOT_XHCI, 3) to determine if device is connected ? 
b) Part for Cnvi Init is pad config, need to check what else is FSP doing.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39317/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 153: params->CnviMode = config->CnviMode;
             : 	params->CnviBtCore = config->CnviBtCore;
> Furquan, for a) do you want me to use pcidev_on_root(PCH_DEV_SLOT_XHCI, 3) to determine if device is […]

a) Yes, it should be possible to check that. You should be able to use pcidev_path_on_root().

b) Can you identify if FSP is doing anything more than setting GPIO pads?

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi Mode
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39317/3/src/soc/intel/tigerlake/fsp... 
PS3, Line 153: params->CnviMode = config->CnviMode;
             : 	params->CnviBtCore = config->CnviBtCore;
> a) Yes, it should be possible to check that. You should be able to use pcidev_path_on_root(). […]

CnviMode = 1 means Auto detection, if its 0 means its disabled, Hence we need this to be in Auto detection mode.  Also Cnvi init is doing couple of other things apart from basic gpio config when its in Auto Mode.
1. Configure sideband communication channel for WiFi
2. Configure BT
3. Configure CNVi interrupt
Details are in ClientOneSiliconPkg/IpBlock/Cnvi/LibraryPrivate/PeiCnviLib/PeiCnviLib.c 
I will update the Cnvi enable as discussed in a).