[PATCH v4 0/2] PCI: add support for firmware initialized DesignWare RCs

[PATCH v4 0/2] PCI: add support for firmware initialized DesignWare RCs

[Ard Biesheuvel]

UEFI based systems incorporating a Synopsys DesignWare PCIe controller
in RC mode will typically configure it before entering the OS. If this
configuration is fully static and ECAM compliant, there is no need to
expose particulars of the device to the OS, and we can simply describe
it as "pci-host-ecam-generic".

However, the Synopsys IP may be synthesized in a way where a quirk is
needed for config space accesses to the first bus. It makes little sense
to instantiate yet another pcie-designware driver that contains all the
low level setup code, so instead, add some quirks handling to the generic
ECAM driver.

v4: - merge with pci-host-generic
    - add Rob's ack to the DT binding doc

v3: - use SoC specific compatible strings
    - drop MSI patch [for now], since it turns out we may not need it

v2: - use dev->fwnode directly
    - replace an instance of pr_err with dev_err, and clarify the error message
    - fix Kconfig/Makefile dependency errors reported by kbuild

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Graeme Gregory <graeme.gregory@linaro.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Will Deacon <will.deacon@arm.com>

Ard Biesheuvel (2):
  PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM
    mode
  dt-bindings: designware: add binding for Designware PCIe in ECAM mode

 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 ++++++++++++++++++
 drivers/pci/host/pci-host-generic.c                            | 46 ++++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt

-- 
2.11.0

[PATCH v4 0/2] PCI: add support for firmware initialized DesignWare RCs

[Ard Biesheuvel]

UEFI based systems incorporating a Synopsys DesignWare PCIe controller
in RC mode will typically configure it before entering the OS. If this
configuration is fully static and ECAM compliant, there is no need to
expose particulars of the device to the OS, and we can simply describe
it as "pci-host-ecam-generic".

However, the Synopsys IP may be synthesized in a way where a quirk is
needed for config space accesses to the first bus. It makes little sense
to instantiate yet another pcie-designware driver that contains all the
low level setup code, so instead, add some quirks handling to the generic
ECAM driver.

v4: - merge with pci-host-generic
    - add Rob's ack to the DT binding doc

v3: - use SoC specific compatible strings
    - drop MSI patch [for now], since it turns out we may not need it

v2: - use dev->fwnode directly
    - replace an instance of pr_err with dev_err, and clarify the error message
    - fix Kconfig/Makefile dependency errors reported by kbuild

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Graeme Gregory <graeme.gregory@linaro.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Will Deacon <will.deacon@arm.com>

Ard Biesheuvel (2):
  PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM
    mode
  dt-bindings: designware: add binding for Designware PCIe in ECAM mode

 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 ++++++++++++++++++
 drivers/pci/host/pci-host-generic.c                            | 46 ++++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt

-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 0/2] PCI: add support for firmware initialized DesignWare RCs

[Ard Biesheuvel]

UEFI based systems incorporating a Synopsys DesignWare PCIe controller
in RC mode will typically configure it before entering the OS. If this
configuration is fully static and ECAM compliant, there is no need to
expose particulars of the device to the OS, and we can simply describe
it as "pci-host-ecam-generic".

However, the Synopsys IP may be synthesized in a way where a quirk is
needed for config space accesses to the first bus. It makes little sense
to instantiate yet another pcie-designware driver that contains all the
low level setup code, so instead, add some quirks handling to the generic
ECAM driver.

v4: - merge with pci-host-generic
    - add Rob's ack to the DT binding doc

v3: - use SoC specific compatible strings
    - drop MSI patch [for now], since it turns out we may not need it

v2: - use dev->fwnode directly
    - replace an instance of pr_err with dev_err, and clarify the error message
    - fix Kconfig/Makefile dependency errors reported by kbuild

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Graeme Gregory <graeme.gregory@linaro.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Will Deacon <will.deacon@arm.com>

Ard Biesheuvel (2):
  PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM
    mode
  dt-bindings: designware: add binding for Designware PCIe in ECAM mode

 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 ++++++++++++++++++
 drivers/pci/host/pci-host-generic.c                            | 46 ++++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt

-- 
2.11.0

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

Some implementations of the Synopsys DesignWare PCIe controller implement
a so-called ECAM shift mode, which allows a static memory window to be
configured that covers the configuration space of the entire bus range.

Usually, when the firmware performs all the low level configuration that
is required to expose this controller in a fully ECAM compatible manner,
we can simply describe it as "pci-host-ecam-generic" and be done with it.
However, in some cases (e.g., the Marvell Armada 80x0 as well as the
Socionext SynQuacer Soc), the IP was synthesized with an ATU window
granularity that does not allow the first bus to be mapped in a way that
prevents the device on the downstream port from appearing more than once,
and so we still need special handling in software to drive this static
almost-ECAM configuration.

So extend the pci-host-generic driver so it can support these controllers
as well, by adding special config space accessors that take the above
quirk into account.

Note that, unlike most drivers for this IP, this driver does not expose
a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
given that this is not a true bridge, and does not require any windows
to be configured in order for the downstream device to operate correctly.
Omitting it also prevents the PCI resource allocation routines from
handing out BAR space to it unnecessarily.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 7d709a7e0aa8..01e81a30e303 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
 	}
 };
 
+static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
+				   int size, u32 *val)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	/*
+	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
+	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
+	 * resulting in devices appearing multiple times on bus 0 unless we
+	 * filter out those accesses here.
+	 */
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return pci_generic_config_read(bus, devfn, where, size, val);
+}
+
+static int pci_dw_ecam_config_write(struct pci_bus *bus, u32 devfn, int where,
+				    int size, u32 val)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return pci_generic_config_write(bus, devfn, where, size, val);
+}
+
+static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_ecam_map_bus,
+		.read		= pci_dw_ecam_config_read,
+		.write		= pci_dw_ecam_config_write,
+	}
+};
+
 static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-cam-generic",
 	  .data = &gen_pci_cfg_cam_bus_ops },
@@ -42,6 +79,15 @@ static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-ecam-generic",
 	  .data = &pci_generic_ecam_ops },
 
+	{ .compatible = "marvell,armada8k-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "socionext,synquacer-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "snps,dw-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
 	{ },
 };
 
-- 
2.11.0

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

Some implementations of the Synopsys DesignWare PCIe controller implement
a so-called ECAM shift mode, which allows a static memory window to be
configured that covers the configuration space of the entire bus range.

Usually, when the firmware performs all the low level configuration that
is required to expose this controller in a fully ECAM compatible manner,
we can simply describe it as "pci-host-ecam-generic" and be done with it.
However, in some cases (e.g., the Marvell Armada 80x0 as well as the
Socionext SynQuacer Soc), the IP was synthesized with an ATU window
granularity that does not allow the first bus to be mapped in a way that
prevents the device on the downstream port from appearing more than once,
and so we still need special handling in software to drive this static
almost-ECAM configuration.

So extend the pci-host-generic driver so it can support these controllers
as well, by adding special config space accessors that take the above
quirk into account.

Note that, unlike most drivers for this IP, this driver does not expose
a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
given that this is not a true bridge, and does not require any windows
to be configured in order for the downstream device to operate correctly.
Omitting it also prevents the PCI resource allocation routines from
handing out BAR space to it unnecessarily.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 7d709a7e0aa8..01e81a30e303 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
 	}
 };
 
+static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
+				   int size, u32 *val)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	/*
+	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
+	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
+	 * resulting in devices appearing multiple times on bus 0 unless we
+	 * filter out those accesses here.
+	 */
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return pci_generic_config_read(bus, devfn, where, size, val);
+}
+
+static int pci_dw_ecam_config_write(struct pci_bus *bus, u32 devfn, int where,
+				    int size, u32 val)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return pci_generic_config_write(bus, devfn, where, size, val);
+}
+
+static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_ecam_map_bus,
+		.read		= pci_dw_ecam_config_read,
+		.write		= pci_dw_ecam_config_write,
+	}
+};
+
 static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-cam-generic",
 	  .data = &gen_pci_cfg_cam_bus_ops },
@@ -42,6 +79,15 @@ static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-ecam-generic",
 	  .data = &pci_generic_ecam_ops },
 
+	{ .compatible = "marvell,armada8k-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "socionext,synquacer-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "snps,dw-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
 	{ },
 };
 
-- 
2.11.0

[PATCH v4 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode

[Ard Biesheuvel]

Describe the binding for firmware-configured instances of the Synopsys
DesignWare PCIe controller in RC mode, that are almost but not quite
ECAM compliant.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 ++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
new file mode 100644
index 000000000000..515b2f9542e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
@@ -0,0 +1,42 @@
+* Synopsys DesignWare PCIe root complex in ECAM shift mode
+
+In some cases, firmware may already have configured the Synopsys DesignWare
+PCIe controller in RC mode with static ATU window mappings that cover all
+config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion.
+In this case, there is no need for the OS to perform any low level setup
+of clocks, PHYs or device registers, nor is there any reason for the driver
+to reconfigure ATU windows for config and/or IO space accesses at runtime.
+
+In cases where the IP was synthesized with a minimum ATU window size of
+64 KB, it cannot be supported by the generic ECAM driver, because it
+requires special config space accessors that filter accesses to device #1
+and beyond on the first bus.
+
+Required properties:
+- compatible: "marvell,armada8k-pcie-ecam" or
+              "socionext,synquacer-pcie-ecam" or
+              "snps,dw-pcie-ecam" (must be preceded by a more specific match)
+
+Please refer to the binding document of "pci-host-ecam-generic" in the
+file host-generic-pci.txt for a description of the remaining required
+and optional properties.
+
+Example:
+
+    pcie1: pcie@7f000000 {
+        compatible = "socionext,synquacer-pcie-ecam", "snps,dw-pcie-ecam";
+        device_type = "pci";
+        reg = <0x0 0x7f000000 0x0 0xf00000>;
+        bus-range = <0x0 0xe>;
+        #address-cells = <3>;
+        #size-cells = <2>;
+        ranges = <0x1000000 0x00 0x00010000 0x00 0x7ff00000 0x0 0x00010000>,
+                 <0x2000000 0x00 0x70000000 0x00 0x70000000 0x0 0x0f000000>,
+                 <0x3000000 0x3f 0x00000000 0x3f 0x00000000 0x1 0x00000000>;
+
+        #interrupt-cells = <0x1>;
+        interrupt-map-mask = <0x0 0x0 0x0 0x0>;
+        interrupt-map = <0x0 0x0 0x0 0x0 &gic 0x0 0x0 0x0 182 0x4>;
+        msi-map = <0x0 &its 0x0 0x10000>;
+        dma-coherent;
+    };
-- 
2.11.0

[PATCH v4 2/2] dt-bindings: designware: add binding for Designware PCIe in ECAM mode

[Ard Biesheuvel]

Describe the binding for firmware-configured instances of the Synopsys
DesignWare PCIe controller in RC mode, that are almost but not quite
ECAM compliant.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 42 ++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
new file mode 100644
index 000000000000..515b2f9542e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
@@ -0,0 +1,42 @@
+* Synopsys DesignWare PCIe root complex in ECAM shift mode
+
+In some cases, firmware may already have configured the Synopsys DesignWare
+PCIe controller in RC mode with static ATU window mappings that cover all
+config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion.
+In this case, there is no need for the OS to perform any low level setup
+of clocks, PHYs or device registers, nor is there any reason for the driver
+to reconfigure ATU windows for config and/or IO space accesses at runtime.
+
+In cases where the IP was synthesized with a minimum ATU window size of
+64 KB, it cannot be supported by the generic ECAM driver, because it
+requires special config space accessors that filter accesses to device #1
+and beyond on the first bus.
+
+Required properties:
+- compatible: "marvell,armada8k-pcie-ecam" or
+              "socionext,synquacer-pcie-ecam" or
+              "snps,dw-pcie-ecam" (must be preceded by a more specific match)
+
+Please refer to the binding document of "pci-host-ecam-generic" in the
+file host-generic-pci.txt for a description of the remaining required
+and optional properties.
+
+Example:
+
+    pcie1: pcie at 7f000000 {
+        compatible = "socionext,synquacer-pcie-ecam", "snps,dw-pcie-ecam";
+        device_type = "pci";
+        reg = <0x0 0x7f000000 0x0 0xf00000>;
+        bus-range = <0x0 0xe>;
+        #address-cells = <3>;
+        #size-cells = <2>;
+        ranges = <0x1000000 0x00 0x00010000 0x00 0x7ff00000 0x0 0x00010000>,
+                 <0x2000000 0x00 0x70000000 0x00 0x70000000 0x0 0x0f000000>,
+                 <0x3000000 0x3f 0x00000000 0x3f 0x00000000 0x1 0x00000000>;
+
+        #interrupt-cells = <0x1>;
+        interrupt-map-mask = <0x0 0x0 0x0 0x0>;
+        interrupt-map = <0x0 0x0 0x0 0x0 &gic 0x0 0x0 0x0 182 0x4>;
+        msi-map = <0x0 &its 0x0 0x10000>;
+        dma-coherent;
+    };
-- 
2.11.0

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Bjorn Helgaas]

On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> Some implementations of the Synopsys DesignWare PCIe controller implement
> a so-called ECAM shift mode, which allows a static memory window to be
> configured that covers the configuration space of the entire bus range.
> 
> Usually, when the firmware performs all the low level configuration that
> is required to expose this controller in a fully ECAM compatible manner,
> we can simply describe it as "pci-host-ecam-generic" and be done with it.
> However, in some cases (e.g., the Marvell Armada 80x0 as well as the
> Socionext SynQuacer Soc), the IP was synthesized with an ATU window
> granularity that does not allow the first bus to be mapped in a way that
> prevents the device on the downstream port from appearing more than once,
> and so we still need special handling in software to drive this static
> almost-ECAM configuration.
> 
> So extend the pci-host-generic driver so it can support these controllers
> as well, by adding special config space accessors that take the above
> quirk into account.
> 
> Note that, unlike most drivers for this IP, this driver does not expose
> a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
> given that this is not a true bridge, and does not require any windows
> to be configured in order for the downstream device to operate correctly.
> Omitting it also prevents the PCI resource allocation routines from
> handing out BAR space to it unnecessarily.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 7d709a7e0aa8..01e81a30e303 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>  	}
>  };
>  
> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> +				   int size, u32 *val)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +
> +	/*
> +	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> +	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
> +	 * resulting in devices appearing multiple times on bus 0 unless we
> +	 * filter out those accesses here.
> +	 */
> +	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;

I think we should return 0xffffffff data here, as we do in other
similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
rockchip_pcie_rd_conf()).

Actually, xilinx-nwl has a nice trick: it implements
nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
can use the generic accessors, and pci_generic_config_read() already
fills in ~0 for this case. 

What do you think of the following?  I put it on my pci/host-generic
branch for now (pending your response and an ack from Will).



commit 5aec78ea05fe23c7c17f8e6c757bc64fb9142728
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Fri Oct 6 17:39:18 2017 +0100

    PCI: generic: Add support for Synopsys DesignWare RC in ECAM mode
    
    Some implementations of the Synopsys DesignWare PCIe controller implement
    a so-called ECAM shift mode, which allows a static memory window to be
    configured that covers the configuration space of the entire bus range.
    
    Usually, when the firmware performs all the low level configuration that
    is required to expose this controller in a fully ECAM compatible manner,
    we can simply describe it as "pci-host-ecam-generic" and be done with it.
    However, in some cases (e.g., the Marvell Armada 80x0 as well as the
    Socionext SynQuacer Soc), the IP was synthesized with an ATU window
    granularity that does not allow the first bus to be mapped in a way that
    prevents the device on the downstream port from appearing more than once,
    and so we still need special handling in software to drive this static
    almost-ECAM configuration.
    
    So extend the pci-host-generic driver so it can support these controllers
    as well, by adding special config space accessors that take the above quirk
    into account.
    
    Note that, unlike most drivers for this IP, this driver does not expose a
    fake bridge device at B/D/F 00:00.0. There is no point in doing so, given
    that this is not a true bridge, and does not require any windows to be
    configured in order for the downstream device to operate correctly.
    Omitting it also prevents the PCI resource allocation routines from handing
    out BAR space to it unnecessarily.
    
    Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
    [bhelgaas: factor out pci_dw_valid_device(), add pci_dw_ecam_map_bus() and
    use generic read/write functions]
    Signed-off-by: Bjorn Helgaas <helgaas@kernel.org>

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 7d709a7e0aa8..2f05511ce718 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -35,6 +35,40 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
 	}
 };
 
+static bool pci_dw_valid_device(struct pci_bus *bus, unsigned int devfn)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	/*
+	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
+	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
+	 * resulting in devices appearing multiple times on bus 0 unless we
+	 * filter out those accesses here.
+	 */
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
+		return false;
+
+	return true;
+}
+
+static void __iomem *pci_dw_ecam_map_bus(struct pci_bus *bus,
+					 unsigned int devfn, int where)
+{
+	if (!pci_dw_valid_device(bus, devfn))
+		return NULL;
+
+	return pci_ecam_map_bus(bus, devfn, where);
+}
+
+static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_dw_ecam_map_bus,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
+};
+
 static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-cam-generic",
 	  .data = &gen_pci_cfg_cam_bus_ops },
@@ -42,6 +76,15 @@ static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-ecam-generic",
 	  .data = &pci_generic_ecam_ops },
 
+	{ .compatible = "marvell,armada8k-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "socionext,synquacer-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "snps,dw-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
 	{ },
 };
 

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Bjorn Helgaas]

On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> Some implementations of the Synopsys DesignWare PCIe controller implement
> a so-called ECAM shift mode, which allows a static memory window to be
> configured that covers the configuration space of the entire bus range.
> 
> Usually, when the firmware performs all the low level configuration that
> is required to expose this controller in a fully ECAM compatible manner,
> we can simply describe it as "pci-host-ecam-generic" and be done with it.
> However, in some cases (e.g., the Marvell Armada 80x0 as well as the
> Socionext SynQuacer Soc), the IP was synthesized with an ATU window
> granularity that does not allow the first bus to be mapped in a way that
> prevents the device on the downstream port from appearing more than once,
> and so we still need special handling in software to drive this static
> almost-ECAM configuration.
> 
> So extend the pci-host-generic driver so it can support these controllers
> as well, by adding special config space accessors that take the above
> quirk into account.
> 
> Note that, unlike most drivers for this IP, this driver does not expose
> a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
> given that this is not a true bridge, and does not require any windows
> to be configured in order for the downstream device to operate correctly.
> Omitting it also prevents the PCI resource allocation routines from
> handing out BAR space to it unnecessarily.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 7d709a7e0aa8..01e81a30e303 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>  	}
>  };
>  
> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> +				   int size, u32 *val)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +
> +	/*
> +	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> +	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
> +	 * resulting in devices appearing multiple times on bus 0 unless we
> +	 * filter out those accesses here.
> +	 */
> +	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;

I think we should return 0xffffffff data here, as we do in other
similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
rockchip_pcie_rd_conf()).

Actually, xilinx-nwl has a nice trick: it implements
nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
can use the generic accessors, and pci_generic_config_read() already
fills in ~0 for this case. 

What do you think of the following?  I put it on my pci/host-generic
branch for now (pending your response and an ack from Will).



commit 5aec78ea05fe23c7c17f8e6c757bc64fb9142728
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Fri Oct 6 17:39:18 2017 +0100

    PCI: generic: Add support for Synopsys DesignWare RC in ECAM mode
    
    Some implementations of the Synopsys DesignWare PCIe controller implement
    a so-called ECAM shift mode, which allows a static memory window to be
    configured that covers the configuration space of the entire bus range.
    
    Usually, when the firmware performs all the low level configuration that
    is required to expose this controller in a fully ECAM compatible manner,
    we can simply describe it as "pci-host-ecam-generic" and be done with it.
    However, in some cases (e.g., the Marvell Armada 80x0 as well as the
    Socionext SynQuacer Soc), the IP was synthesized with an ATU window
    granularity that does not allow the first bus to be mapped in a way that
    prevents the device on the downstream port from appearing more than once,
    and so we still need special handling in software to drive this static
    almost-ECAM configuration.
    
    So extend the pci-host-generic driver so it can support these controllers
    as well, by adding special config space accessors that take the above quirk
    into account.
    
    Note that, unlike most drivers for this IP, this driver does not expose a
    fake bridge device at B/D/F 00:00.0. There is no point in doing so, given
    that this is not a true bridge, and does not require any windows to be
    configured in order for the downstream device to operate correctly.
    Omitting it also prevents the PCI resource allocation routines from handing
    out BAR space to it unnecessarily.
    
    Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
    [bhelgaas: factor out pci_dw_valid_device(), add pci_dw_ecam_map_bus() and
    use generic read/write functions]
    Signed-off-by: Bjorn Helgaas <helgaas@kernel.org>

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 7d709a7e0aa8..2f05511ce718 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -35,6 +35,40 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
 	}
 };
 
+static bool pci_dw_valid_device(struct pci_bus *bus, unsigned int devfn)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	/*
+	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
+	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
+	 * resulting in devices appearing multiple times on bus 0 unless we
+	 * filter out those accesses here.
+	 */
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
+		return false;
+
+	return true;
+}
+
+static void __iomem *pci_dw_ecam_map_bus(struct pci_bus *bus,
+					 unsigned int devfn, int where)
+{
+	if (!pci_dw_valid_device(bus, devfn))
+		return NULL;
+
+	return pci_ecam_map_bus(bus, devfn, where);
+}
+
+static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_dw_ecam_map_bus,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
+};
+
 static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-cam-generic",
 	  .data = &gen_pci_cfg_cam_bus_ops },
@@ -42,6 +76,15 @@ static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-ecam-generic",
 	  .data = &pci_generic_ecam_ops },
 
+	{ .compatible = "marvell,armada8k-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "socionext,synquacer-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "snps,dw-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
 	{ },
 };
 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Bjorn Helgaas]

On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> Some implementations of the Synopsys DesignWare PCIe controller implement
> a so-called ECAM shift mode, which allows a static memory window to be
> configured that covers the configuration space of the entire bus range.
> 
> Usually, when the firmware performs all the low level configuration that
> is required to expose this controller in a fully ECAM compatible manner,
> we can simply describe it as "pci-host-ecam-generic" and be done with it.
> However, in some cases (e.g., the Marvell Armada 80x0 as well as the
> Socionext SynQuacer Soc), the IP was synthesized with an ATU window
> granularity that does not allow the first bus to be mapped in a way that
> prevents the device on the downstream port from appearing more than once,
> and so we still need special handling in software to drive this static
> almost-ECAM configuration.
> 
> So extend the pci-host-generic driver so it can support these controllers
> as well, by adding special config space accessors that take the above
> quirk into account.
> 
> Note that, unlike most drivers for this IP, this driver does not expose
> a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
> given that this is not a true bridge, and does not require any windows
> to be configured in order for the downstream device to operate correctly.
> Omitting it also prevents the PCI resource allocation routines from
> handing out BAR space to it unnecessarily.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 7d709a7e0aa8..01e81a30e303 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>  	}
>  };
>  
> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> +				   int size, u32 *val)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +
> +	/*
> +	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> +	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
> +	 * resulting in devices appearing multiple times on bus 0 unless we
> +	 * filter out those accesses here.
> +	 */
> +	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;

I think we should return 0xffffffff data here, as we do in other
similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
rockchip_pcie_rd_conf()).

Actually, xilinx-nwl has a nice trick: it implements
nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
can use the generic accessors, and pci_generic_config_read() already
fills in ~0 for this case. 

What do you think of the following?  I put it on my pci/host-generic
branch for now (pending your response and an ack from Will).



commit 5aec78ea05fe23c7c17f8e6c757bc64fb9142728
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Fri Oct 6 17:39:18 2017 +0100

    PCI: generic: Add support for Synopsys DesignWare RC in ECAM mode
    
    Some implementations of the Synopsys DesignWare PCIe controller implement
    a so-called ECAM shift mode, which allows a static memory window to be
    configured that covers the configuration space of the entire bus range.
    
    Usually, when the firmware performs all the low level configuration that
    is required to expose this controller in a fully ECAM compatible manner,
    we can simply describe it as "pci-host-ecam-generic" and be done with it.
    However, in some cases (e.g., the Marvell Armada 80x0 as well as the
    Socionext SynQuacer Soc), the IP was synthesized with an ATU window
    granularity that does not allow the first bus to be mapped in a way that
    prevents the device on the downstream port from appearing more than once,
    and so we still need special handling in software to drive this static
    almost-ECAM configuration.
    
    So extend the pci-host-generic driver so it can support these controllers
    as well, by adding special config space accessors that take the above quirk
    into account.
    
    Note that, unlike most drivers for this IP, this driver does not expose a
    fake bridge device at B/D/F 00:00.0. There is no point in doing so, given
    that this is not a true bridge, and does not require any windows to be
    configured in order for the downstream device to operate correctly.
    Omitting it also prevents the PCI resource allocation routines from handing
    out BAR space to it unnecessarily.
    
    Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
    [bhelgaas: factor out pci_dw_valid_device(), add pci_dw_ecam_map_bus() and
    use generic read/write functions]
    Signed-off-by: Bjorn Helgaas <helgaas@kernel.org>

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 7d709a7e0aa8..2f05511ce718 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -35,6 +35,40 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
 	}
 };
 
+static bool pci_dw_valid_device(struct pci_bus *bus, unsigned int devfn)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+
+	/*
+	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
+	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
+	 * resulting in devices appearing multiple times on bus 0 unless we
+	 * filter out those accesses here.
+	 */
+	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
+		return false;
+
+	return true;
+}
+
+static void __iomem *pci_dw_ecam_map_bus(struct pci_bus *bus,
+					 unsigned int devfn, int where)
+{
+	if (!pci_dw_valid_device(bus, devfn))
+		return NULL;
+
+	return pci_ecam_map_bus(bus, devfn, where);
+}
+
+static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_dw_ecam_map_bus,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
+};
+
 static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-cam-generic",
 	  .data = &gen_pci_cfg_cam_bus_ops },
@@ -42,6 +76,15 @@ static const struct of_device_id gen_pci_of_match[] = {
 	{ .compatible = "pci-host-ecam-generic",
 	  .data = &pci_generic_ecam_ops },
 
+	{ .compatible = "marvell,armada8k-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "socionext,synquacer-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
+	{ .compatible = "snps,dw-pcie-ecam",
+	  .data = &pci_dw_ecam_bus_ops },
+
 	{ },
 };
 

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 7 October 2017 at 00:21, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> Some implementations of the Synopsys DesignWare PCIe controller implement
>> a so-called ECAM shift mode, which allows a static memory window to be
>> configured that covers the configuration space of the entire bus range.
>>
>> Usually, when the firmware performs all the low level configuration that
>> is required to expose this controller in a fully ECAM compatible manner,
>> we can simply describe it as "pci-host-ecam-generic" and be done with it.
>> However, in some cases (e.g., the Marvell Armada 80x0 as well as the
>> Socionext SynQuacer Soc), the IP was synthesized with an ATU window
>> granularity that does not allow the first bus to be mapped in a way that
>> prevents the device on the downstream port from appearing more than once,
>> and so we still need special handling in software to drive this static
>> almost-ECAM configuration.
>>
>> So extend the pci-host-generic driver so it can support these controllers
>> as well, by adding special config space accessors that take the above
>> quirk into account.
>>
>> Note that, unlike most drivers for this IP, this driver does not expose
>> a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
>> given that this is not a true bridge, and does not require any windows
>> to be configured in order for the downstream device to operate correctly.
>> Omitting it also prevents the PCI resource allocation routines from
>> handing out BAR space to it unnecessarily.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> index 7d709a7e0aa8..01e81a30e303 100644
>> --- a/drivers/pci/host/pci-host-generic.c
>> +++ b/drivers/pci/host/pci-host-generic.c
>> @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>>       }
>>  };
>>
>> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> +                                int size, u32 *val)
>> +{
>> +     struct pci_config_window *cfg = bus->sysdata;
>> +
>> +     /*
>> +      * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> +      * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> +      * resulting in devices appearing multiple times on bus 0 unless we
>> +      * filter out those accesses here.
>> +      */
>> +     if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>
> I think we should return 0xffffffff data here, as we do in other
> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> rockchip_pcie_rd_conf()).
>
> Actually, xilinx-nwl has a nice trick: it implements
> nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> can use the generic accessors, and pci_generic_config_read() already
> fills in ~0 for this case.
>
> What do you think of the following?  I put it on my pci/host-generic
> branch for now (pending your response and an ack from Will).
>

Thanks Bjorn, that does look better, and it works fine on my system.

Regards,
Ard.


>
>
> commit 5aec78ea05fe23c7c17f8e6c757bc64fb9142728
> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date:   Fri Oct 6 17:39:18 2017 +0100
>
>     PCI: generic: Add support for Synopsys DesignWare RC in ECAM mode
>
>     Some implementations of the Synopsys DesignWare PCIe controller implement
>     a so-called ECAM shift mode, which allows a static memory window to be
>     configured that covers the configuration space of the entire bus range.
>
>     Usually, when the firmware performs all the low level configuration that
>     is required to expose this controller in a fully ECAM compatible manner,
>     we can simply describe it as "pci-host-ecam-generic" and be done with it.
>     However, in some cases (e.g., the Marvell Armada 80x0 as well as the
>     Socionext SynQuacer Soc), the IP was synthesized with an ATU window
>     granularity that does not allow the first bus to be mapped in a way that
>     prevents the device on the downstream port from appearing more than once,
>     and so we still need special handling in software to drive this static
>     almost-ECAM configuration.
>
>     So extend the pci-host-generic driver so it can support these controllers
>     as well, by adding special config space accessors that take the above quirk
>     into account.
>
>     Note that, unlike most drivers for this IP, this driver does not expose a
>     fake bridge device at B/D/F 00:00.0. There is no point in doing so, given
>     that this is not a true bridge, and does not require any windows to be
>     configured in order for the downstream device to operate correctly.
>     Omitting it also prevents the PCI resource allocation routines from handing
>     out BAR space to it unnecessarily.
>
>     Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>     [bhelgaas: factor out pci_dw_valid_device(), add pci_dw_ecam_map_bus() and
>     use generic read/write functions]
>     Signed-off-by: Bjorn Helgaas <helgaas@kernel.org>
>
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 7d709a7e0aa8..2f05511ce718 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -35,6 +35,40 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>         }
>  };
>
> +static bool pci_dw_valid_device(struct pci_bus *bus, unsigned int devfn)
> +{
> +       struct pci_config_window *cfg = bus->sysdata;
> +
> +       /*
> +        * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> +        * type 0 config TLPs sent to devices 1 and up on its downstream port,
> +        * resulting in devices appearing multiple times on bus 0 unless we
> +        * filter out those accesses here.
> +        */
> +       if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> +               return false;
> +
> +       return true;
> +}
> +
> +static void __iomem *pci_dw_ecam_map_bus(struct pci_bus *bus,
> +                                        unsigned int devfn, int where)
> +{
> +       if (!pci_dw_valid_device(bus, devfn))
> +               return NULL;
> +
> +       return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
> +       .bus_shift      = 20,
> +       .pci_ops        = {
> +               .map_bus        = pci_dw_ecam_map_bus,
> +               .read           = pci_generic_config_read,
> +               .write          = pci_generic_config_write,
> +       }
> +};
> +
>  static const struct of_device_id gen_pci_of_match[] = {
>         { .compatible = "pci-host-cam-generic",
>           .data = &gen_pci_cfg_cam_bus_ops },
> @@ -42,6 +76,15 @@ static const struct of_device_id gen_pci_of_match[] = {
>         { .compatible = "pci-host-ecam-generic",
>           .data = &pci_generic_ecam_ops },
>
> +       { .compatible = "marvell,armada8k-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
> +       { .compatible = "socionext,synquacer-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
> +       { .compatible = "snps,dw-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
>         { },
>  };
>

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 7 October 2017 at 00:21, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> Some implementations of the Synopsys DesignWare PCIe controller implement
>> a so-called ECAM shift mode, which allows a static memory window to be
>> configured that covers the configuration space of the entire bus range.
>>
>> Usually, when the firmware performs all the low level configuration that
>> is required to expose this controller in a fully ECAM compatible manner,
>> we can simply describe it as "pci-host-ecam-generic" and be done with it.
>> However, in some cases (e.g., the Marvell Armada 80x0 as well as the
>> Socionext SynQuacer Soc), the IP was synthesized with an ATU window
>> granularity that does not allow the first bus to be mapped in a way that
>> prevents the device on the downstream port from appearing more than once,
>> and so we still need special handling in software to drive this static
>> almost-ECAM configuration.
>>
>> So extend the pci-host-generic driver so it can support these controllers
>> as well, by adding special config space accessors that take the above
>> quirk into account.
>>
>> Note that, unlike most drivers for this IP, this driver does not expose
>> a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
>> given that this is not a true bridge, and does not require any windows
>> to be configured in order for the downstream device to operate correctly.
>> Omitting it also prevents the PCI resource allocation routines from
>> handing out BAR space to it unnecessarily.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> index 7d709a7e0aa8..01e81a30e303 100644
>> --- a/drivers/pci/host/pci-host-generic.c
>> +++ b/drivers/pci/host/pci-host-generic.c
>> @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>>       }
>>  };
>>
>> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> +                                int size, u32 *val)
>> +{
>> +     struct pci_config_window *cfg = bus->sysdata;
>> +
>> +     /*
>> +      * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> +      * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> +      * resulting in devices appearing multiple times on bus 0 unless we
>> +      * filter out those accesses here.
>> +      */
>> +     if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>
> I think we should return 0xffffffff data here, as we do in other
> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> rockchip_pcie_rd_conf()).
>
> Actually, xilinx-nwl has a nice trick: it implements
> nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> can use the generic accessors, and pci_generic_config_read() already
> fills in ~0 for this case.
>
> What do you think of the following?  I put it on my pci/host-generic
> branch for now (pending your response and an ack from Will).
>

Thanks Bjorn, that does look better, and it works fine on my system.

Regards,
Ard.


>
>
> commit 5aec78ea05fe23c7c17f8e6c757bc64fb9142728
> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date:   Fri Oct 6 17:39:18 2017 +0100
>
>     PCI: generic: Add support for Synopsys DesignWare RC in ECAM mode
>
>     Some implementations of the Synopsys DesignWare PCIe controller implement
>     a so-called ECAM shift mode, which allows a static memory window to be
>     configured that covers the configuration space of the entire bus range.
>
>     Usually, when the firmware performs all the low level configuration that
>     is required to expose this controller in a fully ECAM compatible manner,
>     we can simply describe it as "pci-host-ecam-generic" and be done with it.
>     However, in some cases (e.g., the Marvell Armada 80x0 as well as the
>     Socionext SynQuacer Soc), the IP was synthesized with an ATU window
>     granularity that does not allow the first bus to be mapped in a way that
>     prevents the device on the downstream port from appearing more than once,
>     and so we still need special handling in software to drive this static
>     almost-ECAM configuration.
>
>     So extend the pci-host-generic driver so it can support these controllers
>     as well, by adding special config space accessors that take the above quirk
>     into account.
>
>     Note that, unlike most drivers for this IP, this driver does not expose a
>     fake bridge device at B/D/F 00:00.0. There is no point in doing so, given
>     that this is not a true bridge, and does not require any windows to be
>     configured in order for the downstream device to operate correctly.
>     Omitting it also prevents the PCI resource allocation routines from handing
>     out BAR space to it unnecessarily.
>
>     Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>     [bhelgaas: factor out pci_dw_valid_device(), add pci_dw_ecam_map_bus() and
>     use generic read/write functions]
>     Signed-off-by: Bjorn Helgaas <helgaas@kernel.org>
>
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 7d709a7e0aa8..2f05511ce718 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -35,6 +35,40 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>         }
>  };
>
> +static bool pci_dw_valid_device(struct pci_bus *bus, unsigned int devfn)
> +{
> +       struct pci_config_window *cfg = bus->sysdata;
> +
> +       /*
> +        * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> +        * type 0 config TLPs sent to devices 1 and up on its downstream port,
> +        * resulting in devices appearing multiple times on bus 0 unless we
> +        * filter out those accesses here.
> +        */
> +       if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> +               return false;
> +
> +       return true;
> +}
> +
> +static void __iomem *pci_dw_ecam_map_bus(struct pci_bus *bus,
> +                                        unsigned int devfn, int where)
> +{
> +       if (!pci_dw_valid_device(bus, devfn))
> +               return NULL;
> +
> +       return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
> +       .bus_shift      = 20,
> +       .pci_ops        = {
> +               .map_bus        = pci_dw_ecam_map_bus,
> +               .read           = pci_generic_config_read,
> +               .write          = pci_generic_config_write,
> +       }
> +};
> +
>  static const struct of_device_id gen_pci_of_match[] = {
>         { .compatible = "pci-host-cam-generic",
>           .data = &gen_pci_cfg_cam_bus_ops },
> @@ -42,6 +76,15 @@ static const struct of_device_id gen_pci_of_match[] = {
>         { .compatible = "pci-host-ecam-generic",
>           .data = &pci_generic_ecam_ops },
>
> +       { .compatible = "marvell,armada8k-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
> +       { .compatible = "socionext,synquacer-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
> +       { .compatible = "snps,dw-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
>         { },
>  };
>

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 7 October 2017 at 00:21, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> Some implementations of the Synopsys DesignWare PCIe controller implement
>> a so-called ECAM shift mode, which allows a static memory window to be
>> configured that covers the configuration space of the entire bus range.
>>
>> Usually, when the firmware performs all the low level configuration that
>> is required to expose this controller in a fully ECAM compatible manner,
>> we can simply describe it as "pci-host-ecam-generic" and be done with it.
>> However, in some cases (e.g., the Marvell Armada 80x0 as well as the
>> Socionext SynQuacer Soc), the IP was synthesized with an ATU window
>> granularity that does not allow the first bus to be mapped in a way that
>> prevents the device on the downstream port from appearing more than once,
>> and so we still need special handling in software to drive this static
>> almost-ECAM configuration.
>>
>> So extend the pci-host-generic driver so it can support these controllers
>> as well, by adding special config space accessors that take the above
>> quirk into account.
>>
>> Note that, unlike most drivers for this IP, this driver does not expose
>> a fake bridge device at B/D/F 00:00.0. There is no point in doing so,
>> given that this is not a true bridge, and does not require any windows
>> to be configured in order for the downstream device to operate correctly.
>> Omitting it also prevents the PCI resource allocation routines from
>> handing out BAR space to it unnecessarily.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/pci/host/pci-host-generic.c | 46 ++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> index 7d709a7e0aa8..01e81a30e303 100644
>> --- a/drivers/pci/host/pci-host-generic.c
>> +++ b/drivers/pci/host/pci-host-generic.c
>> @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>>       }
>>  };
>>
>> +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> +                                int size, u32 *val)
>> +{
>> +     struct pci_config_window *cfg = bus->sysdata;
>> +
>> +     /*
>> +      * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> +      * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> +      * resulting in devices appearing multiple times on bus 0 unless we
>> +      * filter out those accesses here.
>> +      */
>> +     if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>
> I think we should return 0xffffffff data here, as we do in other
> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> rockchip_pcie_rd_conf()).
>
> Actually, xilinx-nwl has a nice trick: it implements
> nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> can use the generic accessors, and pci_generic_config_read() already
> fills in ~0 for this case.
>
> What do you think of the following?  I put it on my pci/host-generic
> branch for now (pending your response and an ack from Will).
>

Thanks Bjorn, that does look better, and it works fine on my system.

Regards,
Ard.


>
>
> commit 5aec78ea05fe23c7c17f8e6c757bc64fb9142728
> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date:   Fri Oct 6 17:39:18 2017 +0100
>
>     PCI: generic: Add support for Synopsys DesignWare RC in ECAM mode
>
>     Some implementations of the Synopsys DesignWare PCIe controller implement
>     a so-called ECAM shift mode, which allows a static memory window to be
>     configured that covers the configuration space of the entire bus range.
>
>     Usually, when the firmware performs all the low level configuration that
>     is required to expose this controller in a fully ECAM compatible manner,
>     we can simply describe it as "pci-host-ecam-generic" and be done with it.
>     However, in some cases (e.g., the Marvell Armada 80x0 as well as the
>     Socionext SynQuacer Soc), the IP was synthesized with an ATU window
>     granularity that does not allow the first bus to be mapped in a way that
>     prevents the device on the downstream port from appearing more than once,
>     and so we still need special handling in software to drive this static
>     almost-ECAM configuration.
>
>     So extend the pci-host-generic driver so it can support these controllers
>     as well, by adding special config space accessors that take the above quirk
>     into account.
>
>     Note that, unlike most drivers for this IP, this driver does not expose a
>     fake bridge device at B/D/F 00:00.0. There is no point in doing so, given
>     that this is not a true bridge, and does not require any windows to be
>     configured in order for the downstream device to operate correctly.
>     Omitting it also prevents the PCI resource allocation routines from handing
>     out BAR space to it unnecessarily.
>
>     Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>     [bhelgaas: factor out pci_dw_valid_device(), add pci_dw_ecam_map_bus() and
>     use generic read/write functions]
>     Signed-off-by: Bjorn Helgaas <helgaas@kernel.org>
>
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 7d709a7e0aa8..2f05511ce718 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -35,6 +35,40 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>         }
>  };
>
> +static bool pci_dw_valid_device(struct pci_bus *bus, unsigned int devfn)
> +{
> +       struct pci_config_window *cfg = bus->sysdata;
> +
> +       /*
> +        * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> +        * type 0 config TLPs sent to devices 1 and up on its downstream port,
> +        * resulting in devices appearing multiple times on bus 0 unless we
> +        * filter out those accesses here.
> +        */
> +       if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> +               return false;
> +
> +       return true;
> +}
> +
> +static void __iomem *pci_dw_ecam_map_bus(struct pci_bus *bus,
> +                                        unsigned int devfn, int where)
> +{
> +       if (!pci_dw_valid_device(bus, devfn))
> +               return NULL;
> +
> +       return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +static struct pci_ecam_ops pci_dw_ecam_bus_ops = {
> +       .bus_shift      = 20,
> +       .pci_ops        = {
> +               .map_bus        = pci_dw_ecam_map_bus,
> +               .read           = pci_generic_config_read,
> +               .write          = pci_generic_config_write,
> +       }
> +};
> +
>  static const struct of_device_id gen_pci_of_match[] = {
>         { .compatible = "pci-host-cam-generic",
>           .data = &gen_pci_cfg_cam_bus_ops },
> @@ -42,6 +76,15 @@ static const struct of_device_id gen_pci_of_match[] = {
>         { .compatible = "pci-host-ecam-generic",
>           .data = &pci_generic_ecam_ops },
>
> +       { .compatible = "marvell,armada8k-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
> +       { .compatible = "socionext,synquacer-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
> +       { .compatible = "snps,dw-pcie-ecam",
> +         .data = &pci_dw_ecam_bus_ops },
> +
>         { },
>  };
>

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Will Deacon]

On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> > index 7d709a7e0aa8..01e81a30e303 100644
> > --- a/drivers/pci/host/pci-host-generic.c
> > +++ b/drivers/pci/host/pci-host-generic.c
> > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> >  	}
> >  };
> >  
> > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> > +				   int size, u32 *val)
> > +{
> > +	struct pci_config_window *cfg = bus->sysdata;
> > +
> > +	/*
> > +	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> > +	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
> > +	 * resulting in devices appearing multiple times on bus 0 unless we
> > +	 * filter out those accesses here.
> > +	 */
> > +	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> 
> I think we should return 0xffffffff data here, as we do in other
> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> rockchip_pcie_rd_conf()).
> 
> Actually, xilinx-nwl has a nice trick: it implements
> nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> can use the generic accessors, and pci_generic_config_read() already
> fills in ~0 for this case. 
> 
> What do you think of the following?  I put it on my pci/host-generic
> branch for now (pending your response and an ack from Will).

I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
but either way:

Acked-by: Will Deacon <will.deacon@arm.com>

I'm assuming this is so small that there's no point in having a Kconfig
option for these guys that depends on the generic host driver?

Will

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Will Deacon]

On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> > index 7d709a7e0aa8..01e81a30e303 100644
> > --- a/drivers/pci/host/pci-host-generic.c
> > +++ b/drivers/pci/host/pci-host-generic.c
> > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> >  	}
> >  };
> >  
> > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> > +				   int size, u32 *val)
> > +{
> > +	struct pci_config_window *cfg = bus->sysdata;
> > +
> > +	/*
> > +	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> > +	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
> > +	 * resulting in devices appearing multiple times on bus 0 unless we
> > +	 * filter out those accesses here.
> > +	 */
> > +	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> 
> I think we should return 0xffffffff data here, as we do in other
> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> rockchip_pcie_rd_conf()).
> 
> Actually, xilinx-nwl has a nice trick: it implements
> nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> can use the generic accessors, and pci_generic_config_read() already
> fills in ~0 for this case. 
> 
> What do you think of the following?  I put it on my pci/host-generic
> branch for now (pending your response and an ack from Will).

I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
but either way:

Acked-by: Will Deacon <will.deacon@arm.com>

I'm assuming this is so small that there's no point in having a Kconfig
option for these guys that depends on the generic host driver?

Will

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 9 October 2017 at 17:46, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
>> On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> > index 7d709a7e0aa8..01e81a30e303 100644
>> > --- a/drivers/pci/host/pci-host-generic.c
>> > +++ b/drivers/pci/host/pci-host-generic.c
>> > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>> >     }
>> >  };
>> >
>> > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> > +                              int size, u32 *val)
>> > +{
>> > +   struct pci_config_window *cfg = bus->sysdata;
>> > +
>> > +   /*
>> > +    * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> > +    * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> > +    * resulting in devices appearing multiple times on bus 0 unless we
>> > +    * filter out those accesses here.
>> > +    */
>> > +   if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> > +           return PCIBIOS_DEVICE_NOT_FOUND;
>>
>> I think we should return 0xffffffff data here, as we do in other
>> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
>> rockchip_pcie_rd_conf()).
>>
>> Actually, xilinx-nwl has a nice trick: it implements
>> nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
>> can use the generic accessors, and pci_generic_config_read() already
>> fills in ~0 for this case.
>>
>> What do you think of the following?  I put it on my pci/host-generic
>> branch for now (pending your response and an ack from Will).
>
> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> but either way:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>

Thanks

> I'm assuming this is so small that there's no point in having a Kconfig
> option for these guys that depends on the generic host driver?
>

I can make the compatible strings shorter if you like?

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 9 October 2017 at 17:46, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
>> On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> > index 7d709a7e0aa8..01e81a30e303 100644
>> > --- a/drivers/pci/host/pci-host-generic.c
>> > +++ b/drivers/pci/host/pci-host-generic.c
>> > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>> >     }
>> >  };
>> >
>> > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> > +                              int size, u32 *val)
>> > +{
>> > +   struct pci_config_window *cfg = bus->sysdata;
>> > +
>> > +   /*
>> > +    * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> > +    * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> > +    * resulting in devices appearing multiple times on bus 0 unless we
>> > +    * filter out those accesses here.
>> > +    */
>> > +   if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> > +           return PCIBIOS_DEVICE_NOT_FOUND;
>>
>> I think we should return 0xffffffff data here, as we do in other
>> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
>> rockchip_pcie_rd_conf()).
>>
>> Actually, xilinx-nwl has a nice trick: it implements
>> nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
>> can use the generic accessors, and pci_generic_config_read() already
>> fills in ~0 for this case.
>>
>> What do you think of the following?  I put it on my pci/host-generic
>> branch for now (pending your response and an ack from Will).
>
> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> but either way:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>

Thanks

> I'm assuming this is so small that there's no point in having a Kconfig
> option for these guys that depends on the generic host driver?
>

I can make the compatible strings shorter if you like?

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 9 October 2017 at 17:46, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
>> On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> > index 7d709a7e0aa8..01e81a30e303 100644
>> > --- a/drivers/pci/host/pci-host-generic.c
>> > +++ b/drivers/pci/host/pci-host-generic.c
>> > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>> >     }
>> >  };
>> >
>> > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> > +                              int size, u32 *val)
>> > +{
>> > +   struct pci_config_window *cfg = bus->sysdata;
>> > +
>> > +   /*
>> > +    * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> > +    * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> > +    * resulting in devices appearing multiple times on bus 0 unless we
>> > +    * filter out those accesses here.
>> > +    */
>> > +   if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> > +           return PCIBIOS_DEVICE_NOT_FOUND;
>>
>> I think we should return 0xffffffff data here, as we do in other
>> similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
>> rockchip_pcie_rd_conf()).
>>
>> Actually, xilinx-nwl has a nice trick: it implements
>> nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
>> can use the generic accessors, and pci_generic_config_read() already
>> fills in ~0 for this case.
>>
>> What do you think of the following?  I put it on my pci/host-generic
>> branch for now (pending your response and an ack from Will).
>
> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> but either way:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>

Thanks

> I'm assuming this is so small that there's no point in having a Kconfig
> option for these guys that depends on the generic host driver?
>

I can make the compatible strings shorter if you like?

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Bjorn Helgaas]

On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> > > index 7d709a7e0aa8..01e81a30e303 100644
> > > --- a/drivers/pci/host/pci-host-generic.c
> > > +++ b/drivers/pci/host/pci-host-generic.c
> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> > >  	}
> > >  };
> > >  
> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> > > +				   int size, u32 *val)
> > > +{
> > > +	struct pci_config_window *cfg = bus->sysdata;
> > > +
> > > +	/*
> > > +	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> > > +	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
> > > +	 * resulting in devices appearing multiple times on bus 0 unless we
> > > +	 * filter out those accesses here.
> > > +	 */
> > > +	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> > > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > 
> > I think we should return 0xffffffff data here, as we do in other
> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> > rockchip_pcie_rd_conf()).
> > 
> > Actually, xilinx-nwl has a nice trick: it implements
> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> > can use the generic accessors, and pci_generic_config_read() already
> > fills in ~0 for this case. 
> > 
> > What do you think of the following?  I put it on my pci/host-generic
> > branch for now (pending your response and an ack from Will).
> 
> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> but either way:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>

Thanks.  The reason I split out pci_dw_valid_device() is because there are
several other *_valid_device() functions in other drivers that do basically
the same thing, and I think it's useful to be able to see that they're all
similar, as opposed to being buried in the config accessor or map
functions, which vary a little more.

> I'm assuming this is so small that there's no point in having a Kconfig
> option for these guys that depends on the generic host driver?

Good question; I don't care either way.

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Bjorn Helgaas]

On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> > > index 7d709a7e0aa8..01e81a30e303 100644
> > > --- a/drivers/pci/host/pci-host-generic.c
> > > +++ b/drivers/pci/host/pci-host-generic.c
> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> > >  	}
> > >  };
> > >  
> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> > > +				   int size, u32 *val)
> > > +{
> > > +	struct pci_config_window *cfg = bus->sysdata;
> > > +
> > > +	/*
> > > +	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> > > +	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
> > > +	 * resulting in devices appearing multiple times on bus 0 unless we
> > > +	 * filter out those accesses here.
> > > +	 */
> > > +	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> > > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > 
> > I think we should return 0xffffffff data here, as we do in other
> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> > rockchip_pcie_rd_conf()).
> > 
> > Actually, xilinx-nwl has a nice trick: it implements
> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> > can use the generic accessors, and pci_generic_config_read() already
> > fills in ~0 for this case. 
> > 
> > What do you think of the following?  I put it on my pci/host-generic
> > branch for now (pending your response and an ack from Will).
> 
> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> but either way:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>

Thanks.  The reason I split out pci_dw_valid_device() is because there are
several other *_valid_device() functions in other drivers that do basically
the same thing, and I think it's useful to be able to see that they're all
similar, as opposed to being buried in the config accessor or map
functions, which vary a little more.

> I'm assuming this is so small that there's no point in having a Kconfig
> option for these guys that depends on the generic host driver?

Good question; I don't care either way.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Bjorn Helgaas]

On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> > > index 7d709a7e0aa8..01e81a30e303 100644
> > > --- a/drivers/pci/host/pci-host-generic.c
> > > +++ b/drivers/pci/host/pci-host-generic.c
> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> > >  	}
> > >  };
> > >  
> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> > > +				   int size, u32 *val)
> > > +{
> > > +	struct pci_config_window *cfg = bus->sysdata;
> > > +
> > > +	/*
> > > +	 * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> > > +	 * type 0 config TLPs sent to devices 1 and up on its downstream port,
> > > +	 * resulting in devices appearing multiple times on bus 0 unless we
> > > +	 * filter out those accesses here.
> > > +	 */
> > > +	if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> > > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > 
> > I think we should return 0xffffffff data here, as we do in other
> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> > rockchip_pcie_rd_conf()).
> > 
> > Actually, xilinx-nwl has a nice trick: it implements
> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> > can use the generic accessors, and pci_generic_config_read() already
> > fills in ~0 for this case. 
> > 
> > What do you think of the following?  I put it on my pci/host-generic
> > branch for now (pending your response and an ack from Will).
> 
> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> but either way:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>

Thanks.  The reason I split out pci_dw_valid_device() is because there are
several other *_valid_device() functions in other drivers that do basically
the same thing, and I think it's useful to be able to see that they're all
similar, as opposed to being buried in the config accessor or map
functions, which vary a little more.

> I'm assuming this is so small that there's no point in having a Kconfig
> option for these guys that depends on the generic host driver?

Good question; I don't care either way.

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 10 October 2017 at 01:13, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
>> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
>> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> > > index 7d709a7e0aa8..01e81a30e303 100644
>> > > --- a/drivers/pci/host/pci-host-generic.c
>> > > +++ b/drivers/pci/host/pci-host-generic.c
>> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>> > >   }
>> > >  };
>> > >
>> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> > > +                            int size, u32 *val)
>> > > +{
>> > > + struct pci_config_window *cfg = bus->sysdata;
>> > > +
>> > > + /*
>> > > +  * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> > > +  * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> > > +  * resulting in devices appearing multiple times on bus 0 unless we
>> > > +  * filter out those accesses here.
>> > > +  */
>> > > + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> > > +         return PCIBIOS_DEVICE_NOT_FOUND;
>> >
>> > I think we should return 0xffffffff data here, as we do in other
>> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
>> > rockchip_pcie_rd_conf()).
>> >
>> > Actually, xilinx-nwl has a nice trick: it implements
>> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
>> > can use the generic accessors, and pci_generic_config_read() already
>> > fills in ~0 for this case.
>> >
>> > What do you think of the following?  I put it on my pci/host-generic
>> > branch for now (pending your response and an ack from Will).
>>
>> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
>> but either way:
>>
>> Acked-by: Will Deacon <will.deacon@arm.com>
>
> Thanks.  The reason I split out pci_dw_valid_device() is because there are
> several other *_valid_device() functions in other drivers that do basically
> the same thing, and I think it's useful to be able to see that they're all
> similar, as opposed to being buried in the config accessor or map
> functions, which vary a little more.
>
>> I'm assuming this is so small that there's no point in having a Kconfig
>> option for these guys that depends on the generic host driver?
>
> Good question; I don't care either way.

In this case, may I suggest we leave the patch as is? Neither of you
seem to mind, and it will make /my/ life much easier, given that I
don't have to chase distros to get this driver enabled in their kernel
configs (unless we make if 'def_bool y', in which case it makes even
less sense to add the Kconfig option in the first place).

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 10 October 2017 at 01:13, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
>> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
>> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> > > index 7d709a7e0aa8..01e81a30e303 100644
>> > > --- a/drivers/pci/host/pci-host-generic.c
>> > > +++ b/drivers/pci/host/pci-host-generic.c
>> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>> > >   }
>> > >  };
>> > >
>> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> > > +                            int size, u32 *val)
>> > > +{
>> > > + struct pci_config_window *cfg = bus->sysdata;
>> > > +
>> > > + /*
>> > > +  * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> > > +  * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> > > +  * resulting in devices appearing multiple times on bus 0 unless we
>> > > +  * filter out those accesses here.
>> > > +  */
>> > > + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> > > +         return PCIBIOS_DEVICE_NOT_FOUND;
>> >
>> > I think we should return 0xffffffff data here, as we do in other
>> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
>> > rockchip_pcie_rd_conf()).
>> >
>> > Actually, xilinx-nwl has a nice trick: it implements
>> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
>> > can use the generic accessors, and pci_generic_config_read() already
>> > fills in ~0 for this case.
>> >
>> > What do you think of the following?  I put it on my pci/host-generic
>> > branch for now (pending your response and an ack from Will).
>>
>> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
>> but either way:
>>
>> Acked-by: Will Deacon <will.deacon@arm.com>
>
> Thanks.  The reason I split out pci_dw_valid_device() is because there are
> several other *_valid_device() functions in other drivers that do basically
> the same thing, and I think it's useful to be able to see that they're all
> similar, as opposed to being buried in the config accessor or map
> functions, which vary a little more.
>
>> I'm assuming this is so small that there's no point in having a Kconfig
>> option for these guys that depends on the generic host driver?
>
> Good question; I don't care either way.

In this case, may I suggest we leave the patch as is? Neither of you
seem to mind, and it will make /my/ life much easier, given that I
don't have to chase distros to get this driver enabled in their kernel
configs (unless we make if 'def_bool y', in which case it makes even
less sense to add the Kconfig option in the first place).

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Ard Biesheuvel]

On 10 October 2017 at 01:13, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
>> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
>> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
>> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> > > index 7d709a7e0aa8..01e81a30e303 100644
>> > > --- a/drivers/pci/host/pci-host-generic.c
>> > > +++ b/drivers/pci/host/pci-host-generic.c
>> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
>> > >   }
>> > >  };
>> > >
>> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
>> > > +                            int size, u32 *val)
>> > > +{
>> > > + struct pci_config_window *cfg = bus->sysdata;
>> > > +
>> > > + /*
>> > > +  * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
>> > > +  * type 0 config TLPs sent to devices 1 and up on its downstream port,
>> > > +  * resulting in devices appearing multiple times on bus 0 unless we
>> > > +  * filter out those accesses here.
>> > > +  */
>> > > + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
>> > > +         return PCIBIOS_DEVICE_NOT_FOUND;
>> >
>> > I think we should return 0xffffffff data here, as we do in other
>> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
>> > rockchip_pcie_rd_conf()).
>> >
>> > Actually, xilinx-nwl has a nice trick: it implements
>> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
>> > can use the generic accessors, and pci_generic_config_read() already
>> > fills in ~0 for this case.
>> >
>> > What do you think of the following?  I put it on my pci/host-generic
>> > branch for now (pending your response and an ack from Will).
>>
>> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
>> but either way:
>>
>> Acked-by: Will Deacon <will.deacon@arm.com>
>
> Thanks.  The reason I split out pci_dw_valid_device() is because there are
> several other *_valid_device() functions in other drivers that do basically
> the same thing, and I think it's useful to be able to see that they're all
> similar, as opposed to being buried in the config accessor or map
> functions, which vary a little more.
>
>> I'm assuming this is so small that there's no point in having a Kconfig
>> option for these guys that depends on the generic host driver?
>
> Good question; I don't care either way.

In this case, may I suggest we leave the patch as is? Neither of you
seem to mind, and it will make /my/ life much easier, given that I
don't have to chase distros to get this driver enabled in their kernel
configs (unless we make if 'def_bool y', in which case it makes even
less sense to add the Kconfig option in the first place).

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Will Deacon]

On Tue, Oct 10, 2017 at 10:04:03AM +0100, Ard Biesheuvel wrote:
> On 10 October 2017 at 01:13, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
> >> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
> >> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> >> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> >> > > index 7d709a7e0aa8..01e81a30e303 100644
> >> > > --- a/drivers/pci/host/pci-host-generic.c
> >> > > +++ b/drivers/pci/host/pci-host-generic.c
> >> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> >> > >   }
> >> > >  };
> >> > >
> >> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> >> > > +                            int size, u32 *val)
> >> > > +{
> >> > > + struct pci_config_window *cfg = bus->sysdata;
> >> > > +
> >> > > + /*
> >> > > +  * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> >> > > +  * type 0 config TLPs sent to devices 1 and up on its downstream port,
> >> > > +  * resulting in devices appearing multiple times on bus 0 unless we
> >> > > +  * filter out those accesses here.
> >> > > +  */
> >> > > + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> >> > > +         return PCIBIOS_DEVICE_NOT_FOUND;
> >> >
> >> > I think we should return 0xffffffff data here, as we do in other
> >> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> >> > rockchip_pcie_rd_conf()).
> >> >
> >> > Actually, xilinx-nwl has a nice trick: it implements
> >> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> >> > can use the generic accessors, and pci_generic_config_read() already
> >> > fills in ~0 for this case.
> >> >
> >> > What do you think of the following?  I put it on my pci/host-generic
> >> > branch for now (pending your response and an ack from Will).
> >>
> >> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> >> but either way:
> >>
> >> Acked-by: Will Deacon <will.deacon@arm.com>
> >
> > Thanks.  The reason I split out pci_dw_valid_device() is because there are
> > several other *_valid_device() functions in other drivers that do basically
> > the same thing, and I think it's useful to be able to see that they're all
> > similar, as opposed to being buried in the config accessor or map
> > functions, which vary a little more.
> >
> >> I'm assuming this is so small that there's no point in having a Kconfig
> >> option for these guys that depends on the generic host driver?
> >
> > Good question; I don't care either way.
> 
> In this case, may I suggest we leave the patch as is? Neither of you
> seem to mind, and it will make /my/ life much easier, given that I
> don't have to chase distros to get this driver enabled in their kernel
> configs (unless we make if 'def_bool y', in which case it makes even
> less sense to add the Kconfig option in the first place).

Yup, I acked it as-is, just thought it was worth asking the question.

Will

Re: [PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Will Deacon]

On Tue, Oct 10, 2017 at 10:04:03AM +0100, Ard Biesheuvel wrote:
> On 10 October 2017 at 01:13, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
> >> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
> >> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> >> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> >> > > index 7d709a7e0aa8..01e81a30e303 100644
> >> > > --- a/drivers/pci/host/pci-host-generic.c
> >> > > +++ b/drivers/pci/host/pci-host-generic.c
> >> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> >> > >   }
> >> > >  };
> >> > >
> >> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> >> > > +                            int size, u32 *val)
> >> > > +{
> >> > > + struct pci_config_window *cfg = bus->sysdata;
> >> > > +
> >> > > + /*
> >> > > +  * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> >> > > +  * type 0 config TLPs sent to devices 1 and up on its downstream port,
> >> > > +  * resulting in devices appearing multiple times on bus 0 unless we
> >> > > +  * filter out those accesses here.
> >> > > +  */
> >> > > + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> >> > > +         return PCIBIOS_DEVICE_NOT_FOUND;
> >> >
> >> > I think we should return 0xffffffff data here, as we do in other
> >> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> >> > rockchip_pcie_rd_conf()).
> >> >
> >> > Actually, xilinx-nwl has a nice trick: it implements
> >> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> >> > can use the generic accessors, and pci_generic_config_read() already
> >> > fills in ~0 for this case.
> >> >
> >> > What do you think of the following?  I put it on my pci/host-generic
> >> > branch for now (pending your response and an ack from Will).
> >>
> >> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> >> but either way:
> >>
> >> Acked-by: Will Deacon <will.deacon@arm.com>
> >
> > Thanks.  The reason I split out pci_dw_valid_device() is because there are
> > several other *_valid_device() functions in other drivers that do basically
> > the same thing, and I think it's useful to be able to see that they're all
> > similar, as opposed to being buried in the config accessor or map
> > functions, which vary a little more.
> >
> >> I'm assuming this is so small that there's no point in having a Kconfig
> >> option for these guys that depends on the generic host driver?
> >
> > Good question; I don't care either way.
> 
> In this case, may I suggest we leave the patch as is? Neither of you
> seem to mind, and it will make /my/ life much easier, given that I
> don't have to chase distros to get this driver enabled in their kernel
> configs (unless we make if 'def_bool y', in which case it makes even
> less sense to add the Kconfig option in the first place).

Yup, I acked it as-is, just thought it was worth asking the question.

Will

[PATCH v4 1/2] PCI: pci-host-generic: add support for Synopsys DesignWare RC in ECAM mode

[Will Deacon]

On Tue, Oct 10, 2017 at 10:04:03AM +0100, Ard Biesheuvel wrote:
> On 10 October 2017 at 01:13, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Oct 09, 2017 at 05:46:07PM +0100, Will Deacon wrote:
> >> On Fri, Oct 06, 2017 at 06:21:59PM -0500, Bjorn Helgaas wrote:
> >> > On Fri, Oct 06, 2017 at 05:39:18PM +0100, Ard Biesheuvel wrote:
> >> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> >> > > index 7d709a7e0aa8..01e81a30e303 100644
> >> > > --- a/drivers/pci/host/pci-host-generic.c
> >> > > +++ b/drivers/pci/host/pci-host-generic.c
> >> > > @@ -35,6 +35,43 @@ static struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> >> > >   }
> >> > >  };
> >> > >
> >> > > +static int pci_dw_ecam_config_read(struct pci_bus *bus, u32 devfn, int where,
> >> > > +                            int size, u32 *val)
> >> > > +{
> >> > > + struct pci_config_window *cfg = bus->sysdata;
> >> > > +
> >> > > + /*
> >> > > +  * The Synopsys DesignWare PCIe controller in ECAM mode will not filter
> >> > > +  * type 0 config TLPs sent to devices 1 and up on its downstream port,
> >> > > +  * resulting in devices appearing multiple times on bus 0 unless we
> >> > > +  * filter out those accesses here.
> >> > > +  */
> >> > > + if (bus->number == cfg->busr.start && PCI_SLOT(devfn) > 0)
> >> > > +         return PCIBIOS_DEVICE_NOT_FOUND;
> >> >
> >> > I think we should return 0xffffffff data here, as we do in other
> >> > similar accessors (dw_pcie_rd_conf(), altera_pcie_cfg_read(),
> >> > rockchip_pcie_rd_conf()).
> >> >
> >> > Actually, xilinx-nwl has a nice trick: it implements
> >> > nwl_pcie_map_bus(), which returns NULL for invalid devices.  Then it
> >> > can use the generic accessors, and pci_generic_config_read() already
> >> > fills in ~0 for this case.
> >> >
> >> > What do you think of the following?  I put it on my pci/host-generic
> >> > branch for now (pending your response and an ack from Will).
> >>
> >> I'd probably just inline the device check in pci_dw_ecam_map_bus directly,
> >> but either way:
> >>
> >> Acked-by: Will Deacon <will.deacon@arm.com>
> >
> > Thanks.  The reason I split out pci_dw_valid_device() is because there are
> > several other *_valid_device() functions in other drivers that do basically
> > the same thing, and I think it's useful to be able to see that they're all
> > similar, as opposed to being buried in the config accessor or map
> > functions, which vary a little more.
> >
> >> I'm assuming this is so small that there's no point in having a Kconfig
> >> option for these guys that depends on the generic host driver?
> >
> > Good question; I don't care either way.
> 
> In this case, may I suggest we leave the patch as is? Neither of you
> seem to mind, and it will make /my/ life much easier, given that I
> don't have to chase distros to get this driver enabled in their kernel
> configs (unless we make if 'def_bool y', in which case it makes even
> less sense to add the Kconfig option in the first place).

Yup, I acked it as-is, just thought it was worth asking the question.

Will

Cleaning up non-standard PCIe ECAM on Arm servers

[Jon Masters]

On 10/06/2017 12:39 PM, Ard Biesheuvel wrote:
> Some implementations of the Synopsys DesignWare PCIe controller implement
> a so-called ECAM shift mode, which allows a static memory window to be
> configured that covers the configuration space of the entire bus range.

Side note that we gave a presentation at Arm TechCon last week with
Cadence about a new program they're offering to perform verification of
PCIe pre-silicon using Palladium with speedbridges and running full
server Operating Systems booting using UEFI/ACPI under emulation. We've
been able to boot RHEL for Arm on these Palladium based platforms for a
while and are collaborating to turn this into a comprehensive program.

Once that was Cadence effort was announced, I pinged Synopsys to ask
them to go clean things up properly for their IP as well. Ultimately
we'll get to all the major IP vendors, and a number of PCIe specific
vendors have already had prodding from me directly over the years. So if
folks see this thread, are in the business of selling PCIe RC IP for Arm
server designs, and we haven't spoken yet, you should ping me. And you
should also talk with smart folks like Ard on how to do this right.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop