diff mbox

[RFC] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

Message ID 1474122278-32525-1-git-send-email-dhdang@apm.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Duc Dang Sept. 17, 2016, 2:24 p.m. UTC 
PCIe controller in X-Gene SoCs is not ECAM compliant: software
needs to configure additional concontroller register to address
device at bus:dev:function.

This patch depends on "ECAM quirks handling for ARM64 platforms"
series (http://www.spinics.net/lists/arm-kernel/msg530692.html)
to address the limitation above for X-Gene PCIe controller.

The quirk will only be applied for X-Gene PCIe MCFG table with
OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).

Signed-off-by: Duc Dang <dhdang@apm.com>
---
 drivers/acpi/pci_mcfg.c           |  32 +++++
 drivers/pci/host/Makefile         |   2 +-
 drivers/pci/host/pci-xgene-ecam.c | 280 ++++++++++++++++++++++++++++++++++++++
 include/linux/pci-ecam.h          |   5 +
 4 files changed, 318 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pci-xgene-ecam.c

Comments

Bjorn Helgaas Sept. 19, 2016, 8:06 p.m. UTC | #1 
Hi Duc,

On Sat, Sep 17, 2016 at 07:24:38AM -0700, Duc Dang wrote:
> PCIe controller in X-Gene SoCs is not ECAM compliant: software
> needs to configure additional concontroller register to address
> device at bus:dev:function.
> 
> This patch depends on "ECAM quirks handling for ARM64 platforms"
> series (http://www.spinics.net/lists/arm-kernel/msg530692.html)
> to address the limitation above for X-Gene PCIe controller.
> 
> The quirk will only be applied for X-Gene PCIe MCFG table with
> OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
> 
> Signed-off-by: Duc Dang <dhdang@apm.com>
> ---
>  drivers/acpi/pci_mcfg.c           |  32 +++++
>  drivers/pci/host/Makefile         |   2 +-
>  drivers/pci/host/pci-xgene-ecam.c | 280 ++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-ecam.h          |   5 +
>  4 files changed, 318 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pci/host/pci-xgene-ecam.c

This adds a bunch of stuff, but doesn't remove anything.  So I assume
it's adding new functionality that didn't exist before.  What is it?

I sort of expected this to also remove, for example, the seemingly
identical xgene_pcie_config_read32() in drivers/pci/host/pci-xgene.c.
Actually, a bunch of this code seems to be duplicated from there.  It
doesn't seem like we should end up with all this duplicated code.

I'd really like it better if all this could get folded into
pci-xgene.c so we don't end up with more files.

> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index ddf338b..adce35f 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -123,6 +123,38 @@ static struct mcfg_fixup mcfg_quirks[] = {
>  	{ "CAVIUM", "THUNDERX", 2, 13, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
>  	  MCFG_RES_EMPTY},
>  #endif
> +#ifdef CONFIG_PCI_XGENE
> +	{"APM   ", "XGENE   ", 1, 0, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 1, 1, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 1, 2, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 1, 3, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 1, 4, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 2, 0, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 2, 1, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 2, 2, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 2, 3, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 2, 4, MCFG_BUS_ANY,
> +		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 3, 0, MCFG_BUS_ANY,
> +		&xgene_v2_1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 3, 1, MCFG_BUS_ANY,
> +		&xgene_v2_1_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 4, 0, MCFG_BUS_ANY,
> +		&xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 4, 1, MCFG_BUS_ANY,
> +		&xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},
> +	{"APM   ", "XGENE   ", 4, 2, MCFG_BUS_ANY,
> +		&xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},

Most of these are the same.  Let's add a macro that fills in the
boilerplate so each entry only contains the variable parts.  I'm going
to propose the same for the ThunderX quirks.

> +#endif
>  };
>  
>  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 8843410..af4f505 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -15,7 +15,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>  obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
> -obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
> +obj-$(CONFIG_PCI_XGENE) += pci-xgene.o pci-xgene-ecam.o
>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>  obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> diff --git a/drivers/pci/host/pci-xgene-ecam.c b/drivers/pci/host/pci-xgene-ecam.c
> new file mode 100644
> index 0000000..b66a04f
> --- /dev/null
> +++ b/drivers/pci/host/pci-xgene-ecam.c
> @@ -0,0 +1,280 @@
> +/*
> + * APM X-Gene PCIe ECAM fixup driver
> + *
> + * Copyright (c) 2016, Applied Micro Circuits Corporation
> + * Author:
> + *	Duc Dang <dhdang@apm.com>
> + *
> + * 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/>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/pci-ecam.h>
> +
> +#ifdef CONFIG_ACPI
> +#define RTDID			0x160
> +#define ROOT_CAP_AND_CTRL	0x5C
> +
> +/* PCIe IP version */
> +#define XGENE_PCIE_IP_VER_UNKN	0
> +#define XGENE_PCIE_IP_VER_1	1
> +#define XGENE_PCIE_IP_VER_2	2

We only use XGENE_PCIE_IP_VER_1, so I think the others should be
removed.  I think it would be nicer to have a "crs_broken:1" bit set
by the probe path, and get rid of the version field altogether.

> +#define XGENE_CSR_LENGTH	0x10000
> +
> +struct xgene_pcie_acpi_root {
> +	void __iomem *csr_base;
> +	u32 version;
> +};

I think this should be folded into struct xgene_pcie_port so we don't
have to allocate and manage it separately.

> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> +{
> +	struct xgene_pcie_acpi_root *xgene_root;
> +	struct device *dev = cfg->parent;
> +	u32 csr_base;
> +
> +	xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
> +	if (!xgene_root)
> +		return -ENOMEM;
> +
> +	switch (cfg->res.start) {
> +	case 0xE0D0000000ULL:
> +		csr_base = 0x1F2B0000;
> +		break;
> +	case 0xD0D0000000ULL:
> +		csr_base = 0x1F2C0000;
> +		break;
> +	case 0x90D0000000ULL:
> +		csr_base = 0x1F2D0000;
> +		break;
> +	case 0xA0D0000000ULL:
> +		csr_base = 0x1F500000;
> +		break;
> +	case 0xC0D0000000ULL:
> +		csr_base = 0x1F510000;
> +		break;

Ugh.  What in the world is going on here?  Apparently we're testing a
host bridge resource against this hard-coded list of random values,
and based on that, we know about this *other* list of hard-coded CSR
ranges?  This is not the way resource discovery normally works ;)

> +	default:
> +		return -ENODEV;
> +	}
> +
> +	xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);

There should be a request_region() somewhere, too.  Ideal would be to
use devm_ioremap_resource(), but I don't know where this apparent
resource is coming from.

> +	if (!xgene_root->csr_base) {
> +		kfree(xgene_root);
> +		return -ENODEV;
> +	}
> +
> +	xgene_root->version = XGENE_PCIE_IP_VER_1;
> +
> +	cfg->priv = xgene_root;
> +
> +	return 0;
> +}
> +
> +static int xgene_v2_1_pcie_ecam_init(struct pci_config_window *cfg)
> +{
> +	struct xgene_pcie_acpi_root *xgene_root;
> +	struct device *dev = cfg->parent;
> +	resource_size_t csr_base;
> +
> +	xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
> +	if (!xgene_root)
> +		return -ENOMEM;
> +
> +	switch (cfg->res.start) {
> +	case 0xC0D0000000ULL:
> +		csr_base = 0x1F2B0000;
> +		break;
> +	case 0xA0D0000000ULL:
> +		csr_base = 0x1F2C0000;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
> +	xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
> +	if (!xgene_root->csr_base) {
> +		kfree(xgene_root);
> +		return -ENODEV;
> +	}
> +
> +	xgene_root->version = XGENE_PCIE_IP_VER_2;
> +
> +	cfg->priv = xgene_root;
> +
> +	return 0;
> +}
> +
> +static int xgene_v2_2_pcie_ecam_init(struct pci_config_window *cfg)
> +{
> +	struct xgene_pcie_acpi_root *xgene_root;
> +	struct device *dev = cfg->parent;
> +	resource_size_t csr_base;
> +
> +	xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
> +	if (!xgene_root)
> +		return -ENOMEM;
> +
> +	switch (cfg->res.start) {
> +	case 0xE0D0000000ULL:
> +		csr_base = 0x1F2B0000;
> +		break;
> +	case 0xA0D0000000ULL:
> +		csr_base = 0x1F500000;
> +		break;
> +	case 0x90D0000000ULL:
> +		csr_base = 0x1F2D0000;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
> +	xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
> +	if (!xgene_root->csr_base) {
> +		kfree(xgene_root);
> +		return -ENODEV;
> +	}
> +
> +	xgene_root->version = XGENE_PCIE_IP_VER_2;
> +
> +	cfg->priv = xgene_root;
> +
> +	return 0;
> +}
> +/*
> + * For Configuration request, RTDID register is used as Bus Number,
> + * Device Number and Function number of the header fields.
> + */
> +static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct xgene_pcie_acpi_root *port = cfg->priv;
> +	unsigned int b, d, f;
> +	u32 rtdid_val = 0;
> +
> +	b = bus->number;
> +	d = PCI_SLOT(devfn);
> +	f = PCI_FUNC(devfn);
> +
> +	if (!pci_is_root_bus(bus))
> +		rtdid_val = (b << 8) | (d << 3) | f;
> +
> +	writel(rtdid_val, port->csr_base + RTDID);
> +	/* read the register back to ensure flush */
> +	readl(port->csr_base + RTDID);
> +}
> +
> +/*
> + * X-Gene PCIe port uses BAR0-BAR1 of RC's configuration space as
> + * the translation from PCI bus to native BUS.  Entire DDR region
> + * is mapped into PCIe space using these registers, so it can be
> + * reached by DMA from EP devices.  The BAR0/1 of bridge should be
> + * hidden during enumeration to avoid the sizing and resource allocation
> + * by PCIe core.
> + */
> +static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> +{
> +	if (pci_is_root_bus(bus) && ((offset == PCI_BASE_ADDRESS_0) ||
> +				     (offset == PCI_BASE_ADDRESS_1)))
> +		return true;
> +
> +	return false;
> +}
> +
> +void __iomem *xgene_pcie_ecam_map_bus(struct pci_bus *bus,
> +				      unsigned int devfn, int where)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +	unsigned int busn = bus->number;
> +	void __iomem *base;
> +
> +	if (busn < cfg->busr.start || busn > cfg->busr.end)
> +		return NULL;
> +
> +	if ((pci_is_root_bus(bus) && devfn != 0) ||
> +	    xgene_pcie_hide_rc_bars(bus, where))
> +		return NULL;
> +
> +	xgene_pcie_set_rtdid_reg(bus, devfn);
> +
> +	if (busn > cfg->busr.start)
> +		base = cfg->win + (1 << cfg->ops->bus_shift);
> +	else
> +		base = cfg->win;
> +
> +	return base + where;
> +}
> +
> +static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *val)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct xgene_pcie_acpi_root *port = cfg->priv;
> +
> +	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> +	    PCIBIOS_SUCCESSFUL)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	/*
> +	* The v1 controller has a bug in its Configuration Request
> +	* Retry Status (CRS) logic: when CRS is enabled and we read the
> +	* Vendor and Device ID of a non-existent device, the controller
> +	* fabricates return data of 0xFFFF0001 ("device exists but is not
> +	* ready") instead of 0xFFFFFFFF ("device does not exist").  This
> +	* causes the PCI core to retry the read until it times out.
> +	* Avoid this by not claiming to support CRS.
> +	*/
> +	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
> +	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
> +		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
> +
> +	if (size <= 2)
> +		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +struct pci_ecam_ops xgene_v1_pcie_ecam_ops = {
> +	.bus_shift	= 16,
> +	.init		= xgene_v1_pcie_ecam_init,
> +	.pci_ops	= {
> +		.map_bus	= xgene_pcie_ecam_map_bus,
> +		.read		= xgene_pcie_config_read32,
> +		.write		= pci_generic_config_write,
> +	}
> +};
> +
> +struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops = {
> +	.bus_shift	= 16,
> +	.init		= xgene_v2_1_pcie_ecam_init,
> +	.pci_ops	= {
> +		.map_bus	= xgene_pcie_ecam_map_bus,
> +		.read		= xgene_pcie_config_read32,
> +		.write		= pci_generic_config_write,
> +	}
> +};
> +
> +struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops = {
> +	.bus_shift	= 16,
> +	.init		= xgene_v2_2_pcie_ecam_init,
> +	.pci_ops	= {
> +		.map_bus	= xgene_pcie_ecam_map_bus,
> +		.read		= xgene_pcie_config_read32,
> +		.write		= pci_generic_config_write,
> +	}
> +};
> +#endif
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 35f0e81..40da3e7 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -65,6 +65,11 @@ extern struct pci_ecam_ops pci_thunder_pem_ops;
>  #ifdef CONFIG_PCI_HOST_THUNDER_ECAM
>  extern struct pci_ecam_ops pci_thunder_ecam_ops;
>  #endif
> +#ifdef CONFIG_PCI_XGENE
> +extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops;
> +extern struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops;
> +extern struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops;
> +#endif
>  
>  #ifdef CONFIG_PCI_HOST_GENERIC
>  /* for DT-based PCI controllers that support ECAM */
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duc Dang Sept. 20, 2016, 1:07 a.m. UTC | #2 
Hi Bjorn,

Thanks for reviewing my RFC patch.

On Mon, Sep 19, 2016 at 1:06 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Hi Duc,
>
> On Sat, Sep 17, 2016 at 07:24:38AM -0700, Duc Dang wrote:
>> PCIe controller in X-Gene SoCs is not ECAM compliant: software
>> needs to configure additional concontroller register to address
>> device at bus:dev:function.
>>
>> This patch depends on "ECAM quirks handling for ARM64 platforms"
>> series (http://www.spinics.net/lists/arm-kernel/msg530692.html)
>> to address the limitation above for X-Gene PCIe controller.
>>
>> The quirk will only be applied for X-Gene PCIe MCFG table with
>> OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
>>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>> ---
>>  drivers/acpi/pci_mcfg.c           |  32 +++++
>>  drivers/pci/host/Makefile         |   2 +-
>>  drivers/pci/host/pci-xgene-ecam.c | 280 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/pci-ecam.h          |   5 +
>>  4 files changed, 318 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/pci/host/pci-xgene-ecam.c
>
> This adds a bunch of stuff, but doesn't remove anything.  So I assume
> it's adding new functionality that didn't exist before.  What is it?

This patch only adds the ability for X-Gene PCIe controller to be
probable in ACPI boot mode. All the 'quirk' code added is for ECAM to
work with X-Gene.

>
> I sort of expected this to also remove, for example, the seemingly
> identical xgene_pcie_config_read32() in drivers/pci/host/pci-xgene.c.
> Actually, a bunch of this code seems to be duplicated from there.  It
> doesn't seem like we should end up with all this duplicated code.
>
> I'd really like it better if all this could get folded into
> pci-xgene.c so we don't end up with more files.

I am still debating whether to put this X-Gene ECAM quirk code into
drivers/acpi or keep it here in drivers/pci/host. But given the
direction of other quirk patches for ThunderX and HiSi, seem like the
quirk will stay in drivers/pci/host. I can definitely fold the new
quirk code into pci-xgene.c as you suggested and eliminate the
identical one.

>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index ddf338b..adce35f 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -123,6 +123,38 @@ static struct mcfg_fixup mcfg_quirks[] = {
>>       { "CAVIUM", "THUNDERX", 2, 13, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
>>         MCFG_RES_EMPTY},
>>  #endif
>> +#ifdef CONFIG_PCI_XGENE
>> +     {"APM   ", "XGENE   ", 1, 0, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 1, 1, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 1, 2, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 1, 3, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 1, 4, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 2, 0, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 2, 1, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 2, 2, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 2, 3, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 2, 4, MCFG_BUS_ANY,
>> +             &xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 3, 0, MCFG_BUS_ANY,
>> +             &xgene_v2_1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 3, 1, MCFG_BUS_ANY,
>> +             &xgene_v2_1_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 4, 0, MCFG_BUS_ANY,
>> +             &xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 4, 1, MCFG_BUS_ANY,
>> +             &xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},
>> +     {"APM   ", "XGENE   ", 4, 2, MCFG_BUS_ANY,
>> +             &xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},
>
> Most of these are the same.  Let's add a macro that fills in the
> boilerplate so each entry only contains the variable parts.  I'm going
> to propose the same for the ThunderX quirks.

I will follow up on this.

>
>> +#endif
>>  };
>>
>>  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 8843410..af4f505 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -15,7 +15,7 @@ obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>>  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
>>  obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
>> -obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
>> +obj-$(CONFIG_PCI_XGENE) += pci-xgene.o pci-xgene-ecam.o
>>  obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>>  obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>> diff --git a/drivers/pci/host/pci-xgene-ecam.c b/drivers/pci/host/pci-xgene-ecam.c
>> new file mode 100644
>> index 0000000..b66a04f
>> --- /dev/null
>> +++ b/drivers/pci/host/pci-xgene-ecam.c
>> @@ -0,0 +1,280 @@
>> +/*
>> + * APM X-Gene PCIe ECAM fixup driver
>> + *
>> + * Copyright (c) 2016, Applied Micro Circuits Corporation
>> + * Author:
>> + *   Duc Dang <dhdang@apm.com>
>> + *
>> + * 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/>.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/pci-acpi.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pci-ecam.h>
>> +
>> +#ifdef CONFIG_ACPI
>> +#define RTDID                        0x160
>> +#define ROOT_CAP_AND_CTRL    0x5C
>> +
>> +/* PCIe IP version */
>> +#define XGENE_PCIE_IP_VER_UNKN       0
>> +#define XGENE_PCIE_IP_VER_1  1
>> +#define XGENE_PCIE_IP_VER_2  2
>
> We only use XGENE_PCIE_IP_VER_1, so I think the others should be
> removed.  I think it would be nicer to have a "crs_broken:1" bit set
> by the probe path, and get rid of the version field altogether.

We do use XGENE_PCIE_IP_VER_2 for X-Gene v2 PCIe controller, but your
idea about using crs_broken is much better. I will make change
following that. This will affect DT booting too.

>
>> +#define XGENE_CSR_LENGTH     0x10000
>> +
>> +struct xgene_pcie_acpi_root {
>> +     void __iomem *csr_base;
>> +     u32 version;
>> +};
>
> I think this should be folded into struct xgene_pcie_port so we don't
> have to allocate and manage it separately.

I will need to look into this more. When booting with ACPI mode, the
code in pci-xgene.c is not used (except the cfg read/write functions
that are shared with ECAM quirk code), so puting these into
xgene_pcie_port will force ECAM quirk code to allocate this structure
as well.

>
>> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>> +{
>> +     struct xgene_pcie_acpi_root *xgene_root;
>> +     struct device *dev = cfg->parent;
>> +     u32 csr_base;
>> +
>> +     xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
>> +     if (!xgene_root)
>> +             return -ENOMEM;
>> +
>> +     switch (cfg->res.start) {
>> +     case 0xE0D0000000ULL:
>> +             csr_base = 0x1F2B0000;
>> +             break;
>> +     case 0xD0D0000000ULL:
>> +             csr_base = 0x1F2C0000;
>> +             break;
>> +     case 0x90D0000000ULL:
>> +             csr_base = 0x1F2D0000;
>> +             break;
>> +     case 0xA0D0000000ULL:
>> +             csr_base = 0x1F500000;
>> +             break;
>> +     case 0xC0D0000000ULL:
>> +             csr_base = 0x1F510000;
>> +             break;
>
> Ugh.  What in the world is going on here?  Apparently we're testing a
> host bridge resource against this hard-coded list of random values,
> and based on that, we know about this *other* list of hard-coded CSR
> ranges?  This is not the way resource discovery normally works ;)

Yes, this is very ugly :(

But discovering CSR regions by using acpi_walk_resource for each root
port is not a preferred solution because of the concern that it may
open a backdoor for continuously abusive usage of the quirk for new
version of SoCs (https://patchwork.kernel.org/patch/9178791/, last 2
comments from Lorenzo and Ard)

>
>> +     default:
>> +             return -ENODEV;
>> +     }
>> +
>> +     xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
>
> There should be a request_region() somewhere, too.  Ideal would be to
> use devm_ioremap_resource(), but I don't know where this apparent
> resource is coming from.

Yes, I will use request_region/devm_ioremap_resource here.

>
>> +     if (!xgene_root->csr_base) {
>> +             kfree(xgene_root);
>> +             return -ENODEV;
>> +     }
>> +
>> +     xgene_root->version = XGENE_PCIE_IP_VER_1;
>> +
>> +     cfg->priv = xgene_root;
>> +
>> +     return 0;
>> +}
>> +
>> +static int xgene_v2_1_pcie_ecam_init(struct pci_config_window *cfg)
>> +{
>> +     struct xgene_pcie_acpi_root *xgene_root;
>> +     struct device *dev = cfg->parent;
>> +     resource_size_t csr_base;
>> +
>> +     xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
>> +     if (!xgene_root)
>> +             return -ENOMEM;
>> +
>> +     switch (cfg->res.start) {
>> +     case 0xC0D0000000ULL:
>> +             csr_base = 0x1F2B0000;
>> +             break;
>> +     case 0xA0D0000000ULL:
>> +             csr_base = 0x1F2C0000;
>> +             break;
>> +     default:
>> +             return -ENODEV;
>> +     }
>> +
>> +     xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
>> +     if (!xgene_root->csr_base) {
>> +             kfree(xgene_root);
>> +             return -ENODEV;
>> +     }
>> +
>> +     xgene_root->version = XGENE_PCIE_IP_VER_2;
>> +
>> +     cfg->priv = xgene_root;
>> +
>> +     return 0;
>> +}
>> +
>> +static int xgene_v2_2_pcie_ecam_init(struct pci_config_window *cfg)
>> +{
>> +     struct xgene_pcie_acpi_root *xgene_root;
>> +     struct device *dev = cfg->parent;
>> +     resource_size_t csr_base;
>> +
>> +     xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
>> +     if (!xgene_root)
>> +             return -ENOMEM;
>> +
>> +     switch (cfg->res.start) {
>> +     case 0xE0D0000000ULL:
>> +             csr_base = 0x1F2B0000;
>> +             break;
>> +     case 0xA0D0000000ULL:
>> +             csr_base = 0x1F500000;
>> +             break;
>> +     case 0x90D0000000ULL:
>> +             csr_base = 0x1F2D0000;
>> +             break;
>> +     default:
>> +             return -ENODEV;
>> +     }
>> +
>> +     xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
>> +     if (!xgene_root->csr_base) {
>> +             kfree(xgene_root);
>> +             return -ENODEV;
>> +     }
>> +
>> +     xgene_root->version = XGENE_PCIE_IP_VER_2;
>> +
>> +     cfg->priv = xgene_root;
>> +
>> +     return 0;
>> +}
>> +/*
>> + * For Configuration request, RTDID register is used as Bus Number,
>> + * Device Number and Function number of the header fields.
>> + */
>> +static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
>> +{
>> +     struct pci_config_window *cfg = bus->sysdata;
>> +     struct xgene_pcie_acpi_root *port = cfg->priv;
>> +     unsigned int b, d, f;
>> +     u32 rtdid_val = 0;
>> +
>> +     b = bus->number;
>> +     d = PCI_SLOT(devfn);
>> +     f = PCI_FUNC(devfn);
>> +
>> +     if (!pci_is_root_bus(bus))
>> +             rtdid_val = (b << 8) | (d << 3) | f;
>> +
>> +     writel(rtdid_val, port->csr_base + RTDID);
>> +     /* read the register back to ensure flush */
>> +     readl(port->csr_base + RTDID);
>> +}
>> +
>> +/*
>> + * X-Gene PCIe port uses BAR0-BAR1 of RC's configuration space as
>> + * the translation from PCI bus to native BUS.  Entire DDR region
>> + * is mapped into PCIe space using these registers, so it can be
>> + * reached by DMA from EP devices.  The BAR0/1 of bridge should be
>> + * hidden during enumeration to avoid the sizing and resource allocation
>> + * by PCIe core.
>> + */
>> +static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
>> +{
>> +     if (pci_is_root_bus(bus) && ((offset == PCI_BASE_ADDRESS_0) ||
>> +                                  (offset == PCI_BASE_ADDRESS_1)))
>> +             return true;
>> +
>> +     return false;
>> +}
>> +
>> +void __iomem *xgene_pcie_ecam_map_bus(struct pci_bus *bus,
>> +                                   unsigned int devfn, int where)
>> +{
>> +     struct pci_config_window *cfg = bus->sysdata;
>> +     unsigned int busn = bus->number;
>> +     void __iomem *base;
>> +
>> +     if (busn < cfg->busr.start || busn > cfg->busr.end)
>> +             return NULL;
>> +
>> +     if ((pci_is_root_bus(bus) && devfn != 0) ||
>> +         xgene_pcie_hide_rc_bars(bus, where))
>> +             return NULL;
>> +
>> +     xgene_pcie_set_rtdid_reg(bus, devfn);
>> +
>> +     if (busn > cfg->busr.start)
>> +             base = cfg->win + (1 << cfg->ops->bus_shift);
>> +     else
>> +             base = cfg->win;
>> +
>> +     return base + where;
>> +}
>> +
>> +static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>> +                                 int where, int size, u32 *val)
>> +{
>> +     struct pci_config_window *cfg = bus->sysdata;
>> +     struct xgene_pcie_acpi_root *port = cfg->priv;
>> +
>> +     if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
>> +         PCIBIOS_SUCCESSFUL)
>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>> +
>> +     /*
>> +     * The v1 controller has a bug in its Configuration Request
>> +     * Retry Status (CRS) logic: when CRS is enabled and we read the
>> +     * Vendor and Device ID of a non-existent device, the controller
>> +     * fabricates return data of 0xFFFF0001 ("device exists but is not
>> +     * ready") instead of 0xFFFFFFFF ("device does not exist").  This
>> +     * causes the PCI core to retry the read until it times out.
>> +     * Avoid this by not claiming to support CRS.
>> +     */
>> +     if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
>> +         ((where & ~0x3) == ROOT_CAP_AND_CTRL))
>> +             *val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
>> +
>> +     if (size <= 2)
>> +             *val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
>> +
>> +     return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>> +struct pci_ecam_ops xgene_v1_pcie_ecam_ops = {
>> +     .bus_shift      = 16,
>> +     .init           = xgene_v1_pcie_ecam_init,
>> +     .pci_ops        = {
>> +             .map_bus        = xgene_pcie_ecam_map_bus,
>> +             .read           = xgene_pcie_config_read32,
>> +             .write          = pci_generic_config_write,
>> +     }
>> +};
>> +
>> +struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops = {
>> +     .bus_shift      = 16,
>> +     .init           = xgene_v2_1_pcie_ecam_init,
>> +     .pci_ops        = {
>> +             .map_bus        = xgene_pcie_ecam_map_bus,
>> +             .read           = xgene_pcie_config_read32,
>> +             .write          = pci_generic_config_write,
>> +     }
>> +};
>> +
>> +struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops = {
>> +     .bus_shift      = 16,
>> +     .init           = xgene_v2_2_pcie_ecam_init,
>> +     .pci_ops        = {
>> +             .map_bus        = xgene_pcie_ecam_map_bus,
>> +             .read           = xgene_pcie_config_read32,
>> +             .write          = pci_generic_config_write,
>> +     }
>> +};
>> +#endif
>> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
>> index 35f0e81..40da3e7 100644
>> --- a/include/linux/pci-ecam.h
>> +++ b/include/linux/pci-ecam.h
>> @@ -65,6 +65,11 @@ extern struct pci_ecam_ops pci_thunder_pem_ops;
>>  #ifdef CONFIG_PCI_HOST_THUNDER_ECAM
>>  extern struct pci_ecam_ops pci_thunder_ecam_ops;
>>  #endif
>> +#ifdef CONFIG_PCI_XGENE
>> +extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops;
>> +extern struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops;
>> +extern struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops;
>> +#endif
>>
>>  #ifdef CONFIG_PCI_HOST_GENERIC
>>  /* for DT-based PCI controllers that support ECAM */
>> --
>> 1.9.1
>>
Regards,
Duc Dang.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Sept. 21, 2016, 9:22 p.m. UTC | #3 
On Mon, Sep 19, 2016 at 06:07:37PM -0700, Duc Dang wrote:
> On Mon, Sep 19, 2016 at 1:06 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sat, Sep 17, 2016 at 07:24:38AM -0700, Duc Dang wrote:

> This patch only adds the ability for X-Gene PCIe controller to be
> probable in ACPI boot mode. All the 'quirk' code added is for ECAM to
> work with X-Gene.
>
> > I sort of expected this to also remove, for example, the seemingly
> > identical xgene_pcie_config_read32() in drivers/pci/host/pci-xgene.c.
> > Actually, a bunch of this code seems to be duplicated from there.  It
> > doesn't seem like we should end up with all this duplicated code.
> >
> > I'd really like it better if all this could get folded into
> > pci-xgene.c so we don't end up with more files.
> 
> I am still debating whether to put this X-Gene ECAM quirk code into
> drivers/acpi or keep it here in drivers/pci/host. But given the
> direction of other quirk patches for ThunderX and HiSi, seem like the
> quirk will stay in drivers/pci/host. I can definitely fold the new
> quirk code into pci-xgene.c as you suggested and eliminate the
> identical one.

I like Tomasz's patches, where the MCFG quirk itself is in
acpi/pci_mcfg.c, and it uses config accessors exported from
drivers/pci/host.

I do not want to end up with duplicate accessors.  The mapping
functions and accessors should be the same whether we're booting with
DT or ACPI.

I think a patch to add ACPI support should only contain:

  - acpi/pci_mcfg.c quirks to fix incorrect ACPI MCFG resources or use
    special accessors,

  - pnp/quirks.c quirks to compensate for missing ACPI _CRS for the
    ECAM regions, and

  - pci-xgene.c code to derive the csr_base and cfg_base.  Today we
    get that from DT, but the _CRS producer/consumer mess means we
    don't have a good way to get it from ACPI, so you'll need some
    sort of quirk for this.

> >> +struct xgene_pcie_acpi_root {
> >> +     void __iomem *csr_base;
> >> +     u32 version;
> >> +};
> >
> > I think this should be folded into struct xgene_pcie_port so we don't
> > have to allocate and manage it separately.
> 
> I will need to look into this more. When booting with ACPI mode, the
> code in pci-xgene.c is not used (except the cfg read/write functions
> that are shared with ECAM quirk code), so puting these into
> xgene_pcie_port will force ECAM quirk code to allocate this structure
> as well.

This information is needed whether booting with DT or ACPI, so we
should use the existing xgene_pcie_port.csr_base and initialize it
differently depending on which we're using.

> >> +     default:
> >> +             return -ENODEV;
> >> +     }
> >> +
> >> +     xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
> >
> > There should be a request_region() somewhere, too.  Ideal would be to
> > use devm_ioremap_resource(), but I don't know where this apparent
> > resource is coming from.
> 
> Yes, I will use request_region/devm_ioremap_resource here.

We're not *adding* any new resources that need ioremapping; all we're
doing is changing the *source* of the resource, so we should use the
same devm_ioremap_resource() you already have in xgene_pcie_map_reg().
You might have to refactor that slightly so we can lookup the resource
via either DT or ACPI (you'll probably actually use a quirk since ACPI
doesn't have a good mechanism for this), and then use the same call to
devm_ioremap_resource().

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duc Dang Oct. 26, 2016, 1:31 a.m. UTC | #4 
On Wed, Sep 21, 2016 at 2:22 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Sep 19, 2016 at 06:07:37PM -0700, Duc Dang wrote:
>> On Mon, Sep 19, 2016 at 1:06 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Sat, Sep 17, 2016 at 07:24:38AM -0700, Duc Dang wrote:
>
>> This patch only adds the ability for X-Gene PCIe controller to be
>> probable in ACPI boot mode. All the 'quirk' code added is for ECAM to
>> work with X-Gene.
>>
>> > I sort of expected this to also remove, for example, the seemingly
>> > identical xgene_pcie_config_read32() in drivers/pci/host/pci-xgene.c.
>> > Actually, a bunch of this code seems to be duplicated from there.  It
>> > doesn't seem like we should end up with all this duplicated code.
>> >
>> > I'd really like it better if all this could get folded into
>> > pci-xgene.c so we don't end up with more files.
>>
>> I am still debating whether to put this X-Gene ECAM quirk code into
>> drivers/acpi or keep it here in drivers/pci/host. But given the
>> direction of other quirk patches for ThunderX and HiSi, seem like the
>> quirk will stay in drivers/pci/host. I can definitely fold the new
>> quirk code into pci-xgene.c as you suggested and eliminate the
>> identical one.
>
> I like Tomasz's patches, where the MCFG quirk itself is in
> acpi/pci_mcfg.c, and it uses config accessors exported from
> drivers/pci/host.

Yes, I removed the new file and folded the quirk code into pci-xgene.c.

>
> I do not want to end up with duplicate accessors.  The mapping
> functions and accessors should be the same whether we're booting with
> DT or ACPI.
>
> I think a patch to add ACPI support should only contain:
>
>   - acpi/pci_mcfg.c quirks to fix incorrect ACPI MCFG resources or use
>     special accessors,
>
>   - pnp/quirks.c quirks to compensate for missing ACPI _CRS for the
>     ECAM regions, and
>
>   - pci-xgene.c code to derive the csr_base and cfg_base.  Today we
>     get that from DT, but the _CRS producer/consumer mess means we
>     don't have a good way to get it from ACPI, so you'll need some
>     sort of quirk for this.

The new quirk code (v2 patch) follows this direction, but I have not
found a good way to introduce quirk into pnp/quirks.c yet. Just to
clarify a little bit, our ACPI table provides ECAM base address from
_CBA method. The missing piece is the controller register base address
(csr_base) that I need to get from a hard-coded resource array.

I also owe you the rework for Configuration Request Retry Status
workaround, but it will need to be done in a separate patch set for
both DT and ACPI.
>
>> >> +struct xgene_pcie_acpi_root {
>> >> +     void __iomem *csr_base;
>> >> +     u32 version;
>> >> +};
>> >
>> > I think this should be folded into struct xgene_pcie_port so we don't
>> > have to allocate and manage it separately.
>>
>> I will need to look into this more. When booting with ACPI mode, the
>> code in pci-xgene.c is not used (except the cfg read/write functions
>> that are shared with ECAM quirk code), so puting these into
>> xgene_pcie_port will force ECAM quirk code to allocate this structure
>> as well.
>
> This information is needed whether booting with DT or ACPI, so we
> should use the existing xgene_pcie_port.csr_base and initialize it
> differently depending on which we're using.

The new ECAM quirk code will also allocate struct xgene_pcie_port, I
got rid of xgene_pcie_acpi_root struct.

>
>> >> +     default:
>> >> +             return -ENODEV;
>> >> +     }
>> >> +
>> >> +     xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
>> >
>> > There should be a request_region() somewhere, too.  Ideal would be to
>> > use devm_ioremap_resource(), but I don't know where this apparent
>> > resource is coming from.
>>
>> Yes, I will use request_region/devm_ioremap_resource here.
>
> We're not *adding* any new resources that need ioremapping; all we're
> doing is changing the *source* of the resource, so we should use the
> same devm_ioremap_resource() you already have in xgene_pcie_map_reg().
> You might have to refactor that slightly so we can lookup the resource
> via either DT or ACPI (you'll probably actually use a quirk since ACPI
> doesn't have a good mechanism for this), and then use the same call to
> devm_ioremap_resource().

I changed to use devm_ioremap_resource to map the csr_base from the
fixed resource array defined for each X-Gene SoC. The 'cat
/proc/iomem' for PCIe port on Mustang board is like following:

1f2b0000-1f2bffff : PNP0A08:00
e040000000-e07fffffff : PCI Bus 0000:00
  e040000000-e0401fffff : PCI Bus 0000:01
    e040000000-e0400fffff : 0000:01:00.0
      e040000000-e0400fffff : mlx4_core
    e040100000-e0401fffff : 0000:01:00.0
e0d0000000-e0dfffffff : PCI ECAM
f000000000-ffffffffff : PCI Bus 0000:00
  f000000000-f001ffffff : PCI Bus 0000:01
    f000000000-f001ffffff : 0000:01:00.0
      f000000000-f001ffffff : mlx4_core

Is this what you expect? Or you are looking for something else?

Regards,
Duc Dabng.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index ddf338b..adce35f 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -123,6 +123,38 @@  static struct mcfg_fixup mcfg_quirks[] = {
 	{ "CAVIUM", "THUNDERX", 2, 13, MCFG_BUS_ANY, &pci_thunder_ecam_ops,
 	  MCFG_RES_EMPTY},
 #endif
+#ifdef CONFIG_PCI_XGENE
+	{"APM   ", "XGENE   ", 1, 0, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 1, 1, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 1, 2, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 1, 3, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 1, 4, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 2, 0, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 2, 1, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 2, 2, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 2, 3, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 2, 4, MCFG_BUS_ANY,
+		&xgene_v1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 3, 0, MCFG_BUS_ANY,
+		&xgene_v2_1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 3, 1, MCFG_BUS_ANY,
+		&xgene_v2_1_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 4, 0, MCFG_BUS_ANY,
+		&xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 4, 1, MCFG_BUS_ANY,
+		&xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},
+	{"APM   ", "XGENE   ", 4, 2, MCFG_BUS_ANY,
+		&xgene_v2_2_pcie_ecam_ops, MCFG_RES_EMPTY},
+#endif
 };
 
 static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 8843410..af4f505 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -15,7 +15,7 @@  obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
 obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
-obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
+obj-$(CONFIG_PCI_XGENE) += pci-xgene.o pci-xgene-ecam.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
diff --git a/drivers/pci/host/pci-xgene-ecam.c b/drivers/pci/host/pci-xgene-ecam.c
new file mode 100644
index 0000000..b66a04f
--- /dev/null
+++ b/drivers/pci/host/pci-xgene-ecam.c
@@ -0,0 +1,280 @@ 
+/*
+ * APM X-Gene PCIe ECAM fixup driver
+ *
+ * Copyright (c) 2016, Applied Micro Circuits Corporation
+ * Author:
+ *	Duc Dang <dhdang@apm.com>
+ *
+ * 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/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/platform_device.h>
+#include <linux/pci-ecam.h>
+
+#ifdef CONFIG_ACPI
+#define RTDID			0x160
+#define ROOT_CAP_AND_CTRL	0x5C
+
+/* PCIe IP version */
+#define XGENE_PCIE_IP_VER_UNKN	0
+#define XGENE_PCIE_IP_VER_1	1
+#define XGENE_PCIE_IP_VER_2	2
+
+#define XGENE_CSR_LENGTH	0x10000
+
+struct xgene_pcie_acpi_root {
+	void __iomem *csr_base;
+	u32 version;
+};
+
+static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
+{
+	struct xgene_pcie_acpi_root *xgene_root;
+	struct device *dev = cfg->parent;
+	u32 csr_base;
+
+	xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+	if (!xgene_root)
+		return -ENOMEM;
+
+	switch (cfg->res.start) {
+	case 0xE0D0000000ULL:
+		csr_base = 0x1F2B0000;
+		break;
+	case 0xD0D0000000ULL:
+		csr_base = 0x1F2C0000;
+		break;
+	case 0x90D0000000ULL:
+		csr_base = 0x1F2D0000;
+		break;
+	case 0xA0D0000000ULL:
+		csr_base = 0x1F500000;
+		break;
+	case 0xC0D0000000ULL:
+		csr_base = 0x1F510000;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+	if (!xgene_root->csr_base) {
+		kfree(xgene_root);
+		return -ENODEV;
+	}
+
+	xgene_root->version = XGENE_PCIE_IP_VER_1;
+
+	cfg->priv = xgene_root;
+
+	return 0;
+}
+
+static int xgene_v2_1_pcie_ecam_init(struct pci_config_window *cfg)
+{
+	struct xgene_pcie_acpi_root *xgene_root;
+	struct device *dev = cfg->parent;
+	resource_size_t csr_base;
+
+	xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+	if (!xgene_root)
+		return -ENOMEM;
+
+	switch (cfg->res.start) {
+	case 0xC0D0000000ULL:
+		csr_base = 0x1F2B0000;
+		break;
+	case 0xA0D0000000ULL:
+		csr_base = 0x1F2C0000;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+	if (!xgene_root->csr_base) {
+		kfree(xgene_root);
+		return -ENODEV;
+	}
+
+	xgene_root->version = XGENE_PCIE_IP_VER_2;
+
+	cfg->priv = xgene_root;
+
+	return 0;
+}
+
+static int xgene_v2_2_pcie_ecam_init(struct pci_config_window *cfg)
+{
+	struct xgene_pcie_acpi_root *xgene_root;
+	struct device *dev = cfg->parent;
+	resource_size_t csr_base;
+
+	xgene_root = devm_kzalloc(dev, sizeof(*xgene_root), GFP_KERNEL);
+	if (!xgene_root)
+		return -ENOMEM;
+
+	switch (cfg->res.start) {
+	case 0xE0D0000000ULL:
+		csr_base = 0x1F2B0000;
+		break;
+	case 0xA0D0000000ULL:
+		csr_base = 0x1F500000;
+		break;
+	case 0x90D0000000ULL:
+		csr_base = 0x1F2D0000;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	xgene_root->csr_base = ioremap(csr_base, XGENE_CSR_LENGTH);
+	if (!xgene_root->csr_base) {
+		kfree(xgene_root);
+		return -ENODEV;
+	}
+
+	xgene_root->version = XGENE_PCIE_IP_VER_2;
+
+	cfg->priv = xgene_root;
+
+	return 0;
+}
+/*
+ * For Configuration request, RTDID register is used as Bus Number,
+ * Device Number and Function number of the header fields.
+ */
+static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+	struct xgene_pcie_acpi_root *port = cfg->priv;
+	unsigned int b, d, f;
+	u32 rtdid_val = 0;
+
+	b = bus->number;
+	d = PCI_SLOT(devfn);
+	f = PCI_FUNC(devfn);
+
+	if (!pci_is_root_bus(bus))
+		rtdid_val = (b << 8) | (d << 3) | f;
+
+	writel(rtdid_val, port->csr_base + RTDID);
+	/* read the register back to ensure flush */
+	readl(port->csr_base + RTDID);
+}
+
+/*
+ * X-Gene PCIe port uses BAR0-BAR1 of RC's configuration space as
+ * the translation from PCI bus to native BUS.  Entire DDR region
+ * is mapped into PCIe space using these registers, so it can be
+ * reached by DMA from EP devices.  The BAR0/1 of bridge should be
+ * hidden during enumeration to avoid the sizing and resource allocation
+ * by PCIe core.
+ */
+static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
+{
+	if (pci_is_root_bus(bus) && ((offset == PCI_BASE_ADDRESS_0) ||
+				     (offset == PCI_BASE_ADDRESS_1)))
+		return true;
+
+	return false;
+}
+
+void __iomem *xgene_pcie_ecam_map_bus(struct pci_bus *bus,
+				      unsigned int devfn, int where)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+	unsigned int busn = bus->number;
+	void __iomem *base;
+
+	if (busn < cfg->busr.start || busn > cfg->busr.end)
+		return NULL;
+
+	if ((pci_is_root_bus(bus) && devfn != 0) ||
+	    xgene_pcie_hide_rc_bars(bus, where))
+		return NULL;
+
+	xgene_pcie_set_rtdid_reg(bus, devfn);
+
+	if (busn > cfg->busr.start)
+		base = cfg->win + (1 << cfg->ops->bus_shift);
+	else
+		base = cfg->win;
+
+	return base + where;
+}
+
+static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 *val)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+	struct xgene_pcie_acpi_root *port = cfg->priv;
+
+	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
+	    PCIBIOS_SUCCESSFUL)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	/*
+	* The v1 controller has a bug in its Configuration Request
+	* Retry Status (CRS) logic: when CRS is enabled and we read the
+	* Vendor and Device ID of a non-existent device, the controller
+	* fabricates return data of 0xFFFF0001 ("device exists but is not
+	* ready") instead of 0xFFFFFFFF ("device does not exist").  This
+	* causes the PCI core to retry the read until it times out.
+	* Avoid this by not claiming to support CRS.
+	*/
+	if (pci_is_root_bus(bus) && (port->version == XGENE_PCIE_IP_VER_1) &&
+	    ((where & ~0x3) == ROOT_CAP_AND_CTRL))
+		*val &= ~(PCI_EXP_RTCAP_CRSVIS << 16);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+struct pci_ecam_ops xgene_v1_pcie_ecam_ops = {
+	.bus_shift	= 16,
+	.init		= xgene_v1_pcie_ecam_init,
+	.pci_ops	= {
+		.map_bus	= xgene_pcie_ecam_map_bus,
+		.read		= xgene_pcie_config_read32,
+		.write		= pci_generic_config_write,
+	}
+};
+
+struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops = {
+	.bus_shift	= 16,
+	.init		= xgene_v2_1_pcie_ecam_init,
+	.pci_ops	= {
+		.map_bus	= xgene_pcie_ecam_map_bus,
+		.read		= xgene_pcie_config_read32,
+		.write		= pci_generic_config_write,
+	}
+};
+
+struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops = {
+	.bus_shift	= 16,
+	.init		= xgene_v2_2_pcie_ecam_init,
+	.pci_ops	= {
+		.map_bus	= xgene_pcie_ecam_map_bus,
+		.read		= xgene_pcie_config_read32,
+		.write		= pci_generic_config_write,
+	}
+};
+#endif
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 35f0e81..40da3e7 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -65,6 +65,11 @@  extern struct pci_ecam_ops pci_thunder_pem_ops;
 #ifdef CONFIG_PCI_HOST_THUNDER_ECAM
 extern struct pci_ecam_ops pci_thunder_ecam_ops;
 #endif
+#ifdef CONFIG_PCI_XGENE
+extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops;
+extern struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops;
+extern struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops;
+#endif
 
 #ifdef CONFIG_PCI_HOST_GENERIC
 /* for DT-based PCI controllers that support ECAM */