Jonas Moehle has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
src/*.asl: Remove _HID / _ADR objects
A device object must contain either a _HID object or an _ADR object, but should not contain both. _HID objects are enumerated and have their drivers loaded by ACPI, whereas _ADR objects typically perform most functions without involving ACPI.
Untested.
Signed-off-by: Jonas Moehle ad-min@mailbox.org Change-Id: I949393558f5af66689c167b2e593a1461f641962 --- M src/mainboard/aopen/dxplplusu/acpi/p64h2.asl M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/intel/strago/acpi/mainboard.asl M src/soc/intel/broadwell/acpi/serialio.asl M src/southbridge/intel/lynxpoint/acpi/serialio.asl 5 files changed, 17 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/37935/1
diff --git a/src/mainboard/aopen/dxplplusu/acpi/p64h2.asl b/src/mainboard/aopen/dxplplusu/acpi/p64h2.asl index e3f2e5f..ccaa6e3 100644 --- a/src/mainboard/aopen/dxplplusu/acpi/p64h2.asl +++ b/src/mainboard/aopen/dxplplusu/acpi/p64h2.asl @@ -18,7 +18,6 @@ /* I/O APIC id 0x3 */ Device(PBIO) { - Name (_HID, "ACPI000A") Name (_ADR, 0x001c0000) }
@@ -59,7 +58,6 @@ /* I/O APIC id 0x4 */ Device(PAIO) { - Name (_HID, "ACPI000A") Name (_ADR, 0x001e0000) }
diff --git a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl index 01942dc..9b88cdd 100644 --- a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl +++ b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl @@ -18,7 +18,6 @@ /* Grunt specific I2S machine driver */ Device (I2S) { - Name (_ADR, 1) Name (_HID, "AMD7219") Name (_CID, "AMD7219")
diff --git a/src/mainboard/intel/strago/acpi/mainboard.asl b/src/mainboard/intel/strago/acpi/mainboard.asl index 1d5437b..caaae86 100644 --- a/src/mainboard/intel/strago/acpi/mainboard.asl +++ b/src/mainboard/intel/strago/acpi/mainboard.asl @@ -114,7 +114,7 @@ /* Realtek Audio Codec */ Device (RTEK) /* Audio Codec driver I2C */ { - Name (_ADR, 0) + Name (_HID, AUDIO_CODEC_HID) Name (_CID, AUDIO_CODEC_CID) Name (_DDN, AUDIO_CODEC_DDN) diff --git a/src/soc/intel/broadwell/acpi/serialio.asl b/src/soc/intel/broadwell/acpi/serialio.asl index 1b44e95..08311aa 100644 --- a/src/soc/intel/broadwell/acpi/serialio.asl +++ b/src/soc/intel/broadwell/acpi/serialio.asl @@ -157,9 +157,9 @@ Device (SDMA) { // Serial IO DMA Controller + Name (_HID, "INTL9C60") Name (_UID, 1) - Name (_ADR, 0x00150000)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -194,6 +194,7 @@ Device (I2C0) { // Serial IO I2C0 Controller + Method (_HID) { If (\ISWP ()) { @@ -205,7 +206,6 @@ Return ("INT33C2") } Name (_UID, 1) - Name (_ADR, 0x00150001)
Name (SSCN, Package () { 432, 507, 30 }) Name (FMCN, Package () { 72, 160, 30 }) @@ -265,6 +265,7 @@ Device (I2C1) { // Serial IO I2C1 Controller + Method (_HID) { If (\ISWP ()) { @@ -276,7 +277,6 @@ Return ("INT33C3") } Name (_UID, 1) - Name (_ADR, 0x00150002)
Name (SSCN, Package () { 432, 507, 30 }) Name (FMCN, Package () { 72, 160, 30 }) @@ -336,6 +336,7 @@ Device (SPI0) { // Serial IO SPI0 Controller + Method (_HID) { If (\ISWP ()) { @@ -347,7 +348,6 @@ Return ("INT33C0") } Name (_UID, 1) - Name (_ADR, 0x00150003)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -392,6 +392,7 @@ Device (SPI1) { // Serial IO SPI1 Controller + Method (_HID) { If (\ISWP ()) { @@ -403,7 +404,6 @@ Return ("INT33C1") } Name (_UID, 1) - Name (_ADR, 0x00150004)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -460,6 +460,7 @@ Device (UAR0) { // Serial IO UART0 Controller + Method (_HID) { If (\ISWP ()) { @@ -471,7 +472,6 @@ Return ("INT33C4") } Name (_UID, 1) - Name (_ADR, 0x00150005)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -528,6 +528,7 @@ Device (UAR1) { // Serial IO UART1 Controller + Method (_HID) { If (\ISWP ()) { @@ -539,7 +540,6 @@ Return ("INT33C5") } Name (_UID, 1) - Name (_ADR, 0x00150006)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -584,6 +584,7 @@ Device (SDIO) { // Serial IO SDIO Controller + Method (_HID) { If (\ISWP ()) { @@ -596,7 +597,6 @@ } Name (_CID, "PNP0D40") Name (_UID, 1) - Name (_ADR, 0x00170000)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () diff --git a/src/southbridge/intel/lynxpoint/acpi/serialio.asl b/src/southbridge/intel/lynxpoint/acpi/serialio.asl index 9323b91..e33f048 100644 --- a/src/southbridge/intel/lynxpoint/acpi/serialio.asl +++ b/src/southbridge/intel/lynxpoint/acpi/serialio.asl @@ -123,9 +123,9 @@ Device (SDMA) { // Serial IO DMA Controller + Name (_HID, "INTL9C60") Name (_UID, 1) - Name (_ADR, 0x00150000)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -160,10 +160,10 @@ Device (I2C0) { // Serial IO I2C0 Controller + Name (_HID, "INT33C2") Name (_CID, "INT33C2") Name (_UID, 1) - Name (_ADR, 0x00150001)
Name (SSCN, Package () { 432, 507, 30 }) Name (FMCN, Package () { 72, 160, 30 }) @@ -242,10 +242,10 @@ Device (I2C1) { // Serial IO I2C1 Controller + Name (_HID, "INT33C3") Name (_CID, "INT33C3") Name (_UID, 1) - Name (_ADR, 0x00150002)
Name (SSCN, Package () { 432, 507, 30 }) Name (FMCN, Package () { 72, 160, 30 }) @@ -324,10 +324,10 @@ Device (SPI0) { // Serial IO SPI0 Controller + Name (_HID, "INT33C0") Name (_CID, "INT33C0") Name (_UID, 1) - Name (_ADR, 0x00150003)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -362,10 +362,10 @@ Device (SPI1) { // Serial IO SPI1 Controller + Name (_HID, "INT33C1") Name (_CID, "INT33C1") Name (_UID, 1) - Name (_ADR, 0x00150004)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -413,10 +413,10 @@ Device (UAR0) { // Serial IO UART0 Controller + Name (_HID, "INT33C4") Name (_CID, "INT33C4") Name (_UID, 1) - Name (_ADR, 0x00150005)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -464,10 +464,10 @@ Device (UAR1) { // Serial IO UART1 Controller + Name (_HID, "INT33C5") Name (_CID, "INT33C5") Name (_UID, 1) - Name (_ADR, 0x00150006)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate () @@ -502,10 +502,10 @@ Device (SDIO) { // Serial IO SDIO Controller + Name (_HID, "INT33C6") Name (_CID, "PNP0D40") Name (_UID, 1) - Name (_ADR, 0x00170000)
// BAR0 is assigned during PCI enumeration and saved into NVS Name (RBUF, ResourceTemplate ()
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 1: Code-Review+1
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/2//COMMIT_MSG@11 PS2, Line 11: Untested. This should be tested because at least on Broadwell, SerialIO devices can either be used in PCI mode or ACPI mode.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/2//COMMIT_MSG@11 PS2, Line 11: Untested.
This should be tested because at least on Broadwell, SerialIO devices can either be used in PCI mode […]
We can not use _HID and _ADR at the same time
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2: Code-Review-1
This is untested and no objective was given why one of _ADR or _HID was preferred over the other. It's unclear if there are OS that requires one of them to be present. On latest platforms those devices can be placed in ACPI or PCI mode. Making it even worse there seem to be auto-detection features: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
This is untested and no objective was given why one of _ADR or _HID was preferred over the other. It's unclear if there are OS that requires one of them to be present. On latest platforms those devices can be placed in ACPI or PCI mode. Making it even worse there seem to be auto-detection features: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
date Oct 24, 2014 : "The PCI config space is required for many "legacy" OSs"
so should we support Windows 3.11 :D ?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2:
Making it even worse there seem to be auto-detection features: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
ironically, something they never used. Google never shipped legacy boot capability for Baytrail, and depthcharge was never modified to put the eMMC (or anything else) into PCI mode when legacy booting.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2:
This is untested and no objective was given why one of _ADR or _HID was preferred over the other. It's unclear if there are OS that requires one of them to be present.
SerialIO devices are always in ACPI mode for these platforms on all boards in the tree, and _HID is required for ACPI mode, so makes sense to drop _ADR here
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2: Code-Review-2
Parts of this were just reverted because it's known to break things. If I wouldn't have to constantly police this topic, I might find the time to fix things properly one day. But when patches are pushed with the intention to break things just so that somebody else fixes them, I feel like I should wait a month or two until everybody has calmed down.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 2:
Jonas, sorry just realized that you weren't involved in the topic before. The angry words were targeting the reviewers that gave +2 here...
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 3: -Code-Review
Patch Set 2:
Jonas, sorry just realized that you weren't involved in the topic before. The angry words were targeting the reviewers that gave +2 here...
Nico, I'm fine to drop that +2 :) I'm also fine to drop all broken code.
So please feel free to review current patch. it may help to fix current issue.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 3:
Patch Set 2:
This is untested and no objective was given why one of _ADR or _HID was preferred over the other. It's unclear if there are OS that requires one of them to be present.
SerialIO devices are always in ACPI mode for these platforms on all boards in the tree, and _HID is required for ACPI mode, so makes sense to drop _ADR here
Yes this is correct these were always just put in ACPI mode on baytrail. This is something where Intel went back and forth on and eventually settled PCI mode in future chipsets.
I tried to support both modes in coreboot (which is why it had both _HID and _ADR) but it may be easier to settle on one path, or put this behind a Kconfig variable.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 3:
(4 comments)
This is untested and no objective was given why one of _ADR or _HID was preferred over the other. It's unclear if there are OS that requires one of them to be present.
SerialIO devices are always in ACPI mode for these platforms on all boards in the tree, and _HID is required for ACPI mode, so makes sense to drop _ADR here
grep says no.
Yes this is correct these were always just put in ACPI mode on baytrail. This is something where Intel went back and forth on and eventually settled PCI mode in future chipsets.
I tried to support both modes in coreboot (which is why it had both _HID and _ADR) but it may be easier to settle on one path, or put this behind a Kconfig variable.
We just discovered something related a few days ago: There's a chromium kernel driver that expects a specific setting per board. So if upstream coreboot differs from the original firmware, the driver fails. Settling on one path (_HID seems to be the only choice because of Windows compatibility) might be a good idea, but we have to keep in mind that it affects the whole ecosystem.
If we want a quick solution (some people seem anxious to update iasl), I'd prefer that we put the correct method into an SSDT for now.
https://review.coreboot.org/c/coreboot/+/37935/3/src/mainboard/aopen/dxplplu... File src/mainboard/aopen/dxplplusu/acpi/p64h2.asl:
PS3: Changes here seem correct (albeit, the device nodes seem dead anyway?). Checked that the PCI devices show up in a kernel log in board-status.
https://review.coreboot.org/c/coreboot/+/37935/3/src/mainboard/google/kahlee... File src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl:
PS3: Device is not on any bus at all, so this seems right.
https://review.coreboot.org/c/coreboot/+/37935/3/src/mainboard/intel/strago/... File src/mainboard/intel/strago/acpi/mainboard.asl:
PS3: I2C is generally not considered a probed bus, so this should be correct too.
https://review.coreboot.org/c/coreboot/+/37935/3/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/serialio.asl:
PS3: See CB:36318, CB:37710.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/3/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/serialio.asl:
PS3:
See CB:36318, CB:37710.
all broadwell boards with i2c devices have sio_acpi_mode set to 1. It could be dropped and made universal with no loss of functionality, and then we could remove the _ADR entries without issue
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/3/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/serialio.asl:
PS3:
all broadwell boards with i2c devices have sio_acpi_mode set to 1. […]
Ah, "with i2c devices" made me look again. For the boards that set `sio_acpi_mode = 0` those lines are redundant twice indeed, they don't have SIO devices...
Would be a shame to loose functionality (there are more OSes out there than Windows and Linux), but I wouldn't object if you want to drop `sio_acpi_mode`.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/3/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/serialio.asl:
PS3:
Ah, "with i2c devices" made me look again. For the boards that set […]
CB:37568 uses `sio_acpi_mode = 0`.
HAOUAS Elyes has uploaded a new patch set (#4) to the change originally created by Jonas Moehle. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
src/*.asl: Remove _HID / _ADR objects
A device object must contain either a _HID object or an _ADR object, but should not contain both. _HID objects are enumerated and have their drivers loaded by ACPI, whereas _ADR objects typically perform most functions without involving ACPI.
Untested.
Signed-off-by: Jonas Moehle ad-min@mailbox.org Change-Id: I949393558f5af66689c167b2e593a1461f641962 --- M src/mainboard/aopen/dxplplusu/acpi/p64h2.asl M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/intel/strago/acpi/mainboard.asl 3 files changed, 1 insertion(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/37935/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/5//COMMIT_MSG@9 PS5, Line 9: A device object must contain either a _HID object or an _ADR object, but should not contain both. _HID objects are enumerated and have their drivers loaded by ACPI, whereas _ADR objects typically perform most functions without involving ACPI. Please break lines before 72 chars. I'm also not sure about the last part: _ADR links the ASL node to the OS-enumerated device, that's all. It doesn't imply anything about what the ASL code does with the device.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 5:
(1 comment)
Thx
https://review.coreboot.org/c/coreboot/+/37935/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/5//COMMIT_MSG@9 PS5, Line 9: A device object must contain either a _HID object or an _ADR object, but should not contain both. _HID objects are enumerated and have their drivers loaded by ACPI, whereas _ADR objects typically perform most functions without involving ACPI.
Please break lines before 72 chars. I'm also not sure about the […]
Done
HAOUAS Elyes has uploaded a new patch set (#6) to the change originally created by Jonas Moehle. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/ainboard/*.asl: Remove _HID / _ADR objects ......................................................................
src/ainboard/*.asl: Remove _HID / _ADR objects
A device object must contain either a _HID object or an _ADR object, but should not contain both.
Untested.
Signed-off-by: Jonas Moehle ad-min@mailbox.org Change-Id: I949393558f5af66689c167b2e593a1461f641962 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/mainboard/aopen/dxplplusu/acpi/p64h2.asl M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/intel/strago/acpi/mainboard.asl 3 files changed, 1 insertion(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/37935/6
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/ainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/6//COMMIT_MSG@7 PS6, Line 7: ainboard lost an 'm'
HAOUAS Elyes has uploaded a new patch set (#7) to the change originally created by Jonas Moehle. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
src/mainboard/*.asl: Remove _HID / _ADR objects
A device object must contain either a _HID object or an _ADR object, but should not contain both.
Untested.
Signed-off-by: Jonas Moehle ad-min@mailbox.org Change-Id: I949393558f5af66689c167b2e593a1461f641962 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/mainboard/aopen/dxplplusu/acpi/p64h2.asl M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/intel/strago/acpi/mainboard.asl 3 files changed, 1 insertion(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/37935/7
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/6//COMMIT_MSG@7 PS6, Line 7: ainboard
lost an 'm'
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
ping :)
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@7 PS7, Line 7: Remove _HID / _ADR objects I'd add the term 'overlapping' (or similar) for clarity
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@9 PS7, Line 9: A device object must contain either a _HID object or an _ADR object, : but should not contain both. what's the logic for removing one vs the other in each case?
what's the impetus for this change/correction?
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37935/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/2//COMMIT_MSG@11 PS2, Line 11: Untested.
We can not use _HID and _ADR at the same time
Done
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@9 PS7, Line 9: A device object must contain either a _HID object or an _ADR object, : but should not contain both.
what's the logic for removing one vs the other in each case? […]
one of them is wrong. please see ACPI Specification Version 6.3 - January 2019 page 342
https://review.coreboot.org/c/coreboot/+/37935/3/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/serialio.asl:
PS3:
CB:37568 uses `sio_acpi_mode = 0`.
Done
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@9 PS7, Line 9: A device object must contain either a _HID object or an _ADR object, : but should not contain both.
one of them is wrong. […]
I understand this change is to be ACPI 6.3 spec compliant (though that should probably be in the commit msg, not just the comments here), just looking for an explanation as to why the _HID was removed in the case of the Aopen board, vs the _ADR
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@9 PS7, Line 9: A device object must contain either a _HID object or an _ADR object, : but should not contain both.
I understand this change is to be ACPI 6. […]
It's not about 6.3 in particular. I don't know when that rule (to have only one of them) was established, but newer iasl warns about it.
If a bus is enumerated by the OS, like PCI, one is supposed to provide an _ADR so that the device nodes in ACPI can be mapped to the devices the OS discovered. The OS already knows the type of the device and what driver to attach.
If a bus is not enumerated by the OS, existence of the devices is assumed (depending on _STA) and the type is given by _HID.
The devices of the Aopen board are PCI devices, hence _ADR has to be kept. It seems purely cosmetic anyway, as these device nodes contain no information and are not referenced anywhere (I guess one could just remove them).
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@9 PS7, Line 9: A device object must contain either a _HID object or an _ADR object, : but should not contain both.
It's not about 6.3 in particular. I don't know when that rule (to have only […]
Looked it up, 6.0 changed "can contain both" to "should not contain both". Again, it's all about a tool that throws a meaningless warning at us, no idea if we can't just disable the warning?
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@9 PS7, Line 9: A device object must contain either a _HID object or an _ADR object, : but should not contain both.
Looked it up, 6.0 changed "can contain both" to "should not contain both". […]
disabling the warning isn't a good idea. when we'll switch to upper acpi version, we will have too many to fix ...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects ......................................................................
Patch Set 7:
(1 comment)
Jonas, can you please update the commit summary as Matt suggested? Then we can merge this.
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@9 PS7, Line 9: A device object must contain either a _HID object or an _ADR object, : but should not contain both.
disabling the warning isn't a good idea. […]
Currently, it says "should not", not "must not". And I find it hard to predict if that will ever change again.
HAOUAS Elyes has uploaded a new patch set (#9) to the change originally created by Jonas Moehle. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects overlapping ......................................................................
src/mainboard/*.asl: Remove _HID / _ADR objects overlapping
ACPI spec: "A device object must contain either an _HID object or an _ADR object, but should not contain both."
Signed-off-by: Jonas Moehle ad-min@mailbox.org Change-Id: I949393558f5af66689c167b2e593a1461f641962 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/mainboard/aopen/dxplplusu/acpi/p64h2.asl M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/intel/strago/acpi/mainboard.asl 3 files changed, 1 insertion(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/37935/9
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects overlapping ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@7 PS7, Line 7: Remove _HID / _ADR objects
I'd add the term 'overlapping' (or similar) for clarity
Done
https://review.coreboot.org/c/coreboot/+/37935/7//COMMIT_MSG@9 PS7, Line 9: A device object must contain either a _HID object or an _ADR object, : but should not contain both.
Currently, it says "should not", not "must not". And I find it hard to […]
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects overlapping ......................................................................
Patch Set 9: Code-Review+2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects overlapping ......................................................................
Patch Set 9: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37935 )
Change subject: src/mainboard/*.asl: Remove _HID / _ADR objects overlapping ......................................................................
src/mainboard/*.asl: Remove _HID / _ADR objects overlapping
ACPI spec: "A device object must contain either an _HID object or an _ADR object, but should not contain both."
Signed-off-by: Jonas Moehle ad-min@mailbox.org Change-Id: I949393558f5af66689c167b2e593a1461f641962 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr Reviewed-on: https://review.coreboot.org/c/coreboot/+/37935 Reviewed-by: Matt DeVillier matt.devillier@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/aopen/dxplplusu/acpi/p64h2.asl M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/intel/strago/acpi/mainboard.asl 3 files changed, 1 insertion(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified HAOUAS Elyes: Looks good to me, approved Matt DeVillier: Looks good to me, approved
diff --git a/src/mainboard/aopen/dxplplusu/acpi/p64h2.asl b/src/mainboard/aopen/dxplplusu/acpi/p64h2.asl index e3f2e5f..ccaa6e3 100644 --- a/src/mainboard/aopen/dxplplusu/acpi/p64h2.asl +++ b/src/mainboard/aopen/dxplplusu/acpi/p64h2.asl @@ -18,7 +18,6 @@ /* I/O APIC id 0x3 */ Device(PBIO) { - Name (_HID, "ACPI000A") Name (_ADR, 0x001c0000) }
@@ -59,7 +58,6 @@ /* I/O APIC id 0x4 */ Device(PAIO) { - Name (_HID, "ACPI000A") Name (_ADR, 0x001e0000) }
diff --git a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl index 01942dc..9b88cdd 100644 --- a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl +++ b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/acpi/audio.asl @@ -18,7 +18,6 @@ /* Grunt specific I2S machine driver */ Device (I2S) { - Name (_ADR, 1) Name (_HID, "AMD7219") Name (_CID, "AMD7219")
diff --git a/src/mainboard/intel/strago/acpi/mainboard.asl b/src/mainboard/intel/strago/acpi/mainboard.asl index 1d5437b..caaae86 100644 --- a/src/mainboard/intel/strago/acpi/mainboard.asl +++ b/src/mainboard/intel/strago/acpi/mainboard.asl @@ -114,7 +114,7 @@ /* Realtek Audio Codec */ Device (RTEK) /* Audio Codec driver I2C */ { - Name (_ADR, 0) + Name (_HID, AUDIO_CODEC_HID) Name (_CID, AUDIO_CODEC_CID) Name (_DDN, AUDIO_CODEC_DDN)
-
Angel Pons (Code Review) -
Duncan Laurie (Code Review)
-
HAOUAS Elyes (Code Review)
-
Jonas Moehle (Code Review)
-
Martin Roth (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)