Hi, Bjorn,
	Thanks very much for your reviews.
The mis-spells will be fixed in next version.


On Thu, 2017-08-03 at 17:42 -0500, Bjorn Helgaas wrote:


......

quoted

+}
+
+static struct mtk_pcie_port *mtk_pcie_find_port(struct mtk_pcie *pcie,
+						struct pci_bus *bus, int devfn)
+{
+	struct pci_dev *dev;
+	struct pci_bus *pbus;
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		if (bus->number == 0 && port->index == PCI_SLOT(devfn)) {
+			return port;
+		} else if (bus->number != 0) {
+			pbus = bus;
+			do {
+				dev = pbus->self;
+				if (port->index == PCI_SLOT(dev->devfn))
+					return port;
+				pbus = dev->bus;
+			} while (dev->bus->number != 0);
+		}
+	}
+
+	return NULL;

You should be able to use sysdata to avoid searching the list.
See drivers/pci/host/pci-aardvark.c, for example.

I could put the mtk_pcie * in sysdata, but still need to searching the
list to get the mtk_pcie_port *, how about:

	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
		if (port->index == PCI_SLOT(devfn))
			return port;
	}

quoted

+}
+
+static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				int where, int size, u32 *val)
+{
+	struct mtk_pcie_port *port;
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct mtk_pcie *pcie = pci_host_bridge_priv(host);

Sysdata should make this very simple; see advk_pcie_rd_conf().

thanks.

quoted

+	u32 bn = bus->number;
+	int ret;
+
+	port = mtk_pcie_find_port(pcie, bus, devfn);
+	if (!port) {
+		*val = ~0;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	ret = mtk_pcie_hw_rd_cfg(port, bn, devfn, where, size, val);
+	if (ret)
+		*val = ~0;
+
+	return ret;
+}
+
+static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 val)
+{
+	u32 bn = bus->number;
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct mtk_pcie *pcie = pci_host_bridge_priv(host);
+	struct mtk_pcie_port *port;
+
+	port = mtk_pcie_find_port(pcie, bus, devfn);
+	if (!port)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return mtk_pcie_hw_wr_cfg(port, bn, devfn, where, size, val);
+}
+
+static struct pci_ops mtk_pcie_ops_v2 = {
+	.read  = mtk_pcie_config_read,
+	.write = mtk_pcie_config_write,
+};
+
+static int mtk_pcie_startup_ports_v2(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct resource *mem = &pcie->mem;
+	u32 val;
+	size_t size;
+	int err;
+
+	/* mt7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
+	if (pcie->base) {
+		val = readl(pcie->base + PCIE_SYS_CFG_V2);
+		val |= PCIE_CSR_LTSSM_EN(port->index) |
+		       PCIE_CSR_ASPM_L1_EN(port->index);
+		writel(val, pcie->base + PCIE_SYS_CFG_V2);
+	}
+
+	/* Assert all reset signals */
+	writel(0, port->base + PCIE_RST_CTRL);
+
+	/*
+	 * Enable rc internal reset.
+	 * The reset will work when the link is from link up to link down.

?  That sentence doesn't parse for me.

What about:

	/*
	 * Enable PCIe link down reset, if link status changed from link up to
	 * link down, this will reset MAC control registers and configuration
	 * space.
	 */

quoted

+	 */
+	writel(PCI_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+
+	/* De-assert phy, pe, pipe, mac and configuration reset	*/
+	val = readl(port->base + PCIE_RST_CTRL);
+	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
+	       PCIE_MAC_SRSTB | PCIE_CRSTB;
+	writel(val, port->base + PCIE_RST_CTRL);
+
+	/* PCIe v2.0 need at least 100ms delay to train from Gen1 to Gen2 */
+	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
+				 !!(val & PCIE_PORT_LINKUP_V2), 20,
+				 100 * USEC_PER_MSEC);
+	if (err)
+		return -ETIMEDOUT;
+
+	/* Set INTx mask */
+	val = readl(port->base + PCIE_INT_MASK);
+	val &= ~INTX_MASK;
+	writel(val, port->base + PCIE_INT_MASK);
+
+	/* Set AHB to PCIe translation windows */
+	size = mem->end - mem->start;
+	val = AHB2PCIE_BASEL(mem->start) | AHB2PCIE_SIZE(fls(size));
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
+
+	val = AHB2PCIE_BASEH(mem->start);
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
+
+	/* Set PCIe to axi translation memory space.*/

s/axi/AXI/

quoted

+	val = fls(0xffffffff) | WIN_ENABLE;
+	writel(val, port->base + PCIE_AXI_WINDOW0);
+
+	return 0;
+}
+
+static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+			     irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+	.map = mtk_pcie_intx_map,
+};
+
+static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
+				    struct device_node *node)
+{
+	struct device *dev = port->pcie->dev;
+	struct device_node *pcie_intc_node;
+
+	/* Setup INTx */
+	pcie_intc_node = of_get_next_child(node, NULL);
+	if (!pcie_intc_node) {
+		dev_err(dev, "No PCIe Intc node found\n");
+		return PTR_ERR(pcie_intc_node);
+	}
+
+	port->irq_domain = irq_domain_add_linear(pcie_intc_node, INTX_NUM,
+						 &intx_domain_ops, port);

I think there's an issue here with a 4-element IRQ domain and the
hwirq numbers 1-4 from the of_irq_parse_and_map_pci() path, so INTD
may not work correctly.

See
http://lkml.kernel.org/r/20170801212931.GA26498-1RhO1Y9PlrlHTL0Zs8A6p5iNqAH0jzoTYJqu5kTmcBRl57MIdRCFDg@public.gmane.org
and related discussion.

Sorry, I did not get this,
I do some test with an intel E350T4 PCIe NICs, it's a x1 lane
multi-function device.
What I got from the log is below:
->of_irq_parse_and_map_pci
	->of_irq_parse_pci
		->irq_create_of_mapping
			->irq_create_fwspec_mapping
				->irq_domain_translate
				which will go through
				d->ops->translate #the hwirq really start from 0

And I tested every NIC port of the Intel E350T4 with tftp transfer data,
seems all are OK with this code.

What I got from the proc is as below:
cat /proc/interrupts
           CPU0       CPU1       CPU2       
  1:          0          0          0     GICv2  25 Level     vgic
  3:       5042        224        206     GICv2  30 Level     arch_timer
  4:          0          0          0     GICv2  27 Level     kvm guest
timer
  6:        201          0          0  MT_SYSIRQ  91 Level     ttyS0
  7:         57          0          0  MT_SYSIRQ 115 Level     mtk-pcie
  8:          0          0          0  MT_SYSIRQ 117 Level     mtk-pcie
  9:          9          0          0     dummy   0 Edge      eth0
 10:         40          0          0     dummy   1 Edge      eth1
 11:          5          0          0     dummy   2 Edge      eth2
 12:          3          0          0     dummy   3 Edge      eth3
IPI0:       314        507       1164       Rescheduling interrupts

or did I missed something?

quoted

+	if (!port->irq_domain) {
+		dev_err(dev, "Failed to get INTx IRQ domain\n");
+		return PTR_ERR(port->irq_domain);
+	}
+
+	return 0;
+}
+
+static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
+{
+	struct mtk_pcie_port *port = (struct mtk_pcie_port *)data;
+	struct device *dev = port->pcie->dev;
+	unsigned long status;
+	u32 virq;
+	u32 bit = INTX_SHIFT;
+
+	while ((status = readl(port->base + PCIE_INT_STATUS)) & INTX_MASK) {
+		for_each_set_bit_from(bit, &status, INTX_NUM + INTX_SHIFT) {
+			/* Clear the INTx */
+			writel(1 << bit, port->base + PCIE_INT_STATUS);
+			virq = irq_find_mapping(port->irq_domain,
+						bit - INTX_SHIFT);
+			if (virq)
+				generic_handle_irq(virq);
+			else
+				dev_err(dev, "unexpected IRQ, INT%d\n",
+					bit - INTX_SHIFT);

PCI INTx are conventionally INTA, INTB, INTC, INTD (not INT1, INT2,
etc).

quoted

+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
+			      struct device_node *node)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct device *dev = pcie->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	int err, irq;
+
+	irq = platform_get_irq(pdev, port->index);
+	err = devm_request_irq(dev, irq, mtk_pcie_intr_handler,
+			       IRQF_SHARED, "mtk-pcie", port);
+	if (err) {
+		dev_err(dev, "unable to request irq %d\n", irq);

s/irq/IRQ/

quoted

+		return err;
+	}
+
+	err = mtk_pcie_init_irq_domain(port, node);
+	if (err) {
+		dev_err(dev, "failed to init pcie lagecy irq domain\n");

s/lagecy/legacy/
s/irq/IRQ/
s/pcie/PCIe/

quoted

+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
 				      unsigned int devfn, int where)
 {

@@ -249,13 +625,49 @@ static void mtk_pcie_enable_ports(struct mtk_pcie_port *port)
 
 	err = clk_prepare_enable(port->sys_ck);
 	if (err) {
-		dev_err(dev, "failed to enable port%d clock\n", port->index);
+		dev_err(dev, "failed to enable sys_ck%d\n", port->index);
 		goto err_sys_clk;
 	}
 
+	err = clk_prepare_enable(port->ahb_ck);
+	if (err) {
+		dev_err(dev, "failed to enable ahb_ck%d\n", port->index);
+		goto err_ahb_clk;
+	}
+
+	err = clk_prepare_enable(port->aux_ck);
+	if (err) {
+		dev_err(dev, "failed to enable aux_ck%d\n", port->index);
+		goto err_aux_clk;
+	}
+
+	err = clk_prepare_enable(port->axi_ck);
+	if (err) {
+		dev_err(dev, "failed to enable axi_ck%d\n", port->index);
+		goto err_axi_clk;
+	}
+
+	err = clk_prepare_enable(port->obff_ck);
+	if (err) {
+		dev_err(dev, "failed to enable obff_ck%d\n", port->index);
+		goto err_obff_clk;
+	}
+
+	err = clk_prepare_enable(port->pipe_ck);
+	if (err) {
+		dev_err(dev, "failed to enable pipe_ck%d\n", port->index);
+		goto err_pipe_clk;
+	}
+
 	reset_control_assert(port->reset);
 	reset_control_deassert(port->reset);
 
+	err = phy_init(port->phy);
+	if (err) {
+		dev_err(dev, "failed to initialize port%d phy\n", port->index);
+		goto err_phy_init;
+	}
+
 	err = phy_power_on(port->phy);
 	if (err) {
 		dev_err(dev, "failed to power on port%d phy\n", port->index);

@@ -269,6 +681,18 @@ static void mtk_pcie_enable_ports(struct mtk_pcie_port *port)
 
 	phy_power_off(port->phy);
 err_phy_on:
+	phy_exit(port->phy);
+err_phy_init:
+	clk_disable_unprepare(port->pipe_ck);
+err_pipe_clk:
+	clk_disable_unprepare(port->obff_ck);
+err_obff_clk:
+	clk_disable_unprepare(port->axi_ck);
+err_axi_clk:
+	clk_disable_unprepare(port->aux_ck);
+err_aux_clk:
+	clk_disable_unprepare(port->ahb_ck);
+err_ahb_clk:
 	clk_disable_unprepare(port->sys_ck);
 err_sys_clk:
 	mtk_pcie_port_free(port);

@@ -306,10 +730,56 @@ static int mtk_pcie_parse_ports(struct mtk_pcie *pcie,
 	snprintf(name, sizeof(name), "sys_ck%d", index);
 	port->sys_ck = devm_clk_get(dev, name);
 	if (IS_ERR(port->sys_ck)) {
-		dev_err(dev, "failed to get port%d clock\n", index);
+		dev_err(dev, "failed to get sys_ck%d\n", index);
 		return PTR_ERR(port->sys_ck);
 	}
 
+	/* sys_ck might be divided into the following parts in some chips */
+	snprintf(name, sizeof(name), "ahb_ck%d", index);
+	port->ahb_ck = devm_clk_get(dev, name);
+	if (IS_ERR(port->ahb_ck)) {
+		if (PTR_ERR(port->ahb_ck) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		port->ahb_ck = NULL;
+	}
+
+	snprintf(name, sizeof(name), "axi_ck%d", index);
+	port->axi_ck = devm_clk_get(dev, name);
+	if (IS_ERR(port->axi_ck)) {
+		if (PTR_ERR(port->axi_ck) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		port->axi_ck = NULL;
+	}
+
+	snprintf(name, sizeof(name), "aux_ck%d", index);
+	port->aux_ck = devm_clk_get(dev, name);
+	if (IS_ERR(port->aux_ck)) {
+		if (PTR_ERR(port->aux_ck) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		port->aux_ck = NULL;
+	}
+
+	snprintf(name, sizeof(name), "obff_ck%d", index);
+	port->obff_ck = devm_clk_get(dev, name);
+	if (IS_ERR(port->obff_ck)) {
+		if (PTR_ERR(port->obff_ck) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		port->obff_ck = NULL;
+	}
+
+	snprintf(name, sizeof(name), "pipe_ck%d", index);
+	port->pipe_ck = devm_clk_get(dev, name);
+	if (IS_ERR(port->pipe_ck)) {
+		if (PTR_ERR(port->pipe_ck) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		port->pipe_ck = NULL;
+	}
+
 	snprintf(name, sizeof(name), "pcie-rst%d", index);
 	port->reset = devm_reset_control_get_optional(dev, name);
 	if (PTR_ERR(port->reset) == -EPROBE_DEFER)

@@ -324,6 +794,12 @@ static int mtk_pcie_parse_ports(struct mtk_pcie *pcie,
 	port->index = index;
 	port->pcie = pcie;
 
+	if (pcie->soc->setup_irq) {
+		err = pcie->soc->setup_irq(port, node);
+		if (err)
+			return err;
+	}
+
 	INIT_LIST_HEAD(&port->list);
 	list_add_tail(&port->list, &pcie->ports);
 

@@ -553,9 +1029,17 @@ static struct mtk_pcie_soc mtk_pcie_soc_v1 = {
 	.startup = mtk_pcie_startup_ports,
 };
 
+static struct mtk_pcie_soc mtk_pcie_soc_v2 = {
+	.ops = &mtk_pcie_ops_v2,
+	.startup = mtk_pcie_startup_ports_v2,
+	.setup_irq = mtk_pcie_setup_irq,
+};
+
 static const struct of_device_id mtk_pcie_ids[] = {
 	{ .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 },
 	{ .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 },
+	{ .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_v2 },
+	{ .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_v2 },
 	{},
 };
 

-- 
2.6.4

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