[PATCH V5 0/2] Add ACPI support for HiSilicon SoCs Host Controllers

[PATCH V5 0/2] Add ACPI support for HiSilicon SoCs Host Controllers

[Dongdong Liu]

This patchset adds ACPI support for the HiSilicon Hip05/Hip06/Hip07 SoC
PCIe controllers.
The two patches respectively:
        - provides the common function acpi_get_rc_resources() for ARM64
          platform.
        - adds the HiSilicon ACPI specific quirks.

This patchset is based on branch pci/ecam-v6
It can be found here:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git(pci/ecam-v6)

This patchset has been tested on HiSilicon D03 board.
The dmesg log, /proc/iomem, and ACPI table information can be found:
https://bugzilla.kernel.org/show_bug.cgi?id=187961

v4 -> v5:
- obtain rc base addresses from PNP0C02 at the root of the ACPI
namespace (under \_SB) instead of from sub-device under the RC.
- merge the rewrited get rc resources code by Tomasz.
- delete unused code. 
- drop the PATCH V4 1/2, will rework late as a seperate patch.

v3 -> v4:
- rebase on pci/ecam-v6.
- delete the unnecessary link_up check code. 

v2 -> v3:
- rebase against 4.9-rc1 and add Tomasz quirks V6 pathcset. 
- obtain rc base addresses from PNP0C02 as subdevice of PNP0A03 instead of
  hardcode the addresses.
- modify hisi_pcie_acpi_rd_conf/hisi_pcie_acpi_wr_conf() according to
  Arnd comments.

v1 -> v2:
- rebase against Tomasz RFC V5 quirk mechanism
- add ACPI support for the HiSilicon Hip07 SoC PCIe controllers.

Dongdong Liu (2):
  PCI/ACPI: Provide acpi_get_rc_resources() for ARM64 platform
  PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers

 MAINTAINERS                       |   1 +
 drivers/acpi/pci_mcfg.c           |  13 ++++
 drivers/pci/host/Kconfig          |   8 +++
 drivers/pci/host/Makefile         |   1 +
 drivers/pci/host/pcie-hisi-acpi.c | 124 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-acpi.c            |  71 ++++++++++++++++++++++
 drivers/pci/pci.h                 |   4 ++
 include/linux/pci-ecam.h          |   5 ++
 8 files changed, 227 insertions(+)
 create mode 100644 drivers/pci/host/pcie-hisi-acpi.c

-- 
1.9.1

[PATCH V5 1/2] PCI/ACPI: Provide acpi_get_rc_resources() for ARM64 platform

[Dongdong Liu]

The acpi_get_rc_resources() is used to get the RC register address that can
not be described in MCFG. It takes the _HID&segment to look for and outputs
the RC address resource. Use PNP0C02 devices to describe such RC address
resource. Use _UID to match segment to tell which root bus the PNP0C02
resource belong to.

Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/pci/pci-acpi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h      |  4 +++
 2 files changed, 75 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index d966d47..3557e3a 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -29,6 +29,77 @@
 	0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
 };
 
+#ifdef CONFIG_ARM64
+static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
+{
+	struct resource_entry *entry;
+	struct list_head list;
+	unsigned long flags;
+	int ret;
+
+	INIT_LIST_HEAD(&list);
+	flags = IORESOURCE_MEM;
+	ret = acpi_dev_get_resources(adev, &list,
+				     acpi_dev_filter_resource_type_cb,
+				     (void *) flags);
+	if (ret < 0) {
+		dev_err(&adev->dev,
+			"failed to parse _CRS method, error code %d\n", ret);
+		return ret;
+	} else if (ret == 0) {
+		dev_err(&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;
+	acpi_dev_free_resource_list(&list);
+	return 0;
+}
+
+static acpi_status acpi_match_rc(acpi_handle handle, u32 lvl, void *context,
+				 void **retval)
+{
+	u16 *segment = context;
+	unsigned long long uid;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
+	if (ACPI_FAILURE(status) || uid != *segment)
+		return AE_CTRL_DEPTH;
+
+	*(acpi_handle *)retval = handle;
+	return AE_CTRL_TERMINATE;
+}
+
+int acpi_get_rc_resources(const char *hid, u16 segment, struct resource *res)
+{
+	struct acpi_device *adev;
+	acpi_status status;
+	acpi_handle handle;
+	int ret;
+
+	status = acpi_get_devices(hid, acpi_match_rc, &segment, &handle);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Can't find _HID %s device", hid);
+		return -ENODEV;
+	}
+
+	ret = acpi_bus_get_device(handle, &adev);
+	if (ret)
+		return ret;
+
+	ret = acpi_get_rc_addr(adev, res);
+	if (ret) {
+		dev_err(&adev->dev, "can't get RC resource");
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
 phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
 {
 	acpi_status status = AE_NOT_EXIST;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4518562..17ffa38 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -356,4 +356,8 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 }
 #endif
 
+#ifdef CONFIG_ARM64
+int acpi_get_rc_resources(const char *hid, u16 segment, struct resource *res);
+#endif
+
 #endif /* DRIVERS_PCI_H */
-- 
1.9.1

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

[Dongdong Liu]

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 at the root of the ACPI namespace (under \_SB).
2. New entry in common quirk array.

Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 MAINTAINERS                       |   1 +
 drivers/acpi/pci_mcfg.c           |  13 ++++
 drivers/pci/host/Kconfig          |   8 +++
 drivers/pci/host/Makefile         |   1 +
 drivers/pci/host/pcie-hisi-acpi.c | 124 ++++++++++++++++++++++++++++++++++++++
 include/linux/pci-ecam.h          |   5 ++
 6 files changed, 152 insertions(+)
 create mode 100644 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@vger.kernel.org
 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@rock-chips.com>
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index ac21db3..b1b6fc7 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -57,6 +57,19 @@ 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_HISI_ACPI
+	#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 }
+	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 100644
index 0000000..358c7c9
--- /dev/null
+++ b/drivers/pci/host/pcie-hisi-acpi.c
@@ -0,0 +1,124 @@
+/*
+ * PCIe host controller driver for HiSilicon HipXX SoCs
+ *
+ * Copyright (C) 2016 HiSilicon Co., Ltd. http://www.hisilicon.com
+ *
+ * Author: Dongdong Liu <liudongdong3@huawei.com>
+ *         Gabriele Paoloni <gabriele.paoloni@huawei.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.
+ */
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
+#include "../pci.h"
+
+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);
+}
+
+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 PNP0C02 at the root of the ACPI namespace
+ * (under \_SB).
+ * Scope(_SB)
+ * {
+ *   Device (PCI0)
+ *   {
+ *     Name(_HID, "PNP0A08")
+ *     Name(_CID, "PNP0A03")
+ *     Name(_SEG, 0)
+ *     ......
+ *   }
+ *   Device (RES0)
+ *   {
+ *     Name(_HID, "HISI0081")
+ *     Name(_CID, "PNP0C02")
+ *     Name(_UID, 0x0)
+ *     Name(_CRS, ResourceTemplate (){
+ *       Memory32Fixed (ReadWrite, 0xa0090000, 0x10000)
+ *     })
+ *   }
+ *   ......
+ * }
+ */
+static int hisi_pcie_init(struct pci_config_window *cfg)
+{
+	struct acpi_device *adev = to_acpi_device(cfg->parent);
+	struct acpi_pci_root *root = acpi_driver_data(adev);
+	void __iomem *reg_base;
+	struct resource *res;
+	int ret;
+
+	res = devm_kzalloc(&adev->dev, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+
+	ret = acpi_get_rc_resources("HISI0081", root->segment, res);
+	if (ret) {
+		dev_err(&adev->dev, "can't get rc base address");
+		return ret;
+	}
+
+	reg_base = devm_ioremap(&adev->dev, res->start, resource_size(res));
+	if (!reg_base)
+		return -ENOMEM;
+
+	cfg->priv = reg_base;
+	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 f5740b7..1b24d97 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -67,4 +67,9 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
 int pci_host_common_probe(struct platform_device *pdev,
 			  struct pci_ecam_ops *ops);
 #endif
+
+#ifdef CONFIG_PCI_HISI_ACPI
+extern struct pci_ecam_ops hisi_pcie_ops;
+#endif
+
 #endif
-- 
1.9.1

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

[Bjorn Helgaas]

Hi guys,

On Fri, Nov 18, 2016 at 05:22:31PM +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 at the root of the ACPI namespace (under \_SB).
> 2. New entry in common quirk array.

This is great!  I think we're almost there.

The only real technical issue I see is that based on the logs at
https://bugzilla.kernel.org/show_bug.cgi?id=187961 (thank you very
much for them!), there is no _CRS description of the ECAM space.

You have a PNP0C02 device for every host bridge, which is good.  It
describes the RC register space, as it should.  But it should *also*
describe the ECAM space, which it currently does not.

The result is this in /proc/iomem:

  a0200000-a020ffff : pnp 00:01           # PCI1 registers, from _CRS
  a0090000-a009ffff : pnp 00:00           # PCI0 registers, from _CRS
  a00a0000-a00affff : pnp 00:02           # PCI2 registers, from _CRS
  a8000000-a9ffffff : PCI ECAM            # pci/ecam.c request for PCI2
  b0000000-b1ffffff : PCI ECAM            # pci/ecam.c request for PCI0
  be000000-bfffffff : PCI ECAM            # pci/ecam.c request for PCI1

We *should* have this:

  a0200000-a020ffff : pnp 00:01           # PCI1 registers, from _CRS
  a0090000-a009ffff : pnp 00:00           # PCI0 registers, from _CRS
  a00a0000-a00affff : pnp 00:02           # PCI2 registers, from _CRS
  a8000000-a9ffffff : pnp 00:02           # PCI2 ECAM space, from _CRS
    a8000000-a9ffffff : PCI ECAM          # pci/ecam.c request for PCI2
  b0000000-b1ffffff : pnp 00:00           # PCI0 ECAM space, from _CRS
    b0000000-b1ffffff : PCI ECAM          # pci/ecam.c request for PCI0
  be000000-bfffffff : pnp 00:01           # PCI1 ECAM space, from _CRS
    be000000-bfffffff : PCI ECAM          # pci/ecam.c request for PCI1

If you add the ECAM space to the HISI0081 _CRS and it doesn't change
the /proc/iomem contents, don't worry.  Those "pnp 00:xx" reservations
are done by pnp/system.c, and we currently do those out of order
(after the ECAM requests), so they might fail.  For example, on my x86
laptop, I have this:

  PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-0xfbffffff] (base 0xf8000000)
  system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved

Another minor comment/question below.

> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  MAINTAINERS                       |   1 +
>  drivers/acpi/pci_mcfg.c           |  13 ++++
>  drivers/pci/host/Kconfig          |   8 +++
>  drivers/pci/host/Makefile         |   1 +
>  drivers/pci/host/pcie-hisi-acpi.c | 124 ++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-ecam.h          |   5 ++
>  6 files changed, 152 insertions(+)
>  create mode 100644 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@vger.kernel.org
>  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@rock-chips.com>
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index ac21db3..b1b6fc7 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -57,6 +57,19 @@ 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_HISI_ACPI
> +	#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 }
> +	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

I'm not sure about this Kconfig setup.  Do we really want to force
people to enable a special config option to get this support?

I'm comparing it in my mind with other PCI quirks.  They're all
enabled as a group by CONFIG_PCI_QUIRKS.  Ultimately we want an ACPI
kernel to work without requiring any platform-specific config options.

I'm wondering if we should consolidate all the ECAM quirk code in a
single place (maybe pci/ecam-quirks.c, pci/ecam.c, or pci/pci-acpi.c),
under a config option like CONFIG_PCI_ECAM_QUIRKS or maybe even plain
CONFIG_PCI_QUIRKS (of course, it could still be qualified by
CONFIG_ACPI and CONFIG_ARM64).

>  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 100644
> index 0000000..358c7c9
> --- /dev/null
> +++ b/drivers/pci/host/pcie-hisi-acpi.c
> @@ -0,0 +1,124 @@
> +/*
> + * PCIe host controller driver for HiSilicon HipXX SoCs
> + *
> + * Copyright (C) 2016 HiSilicon Co., Ltd. http://www.hisilicon.com
> + *
> + * Author: Dongdong Liu <liudongdong3@huawei.com>
> + *         Gabriele Paoloni <gabriele.paoloni@huawei.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.
> + */
> +#include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +#include "../pci.h"
> +
> +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);
> +}
> +
> +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 PNP0C02 at the root of the ACPI namespace
> + * (under \_SB).
> + * Scope(_SB)
> + * {
> + *   Device (PCI0)
> + *   {
> + *     Name(_HID, "PNP0A08")
> + *     Name(_CID, "PNP0A03")
> + *     Name(_SEG, 0)
> + *     ......
> + *   }
> + *   Device (RES0)
> + *   {
> + *     Name(_HID, "HISI0081")
> + *     Name(_CID, "PNP0C02")
> + *     Name(_UID, 0x0)
> + *     Name(_CRS, ResourceTemplate (){
> + *       Memory32Fixed (ReadWrite, 0xa0090000, 0x10000)
> + *     })
> + *   }
> + *   ......
> + * }
> + */
> +static int hisi_pcie_init(struct pci_config_window *cfg)
> +{
> +	struct acpi_device *adev = to_acpi_device(cfg->parent);
> +	struct acpi_pci_root *root = acpi_driver_data(adev);
> +	void __iomem *reg_base;
> +	struct resource *res;
> +	int ret;
> +
> +	res = devm_kzalloc(&adev->dev, sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return -ENOMEM;
> +
> +	ret = acpi_get_rc_resources("HISI0081", root->segment, res);
> +	if (ret) {
> +		dev_err(&adev->dev, "can't get rc base address");
> +		return ret;
> +	}
> +
> +	reg_base = devm_ioremap(&adev->dev, res->start, resource_size(res));
> +	if (!reg_base)
> +		return -ENOMEM;
> +
> +	cfg->priv = reg_base;
> +	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 f5740b7..1b24d97 100644
> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -67,4 +67,9 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>  int pci_host_common_probe(struct platform_device *pdev,
>  			  struct pci_ecam_ops *ops);
>  #endif
> +
> +#ifdef CONFIG_PCI_HISI_ACPI
> +extern struct pci_ecam_ops hisi_pcie_ops;
> +#endif
> +
>  #endif
> -- 
> 1.9.1
> 

Re: [PATCH V5 1/2] PCI/ACPI: Provide acpi_get_rc_resources() for ARM64 platform

[Rafael J. Wysocki]

On Fri, Nov 18, 2016 at 10:22 AM, Dongdong Liu <liudongdong3@huawei.com> wrote:
> The acpi_get_rc_resources() is used to get the RC register address that can
> not be described in MCFG. It takes the _HID&segment to look for and outputs
> the RC address resource. Use PNP0C02 devices to describe such RC address
> resource. Use _UID to match segment to tell which root bus the PNP0C02
> resource belong to.
>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/pci/pci-acpi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h      |  4 +++
>  2 files changed, 75 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index d966d47..3557e3a 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -29,6 +29,77 @@
>         0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
>  };
>
> +#ifdef CONFIG_ARM64
> +static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)

Why can't it return a resource pointer?

> +{
> +       struct resource_entry *entry;
> +       struct list_head list;
> +       unsigned long flags;
> +       int ret;
> +
> +       INIT_LIST_HEAD(&list);
> +       flags = IORESOURCE_MEM;
> +       ret = acpi_dev_get_resources(adev, &list,
> +                                    acpi_dev_filter_resource_type_cb,
> +                                    (void *) flags);
> +       if (ret < 0) {
> +               dev_err(&adev->dev,

The dev_err() log level is quite excessive here IMO.  Why is the
message useful to users at all?  IOW, what is the user supposed to do
if this message is present in the log?

> +                       "failed to parse _CRS method, error code %d\n", ret);
> +               return ret;
> +       } else if (ret == 0) {
> +               dev_err(&adev->dev,
> +                       "no IO and memory resources present in _CRS\n");

Same here.

> +               return -EINVAL;
> +       }
> +
> +       entry = list_first_entry(&list, struct resource_entry, node);
> +       *res = *entry->res;
> +       acpi_dev_free_resource_list(&list);
> +       return 0;
> +}
> +
> +static acpi_status acpi_match_rc(acpi_handle handle, u32 lvl, void *context,
> +                                void **retval)
> +{
> +       u16 *segment = context;
> +       unsigned long long uid;
> +       acpi_status status;
> +
> +       status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
> +       if (ACPI_FAILURE(status) || uid != *segment)
> +               return AE_CTRL_DEPTH;
> +
> +       *(acpi_handle *)retval = handle;
> +       return AE_CTRL_TERMINATE;
> +}
> +

Please add a kerneldoc comment describing acpi_get_rc_resources().

> +int acpi_get_rc_resources(const char *hid, u16 segment, struct resource *res)
> +{
> +       struct acpi_device *adev;
> +       acpi_status status;
> +       acpi_handle handle;
> +       int ret;
> +
> +       status = acpi_get_devices(hid, acpi_match_rc, &segment, &handle);
> +       if (ACPI_FAILURE(status)) {
> +               pr_err("Can't find _HID %s device", hid);

Same here.

> +               return -ENODEV;
> +       }
> +
> +       ret = acpi_bus_get_device(handle, &adev);
> +       if (ret)
> +               return ret;
> +
> +       ret = acpi_get_rc_addr(adev, res);
> +       if (ret) {
> +               dev_err(&adev->dev, "can't get RC resource");

Same here.

> +               return ret;
> +       }
> +
> +       return 0;
> +}

It looks like this function could return the resource pointer just
fine.  What's the problem with that?

> +#endif
> +
>  phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
>  {
>         acpi_status status = AE_NOT_EXIST;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 4518562..17ffa38 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -356,4 +356,8 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  }
>  #endif
>
> +#ifdef CONFIG_ARM64
> +int acpi_get_rc_resources(const char *hid, u16 segment, struct resource *res);
> +#endif

Doesn't that depend on ACPI too?

> +
>  #endif /* DRIVERS_PCI_H */
> --

Thanks,
Rafael

Re: [PATCH V5 1/2] PCI/ACPI: Provide acpi_get_rc_resources() for ARM64 platform

[Dongdong Liu]

Hi Rafael

Many Thanks for your review.

在 2016/11/19 6:00, Rafael J. Wysocki 写道:
> On Fri, Nov 18, 2016 at 10:22 AM, Dongdong Liu <liudongdong3@huawei.com> wrote:
>> The acpi_get_rc_resources() is used to get the RC register address that can
>> not be described in MCFG. It takes the _HID&segment to look for and outputs
>> the RC address resource. Use PNP0C02 devices to describe such RC address
>> resource. Use _UID to match segment to tell which root bus the PNP0C02
>> resource belong to.
>>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> ---
>>  drivers/pci/pci-acpi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/pci/pci.h      |  4 +++
>>  2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index d966d47..3557e3a 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -29,6 +29,77 @@
>>         0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
>>  };
>>
>> +#ifdef CONFIG_ARM64
>> +static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
>
> Why can't it return a resource pointer?

Yes, it can return a resource pointer.
Good catch. will fix.

>
>> +{
>> +       struct resource_entry *entry;
>> +       struct list_head list;
>> +       unsigned long flags;
>> +       int ret;
>> +
>> +       INIT_LIST_HEAD(&list);
>> +       flags = IORESOURCE_MEM;
>> +       ret = acpi_dev_get_resources(adev, &list,
>> +                                    acpi_dev_filter_resource_type_cb,
>> +                                    (void *) flags);
>> +       if (ret < 0) {
>> +               dev_err(&adev->dev,
>
> The dev_err() log level is quite excessive here IMO.  Why is the
> message useful to users at all?  IOW, what is the user supposed to do
> if this message is present in the log?

Ok, this is not really needed, I will delete it.
>
>> +                       "failed to parse _CRS method, error code %d\n", ret);
>> +               return ret;
>> +       } else if (ret == 0) {
>> +               dev_err(&adev->dev,
>> +                       "no IO and memory resources present in _CRS\n");
>
> Same here.
will delete.
>
>> +               return -EINVAL;
>> +       }
>> +
>> +       entry = list_first_entry(&list, struct resource_entry, node);
>> +       *res = *entry->res;
>> +       acpi_dev_free_resource_list(&list);
>> +       return 0;
>> +}
>> +
>> +static acpi_status acpi_match_rc(acpi_handle handle, u32 lvl, void *context,
>> +                                void **retval)
>> +{
>> +       u16 *segment = context;
>> +       unsigned long long uid;
>> +       acpi_status status;
>> +
>> +       status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
>> +       if (ACPI_FAILURE(status) || uid != *segment)
>> +               return AE_CTRL_DEPTH;
>> +
>> +       *(acpi_handle *)retval = handle;
>> +       return AE_CTRL_TERMINATE;
>> +}
>> +
>
> Please add a kerneldoc comment describing acpi_get_rc_resources().
OK, will do.
>
>> +int acpi_get_rc_resources(const char *hid, u16 segment, struct resource *res)
>> +{
>> +       struct acpi_device *adev;
>> +       acpi_status status;
>> +       acpi_handle handle;
>> +       int ret;
>> +
>> +       status = acpi_get_devices(hid, acpi_match_rc, &segment, &handle);
>> +       if (ACPI_FAILURE(status)) {
>> +               pr_err("Can't find _HID %s device", hid);
>
> Same here.

will delete
>
>> +               return -ENODEV;
>> +       }
>> +
>> +       ret = acpi_bus_get_device(handle, &adev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = acpi_get_rc_addr(adev, res);
>> +       if (ret) {
>> +               dev_err(&adev->dev, "can't get RC resource");
>
> Same here.

will delete
>
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>
> It looks like this function could return the resource pointer just
> fine.  What's the problem with that?
It is ok to return the resource pointer, will fix it.
>
>> +#endif
>> +
>>  phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
>>  {
>>         acpi_status status = AE_NOT_EXIST;
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 4518562..17ffa38 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -356,4 +356,8 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>>  }
>>  #endif
>>
>> +#ifdef CONFIG_ARM64
>> +int acpi_get_rc_resources(const char *hid, u16 segment, struct resource *res);
>> +#endif
>
> Doesn't that depend on ACPI too?

Yes, that depends on ACPI too.
will fix.

Thanks
Dongdong.
>
>> +
>>  #endif /* DRIVERS_PCI_H */
>> --
>
> Thanks,
> Rafael
>
> .
>

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

[Gabriele Paoloni]

Hi Bjorn 

Many thanks for your review

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Bjorn Helgaas
> Sent: 18 November 2016 17:08
> To: liudongdong (C)
> Cc: arnd@arndb.de; rafael@kernel.org; Lorenzo.Pieralisi@arm.com;
> tn@semihalf.com; Wangzhou (B); pratyush.anand@gmail.com; linux-
> pci@vger.kernel.org; linux-acpi@vger.kernel.org; linux-
> kernel@vger.kernel.org; jcm@redhat.com; Gabriele Paoloni; Chenxin
> (Charles); hanjun.guo@linaro.org; Linuxarm
> Subject: Re: [PATCH V5 2/2] PCI/ACPI: hisi: Add ACPI support for
> HiSilicon SoCs Host Controllers
> 
> Hi guys,
> 
> On Fri, Nov 18, 2016 at 05:22:31PM +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 at the root of the ACPI namespace (under
> \_SB).
> > 2. New entry in common quirk array.
> 
> This is great!  I think we're almost there.
> 
> The only real technical issue I see is that based on the logs at
> https://bugzilla.kernel.org/show_bug.cgi?id=187961 (thank you very
> much for them!), there is no _CRS description of the ECAM space.
> 
> You have a PNP0C02 device for every host bridge, which is good.  It
> describes the RC register space, as it should.  But it should *also*
> describe the ECAM space, which it currently does not.
> 
> The result is this in /proc/iomem:
> 
>   a0200000-a020ffff : pnp 00:01           # PCI1 registers, from _CRS
>   a0090000-a009ffff : pnp 00:00           # PCI0 registers, from _CRS
>   a00a0000-a00affff : pnp 00:02           # PCI2 registers, from _CRS
>   a8000000-a9ffffff : PCI ECAM            # pci/ecam.c request for PCI2
>   b0000000-b1ffffff : PCI ECAM            # pci/ecam.c request for PCI0
>   be000000-bfffffff : PCI ECAM            # pci/ecam.c request for PCI1
> 
> We *should* have this:
> 
>   a0200000-a020ffff : pnp 00:01           # PCI1 registers, from _CRS
>   a0090000-a009ffff : pnp 00:00           # PCI0 registers, from _CRS
>   a00a0000-a00affff : pnp 00:02           # PCI2 registers, from _CRS
>   a8000000-a9ffffff : pnp 00:02           # PCI2 ECAM space, from _CRS
>     a8000000-a9ffffff : PCI ECAM          # pci/ecam.c request for PCI2
>   b0000000-b1ffffff : pnp 00:00           # PCI0 ECAM space, from _CRS
>     b0000000-b1ffffff : PCI ECAM          # pci/ecam.c request for PCI0
>   be000000-bfffffff : pnp 00:01           # PCI1 ECAM space, from _CRS
>     be000000-bfffffff : PCI ECAM          # pci/ecam.c request for PCI1
> 
> If you add the ECAM space to the HISI0081 _CRS and it doesn't change
> the /proc/iomem contents, don't worry.  Those "pnp 00:xx" reservations
> are done by pnp/system.c, and we currently do those out of order
> (after the ECAM requests), so they might fail.  For example, on my x86
> laptop, I have this:
> 
>   PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem 0xf8000000-
> 0xfbffffff] (base 0xf8000000)
>   system 00:01: [mem 0xf8000000-0xfbffffff] could not be reserved

Sure we'll do as you suggested

> 
> Another minor comment/question below.
> 
> > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> > ---
> >  MAINTAINERS                       |   1 +
> >  drivers/acpi/pci_mcfg.c           |  13 ++++
> >  drivers/pci/host/Kconfig          |   8 +++
> >  drivers/pci/host/Makefile         |   1 +
> >  drivers/pci/host/pcie-hisi-acpi.c | 124
> ++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-ecam.h          |   5 ++
> >  6 files changed, 152 insertions(+)
> >  create mode 100644 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@vger.kernel.org
> >  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@rock-chips.com>
> > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> > index ac21db3..b1b6fc7 100644
> > --- a/drivers/acpi/pci_mcfg.c
> > +++ b/drivers/acpi/pci_mcfg.c
> > @@ -57,6 +57,19 @@ 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_HISI_ACPI
> > +	#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 }
> > +	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
> 
> I'm not sure about this Kconfig setup.  Do we really want to force
> people to enable a special config option to get this support?
> 
> I'm comparing it in my mind with other PCI quirks.  They're all
> enabled as a group by CONFIG_PCI_QUIRKS.  Ultimately we want an ACPI
> kernel to work without requiring any platform-specific config options.
> 
> I'm wondering if we should consolidate all the ECAM quirk code in a
> single place (maybe pci/ecam-quirks.c, pci/ecam.c, or pci/pci-acpi.c),
> under a config option like CONFIG_PCI_ECAM_QUIRKS or maybe even plain
> CONFIG_PCI_QUIRKS (of course, it could still be qualified by
> CONFIG_ACPI and CONFIG_ARM64).

What about having a single config options but keeping separate files
for each vendors (at least as first step)?

Maybe if we see that we can consolidate all the vendors in one file
we can do it as a second step...

Thanks

Gab

> 
> >  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 100644
> > index 0000000..358c7c9
> > --- /dev/null
> > +++ b/drivers/pci/host/pcie-hisi-acpi.c
> > @@ -0,0 +1,124 @@
> > +/*
> > + * PCIe host controller driver for HiSilicon HipXX SoCs
> > + *
> > + * Copyright (C) 2016 HiSilicon Co., Ltd. http://www.hisilicon.com
> > + *
> > + * Author: Dongdong Liu <liudongdong3@huawei.com>
> > + *         Gabriele Paoloni <gabriele.paoloni@huawei.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.
> > + */
> > +#include <linux/pci.h>
> > +#include <linux/pci-acpi.h>
> > +#include <linux/pci-ecam.h>
> > +#include "../pci.h"
> > +
> > +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);
> > +}
> > +
> > +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 PNP0C02 at the root of the ACPI
> namespace
> > + * (under \_SB).
> > + * Scope(_SB)
> > + * {
> > + *   Device (PCI0)
> > + *   {
> > + *     Name(_HID, "PNP0A08")
> > + *     Name(_CID, "PNP0A03")
> > + *     Name(_SEG, 0)
> > + *     ......
> > + *   }
> > + *   Device (RES0)
> > + *   {
> > + *     Name(_HID, "HISI0081")
> > + *     Name(_CID, "PNP0C02")
> > + *     Name(_UID, 0x0)
> > + *     Name(_CRS, ResourceTemplate (){
> > + *       Memory32Fixed (ReadWrite, 0xa0090000, 0x10000)
> > + *     })
> > + *   }
> > + *   ......
> > + * }
> > + */
> > +static int hisi_pcie_init(struct pci_config_window *cfg)
> > +{
> > +	struct acpi_device *adev = to_acpi_device(cfg->parent);
> > +	struct acpi_pci_root *root = acpi_driver_data(adev);
> > +	void __iomem *reg_base;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	res = devm_kzalloc(&adev->dev, sizeof(*res), GFP_KERNEL);
> > +	if (!res)
> > +		return -ENOMEM;
> > +
> > +	ret = acpi_get_rc_resources("HISI0081", root->segment, res);
> > +	if (ret) {
> > +		dev_err(&adev->dev, "can't get rc base address");
> > +		return ret;
> > +	}
> > +
> > +	reg_base = devm_ioremap(&adev->dev, res->start,
> resource_size(res));
> > +	if (!reg_base)
> > +		return -ENOMEM;
> > +
> > +	cfg->priv = reg_base;
> > +	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 f5740b7..1b24d97 100644
> > --- a/include/linux/pci-ecam.h
> > +++ b/include/linux/pci-ecam.h
> > @@ -67,4 +67,9 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus,
> unsigned int devfn,
> >  int pci_host_common_probe(struct platform_device *pdev,
> >  			  struct pci_ecam_ops *ops);
> >  #endif
> > +
> > +#ifdef CONFIG_PCI_HISI_ACPI
> > +extern struct pci_ecam_ops hisi_pcie_ops;
> > +#endif
> > +
> >  #endif
> > --
> > 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

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

[Bjorn Helgaas]

On Mon, Nov 21, 2016 at 09:09:28AM +0000, Gabriele Paoloni wrote:

> > > +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
> > 
> > I'm not sure about this Kconfig setup.  Do we really want to force
> > people to enable a special config option to get this support?
> > 
> > I'm comparing it in my mind with other PCI quirks.  They're all
> > enabled as a group by CONFIG_PCI_QUIRKS.  Ultimately we want an ACPI
> > kernel to work without requiring any platform-specific config options.
> > 
> > I'm wondering if we should consolidate all the ECAM quirk code in a
> > single place (maybe pci/ecam-quirks.c, pci/ecam.c, or pci/pci-acpi.c),
> > under a config option like CONFIG_PCI_ECAM_QUIRKS or maybe even plain
> > CONFIG_PCI_QUIRKS (of course, it could still be qualified by
> > CONFIG_ACPI and CONFIG_ARM64).
> 
> What about having a single config options but keeping separate files
> for each vendors (at least as first step)?

That sounds fine.  The main thing is that we're trying to build a
generic kernel that can run on any ACPI arm64 platform, so we really
shouldn't have to turn on platform-specific config options.

> Maybe if we see that we can consolidate all the vendors in one file
> we can do it as a second step...

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

[Gabriele Paoloni]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: 21 November 2016 22:32
> To: Gabriele Paoloni
> Cc: liudongdong (C); arnd@arndb.de; rafael@kernel.org;
> Lorenzo.Pieralisi@arm.com; tn@semihalf.com; Wangzhou (B);
> pratyush.anand@gmail.com; linux-pci@vger.kernel.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; jcm@redhat.com;
> Chenxin (Charles); hanjun.guo@linaro.org; Linuxarm
> Subject: Re: [PATCH V5 2/2] PCI/ACPI: hisi: Add ACPI support for
> HiSilicon SoCs Host Controllers
> 
> On Mon, Nov 21, 2016 at 09:09:28AM +0000, Gabriele Paoloni wrote:
> 
> > > > +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
> > >
> > > I'm not sure about this Kconfig setup.  Do we really want to force
> > > people to enable a special config option to get this support?
> > >
> > > I'm comparing it in my mind with other PCI quirks.  They're all
> > > enabled as a group by CONFIG_PCI_QUIRKS.  Ultimately we want an
> ACPI
> > > kernel to work without requiring any platform-specific config
> options.
> > >
> > > I'm wondering if we should consolidate all the ECAM quirk code in a
> > > single place (maybe pci/ecam-quirks.c, pci/ecam.c, or pci/pci-
> acpi.c),
> > > under a config option like CONFIG_PCI_ECAM_QUIRKS or maybe even
> plain
> > > CONFIG_PCI_QUIRKS (of course, it could still be qualified by
> > > CONFIG_ACPI and CONFIG_ARM64).
> >
> > What about having a single config options but keeping separate files
> > for each vendors (at least as first step)?
> 
> That sounds fine.  The main thing is that we're trying to build a
> generic kernel that can run on any ACPI arm64 platform, so we really
> shouldn't have to turn on platform-specific config options.

Ok great we'll do so then

Thanks

Gab

> 
> > Maybe if we see that we can consolidate all the vendors in one file
> > we can do it as a second step...