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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Bjorn,

Thanks for reviewing my RFC patch.

On Mon, Sep 19, 2016 at 1:06 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxx>
>> ---
>>  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@xxxxxxx>
>> + *
>> + * 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux