Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Elyes Haouas, Jamie Ryu, Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Ravishankar Sarawadi, Saurabh Mishra, Wonkyu Kim.
Subrata Banik has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83789?usp=email )
Change subject: soc/intel/ptl: Add GPIOs for Panther Lake SOC
......................................................................
Patch Set 67:
(5 comments)
Patchset:
PS46:
> Thx. Yes. I do plan to have this change in https://review.coreboot. […]
Acknowledged
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83789/comment/897f90f2_c434d5de?us… :
PS17, Line 263: Since Lunarlake, OS pinctrl no long provides SOC GPIO pincntrl info.
: Rather, firmware is responsible for providing such info via APCI.
: Firmware can provide entire PADs or excluding those non GPP_[group]_[number]
: at the end of each community.
> > Subrata, […]
Acknowledged
File src/soc/intel/pantherlake/gpio.c:
https://review.coreboot.org/c/coreboot/+/83789/comment/b5d554f7_ee359622?us… :
PS17, Line 73: .num_groups = ARRAY_SIZE(ptl_community0_groups),
> Yes, only cpu_port needs to be changed. […]
Acknowledged
File src/soc/intel/pantherlake/include/soc/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/83789/comment/1ed83ec9_3fa22c20?us… :
PS49, Line 261: GPP_CPUJTAG_OFFSET
> This is used in gpio.asl.
Acknowledged
File src/soc/intel/pantherlake/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/83789/comment/4d9a6353_d171286d?us… :
PS17, Line 544: #define GPP_VGPIO0 246
> > Let's the feedback on lack of info in PTL EDS and route this to the proper owner for change. […]
Acknowledged
--
To view, visit https://review.coreboot.org/c/coreboot/+/83789?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iae1bc072841214efaec7a10719dbc742f2da795b
Gerrit-Change-Number: 83789
Gerrit-PatchSet: 67
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-CC: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-CC: Sanju Jose Thottan <sanjujose.thottan(a)intel.com>
Gerrit-CC: Saurabh Mishra <mishra.saurabh(a)intel.corp-partner.google.com>
Gerrit-CC: Vikrant L Jadeja <vikrant.l.jadeja(a)intel.com>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Thu, 05 Sep 2024 20:21:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Elyes Haouas, Jamie Ryu, Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Ravishankar Sarawadi, Saurabh Mishra, Wonkyu Kim.
Subrata Banik has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83789?usp=email )
Change subject: soc/intel/ptl: Add GPIOs for Panther Lake SOC
......................................................................
Patch Set 67:
(2 comments)
File src/soc/intel/pantherlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/83789/comment/8b614b41_6378aa05?us… :
PS49, Line 436: Name (JTAG, Package (0x02)
> I have asked our kernel pinctrl owner and we cannot skip PADs within a group, so if any of PADs needs to be exposed, the entire group needs to be added. I didn't get the reason, though. To be conservative and consistent, I follow what we did for the past SOCs and LNL (first with this new schema) for now as it requires more validation and finding out the corner cases if we drop any of those non GPP_[group]_[num] group. Will it be okay if we add this in the TODO list for further revisiting?
sure, please add that as TODO. I'm more interested to understand this whole design as we didn't get involved into LNL design
File src/soc/intel/pantherlake/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/83789/comment/b029f0fb_0398ec19?us… :
PS49, Line 31: 1
> Subrata,
> Currently, we use _UID to describe their community number.
> from PTL, we have:
> INTC10BC:00/uid:0
> INTC10BC:01/uid:1
> INTC10BC:02/uid:3
> INTC10BC:03/uid:4
> INTC10BC:04/uid:5
> I also checked BIOS for other four SOCs, including LNL, that use this new GPIO schema, and they use _UID for the community number. LNL CB also does the same.
>
> Name (_UID, 0) // numeric value, GPIO Community
> Name (_UID, 1) // numeric value, GPIO Community
> Name (_UID, 3) // numeric value, GPIO Community
> Name (_UID, 4) // numeric value, GPIO Community
> Name (_UID, 5) // numeric value, GPIO Community
>
> As their instance already with :0[n] for index number, should we keep this consistent among these SOCs that uses this new schema? Also, I was told that there is no value/benefit/security implications for hiding their actual community number. At the same time, it is not harmful to use the sequential number for their _UID as you suggest.
although I don't think UID is actually used for anything here, because HID and other details are so unique among each GPIO COMM. But lets not make things complicated. marking this section resolved
--
To view, visit https://review.coreboot.org/c/coreboot/+/83789?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iae1bc072841214efaec7a10719dbc742f2da795b
Gerrit-Change-Number: 83789
Gerrit-PatchSet: 67
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-CC: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-CC: Sanju Jose Thottan <sanjujose.thottan(a)intel.com>
Gerrit-CC: Saurabh Mishra <mishra.saurabh(a)intel.corp-partner.google.com>
Gerrit-CC: Vikrant L Jadeja <vikrant.l.jadeja(a)intel.com>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Thu, 05 Sep 2024 19:52:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Attention is currently required from: Anil Kumar K, Bora Guvendik, Elyes Haouas, Jamie Ryu, Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Ravishankar Sarawadi, Saurabh Mishra, Subrata Banik, Wonkyu Kim.
Cliff Huang has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83789?usp=email )
Change subject: soc/intel/ptl: Add GPIOs for Panther Lake SOC
......................................................................
Patch Set 66:
(5 comments)
File src/soc/intel/pantherlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/83789/comment/11118e0c_7b0d127b?us… :
PS49, Line 436: Name (JTAG, Package (0x02)
> this is mostly for FW debugging. […]
I have asked our kernel pinctrl owner and we cannot skip PADs within a group, so if any of PADs needs to be exposed, the entire group needs to be added. I didn't get the reason, though. To be conservative and consistent, I follow what we did for the past SOCs and LNL (first with this new schema) for now as it requires more validation and finding out the corner cases if we drop any of those non GPP_[group]_[num] group. Will it be okay if we add this in the TODO list for further revisiting?
File src/soc/intel/pantherlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/83789/comment/db02c6e2_22343998?us… :
PS55, Line 135: GPPV
> > Acknowledged […]
ouch:( adding comments.
File src/soc/intel/pantherlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/83789/comment/46f1e687_f6ed1cd9?us… :
PS65, Line 770: GPIO
> `VGP0` aka virtual GPIO
okay. this name comes from BIOS and LNL GPIO ASL. VGP0 sounds much better than GPIO. I was also thinking VGP_.
https://review.coreboot.org/c/coreboot/+/83789/comment/1445aee3_bea4d02b?us… :
PS65, Line 805: GADD
> can you please move these ACPI methods before exposing the `GPIO` device ?
Done
File src/soc/intel/pantherlake/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/83789/comment/bd59a2e6_6ce4f0a1?us… :
PS49, Line 31: 1
> > We don't have COMM2, so GPP_COMM3 cannot use this macro, though. […]
Subrata,
Currently, we use _UID to describe their community number.
from PTL, we have:
INTC10BC:00/uid:0
INTC10BC:01/uid:1
INTC10BC:02/uid:3
INTC10BC:03/uid:4
INTC10BC:04/uid:5
I also checked BIOS for other four SOCs, including LNL, that use this new GPIO schema, and they use _UID for the community number. LNL CB also does the same.
Name (_UID, 0) // numeric value, GPIO Community
Name (_UID, 1) // numeric value, GPIO Community
Name (_UID, 3) // numeric value, GPIO Community
Name (_UID, 4) // numeric value, GPIO Community
Name (_UID, 5) // numeric value, GPIO Community
As their instance already with :0[n] for index number, should we keep this consistent among these SOCs that uses this new schema? Also, I was told that there is no value/benefit/security implications for hiding their actual community number. At the same time, it is not harmful to use the sequential number for their _UID as you suggest.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83789?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iae1bc072841214efaec7a10719dbc742f2da795b
Gerrit-Change-Number: 83789
Gerrit-PatchSet: 66
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Reviewer: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-CC: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-CC: Sanju Jose Thottan <sanjujose.thottan(a)intel.com>
Gerrit-CC: Saurabh Mishra <mishra.saurabh(a)intel.corp-partner.google.com>
Gerrit-CC: Vikrant L Jadeja <vikrant.l.jadeja(a)intel.com>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Thu, 05 Sep 2024 19:14:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Jérémy Compostella has submitted this change. ( https://review.coreboot.org/c/coreboot/+/84216?usp=email )
(
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: soc/intel/pantherlake: Hardcode IOM_BASE_ADDR_MAX value
......................................................................
soc/intel/pantherlake: Hardcode IOM_BASE_ADDR_MAX value
iasl refuses to perform an arithmetic computation in a QWordMemory
parameter and fails with the following error.
dsdt.asl 2149: 0x4010800000, ((0x4010800000 + 0x10000) - 1), 0x0,
Error 6051 - ^ Address Min is greater than Address Max
This commit replaces the arithmetic with the result to define
IOM_BASE_ADDR_MAX.
BUG=b:348678529
TEST=Build for google/fatcat mainboard.
Change-Id: Ia5cf899b049cb8eb27b4ea30c7f3ce7a14884f16
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/84216
Reviewed-by: Subrata Banik <subratabanik(a)google.com>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M src/soc/intel/pantherlake/include/soc/iomap.h
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
build bot (Jenkins): Verified
Subrata Banik: Looks good to me, approved
diff --git a/src/soc/intel/pantherlake/include/soc/iomap.h b/src/soc/intel/pantherlake/include/soc/iomap.h
index dbabfd6..652bed8 100644
--- a/src/soc/intel/pantherlake/include/soc/iomap.h
+++ b/src/soc/intel/pantherlake/include/soc/iomap.h
@@ -82,7 +82,7 @@
*/
#define IOM_BASE_ADDR 0x4010800000
#define IOM_BASE_SIZE 0x10000
-#define IOM_BASE_ADDR_MAX ((IOM_BASE_ADDR + IOM_BASE_SIZE) - 1)
+#define IOM_BASE_ADDR_MAX 0x401080ffff /* ((IOM_BASE_ADDR + IOM_BASE_SIZE) - 1) */
/* I/O port address space */
#define ACPI_BASE_ADDRESS 0x1800
--
To view, visit https://review.coreboot.org/c/coreboot/+/84216?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ia5cf899b049cb8eb27b4ea30c7f3ce7a14884f16
Gerrit-Change-Number: 84216
Gerrit-PatchSet: 5
Gerrit-Owner: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Angel Pons.
Hello Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/84225?usp=email
to look at the new patch set (#2).
Change subject: nb/intel/sandybridge: Add Kconfig to set default IGD allocation
......................................................................
nb/intel/sandybridge: Add Kconfig to set default IGD allocation
Add a Kconfig choice to select the default IGD memory allocation, for
users/boards which do not use an option table to set it.
TEST=build/boot google/link, verify IGD size changes with selection.
Change-Id: I83d57cf4657cfccbb21416c5da05eeff9e95a44f
Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com>
---
M src/northbridge/intel/sandybridge/Kconfig
M src/northbridge/intel/sandybridge/early_init.c
2 files changed, 31 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/84225/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/84225?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I83d57cf4657cfccbb21416c5da05eeff9e95a44f
Gerrit-Change-Number: 84225
Gerrit-PatchSet: 2
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>