Attention is currently required from: Nico Huber, Paul Menzel, Arthur Heymans.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63982 )
Change subject: soc/intel/cmn/spi: Add ACPI SSDT extension for fast SPI
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63982/comment/e353a9ac_0dd33e94
PS1, Line 16: controller inaccessible from the OS.
> Sure, can do.
Ack
--
To view, visit https://review.coreboot.org/c/coreboot/+/63982
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifa89cdf41f42d4df5b46f095e22924157d9f3c3f
Gerrit-Change-Number: 63982
Gerrit-PatchSet: 2
Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 03 May 2022 08:26:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Paul Menzel, Arthur Heymans.
Hello build bot (Jenkins), Nico Huber, Arthur Heymans,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/63982
to look at the new patch set (#2).
Change subject: soc/intel/cmn/spi: Add ACPI SSDT extension for fast SPI
......................................................................
soc/intel/cmn/spi: Add ACPI SSDT extension for fast SPI
On Apollo Lake the fast SPI controller is located on a multi-function
PCI device which is hidden after coreboot passes over to the payload.
This makes it impossible for the OS to probe the PCI device and
therefore the OS is not aware of the resources the SPI controller has
occupied. In some circumstances it is possible that the OS moves other
PCI resources around and therefore a conflict can be introduced where
the moved resource will shadow the fast SPI BAR. This will make the SPI
controller inaccessible from the OS. As a consequence of this flashrom
is not able to access the SPI flash device.
On current master the siemens mainboard mc_apl4 is affected by this
issue and all other Apollo Lake based boards are potentially affected,
too. This patch adds a SSDT extension to the common SPI driver which for
now is only handling the fast SPI controller of Apollo Lake. It reports
the BAR0 resource of the fast SPI controller via ACPI to the OS. Since
there is no defined ACPI ID for the fast SPI controller of Apollo Lake
available now, the generic one (PNP0C02) is used.
Test: Boot mc_apl4 and make sure flashrom works again.
Change-Id: Ifa89cdf41f42d4df5b46f095e22924157d9f3c3f
Signed-off-by: Werner Zeh <werner.zeh(a)siemens.com>
---
M src/soc/intel/common/block/spi/spi.c
1 file changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/63982/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/63982
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifa89cdf41f42d4df5b46f095e22924157d9f3c3f
Gerrit-Change-Number: 63982
Gerrit-PatchSet: 2
Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newpatchset
Attention is currently required from: Tim Wawrzynczak.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63178 )
Change subject: mb/google/brya: Make __weak usage consistent
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Patchset:
PS2:
> ( git grep '__weak void' )
I think Tim is just making sure the usage is consistent within the same file. We don't have any guidelines on whether `__weak <ret type>` or `<ret type> __weak` is preferred.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63178
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic59cccdf0fb88fc71a440170ee40b73dd8736a33
Gerrit-Change-Number: 63178
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Tue, 03 May 2022 08:26:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Martin L Roth, Angel Pons, Arthur Heymans.
Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63639 )
Change subject: Add SBOM (Software Bill of Materials) Generation
......................................................................
Patch Set 13:
(13 comments)
This change is ready for review.
Patchset:
PS13:
Now I added the 'goswid' tool (https://github.com/9elements/goswid) as static util program (not the git repo itself). That one still needs some polishing, which I will do in the near future. But for now it does what it is supposed to do, which is converting the json SWID format into a binary format which is smaller and optionally compressed.
File src/sbom/payload-BOOTBOOT.json.src:
https://review.coreboot.org/c/coreboot/+/63639/comment/a4cf7af7_d97d08fa
PS12, Line 6: BootBoot
> Isn't the name all-uppercase? BOOTBOOT
Done
https://review.coreboot.org/c/coreboot/+/63639/comment/1db0efa2_7d551884
PS12, Line 12: BootBoot
> ditto
Done
File src/southbridge/intel/common/firmware/Kconfig:
https://review.coreboot.org/c/coreboot/+/63639/comment/37e169e8_e3544a89
PS12, Line 67: ME_SBOM
> nit: I'd call this `INCLUDE_ME_SBOM`
Done
https://review.coreboot.org/c/coreboot/+/63639/comment/fe2b27e5_224faa2d
PS12, Line 68: "Include SBOM file"
> This prompt text is too generic and can confuse people, as it's the only thing immediately shown to […]
Done
https://review.coreboot.org/c/coreboot/+/63639/comment/a795e999_b2fdd2d7
PS12, Line 70: default y
> Why is this enabled by default?
Didn't mean to enable it by default. At least not for now
https://review.coreboot.org/c/coreboot/+/63639/comment/3d0914cb_94addd30
PS12, Line 74: ME (Management Engine)
> I don't think it's necessary to explain what ME means in this file. […]
I tried to be more explicit, but I guess it's a bit redundant
https://review.coreboot.org/c/coreboot/+/63639/comment/5026017a_e0946679
PS12, Line 78: "Generate SBOM file"
> I'd try to make it clear that this option isn't great, maybe something like this? […]
you are right
I like yours better
https://review.coreboot.org/c/coreboot/+/63639/comment/ce31d067_c946c111
PS12, Line 90: "SBOM file path"
> How about: […]
Done
File util/goswid/cmd/main.go:
https://review.coreboot.org/c/coreboot/+/63639/comment/840cb240_e63fad93
PS12, Line 54: //if (output_format != uswid) && (len(input_file_paths) > 1) {
: // ErrorOut("multiple input files are only supported in conjunction with the .uswid output file extension\n")
: //}
> ?
since this corresponds to a different project/repo, I will make the changes there and pull them at a later time. There is still some polishing on that one left anyway.
https://review.coreboot.org/c/coreboot/+/63639/comment/ecb40b7b_0f87e1f1
PS12, Line 68: if if_len >= 5 && input_file_path[if_len-5:if_len] == ".json" {
: err = input_tag.FromJSON(input_file)
: } else if if_len >= 4 && input_file_path[if_len-4:if_len] == ".xml" {
: err = input_tag.FromXML(input_file)
: } else if if_len >= 5 && input_file_path[if_len-5:if_len] == ".cbor" {
: err = input_tag.FromCBOR(input_file)
: } else if if_len >= 6 && input_file_path[if_len-6:if_len] == ".uswid" {
: _, err = uswid_input_tag.FromUSWID(input_file)
: isUSWID = true
: } else {
: fmt.Printf("input file extension not recognized, assuming USWID: %s\n", input_file_path)
: _, err = uswid_input_tag.FromUSWID(input_file)
: isUSWID = true
: }
> Hmmm, why not split the filename using `. […]
I guess yours is a bit more explicit. Like the other one, I will make the changes on that project and pull the upstream in the near future.
File util/goswid/pkg/uswid/uswid.go:
https://review.coreboot.org/c/coreboot/+/63639/comment/fe04de7b_4b2dc491
PS12, Line 14: // can't be const...
> why?
to be honest I was kind of confused with this one. But it seems golang really doesn't allow a byte array to be const.
https://review.coreboot.org/c/coreboot/+/63639/comment/62c92c54_29b87e05
PS12, Line 32: 16
> Would be nice to avoid having magic offsets. Maybe define constants so that they have a name? […]
in my opinion this is the most explicit and readable version of doing protocol/encoding stuff. An abstraction would only make it worse (at least in my opinion)
--
To view, visit https://review.coreboot.org/c/coreboot/+/63639
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Icb7481d4903f95d200eddbfed7728fbec51819d0
Gerrit-Change-Number: 63639
Gerrit-PatchSet: 13
Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Martin L Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Christian Walter <christian.walter(a)9elements.com>
Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin L Roth <martinroth(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 03 May 2022 08:25:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Paul Menzel, Arthur Heymans.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63982 )
Change subject: soc/intel/cmn/spi: Add ACPI SSDT extension for fast SPI
......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63982/comment/ab1cda0d_c3b5eeea
PS1, Line 7: soc/intel/cmn/spi: Add ACPI SSDT extension for fast SPI
> Mention APL somewhere in here?
Not sure if mentioning APL in the headline is helpful. Without the description the scope could be get wrong. Do you mind to not add APL here?
https://review.coreboot.org/c/coreboot/+/63982/comment/e01ca61a_fe740a38
PS1, Line 16: controller inaccessible from the OS.
> Mention that flashrom is one of the affected use cases?
Sure, can do.
--
To view, visit https://review.coreboot.org/c/coreboot/+/63982
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ifa89cdf41f42d4df5b46f095e22924157d9f3c3f
Gerrit-Change-Number: 63982
Gerrit-PatchSet: 1
Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 03 May 2022 08:20:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Attention is currently required from: Terry Chen.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63955 )
Change subject: mb/google/brya/var/crota: Enable webcam power
......................................................................
Patch Set 3:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63955/comment/849d3b75_9d65d97b
PS3, Line 9: - set GPP_D16 to enable webcam power
No bullet point needed. Maybe also mention schematics.
https://review.coreboot.org/c/coreboot/+/63955/comment/44b68c40_aa29b2d3
PS3, Line 13: TEST=build and boot into kernel v5.10
… and notice log: …
--
To view, visit https://review.coreboot.org/c/coreboot/+/63955
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I01c73006d24b00be348655334232bea5eeb312e4
Gerrit-Change-Number: 63955
Gerrit-PatchSet: 3
Gerrit-Owner: Terry Chen <terry_chen(a)wistron.corp-partner.google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Terry Chen <terry_chen(a)wistron.corp-partner.google.com>
Gerrit-Comment-Date: Tue, 03 May 2022 08:01:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Jon Murphy, Karthik Ramasubramanian.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64004 )
Change subject: mb/google/skyrim/var/skyrim: Add USB WWAN configuration
......................................................................
Patch Set 1:
(1 comment)
File src/mainboard/google/skyrim/variants/skyrim/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/64004/comment/b1b52d7d_7393a322
PS1, Line 54: register "reset_off_delay_ms" = "20"
Please mention the source of the values in the commit message.
--
To view, visit https://review.coreboot.org/c/coreboot/+/64004
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I39f8e7204e31d9a4d093aacd838a18e6d2f44970
Gerrit-Change-Number: 64004
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.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-Comment-Date: Tue, 03 May 2022 07:58:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Tim Wawrzynczak, Jon Murphy, Karthik Ramasubramanian.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63968 )
Change subject: drivers/usb: Add chip driver for VL822 USB hub
......................................................................
Patch Set 2:
(3 comments)
Patchset:
PS2:
What datasheet did you ues?
File src/drivers/usb/vl/acpi_vl822.c:
https://review.coreboot.org/c/coreboot/+/63968/comment/f9ae4209_904dc6e0
PS2, Line 78: .read_resources = noop_read_resources,
: .set_resources = noop_set_resources,
: .scan_bus = scan_static_bus,
: .acpi_fill_ssdt = usb_vl822_acpi_fill_ssdt,
: .acpi_name = usb_vl822_acpi_name
I think in other files, tabulators are used to align the equal signs.
https://review.coreboot.org/c/coreboot/+/63968/comment/9e12247d_67ebb451
PS2, Line 91: ViaLabs
Via Labs
--
To view, visit https://review.coreboot.org/c/coreboot/+/63968
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I11d7ccc42d3dce8e136eb771f120825980e5c027
Gerrit-Change-Number: 63968
Gerrit-PatchSet: 2
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Tue, 03 May 2022 07:56:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment