[Linaro-uefi,1/2] Platforms/AMD: correct legacy PCI interrupt routing in DSDT

Message ID 20170407132851.28513-1-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • [Linaro-uefi,1/2] Platforms/AMD: correct legacy PCI interrupt routing in DSDT
Related show

Commit Message

Ard Biesheuvel April 7, 2017, 1:28 p.m.
The _PRT method in the PCI0 object describes something that resembles
the legacy interrupt routing of the first slot only, but applies it to
all PCI-PCI bridges, which means the wrong interrupt is reported for
devices in slots 2 and 3. Since most devices support MSI, this is not
actually a big deal, but it would be nice to fix this nonetheless.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platforms/AMD/Styx/AcpiTables/Dsdt.asl | 52 +++++++-------------
 1 file changed, 19 insertions(+), 33 deletions(-)

Comments

Graeme Gregory April 10, 2017, 10:05 a.m. | #1
On Fri, Apr 07, 2017 at 02:28:50PM +0100, Ard Biesheuvel wrote:
> The _PRT method in the PCI0 object describes something that resembles

> the legacy interrupt routing of the first slot only, but applies it to

> all PCI-PCI bridges, which means the wrong interrupt is reported for

> devices in slots 2 and 3. Since most devices support MSI, this is not

> actually a big deal, but it would be nice to fix this nonetheless.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Considering exlanation on linux-acpi thread this looks fine to me.

Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>


> ---

>  Platforms/AMD/Styx/AcpiTables/Dsdt.asl | 52 +++++++-------------

>  1 file changed, 19 insertions(+), 33 deletions(-)

> 

> diff --git a/Platforms/AMD/Styx/AcpiTables/Dsdt.asl b/Platforms/AMD/Styx/AcpiTables/Dsdt.asl

> index 3bfa26acea07..49ef55fc7df6 100644

> --- a/Platforms/AMD/Styx/AcpiTables/Dsdt.asl

> +++ b/Platforms/AMD/Styx/AcpiTables/Dsdt.asl

> @@ -508,39 +508,25 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "AMDINC", "SEATTLE ", 3)

>              Name (_SEG, 0x00)  // _SEG: PCI Segment

>              Name (_BBN, 0x00)  // _BBN: BIOS Bus Number

>              Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

> -            Name (_PRT, Package (0x04)  // _PRT: PCI Routing Table

> -            {

> -                Package (0x04)

> -                {

> -                    0xFFFF,

> -                    0x00,

> -                    0x00,

> -                    0x0140

> -                },

> -

> -                Package (0x04)

> -                {

> -                    0xFFFF,

> -                    0x01,

> -                    0x00,

> -                    0x0141

> -                },

> -

> -                Package (0x04)

> -                {

> -                    0xFFFF,

> -                    0x02,

> -                    0x00,

> -                    0x0142

> -                },

> -

> -                Package (0x04)

> -                {

> -                    0xFFFF,

> -                    0x03,

> -                    0x00,

> -                    0x0143

> -                }

> +            Name (_PRT, Package ()  // _PRT: PCI Routing Table

> +            {

> +                // slot 1: dev 2 fn 1

> +                Package () { 0x20001, 0x0, 0x0, 0x140 },

> +                Package () { 0x20001, 0x1, 0x0, 0x141 },

> +                Package () { 0x20001, 0x2, 0x0, 0x142 },

> +                Package () { 0x20001, 0x3, 0x0, 0x143 },

> +

> +                // slot 2: dev 2 fn 2

> +                Package () { 0x20002, 0x0, 0x0, 0x144 },

> +                Package () { 0x20002, 0x1, 0x0, 0x145 },

> +                Package () { 0x20002, 0x2, 0x0, 0x146 },

> +                Package () { 0x20002, 0x3, 0x0, 0x147 },

> +

> +                // slot 3: dev 2 fn 3

> +                Package () { 0x20003, 0x0, 0x0, 0x148 },

> +                Package () { 0x20003, 0x1, 0x0, 0x149 },

> +                Package () { 0x20003, 0x2, 0x0, 0x14a },

> +                Package () { 0x20003, 0x3, 0x0, 0x14b }

>              }) // _PRT

>  

>              Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings

> -- 

> 2.9.3

>
Ard Biesheuvel April 10, 2017, 10:08 a.m. | #2
On 10 April 2017 at 11:05,  <graeme.gregory@linaro.org> wrote:
> On Fri, Apr 07, 2017 at 02:28:50PM +0100, Ard Biesheuvel wrote:
>> The _PRT method in the PCI0 object describes something that resembles
>> the legacy interrupt routing of the first slot only, but applies it to
>> all PCI-PCI bridges, which means the wrong interrupt is reported for
>> devices in slots 2 and 3. Since most devices support MSI, this is not
>> actually a big deal, but it would be nice to fix this nonetheless.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Considering exlanation on linux-acpi thread this looks fine to me.
>
> Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>
>

Uhm, actually is not. As Bjorn and others pointed out, the function
part in the PRT entries must be 0xffff in all cased (according to the
spec), and so this approach doesn't fly.

I have sent a v2 that does the right thing, and works without changes
to the kernel. There was some confusion on my part due to the fact
that I was still receiving some errors (which are also fixed in v2),
but the actual problem was already solved by Bjorn's suggested
approach (confirmed on Cello and Overdrive using devices that are not
MSI capable (or can be made so))

>> ---
>>  Platforms/AMD/Styx/AcpiTables/Dsdt.asl | 52 +++++++-------------
>>  1 file changed, 19 insertions(+), 33 deletions(-)
>>
>> diff --git a/Platforms/AMD/Styx/AcpiTables/Dsdt.asl b/Platforms/AMD/Styx/AcpiTables/Dsdt.asl
>> index 3bfa26acea07..49ef55fc7df6 100644
>> --- a/Platforms/AMD/Styx/AcpiTables/Dsdt.asl
>> +++ b/Platforms/AMD/Styx/AcpiTables/Dsdt.asl
>> @@ -508,39 +508,25 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "AMDINC", "SEATTLE ", 3)
>>              Name (_SEG, 0x00)  // _SEG: PCI Segment
>>              Name (_BBN, 0x00)  // _BBN: BIOS Bus Number
>>              Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute
>> -            Name (_PRT, Package (0x04)  // _PRT: PCI Routing Table
>> -            {
>> -                Package (0x04)
>> -                {
>> -                    0xFFFF,
>> -                    0x00,
>> -                    0x00,
>> -                    0x0140
>> -                },
>> -
>> -                Package (0x04)
>> -                {
>> -                    0xFFFF,
>> -                    0x01,
>> -                    0x00,
>> -                    0x0141
>> -                },
>> -
>> -                Package (0x04)
>> -                {
>> -                    0xFFFF,
>> -                    0x02,
>> -                    0x00,
>> -                    0x0142
>> -                },
>> -
>> -                Package (0x04)
>> -                {
>> -                    0xFFFF,
>> -                    0x03,
>> -                    0x00,
>> -                    0x0143
>> -                }
>> +            Name (_PRT, Package ()  // _PRT: PCI Routing Table
>> +            {
>> +                // slot 1: dev 2 fn 1
>> +                Package () { 0x20001, 0x0, 0x0, 0x140 },
>> +                Package () { 0x20001, 0x1, 0x0, 0x141 },
>> +                Package () { 0x20001, 0x2, 0x0, 0x142 },
>> +                Package () { 0x20001, 0x3, 0x0, 0x143 },
>> +
>> +                // slot 2: dev 2 fn 2
>> +                Package () { 0x20002, 0x0, 0x0, 0x144 },
>> +                Package () { 0x20002, 0x1, 0x0, 0x145 },
>> +                Package () { 0x20002, 0x2, 0x0, 0x146 },
>> +                Package () { 0x20002, 0x3, 0x0, 0x147 },
>> +
>> +                // slot 3: dev 2 fn 3
>> +                Package () { 0x20003, 0x0, 0x0, 0x148 },
>> +                Package () { 0x20003, 0x1, 0x0, 0x149 },
>> +                Package () { 0x20003, 0x2, 0x0, 0x14a },
>> +                Package () { 0x20003, 0x3, 0x0, 0x14b }
>>              }) // _PRT
>>
>>              Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings
>> --
>> 2.9.3
>>
Graeme Gregory April 10, 2017, 10:15 a.m. | #3
On Mon, Apr 10, 2017 at 11:08:31AM +0100, Ard Biesheuvel wrote:
> On 10 April 2017 at 11:05,  <graeme.gregory@linaro.org> wrote:

> > On Fri, Apr 07, 2017 at 02:28:50PM +0100, Ard Biesheuvel wrote:

> >> The _PRT method in the PCI0 object describes something that resembles

> >> the legacy interrupt routing of the first slot only, but applies it to

> >> all PCI-PCI bridges, which means the wrong interrupt is reported for

> >> devices in slots 2 and 3. Since most devices support MSI, this is not

> >> actually a big deal, but it would be nice to fix this nonetheless.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.0

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >

> > Considering exlanation on linux-acpi thread this looks fine to me.

> >

> > Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>

> >

> 

> Uhm, actually is not. As Bjorn and others pointed out, the function

> part in the PRT entries must be 0xffff in all cased (according to the

> spec), and so this approach doesn't fly.

> 

> I have sent a v2 that does the right thing, and works without changes

> to the kernel. There was some confusion on my part due to the fact

> that I was still receiving some errors (which are also fixed in v2),

> but the actual problem was already solved by Bjorn's suggested

> approach (confirmed on Cello and Overdrive using devices that are not

> MSI capable (or can be made so))

> 


yeah sorry what I get for reading half in gmail and half in mutt,
followed the wrong rabbit hole to thread end.

Sorry, re-looking at thread and v2 now!

Graeme

> >> ---

> >>  Platforms/AMD/Styx/AcpiTables/Dsdt.asl | 52 +++++++-------------

> >>  1 file changed, 19 insertions(+), 33 deletions(-)

> >>

> >> diff --git a/Platforms/AMD/Styx/AcpiTables/Dsdt.asl b/Platforms/AMD/Styx/AcpiTables/Dsdt.asl

> >> index 3bfa26acea07..49ef55fc7df6 100644

> >> --- a/Platforms/AMD/Styx/AcpiTables/Dsdt.asl

> >> +++ b/Platforms/AMD/Styx/AcpiTables/Dsdt.asl

> >> @@ -508,39 +508,25 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "AMDINC", "SEATTLE ", 3)

> >>              Name (_SEG, 0x00)  // _SEG: PCI Segment

> >>              Name (_BBN, 0x00)  // _BBN: BIOS Bus Number

> >>              Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

> >> -            Name (_PRT, Package (0x04)  // _PRT: PCI Routing Table

> >> -            {

> >> -                Package (0x04)

> >> -                {

> >> -                    0xFFFF,

> >> -                    0x00,

> >> -                    0x00,

> >> -                    0x0140

> >> -                },

> >> -

> >> -                Package (0x04)

> >> -                {

> >> -                    0xFFFF,

> >> -                    0x01,

> >> -                    0x00,

> >> -                    0x0141

> >> -                },

> >> -

> >> -                Package (0x04)

> >> -                {

> >> -                    0xFFFF,

> >> -                    0x02,

> >> -                    0x00,

> >> -                    0x0142

> >> -                },

> >> -

> >> -                Package (0x04)

> >> -                {

> >> -                    0xFFFF,

> >> -                    0x03,

> >> -                    0x00,

> >> -                    0x0143

> >> -                }

> >> +            Name (_PRT, Package ()  // _PRT: PCI Routing Table

> >> +            {

> >> +                // slot 1: dev 2 fn 1

> >> +                Package () { 0x20001, 0x0, 0x0, 0x140 },

> >> +                Package () { 0x20001, 0x1, 0x0, 0x141 },

> >> +                Package () { 0x20001, 0x2, 0x0, 0x142 },

> >> +                Package () { 0x20001, 0x3, 0x0, 0x143 },

> >> +

> >> +                // slot 2: dev 2 fn 2

> >> +                Package () { 0x20002, 0x0, 0x0, 0x144 },

> >> +                Package () { 0x20002, 0x1, 0x0, 0x145 },

> >> +                Package () { 0x20002, 0x2, 0x0, 0x146 },

> >> +                Package () { 0x20002, 0x3, 0x0, 0x147 },

> >> +

> >> +                // slot 3: dev 2 fn 3

> >> +                Package () { 0x20003, 0x0, 0x0, 0x148 },

> >> +                Package () { 0x20003, 0x1, 0x0, 0x149 },

> >> +                Package () { 0x20003, 0x2, 0x0, 0x14a },

> >> +                Package () { 0x20003, 0x3, 0x0, 0x14b }

> >>              }) // _PRT

> >>

> >>              Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings

> >> --

> >> 2.9.3

> >>

Patch hide | download patch | download mbox

diff --git a/Platforms/AMD/Styx/AcpiTables/Dsdt.asl b/Platforms/AMD/Styx/AcpiTables/Dsdt.asl
index 3bfa26acea07..49ef55fc7df6 100644
--- a/Platforms/AMD/Styx/AcpiTables/Dsdt.asl
+++ b/Platforms/AMD/Styx/AcpiTables/Dsdt.asl
@@ -508,39 +508,25 @@  DefinitionBlock ("DSDT.aml", "DSDT", 2, "AMDINC", "SEATTLE ", 3)
             Name (_SEG, 0x00)  // _SEG: PCI Segment
             Name (_BBN, 0x00)  // _BBN: BIOS Bus Number
             Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute
-            Name (_PRT, Package (0x04)  // _PRT: PCI Routing Table
-            {
-                Package (0x04)
-                {
-                    0xFFFF,
-                    0x00,
-                    0x00,
-                    0x0140
-                },
-
-                Package (0x04)
-                {
-                    0xFFFF,
-                    0x01,
-                    0x00,
-                    0x0141
-                },
-
-                Package (0x04)
-                {
-                    0xFFFF,
-                    0x02,
-                    0x00,
-                    0x0142
-                },
-
-                Package (0x04)
-                {
-                    0xFFFF,
-                    0x03,
-                    0x00,
-                    0x0143
-                }
+            Name (_PRT, Package ()  // _PRT: PCI Routing Table
+            {
+                // slot 1: dev 2 fn 1
+                Package () { 0x20001, 0x0, 0x0, 0x140 },
+                Package () { 0x20001, 0x1, 0x0, 0x141 },
+                Package () { 0x20001, 0x2, 0x0, 0x142 },
+                Package () { 0x20001, 0x3, 0x0, 0x143 },
+
+                // slot 2: dev 2 fn 2
+                Package () { 0x20002, 0x0, 0x0, 0x144 },
+                Package () { 0x20002, 0x1, 0x0, 0x145 },
+                Package () { 0x20002, 0x2, 0x0, 0x146 },
+                Package () { 0x20002, 0x3, 0x0, 0x147 },
+
+                // slot 3: dev 2 fn 3
+                Package () { 0x20003, 0x0, 0x0, 0x148 },
+                Package () { 0x20003, 0x1, 0x0, 0x149 },
+                Package () { 0x20003, 0x2, 0x0, 0x14a },
+                Package () { 0x20003, 0x3, 0x0, 0x14b }
             }) // _PRT
 
             Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings