Re: [PATCH v7 2/3] pci, pci-thunder-pem: Add PCIe host driver for ThunderX processors.

From: Yury Norov
Date: Sat Mar 05 2016 - 10:54:59 EST


Hi David,

On Fri, Mar 04, 2016 at 02:31:47PM -0800, David Daney wrote:
> From: David Daney <david.daney@xxxxxxxxxx>
>
> The root complexes used to access off-chip PCIe devices (called PEM
> units in the hardware manuals) on some Cavium ThunderX processors
> require quirky access methods for the config space of the PCIe bridge.
> Add a driver to provide these config space accessor functions. The
> pci-host-common code is used to configure the PCI machinery.
>
> Signed-off-by: David Daney <david.daney@xxxxxxxxxx>
> Acked-by: Rob Herring <robh@xxxxxxxxxx>
> Acked-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> .../devicetree/bindings/pci/pci-thunder-pem.txt | 43 +++
> MAINTAINERS | 8 +
> drivers/pci/host/Kconfig | 7 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pci-thunder-pem.c | 347 +++++++++++++++++++++
> 5 files changed, 406 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/pci-thunder-pem.txt
> create mode 100644 drivers/pci/host/pci-thunder-pem.c
>
> diff --git a/Documentation/devicetree/bindings/pci/pci-thunder-pem.txt b/Documentation/devicetree/bindings/pci/pci-thunder-pem.txt
> new file mode 100644
> index 0000000..f131fae
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/pci-thunder-pem.txt
> @@ -0,0 +1,43 @@
> +* ThunderX PEM PCIe host controller
> +
> +Firmware-initialized PCI host controller found on some Cavium
> +ThunderX processors.
> +
> +The properties and their meanings are identical to those described in
> +host-generic-pci.txt except as listed below.
> +
> +Properties of the host controller node that differ from
> +host-generic-pci.txt:
> +
> +- compatible : Must be "cavium,pci-host-thunder-pem"
> +
> +- reg : Two entries: First the configuration space for down
> + stream devices base address and size, as accessed
> + from the parent bus. Second, the register bank of
> + the PEM device PCIe bridge.
> +
> +Example:
> +
> + pci@87e0,c2000000 {
> + compatible = "cavium,pci-host-thunder-pem";
> + device_type = "pci";
> + msi-parent = <&its>;
> + msi-map = <0 &its 0x10000 0x10000>;
> + bus-range = <0x8f 0xc7>;
> + #size-cells = <2>;
> + #address-cells = <3>;
> +
> + reg = <0x8880 0x8f000000 0x0 0x39000000>, /* Configuration space */
> + <0x87e0 0xc2000000 0x0 0x00010000>; /* PEM space */
> + ranges = <0x01000000 0x00 0x00020000 0x88b0 0x00020000 0x00 0x00010000>, /* I/O */
> + <0x03000000 0x00 0x10000000 0x8890 0x10000000 0x0f 0xf0000000>, /* mem64 */
> + <0x43000000 0x10 0x00000000 0x88a0 0x00000000 0x10 0x00000000>, /* mem64-pref */
> + <0x03000000 0x87e0 0xc2f00000 0x87e0 0xc2000000 0x00 0x00100000>; /* mem64 PEM BAR4 */
> +
> + #interrupt-cells = <1>;
> + interrupt-map-mask = <0 0 0 7>;
> + interrupt-map = <0 0 0 1 &gic0 0 0 0 24 4>, /* INTA */
> + <0 0 0 2 &gic0 0 0 0 25 4>, /* INTB */
> + <0 0 0 3 &gic0 0 0 0 26 4>, /* INTC */
> + <0 0 0 4 &gic0 0 0 0 27 4>; /* INTD */
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 370b3cb..cef8dda 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8407,6 +8407,14 @@ L: linux-arm-msm@xxxxxxxxxxxxxxx
> S: Maintained
> F: drivers/pci/host/*qcom*
>
> +PCIE DRIVER FOR CAVIUM THUNDERX
> +M: David Daney <david.daney@xxxxxxxxxx>
> +L: linux-pci@xxxxxxxxxxxxxxx
> +L: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx (moderated for non-subscribers)
> +S: Supported
> +F: Documentation/devicetree/bindings/pci/pci-thunder-*
> +F: drivers/pci/host/pci-thunder-*
> +
> PCMCIA SUBSYSTEM
> P: Linux PCMCIA Team
> L: linux-pcmcia@xxxxxxxxxxxxxxxxxxx
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index cd5dfbb..c642797 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -196,4 +196,11 @@ config PCIE_QCOM
> PCIe controller uses the Designware core plus Qualcomm-specific
> hardware wrappers.
>
> +config PCI_HOST_THUNDER_PEM
> + bool "Cavium Thunder PCIe controller to off-chip devices"
> + depends on OF && ARM64
> + select PCI_HOST_COMMON
> + help
> + Say Y here if you want PCIe support for CN88XX Cavium Thunder SoCs.
> +
> endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 3b24af8..8903172 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -23,3 +23,4 @@ obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
> obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o
> obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
> obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> +obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
> diff --git a/drivers/pci/host/pci-thunder-pem.c b/drivers/pci/host/pci-thunder-pem.c
> new file mode 100644
> index 0000000..4b939c7
> --- /dev/null
> +++ b/drivers/pci/host/pci-thunder-pem.c
> @@ -0,0 +1,347 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (C) 2015 - 2016 Cavium, Inc.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/platform_device.h>
> +
> +#include "pci-host-common.h"
> +
> +#define PEM_CFG_WR 0x28
> +#define PEM_CFG_RD 0x30
> +
> +struct thunder_pem_pci {
> + struct gen_pci gen_pci;
> + u32 ea_entry[3];
> + void __iomem *pem_reg_base;
> +};
> +
> +static void __iomem *thunder_pem_map_bus(struct pci_bus *bus,
> + unsigned int devfn, int where)
> +{
> + struct gen_pci *pci = bus->sysdata;
> + resource_size_t idx = bus->number - pci->cfg.bus_range->start;
> +
> + return pci->cfg.win[idx] + ((devfn << 16) | where);
> +}
> +
> +static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 *val)
> +{
> + u64 read_val;
> + struct thunder_pem_pci *pem_pci;
> + struct gen_pci *pci = bus->sysdata;
> +
> + pem_pci = container_of(pci, struct thunder_pem_pci, gen_pci);
> +
> + if (devfn != 0 || where >= 2048) {
> + *val = ~0;
> + return PCIBIOS_DEVICE_NOT_FOUND;
> + }
> +
> + /*
> + * 32-bit accesses only. Write the address to the low order
> + * bits of PEM_CFG_RD, then trigger the read by reading back.
> + * The config data lands in the upper 32-bits of PEM_CFG_RD.
> + */
> + read_val = where & ~3ull;
> + writeq(read_val, pem_pci->pem_reg_base + PEM_CFG_RD);
> + read_val = readq(pem_pci->pem_reg_base + PEM_CFG_RD);
> + read_val >>= 32;
> +
> + /*
> + * The config space contains some garbage, fix it up. Also
> + * synthesize an EA capability for the BAR used by MSI-X.
> + */
> + switch (where & ~3) {
> + case 0x40:
> + read_val &= 0xffff00ff;
> + read_val |= 0x00007000; /* Skip MSI CAP */
> + break;
> + case 0x70: /* Express Cap */
> + /* PME interrupt on vector 2*/
> + read_val |= (2u << 25);
> + break;
> + case 0xb0: /* MSI-X Cap */
> + /* TableSize=4, Next Cap is EA */
> + read_val &= 0xc00000ff;
> + read_val |= 0x0003bc00;
> + break;
> + case 0xb4:
> + /* Table offset=0, BIR=0 */
> + read_val = 0x00000000;
> + break;
> + case 0xb8:
> + /* BPA offset=0xf0000, BIR=0 */
> + read_val = 0x000f0000;
> + break;
> + case 0xbc:
> + /* EA, 1 entry, no next Cap */
> + read_val = 0x00010014;
> + break;
> + case 0xc0:
> + /* DW2 for type-1 */
> + read_val = 0x00000000;
> + break;
> + case 0xc4:
> + /* Entry BEI=0, PP=0x00, SP=0xff, ES=3 */
> + read_val = 0x80ff0003;
> + break;
> + case 0xc8:
> + read_val = pem_pci->ea_entry[0];
> + break;
> + case 0xcc:
> + read_val = pem_pci->ea_entry[1];
> + break;
> + case 0xd0:
> + read_val = pem_pci->ea_entry[2];
> + break;
> + default:
> + break;
> + }
> + read_val >>= (8 * (where & 3));
> + switch (size) {
> + case 1:
> + read_val &= 0xff;
> + break;
> + case 2:
> + read_val &= 0xffff;
> + break;
> + default:
> + break;
> + }
> + *val = read_val;
> + return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_pem_config_read(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 *val)
> +{
> + struct gen_pci *pci = bus->sysdata;
> +
> + if (bus->number < pci->cfg.bus_range->start ||
> + bus->number > pci->cfg.bus_range->end)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + /*
> + * The first device on the bus is the PEM PCIe bridge.
> + * Special case its config access.
> + */
> + if (bus->number == pci->cfg.bus_range->start)
> + return thunder_pem_bridge_read(bus, devfn, where, size, val);
> +
> + return pci_generic_config_read(bus, devfn, where, size, val);
> +}
> +
> +/*
> + * Some of the w1c_bits below also include read-only or non-writable
> + * reserved bits, this makes the code simpler and is OK as the bits
> + * are not affected by writing zeros to them.
> + */
> +static u32 thunder_pem_bridge_w1c_bits(int where)
> +{
> + u32 w1c_bits = 0;
> +
> + switch (where & ~3) {
> + case 0x04: /* Command/Status */
> + case 0x1c: /* Base and I/O Limit/Secondary Status */
> + w1c_bits = 0xff000000;
> + break;
> + case 0x44: /* Power Management Control and Status */
> + w1c_bits = 0xfffffe00;
> + break;
> + case 0x78: /* Device Control/Device Status */
> + case 0x80: /* Link Control/Link Status */
> + case 0x88: /* Slot Control/Slot Status */
> + case 0x90: /* Root Status */
> + case 0xa0: /* Link Control 2 Registers/Link Status 2 */
> + w1c_bits = 0xffff0000;
> + break;
> + case 0x104: /* Uncorrectable Error Status */
> + case 0x110: /* Correctable Error Status */
> + case 0x130: /* Error Status */
> + case 0x160: /* Link Control 4 */

This patchset is full of magic numbers. For better readability and
portability, it's better to declare all that as macro:
#define LINK_CONTROL_4 0x160

If there's some specific reason to use numbers, I think, it should be
explained.

> + w1c_bits = 0xffffffff;
> + break;
> + default:
> + break;
> + }
> + return w1c_bits;
> +}
> +
> +static int thunder_pem_bridge_write(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 val)
> +{
> + struct gen_pci *pci = bus->sysdata;
> + struct thunder_pem_pci *pem_pci;
> + u64 write_val, read_val;
> + u32 mask = 0;
> +
> + pem_pci = container_of(pci, struct thunder_pem_pci, gen_pci);
> +
> + if (devfn != 0 || where >= 2048)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + /*
> + * 32-bit accesses only. If the write is for a size smaller
> + * than 32-bits, we must first read the 32-bit value and merge
> + * in the desired bits and then write the whole 32-bits back
> + * out.
> + */
> + switch (size) {
> + case 1:
> + read_val = where & ~3ull;
> + writeq(read_val, pem_pci->pem_reg_base + PEM_CFG_RD);
> + read_val = readq(pem_pci->pem_reg_base + PEM_CFG_RD);
> + read_val >>= 32;
> + mask = ~(0xff << (8 * (where & 3)));

I'm pretty sure, there's no any rocket science, but it's hard to
understand what happens here. What is 8? Bits in byte? Bytes in word?
Anything PCI-specific? Moreover, you repeat this line several times
here (though little modified). Maybe it deserves to be wrapped by some
explaining macro?

> + read_val &= mask;
> + val = (val & 0xff) << (8 * (where & 3));
> + val |= (u32)read_val;
> + break;
> + case 2:

Case 1 and 2 are looking very similar. Is it possible to wrap them?
For now it looks like code duplication.

> + read_val = where & ~3ull;
> + writeq(read_val, pem_pci->pem_reg_base + PEM_CFG_RD);
> + read_val = readq(pem_pci->pem_reg_base + PEM_CFG_RD);
> + read_val >>= 32;
> + mask = ~(0xffff << (8 * (where & 3)));
> + read_val &= mask;
> + val = (val & 0xffff) << (8 * (where & 3));
> + val |= (u32)read_val;
> + break;
> + default:
> + break;
> + }
> +
> + /*
> + * By expanding the write width to 32 bits, we may
> + * inadvertently hit some W1C bits that were not intended to
> + * be written. Calculate the mask that must be applied to the
> + * data to be written to avoid these cases.
> + */
> + if (mask) {
> + u32 w1c_bits = thunder_pem_bridge_w1c_bits(where);
> +
> + if (w1c_bits) {
> + mask &= w1c_bits;
> + val &= ~mask;
> + }
> + }
> +
> + /*
> + * Low order bits are the config address, the high order 32
> + * bits are the data to be written.
> + */
> + write_val = where & ~3ull;
> + write_val |= (((u64)val) << 32);
> + writeq(write_val, pem_pci->pem_reg_base + PEM_CFG_WR);
> + return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_pem_config_write(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 val)
> +{
> + struct gen_pci *pci = bus->sysdata;
> +
> + if (bus->number < pci->cfg.bus_range->start ||
> + bus->number > pci->cfg.bus_range->end)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> + /*
> + * The first device on the bus is the PEM PCIe bridge.
> + * Special case its config access.
> + */
> + if (bus->number == pci->cfg.bus_range->start)
> + return thunder_pem_bridge_write(bus, devfn, where, size, val);
> +
> +
> + return pci_generic_config_write(bus, devfn, where, size, val);
> +}
> +
> +static struct gen_pci_cfg_bus_ops thunder_pem_bus_ops = {
> + .bus_shift = 24,
> + .ops = {
> + .map_bus = thunder_pem_map_bus,
> + .read = thunder_pem_config_read,
> + .write = thunder_pem_config_write,
> + }
> +};
> +
> +static const struct of_device_id thunder_pem_of_match[] = {
> + { .compatible = "cavium,pci-host-thunder-pem",
> + .data = &thunder_pem_bus_ops },
> +
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
> +
> +static int thunder_pem_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct of_device_id *of_id;
> + resource_size_t bar4_start;
> + struct resource *res_pem;
> + struct thunder_pem_pci *pem_pci;
> +
> + pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
> + if (!pem_pci)
> + return -ENOMEM;
> +
> + of_id = of_match_node(thunder_pem_of_match, dev->of_node);
> + pem_pci->gen_pci.cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
> +
> + /*
> + * The second register range is the PEM bridge to the PCIe
> + * bus. It has a different config access method than those
> + * devices behind the bridge.
> + */
> + res_pem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res_pem) {
> + dev_err(dev, "missing \"reg[1]\"property\n");
> + return -EINVAL;
> + }
> +
> + pem_pci->pem_reg_base = devm_ioremap(dev, res_pem->start, 0x10000);
> + if (!pem_pci->pem_reg_base)
> + return -ENOMEM;
> +
> + /*
> + * The MSI-X BAR for the PEM and AER interrupts is located at
> + * a fixed offset from the PEM register base. Generate a
> + * fragment of the synthesized Enhanced Allocation capability
> + * structure here for the BAR.
> + */
> + bar4_start = res_pem->start + 0xf00000;
> + pem_pci->ea_entry[0] = (u32)bar4_start | 2;
> + pem_pci->ea_entry[1] = (u32)(res_pem->end - bar4_start) & ~3u;
> + pem_pci->ea_entry[2] = (u32)(bar4_start >> 32);
> +
> + return pci_host_common_probe(pdev, &pem_pci->gen_pci);
> +}
> +
> +static struct platform_driver thunder_pem_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = thunder_pem_of_match,
> + },
> + .probe = thunder_pem_probe,
> +};
> +module_platform_driver(thunder_pem_driver);
> +
> +MODULE_DESCRIPTION("Thunder PEM PCIe host driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.8.3.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel