Change in coreboot[master]: soc/intel/broadwell: Comment conflicting use of _ADR and _HID

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Comment conflicting use of _ADR and _HID
......................................................................

soc/intel/broadwell: Comment conflicting use of _ADR and _HID

Change-Id: I3e892c451ee9d4e576b568f7efaad2e390524fe0
Signed-off-by: Elyes HAOUAS 
---
M src/soc/intel/broadwell/acpi/serialio.asl
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38350/1

diff --git a/src/soc/intel/broadwell/acpi/serialio.asl b/src/soc/intel/broadwell/acpi/serialio.asl
index 1b44e95..d6a763b 100644
--- a/src/soc/intel/broadwell/acpi/serialio.asl
+++ b/src/soc/intel/broadwell/acpi/serialio.asl
@@ -157,6 +157,7 @@
 Device (SDMA)
 {
    // Serial IO DMA Controller
+	// FIXME: Device object requires either a _HID or _ADR, but not both
    Name (_HID, "INTL9C60")
    Name (_UID, 1)
    Name (_ADR, 0x00150000)
@@ -194,6 +195,7 @@
 Device (I2C0)
 {
    // Serial IO I2C0 Controller
+	// FIXME: Device object requires either a _HID or _ADR, but not both
    Method (_HID)
    {
    	If (\ISWP ()) {
@@ -265,6 +267,7 @@
 Device (I2C1)
 {
    // Serial IO I2C1 Controller
+	// FIXME: Device object requires either a _HID or _ADR, but not both
    Method (_HID)
    {
    	If (\ISWP ()) {
@@ -336,6 +339,7 @@
 Device (SPI0)
 {
    // Serial IO SPI0 Controller
+	// FIXME: Device object requires either a _HID or _ADR, but not both
    Method (_HID)
    {
    	If (\ISWP ()) {
@@ -392,6 +396,7 @@
 Device (SPI1)
 {
    // Serial IO SPI1 Controller
+	// FIXME: Device object requires either a _HID or _ADR, but not both
    Method (_HID)
    {
    	If (\ISWP ()) {
@@ -460,6 +465,7 @@
 Device (UAR0)
 {
    // Serial IO UART0 Controller
+	// FIXME: Device object requires either a _HID or _ADR, but not both
    Method (_HID)
    {
    	If (\ISWP ()) {
@@ -528,6 +534,7 @@
 Device (UAR1)
 {
    // Serial IO UART1 Controller
+	// FIXME: Device object requires either a _HID or _ADR, but not both
    Method (_HID)
    {
    	If (\ISWP ()) {
@@ -584,6 +591,7 @@
 Device (SDIO)
 {
    // Serial IO SDIO Controller
+	// FIXME: Device object requires either a _HID or _ADR, but not both
    Method (_HID)
    {
    	If (\ISWP ()) {

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Comment conflicting use of _ADR and _HID
......................................................................

Patch Set 1:

(1 comment)

What's the purpose of this change? Just to mark as needing to be addressed for a future IASL toolchain update?

https://review.coreboot.org/c/coreboot/+/38350/1/src/soc/intel/broadwell/acp... 
File src/soc/intel/broadwell/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/38350/1/src/soc/intel/broadwell/acp... 
PS1, Line 198: 	// FIXME: Device object requires either a _HID or _ADR, but not both
I don't understand this and all subsequent markings. These devices only have an _HID entry, so there's no issue?

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Comment conflicting use of _ADR and _HID
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38350/1/src/soc/intel/broadwell/acp... 
File src/soc/intel/broadwell/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/38350/1/src/soc/intel/broadwell/acp... 
PS1, Line 198: 	// FIXME: Device object requires either a _HID or _ADR, but not both
> I don't understand this and all subsequent markings. […]

nevermind, gerrit was hiding them and I didn't look closely enough

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38350

to look at the new patch set (#2).

Change subject: soc/intel/broadwell: Comment conflicting use of _ADR and _HID
......................................................................

soc/intel/broadwell: Comment conflicting use of _ADR and _HID

To be compliant with ACPI specification, we need to solve conflicting
use of _ADR and _HID.

Change-Id: I3e892c451ee9d4e576b568f7efaad2e390524fe0
Signed-off-by: Elyes HAOUAS 
---
M src/soc/intel/broadwell/acpi/serialio.asl
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38350/2

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Comment conflicting use of _ADR and _HID
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38350/1/src/soc/intel/broadwell/acp... 
File src/soc/intel/broadwell/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/38350/1/src/soc/intel/broadwell/acp... 
PS1, Line 198: 	// FIXME: Device object requires either a _HID or _ADR, but not both
> nevermind, gerrit was hiding them and I didn't look closely enough

Done

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Comment conflicting use of _ADR and _HID
......................................................................

Patch Set 3:

ping

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38350

to look at the new patch set (#5).

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

soc/intel/broadwell: Don't use _ADR and _HID

Device object requires either a _HID or _ADR, but not both.

Change-Id: I3e892c451ee9d4e576b568f7efaad2e390524fe0
Signed-off-by: Elyes HAOUAS 
---
M src/soc/intel/broadwell/acpi/serialio.asl
1 file changed, 0 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38350/5

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 5: Code-Review-1

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38350

to look at the new patch set (#6).

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

soc/intel/broadwell: Don't use _ADR and _HID

To be compliant with ACPI specification, device object requires either
a _HID or _ADR, but not both.

Change-Id: I3e892c451ee9d4e576b568f7efaad2e390524fe0
Signed-off-by: Elyes HAOUAS 
---
M src/soc/intel/broadwell/acpi/serialio.asl
1 file changed, 0 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38350/6

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Removed Code-Review-1 by HAOUAS Elyes

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38350

to look at the new patch set (#7).

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

soc/intel/broadwell: Don't use _ADR and _HID

Device object requires either a _HID or _ADR, but not both.

Change-Id: I3e892c451ee9d4e576b568f7efaad2e390524fe0
Signed-off-by: Elyes HAOUAS 
---
M src/soc/intel/broadwell/acpi/serialio.asl
1 file changed, 0 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/38350/7

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 7: Code-Review-1

untested

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/38350/8/src/soc/intel/broadwell/acp... 
File src/soc/intel/broadwell/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/38350/8/src/soc/intel/broadwell/acp... 
PS8, Line 157: Device (SDMA)
My Broadwell laptop has the following code:

Scope (_SB.PCI0.SDMA)
    {
        Name (_HID, "INTL9C60" /* Intel Baytrail SOC DMA Controller */)  // _HID: Hardware ID
        Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
        {
            Return (LCRS (SMD0, SB00, SIR0))
        }

Method (_HRV, 0, NotSerialized)  // _HRV: Hardware Revision
        {
            Return (^^LPCB.CRID) /* _SB_.PCI0.LPCB.CRID */
        }
    }

Scope (_SB.PCI0.SDMA)
    {
        Name (_ADR, 0x00150000)  // _ADR: Address
    }

iasl does not complain about having both _HID and _ADR.

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Abandoned

see 38803

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has restored this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Restored

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Removed Code-Review-1 by HAOUAS Elyes

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8: Code-Review+2

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8:

If anyone disagrees with these patches, let's just ignore the warnings and move on.  Maybe something like this:

Makefile.inc:

# Ignore _HID & _ADR coexisting in Intel Lynxpoint and Broadwell ASL code.
 # See cb:38803 & cb:38802
 # "Multiple types (Device object requires either a _HID or _ADR, but not both)"
 MULTIPLE_TYPES_WARNING = 3073

ifeq ($(CONFIG_SOUTHBRIDGE_INTEL_LYNXPOINT)$(SOC_INTEL_BROADWELL),y)
 IGNORED_IASL_WARNINGS = -vw $(EMPTY_RESOURCE_TEMPLATE_WARNING) -vw $(REDUNDANT_OFFSET_REMARK) -vw $(MULTIPLE_TYPES_WARNING)
 else
 IGNORED_IASL_WARNINGS = -vw $(EMPTY_RESOURCE_TEMPLATE_WARNING) -vw $(REDUNDANT_OFFSET_REMARK)
 endif

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8:
> If anyone disagrees with these patches,

I do. It leaves the tree in an inconsistent state. Nothing changed
since this was reverted. We have discussed this in depth about three
times already. Many alternatives were proposed. IIRC, we even agreed
that the lynxpoint/broadwell code around _ADR (sio_acpi_mod = 0) can
be removed altogether.

I've also proposed to disable the warning (imho, it doesn't provide
any value, just a huge waste of time). Nobody liked that, though.

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8:
> Patch Set 8:
> > If anyone disagrees with these patches,
>
> I do. It leaves the tree in an inconsistent state. Nothing changed
> since this was reverted. We have discussed this in depth about three
> times already. Many alternatives were proposed. IIRC, we even agreed
> that the lynxpoint/broadwell code around _ADR (sio_acpi_mod = 0) can
> be removed altogether.
>
> I've also proposed to disable the warning (imho, it doesn't provide
> any value, just a huge waste of time). Nobody liked that, though.

Ok, So you're saying you agree with my suggested patch then?  Or do you care to write something different?

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8:
> > > If anyone disagrees with these patches,
> >
> > I do. It leaves the tree in an inconsistent state. Nothing changed
> > since this was reverted. We have discussed this in depth about three
> > times already. Many alternatives were proposed. IIRC, we even agreed
> > that the lynxpoint/broadwell code around _ADR (sio_acpi_mod = 0) can
> > be removed altogether.
> >
> > I've also proposed to disable the warning (imho, it doesn't provide
> > any value, just a huge waste of time). Nobody liked that, though.
>
> Ok, So you're saying you agree with my suggested patch then?  Or do you care to write something different?

I don't care anymore (as long as it's not leaving dead code). Spent too
much time on this already, and there is nothing to gain.

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8:

Patch:
https://review.coreboot.org/c/coreboot/+/38810

It allows us to update IASL:
https://qa.coreboot.org/job/coreboot_updated_iasl/647/

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8: Code-Review+1
> Patch Set 8:
> > > > If anyone disagrees with these patches,
> > >
> > > I do. It leaves the tree in an inconsistent state. Nothing changed
> > > since this was reverted. We have discussed this in depth about three
> > > times already. Many alternatives were proposed. IIRC, we even agreed
> > > that the lynxpoint/broadwell code around _ADR (sio_acpi_mod = 0) can
> > > be removed altogether.
> > >
> > > I've also proposed to disable the warning (imho, it doesn't provide
> > > any value, just a huge waste of time). Nobody liked that, though.
> >
> > Ok, So you're saying you agree with my suggested patch then?  Or do you care to write something different?
>
> I don't care anymore (as long as it's not leaving dead code). Spent too
> much time on this already, and there is nothing to gain.

I have a WIP port using sio_acpi_mode = 0, but now that it boots I don't really depend on the UART (part of Serial IO, I had it working in PCI mode for coreboot logs) as much.
> Patch Set 8:
>
> Patch:
> https://review.coreboot.org/c/coreboot/+/38810
>
> It allows us to update IASL:
> https://qa.coreboot.org/job/coreboot_updated_iasl/647/

Sounds good, let's get it done!

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Martin Roth (Code Review)]

Martin Roth has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Removed Code-Review+2 by Martin Roth

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8:
> I have a WIP port using sio_acpi_mode = 0, but now that it boots I don't really depend on the UART (part of Serial IO, I had it working in PCI mode for coreboot logs) as much.

taking a very brief look at the BDW serialio code, seems like there should not be a problem using the UART for coreboot debug output while having it in ACPI mode for the OS

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Patch Set 8:

(1 comment)
> Patch Set 8: Code-Review-1
>
> (1 comment)

https://review.coreboot.org/c/coreboot/+/38350/8/src/soc/intel/broadwell/acp... 
File src/soc/intel/broadwell/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/38350/8/src/soc/intel/broadwell/acp... 
PS8, Line 157: Device (SDMA)
> iasl does not complain about having both _HID and _ADR.

Perhaps the compiler only checks if the two exist within the same declarative block?

Change in coreboot[master]: soc/intel/broadwell: Don't use _ADR and _HID

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38350 )

Change subject: soc/intel/broadwell: Don't use _ADR and _HID
......................................................................

Abandoned