Hello Meera Ravindranath,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38704
to review the following change.
Change subject: soc/intel/tigerlake: Update PMC Register Base for JSP ......................................................................
soc/intel/tigerlake: Update PMC Register Base for JSP
Update PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP to 0X0A00 for JSP
BUG=None TEST= 1. Test for JSL RVP Boot 2. Verify PMC register values are valid for GEN_PMCON and GBLRST_CAUSE
Change-Id: I6017a9703764b5454e7be479c1e08afe614908f1 Signed-off-by: Usha P usha.p@intel.com Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/bootblock/pch.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/38704/1
diff --git a/src/soc/intel/tigerlake/bootblock/pch.c b/src/soc/intel/tigerlake/bootblock/pch.c index 1654809..6e7393a 100644 --- a/src/soc/intel/tigerlake/bootblock/pch.c +++ b/src/soc/intel/tigerlake/bootblock/pch.c @@ -41,7 +41,7 @@ #include <soc/pm.h>
#define PCR_PSF3_TO_SHDW_PMC_REG_BASE_TGP 0x1100 -#define PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP 0x0980 +#define PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP 0x0A00 #define PCR_PSFX_TO_SHDW_BAR0 0 #define PCR_PSFX_TO_SHDW_BAR1 0x4 #define PCR_PSFX_TO_SHDW_BAR2 0x8
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base for JSP ......................................................................
Patch Set 1: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base for JSP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38704/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38704/1//COMMIT_MSG@9 PS1, Line 9: 0X 0xA00 ?
Aamir Bohra has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base for JSP ......................................................................
Removed Code-Review+2 by Aamir Bohra aamir.bohra@intel.com
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base for JSP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38704/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38704/1//COMMIT_MSG@9 PS1, Line 9: Update PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP to 0X0A00 for JSP Can we please update the platform check also in espi,c?
else if (lpc_did_hi_byte == 0x38) return PCH_JSP;
needs to be 4d
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Sridhar Siricilla, Meera Ravindranath, Aamir Bohra, Maulik V Vaghela, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38704
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
soc/intel/tigerlake: Update PMC Register Base and platform check for JSP
Change: 1. PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP to 0X0A00 for JSP 2. Platform check in espi.c
BUG=None TEST= 1. Test for JSL RVP Boot 2. Verify PMC register values are valid for GEN_PMCON and GBLRST_CAUSE
Change-Id: I6017a9703764b5454e7be479c1e08afe614908f1 Signed-off-by: Usha P usha.p@intel.com Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/bootblock/pch.c M src/soc/intel/tigerlake/espi.c 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/38704/2
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38704/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38704/1//COMMIT_MSG@9 PS1, Line 9: Update PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP to 0X0A00 for JSP
Can we please update the platform check also in espi,c? […]
Done
https://review.coreboot.org/c/coreboot/+/38704/1//COMMIT_MSG@9 PS1, Line 9: 0X
0xA00 ?
Done
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... File src/soc/intel/tigerlake/espi.c:
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... PS2, Line 86: 0x4d Why was 0x38 removed? I see at least on JSL ESPI ID which is 0x3887 here: https://review.coreboot.org/cgit/coreboot.git/tree/src/include/device/pci_id...
In general, should we be reading the whole id above in L82 and compare with PCI_DEVICE_ID_INTEL_* that is added in pci_ids.h for TGL and JSL?
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... File src/soc/intel/tigerlake/espi.c:
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... PS2, Line 86: 0x4d
Why was 0x38 removed? I see at least on JSL ESPI ID which is 0x3887 here: https://review.coreboot. […]
We need to update the JSL device id's into pch_ids.h, here is the crosbug https://partnerissuetracker.corp.google.com/issues/149185282 to track on it.
Since there are too many devices for comparison, we are using hi_byte for this purpose.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... File src/soc/intel/tigerlake/espi.c:
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... PS2, Line 86: 0x4d
We need to update the JSL device id's into pch_ids. […]
As per the EDS, the ESPI device ID ranges from 0x4d80 to 0x4d9f, inclusive of both.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38704/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38704/2//COMMIT_MSG@10 PS2, Line 10: 1. PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP to 0X0A00 for JSP : 2. Platform check in espi.c List indentation not needed.
https://review.coreboot.org/c/coreboot/+/38704/2//COMMIT_MSG@11 PS2, Line 11: 2. Platform check in espi.c Please elaborate, why this needs to be changed, and how.
https://review.coreboot.org/c/coreboot/+/38704/2//COMMIT_MSG@17 PS2, Line 17: and GBLRST_CAUSE How do you verify that.
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Sridhar Siricilla, Meera Ravindranath, Aamir Bohra, Maulik V Vaghela, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38704
to look at the new patch set (#3).
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
soc/intel/tigerlake: Update PMC Register Base and platform check for JSP
Change: 1. PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP to 0X0A00 for JSP 2. Platform check in espi.c
BUG=None TEST= 1. Test for JSL RVP Boot 2. Verify PMC register values are valid for GEN_PMCON and GBLRST_CAUSE from the coreboot console logs.
Change-Id: I6017a9703764b5454e7be479c1e08afe614908f1 Signed-off-by: Usha P usha.p@intel.com Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/bootblock/pch.c M src/soc/intel/tigerlake/espi.c 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/38704/3
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38704/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38704/2//COMMIT_MSG@11 PS2, Line 11: 2. Platform check in espi.c
Please elaborate, why this needs to be changed, and how.
The PMC shadow register base value needs to be aligned for JSP and the hi-byte value for platform check in espi.c is updated according to the Device ID range in EDS.
https://review.coreboot.org/c/coreboot/+/38704/2//COMMIT_MSG@10 PS2, Line 10: 1. PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP to 0X0A00 for JSP : 2. Platform check in espi.c
List indentation not needed.
Done
https://review.coreboot.org/c/coreboot/+/38704/2//COMMIT_MSG@17 PS2, Line 17: and GBLRST_CAUSE
How do you verify that.
Done
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... File src/soc/intel/tigerlake/espi.c:
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... PS2, Line 86: 0x4d
As per the EDS, the ESPI device ID ranges from 0x4d80 to 0x4d9f, inclusive of both.
Hi Furquan, Below CL has been pushed for Jasperlake Device ID update, please review. https://review.coreboot.org/c/coreboot/+/38849
Can you also please reconsider the -1 on this patch ?
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Removed Code-Review-1 by Furquan Shaikh furquan@google.com
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... File src/soc/intel/tigerlake/espi.c:
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... PS2, Line 86: 0x4d
Hi Furquan, […]
Thanks for the clarification Usha and Karthik! I have removed the -1.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... File src/soc/intel/tigerlake/espi.c:
https://review.coreboot.org/c/coreboot/+/38704/2/src/soc/intel/tigerlake/esp... PS2, Line 86: 0x4d
Thanks for the clarification Usha and Karthik! I have removed the -1.
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 3: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
Patch Set 3: Code-Review+2
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38704 )
Change subject: soc/intel/tigerlake: Update PMC Register Base and platform check for JSP ......................................................................
soc/intel/tigerlake: Update PMC Register Base and platform check for JSP
Change: 1. PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP to 0X0A00 for JSP 2. Platform check in espi.c
BUG=None TEST= 1. Test for JSL RVP Boot 2. Verify PMC register values are valid for GEN_PMCON and GBLRST_CAUSE from the coreboot console logs.
Change-Id: I6017a9703764b5454e7be479c1e08afe614908f1 Signed-off-by: Usha P usha.p@intel.com Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38704 Reviewed-by: Rizwan Qureshi rizwan.qureshi@intel.com Reviewed-by: Aamir Bohra aamir.bohra@intel.com Reviewed-by: Subrata Banik subrata.banik@intel.com Reviewed-by: Karthik Ramasubramanian kramasub@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/tigerlake/bootblock/pch.c M src/soc/intel/tigerlake/espi.c 2 files changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Rizwan Qureshi: Looks good to me, approved Subrata Banik: Looks good to me, approved Aamir Bohra: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/bootblock/pch.c b/src/soc/intel/tigerlake/bootblock/pch.c index 1654809..cd264d6 100644 --- a/src/soc/intel/tigerlake/bootblock/pch.c +++ b/src/soc/intel/tigerlake/bootblock/pch.c @@ -41,7 +41,7 @@ #include <soc/pm.h>
#define PCR_PSF3_TO_SHDW_PMC_REG_BASE_TGP 0x1100 -#define PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP 0x0980 +#define PCR_PSF3_TO_SHDW_PMC_REG_BASE_JSP 0xA00 #define PCR_PSFX_TO_SHDW_BAR0 0 #define PCR_PSFX_TO_SHDW_BAR1 0x4 #define PCR_PSFX_TO_SHDW_BAR2 0x8 diff --git a/src/soc/intel/tigerlake/espi.c b/src/soc/intel/tigerlake/espi.c index d07a582..7efd210 100644 --- a/src/soc/intel/tigerlake/espi.c +++ b/src/soc/intel/tigerlake/espi.c @@ -83,7 +83,7 @@
if (lpc_did_hi_byte == 0xA0) return PCH_TGP; - else if (lpc_did_hi_byte == 0x38) + else if (lpc_did_hi_byte == 0x4d) return PCH_JSP; else return PCH_UNKNOWN_SERIES;
-
Aamir Bohra (Code Review) -
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Paul Menzel (Code Review)
-
Rizwan Qureshi (Code Review)
-
Subrata Banik (Code Review)
-
Usha P (Code Review)