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

[Yury Norov]

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