[PATCH v3 0/2] Add PCIe host driver support for Mediatek SoCs

[PATCH v3 0/2] Add PCIe host driver support for Mediatek SoCs

[Ryder Lee]

Hi,

This patch series add Mediatek Gen2 PCIe host controller driver and
dt-binding document. It can be found on MT7623 series SoCs.

This driver was validated using Broadcom Tigon3 and Intel(R) 82575/82576
gigabit ethernet card.

Changes since v3:
- correct sub-nodes unit addresses.

Changes since v2:
- modify Kconfig to avoid kbuild test error on some architecture.
- change compatible string.
- revise binding document:
  add missing interrupt-names.
  remove the board dts example and drop 'status' properties.
  remove unnecessary descriptions bout standard PCI bus binding.

Changes since v1:
- add .suppress_bind_attrs.
- remove unnecessary *_valid_device() pattern.
- remove PCI_PROBE_ONLY.
- use the regular readl() instead of readl_relaxed().
- add .map_bus() and change to use pci_generic_config_read/pci_generic_config_write.
- revise dt-binding document and move nonstandard properties to root node.
- change compatible string.
- use interrupt-map property and replace mtk_pcie_map_irq() with of_irq_parse_and_map_pci().
- use the new pci_register_host_bridge() method instead of pci_scan_root_bus().

Ryder Lee (2):
  PCI: mediatek: Add Mediatek PCIe host controller support
  dt-bindings: pcie: Add documentation for Mediatek PCIe

 .../bindings/pci/mediatek,mt7623-pcie.txt          | 149 ++++++
 drivers/pci/host/Kconfig                           |  11 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-mediatek.c                   | 563 +++++++++++++++++++++
 4 files changed, 724 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
 create mode 100644 drivers/pci/host/pcie-mediatek.c

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

[Ryder Lee]

Add support for the Mediatek PCIe Gen2 controller which can
be found on MT7623 series SoCs.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/host/Kconfig         |  11 +
 drivers/pci/host/Makefile        |   1 +
 drivers/pci/host/pcie-mediatek.c | 563 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 575 insertions(+)
 create mode 100644 drivers/pci/host/pcie-mediatek.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index f7c1d4d..aef0de9 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -174,6 +174,17 @@ config PCIE_ROCKCHIP
 	  There is 1 internal PCIe port available to support GEN2 with
 	  4 slots.
 
+config PCIE_MEDIATEK
+	bool "Mediatek PCIe controller"
+	depends on ARM && (ARCH_MEDIATEK || COMPILE_TEST)
+	depends on OF
+	depends on PCI
+	select PCIEPORTBUS
+	help
+	  Say Y here if you want to enable PCIe controller support on MT7623 series
+	  SoCs. There is one single root complex with 3 root ports available.
+	  Each port supports Gen2 lane x1.
+
 config VMD
 	depends on PCI_MSI && X86_64 && SRCU
 	tristate "Intel Volume Management Device Driver"
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 4d36866..265adff 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -17,6 +17,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_PCIE_ROCKCHIP) += pcie-rockchip.o
+obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
 obj-$(CONFIG_VMD) += vmd.o
 
 # The following drivers are for devices that use the generic ACPI
diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
new file mode 100644
index 0000000..5e8c1bf
--- /dev/null
+++ b/drivers/pci/host/pcie-mediatek.c
@@ -0,0 +1,563 @@
+/*
+ * Mediatek PCIe host controller driver.
+ *
+ * Copyright (c) 2017 MediaTek Inc.
+ * Author: Ryder Lee <ryder.lee@mediatek.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.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+/* PCIe shared registers */
+#define PCIE_SYS_CFG		0x00
+#define PCIE_INT_ENABLE		0x0c
+#define PCIE_CFG_ADDR		0x20
+#define PCIE_CFG_DATA		0x24
+
+/* PCIe per port registers */
+#define PCIE_BAR0_SETUP		0x10
+#define PCIE_BAR1_SETUP		0x14
+#define PCIE_BAR0_MEM_BASE	0x18
+#define PCIE_CLASS		0x34
+#define PCIE_LINK_STATUS	0x50
+
+#define PCIE_PORT_INT_EN(x)	BIT(20 + (x))
+#define PCIE_PORT_PERST(x)	BIT(1 + (x))
+#define PCIE_PORT_LINKUP	BIT(0)
+#define PCIE_BAR_MAP_MAX	GENMASK(31, 16)
+
+#define PCIE_BAR_ENABLE		BIT(0)
+#define PCIE_REVISION_ID	BIT(0)
+#define PCIE_CLASS_CODE		(0x60400 << 8)
+#define PCIE_CONF_REG(regn)	(((regn) & GENMASK(7, 2)) | \
+				((((regn) >> 8) & GENMASK(3, 0)) << 24))
+#define PCIE_CONF_FUN(fun)	(((fun) << 8) & GENMASK(10, 8))
+#define PCIE_CONF_DEV(dev)	(((dev) << 11) & GENMASK(15, 11))
+#define PCIE_CONF_BUS(bus)	(((bus) << 16) & GENMASK(23, 16))
+#define PCIE_CONF_ADDR(regn, fun, dev, bus) \
+	(PCIE_CONF_REG(regn) | PCIE_CONF_FUN(fun) | \
+	 PCIE_CONF_DEV(dev) | PCIE_CONF_BUS(bus))
+
+/* Mediatek specific configuration registers */
+#define PCIE_FTS_NUM		0x70c
+#define PCIE_FTS_NUM_MASK	GENMASK(15, 8)
+#define PCIE_FTS_NUM_L0(x)	((x) & 0xff << 8)
+
+#define PCIE_FC_CREDIT		0x73c
+#define PCIE_FC_CREDIT_MASK	(GENMASK(31, 31) | GENMASK(28, 16))
+#define PCIE_FC_CREDIT_VAL(x)	((x) << 16)
+
+/**
+ * struct mtk_pcie_port - PCIe port information
+ * @base: IO mapped register base
+ * @list: port list
+ * @pcie: pointer to PCIe host info
+ * @reset: pointer to port reset control
+ * @regs: port memory region
+ * @sys_ck: pointer to bus clock
+ * @phy: pointer to phy control block
+ * @lane: lane count
+ * @index: port index
+ */
+struct mtk_pcie_port {
+	void __iomem *base;
+	struct list_head list;
+	struct mtk_pcie *pcie;
+	struct reset_control *reset;
+	struct resource regs;
+	struct clk *sys_ck;
+	struct phy *phy;
+	u32 lane;
+	u32 index;
+};
+
+/**
+ * struct mtk_pcie - PCIe host information
+ * @dev: pointer to PCIe device
+ * @base: IO mapped register Base
+ * @free_ck: free-run reference clock
+ * @io: IO resource
+ * @pio: PIO resource
+ * @mem: non-prefetchable memory resource
+ * @busn: bus range
+ * @offset: IO / Memory offset
+ * @ports: pointer to PCIe port information
+ */
+struct mtk_pcie {
+	struct device *dev;
+	void __iomem *base;
+	struct clk *free_ck;
+
+	struct resource io;
+	struct resource pio;
+	struct resource mem;
+	struct resource busn;
+	struct {
+		resource_size_t mem;
+		resource_size_t io;
+	} offset;
+	struct list_head ports;
+};
+
+static inline bool mtk_pcie_link_is_up(struct mtk_pcie_port *port)
+{
+	return !!(readl(port->base + PCIE_LINK_STATUS) &
+		  PCIE_PORT_LINKUP);
+}
+
+static void mtk_pcie_port_free(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct device *dev = pcie->dev;
+
+	devm_iounmap(dev, port->base);
+	devm_release_mem_region(dev, port->regs.start,
+				resource_size(&port->regs));
+	list_del(&port->list);
+	devm_kfree(dev, port);
+}
+
+static void mtk_pcie_put_resources(struct mtk_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		phy_power_off(port->phy);
+		clk_disable_unprepare(port->sys_ck);
+		mtk_pcie_port_free(port);
+	}
+
+	clk_disable_unprepare(pcie->free_ck);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+}
+
+static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
+				      unsigned int devfn, int where)
+{
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct mtk_pcie *pcie = pci_host_bridge_priv(host);
+
+	writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn),
+			      bus->number), pcie->base + PCIE_CFG_ADDR);
+
+	return pcie->base + PCIE_CFG_DATA + (where & 3);
+}
+
+static struct pci_ops mtk_pcie_ops = {
+	.map_bus = mtk_pcie_map_bus,
+	.read  = pci_generic_config_read,
+	.write = pci_generic_config_write,
+};
+
+static void mtk_pcie_configure_rc(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	u32 func = PCI_FUNC(port->index << 3);
+	u32 slot = PCI_SLOT(port->index << 3);
+	u32 val;
+
+	/* enable interrupt */
+	val = readl(pcie->base + PCIE_INT_ENABLE);
+	val |= PCIE_PORT_INT_EN(port->index);
+	writel(val, pcie->base + PCIE_INT_ENABLE);
+
+	/* map to all DDR region. We need to set it before cfg operation. */
+	writel(PCIE_BAR_MAP_MAX | PCIE_BAR_ENABLE,
+	       port->base + PCIE_BAR0_SETUP);
+
+	/* configure class Code and revision ID */
+	writel(PCIE_CLASS_CODE | PCIE_REVISION_ID,
+	       port->base + PCIE_CLASS);
+
+	/* configure FC credit */
+	writel(PCIE_CONF_ADDR(PCIE_FC_CREDIT, func, slot, 0),
+	       pcie->base + PCIE_CFG_ADDR);
+	val = readl(pcie->base + PCIE_CFG_DATA);
+	val &= ~PCIE_FC_CREDIT_MASK;
+	val |= PCIE_FC_CREDIT_VAL(0x806c);
+	writel(PCIE_CONF_ADDR(PCIE_FC_CREDIT, func, slot, 0),
+	       pcie->base + PCIE_CFG_ADDR);
+	writel(val, pcie->base + PCIE_CFG_DATA);
+
+	/* configure RC FTS number to 250 when it leaves L0s */
+	writel(PCIE_CONF_ADDR(PCIE_FTS_NUM, func, slot, 0),
+	       pcie->base + PCIE_CFG_ADDR);
+	val = readl(pcie->base + PCIE_CFG_DATA);
+	val &= ~PCIE_FTS_NUM_MASK;
+	val |= PCIE_FTS_NUM_L0(0x50);
+	writel(PCIE_CONF_ADDR(PCIE_FTS_NUM, func, slot, 0),
+	       pcie->base + PCIE_CFG_ADDR);
+	writel(val, pcie->base + PCIE_CFG_DATA);
+}
+
+static void mtk_pcie_assert_ports(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	u32 val;
+
+	/* assert port PERST_N */
+	val = readl(pcie->base + PCIE_SYS_CFG);
+	val |= PCIE_PORT_PERST(port->index);
+	writel(val, pcie->base + PCIE_SYS_CFG);
+
+	/* de-assert port PERST_N */
+	val = readl(pcie->base + PCIE_SYS_CFG);
+	val &= ~PCIE_PORT_PERST(port->index);
+	writel(val, pcie->base + PCIE_SYS_CFG);
+
+	/* PCIe v2.0 need at least 100ms delay to train from Gen1 to Gen2 */
+	msleep(100);
+}
+
+static int mtk_pcie_enable_ports(struct mtk_pcie_port *port)
+{
+	struct device *dev = port->pcie->dev;
+	int err;
+
+	err = clk_prepare_enable(port->sys_ck);
+	if (err) {
+		dev_err(dev, "failed to enable port%d clock\n", port->index);
+		goto err_sys_clk;
+	}
+
+	reset_control_assert(port->reset);
+	reset_control_deassert(port->reset);
+
+	err = phy_power_on(port->phy);
+	if (err) {
+		dev_err(dev, "failed to power on port%d phy\n", port->index);
+		goto err_phy_on;
+	}
+
+	mtk_pcie_assert_ports(port);
+
+	/* if link up, then setup root port configuration space */
+	if (mtk_pcie_link_is_up(port)) {
+		mtk_pcie_configure_rc(port);
+		return 0;
+	}
+
+	dev_info(dev, "Port%d link down\n", port->index);
+
+	phy_power_off(port->phy);
+err_phy_on:
+	clk_disable_unprepare(port->sys_ck);
+	mtk_pcie_port_free(port);
+err_sys_clk:
+	return err;
+}
+
+static int mtk_pcie_parse_ports(struct mtk_pcie *pcie,
+				struct mtk_pcie_port **p,
+				struct device_node *node,
+				int index)
+{
+	struct mtk_pcie_port *port;
+	struct device *dev = pcie->dev;
+	char name[10];
+	int err;
+
+	*p = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!*p)
+		return -ENOMEM;
+
+	port = *p;
+
+	err = of_property_read_u32(node, "num-lanes", &port->lane);
+	if (err) {
+		dev_err(dev, "missing num-lanes property\n");
+		return err;
+	}
+
+	err = of_address_to_resource(node, 0, &port->regs);
+	if (err) {
+		dev_err(dev, "failed to parse address: %d\n", err);
+		return err;
+	}
+
+	port->base = devm_ioremap_resource(dev, &port->regs);
+	if (IS_ERR(port->base)) {
+		dev_err(dev, "failed to map port%d base\n", index);
+		return PTR_ERR(port->base);
+	}
+
+	snprintf(name, sizeof(name), "sys_ck%d", index);
+	port->sys_ck = devm_clk_get(dev, name);
+	if (IS_ERR(port->sys_ck)) {
+		dev_err(dev, "failed to get port%d clock\n", index);
+		return PTR_ERR(port->sys_ck);
+	}
+
+	snprintf(name, sizeof(name), "pcie-rst%d", index);
+	port->reset = devm_reset_control_get(dev, name);
+	if (IS_ERR(port->reset)) {
+		dev_err(dev, "failed to get port%d reset\n", index);
+		return PTR_ERR(port->reset);
+	}
+
+	snprintf(name, sizeof(name), "pcie-phy%d", index);
+	port->phy = devm_phy_get(dev, name);
+	if (IS_ERR(port->phy)) {
+		dev_err(dev, "failed to get port%d phy\n", index);
+		return PTR_ERR(port->phy);
+	}
+
+	port->index = index;
+	port->pcie = pcie;
+
+	INIT_LIST_HEAD(&port->list);
+	list_add_tail(&port->list, &pcie->ports);
+	return 0;
+}
+
+static int mtk_pcie_handle_shared_resource(struct mtk_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct resource *regs;
+	int err;
+
+	/* get shared registers */
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pcie->base = devm_ioremap_resource(dev, regs);
+	if (IS_ERR(pcie->base)) {
+		dev_err(dev, "failed to map shared register\n");
+		return PTR_ERR(pcie->base);
+	}
+
+	pcie->free_ck = devm_clk_get(dev, "free_ck");
+	if (IS_ERR(pcie->free_ck))
+		return PTR_ERR(pcie->free_ck);
+
+	pm_runtime_enable(dev);
+	err = pm_runtime_get_sync(dev);
+	if (err)
+		goto err_pm;
+
+	/* enable top level clock */
+	err = clk_prepare_enable(pcie->free_ck);
+	if (err) {
+		dev_err(dev, "failed to enable free_ck\n");
+		goto err_free_ck;
+	}
+
+	return 0;
+
+err_free_ck:
+	pm_runtime_put_sync(dev);
+err_pm:
+	pm_runtime_disable(dev);
+
+	return err;
+}
+
+static int mtk_pcie_parse_and_add_res(struct mtk_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node, *child;
+	struct of_pci_range_parser parser;
+	struct of_pci_range range;
+	struct resource res;
+	int err, linkup = 0;
+
+	/* parse shared resources */
+	err = mtk_pcie_handle_shared_resource(pcie);
+	if (err)
+		return err;
+
+	if (of_pci_range_parser_init(&parser, node)) {
+		dev_err(dev, "missing \"ranges\" property\n");
+		return -EINVAL;
+	}
+
+	for_each_of_pci_range(&parser, &range) {
+		err = of_pci_range_to_resource(&range, node, &res);
+		if (err < 0)
+			return err;
+
+		switch (res.flags & IORESOURCE_TYPE_BITS) {
+		case IORESOURCE_IO:
+			pcie->offset.io = res.start - range.pci_addr;
+
+			memcpy(&pcie->pio, &res, sizeof(res));
+			pcie->pio.name = node->full_name;
+
+			pcie->io.start = range.cpu_addr;
+			pcie->io.end = range.cpu_addr + range.size - 1;
+			pcie->io.flags = IORESOURCE_MEM;
+			pcie->io.name = "I/O";
+
+			memcpy(&res, &pcie->io, sizeof(res));
+			break;
+
+		case IORESOURCE_MEM:
+			pcie->offset.mem = res.start - range.pci_addr;
+
+			memcpy(&pcie->mem, &res, sizeof(res));
+			pcie->mem.name = "non-prefetchable";
+			break;
+		}
+	}
+
+	err = of_pci_parse_bus_range(node, &pcie->busn);
+	if (err < 0) {
+		dev_err(dev, "failed to parse ranges property: %d\n", err);
+		pcie->busn.name = node->name;
+		pcie->busn.start = 0;
+		pcie->busn.end = 0xff;
+		pcie->busn.flags = IORESOURCE_BUS;
+	}
+
+	for_each_child_of_node(node, child) {
+		struct mtk_pcie_port *port;
+		int index;
+
+		err = of_pci_get_devfn(child);
+		if (err < 0) {
+			dev_err(dev, "failed to parse devfn: %d\n", err);
+			return err;
+		}
+
+		index = PCI_SLOT(err);
+
+		if (!of_device_is_available(child))
+			continue;
+
+		err = mtk_pcie_parse_ports(pcie, &port, child, index);
+		if (err)
+			return err;
+
+		/* enable each port, and then check link status */
+		err = mtk_pcie_enable_ports(port);
+		if (!err)
+			linkup++;
+	}
+
+	return !linkup;
+}
+
+static int mtk_pcie_request_resources(struct mtk_pcie *pcie)
+{
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct list_head *windows = &host->windows;
+	struct device *dev = pcie->dev;
+	int err;
+
+	pci_add_resource_offset(windows, &pcie->pio, pcie->offset.io);
+	pci_add_resource_offset(windows, &pcie->mem, pcie->offset.mem);
+	pci_add_resource(windows, &pcie->busn);
+
+	err = devm_request_pci_bus_resources(dev, windows);
+	if (err < 0)
+		return err;
+
+	pci_remap_iospace(&pcie->pio, pcie->io.start);
+
+	return 0;
+}
+
+static int mtk_pcie_register_host(struct pci_host_bridge *host)
+{
+	struct mtk_pcie *pcie = pci_host_bridge_priv(host);
+	struct pci_bus *child;
+	int err;
+
+	pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
+	host->busnr = pcie->busn.start;
+	host->dev.parent = pcie->dev;
+	host->ops = &mtk_pcie_ops;
+
+	err = pci_register_host_bridge(host);
+	if (err < 0)
+		return err;
+
+	pci_scan_child_bus(host->bus);
+
+	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+	pci_bus_size_bridges(host->bus);
+	pci_bus_assign_resources(host->bus);
+
+	list_for_each_entry(child, &host->bus->children, node)
+		pcie_bus_configure_settings(child);
+
+	pci_bus_add_devices(host->bus);
+
+	return 0;
+}
+
+static int mtk_pcie_probe(struct platform_device *pdev)
+{
+	struct mtk_pcie *pcie;
+	struct pci_host_bridge *host;
+	int err;
+
+	host = pci_alloc_host_bridge(sizeof(*pcie));
+	if (!host)
+		return -ENOMEM;
+
+	pcie = pci_host_bridge_priv(host);
+
+	pcie->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pcie);
+	INIT_LIST_HEAD(&pcie->ports);
+
+	err = mtk_pcie_parse_and_add_res(pcie);
+	if (err)
+		return err;
+
+	err = mtk_pcie_request_resources(pcie);
+	if (err)
+		goto put_resources;
+
+	err = mtk_pcie_register_host(host);
+	if (err)
+		goto put_resources;
+
+	return 0;
+
+put_resources:
+	mtk_pcie_put_resources(pcie);
+	return err;
+}
+
+static const struct of_device_id mtk_pcie_ids[] = {
+	{ .compatible = "mediatek,mt7623-pcie"},
+	{ .compatible = "mediatek,mt2701-pcie"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_pcie_ids);
+
+static struct platform_driver mtk_pcie_driver = {
+	.probe = mtk_pcie_probe,
+	.driver = {
+		.name = "mtk-pcie",
+		.of_match_table = mtk_pcie_ids,
+		.suppress_bind_attrs = true,
+	},
+};
+
+builtin_platform_driver(mtk_pcie_driver);
+
+MODULE_DESCRIPTION("Mediatek PCIe host controller driver.");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v3 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

[Paul Gortmaker]

On Tue, May 9, 2017 at 10:06 PM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> Add support for the Mediatek PCIe Gen2 controller which can
> be found on MT7623 series SoCs.
>
> Signed-off-by: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/pci/host/Kconfig         |  11 +
>  drivers/pci/host/Makefile        |   1 +
>  drivers/pci/host/pcie-mediatek.c | 563 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 575 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-mediatek.c
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index f7c1d4d..aef0de9 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -174,6 +174,17 @@ config PCIE_ROCKCHIP
>           There is 1 internal PCIe port available to support GEN2 with
>           4 slots.
>
> +config PCIE_MEDIATEK
> +       bool "Mediatek PCIe controller"

You've got bool here and correctly use the builtin register
function, but you still have a couple stray references to
the module.h header and MODULE macros.  Can you
please also clean them up and resend with those gone?

Thanks,
Paul.
--

> +       depends on ARM && (ARCH_MEDIATEK || COMPILE_TEST)
> +       depends on OF
> +       depends on PCI
> +       select PCIEPORTBUS
> +       help
> +         Say Y here if you want to enable PCIe controller support on MT7623 series
> +         SoCs. There is one single root complex with 3 root ports available.
> +         Each port supports Gen2 lane x1.
> +
>  config VMD
>         depends on PCI_MSI && X86_64 && SRCU
>         tristate "Intel Volume Management Device Driver"
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 4d36866..265adff 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -17,6 +17,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_PCIE_ROCKCHIP) += pcie-rockchip.o
> +obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
>  obj-$(CONFIG_VMD) += vmd.o
>
>  # The following drivers are for devices that use the generic ACPI
> diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> new file mode 100644
> index 0000000..5e8c1bf
> --- /dev/null
> +++ b/drivers/pci/host/pcie-mediatek.c
> @@ -0,0 +1,563 @@
> +/*
> + * Mediatek PCIe host controller driver.
> + *
> + * Copyright (c) 2017 MediaTek Inc.
> + * Author: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

[....]

> +};
> +MODULE_DEVICE_TABLE(of, mtk_pcie_ids);
> +
> +static struct platform_driver mtk_pcie_driver = {
> +       .probe = mtk_pcie_probe,
> +       .driver = {
> +               .name = "mtk-pcie",
> +               .of_match_table = mtk_pcie_ids,
> +               .suppress_bind_attrs = true,
> +       },
> +};
> +
> +builtin_platform_driver(mtk_pcie_driver);
> +
> +MODULE_DESCRIPTION("Mediatek PCIe host controller driver.");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

[Ryder Lee]

On Sat, 2017-05-20 at 15:46 -0400, Paul Gortmaker wrote:
> On Tue, May 9, 2017 at 10:06 PM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > Add support for the Mediatek PCIe Gen2 controller which can
> > be found on MT7623 series SoCs.
> >
> > Signed-off-by: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >  drivers/pci/host/Kconfig         |  11 +
> >  drivers/pci/host/Makefile        |   1 +
> >  drivers/pci/host/pcie-mediatek.c | 563 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 575 insertions(+)
> >  create mode 100644 drivers/pci/host/pcie-mediatek.c
> >
> > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > index f7c1d4d..aef0de9 100644
> > --- a/drivers/pci/host/Kconfig
> > +++ b/drivers/pci/host/Kconfig
> > @@ -174,6 +174,17 @@ config PCIE_ROCKCHIP
> >           There is 1 internal PCIe port available to support GEN2 with
> >           4 slots.
> >
> > +config PCIE_MEDIATEK
> > +       bool "Mediatek PCIe controller"
> 
> You've got bool here and correctly use the builtin register
> function, but you still have a couple stray references to
> the module.h header and MODULE macros.  Can you
> please also clean them up and resend with those gone?
> 
> Thanks,
> Paul.
> --

> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> 
> [....]
> 
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_pcie_ids);
> > +
> > +static struct platform_driver mtk_pcie_driver = {
> > +       .probe = mtk_pcie_probe,
> > +       .driver = {
> > +               .name = "mtk-pcie",
> > +               .of_match_table = mtk_pcie_ids,
> > +               .suppress_bind_attrs = true,
> > +       },
> > +};
> > +
> > +builtin_platform_driver(mtk_pcie_driver);
> > +
> > +MODULE_DESCRIPTION("Mediatek PCIe host controller driver.");
> > +MODULE_LICENSE("GPL v2");


I've already removed them at patch v5. Thanks a lot, Paul!


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

Add documentation for PCIe host driver available in MT7623
series SoCs.

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/pci/mediatek,mt7623-pcie.txt          | 149 +++++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
new file mode 100644
index 0000000..a9393ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
@@ -0,0 +1,149 @@
+Mediatek Gen2 PCIe controller which is available on MT7623 series SoCs
+
+PCIe subsys supports single root complex (RC) with 3 Root Ports. Each root
+ports supports a Gen2 1-lane Link and has PIPE interface to PHY.
+
+Required properties:
+- compatible: Should contain "mediatek,mt7623-pcie".
+- device_type: Must be "pci"
+- reg: Base addresses and lengths of the PCIe controller.
+- #address-cells: Address representation for root ports (must be 3)
+- #size-cells: Size representation for root ports (must be 2)
+- #interrupt-cells: Size representation for interrupts (must be 1)
+- interrupts: Three interrupt outputs of the controller. Must contain an
+  entry for each entry in the interrupt-names property.
+- interrupt-names: Must include the following names
+  - "pcie-int0"
+  - "pcie-int1"
+  - "pcie-int2"
+- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
+  Please refer to the standard PCI bus binding document for a more detailed
+  explanation.
+- clocks: Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names: Must include the following entries:
+  - free_ck :for reference clock of PCIe subsys
+  - sys_ck0 :for clock of Port0
+  - sys_ck1 :for clock of Port1
+  - sys_ck2 :for clock of Port2
+- resets: Must contain an entry for each entry in reset-names.
+  See ../reset/reset.txt for details.
+- reset-names: Must include the following entries:
+  - pcie-rst0 :port0 reset
+  - pcie-rst1 :port1 reset
+  - pcie-rst2 :port2 reset
+- phys: list of PHY specifiers (used by generic PHY framework).
+- phy-names : must be "pcie-phy0", "pcie-phy1", "pcie-phyN".. based on the
+  number of PHYs as specified in *phys* property.
+- power-domains: A phandle and power domain specifier pair to the power domain
+  which is responsible for collapsing and restoring power to the peripheral.
+- bus-range: Range of bus numbers associated with this controller.
+- ranges:
+  - The first three entries are expected to translate the addresses for the root
+    port registers, which are referenced by the assigned-addresses property of
+    the root port nodes (see below).
+  - The remaining entries setup the mapping for the standard I/O and memory
+	regions.
+  Please refer to the standard PCI bus binding document for a more detailed
+  explanation.
+
+In addition, the device tree node must have sub-nodes describing each
+PCIe port interface, having the following mandatory properties:
+
+Required properties:
+- device_type: Must be "pci"
+- assigned-addresses: Address and size of the port configuration registers
+- reg: Only the first four bytes are used to refer to the correct bus number
+  and device number.
+- #address-cells: Must be 3
+- #size-cells: Must be 2
+- #interrupt-cells: Must be 1
+- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
+  Please refer to the standard PCI bus binding document for a more detailed
+  explanation.
+- ranges: Sub-ranges distributed from the PCIe controller node. An empty
+  property is sufficient.
+- num-lanes: Number of lanes to use for this port.
+
+Examples:
+
+	hifsys: syscon@1a000000 {
+		compatible = "mediatek,mt7623-hifsys", "syscon";
+		reg = <0 0x1a000000 0 0x1000>;
+		#clock-cells = <1>;
+		#reset-cells = <1>;
+	};
+
+	pcie: pcie-controller@1a140000 {
+		compatible = "mediatek,mt7623-pcie";
+		device_type = "pci";
+		reg = <0 0x1a140000 0 0x1000>; /* PCIe shared registers */
+		#address-cells = <3>;
+		#size-cells = <2>;
+		#interrupt-cells = <1>;
+		interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_SPI 195 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-names = "pcie-int0", "pcie-int1", "pcie-int2";
+		interrupt-map-mask = <0xf800 0 0 0>;
+		interrupt-map = <0x0000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
+				<0x0800 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
+				<0x1000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
+		clocks = <&topckgen CLK_TOP_ETHIF_SEL>,
+			 <&hifsys CLK_HIFSYS_PCIE0>,
+			 <&hifsys CLK_HIFSYS_PCIE1>,
+			 <&hifsys CLK_HIFSYS_PCIE2>;
+		clock-names = "free_ck", "sys_ck0", "sys_ck1", "sys_ck2";
+		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
+			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
+			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
+		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
+		phys = <&pcie0_phy>, <&pcie1_phy>, <&pcie2_phy>;
+		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
+		power-domains = <&scpsys MT2701_POWER_DOMAIN_HIF>;
+		bus-range = <0x00 0xff>;
+		ranges = <0x82000000 0 0x1a142000 0 0x1a142000 0 0x1000 /* Port0 registers */
+			  0x82000000 0 0x1a143000 0 0x1a143000 0 0x1000 /* Port1 registers */
+			  0x82000000 0 0x1a144000 0 0x1a144000 0 0x1000 /* Port2 registers */
+			  0x81000000 0 0x1a160000 0 0x1a160000 0 0x00010000 /* I/O space */
+			  0x83000000 0 0x60000000 0 0x60000000 0 0x10000000>;	/* memory space */
+
+		pcie@0,0 {
+			device_type = "pci";
+			assigned-addresses = <0x82000000 0 0x1a142000 0 0x1000>;
+			reg = <0x0000 0 0 0 0>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>;
+			ranges;
+			num-lanes = <1>;
+		};
+
+		pcie@1,0 {
+			device_type = "pci";
+			assigned-addresses = <0x82000800 0 0x1a143000 0 0x1000>;
+			reg = <0x0800 0 0 0 0>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>;
+			ranges;
+			num-lanes = <1>;
+		};
+
+		pcie@2,0 {
+			device_type = "pci";
+			assigned-addresses = <0x82001000 0 0x1a144000 0 0x1000>;
+			reg = <0x1000 0 0 0 0>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
+			ranges;
+			num-lanes = <1>;
+		};
+	};
-- 
1.9.1

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Matthias Brugger]

On 10/05/17 04:07, Ryder Lee wrote:
> Add documentation for PCIe host driver available in MT7623
> series SoCs.
> 
> Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>   .../bindings/pci/mediatek,mt7623-pcie.txt          | 149 +++++++++++++++++++++
>   1 file changed, 149 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> new file mode 100644
> index 0000000..a9393ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> @@ -0,0 +1,149 @@
> +Mediatek Gen2 PCIe controller which is available on MT7623 series SoCs
> +
> +PCIe subsys supports single root complex (RC) with 3 Root Ports. Each root
> +ports supports a Gen2 1-lane Link and has PIPE interface to PHY.
> +
> +Required properties:
> +- compatible: Should contain "mediatek,mt7623-pcie".
> +- device_type: Must be "pci"
> +- reg: Base addresses and lengths of the PCIe controller.
> +- #address-cells: Address representation for root ports (must be 3)
> +- #size-cells: Size representation for root ports (must be 2)
> +- #interrupt-cells: Size representation for interrupts (must be 1)
> +- interrupts: Three interrupt outputs of the controller. Must contain an
> +  entry for each entry in the interrupt-names property.
> +- interrupt-names: Must include the following names
> +  - "pcie-int0"
> +  - "pcie-int1"
> +  - "pcie-int2"
> +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> +  Please refer to the standard PCI bus binding document for a more detailed
> +  explanation.
> +- clocks: Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names: Must include the following entries:
> +  - free_ck :for reference clock of PCIe subsys
> +  - sys_ck0 :for clock of Port0
> +  - sys_ck1 :for clock of Port1
> +  - sys_ck2 :for clock of Port2
> +- resets: Must contain an entry for each entry in reset-names.
> +  See ../reset/reset.txt for details.
> +- reset-names: Must include the following entries:
> +  - pcie-rst0 :port0 reset
> +  - pcie-rst1 :port1 reset
> +  - pcie-rst2 :port2 reset
> +- phys: list of PHY specifiers (used by generic PHY framework).
> +- phy-names : must be "pcie-phy0", "pcie-phy1", "pcie-phyN".. based on the
> +  number of PHYs as specified in *phys* property.
> +- power-domains: A phandle and power domain specifier pair to the power domain
> +  which is responsible for collapsing and restoring power to the peripheral.
> +- bus-range: Range of bus numbers associated with this controller.
> +- ranges:
> +  - The first three entries are expected to translate the addresses for the root
> +    port registers, which are referenced by the assigned-addresses property of
> +    the root port nodes (see below).
> +  - The remaining entries setup the mapping for the standard I/O and memory
> +	regions.
> +  Please refer to the standard PCI bus binding document for a more detailed
> +  explanation.
> +
> +In addition, the device tree node must have sub-nodes describing each
> +PCIe port interface, having the following mandatory properties:
> +
> +Required properties:
> +- device_type: Must be "pci"
> +- assigned-addresses: Address and size of the port configuration registers
> +- reg: Only the first four bytes are used to refer to the correct bus number
> +  and device number.
> +- #address-cells: Must be 3
> +- #size-cells: Must be 2
> +- #interrupt-cells: Must be 1
> +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> +  Please refer to the standard PCI bus binding document for a more detailed
> +  explanation.
> +- ranges: Sub-ranges distributed from the PCIe controller node. An empty
> +  property is sufficient.
> +- num-lanes: Number of lanes to use for this port.
> +
> +Examples:
> +
> +	hifsys: syscon@1a000000 {
> +		compatible = "mediatek,mt7623-hifsys", "syscon";

If you want to put the clock controller subsystem node to the example, 
please use a valid compatible, for example:

		compatible = "mediatek,mt7623-hifsys",
			     "mediatek,mt2701-hifsys",
			     "syscon";

Thanks,
Matthias

> +		reg = <0 0x1a000000 0 0x1000>;
> +		#clock-cells = <1>;
> +		#reset-cells = <1>;
> +	};
> +
> +	pcie: pcie-controller@1a140000 {
> +		compatible = "mediatek,mt7623-pcie";
> +		device_type = "pci";
> +		reg = <0 0x1a140000 0 0x1000>; /* PCIe shared registers */
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		#interrupt-cells = <1>;
> +		interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 195 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-names = "pcie-int0", "pcie-int1", "pcie-int2";
> +		interrupt-map-mask = <0xf800 0 0 0>;
> +		interrupt-map = <0x0000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
> +				<0x0800 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
> +				<0x1000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
> +		clocks = <&topckgen CLK_TOP_ETHIF_SEL>,
> +			 <&hifsys CLK_HIFSYS_PCIE0>,
> +			 <&hifsys CLK_HIFSYS_PCIE1>,
> +			 <&hifsys CLK_HIFSYS_PCIE2>;
> +		clock-names = "free_ck", "sys_ck0", "sys_ck1", "sys_ck2";
> +		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
> +			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
> +			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
> +		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
> +		phys = <&pcie0_phy>, <&pcie1_phy>, <&pcie2_phy>;
> +		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
> +		power-domains = <&scpsys MT2701_POWER_DOMAIN_HIF>;
> +		bus-range = <0x00 0xff>;
> +		ranges = <0x82000000 0 0x1a142000 0 0x1a142000 0 0x1000 /* Port0 registers */
> +			  0x82000000 0 0x1a143000 0 0x1a143000 0 0x1000 /* Port1 registers */
> +			  0x82000000 0 0x1a144000 0 0x1a144000 0 0x1000 /* Port2 registers */
> +			  0x81000000 0 0x1a160000 0 0x1a160000 0 0x00010000 /* I/O space */
> +			  0x83000000 0 0x60000000 0 0x60000000 0 0x10000000>;	/* memory space */
> +
> +		pcie@0,0 {
> +			device_type = "pci";
> +			assigned-addresses = <0x82000000 0 0x1a142000 0 0x1000>;
> +			reg = <0x0000 0 0 0 0>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0>;
> +			interrupt-map = <0 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>;
> +			ranges;
> +			num-lanes = <1>;
> +		};
> +
> +		pcie@1,0 {
> +			device_type = "pci";
> +			assigned-addresses = <0x82000800 0 0x1a143000 0 0x1000>;
> +			reg = <0x0800 0 0 0 0>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0>;
> +			interrupt-map = <0 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>;
> +			ranges;
> +			num-lanes = <1>;
> +		};
> +
> +		pcie@2,0 {
> +			device_type = "pci";
> +			assigned-addresses = <0x82001000 0 0x1a144000 0 0x1000>;
> +			reg = <0x1000 0 0 0 0>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0>;
> +			interrupt-map = <0 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
> +			ranges;
> +			num-lanes = <1>;
> +		};
> +	};
> 

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

On Wed, 2017-05-10 at 09:58 +0200, Matthias Brugger wrote:
> 
> On 10/05/17 04:07, Ryder Lee wrote:
> > Add documentation for PCIe host driver available in MT7623
> > series SoCs.
> > 
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > Acked-by: Rob Herring <robh@kernel.org>
> > ---
> >   .../bindings/pci/mediatek,mt7623-pcie.txt          | 149 +++++++++++++++++++++
> >   1 file changed, 149 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> > new file mode 100644
> > index 0000000..a9393ce
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> > @@ -0,0 +1,149 @@
> > +Mediatek Gen2 PCIe controller which is available on MT7623 series SoCs
> > +
> > +PCIe subsys supports single root complex (RC) with 3 Root Ports. Each root
> > +ports supports a Gen2 1-lane Link and has PIPE interface to PHY.
> > +
> > +Required properties:
> > +- compatible: Should contain "mediatek,mt7623-pcie".
> > +- device_type: Must be "pci"
> > +- reg: Base addresses and lengths of the PCIe controller.
> > +- #address-cells: Address representation for root ports (must be 3)
> > +- #size-cells: Size representation for root ports (must be 2)
> > +- #interrupt-cells: Size representation for interrupts (must be 1)
> > +- interrupts: Three interrupt outputs of the controller. Must contain an
> > +  entry for each entry in the interrupt-names property.
> > +- interrupt-names: Must include the following names
> > +  - "pcie-int0"
> > +  - "pcie-int1"
> > +  - "pcie-int2"
> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> > +  Please refer to the standard PCI bus binding document for a more detailed
> > +  explanation.
> > +- clocks: Must contain an entry for each entry in clock-names.
> > +  See ../clocks/clock-bindings.txt for details.
> > +- clock-names: Must include the following entries:
> > +  - free_ck :for reference clock of PCIe subsys
> > +  - sys_ck0 :for clock of Port0
> > +  - sys_ck1 :for clock of Port1
> > +  - sys_ck2 :for clock of Port2
> > +- resets: Must contain an entry for each entry in reset-names.
> > +  See ../reset/reset.txt for details.
> > +- reset-names: Must include the following entries:
> > +  - pcie-rst0 :port0 reset
> > +  - pcie-rst1 :port1 reset
> > +  - pcie-rst2 :port2 reset
> > +- phys: list of PHY specifiers (used by generic PHY framework).
> > +- phy-names : must be "pcie-phy0", "pcie-phy1", "pcie-phyN".. based on the
> > +  number of PHYs as specified in *phys* property.
> > +- power-domains: A phandle and power domain specifier pair to the power domain
> > +  which is responsible for collapsing and restoring power to the peripheral.
> > +- bus-range: Range of bus numbers associated with this controller.
> > +- ranges:
> > +  - The first three entries are expected to translate the addresses for the root
> > +    port registers, which are referenced by the assigned-addresses property of
> > +    the root port nodes (see below).
> > +  - The remaining entries setup the mapping for the standard I/O and memory
> > +	regions.
> > +  Please refer to the standard PCI bus binding document for a more detailed
> > +  explanation.
> > +
> > +In addition, the device tree node must have sub-nodes describing each
> > +PCIe port interface, having the following mandatory properties:
> > +
> > +Required properties:
> > +- device_type: Must be "pci"
> > +- assigned-addresses: Address and size of the port configuration registers
> > +- reg: Only the first four bytes are used to refer to the correct bus number
> > +  and device number.
> > +- #address-cells: Must be 3
> > +- #size-cells: Must be 2
> > +- #interrupt-cells: Must be 1
> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> > +  Please refer to the standard PCI bus binding document for a more detailed
> > +  explanation.
> > +- ranges: Sub-ranges distributed from the PCIe controller node. An empty
> > +  property is sufficient.
> > +- num-lanes: Number of lanes to use for this port.
> > +
> > +Examples:
> > +
> > +	hifsys: syscon@1a000000 {
> > +		compatible = "mediatek,mt7623-hifsys", "syscon";
> 
> If you want to put the clock controller subsystem node to the example, 
> please use a valid compatible, for example:
> 
> 		compatible = "mediatek,mt7623-hifsys",
> 			     "mediatek,mt2701-hifsys",
> 			     "syscon";
> 
> Thanks,
> Matthias

OK i will correct it.
> 
> > +		reg = <0 0x1a000000 0 0x1000>;
> > +		#clock-cells = <1>;
> > +		#reset-cells = <1>;
> > +	};
> > +
> > +	pcie: pcie-controller@1a140000 {
> > +		compatible = "mediatek,mt7623-pcie";
> > +		device_type = "pci";
> > +		reg = <0 0x1a140000 0 0x1000>; /* PCIe shared registers */
> > +		#address-cells = <3>;
> > +		#size-cells = <2>;
> > +		#interrupt-cells = <1>;
> > +		interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_LOW>,
> > +			     <GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>,
> > +			     <GIC_SPI 195 IRQ_TYPE_LEVEL_LOW>;
> > +		interrupt-names = "pcie-int0", "pcie-int1", "pcie-int2";
> > +		interrupt-map-mask = <0xf800 0 0 0>;
> > +		interrupt-map = <0x0000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
> > +				<0x0800 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
> > +				<0x1000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
> > +		clocks = <&topckgen CLK_TOP_ETHIF_SEL>,
> > +			 <&hifsys CLK_HIFSYS_PCIE0>,
> > +			 <&hifsys CLK_HIFSYS_PCIE1>,
> > +			 <&hifsys CLK_HIFSYS_PCIE2>;
> > +		clock-names = "free_ck", "sys_ck0", "sys_ck1", "sys_ck2";
> > +		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
> > +			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
> > +			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
> > +		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
> > +		phys = <&pcie0_phy>, <&pcie1_phy>, <&pcie2_phy>;
> > +		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
> > +		power-domains = <&scpsys MT2701_POWER_DOMAIN_HIF>;
> > +		bus-range = <0x00 0xff>;
> > +		ranges = <0x82000000 0 0x1a142000 0 0x1a142000 0 0x1000 /* Port0 registers */
> > +			  0x82000000 0 0x1a143000 0 0x1a143000 0 0x1000 /* Port1 registers */
> > +			  0x82000000 0 0x1a144000 0 0x1a144000 0 0x1000 /* Port2 registers */
> > +			  0x81000000 0 0x1a160000 0 0x1a160000 0 0x00010000 /* I/O space */
> > +			  0x83000000 0 0x60000000 0 0x60000000 0 0x10000000>;	/* memory space */
> > +
> > +		pcie@0,0 {
> > +			device_type = "pci";
> > +			assigned-addresses = <0x82000000 0 0x1a142000 0 0x1000>;
> > +			reg = <0x0000 0 0 0 0>;
> > +			#address-cells = <3>;
> > +			#size-cells = <2>;
> > +			#interrupt-cells = <1>;
> > +			interrupt-map-mask = <0 0 0 0>;
> > +			interrupt-map = <0 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>;
> > +			ranges;
> > +			num-lanes = <1>;
> > +		};
> > +
> > +		pcie@1,0 {
> > +			device_type = "pci";
> > +			assigned-addresses = <0x82000800 0 0x1a143000 0 0x1000>;
> > +			reg = <0x0800 0 0 0 0>;
> > +			#address-cells = <3>;
> > +			#size-cells = <2>;
> > +			#interrupt-cells = <1>;
> > +			interrupt-map-mask = <0 0 0 0>;
> > +			interrupt-map = <0 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>;
> > +			ranges;
> > +			num-lanes = <1>;
> > +		};
> > +
> > +		pcie@2,0 {
> > +			device_type = "pci";
> > +			assigned-addresses = <0x82001000 0 0x1a144000 0 0x1000>;
> > +			reg = <0x1000 0 0 0 0>;
> > +			#address-cells = <3>;
> > +			#size-cells = <2>;
> > +			#interrupt-cells = <1>;
> > +			interrupt-map-mask = <0 0 0 0>;
> > +			interrupt-map = <0 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
> > +			ranges;
> > +			num-lanes = <1>;
> > +		};
> > +	};
> > 

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Arnd Bergmann]

On Wed, May 10, 2017 at 4:07 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:

> +- ranges:
> +  - The first three entries are expected to translate the addresses for the root
> +    port registers, which are referenced by the assigned-addresses property of
> +    the root port nodes (see below).

I don't understand this part. Why do you need a static translation for these?
Shouldn't they just be listed in the 'reg' property of the parent node now that
you have the clk/reset/phy properties in the parent as well?

> +Required properties:
> +- device_type: Must be "pci"
> +- assigned-addresses: Address and size of the port configuration registers
> +- reg: Only the first four bytes are used to refer to the correct bus number
> +  and device number.
> +- #address-cells: Must be 3
> +- #size-cells: Must be 2
> +- #interrupt-cells: Must be 1
> +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> +  Please refer to the standard PCI bus binding document for a more detailed
> +  explanation.

Child nodes do not normally have interrupt-map properties. Isn't this
already covered by the interrupt-map in the parent?

      Arnd

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

On Wed, 2017-05-10 at 10:08 +0200, Arnd Bergmann wrote:
> On Wed, May 10, 2017 at 4:07 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> 
> > +- ranges:
> > +  - The first three entries are expected to translate the addresses for the root
> > +    port registers, which are referenced by the assigned-addresses property of
> > +    the root port nodes (see below).
> 
> I don't understand this part. Why do you need a static translation for these?
> Shouldn't they just be listed in the 'reg' property of the parent node now that
> you have the clk/reset/phy properties in the parent as well?

At first, I did like that. But I noticed that someone suggest it's
better to use 'assigned-addresses' to handle per-port registers, the
same path as tegra and marvell did, in other platform discussion thread.
So I just put shared register in root node. It could be rolled back if
you feel this is inappropriate.

> > +Required properties:
> > +- device_type: Must be "pci"
> > +- assigned-addresses: Address and size of the port configuration registers
> > +- reg: Only the first four bytes are used to refer to the correct bus number
> > +  and device number.
> > +- #address-cells: Must be 3
> > +- #size-cells: Must be 2
> > +- #interrupt-cells: Must be 1
> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> > +  Please refer to the standard PCI bus binding document for a more detailed
> > +  explanation.
> 
> Child nodes do not normally have interrupt-map properties. Isn't this
> already covered by the interrupt-map in the parent?
> 

I have one Intel 4 port ethernet card(0000:00:01) and MTK WLAN card
(0000:00:02), probe message looks good to me.

pci 0000:00:01.0: fixup irq: got 224
pci 0000:00:01.0: assigning IRQ 224
pci 0000:00:02.0: fixup irq: got 225
pci 0000:00:02.0: assigning IRQ 225

pci 0000:01:00.0: fixup irq: got 224
pci 0000:01:00.0: assigning IRQ 224
pci 0000:01:00.1: fixup irq: got 224
pci 0000:01:00.1: assigning IRQ 224
pci 0000:01:00.2: fixup irq: got 224
pci 0000:01:00.2: assigning IRQ 224
pci 0000:01:00.3: fixup irq: got 224
pci 0000:01:00.3: assigning IRQ 224

pci 0000:02:00.0: fixup irq: got 225
pci 0000:02:00.0: assigning IRQ 225


But child nodes without interrupt-map properties:
It seems incorrect.

pci 0000:00:01.0: fixup irq: got 224
pci 0000:00:01.0: assigning IRQ 224
pci 0000:00:02.0: fixup irq: got 225
pci 0000:00:02.0: assigning IRQ 225

pci 0000:01:00.0: fixup irq: got 223
pci 0000:01:00.0: assigning IRQ 223
pci 0000:01:00.1: fixup irq: got 223
pci 0000:01:00.1: assigning IRQ 223
pci 0000:01:00.2: fixup irq: got 223
pci 0000:01:00.2: assigning IRQ 223
pci 0000:01:00.3: fixup irq: got 223
pci 0000:01:00.3: assigning IRQ 223

pci 0000:02:00.0: fixup irq: got 223
pci 0000:02:00.0: assigning IRQ 223

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Arnd Bergmann]

On Wed, May 10, 2017 at 11:31 AM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> On Wed, 2017-05-10 at 10:08 +0200, Arnd Bergmann wrote:
>> On Wed, May 10, 2017 at 4:07 AM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>>
>> > +- ranges:
>> > +  - The first three entries are expected to translate the addresses for the root
>> > +    port registers, which are referenced by the assigned-addresses property of
>> > +    the root port nodes (see below).
>>
>> I don't understand this part. Why do you need a static translation for these?
>> Shouldn't they just be listed in the 'reg' property of the parent node now that
>> you have the clk/reset/phy properties in the parent as well?
>
> At first, I did like that. But I noticed that someone suggest it's
> better to use 'assigned-addresses' to handle per-port registers, the
> same path as tegra and marvell did, in other platform discussion thread.
> So I just put shared register in root node. It could be rolled back if
> you feel this is inappropriate.

The marvell case is not a good example for your case: their top-level
device is made up by the OS to help with the shared resource allocation,
while in your case the bus bridge actually exists in hardware.

I'm not too familiar with the Tegra case, and haven't looked at that here,
but it could be an artifact of how for a while we used to list the config
space access in the top-level "ranges" instead of the "reg" property.

I'd vote for moving it back, for consistency with the other port specific
properties that are now in the root node. Once you do that, the port
nodes can be removed completely, which is what I was aiming for with
the comments on the previous version.

>> > +Required properties:
>> > +- device_type: Must be "pci"
>> > +- assigned-addresses: Address and size of the port configuration registers
>> > +- reg: Only the first four bytes are used to refer to the correct bus number
>> > +  and device number.
>> > +- #address-cells: Must be 3
>> > +- #size-cells: Must be 2
>> > +- #interrupt-cells: Must be 1
>> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
>> > +  Please refer to the standard PCI bus binding document for a more detailed
>> > +  explanation.
>>
>> Child nodes do not normally have interrupt-map properties. Isn't this
>> already covered by the interrupt-map in the parent?
>>
>
> I have one Intel 4 port ethernet card(0000:00:01) and MTK WLAN card
> (0000:00:02), probe message looks good to me.
>
> pci 0000:00:01.0: fixup irq: got 224
> pci 0000:00:01.0: assigning IRQ 224
> pci 0000:00:02.0: fixup irq: got 225
> pci 0000:00:02.0: assigning IRQ 225
>
> pci 0000:01:00.0: fixup irq: got 224
> pci 0000:01:00.0: assigning IRQ 224
> pci 0000:01:00.1: fixup irq: got 224
> pci 0000:01:00.1: assigning IRQ 224
> pci 0000:01:00.2: fixup irq: got 224
> pci 0000:01:00.2: assigning IRQ 224
> pci 0000:01:00.3: fixup irq: got 224
> pci 0000:01:00.3: assigning IRQ 224
>
> pci 0000:02:00.0: fixup irq: got 225
> pci 0000:02:00.0: assigning IRQ 225
>
>
> But child nodes without interrupt-map properties:
> It seems incorrect.
>
> pci 0000:00:01.0: fixup irq: got 224
> pci 0000:00:01.0: assigning IRQ 224
> pci 0000:00:02.0: fixup irq: got 225
> pci 0000:00:02.0: assigning IRQ 225
>
> pci 0000:01:00.0: fixup irq: got 223
> pci 0000:01:00.0: assigning IRQ 223

Not entirely sure what happens here, but I guess the problem
is that the 'reg' portion of the parent interrupt-map refers to
the port devices, not the devices attached the devices behind
them.

On a related note, I see that you still list

> +- interrupts: Three interrupt outputs of the controller. Must contain an
> +  entry for each entry in the interrupt-names property.
> +- interrupt-names: Must include the following names
> +  - "pcie-int0"
> +  - "pcie-int1"
> +  - "pcie-int2"

This seems to be an artifact from the older version and should be
removed as the driver correctly ignores the properties now.

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

On Wed, 2017-05-10 at 12:01 +0200, Arnd Bergmann wrote:
> On Wed, May 10, 2017 at 11:31 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> > On Wed, 2017-05-10 at 10:08 +0200, Arnd Bergmann wrote:
> >> On Wed, May 10, 2017 at 4:07 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> >>
> >> > +- ranges:
> >> > +  - The first three entries are expected to translate the addresses for the root
> >> > +    port registers, which are referenced by the assigned-addresses property of
> >> > +    the root port nodes (see below).
> >>
> >> I don't understand this part. Why do you need a static translation for these?
> >> Shouldn't they just be listed in the 'reg' property of the parent node now that
> >> you have the clk/reset/phy properties in the parent as well?
> >
> > At first, I did like that. But I noticed that someone suggest it's
> > better to use 'assigned-addresses' to handle per-port registers, the
> > same path as tegra and marvell did, in other platform discussion thread.
> > So I just put shared register in root node. It could be rolled back if
> > you feel this is inappropriate.
> 
> The marvell case is not a good example for your case: their top-level
> device is made up by the OS to help with the shared resource allocation,
> while in your case the bus bridge actually exists in hardware.
> 
> I'm not too familiar with the Tegra case, and haven't looked at that here,
> but it could be an artifact of how for a while we used to list the config
> space access in the top-level "ranges" instead of the "reg" property.
> 
> I'd vote for moving it back, for consistency with the other port specific
> properties that are now in the root node. Once you do that, the port
> nodes can be removed completely, which is what I was aiming for with
> the comments on the previous version.
 
I'll move it back.

> >> > +Required properties:
> >> > +- device_type: Must be "pci"
> >> > +- assigned-addresses: Address and size of the port configuration registers
> >> > +- reg: Only the first four bytes are used to refer to the correct bus number
> >> > +  and device number.
> >> > +- #address-cells: Must be 3
> >> > +- #size-cells: Must be 2
> >> > +- #interrupt-cells: Must be 1
> >> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> >> > +  Please refer to the standard PCI bus binding document for a more detailed
> >> > +  explanation.
> >>
> >> Child nodes do not normally have interrupt-map properties. Isn't this
> >> already covered by the interrupt-map in the parent?
> >>
> >
> > I have one Intel 4 port ethernet card(0000:00:01) and MTK WLAN card
> > (0000:00:02), probe message looks good to me.
> >
> > pci 0000:00:01.0: fixup irq: got 224
> > pci 0000:00:01.0: assigning IRQ 224
> > pci 0000:00:02.0: fixup irq: got 225
> > pci 0000:00:02.0: assigning IRQ 225
> >
> > pci 0000:01:00.0: fixup irq: got 224
> > pci 0000:01:00.0: assigning IRQ 224
> > pci 0000:01:00.1: fixup irq: got 224
> > pci 0000:01:00.1: assigning IRQ 224
> > pci 0000:01:00.2: fixup irq: got 224
> > pci 0000:01:00.2: assigning IRQ 224
> > pci 0000:01:00.3: fixup irq: got 224
> > pci 0000:01:00.3: assigning IRQ 224
> >
> > pci 0000:02:00.0: fixup irq: got 225
> > pci 0000:02:00.0: assigning IRQ 225
> >
> >
> > But child nodes without interrupt-map properties:
> > It seems incorrect.
> >
> > pci 0000:00:01.0: fixup irq: got 224
> > pci 0000:00:01.0: assigning IRQ 224
> > pci 0000:00:02.0: fixup irq: got 225
> > pci 0000:00:02.0: assigning IRQ 225
> >
> > pci 0000:01:00.0: fixup irq: got 223
> > pci 0000:01:00.0: assigning IRQ 223
> 
> Not entirely sure what happens here, but I guess the problem
> is that the 'reg' portion of the parent interrupt-map refers to
> the port devices, not the devices attached the devices behind
> them.

I agree with you. That's why I need additional interrupt-map properties
to resolve IRQ correctly for the devices behind root ports.

Not sure whether other platforms have similar case like me here.

> On a related note, I see that you still list
> 
> > +- interrupts: Three interrupt outputs of the controller. Must contain an
> > +  entry for each entry in the interrupt-names property.
> > +- interrupt-names: Must include the following names
> > +  - "pcie-int0"
> > +  - "pcie-int1"
> > +  - "pcie-int2"
> 
> This seems to be an artifact from the older version and should be
> removed as the driver correctly ignores the properties now.

Actually, everything works fine without these properties however when it
loads we see a few weird error message:

pcieport 0000:00:01.0: Signaling PME with IRQ 232
pcieport 0000:00:02.0: enabling device (0140 -> 0142)
pcieport 0000:00:02.0: enabling bus mastering
irq 232: nobody cared (try booting with the "irqpoll" option)
...
[<c03f6be4>] (pcie_pme_probe) from [<c03f47b8>] (pcie_port_probe_service
+0x44/0x6c)
(pcie_port_probe_service) from [<c0454cf8>] (driver_probe_device
+0x280/0x470)
...
(pcie_port_device_register) from [<c03f51a0>] (pcie_portdrv_probe
+0x3c/0xb4)
(pcie_portdrv_probe) from [<c03e7acc>] (pci_device_probe+0x98/0xfc)
(pci_device_probe) from [<c0454cf8>] (driver_probe_device+0x280/0x470)
handlers:
[<c03f68b0>] pcie_pme_irq
Disabling IRQ #233

I haven't dig it out yet, but just keep them here to solve that.

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Arnd Bergmann]

On Thu, May 11, 2017 at 4:44 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> On Wed, 2017-05-10 at 12:01 +0200, Arnd Bergmann wrote:
>> On Wed, May 10, 2017 at 11:31 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
>> > On Wed, 2017-05-10 at 10:08 +0200, Arnd Bergmann wrote:

>> >> > +Required properties:
>> >> > +- device_type: Must be "pci"
>> >> > +- assigned-addresses: Address and size of the port configuration registers
>> >> > +- reg: Only the first four bytes are used to refer to the correct bus number
>> >> > +  and device number.
>> >> > +- #address-cells: Must be 3
>> >> > +- #size-cells: Must be 2
>> >> > +- #interrupt-cells: Must be 1
>> >> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
>> >> > +  Please refer to the standard PCI bus binding document for a more detailed
>> >> > +  explanation.
>> >>
>> >> Child nodes do not normally have interrupt-map properties. Isn't this
>> >> already covered by the interrupt-map in the parent?
>> >>
>> >
>> > I have one Intel 4 port ethernet card(0000:00:01) and MTK WLAN card
>> > (0000:00:02), probe message looks good to me.
>> >
>> > pci 0000:00:01.0: fixup irq: got 224
>> > pci 0000:00:01.0: assigning IRQ 224
>> > pci 0000:00:02.0: fixup irq: got 225
>> > pci 0000:00:02.0: assigning IRQ 225
>> >
>> > pci 0000:01:00.0: fixup irq: got 224
>> > pci 0000:01:00.0: assigning IRQ 224
>> > pci 0000:01:00.1: fixup irq: got 224
>> > pci 0000:01:00.1: assigning IRQ 224
>> > pci 0000:01:00.2: fixup irq: got 224
>> > pci 0000:01:00.2: assigning IRQ 224
>> > pci 0000:01:00.3: fixup irq: got 224
>> > pci 0000:01:00.3: assigning IRQ 224
>> >
>> > pci 0000:02:00.0: fixup irq: got 225
>> > pci 0000:02:00.0: assigning IRQ 225
>> >
>> >
>> > But child nodes without interrupt-map properties:
>> > It seems incorrect.
>> >
>> > pci 0000:00:01.0: fixup irq: got 224
>> > pci 0000:00:01.0: assigning IRQ 224
>> > pci 0000:00:02.0: fixup irq: got 225
>> > pci 0000:00:02.0: assigning IRQ 225
>> >
>> > pci 0000:01:00.0: fixup irq: got 223
>> > pci 0000:01:00.0: assigning IRQ 223
>>
>> Not entirely sure what happens here, but I guess the problem
>> is that the 'reg' portion of the parent interrupt-map refers to
>> the port devices, not the devices attached the devices behind
>> them.
>
> I agree with you. That's why I need additional interrupt-map properties
> to resolve IRQ correctly for the devices behind root ports.
>
> Not sure whether other platforms have similar case like me here.

I think it's just a bug in this specific chip where the HW designers
wired the IRQs in a nonstandard way.

However, you really should not need the interrupt-map properties
in the child nodes, just change the address part in the parent
interrupt-map. Specifically, the 'bus' portion of the device address
in the interrupt-map would have to be nonzero to refer to
child devices.

>> On a related note, I see that you still list
>>
>> > +- interrupts: Three interrupt outputs of the controller. Must contain an
>> > +  entry for each entry in the interrupt-names property.
>> > +- interrupt-names: Must include the following names
>> > +  - "pcie-int0"
>> > +  - "pcie-int1"
>> > +  - "pcie-int2"
>>
>> This seems to be an artifact from the older version and should be
>> removed as the driver correctly ignores the properties now.
>
> Actually, everything works fine without these properties however when it
> loads we see a few weird error message:
>
> pcieport 0000:00:01.0: Signaling PME with IRQ 232
> pcieport 0000:00:02.0: enabling device (0140 -> 0142)
> pcieport 0000:00:02.0: enabling bus mastering
> irq 232: nobody cared (try booting with the "irqpoll" option)
> ...
> [<c03f6be4>] (pcie_pme_probe) from [<c03f47b8>] (pcie_port_probe_service
> +0x44/0x6c)
> (pcie_port_probe_service) from [<c0454cf8>] (driver_probe_device
> +0x280/0x470)
> ...
> (pcie_port_device_register) from [<c03f51a0>] (pcie_portdrv_probe
> +0x3c/0xb4)
> (pcie_portdrv_probe) from [<c03e7acc>] (pci_device_probe+0x98/0xfc)
> (pci_device_probe) from [<c0454cf8>] (driver_probe_device+0x280/0x470)
> handlers:
> [<c03f68b0>] pcie_pme_irq
> Disabling IRQ #233
>
> I haven't dig it out yet, but just keep them here to solve that.

Something is going very wrong if adding the properties helps. I can't
think of what that is, but we have to find out before the binding can
be merged.

      Arnd

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

On Thu, 2017-05-11 at 09:17 +0200, Arnd Bergmann wrote:
> On Thu, May 11, 2017 at 4:44 AM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > On Wed, 2017-05-10 at 12:01 +0200, Arnd Bergmann wrote:
> >> On Wed, May 10, 2017 at 11:31 AM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >> > On Wed, 2017-05-10 at 10:08 +0200, Arnd Bergmann wrote:
> 
> >> >> > +Required properties:
> >> >> > +- device_type: Must be "pci"
> >> >> > +- assigned-addresses: Address and size of the port configuration registers
> >> >> > +- reg: Only the first four bytes are used to refer to the correct bus number
> >> >> > +  and device number.
> >> >> > +- #address-cells: Must be 3
> >> >> > +- #size-cells: Must be 2
> >> >> > +- #interrupt-cells: Must be 1
> >> >> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> >> >> > +  Please refer to the standard PCI bus binding document for a more detailed
> >> >> > +  explanation.
> >> >>
> >> >> Child nodes do not normally have interrupt-map properties. Isn't this
> >> >> already covered by the interrupt-map in the parent?
> >> >>
> >> >
> >> > I have one Intel 4 port ethernet card(0000:00:01) and MTK WLAN card
> >> > (0000:00:02), probe message looks good to me.
> >> >
> >> > pci 0000:00:01.0: fixup irq: got 224
> >> > pci 0000:00:01.0: assigning IRQ 224
> >> > pci 0000:00:02.0: fixup irq: got 225
> >> > pci 0000:00:02.0: assigning IRQ 225
> >> >
> >> > pci 0000:01:00.0: fixup irq: got 224
> >> > pci 0000:01:00.0: assigning IRQ 224
> >> > pci 0000:01:00.1: fixup irq: got 224
> >> > pci 0000:01:00.1: assigning IRQ 224
> >> > pci 0000:01:00.2: fixup irq: got 224
> >> > pci 0000:01:00.2: assigning IRQ 224
> >> > pci 0000:01:00.3: fixup irq: got 224
> >> > pci 0000:01:00.3: assigning IRQ 224
> >> >
> >> > pci 0000:02:00.0: fixup irq: got 225
> >> > pci 0000:02:00.0: assigning IRQ 225
> >> >
> >> >
> >> > But child nodes without interrupt-map properties:
> >> > It seems incorrect.
> >> >
> >> > pci 0000:00:01.0: fixup irq: got 224
> >> > pci 0000:00:01.0: assigning IRQ 224
> >> > pci 0000:00:02.0: fixup irq: got 225
> >> > pci 0000:00:02.0: assigning IRQ 225
> >> >
> >> > pci 0000:01:00.0: fixup irq: got 223
> >> > pci 0000:01:00.0: assigning IRQ 223
> >>
> >> Not entirely sure what happens here, but I guess the problem
> >> is that the 'reg' portion of the parent interrupt-map refers to
> >> the port devices, not the devices attached the devices behind
> >> them.
> >
> > I agree with you. That's why I need additional interrupt-map properties
> > to resolve IRQ correctly for the devices behind root ports.
> >
> > Not sure whether other platforms have similar case like me here.
> 
> I think it's just a bug in this specific chip where the HW designers
> wired the IRQs in a nonstandard way.
> 
> However, you really should not need the interrupt-map properties
> in the child nodes, just change the address part in the parent
> interrupt-map. Specifically, the 'bus' portion of the device address
> in the interrupt-map would have to be nonzero to refer to
> child devices.

This is what I modify for the parent node and remove interrupt-map
properties from child..

interrupt-map-mask = <0xff800 0 0 0>;
interrupt-map = <0x0000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
		 <0x0800 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
		 <0x1000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>,

		 /* workaround here*/
		 <0x10000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
		 <0x20000 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
	     <0x30000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;

It works well. But how could we handle the situation if root port0
status = "disabled" ? I think we cannot assign child bus number
dynamically from binding.

> >> On a related note, I see that you still list
> >>
> >> > +- interrupts: Three interrupt outputs of the controller. Must contain an
> >> > +  entry for each entry in the interrupt-names property.
> >> > +- interrupt-names: Must include the following names
> >> > +  - "pcie-int0"
> >> > +  - "pcie-int1"
> >> > +  - "pcie-int2"
> >>
> >> This seems to be an artifact from the older version and should be
> >> removed as the driver correctly ignores the properties now.
> >
> > Actually, everything works fine without these properties however when it
> > loads we see a few weird error message:
> >
> > pcieport 0000:00:01.0: Signaling PME with IRQ 232
> > pcieport 0000:00:02.0: enabling device (0140 -> 0142)
> > pcieport 0000:00:02.0: enabling bus mastering
> > irq 232: nobody cared (try booting with the "irqpoll" option)
> > ...
> > [<c03f6be4>] (pcie_pme_probe) from [<c03f47b8>] (pcie_port_probe_service
> > +0x44/0x6c)
> > (pcie_port_probe_service) from [<c0454cf8>] (driver_probe_device
> > +0x280/0x470)
> > ...
> > (pcie_port_device_register) from [<c03f51a0>] (pcie_portdrv_probe
> > +0x3c/0xb4)
> > (pcie_portdrv_probe) from [<c03e7acc>] (pci_device_probe+0x98/0xfc)
> > (pci_device_probe) from [<c0454cf8>] (driver_probe_device+0x280/0x470)
> > handlers:
> > [<c03f68b0>] pcie_pme_irq
> > Disabling IRQ #233
> >
> > I haven't dig it out yet, but just keep them here to solve that.
> 
> Something is going very wrong if adding the properties helps. I can't
> think of what that is, but we have to find out before the binding can
> be merged.

Not really understand PME service. But I will find the reason here.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

Hi Arnd,

I want to further explain what I have discussed in previous mail.


On Thu, 2017-05-11 at 17:08 +0800, Ryder Lee wrote:
> On Thu, 2017-05-11 at 09:17 +0200, Arnd Bergmann wrote:
> > On Thu, May 11, 2017 at 4:44 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> > > On Wed, 2017-05-10 at 12:01 +0200, Arnd Bergmann wrote:
> > >> On Wed, May 10, 2017 at 11:31 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> > >> > On Wed, 2017-05-10 at 10:08 +0200, Arnd Bergmann wrote:
> > 
> > >> >> > +Required properties:
> > >> >> > +- device_type: Must be "pci"
> > >> >> > +- assigned-addresses: Address and size of the port configuration registers
> > >> >> > +- reg: Only the first four bytes are used to refer to the correct bus number
> > >> >> > +  and device number.
> > >> >> > +- #address-cells: Must be 3
> > >> >> > +- #size-cells: Must be 2
> > >> >> > +- #interrupt-cells: Must be 1
> > >> >> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> > >> >> > +  Please refer to the standard PCI bus binding document for a more detailed
> > >> >> > +  explanation.
> > >> >>
> > >> >> Child nodes do not normally have interrupt-map properties. Isn't this
> > >> >> already covered by the interrupt-map in the parent?
> > >> >>
> > >> >
> > >> > I have one Intel 4 port ethernet card(0000:00:01) and MTK WLAN card
> > >> > (0000:00:02), probe message looks good to me.
> > >> >
> > >> > pci 0000:00:01.0: fixup irq: got 224
> > >> > pci 0000:00:01.0: assigning IRQ 224
> > >> > pci 0000:00:02.0: fixup irq: got 225
> > >> > pci 0000:00:02.0: assigning IRQ 225
> > >> >
> > >> > pci 0000:01:00.0: fixup irq: got 224
> > >> > pci 0000:01:00.0: assigning IRQ 224
> > >> > pci 0000:01:00.1: fixup irq: got 224
> > >> > pci 0000:01:00.1: assigning IRQ 224
> > >> > pci 0000:01:00.2: fixup irq: got 224
> > >> > pci 0000:01:00.2: assigning IRQ 224
> > >> > pci 0000:01:00.3: fixup irq: got 224
> > >> > pci 0000:01:00.3: assigning IRQ 224
> > >> >
> > >> > pci 0000:02:00.0: fixup irq: got 225
> > >> > pci 0000:02:00.0: assigning IRQ 225
> > >> >
> > >> >
> > >> > But child nodes without interrupt-map properties:
> > >> > It seems incorrect.
> > >> >
> > >> > pci 0000:00:01.0: fixup irq: got 224
> > >> > pci 0000:00:01.0: assigning IRQ 224
> > >> > pci 0000:00:02.0: fixup irq: got 225
> > >> > pci 0000:00:02.0: assigning IRQ 225
> > >> >
> > >> > pci 0000:01:00.0: fixup irq: got 223
> > >> > pci 0000:01:00.0: assigning IRQ 223
> > >>
> > >> Not entirely sure what happens here, but I guess the problem
> > >> is that the 'reg' portion of the parent interrupt-map refers to
> > >> the port devices, not the devices attached the devices behind
> > >> them.
> > >
> > > I agree with you. That's why I need additional interrupt-map properties
> > > to resolve IRQ correctly for the devices behind root ports.
> > >
> > > Not sure whether other platforms have similar case like me here.
> > 
> > I think it's just a bug in this specific chip where the HW designers
> > wired the IRQs in a nonstandard way.
> > 
> > However, you really should not need the interrupt-map properties
> > in the child nodes, just change the address part in the parent
> > interrupt-map. Specifically, the 'bus' portion of the device address
> > in the interrupt-map would have to be nonzero to refer to
> > child devices.
> 
> This is what I modify for the parent node and remove interrupt-map
> properties from child..
> 
> interrupt-map-mask = <0xff800 0 0 0>;
> interrupt-map = <0x0000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
> 		 <0x0800 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
> 		 <0x1000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>,
> 
> 		 /* workaround here*/
> 		 <0x10000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
> 		 <0x20000 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
> 	     <0x30000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
> 
> It works well. But how could we handle the situation if root port0
> status = "disabled" ? I think we cannot assign child bus number
> dynamically from binding.

That is to say, we route it statically if port0 (or port1) is
unavailable. The PCI child bus enumeration should look something like
this:

 pci 0000:00:01.0: fixup irq: got 224
 pci 0000:00:01.0: assigning IRQ 224
 pci 0000:00:02.0: fixup irq: got 225
 pci 0000:00:02.0: assigning IRQ 225
 
 Go wrong here! IRQ 223/224 should be assigned to the devices behind
port0 and port1.
 pci 0000:01:00.0: fixup irq: got 223
 pci 0000:01:00.0: assigning IRQ 223
 pci 0000:02:00.0: fixup irq: got 224
 pci 0000:02:00.0: assigning IRQ 224

> > >> On a related note, I see that you still list
> > >>
> > >> > +- interrupts: Three interrupt outputs of the controller. Must contain an
> > >> > +  entry for each entry in the interrupt-names property.
> > >> > +- interrupt-names: Must include the following names
> > >> > +  - "pcie-int0"
> > >> > +  - "pcie-int1"
> > >> > +  - "pcie-int2"
> > >>
> > >> This seems to be an artifact from the older version and should be
> > >> removed as the driver correctly ignores the properties now.
> > >
> > > Actually, everything works fine without these properties however when it
> > > loads we see a few weird error message:
> > >
> > > pcieport 0000:00:01.0: Signaling PME with IRQ 232
> > > pcieport 0000:00:02.0: enabling device (0140 -> 0142)
> > > pcieport 0000:00:02.0: enabling bus mastering
> > > irq 232: nobody cared (try booting with the "irqpoll" option)
> > > ...
> > > [<c03f6be4>] (pcie_pme_probe) from [<c03f47b8>] (pcie_port_probe_service
> > > +0x44/0x6c)
> > > (pcie_port_probe_service) from [<c0454cf8>] (driver_probe_device
> > > +0x280/0x470)
> > > ...
> > > (pcie_port_device_register) from [<c03f51a0>] (pcie_portdrv_probe
> > > +0x3c/0xb4)
> > > (pcie_portdrv_probe) from [<c03e7acc>] (pci_device_probe+0x98/0xfc)
> > > (pci_device_probe) from [<c0454cf8>] (driver_probe_device+0x280/0x470)
> > > handlers:
> > > [<c03f68b0>] pcie_pme_irq
> > > Disabling IRQ #233
> > >
> > > I haven't dig it out yet, but just keep them here to solve that.
> > 
> > Something is going very wrong if adding the properties helps. I can't
> > think of what that is, but we have to find out before the binding can
> > be merged.
> 
> Not really understand PME service. But I will find the reason here.

I have do some test here. PME needs port IRQs, which interrupt type was
set correctly(IRQ_TYPE_LEVEL_LOW). But we cannot set it from
interrupt-map, according to gic_set_type() /* SPIs have restrictions on
the supported types */ .

So we need to add additional interrupt properties.

 

Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

Hi Arnd,

Sorry to bother you again.

On Thu, 2017-05-11 at 20:11 +0800, Ryder Lee wrote:
> > interrupt-map-mask = <0xff800 0 0 0>;
> > interrupt-map = <0x0000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
> > 		 <0x0800 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
> > 		 <0x1000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>,
> > 
> > 		 /* workaround here*/
> > 		 <0x10000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
> > 		 <0x20000 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
> > 	     <0x30000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
> > 
> > It works well. But how could we handle the situation if root port0
> > status = "disabled" ? I think we cannot assign child bus number
> > dynamically from binding.
> 
> That is to say, we route it statically if port0 (or port1) is
> unavailable. The PCI child bus enumeration should look something like
> this:
> 
>  pci 0000:00:01.0: fixup irq: got 224
>  pci 0000:00:01.0: assigning IRQ 224
>  pci 0000:00:02.0: fixup irq: got 225
>  pci 0000:00:02.0: assigning IRQ 225
>  
>  Go wrong here! IRQ 223/224 should be assigned to the devices behind
> port0 and port1.
>  pci 0000:01:00.0: fixup irq: got 223
>  pci 0000:01:00.0: assigning IRQ 223
>  pci 0000:02:00.0: fixup irq: got 224
>  pci 0000:02:00.0: assigning IRQ 224

What I thought was wrong. I have misunderstood something in previous
discussion. Actually it could work for the situation that I mentioned
before. However, I'm not sure whether this is a proper representation
you want to see.

> > > >> On a related note, I see that you still list
> > > >>
> > > >> > +- interrupts: Three interrupt outputs of the controller. Must contain an
> > > >> > +  entry for each entry in the interrupt-names property.
> > > >> > +- interrupt-names: Must include the following names
> > > >> > +  - "pcie-int0"
> > > >> > +  - "pcie-int1"
> > > >> > +  - "pcie-int2"
> > > >>
> > > >> This seems to be an artifact from the older version and should be
> > > >> removed as the driver correctly ignores the properties now.
> > > >
> > > > Actually, everything works fine without these properties however when it
> > > > loads we see a few weird error message:
> > > >
> > > > pcieport 0000:00:01.0: Signaling PME with IRQ 232
> > > > pcieport 0000:00:02.0: enabling device (0140 -> 0142)
> > > > pcieport 0000:00:02.0: enabling bus mastering
> > > > irq 232: nobody cared (try booting with the "irqpoll" option)
> > > > ...
> > > > [<c03f6be4>] (pcie_pme_probe) from [<c03f47b8>] (pcie_port_probe_service
> > > > +0x44/0x6c)
> > > > (pcie_port_probe_service) from [<c0454cf8>] (driver_probe_device
> > > > +0x280/0x470)
> > > > ...
> > > > (pcie_port_device_register) from [<c03f51a0>] (pcie_portdrv_probe
> > > > +0x3c/0xb4)
> > > > (pcie_portdrv_probe) from [<c03e7acc>] (pci_device_probe+0x98/0xfc)
> > > > (pci_device_probe) from [<c0454cf8>] (driver_probe_device+0x280/0x470)
> > > > handlers:
> > > > [<c03f68b0>] pcie_pme_irq
> > > > Disabling IRQ #233
> > > >
> > > > I haven't dig it out yet, but just keep them here to solve that.
> > > 
> > > Something is going very wrong if adding the properties helps. I can't
> > > think of what that is, but we have to find out before the binding can
> > > be merged.
> > 
> > Not really understand PME service. But I will find the reason here.
> 
> I have do some test here. PME needs port IRQs, which interrupt type was
> set correctly(IRQ_TYPE_LEVEL_LOW). But we cannot set it from
> interrupt-map, according to gic_set_type() /* SPIs have restrictions on
> the supported types */ .
> 
> So we need to add additional interrupt properties.

I could use iPerf to test my WLAN card normally. But just ignore this
exception message. I would definitely appreciate if someone could give
me some hint on how to properly solve it. 

Thanks a lot.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html