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

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

[Ryder Lee]

Hi,

This patch series add PCIe host controller driver and dt-binding
documentation for Mediatek mt7623 SoCs families.

This driver was validated using Broadcom Tigon3 ethernet card.

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          | 153 ++++++
 drivers/pci/host/Kconfig                           |  11 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-mediatek.c                   | 611 +++++++++++++++++++++
 4 files changed, 776 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
 create mode 100644 drivers/pci/host/pcie-mediatek.c

-- 
1.9.1

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

[Ryder Lee]

Add support for the Mediatek PCIe controller which can be found
on MT7623A/N, MT2701 and MT8521p platforms.

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 | 611 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 623 insertions(+)
 create mode 100644 drivers/pci/host/pcie-mediatek.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index f7c1d4d..cf13b5d 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 for MT7623 SoCs families"
+	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 A/N
+	  series SoCs. There is a one 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..98e84d9
--- /dev/null
+++ b/drivers/pci/host/pcie-mediatek.c
@@ -0,0 +1,611 @@
+/*
+ * PCIe host controller driver for Mediatek MT7623 SoCs families
+ *
+ * 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
+ * @dev: pointer to root port device
+ * @base: IO mapped register base
+ * @list: port list
+ * @pcie: pointer to PCIe host info
+ * @reset: pointer to RC reset control
+ * @regs: port memory region
+ * @sys_ck: root port clock
+ * @phy: pointer to phy control block
+ * @irq: IRQ number
+ * @lane: lane count
+ * @index: port index
+ */
+struct mtk_pcie_port {
+	struct device *dev;
+	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;
+	int irq;
+	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
+ * @resources: bus resources
+ * @ports: pointer to PCIe port information
+ */
+struct mtk_pcie {
+	struct device *dev;
+	void __iomem *base;
+	struct clk *free_ck;
+	struct list_head resources;
+	struct list_head ports;
+};
+
+static inline bool mtk_pcie_link_is_up(struct mtk_pcie_port *port)
+{
+	return !!(readl_relaxed(port->base + PCIE_LINK_STATUS) &
+		  PCIE_PORT_LINKUP);
+}
+
+static bool mtk_pcie_valid_device(struct mtk_pcie *pcie,
+				  struct pci_bus *bus, int devfn)
+{
+	struct mtk_pcie_port *port;
+	struct pci_dev *dev;
+	struct pci_bus *pbus;
+
+	/* if there is no link, then there is no device */
+	list_for_each_entry(port, &pcie->ports, list) {
+		if (bus->number == 0 && port->index == PCI_SLOT(devfn) &&
+		    mtk_pcie_link_is_up(port)) {
+			return true;
+		} else if (bus->number != 0) {
+			pbus = bus;
+			do {
+				dev = pbus->self;
+				if (port->index == PCI_SLOT(dev->devfn) &&
+				    mtk_pcie_link_is_up(port)) {
+					return true;
+				}
+				pbus = dev->bus;
+			} while (dev->bus->number != 0);
+		}
+	}
+
+	return false;
+}
+
+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 int mtk_pcie_hw_rd_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
+			      int where, int size, u32 *val)
+{
+	writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
+	       pcie->base + PCIE_CFG_ADDR);
+
+	*val = 0;
+
+	switch (size) {
+	case 1:
+		*val = readb(pcie->base + PCIE_CFG_DATA + (where & 3));
+		break;
+	case 2:
+		*val = readw(pcie->base + PCIE_CFG_DATA + (where & 2));
+		break;
+	case 4:
+		*val = readl(pcie->base + PCIE_CFG_DATA);
+		break;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int mtk_pcie_hw_wr_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
+			      int where, int size, u32 val)
+
+{
+	writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
+	       pcie->base + PCIE_CFG_ADDR);
+
+	switch (size) {
+	case 1:
+		writeb(val, pcie->base + PCIE_CFG_DATA + (where & 3));
+		break;
+	case 2:
+		writew(val, pcie->base + PCIE_CFG_DATA + (where & 2));
+		break;
+	case 4:
+		writel(val, pcie->base + PCIE_CFG_DATA);
+		break;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int mtk_pcie_read_config(struct pci_bus *bus, u32 devfn,
+				int where, int size, u32 *val)
+{
+	struct mtk_pcie *pcie = bus->sysdata;
+	u32 bn = bus->number;
+
+	if (!mtk_pcie_valid_device(pcie, bus, devfn)) {
+		*val = 0xffffffff;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	return mtk_pcie_hw_rd_cfg(pcie, bn, devfn, where, size, val);
+}
+
+static int mtk_pcie_write_config(struct pci_bus *bus, u32 devfn,
+				 int where, int size, u32 val)
+{
+	struct mtk_pcie *pcie = bus->sysdata;
+	u32 bn = bus->number;
+
+	if (!mtk_pcie_valid_device(pcie, bus, devfn))
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return mtk_pcie_hw_wr_cfg(pcie, bn, devfn, where, size, val);
+}
+
+static struct pci_ops mtk_pcie_ops = {
+	.read  = mtk_pcie_read_config,
+	.write = mtk_pcie_write_config,
+};
+
+static void mtk_pcie_configure_rc(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	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 */
+	mtk_pcie_hw_rd_cfg(pcie, 0, (port->index << 3),
+			   PCIE_FC_CREDIT, 4, &val);
+	val &= ~PCIE_FC_CREDIT_MASK;
+	val |= PCIE_FC_CREDIT_VAL(0x806c);
+	mtk_pcie_hw_wr_cfg(pcie, 0, (port->index << 3),
+			   PCIE_FC_CREDIT, 4, val);
+
+	/* configure RC FTS number to 250 when it leaves L0s */
+	mtk_pcie_hw_rd_cfg(pcie, 0, (port->index << 3),
+			   PCIE_FTS_NUM, 4, &val);
+	val &= ~PCIE_FTS_NUM_MASK;
+	val |= PCIE_FTS_NUM_L0(0x50);
+	mtk_pcie_hw_wr_cfg(pcie, 0, (port->index << 3),
+			   PCIE_FTS_NUM, 4, val);
+}
+
+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);
+
+	/*
+	 * at least 100ms delay because PCIe v2.0 need more time to
+	 * train from Gen1 to Gen2
+	 */
+	msleep(100);
+}
+
+static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct mtk_pcie_port *port, *tmp;
+	int err, linkup = 0;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		err = clk_prepare_enable(port->sys_ck);
+		if (err) {
+			dev_err(dev, "failed to enable port%d clock\n",
+				port->index);
+			continue;
+		}
+
+		/* assert RC */
+		reset_control_assert(port->reset);
+		/* de-assert RC */
+		reset_control_deassert(port->reset);
+
+		/* power on PHY */
+		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);
+			linkup++;
+			continue;
+		}
+
+		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);
+	}
+
+	return linkup;
+}
+
+static int mtk_pcie_get_port_resource(struct mtk_pcie_port *port,
+				      struct device_node *node)
+{
+	struct device *dev = port->pcie->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct platform_device *plat_dev;
+	char name[10];
+	int 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", port->index);
+		return PTR_ERR(port->base);
+	}
+
+	plat_dev = of_find_device_by_node(node);
+	if (!plat_dev) {
+		plat_dev = of_platform_device_create(
+					node, NULL,
+					platform_bus_type.dev_root);
+		if (!plat_dev)
+			return -EPROBE_DEFER;
+	}
+
+	port->dev = &plat_dev->dev;
+
+	port->irq = platform_get_irq(pdev, port->index);
+	if (!port->irq) {
+		dev_err(dev, "failed to get irq\n");
+		return -ENODEV;
+	}
+
+	port->sys_ck = devm_clk_get(port->dev, "sys_ck");
+	if (IS_ERR(port->sys_ck)) {
+		dev_err(port->dev, "failed to get port%d clock\n", port->index);
+		return PTR_ERR(port->sys_ck);
+	}
+
+	port->reset = devm_reset_control_get(port->dev, "pcie-reset");
+	if (IS_ERR(port->reset)) {
+		dev_err(port->dev, "failed to get port%d reset control\n",
+			port->index);
+		return PTR_ERR(port->reset);
+	}
+
+	snprintf(name, sizeof(name), "pcie-phy%d", port->index);
+	port->phy = devm_of_phy_get(port->dev, node, name);
+	if (IS_ERR(port->phy)) {
+		dev_err(port->dev, "failed to get port%d phy\n", port->index);
+		return PTR_ERR(port->phy);
+	}
+
+	return 0;
+}
+
+static int mtk_pcie_parse_and_add_res(struct mtk_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct device_node *node = dev->of_node, *child;
+	struct resource_entry *win, *tmp;
+	struct resource *regs;
+	resource_size_t iobase;
+	int err;
+
+	/* parse shared resources */
+	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 get PCIe base\n");
+		return PTR_ERR(pcie->base);
+	}
+
+	pcie->free_ck = devm_clk_get(dev, "free_ck");
+	if (IS_ERR(pcie->free_ck)) {
+		dev_err(dev, "failed to get free_ck\n");
+		return PTR_ERR(pcie->free_ck);
+	}
+
+	err = of_pci_get_host_bridge_resources(node, 0, 0xff, &pcie->resources,
+					       &iobase);
+	if (err)
+		return err;
+
+	err = devm_request_pci_bus_resources(dev, &pcie->resources);
+	if (err)
+		return err;
+
+	resource_list_for_each_entry_safe(win, tmp, &pcie->resources) {
+		struct resource *res = win->res;
+
+		switch (resource_type(res)) {
+		case IORESOURCE_IO:
+			err = pci_remap_iospace(res, iobase);
+			if (err) {
+				dev_warn(dev, "failed to map resource %pR\n",
+					 res);
+				resource_list_destroy_entry(win);
+			}
+			break;
+		}
+	}
+
+	/* parse port resources */
+	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(pcie->dev, "failed to parse devfn: %d\n", err);
+			return err;
+		}
+
+		index = PCI_SLOT(err);
+		if (index < 1) {
+			dev_err(dev, "invalid port number: %d\n", index);
+			return -EINVAL;
+		}
+
+		index--;
+
+		if (!of_device_is_available(child))
+			continue;
+
+		port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+		if (!port)
+			return -ENOMEM;
+
+		err = of_property_read_u32(child, "num-lanes", &port->lane);
+		if (err) {
+			dev_err(dev, "missing num-lanes property\n");
+			return err;
+		}
+
+		port->index = index;
+		port->pcie = pcie;
+
+		err = mtk_pcie_get_port_resource(port, child);
+		if (err)
+			return err;
+
+		INIT_LIST_HEAD(&port->list);
+		list_add_tail(&port->list, &pcie->ports);
+	}
+
+	return 0;
+}
+
+/*
+ * This IP lacks interrupt status register to check or map INTx from
+ * different devices at the same time.
+ */
+static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
+{
+	struct mtk_pcie *pcie = dev->bus->sysdata;
+	struct mtk_pcie_port *port;
+
+	list_for_each_entry(port, &pcie->ports, list)
+		if (port->index == slot)
+			return port->irq;
+
+	return -1;
+}
+
+static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
+{
+	struct pci_bus *bus, *child;
+
+	bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
+				&pcie->resources);
+	if (!bus) {
+		dev_err(pcie->dev, "failed to create root bus\n");
+		return -ENOMEM;
+	}
+
+	if (!pci_has_flag(PCI_PROBE_ONLY)) {
+		pci_fixup_irqs(pci_common_swizzle, mtk_pcie_map_irq);
+		pci_bus_size_bridges(bus);
+		pci_bus_assign_resources(bus);
+
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+	}
+
+	pci_bus_add_devices(bus);
+
+	return 0;
+}
+
+static int mtk_pcie_probe(struct platform_device *pdev)
+{
+	struct mtk_pcie *pcie;
+	int err;
+
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pcie);
+
+	/*
+	 * parse PCI ranges, configuration bus range and
+	 * request their resources
+	 */
+	INIT_LIST_HEAD(&pcie->ports);
+	INIT_LIST_HEAD(&pcie->resources);
+
+	err = mtk_pcie_parse_and_add_res(pcie);
+	if (err)
+		goto err_parse;
+
+	pm_runtime_enable(pcie->dev);
+	err = pm_runtime_get_sync(pcie->dev);
+	if (err)
+		goto err_pm;
+
+	err = clk_prepare_enable(pcie->free_ck);
+	if (err) {
+		dev_err(pcie->dev, "failed to enable free_ck\n");
+		goto err_free_ck;
+	}
+
+	/* power on PCIe ports */
+	err = mtk_pcie_enable_ports(pcie);
+	if (!err)
+		goto err_enable;
+
+	/* register PCIe ports */
+	err = mtk_pcie_register_ports(pcie);
+	if (err)
+		goto err_enable;
+
+	return 0;
+
+err_enable:
+	clk_disable_unprepare(pcie->free_ck);
+err_free_ck:
+	pm_runtime_put_sync(pcie->dev);
+err_pm:
+	pm_runtime_disable(pcie->dev);
+err_parse:
+	pci_free_resource_list(&pcie->resources);
+
+	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,
+	},
+};
+
+builtin_platform_driver(mtk_pcie_driver);
+
+MODULE_DESCRIPTION("Mediatek PCIe host driver for MT7623 SoCs families");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]

Hi Ryder,

[auto build test WARNING on pci/next]
[also build test WARNING on v4.11-rc7 next-20170421]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ryder-Lee/Add-PCIe-host-driver-support-for-some-Mediatek-SoCs/20170423-163432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All warnings (new ones prefixed by >>):

>> WARNING: drivers/pci/host/built-in.o(.text+0x1830): Section mismatch in reference from the function mtk_pcie_probe() to the function .init.text:mtk_pcie_map_irq()
   The function mtk_pcie_probe() references
   the function __init mtk_pcie_map_irq().
   This is often because mtk_pcie_probe lacks a __init
   annotation or the annotation of mtk_pcie_map_irq is wrong.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46818 bytes --]

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

[Bjorn Helgaas]

Hi Ryder,

Looks good, but I have a few questions below.

On Sun, Apr 23, 2017 at 04:19:02PM +0800, Ryder Lee wrote:
> Add support for the Mediatek PCIe controller which can be found
> on MT7623A/N, MT2701 and MT8521p platforms.
> 
> 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 | 611 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 623 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-mediatek.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index f7c1d4d..cf13b5d 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 for MT7623 SoCs families"
> +	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 A/N
> +	  series SoCs. There is a one 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..98e84d9
> --- /dev/null
> +++ b/drivers/pci/host/pcie-mediatek.c
> @@ -0,0 +1,611 @@
> +/*
> + * PCIe host controller driver for Mediatek MT7623 SoCs families
> + *
> + * 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>
> +#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
> + * @dev: pointer to root port device
> + * @base: IO mapped register base
> + * @list: port list
> + * @pcie: pointer to PCIe host info
> + * @reset: pointer to RC reset control
> + * @regs: port memory region
> + * @sys_ck: root port clock
> + * @phy: pointer to phy control block
> + * @irq: IRQ number
> + * @lane: lane count
> + * @index: port index
> + */
> +struct mtk_pcie_port {
> +	struct device *dev;
> +	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;
> +	int irq;
> +	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
> + * @resources: bus resources
> + * @ports: pointer to PCIe port information
> + */
> +struct mtk_pcie {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct clk *free_ck;
> +	struct list_head resources;
> +	struct list_head ports;
> +};
> +
> +static inline bool mtk_pcie_link_is_up(struct mtk_pcie_port *port)
> +{
> +	return !!(readl_relaxed(port->base + PCIE_LINK_STATUS) &
> +		  PCIE_PORT_LINKUP);
> +}
> +
> +static bool mtk_pcie_valid_device(struct mtk_pcie *pcie,
> +				  struct pci_bus *bus, int devfn)
> +{
> +	struct mtk_pcie_port *port;
> +	struct pci_dev *dev;
> +	struct pci_bus *pbus;
> +
> +	/* if there is no link, then there is no device */
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		if (bus->number == 0 && port->index == PCI_SLOT(devfn) &&
> +		    mtk_pcie_link_is_up(port)) {
> +			return true;
> +		} else if (bus->number != 0) {
> +			pbus = bus;
> +			do {
> +				dev = pbus->self;
> +				if (port->index == PCI_SLOT(dev->devfn) &&
> +				    mtk_pcie_link_is_up(port)) {
> +					return true;
> +				}
> +				pbus = dev->bus;
> +			} while (dev->bus->number != 0);
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +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 int mtk_pcie_hw_rd_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
> +			      int where, int size, u32 *val)
> +{
> +	writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
> +	       pcie->base + PCIE_CFG_ADDR);
> +
> +	*val = 0;
> +
> +	switch (size) {
> +	case 1:
> +		*val = readb(pcie->base + PCIE_CFG_DATA + (where & 3));
> +		break;
> +	case 2:
> +		*val = readw(pcie->base + PCIE_CFG_DATA + (where & 2));
> +		break;
> +	case 4:
> +		*val = readl(pcie->base + PCIE_CFG_DATA);
> +		break;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int mtk_pcie_hw_wr_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
> +			      int where, int size, u32 val)
> +
> +{
> +	writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
> +	       pcie->base + PCIE_CFG_ADDR);
> +
> +	switch (size) {
> +	case 1:
> +		writeb(val, pcie->base + PCIE_CFG_DATA + (where & 3));
> +		break;
> +	case 2:
> +		writew(val, pcie->base + PCIE_CFG_DATA + (where & 2));
> +		break;
> +	case 4:
> +		writel(val, pcie->base + PCIE_CFG_DATA);
> +		break;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int mtk_pcie_read_config(struct pci_bus *bus, u32 devfn,
> +				int where, int size, u32 *val)
> +{
> +	struct mtk_pcie *pcie = bus->sysdata;
> +	u32 bn = bus->number;
> +
> +	if (!mtk_pcie_valid_device(pcie, bus, devfn)) {
> +		*val = 0xffffffff;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}

I know there are some other drivers with the *_valid_device() pattern
in their config accessors, but I don't like it because it's racy.
It's possible for the link to be up for the test above, then go down
before the actual config access below.

Your hardware *should* do something sensible if we try to read config
space when the link is down, and ideally that would be enough that we
don't need this "valid_device()" check.

> +	return mtk_pcie_hw_rd_cfg(pcie, bn, devfn, where, size, val);
> +}
> +
> +static int mtk_pcie_write_config(struct pci_bus *bus, u32 devfn,
> +				 int where, int size, u32 val)
> +{
> +	struct mtk_pcie *pcie = bus->sysdata;
> +	u32 bn = bus->number;
> +
> +	if (!mtk_pcie_valid_device(pcie, bus, devfn))
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	return mtk_pcie_hw_wr_cfg(pcie, bn, devfn, where, size, val);
> +}
> +
> +static struct pci_ops mtk_pcie_ops = {
> +	.read  = mtk_pcie_read_config,
> +	.write = mtk_pcie_write_config,
> +};
> +
> +static void mtk_pcie_configure_rc(struct mtk_pcie_port *port)
> +{
> +	struct mtk_pcie *pcie = port->pcie;
> +	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 */
> +	mtk_pcie_hw_rd_cfg(pcie, 0, (port->index << 3),
> +			   PCIE_FC_CREDIT, 4, &val);
> +	val &= ~PCIE_FC_CREDIT_MASK;
> +	val |= PCIE_FC_CREDIT_VAL(0x806c);
> +	mtk_pcie_hw_wr_cfg(pcie, 0, (port->index << 3),
> +			   PCIE_FC_CREDIT, 4, val);
> +
> +	/* configure RC FTS number to 250 when it leaves L0s */
> +	mtk_pcie_hw_rd_cfg(pcie, 0, (port->index << 3),
> +			   PCIE_FTS_NUM, 4, &val);
> +	val &= ~PCIE_FTS_NUM_MASK;
> +	val |= PCIE_FTS_NUM_L0(0x50);
> +	mtk_pcie_hw_wr_cfg(pcie, 0, (port->index << 3),
> +			   PCIE_FTS_NUM, 4, val);
> +}
> +
> +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);
> +
> +	/*
> +	 * at least 100ms delay because PCIe v2.0 need more time to
> +	 * train from Gen1 to Gen2
> +	 */
> +	msleep(100);
> +}
> +
> +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct mtk_pcie_port *port, *tmp;
> +	int err, linkup = 0;
> +
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		err = clk_prepare_enable(port->sys_ck);
> +		if (err) {
> +			dev_err(dev, "failed to enable port%d clock\n",
> +				port->index);
> +			continue;
> +		}
> +
> +		/* assert RC */
> +		reset_control_assert(port->reset);
> +		/* de-assert RC */
> +		reset_control_deassert(port->reset);
> +
> +		/* power on PHY */
> +		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);
> +			linkup++;
> +			continue;
> +		}
> +
> +		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);
> +	}
> +
> +	return linkup;
> +}
> +
> +static int mtk_pcie_get_port_resource(struct mtk_pcie_port *port,
> +				      struct device_node *node)
> +{
> +	struct device *dev = port->pcie->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct platform_device *plat_dev;
> +	char name[10];
> +	int 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", port->index);
> +		return PTR_ERR(port->base);
> +	}
> +
> +	plat_dev = of_find_device_by_node(node);
> +	if (!plat_dev) {
> +		plat_dev = of_platform_device_create(
> +					node, NULL,
> +					platform_bus_type.dev_root);
> +		if (!plat_dev)
> +			return -EPROBE_DEFER;
> +	}
> +
> +	port->dev = &plat_dev->dev;
> +
> +	port->irq = platform_get_irq(pdev, port->index);
> +	if (!port->irq) {
> +		dev_err(dev, "failed to get irq\n");
> +		return -ENODEV;
> +	}
> +
> +	port->sys_ck = devm_clk_get(port->dev, "sys_ck");
> +	if (IS_ERR(port->sys_ck)) {
> +		dev_err(port->dev, "failed to get port%d clock\n", port->index);
> +		return PTR_ERR(port->sys_ck);
> +	}
> +
> +	port->reset = devm_reset_control_get(port->dev, "pcie-reset");
> +	if (IS_ERR(port->reset)) {
> +		dev_err(port->dev, "failed to get port%d reset control\n",
> +			port->index);
> +		return PTR_ERR(port->reset);
> +	}
> +
> +	snprintf(name, sizeof(name), "pcie-phy%d", port->index);
> +	port->phy = devm_of_phy_get(port->dev, node, name);
> +	if (IS_ERR(port->phy)) {
> +		dev_err(port->dev, "failed to get port%d phy\n", port->index);
> +		return PTR_ERR(port->phy);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_parse_and_add_res(struct mtk_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct device_node *node = dev->of_node, *child;
> +	struct resource_entry *win, *tmp;
> +	struct resource *regs;
> +	resource_size_t iobase;
> +	int err;
> +
> +	/* parse shared resources */
> +	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 get PCIe base\n");
> +		return PTR_ERR(pcie->base);
> +	}
> +
> +	pcie->free_ck = devm_clk_get(dev, "free_ck");
> +	if (IS_ERR(pcie->free_ck)) {
> +		dev_err(dev, "failed to get free_ck\n");
> +		return PTR_ERR(pcie->free_ck);
> +	}
> +
> +	err = of_pci_get_host_bridge_resources(node, 0, 0xff, &pcie->resources,
> +					       &iobase);
> +	if (err)
> +		return err;
> +
> +	err = devm_request_pci_bus_resources(dev, &pcie->resources);
> +	if (err)
> +		return err;
> +
> +	resource_list_for_each_entry_safe(win, tmp, &pcie->resources) {
> +		struct resource *res = win->res;
> +
> +		switch (resource_type(res)) {
> +		case IORESOURCE_IO:
> +			err = pci_remap_iospace(res, iobase);
> +			if (err) {
> +				dev_warn(dev, "failed to map resource %pR\n",
> +					 res);
> +				resource_list_destroy_entry(win);
> +			}
> +			break;
> +		}
> +	}
> +
> +	/* parse port resources */
> +	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(pcie->dev, "failed to parse devfn: %d\n", err);

dev_err(dev, ...)

> +			return err;
> +		}
> +
> +		index = PCI_SLOT(err);
> +		if (index < 1) {
> +			dev_err(dev, "invalid port number: %d\n", index);
> +			return -EINVAL;
> +		}
> +
> +		index--;
> +
> +		if (!of_device_is_available(child))
> +			continue;
> +
> +		port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +		if (!port)
> +			return -ENOMEM;
> +
> +		err = of_property_read_u32(child, "num-lanes", &port->lane);
> +		if (err) {
> +			dev_err(dev, "missing num-lanes property\n");
> +			return err;
> +		}
> +
> +		port->index = index;
> +		port->pcie = pcie;
> +
> +		err = mtk_pcie_get_port_resource(port, child);
> +		if (err)
> +			return err;
> +
> +		INIT_LIST_HEAD(&port->list);
> +		list_add_tail(&port->list, &pcie->ports);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * This IP lacks interrupt status register to check or map INTx from
> + * different devices at the same time.
> + */
> +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> +{
> +	struct mtk_pcie *pcie = dev->bus->sysdata;
> +	struct mtk_pcie_port *port;
> +
> +	list_for_each_entry(port, &pcie->ports, list)
> +		if (port->index == slot)
> +			return port->irq;
> +
> +	return -1;
> +}
> +
> +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
> +{
> +	struct pci_bus *bus, *child;
> +
> +	bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
> +				&pcie->resources);
> +	if (!bus) {
> +		dev_err(pcie->dev, "failed to create root bus\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> +		pci_fixup_irqs(pci_common_swizzle, mtk_pcie_map_irq);
> +		pci_bus_size_bridges(bus);
> +		pci_bus_assign_resources(bus);
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);

Do you actually need the functionality of PCI_PROBE_ONLY?  We're
trying to get rid of this, so if you don't need it, please omit it.

If you *do* need it, can you include a note about why?

If you do need it, I don't think PCI_PROBE_ONLY should control
pci_fixup_irqs() or pcie_bus_configure_settings().  I know there is
some other similar code that does this, but I think PCI_PROBE_ONLY
should only influence resource assignment, i.e., BARs and bridge
windows.  I don't want it to influence IRQs or the MPS/MRRS settings
done by pcie_bus_configure_settings() if we can avoid it.

> +	}
> +
> +	pci_bus_add_devices(bus);
> +
> +	return 0;
> +}
> +
> +static int mtk_pcie_probe(struct platform_device *pdev)
> +{
> +	struct mtk_pcie *pcie;
> +	int err;
> +
> +	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pcie->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	/*
> +	 * parse PCI ranges, configuration bus range and
> +	 * request their resources
> +	 */
> +	INIT_LIST_HEAD(&pcie->ports);
> +	INIT_LIST_HEAD(&pcie->resources);
> +
> +	err = mtk_pcie_parse_and_add_res(pcie);
> +	if (err)
> +		goto err_parse;
> +
> +	pm_runtime_enable(pcie->dev);
> +	err = pm_runtime_get_sync(pcie->dev);
> +	if (err)
> +		goto err_pm;
> +
> +	err = clk_prepare_enable(pcie->free_ck);
> +	if (err) {
> +		dev_err(pcie->dev, "failed to enable free_ck\n");
> +		goto err_free_ck;
> +	}
> +
> +	/* power on PCIe ports */
> +	err = mtk_pcie_enable_ports(pcie);
> +	if (!err)
> +		goto err_enable;
> +
> +	/* register PCIe ports */
> +	err = mtk_pcie_register_ports(pcie);
> +	if (err)
> +		goto err_enable;
> +
> +	return 0;
> +
> +err_enable:
> +	clk_disable_unprepare(pcie->free_ck);
> +err_free_ck:
> +	pm_runtime_put_sync(pcie->dev);
> +err_pm:
> +	pm_runtime_disable(pcie->dev);
> +err_parse:
> +	pci_free_resource_list(&pcie->resources);
> +
> +	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,

Per [1], I think you should have ".suppress_bind_attrs = true," here.
Without it, apparently you can easily crash the system by unbinding
the driver, as in [2].

[1] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=65e0527b933a
[2] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?id=073d3dbe9a7c

> +	},
> +};
> +
> +builtin_platform_driver(mtk_pcie_driver);
> +
> +MODULE_DESCRIPTION("Mediatek PCIe host driver for MT7623 SoCs families");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
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: Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

[Ryder Lee]

Hi,

On Mon, 2017-04-24 at 17:02 -0500, Bjorn Helgaas wrote:
> Hi Ryder,
> 
> Looks good, but I have a few questions below.
> 
> On Sun, Apr 23, 2017 at 04:19:02PM +0800, Ryder Lee wrote:
> > Add support for the Mediatek PCIe controller which can be found
> > on MT7623A/N, MT2701 and MT8521p platforms.
> > 
> > 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 | 611 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 623 insertions(+)
> >  create mode 100644 drivers/pci/host/pcie-mediatek.c
> > 
> > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > index f7c1d4d..cf13b5d 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 for MT7623 SoCs families"
> > +	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 A/N
> > +	  series SoCs. There is a one 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..98e84d9
> > --- /dev/null
> > +++ b/drivers/pci/host/pcie-mediatek.c
> > @@ -0,0 +1,611 @@
> > +/*
> > + * PCIe host controller driver for Mediatek MT7623 SoCs families
> > + *
> > + * 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>
> > +#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
> > + * @dev: pointer to root port device
> > + * @base: IO mapped register base
> > + * @list: port list
> > + * @pcie: pointer to PCIe host info
> > + * @reset: pointer to RC reset control
> > + * @regs: port memory region
> > + * @sys_ck: root port clock
> > + * @phy: pointer to phy control block
> > + * @irq: IRQ number
> > + * @lane: lane count
> > + * @index: port index
> > + */
> > +struct mtk_pcie_port {
> > +	struct device *dev;
> > +	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;
> > +	int irq;
> > +	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
> > + * @resources: bus resources
> > + * @ports: pointer to PCIe port information
> > + */
> > +struct mtk_pcie {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	struct clk *free_ck;
> > +	struct list_head resources;
> > +	struct list_head ports;
> > +};
> > +
> > +static inline bool mtk_pcie_link_is_up(struct mtk_pcie_port *port)
> > +{
> > +	return !!(readl_relaxed(port->base + PCIE_LINK_STATUS) &
> > +		  PCIE_PORT_LINKUP);
> > +}
> > +
> > +static bool mtk_pcie_valid_device(struct mtk_pcie *pcie,
> > +				  struct pci_bus *bus, int devfn)
> > +{
> > +	struct mtk_pcie_port *port;
> > +	struct pci_dev *dev;
> > +	struct pci_bus *pbus;
> > +
> > +	/* if there is no link, then there is no device */
> > +	list_for_each_entry(port, &pcie->ports, list) {
> > +		if (bus->number == 0 && port->index == PCI_SLOT(devfn) &&
> > +		    mtk_pcie_link_is_up(port)) {
> > +			return true;
> > +		} else if (bus->number != 0) {
> > +			pbus = bus;
> > +			do {
> > +				dev = pbus->self;
> > +				if (port->index == PCI_SLOT(dev->devfn) &&
> > +				    mtk_pcie_link_is_up(port)) {
> > +					return true;
> > +				}
> > +				pbus = dev->bus;
> > +			} while (dev->bus->number != 0);
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +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 int mtk_pcie_hw_rd_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
> > +			      int where, int size, u32 *val)
> > +{
> > +	writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
> > +	       pcie->base + PCIE_CFG_ADDR);
> > +
> > +	*val = 0;
> > +
> > +	switch (size) {
> > +	case 1:
> > +		*val = readb(pcie->base + PCIE_CFG_DATA + (where & 3));
> > +		break;
> > +	case 2:
> > +		*val = readw(pcie->base + PCIE_CFG_DATA + (where & 2));
> > +		break;
> > +	case 4:
> > +		*val = readl(pcie->base + PCIE_CFG_DATA);
> > +		break;
> > +	}
> > +
> > +	return PCIBIOS_SUCCESSFUL;
> > +}
> > +
> > +static int mtk_pcie_hw_wr_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
> > +			      int where, int size, u32 val)
> > +
> > +{
> > +	writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
> > +	       pcie->base + PCIE_CFG_ADDR);
> > +
> > +	switch (size) {
> > +	case 1:
> > +		writeb(val, pcie->base + PCIE_CFG_DATA + (where & 3));
> > +		break;
> > +	case 2:
> > +		writew(val, pcie->base + PCIE_CFG_DATA + (where & 2));
> > +		break;
> > +	case 4:
> > +		writel(val, pcie->base + PCIE_CFG_DATA);
> > +		break;
> > +	}
> > +
> > +	return PCIBIOS_SUCCESSFUL;
> > +}
> > +
> > +static int mtk_pcie_read_config(struct pci_bus *bus, u32 devfn,
> > +				int where, int size, u32 *val)
> > +{
> > +	struct mtk_pcie *pcie = bus->sysdata;
> > +	u32 bn = bus->number;
> > +
> > +	if (!mtk_pcie_valid_device(pcie, bus, devfn)) {
> > +		*val = 0xffffffff;
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > +	}
> 
> I know there are some other drivers with the *_valid_device() pattern
> in their config accessors, but I don't like it because it's racy.
> It's possible for the link to be up for the test above, then go down
> before the actual config access below.
> 
> Your hardware *should* do something sensible if we try to read config
> space when the link is down, and ideally that would be enough that we
> don't need this "valid_device()" check.
> 
Yup,it's unnecessary, will remove it.

> > +	return mtk_pcie_hw_rd_cfg(pcie, bn, devfn, where, size, val);
> > +}
> > +
> > +static int mtk_pcie_write_config(struct pci_bus *bus, u32 devfn,
> > +				 int where, int size, u32 val)
> > +{
> > +	struct mtk_pcie *pcie = bus->sysdata;
> > +	u32 bn = bus->number;
> > +
> > +	if (!mtk_pcie_valid_device(pcie, bus, devfn))
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > +	return mtk_pcie_hw_wr_cfg(pcie, bn, devfn, where, size, val);
> > +}
> > +
> > +static struct pci_ops mtk_pcie_ops = {
> > +	.read  = mtk_pcie_read_config,
> > +	.write = mtk_pcie_write_config,
> > +};
> > +
> > +static void mtk_pcie_configure_rc(struct mtk_pcie_port *port)
> > +{
> > +	struct mtk_pcie *pcie = port->pcie;
> > +	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 */
> > +	mtk_pcie_hw_rd_cfg(pcie, 0, (port->index << 3),
> > +			   PCIE_FC_CREDIT, 4, &val);
> > +	val &= ~PCIE_FC_CREDIT_MASK;
> > +	val |= PCIE_FC_CREDIT_VAL(0x806c);
> > +	mtk_pcie_hw_wr_cfg(pcie, 0, (port->index << 3),
> > +			   PCIE_FC_CREDIT, 4, val);
> > +
> > +	/* configure RC FTS number to 250 when it leaves L0s */
> > +	mtk_pcie_hw_rd_cfg(pcie, 0, (port->index << 3),
> > +			   PCIE_FTS_NUM, 4, &val);
> > +	val &= ~PCIE_FTS_NUM_MASK;
> > +	val |= PCIE_FTS_NUM_L0(0x50);
> > +	mtk_pcie_hw_wr_cfg(pcie, 0, (port->index << 3),
> > +			   PCIE_FTS_NUM, 4, val);
> > +}
> > +
> > +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);
> > +
> > +	/*
> > +	 * at least 100ms delay because PCIe v2.0 need more time to
> > +	 * train from Gen1 to Gen2
> > +	 */
> > +	msleep(100);
> > +}
> > +
> > +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
> > +{
> > +	struct device *dev = pcie->dev;
> > +	struct mtk_pcie_port *port, *tmp;
> > +	int err, linkup = 0;
> > +
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> > +		err = clk_prepare_enable(port->sys_ck);
> > +		if (err) {
> > +			dev_err(dev, "failed to enable port%d clock\n",
> > +				port->index);
> > +			continue;
> > +		}
> > +
> > +		/* assert RC */
> > +		reset_control_assert(port->reset);
> > +		/* de-assert RC */
> > +		reset_control_deassert(port->reset);
> > +
> > +		/* power on PHY */
> > +		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);
> > +			linkup++;
> > +			continue;
> > +		}
> > +
> > +		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);
> > +	}
> > +
> > +	return linkup;
> > +}
> > +
> > +static int mtk_pcie_get_port_resource(struct mtk_pcie_port *port,
> > +				      struct device_node *node)
> > +{
> > +	struct device *dev = port->pcie->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct platform_device *plat_dev;
> > +	char name[10];
> > +	int 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", port->index);
> > +		return PTR_ERR(port->base);
> > +	}
> > +
> > +	plat_dev = of_find_device_by_node(node);
> > +	if (!plat_dev) {
> > +		plat_dev = of_platform_device_create(
> > +					node, NULL,
> > +					platform_bus_type.dev_root);
> > +		if (!plat_dev)
> > +			return -EPROBE_DEFER;
> > +	}
> > +
> > +	port->dev = &plat_dev->dev;
> > +
> > +	port->irq = platform_get_irq(pdev, port->index);
> > +	if (!port->irq) {
> > +		dev_err(dev, "failed to get irq\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	port->sys_ck = devm_clk_get(port->dev, "sys_ck");
> > +	if (IS_ERR(port->sys_ck)) {
> > +		dev_err(port->dev, "failed to get port%d clock\n", port->index);
> > +		return PTR_ERR(port->sys_ck);
> > +	}
> > +
> > +	port->reset = devm_reset_control_get(port->dev, "pcie-reset");
> > +	if (IS_ERR(port->reset)) {
> > +		dev_err(port->dev, "failed to get port%d reset control\n",
> > +			port->index);
> > +		return PTR_ERR(port->reset);
> > +	}
> > +
> > +	snprintf(name, sizeof(name), "pcie-phy%d", port->index);
> > +	port->phy = devm_of_phy_get(port->dev, node, name);
> > +	if (IS_ERR(port->phy)) {
> > +		dev_err(port->dev, "failed to get port%d phy\n", port->index);
> > +		return PTR_ERR(port->phy);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_parse_and_add_res(struct mtk_pcie *pcie)
> > +{
> > +	struct device *dev = pcie->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct device_node *node = dev->of_node, *child;
> > +	struct resource_entry *win, *tmp;
> > +	struct resource *regs;
> > +	resource_size_t iobase;
> > +	int err;
> > +
> > +	/* parse shared resources */
> > +	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 get PCIe base\n");
> > +		return PTR_ERR(pcie->base);
> > +	}
> > +
> > +	pcie->free_ck = devm_clk_get(dev, "free_ck");
> > +	if (IS_ERR(pcie->free_ck)) {
> > +		dev_err(dev, "failed to get free_ck\n");
> > +		return PTR_ERR(pcie->free_ck);
> > +	}
> > +
> > +	err = of_pci_get_host_bridge_resources(node, 0, 0xff, &pcie->resources,
> > +					       &iobase);
> > +	if (err)
> > +		return err;
> > +
> > +	err = devm_request_pci_bus_resources(dev, &pcie->resources);
> > +	if (err)
> > +		return err;
> > +
> > +	resource_list_for_each_entry_safe(win, tmp, &pcie->resources) {
> > +		struct resource *res = win->res;
> > +
> > +		switch (resource_type(res)) {
> > +		case IORESOURCE_IO:
> > +			err = pci_remap_iospace(res, iobase);
> > +			if (err) {
> > +				dev_warn(dev, "failed to map resource %pR\n",
> > +					 res);
> > +				resource_list_destroy_entry(win);
> > +			}
> > +			break;
> > +		}
> > +	}
> > +
> > +	/* parse port resources */
> > +	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(pcie->dev, "failed to parse devfn: %d\n", err);
> 
> dev_err(dev, ...)

OK.
> > +			return err;
> > +		}
> > +
> > +		index = PCI_SLOT(err);
> > +		if (index < 1) {
> > +			dev_err(dev, "invalid port number: %d\n", index);
> > +			return -EINVAL;
> > +		}
> > +
> > +		index--;
> > +
> > +		if (!of_device_is_available(child))
> > +			continue;
> > +
> > +		port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > +		if (!port)
> > +			return -ENOMEM;
> > +
> > +		err = of_property_read_u32(child, "num-lanes", &port->lane);
> > +		if (err) {
> > +			dev_err(dev, "missing num-lanes property\n");
> > +			return err;
> > +		}
> > +
> > +		port->index = index;
> > +		port->pcie = pcie;
> > +
> > +		err = mtk_pcie_get_port_resource(port, child);
> > +		if (err)
> > +			return err;
> > +
> > +		INIT_LIST_HEAD(&port->list);
> > +		list_add_tail(&port->list, &pcie->ports);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * This IP lacks interrupt status register to check or map INTx from
> > + * different devices at the same time.
> > + */
> > +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> > +{
> > +	struct mtk_pcie *pcie = dev->bus->sysdata;
> > +	struct mtk_pcie_port *port;
> > +
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		if (port->index == slot)
> > +			return port->irq;
> > +
> > +	return -1;
> > +}
> > +
> > +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
> > +{
> > +	struct pci_bus *bus, *child;
> > +
> > +	bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
> > +				&pcie->resources);
> > +	if (!bus) {
> > +		dev_err(pcie->dev, "failed to create root bus\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> > +		pci_fixup_irqs(pci_common_swizzle, mtk_pcie_map_irq);
> > +		pci_bus_size_bridges(bus);
> > +		pci_bus_assign_resources(bus);
> > +
> > +		list_for_each_entry(child, &bus->children, node)
> > +			pcie_bus_configure_settings(child);
> 
> Do you actually need the functionality of PCI_PROBE_ONLY?  We're
> trying to get rid of this, so if you don't need it, please omit it.
> 
> If you *do* need it, can you include a note about why?
> 
> If you do need it, I don't think PCI_PROBE_ONLY should control
> pci_fixup_irqs() or pcie_bus_configure_settings().  I know there is
> some other similar code that does this, but I think PCI_PROBE_ONLY
> should only influence resource assignment, i.e., BARs and bridge
> windows.  I don't want it to influence IRQs or the MPS/MRRS settings
> done by pcie_bus_configure_settings() if we can avoid it.

I will remove it, thanks.
> > +	}
> > +
> > +	pci_bus_add_devices(bus);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_probe(struct platform_device *pdev)
> > +{
> > +	struct mtk_pcie *pcie;
> > +	int err;
> > +
> > +	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> > +	if (!pcie)
> > +		return -ENOMEM;
> > +
> > +	pcie->dev = &pdev->dev;
> > +	platform_set_drvdata(pdev, pcie);
> > +
> > +	/*
> > +	 * parse PCI ranges, configuration bus range and
> > +	 * request their resources
> > +	 */
> > +	INIT_LIST_HEAD(&pcie->ports);
> > +	INIT_LIST_HEAD(&pcie->resources);
> > +
> > +	err = mtk_pcie_parse_and_add_res(pcie);
> > +	if (err)
> > +		goto err_parse;
> > +
> > +	pm_runtime_enable(pcie->dev);
> > +	err = pm_runtime_get_sync(pcie->dev);
> > +	if (err)
> > +		goto err_pm;
> > +
> > +	err = clk_prepare_enable(pcie->free_ck);
> > +	if (err) {
> > +		dev_err(pcie->dev, "failed to enable free_ck\n");
> > +		goto err_free_ck;
> > +	}
> > +
> > +	/* power on PCIe ports */
> > +	err = mtk_pcie_enable_ports(pcie);
> > +	if (!err)
> > +		goto err_enable;
> > +
> > +	/* register PCIe ports */
> > +	err = mtk_pcie_register_ports(pcie);
> > +	if (err)
> > +		goto err_enable;
> > +
> > +	return 0;
> > +
> > +err_enable:
> > +	clk_disable_unprepare(pcie->free_ck);
> > +err_free_ck:
> > +	pm_runtime_put_sync(pcie->dev);
> > +err_pm:
> > +	pm_runtime_disable(pcie->dev);
> > +err_parse:
> > +	pci_free_resource_list(&pcie->resources);
> > +
> > +	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,
> 
> Per [1], I think you should have ".suppress_bind_attrs = true," here.
> Without it, apparently you can easily crash the system by unbinding
> the driver, as in [2].
> 
> [1] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=65e0527b933a
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?id=073d3dbe9a7c
OK!
> > +	},
> > +};
> > +
> > +builtin_platform_driver(mtk_pcie_driver);
> > +
> > +MODULE_DESCRIPTION("Mediatek PCIe host driver for MT7623 SoCs families");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 1.9.1
> > 

Thanks for your review!
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek


--
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 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

[Arnd Bergmann]

On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:

> +static inline bool mtk_pcie_link_is_up(struct mtk_pcie_port *port)
> +{
> +       return !!(readl_relaxed(port->base + PCIE_LINK_STATUS) &
> +                 PCIE_PORT_LINKUP);
> +}

If this is not performance critical, please use the regular readl() instead
of readl_relaxed().

> +static bool mtk_pcie_valid_device(struct mtk_pcie *pcie,
> +                                 struct pci_bus *bus, int devfn)
> +{
> +       struct mtk_pcie_port *port;
> +       struct pci_dev *dev;
> +       struct pci_bus *pbus;
> +
> +       /* if there is no link, then there is no device */
> +       list_for_each_entry(port, &pcie->ports, list) {
> +               if (bus->number == 0 && port->index == PCI_SLOT(devfn) &&
> +                   mtk_pcie_link_is_up(port)) {
> +                       return true;
> +               } else if (bus->number != 0) {
> +                       pbus = bus;
> +                       do {
> +                               dev = pbus->self;
> +                               if (port->index == PCI_SLOT(dev->devfn) &&
> +                                   mtk_pcie_link_is_up(port)) {
> +                                       return true;
> +                               }
> +                               pbus = dev->bus;
> +                       } while (dev->bus->number != 0);
> +               }
> +       }
> +
> +       return false;
> +}




> +static int mtk_pcie_hw_rd_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
> +                             int where, int size, u32 *val)
> +{
> +       writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
> +              pcie->base + PCIE_CFG_ADDR);
> +
> +       *val = 0;
> +
> +       switch (size) {
> +       case 1:
> +               *val = readb(pcie->base + PCIE_CFG_DATA + (where & 3));
> +               break;
> +       case 2:
> +               *val = readw(pcie->base + PCIE_CFG_DATA + (where & 2));
> +               break;
> +       case 4:
> +               *val = readl(pcie->base + PCIE_CFG_DATA);
> +               break;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}

This is a fairly standard set of read/write operations. Can you change
the pci_ops
to use pci_generic_config_read/pci_generic_config_write and an appropriate
map function instead?

> +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
> +{
> +       struct device *dev = pcie->dev;
> +       struct mtk_pcie_port *port, *tmp;
> +       int err, linkup = 0;
> +
> +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +               err = clk_prepare_enable(port->sys_ck);
> +               if (err) {
> +                       dev_err(dev, "failed to enable port%d clock\n",
> +                               port->index);
> +                       continue;
> +               }
> +
> +               /* assert RC */
> +               reset_control_assert(port->reset);
> +               /* de-assert RC */
> +               reset_control_deassert(port->reset);
> +
> +               /* power on PHY */
> +               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);
> +

Similar to the comment I had for the binding, I wonder if it would be
better to keep all the information about the ports in one place and
then just deal with it at the root level.

Alternatively, we could decide to standardize on the properties
you have added to the pcie port node, but then I would handle
them in the pcieport driver rather than in the host bridge driver.

> +/*
> + * This IP lacks interrupt status register to check or map INTx from
> + * different devices at the same time.
> + */
> +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> +{
> +       struct mtk_pcie *pcie = dev->bus->sysdata;
> +       struct mtk_pcie_port *port;
> +
> +       list_for_each_entry(port, &pcie->ports, list)
> +               if (port->index == slot)
> +                       return port->irq;
> +
> +       return -1;
> +}

This looks odd, what is it needed for specifically? It looks like
it's broken for devices behind bridges, and the interrupt mapping
should normally come from the interrupt-map property, without
the need for a driver specific map_irq override.

> +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
> +{
> +       struct pci_bus *bus, *child;
> +
> +       bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
> +                               &pcie->resources);

Can you use the new pci_register_host_bridge() method instead of
pci_scan_root_bus() here?

       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 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

[Ryder Lee]

Hi,

On Tue, 2017-04-25 at 14:38 +0200, Arnd Bergmann wrote:
> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> 
> > +static inline bool mtk_pcie_link_is_up(struct mtk_pcie_port *port)
> > +{
> > +       return !!(readl_relaxed(port->base + PCIE_LINK_STATUS) &
> > +                 PCIE_PORT_LINKUP);
> > +}
> 
> If this is not performance critical, please use the regular readl() instead
> of readl_relaxed().

I will correct it.

> > +static bool mtk_pcie_valid_device(struct mtk_pcie *pcie,
> > +                                 struct pci_bus *bus, int devfn)
> > +{
> > +       struct mtk_pcie_port *port;
> > +       struct pci_dev *dev;
> > +       struct pci_bus *pbus;
> > +
> > +       /* if there is no link, then there is no device */
> > +       list_for_each_entry(port, &pcie->ports, list) {
> > +               if (bus->number == 0 && port->index == PCI_SLOT(devfn) &&
> > +                   mtk_pcie_link_is_up(port)) {
> > +                       return true;
> > +               } else if (bus->number != 0) {
> > +                       pbus = bus;
> > +                       do {
> > +                               dev = pbus->self;
> > +                               if (port->index == PCI_SLOT(dev->devfn) &&
> > +                                   mtk_pcie_link_is_up(port)) {
> > +                                       return true;
> > +                               }
> > +                               pbus = dev->bus;
> > +                       } while (dev->bus->number != 0);
> > +               }
> > +       }
> > +
> > +       return false;
> > +}
> 
> 
> 
> 
> > +static int mtk_pcie_hw_rd_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
> > +                             int where, int size, u32 *val)
> > +{
> > +       writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
> > +              pcie->base + PCIE_CFG_ADDR);
> > +
> > +       *val = 0;
> > +
> > +       switch (size) {
> > +       case 1:
> > +               *val = readb(pcie->base + PCIE_CFG_DATA + (where & 3));
> > +               break;
> > +       case 2:
> > +               *val = readw(pcie->base + PCIE_CFG_DATA + (where & 2));
> > +               break;
> > +       case 4:
> > +               *val = readl(pcie->base + PCIE_CFG_DATA);
> > +               break;
> > +       }
> > +
> > +       return PCIBIOS_SUCCESSFUL;
> > +}
> 
> This is a fairly standard set of read/write operations. Can you change
> the pci_ops
> to use pci_generic_config_read/pci_generic_config_write and an appropriate
> map function instead?

OK I will add a .map_bus() like this:
{ .

  writel(PCIE_CONF_ADDR(where, fun, slot, bus), base + PCIE_CFG_ADDR);

  return base + PCIE_CFG_DATA + (where & 3);

}
> > +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
> > +{
> > +       struct device *dev = pcie->dev;
> > +       struct mtk_pcie_port *port, *tmp;
> > +       int err, linkup = 0;
> > +
> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> > +               err = clk_prepare_enable(port->sys_ck);
> > +               if (err) {
> > +                       dev_err(dev, "failed to enable port%d clock\n",
> > +                               port->index);
> > +                       continue;
> > +               }
> > +
> > +               /* assert RC */
> > +               reset_control_assert(port->reset);
> > +               /* de-assert RC */
> > +               reset_control_deassert(port->reset);
> > +
> > +               /* power on PHY */
> > +               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);
> > +
> 
> Similar to the comment I had for the binding, I wonder if it would be
> better to keep all the information about the ports in one place and
> then just deal with it at the root level.
> 
> Alternatively, we could decide to standardize on the properties
> you have added to the pcie port node, but then I would handle
> them in the pcieport driver rather than in the host bridge driver.

Sorry, I'm not sure what you want me to do here.

I could move all clock operation in root level. But we need to keep the
reset and PHY operation sequence in the loop, In addition, we could
easily free resources if ports link fail.

How about moving this function to mtk_pcie_parse_and_add_res()? 


> > +/*
> > + * This IP lacks interrupt status register to check or map INTx from
> > + * different devices at the same time.
> > + */
> > +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> > +{
> > +       struct mtk_pcie *pcie = dev->bus->sysdata;
> > +       struct mtk_pcie_port *port;
> > +
> > +       list_for_each_entry(port, &pcie->ports, list)
> > +               if (port->index == slot)
> > +                       return port->irq;
> > +
> > +       return -1;
> > +}
> 
> This looks odd, what is it needed for specifically? It looks like
> it's broken for devices behind bridges, and the interrupt mapping
> should normally come from the interrupt-map property, without
> the need for a driver specific map_irq override.

Our hardware just has a GIC for each port and lacks interrupt status for
host driver to distinguish INTx. So I return port IRQ here.


> > +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
> > +{
> > +       struct pci_bus *bus, *child;
> > +
> > +       bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
> > +                               &pcie->resources);
> 
> Can you use the new pci_register_host_bridge() method instead of
> pci_scan_root_bus() here?

May I know what's difference between pci_scan_root_bus() and using
pci_register_host_bridge() directly? What situation should we use it?
It seems that just tegra use this new method currently.

I'm not sure whether I can still use pci_scan_root_bus() here? 

>        ARnd


Thanks for the review!

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

[Arnd Bergmann]

On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> On Tue, 2017-04-25 at 14:38 +0200, Arnd Bergmann wrote:
>> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:

>> > +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
>> > +{
>> > +       struct device *dev = pcie->dev;
>> > +       struct mtk_pcie_port *port, *tmp;
>> > +       int err, linkup = 0;
>> > +
>> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>> > +               err = clk_prepare_enable(port->sys_ck);
>> > +               if (err) {
>> > +                       dev_err(dev, "failed to enable port%d clock\n",
>> > +                               port->index);
>> > +                       continue;
>> > +               }
>> > +
>> > +               /* assert RC */
>> > +               reset_control_assert(port->reset);
>> > +               /* de-assert RC */
>> > +               reset_control_deassert(port->reset);
>> > +
>> > +               /* power on PHY */
>> > +               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);
>> > +
>>
>> Similar to the comment I had for the binding, I wonder if it would be
>> better to keep all the information about the ports in one place and
>> then just deal with it at the root level.
>>
>> Alternatively, we could decide to standardize on the properties
>> you have added to the pcie port node, but then I would handle
>> them in the pcieport driver rather than in the host bridge driver.
>
> Sorry, I'm not sure what you want me to do here.
>
> I could move all clock operation in root level. But we need to keep the
> reset and PHY operation sequence in the loop, In addition, we could
> easily free resources if ports link fail.
>
> How about moving this function to mtk_pcie_parse_and_add_res()?

That could work, please try it out and see if the code gets better or
worse. This may depend on what we end up doing with the DT
properties.

>> > +/*
>> > + * This IP lacks interrupt status register to check or map INTx from
>> > + * different devices at the same time.
>> > + */
>> > +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>> > +{
>> > +       struct mtk_pcie *pcie = dev->bus->sysdata;
>> > +       struct mtk_pcie_port *port;
>> > +
>> > +       list_for_each_entry(port, &pcie->ports, list)
>> > +               if (port->index == slot)
>> > +                       return port->irq;
>> > +
>> > +       return -1;
>> > +}
>>
>> This looks odd, what is it needed for specifically? It looks like
>> it's broken for devices behind bridges, and the interrupt mapping
>> should normally come from the interrupt-map property, without
>> the need for a driver specific map_irq override.
>
> Our hardware just has a GIC for each port and lacks interrupt status for
> host driver to distinguish INTx. So I return port IRQ here.

You should still be able to express this with standard interrupt-map
DT property, without having to resort to your own map_irq
callback handler.

In the interrupt-map-mask, you can ignore the interrupt line
only list the devfn of the root ports for each entry.

>> > +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
>> > +{
>> > +       struct pci_bus *bus, *child;
>> > +
>> > +       bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
>> > +                               &pcie->resources);
>>
>> Can you use the new pci_register_host_bridge() method instead of
>> pci_scan_root_bus() here?
>
> May I know what's difference between pci_scan_root_bus() and using
> pci_register_host_bridge() directly? What situation should we use it?
> It seems that just tegra use this new method currently.

We introduced the new function for tegra for now, in the long run
I would hope we can convert all other drivers to it as well, to make it
easier to add further parameters.

The new function also has a cleaner way of dealing with the memory
allocations, similar to how other subsystems work.

     Arnd

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

[Ryder Lee]

Hi,

On Thu, 2017-04-27 at 20:55 +0200, Arnd Bergmann wrote:
> On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> > On Tue, 2017-04-25 at 14:38 +0200, Arnd Bergmann wrote:
> >> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> 
> >> > +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
> >> > +{
> >> > +       struct device *dev = pcie->dev;
> >> > +       struct mtk_pcie_port *port, *tmp;
> >> > +       int err, linkup = 0;
> >> > +
> >> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> >> > +               err = clk_prepare_enable(port->sys_ck);
> >> > +               if (err) {
> >> > +                       dev_err(dev, "failed to enable port%d clock\n",
> >> > +                               port->index);
> >> > +                       continue;
> >> > +               }
> >> > +
> >> > +               /* assert RC */
> >> > +               reset_control_assert(port->reset);
> >> > +               /* de-assert RC */
> >> > +               reset_control_deassert(port->reset);
> >> > +
> >> > +               /* power on PHY */
> >> > +               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);
> >> > +
> >>
> >> Similar to the comment I had for the binding, I wonder if it would be
> >> better to keep all the information about the ports in one place and
> >> then just deal with it at the root level.
> >>
> >> Alternatively, we could decide to standardize on the properties
> >> you have added to the pcie port node, but then I would handle
> >> them in the pcieport driver rather than in the host bridge driver.
> >
> > Sorry, I'm not sure what you want me to do here.
> >
> > I could move all clock operation in root level. But we need to keep the
> > reset and PHY operation sequence in the loop, In addition, we could
> > easily free resources if ports link fail.
> >
> > How about moving this function to mtk_pcie_parse_and_add_res()?
> 
> That could work, please try it out and see if the code gets better or
> worse. This may depend on what we end up doing with the DT
> properties.

I will try it on next version, and we can continue our discussion on
that series.

> >> > +/*
> >> > + * This IP lacks interrupt status register to check or map INTx from
> >> > + * different devices at the same time.
> >> > + */
> >> > +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> >> > +{
> >> > +       struct mtk_pcie *pcie = dev->bus->sysdata;
> >> > +       struct mtk_pcie_port *port;
> >> > +
> >> > +       list_for_each_entry(port, &pcie->ports, list)
> >> > +               if (port->index == slot)
> >> > +                       return port->irq;
> >> > +
> >> > +       return -1;
> >> > +}
> >>
> >> This looks odd, what is it needed for specifically? It looks like
> >> it's broken for devices behind bridges, and the interrupt mapping
> >> should normally come from the interrupt-map property, without
> >> the need for a driver specific map_irq override.
> >
> > Our hardware just has a GIC for each port and lacks interrupt status for
> > host driver to distinguish INTx. So I return port IRQ here.
> 
> You should still be able to express this with standard interrupt-map
> DT property, without having to resort to your own map_irq
> callback handler.
> 
> In the interrupt-map-mask, you can ignore the interrupt line
> only list the devfn of the root ports for each entry.

Okay, I will fix it.

> >> > +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
> >> > +{
> >> > +       struct pci_bus *bus, *child;
> >> > +
> >> > +       bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
> >> > +                               &pcie->resources);
> >>
> >> Can you use the new pci_register_host_bridge() method instead of
> >> pci_scan_root_bus() here?
> >
> > May I know what's difference between pci_scan_root_bus() and using
> > pci_register_host_bridge() directly? What situation should we use it?
> > It seems that just tegra use this new method currently.
> 
> We introduced the new function for tegra for now, in the long run
> I would hope we can convert all other drivers to it as well, to make it
> easier to add further parameters.
> 
> The new function also has a cleaner way of dealing with the memory
> allocations, similar to how other subsystems work.

Sounds good. I will change to use that.

Thanks!

[PATCH 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>
---
 .../bindings/pci/mediatek,mt7623-pcie.txt          | 153 +++++++++++++++++++++
 1 file changed, 153 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..ee93ba2
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
@@ -0,0 +1,153 @@
+Mediatek MT7623 PCIe controller
+
+Required properties:
+- compatible: Should contain "mediatek,mt7623-pcie".
+- device_type: Must be "pci"
+- reg: Base addresses and lengths of the pcie controller.
+- interrupts: A list of interrupt outputs of the controller.
+- #address-cells: Address representation for root ports (must be 3)
+  - cell 0 specifies the bus and device numbers of the root port:
+    [23:16]: bus number
+    [15:11]: device number
+  - cell 1 denotes the upper 32 address bits and should be 0
+  - cell 2 contains the lower 32 address bits and is used to translate to the
+    CPU address space
+- #size-cells: Size representation for root ports (must be 2)
+- 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
+- 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: Describes the translation of addresses for root ports and standard
+  PCI regions. The entries must be 6 cells each, where the first three cells
+  correspond to the address as described for the #address-cells property
+  above, the fourth cell is the physical CPU address to translate to and the
+  fifth and six cells are as described for the #size-cells property above.
+  - 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 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
+- ranges: Sub-ranges distributed from the PCIe controller node. An empty
+  property is sufficient.
+- 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:
+  - sys_ck
+- 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-reset
+- num-lanes: Number of lanes to use for this port.
+- phys: Must contain an entry for each entry in phy-names.
+- phy-names: Must include an entry for each sub node. Entries are of the form
+  "pcie-phyN": where N ranges from 0 to the value specified for port number.
+  See ../phy/phy-mt7623-pcie.txt for details.
+
+Examples:
+
+SoC dtsi:
+
+	pcie: pcie@1a140000 {
+		compatible = "mediatek,mt7623-pcie";
+		device_type = "pci";
+		reg = <0 0x1a140000 0 0x1000>; /* PCIe shared registers */
+		interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>,
+			     <GIC_SPI 195 IRQ_TYPE_LEVEL_LOW>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		clocks = <&topckgen CLK_TOP_ETHIF_SEL>;
+		clock-names = "free_ck";
+		power-domains = <&scpsys MT2701_POWER_DOMAIN_HIF>;
+
+		bus-range = <0x00 0xff>;
+		ranges = <0x82000000 0 0x1a142000 0 0x1a142000 0 0x1000 /* Por0 registers */
+			  0x82000000 0 0x1a143000 0 0x1a143000 0 0x1000 /* Por1 registers */
+			  0x82000000 0 0x1a144000 0 0x1a144000 0 0x1000 /* Por2 registers */
+			  0x81000000 0 0x1a160000 0 0x1a160000 0 0x00010000 /* I/O space */
+			  0x83000000 0 0x60000000 0 0x60000000 0 0x10000000>;	/* memory space */
+		status = "disabled";
+
+		pcie@1,0 {
+			device_type = "pci";
+			assigned-addresses = <0x82000800 0 0x1a142000 0 0x1000>;
+			reg = <0x0800 0 0 0 0>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges;
+			status = "disabled";
+
+			clocks = <&hifsys CLK_HIFSYS_PCIE0>;
+			clock-names = "sys_ck";
+			resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>;
+			reset-names = "pcie-reset";
+
+			num-lanes = <1>;
+			phys = <&pcie0_phy>;
+			phy-names = "pcie-phy0";
+		};
+
+		pcie@2,0 {
+			device_type = "pci";
+			assigned-addresses = <0x82001000 0 0x1a143000 0 0x1000>;
+			reg = <0x1000 0 0 0 0>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges;
+			status = "disabled";
+
+			clocks = <&hifsys CLK_HIFSYS_PCIE1>;
+			clock-names = "sys_ck";
+			resets = <&hifsys MT2701_HIFSYS_PCIE1_RST>;
+			reset-names = "pcie-reset";
+
+			num-lanes = <1>;
+			phys = <&pcie1_phy>;
+			phy-names = "pcie-phy1";
+		};
+
+		pcie@3,0 {
+			device_type = "pci";
+			assigned-addresses = <0x82001800 0 0x1a144000 0 0x1000>;
+			reg = <0x1800 0 0 0 0>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges;
+			status = "disabled";
+
+			clocks = <&hifsys CLK_HIFSYS_PCIE2>;
+			clock-names = "sys_ck";
+			resets = <&hifsys MT2701_HIFSYS_PCIE2_RST>;
+			reset-names = "pcie-reset";
+
+			num-lanes = <1>;
+			phys = <&pcie2_phy>;
+			phy-names = "pcie-phy2";
+		};
+	};
+
+Board dts:
+
+	&pcie {
+		status = "okay";
+
+		pcie@1,0 {
+			status = "okay";
+		};
+	};
\ No newline at end of file
-- 
1.9.1

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

[Arnd Bergmann]

On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> Add documentation for PCIe host driver available in MT7623
> series SoCs.
>
> Signed-off-by: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  .../bindings/pci/mediatek,mt7623-pcie.txt          | 153 +++++++++++++++++++++
>  1 file changed, 153 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..ee93ba2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> @@ -0,0 +1,153 @@
> +Mediatek MT7623 PCIe controller
> +
> +Required properties:
> +- compatible: Should contain "mediatek,mt7623-pcie".

Did mediatek license the IP block from someone else or was it
developed in-house? Is there a name and/or version identifier
for the block itself other than identifying it as the one in mt7623?

> +- device_type: Must be "pci"
> +- reg: Base addresses and lengths of the pcie controller.
> +- interrupts: A list of interrupt outputs of the controller.

Please be more specific about what each interrupt is for, and how
many there are.

> +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
> +- ranges: Sub-ranges distributed from the PCIe controller node. An empty
> +  property is sufficient.
> +- 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:
> +  - sys_ck
> +- resets: Must contain an entry for each entry in reset-names.
> +  See ../reset/reset.txt for details.

This seems odd: you have a device that is simply identified as "pci"
without any more specific ID, but you require additional properties
(clocks, reset, ...) that are not part of the standard PCI binding.

Can you clarify how the port devices related to the root device in
this hardware design?

Have you considered moving the nonstandard properties into the host
bridge node and having that device deal with setting up the links
to the other drivers? That way we could use the regular pcie
port driver for the children.

> +- reset-names: Must include the following entries:
> +  - pcie-reset
> +- num-lanes: Number of lanes to use for this port.
> +- phys: Must contain an entry for each entry in phy-names.
> +- phy-names: Must include an entry for each sub node. Entries are of the form
> +  "pcie-phyN": where N ranges from 0 to the value specified for port number.
> +  See ../phy/phy-mt7623-pcie.txt for details.

I think the name should not include the number of the port but rather
be always the same here.

      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: [SPAM]Re: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

Hi

On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote:
> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> > Add documentation for PCIe host driver available in MT7623
> > series SoCs.
> >
> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> >  .../bindings/pci/mediatek,mt7623-pcie.txt          | 153 +++++++++++++++++++++
> >  1 file changed, 153 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..ee93ba2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> > @@ -0,0 +1,153 @@
> > +Mediatek MT7623 PCIe controller
> > +
> > +Required properties:
> > +- compatible: Should contain "mediatek,mt7623-pcie".
> 
> Did mediatek license the IP block from someone else or was it
> developed in-house? Is there a name and/or version identifier
> for the block itself other than identifying it as the one in mt7623?

Originally, it license from synopsys. Our designer add a wrapper to hide
the DBI detail so that we cannot use them directly. Perhaps I can call
it "mediatek,gen2v1-pcie", because we have a plan to upstream a in-house
Gen2 IP in the future.

> > +- device_type: Must be "pci"
> > +- reg: Base addresses and lengths of the pcie controller.
> > +- interrupts: A list of interrupt outputs of the controller.
> 
> Please be more specific about what each interrupt is for, and how
> many there are.

OK.

> > +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
> > +- ranges: Sub-ranges distributed from the PCIe controller node. An empty
> > +  property is sufficient.
> > +- 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:
> > +  - sys_ck
> > +- resets: Must contain an entry for each entry in reset-names.
> > +  See ../reset/reset.txt for details.
> 
> This seems odd: you have a device that is simply identified as "pci"
> without any more specific ID, but you require additional properties
> (clocks, reset, ...) that are not part of the standard PCI binding.
> 
> Can you clarify how the port devices related to the root device in
> this hardware design?

I will write clarify like this:

PCIe subsys includes one Host/PCI bridge and 3 PCIe MAC port. There
are 3 bus master for data access and 1 slave for configuration and
status register access. Each port has PIPE interface to PHY and

> Have you considered moving the nonstandard properties into the host
> bridge node and having that device deal with setting up the links
> to the other drivers? That way we could use the regular pcie
> port driver for the children.
> 

OK, but I still want to use port->reset to catch reset properties in
driver.
 
> > +- reset-names: Must include the following entries:
> > +  - pcie-reset
> > +- num-lanes: Number of lanes to use for this port.
> > +- phys: Must contain an entry for each entry in phy-names.
> > +- phy-names: Must include an entry for each sub node. Entries are of the form
> > +  "pcie-phyN": where N ranges from 0 to the value specified for port number.
> > +  See ../phy/phy-mt7623-pcie.txt for details.
> 
> I think the name should not include the number of the port but rather
> be always the same here.
> 

Hmm, I think it's better to keep the name here. It's more readable for
user to understand the relationship between port0 and phy0.

>       Arnd
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Arnd Bergmann]

On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> Hi
>
> On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote:
>> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
>> > Add documentation for PCIe host driver available in MT7623
>> > series SoCs.
>> >
>> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
>> > ---
>> >  .../bindings/pci/mediatek,mt7623-pcie.txt          | 153 +++++++++++++++++++++
>> >  1 file changed, 153 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..ee93ba2
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
>> > @@ -0,0 +1,153 @@
>> > +Mediatek MT7623 PCIe controller
>> > +
>> > +Required properties:
>> > +- compatible: Should contain "mediatek,mt7623-pcie".
>>
>> Did mediatek license the IP block from someone else or was it
>> developed in-house? Is there a name and/or version identifier
>> for the block itself other than identifying it as the one in mt7623?
>
> Originally, it license from synopsys. Our designer add a wrapper to hide
> the DBI detail so that we cannot use them directly. Perhaps I can call
> it "mediatek,gen2v1-pcie", because we have a plan to upstream a in-house
> Gen2 IP in the future.

Ok, so this is the same hardware that drivers/pci/dwc/ handles, but
it needs a separate driver because the wrapper that was added uses
a completely different register layout, right?

Are any of the registers the same at all, e.g. for MSI handling?

>> > +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
>> > +- ranges: Sub-ranges distributed from the PCIe controller node. An empty
>> > +  property is sufficient.
>> > +- 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:
>> > +  - sys_ck
>> > +- resets: Must contain an entry for each entry in reset-names.
>> > +  See ../reset/reset.txt for details.
>>
>> This seems odd: you have a device that is simply identified as "pci"
>> without any more specific ID, but you require additional properties
>> (clocks, reset, ...) that are not part of the standard PCI binding.
>>
>> Can you clarify how the port devices related to the root device in
>> this hardware design?
>
> I will write clarify like this:
>
> PCIe subsys includes one Host/PCI bridge and 3 PCIe MAC port. There
> are 3 bus master for data access and 1 slave for configuration and
> status register access. Each port has PIPE interface to PHY and

If I understand this right, then each of the ports in your hardware
is what we normally drive using the drivers/pci/dwc/ driver framework,
but your implementation actually made it more PCI standard compliant
by implementing the normal PCIe host bridge registers for all ports
combined, something that most others don't.

>> Have you considered moving the nonstandard properties into the host
>> bridge node and having that device deal with setting up the links
>> to the other drivers? That way we could use the regular pcie
>> port driver for the children.
>>
>
> OK, but I still want to use port->reset to catch reset properties in
> driver.

Do you mean in drivers/pci/pcie/portdrv_pci.c? I see that it
has a function called pcie_portdrv_slot_reset(), but I don't see
how that relates to your reset line at the moment. Is this
something you have submitted in a different series?

Or do you mean in this host driver? The problem I see with
that approach is that the port device is owned by portdrv_pci,
so the host bridge driver should not look at the properties of
the port.

>> > +- reset-names: Must include the following entries:
>> > +  - pcie-reset
>> > +- num-lanes: Number of lanes to use for this port.
>> > +- phys: Must contain an entry for each entry in phy-names.
>> > +- phy-names: Must include an entry for each sub node. Entries are of the form
>> > +  "pcie-phyN": where N ranges from 0 to the value specified for port number.
>> > +  See ../phy/phy-mt7623-pcie.txt for details.
>>
>> I think the name should not include the number of the port but rather
>> be always the same here.
>>
>
> Hmm, I think it's better to keep the name here. It's more readable for
> user to understand the relationship between port0 and phy0.

No, I would argue that it's confusing for the reader because it
is different from how most other DT bindings work: In each device
node, you tend to have a set of properties with well-known names
that are documented. When your reference is called "pcie-phy1"
in one node and "pcie-phy2", I would interpret that as both ports
having two phys each, but only one of them being used.

       Arnd

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

[Ryder Lee]

On Thu, 2017-04-27 at 21:06 +0200, Arnd Bergmann wrote:
> On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > Hi
> >
> > On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote:
> >> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >> > Add documentation for PCIe host driver available in MT7623
> >> > series SoCs.
> >> >
> >> > Signed-off-by: Ryder Lee <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> >> > ---
> >> >  .../bindings/pci/mediatek,mt7623-pcie.txt          | 153 +++++++++++++++++++++
> >> >  1 file changed, 153 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..ee93ba2
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt
> >> > @@ -0,0 +1,153 @@
> >> > +Mediatek MT7623 PCIe controller
> >> > +
> >> > +Required properties:
> >> > +- compatible: Should contain "mediatek,mt7623-pcie".
> >>
> >> Did mediatek license the IP block from someone else or was it
> >> developed in-house? Is there a name and/or version identifier
> >> for the block itself other than identifying it as the one in mt7623?
> >
> > Originally, it license from synopsys. Our designer add a wrapper to hide
> > the DBI detail so that we cannot use them directly. Perhaps I can call
> > it "mediatek,gen2v1-pcie", because we have a plan to upstream a in-house
> > Gen2 IP in the future.
> 
> Ok, so this is the same hardware that drivers/pci/dwc/ handles, but
> it needs a separate driver because the wrapper that was added uses
> a completely different register layout, right?

Yes, that's what I mean. At first, I really want to base on
drivers/pci/dwc/ to implement this driver. Eventually I found it hard to
go on, like what I said before.

> Are any of the registers the same at all, e.g. for MSI handling?

No, It doesn't support MSI. All I can do is using the registers that designer provide
to me. The others are inviable for software. So I treat it as different hardware.
Furthermore, we hope that we can put all mediatek drivers together
regardless of in-house IP or lincense IP

We have no particular IP name but just use chip name to call it. So I
will temporarily use "mediatek,gen2v1-pcie" in patch v1.

> >> > +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
> >> > +- ranges: Sub-ranges distributed from the PCIe controller node. An empty
> >> > +  property is sufficient.
> >> > +- 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:
> >> > +  - sys_ck
> >> > +- resets: Must contain an entry for each entry in reset-names.
> >> > +  See ../reset/reset.txt for details.
> >>
> >> This seems odd: you have a device that is simply identified as "pci"
> >> without any more specific ID, but you require additional properties
> >> (clocks, reset, ...) that are not part of the standard PCI binding.
> >>
> >> Can you clarify how the port devices related to the root device in
> >> this hardware design?
> >
> > I will write clarify like this:
> >
> > PCIe subsys includes one Host/PCI bridge and 3 PCIe MAC port. There
> > are 3 bus master for data access and 1 slave for configuration and
> > status register access. Each port has PIPE interface to PHY and
> 
> If I understand this right, then each of the ports in your hardware
> is what we normally drive using the drivers/pci/dwc/ driver framework,
> but your implementation actually made it more PCI standard compliant
> by implementing the normal PCIe host bridge registers for all ports
> combined, something that most others don't.

In my view, it's correct to implement our driver in this way. But I
don't really understand the details about other platforms. 

> >> Have you considered moving the nonstandard properties into the host
> >> bridge node and having that device deal with setting up the links
> >> to the other drivers? That way we could use the regular pcie
> >> port driver for the children.
> >>
> >
> > OK, but I still want to use port->reset to catch reset properties in
> > driver.
> 
> Do you mean in drivers/pci/pcie/portdrv_pci.c? I see that it
> has a function called pcie_portdrv_slot_reset(), but I don't see
> how that relates to your reset line at the moment. Is this
> something you have submitted in a different series?
> 
> Or do you mean in this host driver? The problem I see with
> that approach is that the port device is owned by portdrv_pci,
> so the host bridge driver should not look at the properties of
> the port.

hifsys: syscon@1a000000 {
		compatible = "mediatek,mt7623-hifsys", "syscon";
		reg = <0 0x1a000000 0 0x1000>;
		#clock-cells = <1>;
		#reset-cells = <1>;
	};

We have a reset controller(hifsys) in our platform. We can just use
devm_reset_control_get() to catch it in the host driver to control port reset.

After much consideration, I will move all nonstandard properties to root
node, let child node cleanable.

> >> > +- reset-names: Must include the following entries:
> >> > +  - pcie-reset
> >> > +- num-lanes: Number of lanes to use for this port.
> >> > +- phys: Must contain an entry for each entry in phy-names.
> >> > +- phy-names: Must include an entry for each sub node. Entries are of the form
> >> > +  "pcie-phyN": where N ranges from 0 to the value specified for port number.
> >> > +  See ../phy/phy-mt7623-pcie.txt for details.
> >>
> >> I think the name should not include the number of the port but rather
> >> be always the same here.
> >>
> >
> > Hmm, I think it's better to keep the name here. It's more readable for
> > user to understand the relationship between port0 and phy0.
> 
> No, I would argue that it's confusing for the reader because it
> is different from how most other DT bindings work: In each device
> node, you tend to have a set of properties with well-known names
> that are documented. When your reference is called "pcie-phy1"
> in one node and "pcie-phy2", I would interpret that as both ports
> having two phys each, but only one of them being used.

Okay I will write it more clearly

- 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.

--
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 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Arnd Bergmann]

On Fri, Apr 28, 2017 at 4:46 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> On Thu, 2017-04-27 at 21:06 +0200, Arnd Bergmann wrote:
>> On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
>> > On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote:
>> >> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
>> Are any of the registers the same at all, e.g. for MSI handling?
>
> No, It doesn't support MSI. All I can do is using the registers that designer provide
> to me. The others are inviable for software. So I treat it as different hardware.
> Furthermore, we hope that we can put all mediatek drivers together
> regardless of in-house IP or lincense IP
>
> We have no particular IP name but just use chip name to call it. So I
> will temporarily use "mediatek,gen2v1-pcie" in patch v1.

I think using the chip name as in the first version of your patch name is
better then, in particular since the 'gen2v1' would not be an actual version
number but just say which variant got merged into mainline first.

A related question would be on how general we want the binding to be.
Your binding text starts out by describing that there are three root ports
and what their capabilities are.

If you think there might be other (existing or future) chips that use the
same binding and driver, then being a little more abstract could help
in the long run.

       Arnd

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

[Ryder Lee]

Hi Arnd,

> 2017-04-28 19:41 GMT+08:00 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>:
> 
>         On Fri, Apr 28, 2017 at 4:46 AM, Ryder Lee
>         <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>         > On Thu, 2017-04-27 at 21:06 +0200, Arnd Bergmann wrote:
>         >> On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee
>         <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>         >> > On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote:
>         >> >> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee
>         <ryder.lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>         >> Are any of the registers the same at all, e.g. for MSI
>         handling?
>         >
>         > No, It doesn't support MSI. All I can do is using the
>         registers that designer provide to me. The others are inviable
>         for software. So I treat it as different hardware.
>         Furthermore, we hope that we can put all mediatek drivers
>         together regardless of in-house IP or lincense IP
>         >
>         > We have no particular IP name but just use chip name to call
>         it. So I will temporarily use "mediatek,gen2v1-pcie" in patch
>         v1.
>         
>         I think using the chip name as in the first version of your
>         patch name is better then, in particular since the 'gen2v1'
>         would not be an actual version number but just say which
>         variant got merged into mainline first.

Okay, i will correct it.

>         A related question would be on how general we want the binding
>         to be.
>         Your binding text starts out by describing that there are
>         three root ports and what their capabilities are.
>         
>         If you think there might be other (existing or future) chips
>         that use the same binding and driver, then being a little more
>         abstract could help in the long run.

Thanks for reminding me. If we decide to use the same driver in the
future, we will have a internal discussion about it.

Ryder.





--
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