coreboot-gerrit April 2023

coreboot-gerrit@coreboot.org

1 participants
5761 discussions

[S] Change in coreboot[master]: mb/google/dedede/var/kracko: Add G2touch touchscreen support
by Robert Chen (Code Review) 13 Apr '23

13 Apr '23

Attention is currently required from: Paul Menzel, Shou-Chieh Hsu. Hello Kevin Chiu, build bot (Jenkins), Eric Lai, Shou-Chieh Hsu, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/74307 to look at the new patch set (#7). Change subject: mb/google/dedede/var/kracko: Add G2touch touchscreen support ...................................................................... mb/google/dedede/var/kracko: Add G2touch touchscreen support Add G2touch touchscreen support for kracko. BOE NV116WHM-T04 V8.0 with G7500 touch panel sensor IC BUG=b:277852921 BRANCH=firmware-dedede-13606.B TEST=emerge-dedede coreboot & test on DUT Change-Id: Ic065d5dc2900c6ccfee09031f7a80cefc391f5dd Signed-off-by: Robert Chen <robert.chen(a)quanta.corp-partner.google.com> --- M src/mainboard/google/dedede/variants/kracko/overridetree.cb 1 file changed, 31 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/74307/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/74307 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic065d5dc2900c6ccfee09031f7a80cefc391f5dd Gerrit-Change-Number: 74307 Gerrit-PatchSet: 7 Gerrit-Owner: Robert Chen <robert.chen(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Kevin Chiu <kevin.chiu.17802(a)gmail.com> Gerrit-Reviewer: Shou-Chieh Hsu <shouchieh(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Shou-Chieh Hsu <shouchieh(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: sb,soc/amd,intel: Add and use ACPI_COMMON_MADT_LAPIC
by Nico Huber (Code Review) 13 Apr '23

13 Apr '23

Attention is currently required from: Lance Zhao, Jason Glenesk, Raul Rangel, Matt DeVillier, Sridhar Siricilla, Arthur Heymans, Kyösti Mälkki, Fred Reitberger, Felix Held. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74316 ) Change subject: sb,soc/amd,intel: Add and use ACPI_COMMON_MADT_LAPIC ...................................................................... Patch Set 2: Code-Review+1 (3 comments) Patchset: PS2: Tried to match Kconfig and code changes, only found asus/p2b unmatched. File src/acpi/acpi.c: https://review.coreboot.org/c/coreboot/+/74316/comment/7d0d1023_92012c9e PS2, Line 296: if (CONFIG(ACPI_COMMON_MADT_LAPIC)) Not sure if this should be guarded by !ACPI_NO_MADT as well. Looking through the affected boards, I found asus/p2b which selects ACPI_NO_MADT but gets ACPI_COMMON_MADT_LAPIC selected now. It wouldn't be wrong I guess. But if the change is expected, it should be noted in the commit message. File src/soc/intel/common/block/acpi/acpi.c: https://review.coreboot.org/c/coreboot/+/74316/comment/c9623cb4_be499de8 PS2, Line 90: ACPI_COMMON_MADT_LAPIC I would prefer to keep the original SOC_INTEL_COMMON_ACPI_CPU_HYBRID. If by any chance in the future, ACPI_COMMON_MADT_LAPIC wouldn't be selected by accident, this could get quite confusing. -- To view, visit https://review.coreboot.org/c/coreboot/+/74316 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f8d7ee9891553742d73a92b55a87c04fa95a132 Gerrit-Change-Number: 74316 Gerrit-PatchSet: 2 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 12 Apr 2023 22:31:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: security/vboot: Add function to clear recovery request
by Julius Werner (Code Review) 13 Apr '23

13 Apr '23

Attention is currently required from: Michał Żygowski, Yu-Ping Wu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74343 ) Change subject: security/vboot: Add function to clear recovery request ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Can you please split this into a patch that does just the vboot uprev itself (with the commit message following the format of other uprevs like CB:73058), and a second patch that adds the code using it to coreboot? For the second patch, please also include the part where you actually want to call the function so we can review the whole picture. (Not exactly sure what your plan is there... if you want to call it from the payload, then obviously you wouldn't need this wrapper in coreboot.) -- To view, visit https://review.coreboot.org/c/coreboot/+/74343 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7ffaf3e8f61a28a68c9802c184961b1b9bf9d617 Gerrit-Change-Number: 74343 Gerrit-PatchSet: 2 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Wed, 12 Apr 2023 22:04:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/myst: Enable tis_plat_irq_status
by Tim Van Patten (Code Review) 13 Apr '23

13 Apr '23

Attention is currently required from: Raul Rangel, Jon Murphy, Karthik Ramasubramanian, Mark Hasemeyer. Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74286 ) Change subject: mb/google/myst: Enable tis_plat_irq_status ...................................................................... Patch Set 12: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/74286 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I611a2855d94167748d0f82a478687fe2cdf5846a Gerrit-Change-Number: 74286 Gerrit-PatchSet: 12 Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com> Gerrit-Comment-Date: Wed, 12 Apr 2023 22:04:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: soc/amd/common/block/lpc/spi_dma: Leverage CBFS_CACHE when using SPI DMA
by Tim Van Patten (Code Review) 12 Apr '23

12 Apr '23

Attention is currently required from: Jason Glenesk, Raul Rangel, Martin Roth, Fred Reitberger, Karthik Ramasubramanian, Felix Held. Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74334 ) Change subject: soc/amd/common/block/lpc/spi_dma: Leverage CBFS_CACHE when using SPI DMA ...................................................................... Patch Set 4: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/74334/comment/9f34cac5_2952018d PS4, Line 13: de-compression typo: de-compressed File src/soc/amd/common/block/lpc/spi_dma.c: https://review.coreboot.org/c/coreboot/+/74334/comment/c4e2c1f1_e9e64b05 PS4, Line 218: if (!CONFIG_CBFS_CACHE_SIZE) I suggest updating this to: ``` // Assume the destination returned by `mem_pool_alloc()` is aligned. if (!CONFIG_CBFS_CACHE_SIZE || !can_use_dma(LPC_ROM_DMA_MIN_ALIGNMENT, offset, size)) ``` Otherwise, `spi_dma_readat_mmap()` (via `spi_dma_readat()`, called later) will perform an extra copy into the cache, which we can avoid. https://review.coreboot.org/c/coreboot/+/74334/comment/296755da_5c5df2a3 PS4, Line 223: printk(BIOS_INFO, "%s: Could not allocate from memory pool\n", __func__); Please log `size` here also, so we know how much we're trying to allocate and whether it even has a chance to succeed or not. -- To view, visit https://review.coreboot.org/c/coreboot/+/74334 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie30b6324f9977261c60e55ed509e979ef290f1f1 Gerrit-Change-Number: 74334 Gerrit-PatchSet: 4 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)amd.corp-partner.google.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 12 Apr 2023 21:55:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/rex: add variant gpio tables for variant creation
by YH Lin (Code Review) 12 Apr '23

12 Apr '23

Attention is currently required from: Tarun Tuli, Subrata Banik, Daniel Rosa Franzini, Kapil Porwal, Eric Lai. Hello build bot (Jenkins), Tarun Tuli, Subrata Banik, Kapil Porwal, Paul Fagerburg, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/74257 to look at the new patch set (#6). Change subject: mb/google/rex: add variant gpio tables for variant creation ...................................................................... mb/google/rex: add variant gpio tables for variant creation BUG=b:276818954 TEST=new_variant_fulltest.sh rex0 BRANCH=None Signed-off-by: YH Lin <yueherngl(a)google.com> Change-Id: Iebc098f8d480ac3e1835b00861fd844d97f281a8 --- A util/mainboard/google/rex0/template/Makefile.inc A util/mainboard/google/rex0/template/gpio.c 2 files changed, 49 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/74257/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/74257 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iebc098f8d480ac3e1835b00861fd844d97f281a8 Gerrit-Change-Number: 74257 Gerrit-PatchSet: 6 Gerrit-Owner: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Rosa Franzini <danielt3(a)usp.br> Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Daniel Rosa Franzini <danielt3(a)usp.br> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: mb/google/rex: add variant gpio tables for variant creation
by YH Lin (Code Review) 12 Apr '23

12 Apr '23

Attention is currently required from: Tarun Tuli, Subrata Banik, Daniel Rosa Franzini, Kapil Porwal, Eric Lai. YH Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74257 ) Change subject: mb/google/rex: add variant gpio tables for variant creation ...................................................................... Patch Set 5: (1 comment) Patchset: PS4: > @YH, let's put the gpio. […] Done. -- To view, visit https://review.coreboot.org/c/coreboot/+/74257 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iebc098f8d480ac3e1835b00861fd844d97f281a8 Gerrit-Change-Number: 74257 Gerrit-PatchSet: 5 Gerrit-Owner: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Rosa Franzini <danielt3(a)usp.br> Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Daniel Rosa Franzini <danielt3(a)usp.br> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Wed, 12 Apr 2023 21:29:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/rex: add variant gpio tables for variant creation
by YH Lin (Code Review) 12 Apr '23

12 Apr '23

Attention is currently required from: YH Lin, Tarun Tuli, Subrata Banik, Daniel Rosa Franzini, Kapil Porwal. Hello build bot (Jenkins), Tarun Tuli, Subrata Banik, Kapil Porwal, Paul Fagerburg, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/74257 to look at the new patch set (#5). Change subject: mb/google/rex: add variant gpio tables for variant creation ...................................................................... mb/google/rex: add variant gpio tables for variant creation BUG=b:276818954 TEST=new_variant_fulltest.sh rex0 BRANCH=None Signed-off-by: YH Lin <yueherngl(a)google.com> Change-Id: Iebc098f8d480ac3e1835b00861fd844d97f281a8 --- A util/mainboard/google/rex0/template/Makefile.inc A util/mainboard/google/rex0/template/gpio.c 2 files changed, 49 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/74257/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/74257 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iebc098f8d480ac3e1835b00861fd844d97f281a8 Gerrit-Change-Number: 74257 Gerrit-PatchSet: 5 Gerrit-Owner: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Daniel Rosa Franzini <danielt3(a)usp.br> Gerrit-CC: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: YH Lin <yueherngl(a)google.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Daniel Rosa Franzini <danielt3(a)usp.br> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: amdfwtool: Add --fsp-version option
by Raul Rangel (Code Review) 12 Apr '23

12 Apr '23

Attention is currently required from: Jason Glenesk, Matt DeVillier, Paul Menzel, Grzegorz Bernacki, Arthur Heymans, Fred Reitberger, Karthik Ramasubramanian, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74266 ) Change subject: amdfwtool: Add --fsp-version option ...................................................................... Patch Set 1: (1 comment) File util/amdfwtool/amdfwtool.c: https://review.coreboot.org/c/coreboot/+/74266/comment/4b2fd4ce_eac5eef2 PS1, Line 214: FSP blobs > This is basically a misnomer. […] `--output-manifest <FILE> Writes a manifest containing the versions of all the blobs` -- To view, visit https://review.coreboot.org/c/coreboot/+/74266 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idaa3a02ace524f44cfa656e34308bd896016dff6 Gerrit-Change-Number: 74266 Gerrit-PatchSet: 1 Gerrit-Owner: Grzegorz Bernacki Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Grzegorz Bernacki Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 12 Apr 2023 20:45:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Grzegorz Bernacki Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: src/mb/purism/librem_cnl: Enable Librem 14 jack detect with fixed EC
by Jonathon Hall (Code Review) 12 Apr '23

12 Apr '23

Attention is currently required from: Tim Crawford, Jeremy Soller, Paul Menzel, Angel Pons, Felix Held. Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74364 ) Change subject: src/mb/purism/librem_cnl: Enable Librem 14 jack detect with fixed EC ...................................................................... Patch Set 2: (13 comments) Patchset: PS1: > If possible, it’d be nice to split the mainboard change out into a separate commit. I split this up. File src/ec/purism/librem-ec/librem_ec.h: https://review.coreboot.org/c/coreboot/+/74364/comment/2c87809d_64cbdd78 PS1, Line 9: update > If you know the version, please mention it. Done https://review.coreboot.org/c/coreboot/+/74364/comment/623a1c91_c350247a PS1, Line 8: /* Check whether librem-ec has working jack detect. This was fixed in an : * update, so we only use the verbs with jack detect if the EC has been updated. : */ > Please use the recommended multi-line comment style. Done File src/ec/purism/librem-ec/librem_ec.c: https://review.coreboot.org/c/coreboot/+/74364/comment/b0671d70_43ffb523 PS1, Line 11: /* The 'flags' field in the probe command reply was added in an update. : * Send 4 bytes of zeroes in the "request" to zero out the field if the : * EC does not set it for its reply. */ > Please use the recommended multi-line comment style. Done https://review.coreboot.org/c/coreboot/+/74364/comment/2527546d_5d70e3f5 PS1, Line 17: reply_data, ARRAY_SIZE(reply_data))) : return false; > Aligning both lines the same is a little confusing to me. […] Hmm, yeah this looked odd, adding braces did not help either. I moved the condition out to a separate statement which I think looks better. File src/ec/system76/ec/system76_ec.h: https://review.coreboot.org/c/coreboot/+/74364/comment/541a728b_13b13721 PS1, Line 9: /* Send a command to the EC. request_data/request_size are the request payload, : * request_data can be NULL if request_size is 0. reply_data/reply_size are : * the reply payload, reply_data can be NULL if reply_size is 0. */ > Please use the recommended multi-line comment style. Done File src/ec/system76/ec/system76_ec.c: https://review.coreboot.org/c/coreboot/+/74364/comment/babb15a0_760b0122 PS1, Line 84: uint8_t i; > No need to limit the length. […] system76_ec_write() takes a uint8_t for the offset since it is limited to 256 bytes. If I use an unsigned int, it has to be truncated for each call to system76_ec_write(). https://review.coreboot.org/c/coreboot/+/74364/comment/719d54a9_16f35508 PS1, Line 86: REG_DATA+i > Please spaces around the operator. Done https://review.coreboot.org/c/coreboot/+/74364/comment/d42ddd6c_5144bd62 PS1, Line 96: ret = false; > Log an error? Done File src/mainboard/purism/librem_cnl/variants/librem_14/hda_verb.c: https://review.coreboot.org/c/coreboot/+/74364/comment/cd23ae52_e94352d3 PS1, Line 64: /* Older verbs with no jack detect - needed if an older Librem EC is in use that : * lacks jack detect. Headphones can be selected manually. */ > Please use the recommended multi-line comment style. Done https://review.coreboot.org/c/coreboot/+/74364/comment/2aa4b755_c83c9422 PS1, Line 74: /* Now that the codec is configured, we can check if the EC has : * jack detect. */ > Please use the recommended multi-line comment style. Done https://review.coreboot.org/c/coreboot/+/74364/comment/aef006f8_a21d1cee PS1, Line 79: printk(BIOS_WARNING, "EC firmware lacks jack detect, applying workaround\n"); > Add a comment to update the EC firmware? I added a log message, if any end users see this in the logs they will know to update the EC firmware. https://review.coreboot.org/c/coreboot/+/74364/comment/aa84f47d_121fe234 PS1, Line 80: /* The EC firmware lacks jack detect. Program the : * older workaround verbs with no jack detect. */ > Please use the recommended multi-line comment style. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/74364 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I57a27b1d51e4f6c7c712bcb2823d21692b9c5ce6 Gerrit-Change-Number: 74364 Gerrit-PatchSet: 2 Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jeremy Soller <jeremy(a)system76.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tim Crawford <tcrawford(a)system76.com> Gerrit-Attention: Jeremy Soller <jeremy(a)system76.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 12 Apr 2023 20:24:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2023