Change in coreboot[master]: src/mainboard/{intel/google}: Include ASL for additional PCI segment

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41012 )

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

src/mainboard/{intel/google}: Include ASL for additional PCI segment

This patch allows mainboard to include static ASL for TBT PCI segment

extracted build/dsdt.aml

Device (PCI1)
{
    Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: Hardware ID
    Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: Compatible ID
    Name (_SEG, One)  // _SEG: PCI Segment
    Name (_UID, One)  // _UID: Unique ID
    Name (_ADR, Zero)  // _ADR: Address
....
}

Change-Id: I3601aa4e9002334fd80fc86ced9e1df2afc739b5
Signed-off-by: Subrata Banik 
---
M src/mainboard/google/deltaur/dsdt.asl
M src/mainboard/google/volteer/dsdt.asl
M src/mainboard/intel/tglrvp/dsdt.asl
3 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/41012/1

diff --git a/src/mainboard/google/deltaur/dsdt.asl b/src/mainboard/google/deltaur/dsdt.asl
index 631ec5e..6359feb 100644
--- a/src/mainboard/google/deltaur/dsdt.asl
+++ b/src/mainboard/google/deltaur/dsdt.asl
@@ -30,6 +30,9 @@
    		#include <soc/intel/common/block/acpi/acpi/northbridge.asl>
    		#include <soc/intel/tigerlake/acpi/southbridge.asl>
    	}
+#if CONFIG_PCI_SEGMENT_GROUPS > 1
+		#include <soc/intel/common/block/acpi/acpi/pcisegment.asl>
+#endif
    }

/* Chrome OS specific */
diff --git a/src/mainboard/google/volteer/dsdt.asl b/src/mainboard/google/volteer/dsdt.asl
index a87c743..9a83359 100644
--- a/src/mainboard/google/volteer/dsdt.asl
+++ b/src/mainboard/google/volteer/dsdt.asl
@@ -33,6 +33,9 @@
    		#include <soc/intel/tigerlake/acpi/southbridge.asl>
    		#include <soc/intel/tigerlake/acpi/tcss.asl>
    	}
+#if CONFIG_PCI_SEGMENT_GROUPS > 1
+		#include <soc/intel/common/block/acpi/acpi/pcisegment.asl>
+#endif
    	/* Mainboard hooks */
    	#include "mainboard.asl"
    }
diff --git a/src/mainboard/intel/tglrvp/dsdt.asl b/src/mainboard/intel/tglrvp/dsdt.asl
index c66e972..60275cc 100644
--- a/src/mainboard/intel/tglrvp/dsdt.asl
+++ b/src/mainboard/intel/tglrvp/dsdt.asl
@@ -29,6 +29,9 @@
    		#include <soc/intel/tigerlake/acpi/southbridge.asl>
    		#include <soc/intel/tigerlake/acpi/tcss.asl>
    	}
+#if CONFIG_PCI_SEGMENT_GROUPS > 1
+		#include <soc/intel/common/block/acpi/acpi/pcisegment.asl>
+#endif
    }

#if CONFIG(CHROMEOS)

[HAOUAS Elyes (Code Review)]

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

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

Patch Set 1: Code-Review-1

something is wrong with Jenkins, this should fail...
"A device object must contain either an _HID object or an _ADR
object, but should not contain both."

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41012 )

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-1
>
> something is wrong with Jenkins, this should fail...
> "A device object must contain either an _HID object or an _ADR
> object, but should not contain both."

Jenkins might still use an older iasl version that doesn't check for that.

[HAOUAS Elyes (Code Review)]

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

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

Patch Set 1:

please see also https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html section "Device Tree namespace link device ID"

[Angel Pons (Code Review)]

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

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1: Code-Review-1
> >
> > something is wrong with Jenkins, this should fail...
> > "A device object must contain either an _HID object or an _ADR
> > object, but should not contain both."
>
> Jenkins might still use an older iasl version that doesn't check for that.

We upgraded, but we explicitly disabled that warning. There has been a lot of drama about this warning though, so I don't know what's best.

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, HAOUAS Elyes,

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

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

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

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

src/mainboard/{intel/google}: Include ASL for additional PCI segment

This patch allows mainboard to include static ASL for TBT PCI segment

extracted build/dsdt.aml

Device (PCI1)
{
    Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: Hardware ID
    Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: Compatible ID
    Name (_SEG, One)  // _SEG: PCI Segment
    Name (_UID, One)  // _UID: Unique ID
....
}

Change-Id: I3601aa4e9002334fd80fc86ced9e1df2afc739b5
Signed-off-by: Subrata Banik 
---
M src/mainboard/google/deltaur/dsdt.asl
M src/mainboard/google/volteer/dsdt.asl
M src/mainboard/intel/tglrvp/dsdt.asl
3 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/41012/2

[HAOUAS Elyes (Code Review)]

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

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1: Code-Review-1
> > >
> > > something is wrong with Jenkins, this should fail...
> > > "A device object must contain either an _HID object or an _ADR
> > > object, but should not contain both."
> >
> > Jenkins might still use an older iasl version that doesn't check for that.
>
> We upgraded, but we explicitly disabled that warning. There has been a lot of drama about this warning though, so I don't know what's best.

No "drama".
We have acpi specs, we also have https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html  
so using _ADR and _HID at the same time is absolutely out of specs.
now, I don't know what is the best : 
still using out of specs code or fix wrong code ...

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, HAOUAS Elyes,

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

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

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

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

src/mainboard/{intel/google}: Include ASL for additional PCI segment

This patch allows mainboard to include static ASL for TBT PCI segment

extracted build/dsdt.aml

Device (PCI1)
{
    Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: Hardware ID
    Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: Compatible ID
    Name (_SEG, One)  // _SEG: PCI Segment
    Name (_UID, One)  // _UID: Unique ID
....
}

Change-Id: I3601aa4e9002334fd80fc86ced9e1df2afc739b5
Signed-off-by: Subrata Banik 
---
M src/mainboard/google/deltaur/dsdt.asl
M src/mainboard/google/volteer/dsdt.asl
M src/mainboard/intel/tglrvp/dsdt.asl
3 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/41012/3

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41012 )

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

Patch Set 5: Code-Review+1

[XS] Change in coreboot[master]: src/mainboard/{intel/google}: Include ASL for additional PCI segment

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41012?usp=email )

Change subject: src/mainboard/{intel/google}: Include ASL for additional PCI segment
......................................................................

Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.