Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Hannah Williams, Jamie Ryu, Subrata Banik.
Felix Held 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 11:
(2 comments)
File src/soc/intel/common/block/pmc/pmclib.c:
PS11:
> > are the gpe1 registers for the same event as the gpe0 register with the same number? the code usin […]
so i'd guess that it's not correct to use the identical mask pmc_enable_std_gpe and pmc_disable_std_gpe to enable/disable the GPE0 and GPE1
https://review.coreboot.org/c/coreboot/+/84104/comment/1b37ef77_afbebd8b?us… :
PS11, Line 68: /* SoC overrides for GPE1 when SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1 is enabled */
: __weak const char *const *soc_std_gpe1_sts_array(int idx, size_t *a)
: {
: return NULL;
: }
:
: /* disable the corresponding GPE1 bits based on standard GPE0 bits */
: __weak void soc_pmc_disable_std_gpe1(uint32_t gpe0_mask)
: {
: }
:
: /* enable the corresponding GPE1 bits based on standard GPE0 bits */
: __weak void soc_pmc_enable_std_gpe1(uint32_t gpe0_mask)
: {
: }
> > are those weak functions needed? i'd assume that if a soc selects SOC_INTEL_COMMON_BLOCK_ACPI_HAVE […]
when a soc selects SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1, it has to implement those 3 functions in the soc code; when a soc doesn't select SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1, all calls of those functions get optimized out, so i don't think that there's the need to have a weak implementation of those
--
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: 11
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-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: Subrata Banik <subratabanik(a)google.com>
Gerrit-Comment-Date: Mon, 09 Sep 2024 15:46:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Felix Held, Hannah Williams, Jamie Ryu.
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 11: -Code-Review
(1 comment)
File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/84104/comment/a5b31fa9_5c45147d?us… :
PS11, Line 241: gpe0_mask
> shouldn't this be gpe1_mask? same below
Good catch
--
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: 11
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-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: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 09 Sep 2024 15:35:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Felix Held, Hannah Williams, Jamie Ryu.
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 11:
(2 comments)
File src/soc/intel/common/block/pmc/pmclib.c:
PS11:
> are the gpe1 registers for the same event as the gpe0 register with the same number? the code using the same masks for the gpe0 and gpe1 suggest that to me, but i have some doubts that this is actually the case
no, GPE events can't be generalized like GPE0 events, this is some SoC specific depending upon the internal IPs
https://review.coreboot.org/c/coreboot/+/84104/comment/98257a33_f2ea5f0c?us… :
PS11, Line 68: /* SoC overrides for GPE1 when SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1 is enabled */
: __weak const char *const *soc_std_gpe1_sts_array(int idx, size_t *a)
: {
: return NULL;
: }
:
: /* disable the corresponding GPE1 bits based on standard GPE0 bits */
: __weak void soc_pmc_disable_std_gpe1(uint32_t gpe0_mask)
: {
: }
:
: /* enable the corresponding GPE1 bits based on standard GPE0 bits */
: __weak void soc_pmc_enable_std_gpe1(uint32_t gpe0_mask)
: {
: }
> are those weak functions needed? i'd assume that if a soc selects SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1, it needs to implement those and if it doesn't select that option, those function implementations won't be needed.
we need to implement those weak function to provide the lists of GPE1 event as this is soc specific
--
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: 11
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-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: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Mon, 09 Sep 2024 15:35:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Hannah Williams, Jamie Ryu.
Felix Held has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/84104?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: soc/intel/common/block/pmc: Add GPE1 functions
......................................................................
Patch Set 11:
(4 comments)
File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/84104/comment/69d5a39a_643c18f7?us… :
PS11, Line 239: based on standard GPE0 EN bits
hmm, this sounds odd to me
https://review.coreboot.org/c/coreboot/+/84104/comment/ecda8374_54345401?us… :
PS11, Line 241: gpe0_mask
shouldn't this be gpe1_mask? same below
File src/soc/intel/common/block/pmc/pmclib.c:
PS11:
are the gpe1 registers for the same event as the gpe0 register with the same number? the code using the same masks for the gpe0 and gpe1 suggest that to me, but i have some doubts that this is actually the case
https://review.coreboot.org/c/coreboot/+/84104/comment/b5a20cdb_29759aed?us… :
PS11, Line 68: /* SoC overrides for GPE1 when SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1 is enabled */
: __weak const char *const *soc_std_gpe1_sts_array(int idx, size_t *a)
: {
: return NULL;
: }
:
: /* disable the corresponding GPE1 bits based on standard GPE0 bits */
: __weak void soc_pmc_disable_std_gpe1(uint32_t gpe0_mask)
: {
: }
:
: /* enable the corresponding GPE1 bits based on standard GPE0 bits */
: __weak void soc_pmc_enable_std_gpe1(uint32_t gpe0_mask)
: {
: }
are those weak functions needed? i'd assume that if a soc selects SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1, it needs to implement those and if it doesn't select that option, those function implementations won't be needed.
--
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: 11
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-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-Comment-Date: Mon, 09 Sep 2024 15:13:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Cliff Huang.
Felix Held has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/84103?usp=email )
Change subject: soc/intel/common/block/acpi: Add GPE1 blocks to ACPI FADT table
......................................................................
Patch Set 15:
(1 comment)
File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/84103/comment/f0a82554_c1c0c50b?us… :
PS15, Line 110: fadt->gpe1_blk = GPE1_STS(0) ? (pmbase + GPE1_STS(0)) : 0;
: if (fadt->gpe1_blk) {
checking if CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_HAVE_GPE1) is true and if so setting the 3 values within that if block would make the code easier to read
--
To view, visit https://review.coreboot.org/c/coreboot/+/84103?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: Ia6928c35b86f4a2243d58597b17b2a3a5f54271e
Gerrit-Change-Number: 84103
Gerrit-PatchSet: 15
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: 9elements QA <hardwaretestrobot(a)gmail.com>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Comment-Date: Mon, 09 Sep 2024 15:11:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Sean Rhodes.
Matt DeVillier has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/84262?usp=email )
Change subject: mb/starlabs/starbook/adl: Remove PMC GPIO routing
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/84262?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: I340b3474fba1bc7fbde520138ae99c3e355882bf
Gerrit-Change-Number: 84262
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Comment-Date: Mon, 09 Sep 2024 13:27:48 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Sean Rhodes.
Matt DeVillier has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/84261?usp=email )
Change subject: mb/starlabs/starbook/adl: Alphabetize and group FSP UPDs
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/84261?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: I63612af7320dfdbe57029b898b4cf07e9d6f13b0
Gerrit-Change-Number: 84261
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Comment-Date: Mon, 09 Sep 2024 13:27:34 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Sean Rhodes.
Matt DeVillier has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/84263?usp=email )
Change subject: mb/starlabs/starbook/adl: Add USB ACPI to devicetree
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/84263?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: I7050a4d12efd65c7026abf3e45961e2061b7170a
Gerrit-Change-Number: 84263
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Comment-Date: Mon, 09 Sep 2024 13:27:10 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Sean Rhodes.
Matt DeVillier has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/83633?usp=email )
Change subject: ec/starlabs/merlin: Don't report the battery serial number to ACPI
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83633?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: I97a28b1d8d7bb45ea4790c8125cd3c1bc52ee5f9
Gerrit-Change-Number: 83633
Gerrit-PatchSet: 2
Gerrit-Owner: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Comment-Date: Mon, 09 Sep 2024 13:22:25 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes