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

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

soc/intel/tigerlake: Enable CNVi through dev_enabled

Check for dev enabled status for CNVi and update the
UPD accordingly.

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

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/39465/1

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index d23148a..93c8126 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -214,10 +214,6 @@
    /* 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 14997c5..7863fd9 100644
--- a/src/soc/intel/tigerlake/fsp_params_tgl.c
+++ b/src/soc/intel/tigerlake/fsp_params_tgl.c
@@ -150,8 +150,11 @@
    	params->PchLanEnable = dev->enabled;

/* CNVi */
-	params->CnviMode = config->CnviMode;
-	params->CnviBtCore = config->CnviBtCore;
+	dev = pcidev_path_on_root(PCH_DEVFN_CNVI_WIFI);
+	if (dev)
+		params->CnviMode = dev->enabled;
+	else
+		params->CnviMode = 0;

/* Legacy 8254 timer support */
    params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER;

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39465/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 154: CnviBtCore
What about this?

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39465/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 154: CnviBtCore
> What about this?

When I checked the code, found that when we have CnviMode set to Auto then we dont need to explicitly set BtCore. I am verifying the functionality with this change, will update once I have the result.

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39465/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 154: CnviBtCore
> When I checked the code, found that when we have CnviMode set to Auto then we dont need to explicitl […]

Is the value 'auto' equal to 1?  Should we have an enumerated value here instead?

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39465/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 154: CnviBtCore
> Is the value 'auto' equal to 1?  Should we have an enumerated value here instead?

This code is changed to params->CnviMode = dev->enabled

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1: Code-Review+1

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39465/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 154: CnviBtCore
> This code is changed to params->CnviMode = dev->enabled

Done

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39465/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 154: CnviBtCore
> Done

You said above that "when we have CnviMode set to Auto", so I'm just checking that dev->enabled, which will be 1 in this case, is what the value of "Auto" is, so that BtCore can be ignored, as you said.

[Wonkyu Kim (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1: Code-Review+2

[Nick Vaccaro (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39465/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 154: CnviBtCore
> You said above that "when we have CnviMode set to Auto", so I'm just checking that dev->enabled, whi […]

There is a device for Cnvi BT in device tree. I believe we should be using that to decide whether to set CnviBtCore on or off.

[Srinidhi N Kaushik (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/39465/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 154: CnviBtCore
> There is a device for Cnvi BT in device tree. […]

Yes I agree, we can use devicetree to turn on/off for BT device 10.2

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/tigerlake: Enable CNVi through dev_enabled
......................................................................

soc/intel/tigerlake: Enable CNVi through dev_enabled

Check for dev enabled status for CNVi and update the
UPD accordingly.

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

Signed-off-by: Srinidhi N Kaushik 
Change-Id: I15a03cc70f12e094badf942dd81f22bd09531051
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39465
Tested-by: build bot (Jenkins) 
Reviewed-by: Wonkyu Kim 
Reviewed-by: Nick Vaccaro 
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params_tgl.c
2 files changed, 5 insertions(+), 6 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
  Wonkyu Kim: Looks good to me, approved

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index 5e010bd..87aa894 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -222,10 +222,6 @@
    /* 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 7230a4c..33abac4 100644
--- a/src/soc/intel/tigerlake/fsp_params_tgl.c
+++ b/src/soc/intel/tigerlake/fsp_params_tgl.c
@@ -174,8 +174,11 @@
    	params->PchLanEnable = dev->enabled;

/* CNVi */
-	params->CnviMode = config->CnviMode;
-	params->CnviBtCore = config->CnviBtCore;
+	dev = pcidev_path_on_root(PCH_DEVFN_CNVI_WIFI);
+	if (dev)
+		params->CnviMode = dev->enabled;
+	else
+		params->CnviMode = 0;

/* Legacy 8254 timer support */
    params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER;