[PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller
Duc Dang
dhdang at apm.com
Thu Dec 1 18:52:23 PST 2016
On Thu, Dec 1, 2016 at 10:33 AM, Bjorn Helgaas <helgaas at kernel.org> wrote:
> Hi Duc,
>
> On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>> PCIe controllers in X-Gene SoCs is not ECAM compliant: software
>> needs to configure additional controller's register to address
>> device at bus:dev:function.
>>
>> 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).
>>
>> The quirk declares the X-Gene PCIe controller register space as 64KB
>> fixed memory resource with name "PCIe CSR". This name will be showed
>> next to the resource range in the output of "cat /proc/iomem".
>>
>> Signed-off-by: Duc Dang <dhdang at apm.com>
>> ---
>> v3:
>> - Rebase on top of pci/ecam-v6 tree.
>> - Use DEFINE_RES_MEM_NAMED to declare controller register space
>> with name "PCIe CSR"
>> v2:
>> RFC v2: https://patchwork.ozlabs.org/patch/686846/
>> v1:
>> RFC v1: https://patchwork.kernel.org/patch/9337115/
>>
>> drivers/acpi/pci_mcfg.c | 31 ++++++++
>> drivers/pci/host/pci-xgene.c | 165 ++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/pci-ecam.h | 7 ++
>> 3 files changed, 200 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index ac21db3..eb6125b 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -57,6 +57,37 @@ struct mcfg_fixup {
>> { "QCOM ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, &pci_32b_ops },
>> { "QCOM ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, &pci_32b_ops },
>> { "QCOM ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, &pci_32b_ops },
>> +
>> +#ifdef CONFIG_PCI_XGENE
>
> As you've no doubt noticed, I'm proposing to add these quirks without
> CONFIG_PCI_XGENE, so we don't have to select each device when building
> a generic ACPI kernel.
>
> I'm also proposing some Kconfig and Makefile changes so we don't build
> the platform driver part in a generic ACPI kernel (unless we *also*
> explicitly select the platform driver).
>
> Here's an example:
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ecam&id=f80edf4d6c05
I made similar changes in v4 patch. The ECAM quirk will be built when
ACPI and PCI_QUIRKS are enabled.
When building for DT only, the ECAM quirk won't be compiled.
>
>> +#define XGENE_V1_ECAM_MCFG(rev, seg) \
>> + {"APM ", "XGENE ", rev, seg, MCFG_BUS_ANY, \
>> + &xgene_v1_pcie_ecam_ops }
>> +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
>> + {"APM ", "XGENE ", rev, seg, MCFG_BUS_ANY, \
>> + &xgene_v2_1_pcie_ecam_ops }
>> +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
>> + {"APM ", "XGENE ", rev, seg, MCFG_BUS_ANY, \
>> + &xgene_v2_2_pcie_ecam_ops }
>> +
>> + /* X-Gene SoC with v1 PCIe controller */
>> + XGENE_V1_ECAM_MCFG(1, 0),
>> + XGENE_V1_ECAM_MCFG(1, 1),
>
>> @@ -64,6 +66,7 @@
>> /* PCIe IP version */
>> #define XGENE_PCIE_IP_VER_UNKN 0
>> #define XGENE_PCIE_IP_VER_1 1
>> +#define XGENE_PCIE_IP_VER_2 2
>
> This isn't used anywhere, which makes me wonder whether it's worth
> keeping it.
V2 controller will use this XGENE_PCIE_IP_VER_2 (port->version =
XGENE_PCIE_IP_VER_2). This will be used to indicate that the
controller is V2, and to enable configuration request retry status
feature (by not disable it like V1 controller).
>
>> static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>> {
>> - struct xgene_pcie_port *port = bus->sysdata;
>> + struct pci_config_window *cfg;
>> + struct xgene_pcie_port *port;
>> +
>> + if (acpi_disabled)
>> + port = bus->sysdata;
>> + else {
>> + cfg = bus->sysdata;
>> + port = cfg->priv;
>> + }
>
> I would really, really like to figure out a way to get rid of these
> "if (acpi_disabled)" checks sprinkled through here. Is there any way
> we can set up bus->sysdata to be the same, regardless of whether we're
> using this as a platform driver or an ACPI quirk?
Right now, I created a inline function to extract xgene_pcie_port from
pci_bus. In order to get rid of acpi_disabled, I will need to make
sysdata in DT case also point to pci_config_window structure, which
means I will need to convert and test the DT driver to use ecam ops.
It is a separate patch itself. So I think I should do it at later time
(after this ECAM quirk patch). I hope you are ok with this.
>
>> +#ifdef CONFIG_ACPI
>
> You've probably noticed that I've been using
>
> #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
>
> in this situation, mostly to make it clear that this is part of a
> workaround. I don't want people to blindly copy this stuff without
> realizing that it's a workaround for a hardware issue.
Yes, I used defined(CONFIG_PCI_QUIRKS) in v4 patch as well.
>
>> +static struct resource xgene_v1_csr_res[] = {
>> + [0] = DEFINE_RES_MEM_NAMED(0x1f2b0000UL, SZ_64K, "PCIe CSR"),
>> + [1] = DEFINE_RES_MEM_NAMED(0x1f2c0000UL, SZ_64K, "PCIe CSR"),
>> + [2] = DEFINE_RES_MEM_NAMED(0x1f2d0000UL, SZ_64K, "PCIe CSR"),
>> + [3] = DEFINE_RES_MEM_NAMED(0x1f500000UL, SZ_64K, "PCIe CSR"),
>> + [4] = DEFINE_RES_MEM_NAMED(0x1f510000UL, SZ_64K, "PCIe CSR"),
>
> I assume these ranges are not the actual ECAM space, right?
> If they *were* ECAM, I assume you would have included them in the
> quirk itself in the mcfg_quirks[] table.
Yes, as Mark also pointed out. These are MMIO space for controller registers.
>
>> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>> +{
>> + struct acpi_device *adev = to_acpi_device(cfg->parent);
>> + struct acpi_pci_root *root = acpi_driver_data(adev);
>> + struct device *dev = cfg->parent;
>> + struct xgene_pcie_port *port;
>> + struct resource *csr;
>> +
>> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> + if (!port)
>> + return -ENOMEM;
>> +
>> + csr = &xgene_v1_csr_res[root->segment];
>
> This makes me nervous because root->segment comes from the ACPI _SEG,
> and if firmware gives us junk in _SEG, we will reference something in
> the weeds.
I use Mark's approach in v4 patch (discover the MMIO space using
acpi_dev_get_resources. Both approach have pros and cons. I can also
fallback to hard-coded resource array if you want to, but as you
mentioned right above, we will need to rely on firmware for correct
_SEG information.
I need to define the function (xgene_get_csr_resource()) inside
pci-xgene.c to duplicate the code of acpi_get_rc_addr. The reason is
X-Gene firmware does not have a dedicate PNP0C02 node to declare the
resource, and if I use acpi_get_rc_resources() with "PNP0A08", I got
error due to acpi_bus_get_device() returns error.
>
>> + port->csr_base = devm_ioremap_resource(dev, csr);
>> + if (IS_ERR(port->csr_base)) {
>> + kfree(port);
>> + return -ENOMEM;
>> + }
>> +
>> + port->cfg_base = cfg->win;
>> + port->version = XGENE_PCIE_IP_VER_1;
>> +
>> + cfg->priv = port;
>
> All these init functions are almost identical. Can we factor this out
> by having wrappers that do nothing more than pass in the table and
> version, and put the kzalloc and ioremap in a shared back-end?
I refactor-ed these .init functions. And as a result, there are only 2
ecam ops left: xgene_v1_pcie_ecam_ops and xgene_v2_pcie_ecam_ops.
>
> We're so close I can taste it! I can't wait to see all this work come
> to fruition.
>
> Bjorn
Regards,
Duc Dang.
More information about the linux-arm-kernel
mailing list