Attention is currently required from: Furquan Shaikh, Martin Roth, Duncan Laurie.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52747 )
Change subject: util/sconfig: Add support for discontiguous FW_CONFIG fields
......................................................................
Patch Set 4:
(1 comment)
File Documentation/lib/fw_config.md:
https://review.coreboot.org/c/coreboot/+/52747/comment/fd7c3663_616e2957
PS4, Line 160:
I think when Markdown is rendered tabs are only 4 spaces wide...
--
To view, visit https://review.coreboot.org/c/coreboot/+/52747
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5ed76706347ee9642198efc77139abdc3af1b8a6
Gerrit-Change-Number: 52747
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Duncan Laurie <duncan(a)iceblink.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Duncan Laurie <duncan(a)iceblink.org>
Gerrit-Comment-Date: Thu, 29 Apr 2021 20:02:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Martin Roth, Duncan Laurie.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52747 )
Change subject: util/sconfig: Add support for discontiguous FW_CONFIG fields
......................................................................
Patch Set 4:
(3 comments)
Patchset:
PS3:
> Please update Documentation/lib/fw_config. […]
Ack.
File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/52747/comment/92ffc101_823ac185
PS3, Line 598: ((1ull << (1ull + bits->end_bit - bits->start_bit)) - 1ull);
> could pull this into a function that takes fw_config_field_bits since it is used in multiple places […]
Done
File util/sconfig/sconfig.h:
https://review.coreboot.org/c/coreboot/+/52747/comment/a3fffaa3_eca8c48a
PS3, Line 230: append_bits
> I would suggest adding fw_config in the name to match all the other fw_config callbacks. […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/52747
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5ed76706347ee9642198efc77139abdc3af1b8a6
Gerrit-Change-Number: 52747
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Duncan Laurie <duncan(a)iceblink.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Duncan Laurie <duncan(a)iceblink.org>
Gerrit-Comment-Date: Thu, 29 Apr 2021 20:01:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Duncan Laurie <duncan(a)iceblink.org>
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Martin Roth, Tim Wawrzynczak.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/52747
to look at the new patch set (#4).
Change subject: util/sconfig: Add support for discontiguous FW_CONFIG fields
......................................................................
util/sconfig: Add support for discontiguous FW_CONFIG fields
Sooner or later, some board was going to need extra FW_CONFIG bits for
a field that was already in production, so this patch adds support for
adding extra (unused) bits to a field.
The extra are appended via a syntax like:
`field FIELD_NAME START0 END0 | START1 END1 | START2 END2 ...`
and the suffixed bits are all treated as if they are contiguous when
defining option values.
BUG=b:185190978
TEST=Modified volteer fw_config to the following:
field AUDIO 8 10 | 29 29 | 31 31
option NONE 0
option MAX98357_ALC5682I_I2S 1
option MAX98373_ALC5682I_I2S 2
option MAX98373_ALC5682_SNDW 3
option MAX98373_ALC5682I_I2S_UP4 4
option MAX98360_ALC5682I_I2S 5
option RT1011_ALC5682I_I2S 6
option AUDIO_FOO 7
option AUDIO_BAR 8
option AUDIO_QUUX 9
option AUDIO_BLAH1 10
option AUDIO_BLAH2 15
option AUDIO_BLAH3 16
option AUDIO_BLAH4 31
end
which yielded (in static_fw_config.h):
FW_CONFIG_FIELD_AUDIO_MASK 0xa0000700
FW_CONFIG_FIELD_AUDIO_OPTION_NONE_VALUE 0x0
FW_CONFIG_FIELD_AUDIO_OPTION_MAX98357_ALC5682I_I2S_VALUE 0x100
FW_CONFIG_FIELD_AUDIO_OPTION_MAX98373_ALC5682I_I2S_VALUE 0x200
FW_CONFIG_FIELD_AUDIO_OPTION_MAX98373_ALC5682_SNDW_VALUE 0x300
FW_CONFIG_FIELD_AUDIO_OPTION_MAX98373_ALC5682I_I2S_UP4_VALUE 0x400
FW_CONFIG_FIELD_AUDIO_OPTION_MAX98360_ALC5682I_I2S_VALUE 0x500
FW_CONFIG_FIELD_AUDIO_OPTION_RT1011_ALC5682I_I2S_VALUE 0x600
FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_FOO_VALUE 0x700
FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_BAR_VALUE 0x20000000
FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_QUUX_VALUE 0x20000100
FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_BLAH1_VALUE 0x20000200
FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_BLAH2_VALUE 0x20000700
FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_BLAH3_VALUE 0x80000000
FW_CONFIG_FIELD_AUDIO_OPTION_AUDIO_BLAH4_VALUE 0xa0000700
Change-Id: I5ed76706347ee9642198efc77139abdc3af1b8a6
Signed-off-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
---
M Documentation/lib/fw_config.md
M util/sconfig/lex.yy.c_shipped
M util/sconfig/main.c
M util/sconfig/sconfig.h
M util/sconfig/sconfig.l
M util/sconfig/sconfig.tab.c_shipped
M util/sconfig/sconfig.tab.h_shipped
M util/sconfig/sconfig.y
8 files changed, 537 insertions(+), 361 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/52747/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/52747
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5ed76706347ee9642198efc77139abdc3af1b8a6
Gerrit-Change-Number: 52747
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Duncan Laurie <duncan(a)iceblink.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Tim Wawrzynczak, Nick Vaccaro, Patrick Rudolph.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52765 )
Change subject: device: Switch pci_dev_is_wake_source to take pci_devfn_t
......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/52765/comment/ee4c1df1_4d705a87
PS1, Line 15: entire devicetree from SMM (only usage left
: is when disabling HECI via SMM)
This change seems fine. But, we really need to work on eliminating the tree topology (links) for non-ramstage stages. We can make use of aliases to skip walking the device tree. This will allow us to pull in only relevant parts of the device tree into any stage. We then don't have to worry about per-stage optimizations (I had to do something similar for bootblock some time back: https://review.coreboot.org/q/hashtag:%22dt-bootblock-simplification%22+(st…)
File src/soc/intel/alderlake/elog.c:
https://review.coreboot.org/c/coreboot/+/52765/comment/794a9a40_633a76c8
PS1, Line 51: pci_dev_is_wake_source(pme_map[i].devfn)
I don't think this usage is correct. Unfortunately, the `devfn` is overloaded in coreboot and this is not really the same as pci_devfn_t, even though line 15 says that it is (That is another thing to fix here).
1. Line 15 `devfn` should really be `unsigned int` as it expects `PCI_DEVFN()` format
2. `pci_dev_is_wake_source()` expects `pci_devfn_t` which would be something like `PCI_DEV(0, PCI_SLOT(pme_map[i].devfn), PCI_FUNC(pme_map[i].devfn))`
--
To view, visit https://review.coreboot.org/c/coreboot/+/52765
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4c39e5188321c8711d6479b15065e5aaedad8f38
Gerrit-Change-Number: 52765
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 29 Apr 2021 19:48:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52766 )
Change subject: mb/google/guybrush: Add temporary _AIE table
......................................................................
Patch Set 1:
(2 comments)
File src/mainboard/google/guybrush/variants/baseboard/include/baseboard/ec.h:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-118064):
https://review.coreboot.org/c/coreboot/+/52766/comment/b818040e_b5cb9d81
PS1, Line 61: #define EC_ENABLE_WAKE_PIN Package () {\_SB.GPIO, 0}
space prohibited between function name and open parenthesis '('
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-118064):
https://review.coreboot.org/c/coreboot/+/52766/comment/9b74660d_4de69cf2
PS1, Line 61: #define EC_ENABLE_WAKE_PIN Package () {\_SB.GPIO, 0}
Macros with complex values should be enclosed in parentheses
--
To view, visit https://review.coreboot.org/c/coreboot/+/52766
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8508e5fd8f23a6a8364a8dbf919568d472cc9409
Gerrit-Change-Number: 52766
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Thu, 29 Apr 2021 19:24:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/52766 )
Change subject: mb/google/guybrush: Add temporary _AIE table
......................................................................
mb/google/guybrush: Add temporary _AIE table
On Cezanne, S0i3 wakes are all handled by the GPIO controller. When a
wake is asserted, the GPIO controller is the only subsystem that
registers the wake. There are no GEVENTs/GPEs raised. This means that we
can't use the GPE number in the _PRW package for two reasons:
1) The GPE will never get asserted, so the device will not be credited
as being the wake source.
2) The GPIO's wake_status bit will never get cleared. This means that on
a second S0i3 sleep, the system will be instantly woken up.
In order to solve these problems, we need to tell the OS which GPIOs can
be used to wake up the system. By providing the _AEI table we give the
OS the ability to
* Enable/disable if a GPIO is a wake source. This doesn't technically
work yet because the GPIO kernel driver had not yet implemented
irq_set_wake.
* Associate a GPIO wake with a device so it can correctly increment the
wake_source.
* Clear the wake_status bit from the GPIO.
This CL is currently a workaround. Coreboot doesn't yet have any
infrastructure for generating the _AEI table. The ACPI chip drivers
(drivers/i2c/generic, drivers/uart/acpi, etc) don't have the ability to
specify the _AEI index as a wake source either. This will all be
implemented in later CLs.
This CL only handles EC_SOC_WAKE_ODL. It allows testing S0i3 resume
using the internal keyboard, and the lid switch. Other devices will need
to be added as they are tested.
BUG=b:185621145
TEST=Multiple S0i3 cycles using internal keyboard
Signed-off-by: Raul E Rangel <rrangel(a)chromium.org>
Change-Id: I8508e5fd8f23a6a8364a8dbf919568d472cc9409
---
A src/mainboard/google/guybrush/aei.asl
M src/mainboard/google/guybrush/dsdt.asl
M src/mainboard/google/guybrush/variants/baseboard/include/baseboard/ec.h
3 files changed, 15 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/52766/1
diff --git a/src/mainboard/google/guybrush/aei.asl b/src/mainboard/google/guybrush/aei.asl
new file mode 100644
index 0000000..b35e087
--- /dev/null
+++ b/src/mainboard/google/guybrush/aei.asl
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+Scope (\_SB.GPIO) {
+ Name (_AEI, ResourceTemplate () {
+ GpioInt(Edge, ActiveLow, ExclusiveAndWake, PullNone, 0x0000, "\\_SB.GPIO", 0x00, ResourceConsumer, , ) {22} /* lpc_pme_l_agpio22 */
+ })
+
+ Method (_E16, 0, Serialized) {
+ Notify (\_SB_.PCI0.LPCB.EC0_.LID0, 0x02)
+ Notify (\_SB_.PCI0.LPCB.EC0_.CREC, 0x02)
+ }
+}
diff --git a/src/mainboard/google/guybrush/dsdt.asl b/src/mainboard/google/guybrush/dsdt.asl
index 9579ae3..5e34740 100644
--- a/src/mainboard/google/guybrush/dsdt.asl
+++ b/src/mainboard/google/guybrush/dsdt.asl
@@ -23,4 +23,6 @@
/* ACPI code for EC functions */
#include <ec/google/chromeec/acpi/ec.asl>
}
+
+ #include "aei.asl"
}
diff --git a/src/mainboard/google/guybrush/variants/baseboard/include/baseboard/ec.h b/src/mainboard/google/guybrush/variants/baseboard/include/baseboard/ec.h
index 0b51eb2..9b86293 100644
--- a/src/mainboard/google/guybrush/variants/baseboard/include/baseboard/ec.h
+++ b/src/mainboard/google/guybrush/variants/baseboard/include/baseboard/ec.h
@@ -58,7 +58,7 @@
/* Enable LID switch and provide wake pin for EC */
#define EC_ENABLE_LID_SWITCH
-#define EC_ENABLE_WAKE_PIN GEVENT_3 /* AGPIO 22 -> GPE 3 */
+#define EC_ENABLE_WAKE_PIN Package () {\_SB.GPIO, 0}
/* Enable Tablet switch */
#define EC_ENABLE_TBMC_DEVICE
--
To view, visit https://review.coreboot.org/c/coreboot/+/52766
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8508e5fd8f23a6a8364a8dbf919568d472cc9409
Gerrit-Change-Number: 52766
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-MessageType: newchange