| Message ID | 1461161371-17890-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
On 20 April 2016 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote: > XPress-RICH3 PCIe driver initialises the root complex with the same > source and target address for IO window which makes translate offset 0. > > This patch fixes the translate offset for the IO window in Juno PCIe > root complex ACPI table. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Cc: Tomasz Nowicki <tn@semihalf.com> > Cc: Graeme Gregory <graeme.gregory@linaro.org> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > v1->v2: > - Made changes as suggested by Graeme & Tomasz You may want to update the commit log as well > > diff --git a/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl b/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl > index 800d2cb3b2fb..5ff267fe5124 100644 > --- a/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl > +++ b/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl > @@ -107,8 +107,8 @@ DefinitionBlock("SsdtPci.aml", "SSDT", 1, "ARMLTD", "ARM-JUNO", EFI_ACPI_ARM_OEM > PosDecode, > EntireRange, > 0x00000000, // Granularity > - 0x5f800000, // Min Base Address > - 0x5fffffff, // Max Base Address > + 0x00000000, // Min Base Address > + 0x000fffff, // Max Base Address > 0x5f800000, // Translate > 0x00800000 // Length > ) > -- > 1.9.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 20/04/16 15:10, Ard Biesheuvel wrote: > On 20 April 2016 at 16:09, Sudeep Holla <sudeep.holla@arm.com> wrote: >> XPress-RICH3 PCIe driver initialises the root complex with the same >> source and target address for IO window which makes translate offset 0. >> >> This patch fixes the translate offset for the IO window in Juno PCIe >> root complex ACPI table. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Leif Lindholm <leif.lindholm@linaro.org> >> Cc: Tomasz Nowicki <tn@semihalf.com> >> Cc: Graeme Gregory <graeme.gregory@linaro.org> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> v1->v2: >> - Made changes as suggested by Graeme & Tomasz > > You may want to update the commit log as well > Ah right in fact $subject too :), it was stupid of me. -- Regards, Sudeep _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 20 April 2016 at 15:30, Sudeep Holla <sudeep.holla@arm.com> wrote: > XPress-RICH3 PCIe driver initializes the root complex with the source > and target address for IO window. The root complex resources in SSDT > should match these settings. > > This patch fixes the min/max base address for the IO window in Juno PCIe > root complex ACPI table. > I have not tested it but diff looks good to me. Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org> > Contributed-under: TianoCore Contribution Agreement 1.0 > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Cc: Tomasz Nowicki <tn@semihalf.com> > Cc: Graeme Gregory <graeme.gregory@linaro.org> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > v2->v3: > - Fixed $subject and the commit log > v1->v2: > - Made changes as suggested by Graeme & Tomasz > > diff --git a/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl b/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl > index 800d2cb3b2fb..5ff267fe5124 100644 > --- a/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl > +++ b/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl > @@ -107,8 +107,8 @@ DefinitionBlock("SsdtPci.aml", "SSDT", 1, "ARMLTD", "ARM-JUNO", EFI_ACPI_ARM_OEM > PosDecode, > EntireRange, > 0x00000000, // Granularity > - 0x5f800000, // Min Base Address > - 0x5fffffff, // Max Base Address > + 0x00000000, // Min Base Address > + 0x000fffff, // Max Base Address > 0x5f800000, // Translate > 0x00800000 // Length > ) > -- > 1.9.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 20/04/16 15:39, G Gregory wrote: > On 20 April 2016 at 15:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >> XPress-RICH3 PCIe driver initializes the root complex with the source >> and target address for IO window. The root complex resources in SSDT >> should match these settings. >> >> This patch fixes the min/max base address for the IO window in Juno PCIe >> root complex ACPI table. >> > > I have not tested it but diff looks good to me. > Ah crap, something really wrong today with me, I failed to notice that error UEFI is throwing with this change :( uefi/Build/ArmJuno/RELEASE_GCC49/AARCH64/OpenPlatformPkg/Platforms/ARM/Juno/AcpiTables/AcpiTables/OUTPUT/./AcpiSsdtRootPci.iiii 60: 0x00800000 Error 6049 Length is larger than Min/Max window ^ -- Regards, Sudeep _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 20/04/16 16:13, Jeremy Linton wrote: > On 04/20/2016 09:39 AM, G Gregory wrote: >> On 20 April 2016 at 15:30, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> XPress-RICH3 PCIe driver initializes the root complex with the source >>> and target address for IO window. The root complex resources in SSDT >>> should match these settings. >>> >>> This patch fixes the min/max base address for the IO window in Juno PCIe >>> root complex ACPI table. >>> [...] > > I believe you also need to specify type translate to indicate that the > secondary side of the bridge is I/O. This part of the specification is > entirely unclear and disagrees with itself depending on which > version/paragraph of the specification you read. But the intent of that > bit for both DwordMemory and DwordIO seems to indicate that an io/memory > transaction is changing types when it crosses the bridge. That is true > for ARM devices which are writing a MMIO and a PCIe IO transaction is > being generated. True, so what do you suggest ? How do we proceed on this then ? > > Also, are you sure that the length and min/max range should be different? > Yes I got bitten by that and I failed to notice it :). I have fixed it locally and tested correctly now. -- Regards, Sudeep _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 20/04/16 16:44, Jeremy Linton wrote: > On 04/20/2016 10:35 AM, Sudeep Holla wrote: >> [...] >> >> Yes I got bitten by that and I failed to notice it :). I have fixed it >> locally and tested correctly now. >> > > DWordIo ( // IO window > ResourceProducer, > MinFixed, > MaxFixed, > PosDecode, > EntireRange, > 0x00000000, // Granularity > 0x00000000, // Min Base Address > 0x007fffff, // Max Base Address > 0x5f800000, // Translate > 0x00800000, // Length > , > , > , This is exactly what I have now after fixing the length and works fine, but ... > TypeTranslation ... this is TypeStatic which I assume is default when not specified. I tried TypeTranslation too and it works fine. It adds DenseTranslation by default. Do you want me to add TypeTranslation? -- Regards, Sudeep _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 20/04/16 16:54, Sudeep Holla wrote: > > > On 20/04/16 16:44, Jeremy Linton wrote: >> On 04/20/2016 10:35 AM, Sudeep Holla wrote: >>> > > [...] > >>> >>> Yes I got bitten by that and I failed to notice it :). I have fixed it >>> locally and tested correctly now. >>> >> >> DWordIo ( // IO window >> ResourceProducer, >> MinFixed, >> MaxFixed, >> PosDecode, >> EntireRange, >> 0x00000000, // Granularity >> 0x00000000, // Min Base Address >> 0x007fffff, // Max Base Address >> 0x5f800000, // Translate >> 0x00800000, // Length >> , >> , >> , > > This is exactly what I have now after fixing the length and works fine, > but ... > >> TypeTranslation > > ... this is TypeStatic which I assume is default when not specified. > > I tried TypeTranslation too and it works fine. It adds DenseTranslation > by default. Do you want me to add TypeTranslation? > I must have seen spec before replying to you :( It states: "TranslationType is an optional argument that specifies whether the resource type on the secondary side of the bus is different (TypeTranslation) from that on the primary side of the bus or the same (TypeStatic). If TypeTranslation is specified, then the secondary side of the bus is Memory. If TypeStatic is specified, then the secondary side of the bus is I/O. If nothing is specified, then TypeStatic is assumed." So IIUC it's wrong to have TypeTranslation. OTH, don't we need that for memory window ? -- Regards, Sudeep _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Apr 20, 2016 at 11:06:28AM -0500, Jeremy Linton wrote: > On 04/20/2016 11:00 AM, Sudeep Holla wrote: > > > > > >On 20/04/16 16:54, Sudeep Holla wrote: > >> > >> > >>On 20/04/16 16:44, Jeremy Linton wrote: > >>>On 04/20/2016 10:35 AM, Sudeep Holla wrote: > >>>> > >> > >>[...] > >> > >>>> > >>>>Yes I got bitten by that and I failed to notice it :). I have fixed it > >>>>locally and tested correctly now. > >>>> > >>> > >>>DWordIo ( // IO window > >>> ResourceProducer, > >>> MinFixed, > >>> MaxFixed, > >>> PosDecode, > >>> EntireRange, > >>> 0x00000000, // Granularity > >>> 0x00000000, // Min Base Address > >>> 0x007fffff, // Max Base Address > >>> 0x5f800000, // Translate > >>> 0x00800000, // Length > >>> , > >>> , > >>> , > >> > >>This is exactly what I have now after fixing the length and works fine, > >>but ... > >> > >>> TypeTranslation > >> > >>... this is TypeStatic which I assume is default when not specified. > >> > >>I tried TypeTranslation too and it works fine. It adds DenseTranslation > >>by default. Do you want me to add TypeTranslation? > >> > > > >I must have seen spec before replying to you :( It states: > > > >"TranslationType is an optional argument that specifies whether the > >resource type on the secondary side of the bus is different > >(TypeTranslation) from that on the primary side of the bus or the same > >(TypeStatic). If TypeTranslation is specified, then the secondary side > >of the bus is Memory. If TypeStatic is specified, then the secondary > >side of the bus is I/O. If nothing is specified, then TypeStatic is > >assumed." > > See table 6-214, it disagrees. Also older spec's seem to say something > different. > > From memory the conclusion on ASWG was that TypeTranslation was correct but it was so misused in past due to misleading spec that most OSes ignore it. Graeme _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 20/04/16 17:04, Jeremy Linton wrote: > On 04/20/2016 10:54 AM, Sudeep Holla wrote: >> >> >> On 20/04/16 16:44, Jeremy Linton wrote: >>> On 04/20/2016 10:35 AM, Sudeep Holla wrote: >>>> >> >> [...] >> >>>> >>>> Yes I got bitten by that and I failed to notice it :). I have fixed it >>>> locally and tested correctly now. >>>> >>> >>> DWordIo ( // IO window >>> ResourceProducer, >>> MinFixed, >>> MaxFixed, >>> PosDecode, >>> EntireRange, >>> 0x00000000, // Granularity >>> 0x00000000, // Min Base Address >>> 0x007fffff, // Max Base Address >>> 0x5f800000, // Translate >>> 0x00800000, // Length >>> , >>> , >>> , >> >> This is exactly what I have now after fixing the length and works fine, >> but ... >> >>> TypeTranslation >> >> ... this is TypeStatic which I assume is default when not specified. >> >> I tried TypeTranslation too and it works fine. It adds DenseTranslation >> by default. Do you want me to add TypeTranslation? > > That is your call, but I believe that TypeTranslation is correct, if you > read table 6-214 in ACPI6 (I/O Resource Flag (resource Type=1) > Definitions). That table to be the clearest statement of intent for the > field and the one I would refer to. Agreed, I didn't see that table. -- Regards, Sudeep _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl b/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl index 800d2cb3b2fb..5ff267fe5124 100644 --- a/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl +++ b/Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl @@ -107,8 +107,8 @@ DefinitionBlock("SsdtPci.aml", "SSDT", 1, "ARMLTD", "ARM-JUNO", EFI_ACPI_ARM_OEM PosDecode, EntireRange, 0x00000000, // Granularity - 0x5f800000, // Min Base Address - 0x5fffffff, // Max Base Address + 0x00000000, // Min Base Address + 0x000fffff, // Max Base Address 0x5f800000, // Translate 0x00800000 // Length )
XPress-RICH3 PCIe driver initialises the root complex with the same source and target address for IO window which makes translate offset 0. This patch fixes the translate offset for the IO window in Juno PCIe root complex ACPI table. Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Leif Lindholm <leif.lindholm@linaro.org> Cc: Tomasz Nowicki <tn@semihalf.com> Cc: Graeme Gregory <graeme.gregory@linaro.org> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- Platforms/ARM/Juno/AcpiTables/AcpiSsdtRootPci.asl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) v1->v2: - Made changes as suggested by Graeme & Tomasz -- 1.9.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel