This implements a quirk for the Marvell Armada80x0 running in ACPI mode, in which case the config space configuration is not 100% ECAM compatible.
The issue with this SoC is that it only consists of a host bridge, and all type 0 configuration cycles are forwarded to the peer (i.e., the device in the slot), which will therefore appear at all device slots on bus 0. This can be fixed by reducing the IATU window size for bus 0, but due to CX_ATU_MIN_REGION_SIZE == 64KB (which is a synthesis-time configuration parameter of the IP block) the window producing type 0 config cycles will always produce two copies of the peer device.
So add a quirk that specifies a config space accessor override, and override config space accesses to devices on bus 0 other than device 0.
Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org --- drivers/acpi/pci_mcfg.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 2944353253ed..7d68b3208043 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -49,6 +49,40 @@ struct mcfg_fixup { NULL, IORESOURCE_BUS) #define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff)
+/* + * The Armada 8k uses Synopsys PCIe IP, which /can/ be configured at + * synthesis time to be ECAM compatible, by reducing the IATU minimum + * window size to 32 KB. This way, we can reduce the window that produces + * type 0 config cycles to only cover b/d 0/0, which will make the peer + * only appear once on bus 0. The rest of the ECAM region can be enabled + * to generate type 1 config cycles in an ECAM compliant manner, which + * makes the rest of the hierarchy accessible if the peer is a true + * bridge. + * + * Note that in this configuration, there is only a host bridge that + * translates memory accesses to PCI bus cycles, and given that there + * is only a single peer, there is no reason to model a pseudobridge + * on bus 0 and make the peer appear on bus 1. (This is how the various + * DT drivers for snps,dw-pcie compatible controllers are implemented.) + */ +static void __iomem *armada8k_pcie_ecam_map_bus(struct pci_bus *bus, + unsigned int devfn, + int where) +{ + if (bus->number == 0 && PCI_SLOT(devfn) > 0) + return NULL; + return pci_ecam_map_bus(bus, devfn, where); +} + +static struct pci_ecam_ops armada8k_pcie_ecam_ops = { + .bus_shift = 20, + .pci_ops = { + .map_bus = armada8k_pcie_ecam_map_bus, + .read = pci_generic_config_read, + .write = pci_generic_config_write, + } +}; + static struct mcfg_fixup mcfg_quirks[] = { /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */
@@ -133,6 +167,8 @@ static struct mcfg_fixup mcfg_quirks[] = { XGENE_V2_ECAM_MCFG(4, 0), XGENE_V2_ECAM_MCFG(4, 1), XGENE_V2_ECAM_MCFG(4, 2), + + { "MVEBU ", "ARMADA8K", 0, 0, MCFG_BUS_ANY, &armada8k_pcie_ecam_ops }, };
static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
On Thu, Apr 27, 2017 at 02:17:32PM +0100, Ard Biesheuvel wrote:
This implements a quirk for the Marvell Armada80x0 running in ACPI mode, in which case the config space configuration is not 100% ECAM compatible.
Too bad, I am not keen on merging any more ECAM quirks, we have been very clear about this and it is in our best interest to keep this code out of tree - we will bootstrap ACPI PCI systems on ECAM HW/FW compliant systems, that's it (I understand it is painful but it is necessary).
Thanks, Lorenzo
The issue with this SoC is that it only consists of a host bridge, and all type 0 configuration cycles are forwarded to the peer (i.e., the device in the slot), which will therefore appear at all device slots on bus 0. This can be fixed by reducing the IATU window size for bus 0, but due to CX_ATU_MIN_REGION_SIZE == 64KB (which is a synthesis-time configuration parameter of the IP block) the window producing type 0 config cycles will always produce two copies of the peer device.
So add a quirk that specifies a config space accessor override, and override config space accesses to devices on bus 0 other than device 0.
Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
drivers/acpi/pci_mcfg.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 2944353253ed..7d68b3208043 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -49,6 +49,40 @@ struct mcfg_fixup { NULL, IORESOURCE_BUS) #define MCFG_BUS_ANY MCFG_BUS_RANGE(0x0, 0xff) +/*
- The Armada 8k uses Synopsys PCIe IP, which /can/ be configured at
- synthesis time to be ECAM compatible, by reducing the IATU minimum
- window size to 32 KB. This way, we can reduce the window that produces
- type 0 config cycles to only cover b/d 0/0, which will make the peer
- only appear once on bus 0. The rest of the ECAM region can be enabled
- to generate type 1 config cycles in an ECAM compliant manner, which
- makes the rest of the hierarchy accessible if the peer is a true
- bridge.
- Note that in this configuration, there is only a host bridge that
- translates memory accesses to PCI bus cycles, and given that there
- is only a single peer, there is no reason to model a pseudobridge
- on bus 0 and make the peer appear on bus 1. (This is how the various
- DT drivers for snps,dw-pcie compatible controllers are implemented.)
- */
+static void __iomem *armada8k_pcie_ecam_map_bus(struct pci_bus *bus,
unsigned int devfn,int where)+{
- if (bus->number == 0 && PCI_SLOT(devfn) > 0)
return NULL;- return pci_ecam_map_bus(bus, devfn, where);
+}
+static struct pci_ecam_ops armada8k_pcie_ecam_ops = {
- .bus_shift = 20,
- .pci_ops = {
.map_bus = armada8k_pcie_ecam_map_bus,.read = pci_generic_config_read,.write = pci_generic_config_write,- }
+};
static struct mcfg_fixup mcfg_quirks[] = { /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */ @@ -133,6 +167,8 @@ static struct mcfg_fixup mcfg_quirks[] = { XGENE_V2_ECAM_MCFG(4, 0), XGENE_V2_ECAM_MCFG(4, 1), XGENE_V2_ECAM_MCFG(4, 2),
- { "MVEBU ", "ARMADA8K", 0, 0, MCFG_BUS_ANY, &armada8k_pcie_ecam_ops },
}; static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; -- 2.9.3
On 28 April 2017 at 17:14, Lorenzo Pieralisi lorenzo.pieralisi@arm.com wrote:
On Thu, Apr 27, 2017 at 02:17:32PM +0100, Ard Biesheuvel wrote:
This implements a quirk for the Marvell Armada80x0 running in ACPI mode, in which case the config space configuration is not 100% ECAM compatible.
Too bad, I am not keen on merging any more ECAM quirks, we have been very clear about this and it is in our best interest to keep this code out of tree - we will bootstrap ACPI PCI systems on ECAM HW/FW compliant systems, that's it (I understand it is painful but it is necessary).
I agree, and I was one of the vocal ones, remember?
The only difference is that this particular board may be very useful for development purposes, i.e., the firmware is completely open (and easily updated) and the board is cheap compared to other development hardware.
Graeme has volunteered to carry this patch in the Linaro ERP release, and if this is all we can wish for, I am not going to debate it any further (and I deliberately kept it off the linux-acpi list for now). But I do think it is useful to have the discussion.
On Fri, Apr 28, 2017 at 05:16:41PM +0100, Ard Biesheuvel wrote:
On 28 April 2017 at 17:14, Lorenzo Pieralisi lorenzo.pieralisi@arm.com wrote:
On Thu, Apr 27, 2017 at 02:17:32PM +0100, Ard Biesheuvel wrote:
This implements a quirk for the Marvell Armada80x0 running in ACPI mode, in which case the config space configuration is not 100% ECAM compatible.
Too bad, I am not keen on merging any more ECAM quirks, we have been very clear about this and it is in our best interest to keep this code out of tree - we will bootstrap ACPI PCI systems on ECAM HW/FW compliant systems, that's it (I understand it is painful but it is necessary).
I agree, and I was one of the vocal ones, remember?
Of course I do and I am grateful for that but I do not want to see these quirks upstream, please understand.
The only difference is that this particular board may be very useful for development purposes, i.e., the firmware is completely open (and easily updated) and the board is cheap compared to other development hardware.
ACPI can work on ARM64 on a standard subset, SBSA compliant HW platforms.
If HW deviates from the SBSA standard (and PCI) it can't/won't be bootstrapped via ACPI and I do not think we should try to make it work.
Graeme has volunteered to carry this patch in the Linaro ERP release, and if this is all we can wish for, I am not going to debate it any further (and I deliberately kept it off the linux-acpi list for now). But I do think it is useful to have the discussion.
Yes it is and I personally think that allowing MCFG quirks did more harm than good to the ARM ecosystem, that's my opinion, we enabled them but as soon as they disappear the better.
Thanks, Lorenzo
-
Ard Biesheuvel -
Lorenzo Pieralisi