Ronak Kanabar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
vendorcode/intel/fsp: Update FSP header for Tiger Lake
Update FSPM header to include DisableDimmCh Upds for Tiger Lake platform version 2457.
Change-Id: Ic743cb2134e6273a63c1212506c81ccbbdec442a Signed-off-by: Ronak Kanabar ronak.kanabar@intel.com --- M src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h 1 file changed, 38 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/39797/1
diff --git a/src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h b/src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h index 9bc1a40..b27514c 100644 --- a/src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h +++ b/src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h @@ -356,9 +356,41 @@ **/ UINT8 RMT;
-/** Offset 0x0194 - Reserved +/** Offset 0x0194 - DisableDimmCh0 **/ - UINT8 Reserved8[10]; + UINT8 DisableDimmCh0; + +/** Offset 0x0195 - DisableDimmCh1 +**/ + UINT8 DisableDimmCh1; + +/** Offset 0x0196 - DisableDimmCh2 +**/ + UINT8 DisableDimmCh2; + +/** Offset 0x0197 - DisableDimmCh3 +**/ + UINT8 DisableDimmCh3; + +/** Offset 0x0198 - DisableDimmCh4 +**/ + UINT8 DisableDimmCh4; + +/** Offset 0x0199 - DisableDimmCh5 +**/ + UINT8 DisableDimmCh5; + +/** Offset 0x019A - DisableDimmCh6 +**/ + UINT8 DisableDimmCh6; + +/** Offset 0x019B - DisableDimmCh7 +**/ + UINT8 DisableDimmCh7; + +/** Offset 0x019C - Reserved +**/ + UINT8 Reserved8[2];
/** Offset 0x019E - Memory Reference Clock 100MHz, 133MHz. @@ -861,7 +893,7 @@
/** Offset 0x0775 - Reserved **/ - UINT8 Reserved38[355]; + UINT8 Reserved38[315]; } FSP_M_CONFIG;
/** Fsp M UPD Configuration @@ -880,11 +912,11 @@ **/ FSP_M_CONFIG FspmConfig;
-/** Offset 0x08D8 +/** Offset 0x08B0 **/ - UINT8 UnusedUpdSpace24[6]; + UINT8 UnusedUpdSpace23[6];
-/** Offset 0x08DE +/** Offset 0x08B6 **/ UINT16 UpdTerminator; } FSPM_UPD;
Hello Furquan Shaikh, Aamir Bohra, Srinidhi N Kaushik, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39797
to look at the new patch set (#2).
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
vendorcode/intel/fsp: Update FSP header for Tiger Lake
Update FSPM header to include DisableDimmCh Upds for Tiger Lake platform version 2457.
BUG=b:152000235 Change-Id: Ic743cb2134e6273a63c1212506c81ccbbdec442a Signed-off-by: Ronak Kanabar ronak.kanabar@intel.com --- M src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h 1 file changed, 38 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/39797/2
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2: Code-Review+2
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315 did you mean to shrink this struct size in the same patch? seems unrelated.
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
did you mean to shrink this struct size in the same patch? […]
this is auto generated header. generated from fsp source code.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
this is auto generated header. generated from fsp source code.
But why did this size change? Are the headers being generated from the same FSP version?
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
But why did this size change? Are the headers being generated from the same FSP version?
As open up upd based on old base(2457), it's better have same size.
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
As open up upd based on old base(2457), it's better have same size.
This one is generated automatically. I have explain things in Bug. if you take reference of following CL you will get same size and it has same version. https://review.coreboot.org/c/coreboot/+/37830
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
This one is generated automatically. I have explain things in Bug. […]
if you want i can split this Cl and make size correct first and then rebase this one on top of that CL
Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Removed Code-Review-1 by Furquan Shaikh furquan@google.com
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG@12 PS2, Line 12: BUG=b:152000235 TEST=?
Did you try booting with these headers?
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
if you want i can split this Cl and make size correct first and then rebase this one on top of that […]
Thanks for the pointer Rohan. So, it looks like the header for some reason had changed the size here: https://review.coreboot.org/c/coreboot/+/38555. I think we can continue with this CL since it actually uses the correct size, but I am really concerned how the size change happened earlier if the headers are generated using a script.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
Can you add topic? TGL_UPSTREAM
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
Thanks for the pointer Rohan. […]
I suspect that in that default header was updated and correct method is to build header first and then it should be put here. should is mark it as resolved ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
I suspect that in that default header was updated and correct method is to build header first and th […]
Yes.
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/39797/2/src/vendorcode/intel/fsp/fs... PS2, Line 896: 315
Yes.
Done
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG@12 PS2, Line 12: BUG=b:152000235
TEST=? […]
Ack
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG@12 PS2, Line 12: BUG=b:152000235
Ack
By ACK do you mean that this change was boot tested on volteer? If so, can you please add that to the commit message?
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG@12 PS2, Line 12: BUG=b:152000235
By ACK do you mean that this change was boot tested on volteer? If so, can you please add that to th […]
hey Furqaun i don't have board here to check this . I was waiting for Srinidhi to check @Srinidhi N Kaushik can you please check boot and update here so we can close this.
Srinidhi N Kaushik has uploaded a new patch set (#3) to the change originally created by Ronak Kanabar. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
vendorcode/intel/fsp: Update FSP header for Tiger Lake
Update FSPM header to include DisableDimmCh Upds for Tiger Lake platform version 2457.
BUG=b:152000235 BRANCH=none TEST="Build and Boot on Ripto/Volteer"
Change-Id: Ic743cb2134e6273a63c1212506c81ccbbdec442a Signed-off-by: Ronak Kanabar ronak.kanabar@intel.com --- M src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h 1 file changed, 38 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/39797/3
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39797/2//COMMIT_MSG@12 PS2, Line 12: BUG=b:152000235
hey Furqaun i don't have board here to check this . I was waiting for Srinidhi to check […]
Done
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
vendorcode/intel/fsp: Update FSP header for Tiger Lake
Update FSPM header to include DisableDimmCh Upds for Tiger Lake platform version 2457.
BUG=b:152000235 BRANCH=none TEST="Build and Boot on Ripto/Volteer"
Change-Id: Ic743cb2134e6273a63c1212506c81ccbbdec442a Signed-off-by: Ronak Kanabar ronak.kanabar@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39797 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Reviewed-by: Wonkyu Kim wonkyu.kim@intel.com --- M src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h 1 file changed, 38 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Srinidhi N Kaushik: Looks good to me, approved Wonkyu Kim: Looks good to me, approved
diff --git a/src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h b/src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h index 9bc1a40..b27514c 100644 --- a/src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h +++ b/src/vendorcode/intel/fsp/fsp2_0/tigerlake/FspmUpd.h @@ -356,9 +356,41 @@ **/ UINT8 RMT;
-/** Offset 0x0194 - Reserved +/** Offset 0x0194 - DisableDimmCh0 **/ - UINT8 Reserved8[10]; + UINT8 DisableDimmCh0; + +/** Offset 0x0195 - DisableDimmCh1 +**/ + UINT8 DisableDimmCh1; + +/** Offset 0x0196 - DisableDimmCh2 +**/ + UINT8 DisableDimmCh2; + +/** Offset 0x0197 - DisableDimmCh3 +**/ + UINT8 DisableDimmCh3; + +/** Offset 0x0198 - DisableDimmCh4 +**/ + UINT8 DisableDimmCh4; + +/** Offset 0x0199 - DisableDimmCh5 +**/ + UINT8 DisableDimmCh5; + +/** Offset 0x019A - DisableDimmCh6 +**/ + UINT8 DisableDimmCh6; + +/** Offset 0x019B - DisableDimmCh7 +**/ + UINT8 DisableDimmCh7; + +/** Offset 0x019C - Reserved +**/ + UINT8 Reserved8[2];
/** Offset 0x019E - Memory Reference Clock 100MHz, 133MHz. @@ -861,7 +893,7 @@
/** Offset 0x0775 - Reserved **/ - UINT8 Reserved38[355]; + UINT8 Reserved38[315]; } FSP_M_CONFIG;
/** Fsp M UPD Configuration @@ -880,11 +912,11 @@ **/ FSP_M_CONFIG FspmConfig;
-/** Offset 0x08D8 +/** Offset 0x08B0 **/ - UINT8 UnusedUpdSpace24[6]; + UINT8 UnusedUpdSpace23[6];
-/** Offset 0x08DE +/** Offset 0x08B6 **/ UINT16 UpdTerminator; } FSPM_UPD;
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39797 )
Change subject: vendorcode/intel/fsp: Update FSP header for Tiger Lake ......................................................................
Patch Set 4:
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/1963 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1962 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1961
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Caveh Jalali (Code Review)
-
Furquan Shaikh (Code Review)
-
Ronak Kanabar (Code Review)
-
Srinidhi N Kaushik (Code Review)
-
Wonkyu Kim (Code Review)