Change in ...coreboot[master]: PO changes 1. Fix PSF base address for CML PCH

[Maulik V Vaghela (Code Review)]

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

Change subject: PO changes 1. Fix PSF base address for CML PCH
......................................................................

PO changes 1. Fix PSF base address for CML PCH

Change-Id: I932585f6e7525830bd57ecfc372bf3120e7cca66
Signed-off-by: Maulik V Vaghela 
---
M src/soc/intel/cannonlake/bootblock/pch.c
M src/soc/intel/cannonlake/include/soc/pch.h
M src/soc/intel/cannonlake/lpc.c
3 files changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/31434/1

diff --git a/src/soc/intel/cannonlake/bootblock/pch.c b/src/soc/intel/cannonlake/bootblock/pch.c
index f45e177..6c527e7 100644
--- a/src/soc/intel/cannonlake/bootblock/pch.c
+++ b/src/soc/intel/cannonlake/bootblock/pch.c
@@ -34,9 +34,11 @@
 #include <soc/pcr_ids.h>
 #include <soc/pm.h>
 #include <soc/smbus.h>
+#include <string.h>

#define PCR_PSF3_TO_SHDW_PMC_REG_BASE_CNP_LP	0x1400
 #define PCR_PSF3_TO_SHDW_PMC_REG_BASE_CNP_H	0x0980
+#define PCR_PSF3_TO_SHDW_PMC_REG_BASE_CMP_LP    0x0700

#define PCR_PSFX_TO_SHDW_BAR0	0
 #define PCR_PSFX_TO_SHDW_BAR1	0x4
@@ -67,6 +69,8 @@
    	return PCR_PSF3_TO_SHDW_PMC_REG_BASE_CNP_H;
    else if (pch_series == PCH_LP)
    	return PCR_PSF3_TO_SHDW_PMC_REG_BASE_CNP_LP;
+	else if (pch_series == PCH_CMP)
+		return PCR_PSF3_TO_SHDW_PMC_REG_BASE_CMP_LP;
    else
    	return 0;
 }
diff --git a/src/soc/intel/cannonlake/include/soc/pch.h b/src/soc/intel/cannonlake/include/soc/pch.h
index 5253053..c3ce6ac 100644
--- a/src/soc/intel/cannonlake/include/soc/pch.h
+++ b/src/soc/intel/cannonlake/include/soc/pch.h
@@ -22,6 +22,7 @@

#define PCH_H				1
 #define PCH_LP				2
+#define PCH_CMP				3
 #define PCH_UNKNOWN_SERIES		0xFF

#define PCIE_CLK_NOTUSED		0xFF
diff --git a/src/soc/intel/cannonlake/lpc.c b/src/soc/intel/cannonlake/lpc.c
index c33b3c3..6fc6c54 100644
--- a/src/soc/intel/cannonlake/lpc.c
+++ b/src/soc/intel/cannonlake/lpc.c
@@ -17,6 +17,7 @@

#include "chip.h"
 #include <delay.h>
+#include <console/console.h>
 #include <device/device.h>
 #include <device/pci.h>
 #include <pc80/isa-dma.h>
@@ -77,10 +78,14 @@
     */
    lpc_did_hi_byte = pci_read_config8(PCH_DEV_LPC, PCI_DEVICE_ID + 1);

+	printk(BIOS_DEBUG, "LPC DID: 0x%x", lpc_did_hi_byte);
+
    if (lpc_did_hi_byte == 0x9D)
    	return PCH_LP;
    else if (lpc_did_hi_byte == 0xA3)
    	return PCH_H;
+	else if (lpc_did_hi_byte == 0x02)
+		return PCH_CMP;
    else
    	return PCH_UNKNOWN_SERIES;
 }

[Maulik V Vaghela (Code Review)]

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

Change subject: PO changes 1. Fix PSF base address for CML PCH
......................................................................

Patch Set 1: Code-Review-1

This is ongoing changes and will push final patch later

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has removed Patrick Rudolph from this change.  ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: PO changes 1. Fix PSF base address for CML PCH
......................................................................

Removed reviewer Patrick Rudolph.

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 5: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c 
File src/soc/intel/cannonlake/lpc.c:

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c@70 
PS7, Line 70: uint8_t get_pch_series(void)
I'm confused about the existence of this function... don't we decide
this statically at compile time? e.g. with SOC_INTEL_CANNONLAKE_PCH_H

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c 
File src/soc/intel/cannonlake/lpc.c:

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c@70 
PS7, Line 70: uint8_t get_pch_series(void)
> I'm confused about the existence of this function... don't we decide […]

SOC_INTEL_CANNONLAKE_PCH_H is "only" needed for GPIO table inclusion as i understand.

this function to read LPC ID dynamically and to pick correct PSF ID

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c 
File src/soc/intel/cannonlake/lpc.c:

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c@70 
PS7, Line 70: uint8_t get_pch_series(void)
> SOC_INTEL_CANNONLAKE_PCH_H is "only" needed for GPIO table inclusion as i understand.  […]

I see what it does, but I wonder if we ever expect it to report something
different than what we know from SOC_INTEL_CANNONLAKE_PCH_H. If not, we
could just use the latter instead of calling get_pch_series().

In more general terms, this seems redundant. Redundancy slows projects
down.

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Maulik V Vaghela (Code Review)]

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

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c 
File src/soc/intel/cannonlake/lpc.c:

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c@70 
PS7, Line 70: uint8_t get_pch_series(void)
> I see what it does, but I wonder if we ever expect it to report something […]

yes Nico, currently it seems redundant and we could use macro to statically get pch_series or base. We want to keep this function for future use cases where Intel may add new PCH with new device ID and base/series may change. In that case, dynamic detection would help.

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c 
File src/soc/intel/cannonlake/lpc.c:

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c@70 
PS7, Line 70: uint8_t get_pch_series(void)
> yes Nico, currently it seems redundant and we could use macro to statically get pch_series or base. […]

Not sure what you mean with `pch_series` or `base`. `base` is SoC
integrated and `pch_series` a discrete PCH? This sounds like you'd
want to encode something orthogonal to the SoC vs discrete PCH
decision in the future? This would be even worse software design.

I think the soc/intel/ code is already twice as complex as it needs
to be. We have to trim it down and not twist it further if we don't
want to slow future development down.

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Maulik V Vaghela (Code Review)]

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

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c 
File src/soc/intel/cannonlake/lpc.c:

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c@70 
PS7, Line 70: uint8_t get_pch_series(void)
> Not sure what you mean with `pch_series` or `base`. `base` is SoC […]

Let me clarify, PCH base is base address register to access PCH registers. This base address varies among various PCH IDs. Currently we are able to differentiate based on PCH_LP and PCH_H macro. In future, it may change for new PCH (where PCH might be LP only) but base address may be different. To handle this kind of scenarios, we have kept this function of getting PCH series.

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c 
File src/soc/intel/cannonlake/lpc.c:

https://review.coreboot.org/#/c/31434/7/src/soc/intel/cannonlake/lpc.c@70 
PS7, Line 70: uint8_t get_pch_series(void)
> Let me clarify, PCH base is base address register to access PCH registers. […]

Got it, just don't agree with it.

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

Patch Set 7: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Add PCH series check for CML LP PCH

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31434 )

Change subject: soc/intel/cannonlake: Add PCH series check for CML LP PCH
......................................................................

soc/intel/cannonlake: Add PCH series check for CML LP PCH

TEST=Verify PM_STS1 value is is not 0xFF.

Change-Id: I932585f6e7525830bd57ecfc372bf3120e7cca66
Signed-off-by: Maulik V Vaghela 
Reviewed-on: https://review.coreboot.org/c/31434
Reviewed-by: Subrata Banik 
Reviewed-by: Rizwan Qureshi 
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/lpc.c
1 file changed, 13 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Rizwan Qureshi: Looks good to me, approved
  Subrata Banik: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/lpc.c b/src/soc/intel/cannonlake/lpc.c
index c33b3c3..c06ce97 100644
--- a/src/soc/intel/cannonlake/lpc.c
+++ b/src/soc/intel/cannonlake/lpc.c
@@ -70,19 +70,25 @@
 uint8_t get_pch_series(void)
 {
    uint16_t lpc_did_hi_byte;
-
+	uint8_t pch_series = PCH_UNKNOWN_SERIES;
    /*
     * Fetch upper 8 bits on LPC device ID to determine PCH type
     * Adding 1 to the offset to fetch upper 8 bits
     */
    lpc_did_hi_byte = pci_read_config8(PCH_DEV_LPC, PCI_DEVICE_ID + 1);

-	if (lpc_did_hi_byte == 0x9D)
-		return PCH_LP;
-	else if (lpc_did_hi_byte == 0xA3)
-		return PCH_H;
-	else
-		return PCH_UNKNOWN_SERIES;
+	switch (lpc_did_hi_byte) {
+	case 0x9D: /* CNL-LP */
+	case 0x02: /* CML-LP */
+		pch_series = PCH_LP;
+		break;
+	case 0xA3:
+		pch_series = PCH_H;
+		break;
+	default:
+		break;
+	}
+	return pch_series;
 }

#if ENV_RAMSTAGE