| 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 | expand |
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 >
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 >>
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 > >>
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
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(-)