coreboot-gerrit January 2024

coreboot-gerrit@coreboot.org

1 participants
3043 discussions

[XL] Change in coreboot[main]: acpi: Reformat C files with clang-format v16
by Julius Werner (Code Review) 24 Jan '24

24 Jan '24

Attention is currently required from: Cliff Huang, Lance Zhao, Martin L Roth, Tim Wawrzynczak. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80007?usp=email ) Change subject: acpi: Reformat C files with clang-format v16 ...................................................................... Patch Set 2: (1 comment) File src/acpi/acpi.c: https://review.coreboot.org/c/coreboot/+/80007/comment/91158f45_4f19783b : PS1, Line 100: xsdt->header > it was pointed out to me that this wrapping looks like regular next-level indentation and argued tha […] [to continue the discussion here] I've always done it this way and seen other people do it like this on pretty much every patch I worked on (if they bother to pay special attention to alignment at all). I've almost never seen double-indent in coreboot, only in special cases where there's really a risk of confusion (e.g. if you continue an if(), while() or for() statement then there's actually an indented body below which you need to differentiate from the continuation line; but those are notably shorter than 8 characters so aligning to the parenthesis is also a good alternative to provide sufficient visual distinction there). I think the parenthesis align is very important for readability in more complicated cases (with nested parenthesis), and I don't think it does any harm in cases like this, so I'd strongly prefer we keep it. -- To view, visit https://review.coreboot.org/c/coreboot/+/80007?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I5814fc7f04544a4968d3328e19cf67264a5f39d8 Gerrit-Change-Number: 80007 Gerrit-PatchSet: 2 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Comment-Date: Wed, 24 Jan 2024 18:46:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin L Roth <gaumless(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: mb/google/brya: alphabetize model configs
by Subrata Banik (Code Review) 24 Jan '24

24 Jan '24

Attention is currently required from: Eric Lai, Kapil Porwal, Nick Vaccaro, Nick Vaccaro, Shelley Chen. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80162?usp=email ) Change subject: mb/google/brya: alphabetize model configs ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/80162?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Id9347421337d451ce72fcf3984489b06f372f70c Gerrit-Change-Number: 80162 Gerrit-PatchSet: 1 Gerrit-Owner: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Eric Lai <ericllai(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Comment-Date: Wed, 24 Jan 2024 18:29:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: Add MTCL function to ACPI SSDT tables
by Nico Huber (Code Review) 24 Jan '24

24 Jan '24

Attention is currently required from: Cliff Huang, David Ruth, Lance Zhao, Paul Menzel, Subrata Banik, Subrata Banik, Tim Wawrzynczak. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80170?usp=email ) Change subject: Add MTCL function to ACPI SSDT tables ...................................................................... Patch Set 7: (5 comments) Patchset: PS6: > The information is put into firmware so that it's available immediately after power-on, as it handle […] Sorry, is there a misunderstanding? I meant the coreboot firmware (not the firmware of the wifi controller). File src/drivers/wifi/generic/acpi.c: https://review.coreboot.org/c/coreboot/+/80170/comment/e7751c0d_39f9b779 : PS6, Line 45: } > I was following what was previously implemented for a similar feature. […] Thanks! https://review.coreboot.org/c/coreboot/+/80170/comment/056b45f4_b777171d : PS6, Line 592: uint8_t mtcl_package[sizeof(struct wifi_mtcl)]; > The ACPI function realistically needs to handle a byte array. […] Ok, I think I understand better now why you want this part to be more generic. But isn't actually everything in this patch Mediatek specific? I don't understand the split (acpigen/, drivers/wifi/, chromeos/) when everything has Mediatek in the name (assuming MTCL means mediatek country list). File src/vendorcode/google/chromeos/mtcl.c: PS6: > The functionality is being added specifically for Chromebooks and ChromeOS. We generally name things by the chip manufacturers not the OEMs or who added it or alike. Also, vendorcode/ is officially for mirrored code that has its upstream somewhere else. I don't expect it to be the case here. The counter- part in Linux is regularly under drivers/net/wireless/, there is no reason to do anything else in coreboot. `drivers/wifi/generic/` needs this, and if there is no alternative implementation, why not place it there? https://review.coreboot.org/c/coreboot/+/80170/comment/a4f73970_47e506c5 : PS6, Line 59: if (mtcl_bin_len != sizeof(struct wifi_mtcl)) { > Because that would require callers to use this implementation, and also require all implementations […] Do we expect any other callers, any other implementations? -- To view, visit https://review.coreboot.org/c/coreboot/+/80170?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I9b5e7312a44e114270e664b983626faa6cfee350 Gerrit-Change-Number: 80170 Gerrit-PatchSet: 7 Gerrit-Owner: David Ruth <druth(a)chromium.org> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: David Ruth <druth(a)chromium.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Comment-Date: Wed, 24 Jan 2024 17:53:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Ruth <druth(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: Add MTCL function to ACPI SSDT tables
by David Ruth (Code Review) 24 Jan '24

24 Jan '24

Attention is currently required from: Cliff Huang, Lance Zhao, Nico Huber, Paul Menzel, Subrata Banik, Subrata Banik, Tim Wawrzynczak. David Ruth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80170?usp=email ) Change subject: Add MTCL function to ACPI SSDT tables ...................................................................... Patch Set 6: (6 comments) Patchset: PS6: > Hi, I have a very general question: why is this information put into the firmware? […] The information is put into firmware so that it's available immediately after power-on, as it handles functionality that is related to regulatory concerns. It's specific to MediaTek chipsets that support operation on 6GHz bands and OEMs that want to leverage this functionality. File src/drivers/wifi/generic/acpi.c: https://review.coreboot.org/c/coreboot/+/80170/comment/759e3bbe_7423cd8d : PS6, Line 45: } > Why the weak function? Wouldn't it be an error to select USE_MTCL without […] I was following what was previously implemented for a similar feature. get_wifi_sar_limits above defines a weak function, so I naively assumed that following the same pattern would make sense. That said, I agree that enabling the feature without an implementation should probably fail, so I've removed it. https://review.coreboot.org/c/coreboot/+/80170/comment/b7145303_fead6f43 : PS6, Line 591: CONFIG(USE_MTCL) > Putting this first would allow the compiler/linker to eliminate the other […] Done. https://review.coreboot.org/c/coreboot/+/80170/comment/a6ae6cc1_4d629a9b : PS6, Line 592: uint8_t mtcl_package[sizeof(struct wifi_mtcl)]; > Why not declare a `struct wifi_mtcl`? The ACPI function realistically needs to handle a byte array. I only added the struct to help make the format clearer, and to validate that it contains what is expected if the file is read (validation is handled in mtcl.c). I think it's easier and more straightforward to leave the struct as an implementation detail in mtcl.c, as mentioned in the other comment related to this topic. I don't see a compelling reason to use the struct in this case. File src/vendorcode/google/chromeos/mtcl.c: PS6: > What is chromeos specific about this? The functionality is being added specifically for Chromebooks and ChromeOS. It is similar in many ways to the functionality in sar.c and wrdd.c in the same directory, so this seemed to be a natural place for it. Let me know if there's a better place for it. https://review.coreboot.org/c/coreboot/+/80170/comment/e91a7292_93ce91c4 : PS6, Line 59: if (mtcl_bin_len != sizeof(struct wifi_mtcl)) { > If this function knows `struct wifi_mtcl` why isn't it used in the signature? Because that would require callers to use this implementation, and also require all implementations to use that struct. My goal is to keep this function easy and straightforward to call, and abstract implementation details into the function, rather than require callers to know about them. -- To view, visit https://review.coreboot.org/c/coreboot/+/80170?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I9b5e7312a44e114270e664b983626faa6cfee350 Gerrit-Change-Number: 80170 Gerrit-PatchSet: 6 Gerrit-Owner: David Ruth <druth(a)chromium.org> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Comment-Date: Wed, 24 Jan 2024 17:30:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: Add MTCL function to ACPI SSDT tables
by David Ruth (Code Review) 24 Jan '24

24 Jan '24

Attention is currently required from: Cliff Huang, David Ruth, Lance Zhao, Paul Menzel, Subrata Banik, Subrata Banik, Tim Wawrzynczak. Hello Cliff Huang, Lance Zhao, Subrata Banik, Tim Wawrzynczak, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80170?usp=email to look at the new patch set (#7). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: Add MTCL function to ACPI SSDT tables ...................................................................... Add MTCL function to ACPI SSDT tables The MTCL function provides a country list to the Linux kernel via an ACPI function in SSDT tables for MediaTek chipsets that are capable of operating on the 6GHz band. The country list is used to selectively disable 6GHz and 5.9GHz operation based on the country the device is operating in. The function needs to read a binary file and send it as a package via the MTCL method in SSDT for PCIe WiFi chips for MediaTek chipsets. BUG=b:295544553 TEST=Add Kconfig entry USE_MTCL for pujjo TEST=Add wifi_mtcl_defaults.hex blob to cbfs TEST=Build coreboot for pujjo `emerge-nissa coreboot chromeos-bootimage` TEST=Verify that MTCL defined in the file is present: TEST=`acpidump -b` TEST=`iasl ssdt.dat` TEST=`less ssdt.dsl` TEST=Search for MTCL Signed-off-by: David Ruth <druth(a)chromium.org> Change-Id: I9b5e7312a44e114270e664b983626faa6cfee350 --- M src/acpi/acpigen.c M src/drivers/wifi/generic/Kconfig M src/drivers/wifi/generic/Makefile.inc M src/drivers/wifi/generic/acpi.c M src/include/acpi/acpigen.h M src/include/device/pci_ids.h A src/include/mtcl.h M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/mtcl.c 9 files changed, 219 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/80170/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/80170?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I9b5e7312a44e114270e664b983626faa6cfee350 Gerrit-Change-Number: 80170 Gerrit-PatchSet: 7 Gerrit-Owner: David Ruth <druth(a)chromium.org> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Subrata Banik <subi.banik(a)gmail.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: David Ruth <druth(a)chromium.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[main]: mb/google/brya: alphabetize model configs
by Nick Vaccaro (Code Review) 24 Jan '24

24 Jan '24

Nick Vaccaro has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80162?usp=email ) Change subject: mb/google/brya: alphabetize model configs ...................................................................... mb/google/brya: alphabetize model configs Alphabetize the ordering of model configs and selects in Kconfig and Kconfig.name BUG=None BRANCH=None TEST='emerge-brya coreboot' and verify it builds. Change-Id: Id9347421337d451ce72fcf3984489b06f372f70c Signed-off-by: Nick Vaccaro <nvaccaro(a)google.com> --- M src/mainboard/google/brya/Kconfig M src/mainboard/google/brya/Kconfig.name 2 files changed, 29 insertions(+), 29 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/80162/1 diff --git a/src/mainboard/google/brya/Kconfig b/src/mainboard/google/brya/Kconfig index 7b0d587..dae3f16 100644 --- a/src/mainboard/google/brya/Kconfig +++ b/src/mainboard/google/brya/Kconfig @@ -123,6 +123,17 @@ select HAVE_PCIE_WWAN select HAVE_WWAN_POWER_SEQUENCE +config BOARD_GOOGLE_ANRAGGAR + select BOARD_GOOGLE_BASEBOARD_NISSA + select BOARD_ROMSIZE_KB_16384 + select DRIVERS_GENERIC_BAYHUB_LV2 + select DRIVERS_GENERIC_GPIO_KEYS + select INTEL_GMA_HAVE_VBT + select DRIVERS_INTEL_MIPI_CAMERA + select HAVE_WWAN_POWER_SEQUENCE + select DRIVERS_GFX_GENERIC + select CHROMEOS_WIFI_SAR if CHROMEOS + config BOARD_GOOGLE_AURASH select BOARD_GOOGLE_BASEBOARD_BRASK select INTEL_GMA_HAVE_VBT @@ -180,6 +191,12 @@ select INTEL_GMA_HAVE_VBT select SOC_INTEL_RAPTORLAKE +config BOARD_GOOGLE_DOCHI + select BOARD_GOOGLE_BASEBOARD_BRYA + select SOC_INTEL_RAPTORLAKE + select DRIVERS_INTEL_ISH + select USE_UNIFIED_AP_FIRMWARE_FOR_UFS_AND_NON_UFS + config BOARD_GOOGLE_FELWINTER select BOARD_GOOGLE_BASEBOARD_BRYA select CHROMEOS_WIFI_SAR if CHROMEOS @@ -303,6 +320,9 @@ select DRIVERS_GENERIC_GPIO_KEYS select INTEL_GMA_HAVE_VBT +config BOARD_GOOGLE_NOKRIS + select BOARD_GOOGLE_BASEBOARD_NISSA + config BOARD_GOOGLE_OMNIGUL select BOARD_GOOGLE_BASEBOARD_BRYA select CHROMEOS_WIFI_SAR if CHROMEOS @@ -487,26 +507,6 @@ select SOC_INTEL_COMMON_BLOCK_IPU select SOC_INTEL_RAPTORLAKE -config BOARD_GOOGLE_NOKRIS - select BOARD_GOOGLE_BASEBOARD_NISSA - -config BOARD_GOOGLE_DOCHI - select BOARD_GOOGLE_BASEBOARD_BRYA - select SOC_INTEL_RAPTORLAKE - select DRIVERS_INTEL_ISH - select USE_UNIFIED_AP_FIRMWARE_FOR_UFS_AND_NON_UFS - -config BOARD_GOOGLE_ANRAGGAR - select BOARD_GOOGLE_BASEBOARD_NISSA - select BOARD_ROMSIZE_KB_16384 - select DRIVERS_GENERIC_BAYHUB_LV2 - select DRIVERS_GENERIC_GPIO_KEYS - select INTEL_GMA_HAVE_VBT - select DRIVERS_INTEL_MIPI_CAMERA - select HAVE_WWAN_POWER_SEQUENCE - select DRIVERS_GFX_GENERIC - select CHROMEOS_WIFI_SAR if CHROMEOS - if BOARD_GOOGLE_BRYA_COMMON config BASEBOARD_DIR diff --git a/src/mainboard/google/brya/Kconfig.name b/src/mainboard/google/brya/Kconfig.name index 9c3baf0..dcdf3c7 100644 --- a/src/mainboard/google/brya/Kconfig.name +++ b/src/mainboard/google/brya/Kconfig.name @@ -9,6 +9,9 @@ config BOARD_GOOGLE_ANAHERA4ES bool "-> Anahera4ES" +config BOARD_GOOGLE_ANRAGGAR + bool "-> Anraggar" + config BOARD_GOOGLE_AURASH bool "-> Aurash" @@ -33,6 +36,9 @@ config BOARD_GOOGLE_CROTA bool "-> Crota" +config BOARD_GOOGLE_DOCHI + bool "-> Dochi" + config BOARD_GOOGLE_FELWINTER bool "-> Felwinter" @@ -84,6 +90,9 @@ config BOARD_GOOGLE_NEREID bool "-> Nereid" +config BOARD_GOOGLE_NOKRIS + bool "-> Nokris" + config BOARD_GOOGLE_OMNIGUL bool "-> Omnigul" @@ -144,14 +153,5 @@ config BOARD_GOOGLE_ZYDRON bool "-> Zydron" -config BOARD_GOOGLE_NOKRIS - bool "-> Nokris" - -config BOARD_GOOGLE_DOCHI - bool "-> Dochi" - -config BOARD_GOOGLE_ANRAGGAR - bool "-> Anraggar" - config BOARD_GOOGLE_XOL bool "-> Xol" -- To view, visit https://review.coreboot.org/c/coreboot/+/80162?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Id9347421337d451ce72fcf3984489b06f372f70c Gerrit-Change-Number: 80162 Gerrit-PatchSet: 1 Gerrit-Owner: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-MessageType: newchange

1 0

[XS] Change in coreboot[main]: soc/amd/common: Fix typo
by Felix Held (Code Review) 24 Jan '24

24 Jan '24

Attention is currently required from: Fred Reitberger, Jason Glenesk, Matt DeVillier, Varshit Pandya. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80161?usp=email ) Change subject: soc/amd/common: Fix typo ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/80161?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ida6e87908ae6996529057c8df12dbe046ee54b98 Gerrit-Change-Number: 80161 Gerrit-PatchSet: 1 Gerrit-Owner: Varshit Pandya <pandyavarshit(a)gmail.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)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Comment-Date: Wed, 24 Jan 2024 17:04:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: soc/amd/common: Fix typo
by Varshit Pandya (Code Review) 24 Jan '24

24 Jan '24

Varshit Pandya has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/80161?usp=email ) Change subject: soc/amd/common: Fix typo ...................................................................... soc/amd/common: Fix typo Change-Id: Ida6e87908ae6996529057c8df12dbe046ee54b98 Signed-off-by: Varshit Pandya <pandyavarshit(a)gmail.com> --- M src/soc/amd/common/fsp/pci/pcie_clk_req.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/80161/1 diff --git a/src/soc/amd/common/fsp/pci/pcie_clk_req.c b/src/soc/amd/common/fsp/pci/pcie_clk_req.c index 881c650..a16b0de 100644 --- a/src/soc/amd/common/fsp/pci/pcie_clk_req.c +++ b/src/soc/amd/common/fsp/pci/pcie_clk_req.c @@ -7,7 +7,7 @@ #include <device/pci_ids.h> #include <soc/platform_descriptors.h> -/* Update gpp glk req config based on DXIO descriptors and enabled devices. */ +/* Update gpp clk req config based on DXIO descriptors and enabled devices. */ void pcie_gpp_dxio_update_clk_req_config(enum gpp_clk_req *gpp_clk_config, size_t gpp_clk_config_num) { -- To view, visit https://review.coreboot.org/c/coreboot/+/80161?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ida6e87908ae6996529057c8df12dbe046ee54b98 Gerrit-Change-Number: 80161 Gerrit-PatchSet: 1 Gerrit-Owner: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[main]: mb/google/brya/var/*: Use name 'LCD0' for internal panel output
by Matt DeVillier (Code Review) 24 Jan '24

24 Jan '24

Attention is currently required from: Eric Lai, Kapil Porwal, Nick Vaccaro, Subrata Banik, Won Chung. Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80174?usp=email ) Change subject: mb/google/brya/var/*: Use name 'LCD0' for internal panel output ...................................................................... Patch Set 3: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/80174/comment/53b4c3e9_1d9fba90 : PS2, Line 9: The GMA driver generates the brightness controls expecting the name : LCD0 > not sure if the original code has added w/ the same intention. […] so, the GMA driver ACPI has two parts - the dsdt, which contains the brightness level list and the actual brightness control functions, and the ssdt which contains the panel definition as well as the methods which call the brightness functions in the DSDT. See: src/drivers/intel/gma/acpi/*.asl For boards which define a panel in devicetree (e.g., `register "gfx" = "GMA_DEFAULT_PANEL(0)"`), the SSDT will contain code scoped to SB.PCI0.GFX0 which includes a _DOD method, the LCD0 device, and the _BCL, _BCM, and _BCQ ACPI methods. The problem is, for boards which use drivers/gfx/generic, they also create a _DOD method in the scope of the display controller, which for Intel iGPUs is SB.PCI0.GFX0. This is not valid ACPI, and Windows chokes. My solution is to add the brightness control hooks to the drivers/gfx/generic code, and drop the panel definition in devicetree, so there is no duplicate _DOD method, and brightness controls still function properly. Renaming the panel device from LCD to LCD0 ensures the two halves are in sync. https://review.coreboot.org/c/coreboot/+/80174/comment/b093fa6d_b2a7ef08 : PS2, Line 14: functional. > no need warp. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/80174?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I389553b2ddc5b09d165229e2d8066cacf852b82c Gerrit-Change-Number: 80174 Gerrit-PatchSet: 3 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Won Chung <wonchung(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Eric Lai <ericllai(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Won Chung <wonchung(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Comment-Date: Wed, 24 Jan 2024 16:42:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Eric Lai <ericllai(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: mb/google/brya/var/*: Use name 'LCD0' for internal panel output
by Matt DeVillier (Code Review) 24 Jan '24

24 Jan '24

Attention is currently required from: Kapil Porwal, Matt DeVillier, Nick Vaccaro, Won Chung. Hello Eric Lai, Kapil Porwal, Nick Vaccaro, Subrata Banik, Won Chung, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/80174?usp=email to look at the new patch set (#3). Change subject: mb/google/brya/var/*: Use name 'LCD0' for internal panel output ...................................................................... mb/google/brya/var/*: Use name 'LCD0' for internal panel output The GMA driver generates the brightness controls expecting the name LCD0, so we need to use it here as well so that the DSDT and SSDT parts match. TEST=build/boot Win11 on google/brya (redrix), verify brightness controls are functional. Change-Id: I389553b2ddc5b09d165229e2d8066cacf852b82c Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M src/mainboard/google/brya/variants/anahera/overridetree.cb M src/mainboard/google/brya/variants/anahera4es/overridetree.cb M src/mainboard/google/brya/variants/anraggar/overridetree.cb M src/mainboard/google/brya/variants/banshee/overridetree.cb M src/mainboard/google/brya/variants/brya0/overridetree.cb M src/mainboard/google/brya/variants/crota/overridetree.cb M src/mainboard/google/brya/variants/dochi/overridetree.cb M src/mainboard/google/brya/variants/felwinter/overridetree.cb M src/mainboard/google/brya/variants/gimble/overridetree.cb M src/mainboard/google/brya/variants/gimble4es/overridetree.cb M src/mainboard/google/brya/variants/kano/overridetree.cb M src/mainboard/google/brya/variants/marasov/overridetree.cb M src/mainboard/google/brya/variants/mithrax/overridetree.cb M src/mainboard/google/brya/variants/omnigul/overridetree.cb M src/mainboard/google/brya/variants/osiris/overridetree.cb M src/mainboard/google/brya/variants/primus/overridetree.cb M src/mainboard/google/brya/variants/redrix/overridetree.cb M src/mainboard/google/brya/variants/redrix4es/overridetree.cb M src/mainboard/google/brya/variants/skolas/overridetree.cb M src/mainboard/google/brya/variants/skolas4es/overridetree.cb M src/mainboard/google/brya/variants/taeko/overridetree.cb M src/mainboard/google/brya/variants/taeko4es/overridetree.cb M src/mainboard/google/brya/variants/taniks/overridetree.cb M src/mainboard/google/brya/variants/vell/overridetree.cb M src/mainboard/google/brya/variants/volmar/overridetree.cb 25 files changed, 29 insertions(+), 29 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/80174/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/80174?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I389553b2ddc5b09d165229e2d8066cacf852b82c Gerrit-Change-Number: 80174 Gerrit-PatchSet: 3 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Won Chung <wonchung(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Won Chung <wonchung(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2024