Re: [PATCH V3 2/2] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers

From: Bjorn Helgaas
Date: Wed Nov 02 2016 - 19:41:08 EST

Next message: Julius Werner: "[PATCH] clk: rockchip: Ignore frac divisor for PLL equivalence when it's unused"
Previous message: Mrs Julie Leach: "Grant Benefit"
Next in thread: Dongdong Liu: "Re: [PATCH V3 2/2] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Oct 20, 2016 at 11:10:34AM +0800, Dongdong Liu wrote:
> PCIe controller in Hip05/HIP06/HIP07 SoCs is not ECAM compliant.
> It is non ECAM only for the RC bus config space;for any other bus
> underneath the root bus we support ECAM access.
> Add specific quirks for PCI config space accessors.This involves:
> 1. New initialization call hisi_pcie_init() to obtain rc base
> addresses from PNP0C02 as subdevice of PNP0A03.
> 2. New entry in common quirk array.
>
> Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/acpi/pci_mcfg.c | 15 ++++
> drivers/pci/host/Kconfig | 8 ++
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-hisi-acpi.c | 170 ++++++++++++++++++++++++++++++++++++++
> include/linux/pci-ecam.h | 4 +
> 6 files changed, 199 insertions(+)
> create mode 100755 drivers/pci/host/pcie-hisi-acpi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1cd38a7..b224caa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9358,6 +9358,7 @@ L: linux-pci@xxxxxxxxxxxxxxx
> S: Maintained
> F: Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> F: drivers/pci/host/pcie-hisi.c
> +F: drivers/pci/host/pcie-hisi-acpi.c
>
> PCIE DRIVER FOR ROCKCHIP
> M: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index bb2c508..135a9b4 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -96,6 +96,21 @@ struct mcfg_fixup {
> THUNDER_ECAM_MCFG(2, 12),
> THUNDER_ECAM_MCFG(2, 13),
> #endif
> +#define PCI_ACPI_QUIRK_QUAD_DOM(table_id, seg, ops) \
> + { "HISI ", table_id, 0, seg + 0, MCFG_BUS_ANY, ops }, \
> + { "HISI ", table_id, 0, seg + 1, MCFG_BUS_ANY, ops }, \
> + { "HISI ", table_id, 0, seg + 2, MCFG_BUS_ANY, ops }, \
> + { "HISI ", table_id, 0, seg + 3, MCFG_BUS_ANY, ops }
> +
> +#ifdef CONFIG_PCI_HISI_ACPI
> + PCI_ACPI_QUIRK_QUAD_DOM("HIP05 ", 0, &hisi_pcie_ops),
> + PCI_ACPI_QUIRK_QUAD_DOM("HIP06 ", 0, &hisi_pcie_ops),
> + PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 0, &hisi_pcie_ops),
> + PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 4, &hisi_pcie_ops),
> + PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 8, &hisi_pcie_ops),
> + PCI_ACPI_QUIRK_QUAD_DOM("HIP07 ", 12, &hisi_pcie_ops),
> +#endif
> +
> };
>
> static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index ae98644..9ff2bcd 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -227,6 +227,14 @@ config PCI_HISI
> Say Y here if you want PCIe controller support on HiSilicon
> Hip05 and Hip06 and Hip07 SoCs
>
> +config PCI_HISI_ACPI
> + depends on ACPI && ARM64
> + bool "HiSilicon Hip05 and Hip06 and Hip07 SoCs ACPI PCIe controllers"
> + select PNP
> + help
> + Say Y here if you want ACPI PCIe controller support on HiSilicon
> + Hip05 and Hip06 and Hip07 SoCs
> +
> config PCIE_QCOM
> bool "Qualcomm PCIe controller"
> depends on ARCH_QCOM && OF
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 084cb49..9402858 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> 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_PCI_HISI_ACPI) += pcie-hisi-acpi.o
> obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
> obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
> diff --git a/drivers/pci/host/pcie-hisi-acpi.c b/drivers/pci/host/pcie-hisi-acpi.c
> new file mode 100755
> index 0000000..5650d91
> --- /dev/null
> +++ b/drivers/pci/host/pcie-hisi-acpi.c
> @@ -0,0 +1,170 @@
> +/*
> + * PCIe host controller driver for HiSilicon HipXX SoCs
> + *
> + * Copyright (C) 2016 HiSilicon Co., Ltd. http://www.hisilicon.com
> + *
> + * Author: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> + * Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> + *
> + * 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.
> + */
> +#include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +#define DEBUG0 0x728
> +#define PCIE_LTSSM_LINKUP_STATE 0x11
> +#define PCIE_LTSSM_STATE_MASK 0x3F
> +
> +static const struct acpi_device_id hisi_pcie_rc_res_ids[] = {
> + {"HISI0081", 0},
> + {"", 0},
> +};
> +
> +static int hisi_pcie_link_up_acpi(struct pci_config_window *cfg)
> +{
> + u32 val;
> + void __iomem *reg_base = cfg->priv;
> +
> + val = readl(reg_base + DEBUG0);
> + return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE);
> +}

Maybe reorder this so hisi_pcie_link_up_acpi() is next to the caller,
without the config accessors in between?

> +static int hisi_pcie_acpi_rd_conf(struct pci_bus *bus, u32 devfn, int where,
> + int size, u32 *val)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> + int dev = PCI_SLOT(devfn);
> +
> + if (bus->number == cfg->busr.start) {
> + /* access only one slot on each root port */
> + if (dev > 0)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> + else
> + return pci_generic_config_read32(bus, devfn, where,
> + size, val);
> + }
> +
> + return pci_generic_config_read(bus, devfn, where, size, val);
> +}
> +
> +static int hisi_pcie_acpi_wr_conf(struct pci_bus *bus, u32 devfn,
> + int where, int size, u32 val)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> + int dev = PCI_SLOT(devfn);
> +
> + if (bus->number == cfg->busr.start) {
> + /* access only one slot on each root port */
> + if (dev > 0)
> + return PCIBIOS_DEVICE_NOT_FOUND;
> + else
> + return pci_generic_config_write32(bus, devfn, where,
> + size, val);
> + }
> +
> + return pci_generic_config_write(bus, devfn, where, size, val);
> +}

Seems like we could *almost* get rid of these and use only the generic
32-bit accessors:

struct pci_ecam_ops hisi_pcie_ops = {
.bus_shift = 20,
.init = hisi_pcie_init,
.pci_ops = {
.map_bus = hisi_pcie_map_bus,
.read = pci_generic_config_read32,
.write = pci_generic_config_write32,
}
}

We'd be using the 32-bit accessors on the root bus when it's not
strictly necessary. And we'd give up the "dev > 0" check. Not sure
how important those are, though. If "dev > 0" accesses just fail
naturally, there'd be no need to do the check.

> +static void __iomem *hisi_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> + int where)
> +{
> + struct pci_config_window *cfg = bus->sysdata;
> + void __iomem *reg_base = cfg->priv;
> +
> + if (bus->number == cfg->busr.start)
> + return reg_base + where;
> + else
> + return pci_ecam_map_bus(bus, devfn, where);
> +}
> +
> +/*
> + * Retrieve RC base and size from sub-device under the RC
> + * Device (RES1)
> + * {
> + * Name (_HID, "HISI0081")
> + * Name (_CID, "PNP0C02")
> + * Name (_CRS, ResourceTemplate (){
> + * Memory32Fixed (ReadWrite, 0xb0080000, 0x10000)
> + * })
> + * }
> + */
> +static int hisi_pcie_rc_addr_get(struct acpi_device *adev,
> + void __iomem **addr)
> +{
> + struct acpi_device *child_adev;
> + struct list_head list;
> + struct resource *res;
> + struct resource_entry *entry;
> + unsigned long flags;
> + int ret;
> +
> + list_for_each_entry(child_adev, &adev->children, node) {
> + ret = acpi_match_device_ids(child_adev, hisi_pcie_rc_res_ids);
> + if (ret)
> + continue;
> +
> + INIT_LIST_HEAD(&list);
> + flags = IORESOURCE_MEM;
> + ret = acpi_dev_get_resources(child_adev, &list,
> + acpi_dev_filter_resource_type_cb,
> + (void *)flags);
> + if (ret < 0) {
> + dev_err(&child_adev->dev,
> + "failed to parse _CRS method, error code %d\n",
> + ret);
> + return ret;
> + } else if (ret == 0) {
> + dev_err(&child_adev->dev,
> + "no IO and memory resources present in _CRS\n");
> + return -EINVAL;
> + }
> +
> + entry = list_first_entry(&list, struct resource_entry, node);
> + res = entry->res;
> + *addr = devm_ioremap(&child_adev->dev,
> + res->start, resource_size(res));
> + acpi_dev_free_resource_list(&list);
> + if (IS_ERR(*addr)) {
> + dev_err(&child_adev->dev, "error with ioremap\n");
> + return -ENOMEM;
> + }
> +
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int hisi_pcie_init(struct pci_config_window *cfg)
> +{
> + int ret;
> + struct acpi_device *adev = to_acpi_device(cfg->parent);
> + void __iomem *reg_base;
> +
> + ret = hisi_pcie_rc_addr_get(adev, &reg_base);
> + if (ret) {
> + dev_err(&adev->dev, "can't get rc base address");
> + return ret;
> + }
> +
> + cfg->priv = reg_base;
> + if (!hisi_pcie_link_up_acpi(cfg)) {
> + dev_err(&adev->dev, "link status is down\n");
> + return -EINVAL;
> + }

How necessary is this link_up check? The link can go down
asynchronously anyway, at any time after we do this check, so why
bother doing it here?

If we don't need to do it, we don't really need to know the register
addresses here, do we? Maybe we can just fall back to the standard
x86 solution of a PNP0C02 device at the ACPI root that has _CRS to
cover the host bridge registers?

It'd be nice if we didn't have to grub around in the ACPI devices
looking for a corresponding device. That's sort of a new solution to
a problem that will go away soon anyway (assuming future hardware is
more spec-conforming as far as ECAM).

> + return 0;
> +}
> +
> +struct pci_ecam_ops hisi_pcie_ops = {
> + .bus_shift = 20,
> + .init = hisi_pcie_init,
> + .pci_ops = {
> + .map_bus = hisi_pcie_map_bus,
> + .read = hisi_pcie_acpi_rd_conf,
> + .write = hisi_pcie_acpi_wr_conf,
> + }
> +};
> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
> index 35f0e81..2db4db8 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -66,6 +66,10 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
> extern struct pci_ecam_ops pci_thunder_ecam_ops;
> #endif
>
> +#ifdef CONFIG_PCI_HISI_ACPI
> +extern struct pci_ecam_ops hisi_pcie_ops;
> +#endif
> +
> #ifdef CONFIG_PCI_HOST_GENERIC
> /* for DT-based PCI controllers that support ECAM */
> int pci_host_common_probe(struct platform_device *pdev,
> --
> 1.9.1
>

Next message: Julius Werner: "[PATCH] clk: rockchip: Ignore frac divisor for PLL equivalence when it's unused"
Previous message: Mrs Julie Leach: "Grant Benefit"
Next in thread: Dongdong Liu: "Re: [PATCH V3 2/2] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]