Attention is currently required from: Marshall Dawson.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80148?usp=email )
Change subject: vc/amd/opensil/genoa_poc: move configure_mpio call to setup_opensil
......................................................................
Patch Set 1:
(1 comment)
File src/vendorcode/amd/opensil/genoa_poc/opensil.h:
https://review.coreboot.org/c/coreboot/+/80148/comment/3eabbbfe_01ea667b :
PS1, Line 14: configure_mpio
> Does this eventually get moved somewhere common?
the prototype can be moved to a more common location; the implementation probably not so easily. my plan here is to first improve the genoa_poc opensil integration and align it more with the fsp flow where it makes sense and then see what can be made common
--
To view, visit https://review.coreboot.org/c/coreboot/+/80148?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: If37077c879e266763fd2748a1a8d71c63c94729b
Gerrit-Change-Number: 80148
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Comment-Date: Wed, 24 Jan 2024 18:49:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-MessageType: comment
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
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
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
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
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
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
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