Change in coreboot[master]: soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification ...

[Aamir Bohra (Code Review)]

Aamir Bohra has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37604 )

Change subject: soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle
......................................................................

soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle

Change-Id: I20b793b95483af03ce4ae068ac07864a9e90d39b
Signed-off-by: Aamir Bohra 
---
M src/soc/intel/cannonlake/finalize.c
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37604/1

diff --git a/src/soc/intel/cannonlake/finalize.c b/src/soc/intel/cannonlake/finalize.c
index 115b732..a37ac98 100644
--- a/src/soc/intel/cannonlake/finalize.c
+++ b/src/soc/intel/cannonlake/finalize.c
@@ -92,11 +92,16 @@
    	write8(pmcbase + PCH_PWRM_ACPI_TMR_CTL, reg8);
    }

-	/* Disable XTAL shutdown qualification for low power idle. */
    if (config->s0ix_enable) {
+		/* Disable XTAL shutdown qualification for low power idle. */
    	reg32 = read32(pmcbase + CPPMVRIC);
    	reg32 |= XTALSDQDIS;
    	write32(pmcbase + CPPMVRIC, reg32);
+
+		/* Enable Audio DSP OSC qualification for low power idle. */
+		reg32 = read32(pmcbase + CPPMVRIC2);
+		reg32 &= ~ADSPOSCDIS;
+		write32(pmcbase + CPPMVRIC2, reg32);
    }

pch_handle_sideband(config);
diff --git a/src/soc/intel/cannonlake/include/soc/pmc.h b/src/soc/intel/cannonlake/include/soc/pmc.h
index 252c719..878271c 100644
--- a/src/soc/intel/cannonlake/include/soc/pmc.h
+++ b/src/soc/intel/cannonlake/include/soc/pmc.h
@@ -156,6 +156,9 @@
 #define CPPMVRIC			0x1B1C
 #define   XTALSDQDIS			(1 << 22)

+#define CPPMVRIC2			0x1B1C
+#define   ADSPOSCDIS			(1 << 22)
+
 #define IRQ_REG				ACTL
 #define SCI_IRQ_ADJUST			0
 #define ACTL				0x1BD8

[Aamir Bohra (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle
......................................................................

soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle

Change-Id: I20b793b95483af03ce4ae068ac07864a9e90d39b
Signed-off-by: Aamir Bohra 
---
M src/soc/intel/cannonlake/finalize.c
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37604/2

[Rajat Jain (Code Review)]

Rajat Jain has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37604 )

Change subject: soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 2:

Furquan, this needs to go in the qualified image....

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 2:

(3 comments)
> Patch Set 2:
>
> Furquan, this needs to go in the qualified image....

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

https://review.coreboot.org/c/coreboot/+/37604/2//COMMIT_MSG@7 
PS2, Line 7: Enable Audio DSP OSC qualification for low power idle
Can you please update the commit message to reflect why this change is required? Or what problem is it attempting to solve?

https://review.coreboot.org/c/coreboot/+/37604/2//COMMIT_MSG@8 
PS2, Line 8: 
BUG=?

https://review.coreboot.org/c/coreboot/+/37604/2/src/soc/intel/cannonlake/fi... 
File src/soc/intel/cannonlake/finalize.c:

https://review.coreboot.org/c/coreboot/+/37604/2/src/soc/intel/cannonlake/fi... 
PS2, Line 101: Enable Audio DSP OSC qualification for low power idle.
Does this break runtime S0ix?

[Aamir Bohra (Code Review)]

Hello Patrick Rudolph, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle
......................................................................

soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle

With Audio DSP OSC qualification disabled from S0ix criteria.
S0ix is achieved before the DSP is suspended. When driver tries
to suspend DSP its already turned off.

BUG=b:139481313

Change-Id: I20b793b95483af03ce4ae068ac07864a9e90d39b
Signed-off-by: Aamir Bohra 
---
M src/soc/intel/cannonlake/finalize.c
M src/soc/intel/cannonlake/include/soc/pmc.h
2 files changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37604/3

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37604 )

Change subject: soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37604/2/src/soc/intel/cannonlake/fi... 
File src/soc/intel/cannonlake/finalize.c:

https://review.coreboot.org/c/coreboot/+/37604/2/src/soc/intel/cannonlake/fi... 
PS2, Line 101: Enable Audio DSP OSC qualification for low power idle.
> Does this break runtime S0ix?

https://partnerissuetracker.corp.google.com/issues/139481313#comment98
As per the testing , the S0ix is working

[Sathya Prakash M R (Code Review)]

Sathya Prakash M R has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37604 )

Change subject: soc/intel/cannonlake: [TEST-ONLY] Enable Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 3:

The change was tested with below scenarios:

a. runtime soix 
b. soix attempted via powerd 
c. soix on future SOF release with WOV enabled. ( internal testing)

All 3 are pass

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Aamir Bohra (Code Review)]

Hello Patrick Rudolph, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle

With Audio DSP OSC qualification disabled from S0ix criteria.
S0ix is achieved before the DSP is suspended. When driver tries
to suspend DSP its already turned off.

BUG=b:139481313

Change-Id: I20b793b95483af03ce4ae068ac07864a9e90d39b
Signed-off-by: Aamir Bohra 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/finalize.c
M src/soc/intel/cannonlake/include/soc/pmc.h
3 files changed, 16 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37604/4

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[build bot (Jenkins) (Code Review)]

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

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37604/4/src/soc/intel/cannonlake/fi... 
File src/soc/intel/cannonlake/finalize.c:

https://review.coreboot.org/c/coreboot/+/37604/4/src/soc/intel/cannonlake/fi... 
PS4, Line 100: 		if (config->cppmvric2_adsposcdis) {
suspect code indent for conditional statements (16, 16)

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37604/4/src/soc/intel/cannonlake/fi... 
File src/soc/intel/cannonlake/finalize.c:

https://review.coreboot.org/c/coreboot/+/37604/4/src/soc/intel/cannonlake/fi... 
PS4, Line 101: 		/* Enable Audio DSP OSC qualification for low power idle. */
             : 		reg32 = read32(pmcbase + CPPMVRIC2);
             : 		reg32 &= ~ADSPOSCDIS;
             : 		write32(pmcbase + CPPMVRIC2, reg32);
Can you fix the indentation?

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Aamir Bohra (Code Review)]

Hello Patrick Rudolph, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle

With Audio DSP OSC qualification disabled from S0ix criteria.
S0ix is achieved before the DSP is suspended. When driver tries
to suspend DSP its already turned off.

BUG=b:139481313

Change-Id: I20b793b95483af03ce4ae068ac07864a9e90d39b
Signed-off-by: Aamir Bohra 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/finalize.c
M src/soc/intel/cannonlake/include/soc/pmc.h
3 files changed, 16 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37604/5

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37604 )

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37604/4/src/soc/intel/cannonlake/fi... 
File src/soc/intel/cannonlake/finalize.c:

https://review.coreboot.org/c/coreboot/+/37604/4/src/soc/intel/cannonlake/fi... 
PS4, Line 100: 		if (config->cppmvric2_adsposcdis) {
> suspect code indent for conditional statements (16, 16)

Done

https://review.coreboot.org/c/coreboot/+/37604/4/src/soc/intel/cannonlake/fi... 
PS4, Line 101: 		/* Enable Audio DSP OSC qualification for low power idle. */
             : 		reg32 = read32(pmcbase + CPPMVRIC2);
             : 		reg32 &= ~ADSPOSCDIS;
             : 		write32(pmcbase + CPPMVRIC2, reg32);
> Can you fix the indentation?

Done

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Aamir Bohra (Code Review)]

Hello Patrick Rudolph, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle

With Audio DSP OSC qualification disabled from S0ix criteria.
S0ix is achieved before the DSP is suspended. When driver tries
to suspend DSP its already turned off.

BUG=b:139481313

Change-Id: I20b793b95483af03ce4ae068ac07864a9e90d39b
Signed-off-by: Aamir Bohra 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/finalize.c
M src/soc/intel/cannonlake/include/soc/pmc.h
3 files changed, 16 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37604/6

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37604 )

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 6:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/37604/2//COMMIT_MSG@7 
PS2, Line 7: Enable Audio DSP OSC qualification for low power idle
> Can you please update the commit message to reflect why this change is required? Or what problem is  […]

Done

https://review.coreboot.org/c/coreboot/+/37604/2//COMMIT_MSG@8 
PS2, Line 8:
> BUG=?

Done

https://review.coreboot.org/c/coreboot/+/37604/2/src/soc/intel/cannonlake/fi... 
File src/soc/intel/cannonlake/finalize.c:

https://review.coreboot.org/c/coreboot/+/37604/2/src/soc/intel/cannonlake/fi... 
PS2, Line 101: Enable Audio DSP OSC qualification for low power idle.
> https://partnerissuetracker.corp.google.com/issues/139481313#comment98 […]

Done

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Tim Wawrzynczak (Code Review)]

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

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................

soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle

With Audio DSP OSC qualification disabled from S0ix criteria.
S0ix is achieved before the DSP is suspended. When driver tries
to suspend DSP its already turned off.

BUG=b:139481313

Change-Id: I20b793b95483af03ce4ae068ac07864a9e90d39b
Signed-off-by: Aamir Bohra 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37604
Reviewed-by: Tim Wawrzynczak 
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/finalize.c
M src/soc/intel/cannonlake/include/soc/pmc.h
3 files changed, 16 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h
index 0712146..fd37d26 100644
--- a/src/soc/intel/cannonlake/chip.h
+++ b/src/soc/intel/cannonlake/chip.h
@@ -59,8 +59,13 @@
    uint32_t gen3_dec;
    uint32_t gen4_dec;

+	/* S0ix configuration */
+
    /* Enable S0iX support */
    int s0ix_enable;
+	/* Enable Audio DSP oscillator qualification for S0ix */
+	uint8_t cppmvric2_adsposcdis;
+
    /* Enable DPTF support */
    int dptf_enable;

diff --git a/src/soc/intel/cannonlake/finalize.c b/src/soc/intel/cannonlake/finalize.c
index 002e8ea..b2fb9f9 100644
--- a/src/soc/intel/cannonlake/finalize.c
+++ b/src/soc/intel/cannonlake/finalize.c
@@ -91,11 +91,18 @@
    	write8(pmcbase + PCH_PWRM_ACPI_TMR_CTL, reg8);
    }

-	/* Disable XTAL shutdown qualification for low power idle. */
    if (config->s0ix_enable) {
+		/* Disable XTAL shutdown qualification for low power idle. */
    	reg32 = read32(pmcbase + CPPMVRIC);
    	reg32 |= XTALSDQDIS;
    	write32(pmcbase + CPPMVRIC, reg32);
+
+		if (config->cppmvric2_adsposcdis) {
+			/* Enable Audio DSP OSC qualification for S0ix */
+			reg32 = read32(pmcbase + CPPMVRIC2);
+			reg32 &= ~ADSPOSCDIS;
+			write32(pmcbase + CPPMVRIC2, reg32);
+		}
    }

pch_handle_sideband(config);
diff --git a/src/soc/intel/cannonlake/include/soc/pmc.h b/src/soc/intel/cannonlake/include/soc/pmc.h
index 252c719..fbd366b 100644
--- a/src/soc/intel/cannonlake/include/soc/pmc.h
+++ b/src/soc/intel/cannonlake/include/soc/pmc.h
@@ -156,6 +156,9 @@
 #define CPPMVRIC			0x1B1C
 #define   XTALSDQDIS			(1 << 22)

+#define CPPMVRIC2			0x1B4C
+#define   ADSPOSCDIS			(1 << 22)
+
 #define IRQ_REG				ACTL
 #define SCI_IRQ_ADJUST			0
 #define ACTL				0x1BD8

Change in coreboot[master]: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power...

[Michael Niewöhner (Code Review)]

Michael Niewöhner has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/37604 )

Change subject: soc/intel/cannonlake: Allow Audio DSP OSC qualification for low power idle
......................................................................