Attention is currently required from: Ashish Kumar Mishra, Cliff Huang, Felix Held, Jérémy Compostella, Paul Menzel, Pranava Y N, Saurabh Mishra.
Subrata Banik has posted comments on this change by Saurabh Mishra. ( https://review.coreboot.org/c/coreboot/+/83419?usp=email )
Change subject: mb/google/fatcat: Add Panther Lake SOC support
......................................................................
Patch Set 180:
(1 comment)
File src/mainboard/google/fatcat/variants/fatcat/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/83419/comment/36f9edd0_83f67de3?us… :
PS180, Line 9: /* EC wake is LAN_WAKE# which is a special DeepSX wake pin */
: #define GPE_EC_WAKE GPE0_LAN_WAK
should move to next CL (aka GPIO)
--
To view, visit https://review.coreboot.org/c/coreboot/+/83419?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: I914f73ff06bfb801fc319b45b23d7ce4cb7a6d5e
Gerrit-Change-Number: 83419
Gerrit-PatchSet: 180
Gerrit-Owner: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
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>
Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-CC: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-CC: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(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-CC: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 18 Sep 2024 02:38:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Ashish Kumar Mishra, Cliff Huang, Felix Held, Jérémy Compostella, Paul Menzel, Pranava Y N, Saurabh Mishra.
Subrata Banik has posted comments on this change by Saurabh Mishra. ( https://review.coreboot.org/c/coreboot/+/83419?usp=email )
Change subject: mb/google/fatcat: Add Panther Lake SOC support
......................................................................
Patch Set 180:
(11 comments)
File src/mainboard/google/fatcat/Kconfig:
https://review.coreboot.org/c/coreboot/+/83419/comment/e057084f_ed010f5f?us… :
PS180, Line 25: select GBB_FLAG_DISABLE_EC_SOFTWARE_SYNC
why we are disabling EC sync ? if we need to disable EC sync then please move this into line 75 and add a TODO with a bug to reenable it
https://review.coreboot.org/c/coreboot/+/83419/comment/cdccfb52_0f8ae856?us… :
PS180, Line 40: select DRIVERS_INTEL_PMC
why not at common ?
https://review.coreboot.org/c/coreboot/+/83419/comment/62c1feba_e1dfc318?us… :
PS180, Line 42: select DRIVERS_SOUNDWIRE_ALC722
this should go as part of HDA CL ? why not at common ?
https://review.coreboot.org/c/coreboot/+/83419/comment/afa21ea6_45132617?us… :
PS180, Line 50: select SOC_INTEL_PANTHERLAKE_U_H
should be in common as well
https://review.coreboot.org/c/coreboot/+/83419/comment/6fb552c3_9ae57395?us… :
PS180, Line 51: select SOC_INTEL_TCSS_USE_PDC_PMC_USBC_MUX_CONFIGURATION
this should be in common as well
https://review.coreboot.org/c/coreboot/+/83419/comment/bc015389_3c4051e9?us… :
PS180, Line 61: select HAVE_X86_64_SUPPORT
: select USE_X86_64_SUPPORT
why are we selecting it again when PTL SoC already selects the same?
https://review.coreboot.org/c/coreboot/+/83419/comment/10800a8d_058affb6?us… :
PS180, Line 75:
use tab ?
https://review.coreboot.org/c/coreboot/+/83419/comment/04ee5734_c7b9bc8f?us… :
PS180, Line 75: GBB_FLAG_FORCE_DEV_BOOT_LEGACY
why we need this ?
File src/mainboard/google/fatcat/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/83419/comment/1156b733_36320fca?us… :
PS180, Line 32:
one tab less and all below lines as well
File src/mainboard/google/fatcat/variants/baseboard/fatcat/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/83419/comment/3d32da24_bab81f0c?us… :
PS180, Line 3: end
can you please keep the basic and default device entries here ? like IGD, PMC etc. follow other platforms like Rex baseboard code.
File src/mainboard/google/fatcat/variants/fatcat/gpio.c:
https://review.coreboot.org/c/coreboot/+/83419/comment/da7f5737_eca12299?us… :
PS180, Line 49: CROS_GPIO_DEVICE0_NAME
why `DEVICE0` ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/83419?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: I914f73ff06bfb801fc319b45b23d7ce4cb7a6d5e
Gerrit-Change-Number: 83419
Gerrit-PatchSet: 180
Gerrit-Owner: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
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>
Gerrit-CC: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-CC: Balaji Manigandan <balaji.manigandan(a)intel.com>
Gerrit-CC: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-CC: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(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-CC: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Saurabh Mishra <mishra.saurabh(a)intel.com>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 18 Sep 2024 02:38:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: David Wu, Dinesh Gehlot, Eric Lai, Jayvik Desai, Kapil Porwal, Nick Vaccaro, Paul Menzel, Rishika Raj, YH Lin.
Subrata Banik has posted comments on this change by David Wu. ( https://review.coreboot.org/c/coreboot/+/84339?usp=email )
Change subject: mb/google/nissa/var/riven: enable WIFI SAR
......................................................................
Patch Set 6:
(1 comment)
File src/mainboard/google/brya/variants/riven/variant.c:
https://review.coreboot.org/c/coreboot/+/84339/comment/0bac5c8f_93695194?us… :
PS6, Line 14:
: if ((type << 3 | sar_id) == UNDEFINED_FW_CONFIG) {
shouldn't we check if `type` || `sar_id` is UNDEFINED_FW_CONFIG and bail out early ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/84339?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: I647d64a008991a7a20791b2c87ea6308af6bb82e
Gerrit-Change-Number: 84339
Gerrit-PatchSet: 6
Gerrit-Owner: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: YH Lin <yueherngl(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jayvik Desai <jayvik(a)google.com>
Gerrit-Attention: Eric Lai <ericllai(a)google.com>
Gerrit-Attention: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-Attention: YH Lin <yueherngl(a)google.com>
Gerrit-Attention: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Comment-Date: Wed, 18 Sep 2024 02:31:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Cliff Huang, Kapil Porwal, Paul Menzel, Pranava Y N.
Subrata Banik has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/84297?usp=email )
Change subject: soc/intel/ptl: Define GPE1 macros and register fields
......................................................................
Patch Set 12:
(4 comments)
File src/soc/intel/pantherlake/include/soc/gpe.h:
https://review.coreboot.org/c/coreboot/+/84297/comment/8292243e_0e5e06f7?us… :
PS12, Line 153: all
`All` and same for all below comments as well
File src/soc/intel/pantherlake/include/soc/gpe.h:
https://review.coreboot.org/c/coreboot/+/84297/comment/915a7dbc_3c8fa2f5?us… :
PS4, Line 30: 146
> > okay. will it be okay if TCSS still contain the defines from gpe.h and pm.h for now? […]
Acknowledged
File src/soc/intel/pantherlake/include/soc/pm.h:
PS9:
> The actual event bits for GPE1 are PMC I/O registers, which are defines in pm.h. […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/84297/comment/98d80fce_940b47d8?us… :
PS9, Line 133: #if CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1)
> > Subrata, we will hit the GPE1_* marco redefined build error if CONFIG_SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1 is not set in PTL. I recall I brought up this issue.
>
> yes, I remember. In such case we should introduce `SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1` and `SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1`
>
> PTL SoC to select SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1 and mainboard can select SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1 if they wish to use it.
>
> this way, you don't run into any issue (macro definition of is still inside !SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1, therefore, for PTL it will only include one/proper register definition)
>
> ```
> fadt->gpe1_blk = 0;
> if (CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_USE_GPE1)) {
> fadt->gpe1_blk = pmbase + GPE1_STS(0);
> fadt->gpe1_blk_len = 2 * GPE1_REG_MAX * sizeof(uint32_t);
> /*
> * NOTE: gpe1 is after gpe0, which has _STS and _EN register sets.
> * gpe1_base is the starting bit offset for GPE1.
> */
> fadt->gpe1_base = fadt->gpe0_blk_len / 2 * 8;
> }
> ```
can you drop this CPP guard now ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/84297?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: Iebf6f6d02b37cc9702e4ee07c1ec0017b6628836
Gerrit-Change-Number: 84297
Gerrit-PatchSet: 12
Gerrit-Owner: Cliff Huang <cliff.huang(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>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Cliff Huang <cliff.huang(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-Comment-Date: Wed, 18 Sep 2024 02:30:14 +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>
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Felix Held, Hannah Williams, Jamie Ryu, Paul Menzel.
Subrata Banik has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/84104?usp=email )
Change subject: soc/intel/common/block/pmc: Add GPE1 functions
......................................................................
Patch Set 27:
(3 comments)
File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/84104/comment/37fffe2d_7226df40?us… :
PS27, Line 235: const char *const *soc_std_gpe1_sts_array(int idx, size_t *a);
u can also move this inside `#if` and keep a inline `NULL` API when `!SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1`
File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/84104/comment/083a9071_9b60cb05?us… :
PS11, Line 241: gpe0_mask
> sure. on my list.
Acknowledged
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/84104/comment/3b71675c_9b23b5a8?us… :
PS24, Line 421: print_std_gpe1_sts(gpe1_sts);
> Similar to GPE0 in line 415, the STS values are read and saved prior to the reset. […]
Acknowledged
--
To view, visit https://review.coreboot.org/c/coreboot/+/84104?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: I7ac1fbe6d45cbe0c86c3f72911900d92a186168d
Gerrit-Change-Number: 84104
Gerrit-PatchSet: 27
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: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 18 Sep 2024 02:28:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hannah Williams <hannah.williams(a)intel.com>
Comment-In-Reply-To: Cliff Huang <cliff.huang(a)intel.com>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>