Hi Andrew,

Thanks a lot for your comments!

-----Original Message-----
From: Andrew Murray <redacted>
Sent: 2020年1月13日 18:35
To: Z.q. Hou <zhiqiang.hou@nxp.com>
Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
bhelgaas@google.com; robh+dt@kernel.org; arnd@arndb.de;
mark.rutland@arm.com; l.subrahmanya@mobiveil.co.in;
shawnguo@kernel.org; m.karthikeyan@mobiveil.co.in; Leo Li
[off-list ref]; lorenzo.pieralisi@arm.com;
catalin.marinas@arm.com; will.deacon@arm.com; Mingkai Hu
[off-list ref]; M.h. Lian [off-list ref]; Xiaowei Bao
[off-list ref]
Subject: Re: [PATCHv9 03/12] PCI: mobiveil: Collect the interrupt related
operations into a routine

On Wed, Nov 20, 2019 at 03:45:37AM +0000, Z.q. Hou wrote:

quoted

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

Collect the interrupt initialization related operations into a new
routine to make it more readable.

I prefer the word 'function' instead of routine. Also indicate why, not only is it
nicer but it is in preparation for EP support.

Will replace the 'routine' with 'function', it is only used in RC mode, not used in EP mode.

quoted

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
---
V9:
 - New patch splited from the #1 of V8 patches to make it easy to review.

 drivers/pci/controller/pcie-mobiveil.c | 65
+++++++++++++++++---------
 1 file changed, 42 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/pcie-mobiveil.c

b/drivers/pci/controller/pcie-mobiveil.c
index 97f682ca7c7a..512b27a0536e 100644

--- a/drivers/pci/controller/pcie-mobiveil.c
+++ b/drivers/pci/controller/pcie-mobiveil.c

@@ -454,12 +454,6 @@ static int mobiveil_pcie_parse_dt(struct

mobiveil_pcie *pcie)

quoted

 		return PTR_ERR(pcie->csr_axi_slave_base);
 	pcie->pcie_reg_base = res->start;

-	/* map MSI config resource */
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

"apb_csr");

quoted

-	pcie->apb_csr_base = devm_pci_remap_cfg_resource(dev, res);
-	if (IS_ERR(pcie->apb_csr_base))
-		return PTR_ERR(pcie->apb_csr_base);
-
 	/* read the number of windows requested */
 	if (of_property_read_u32(node, "apio-wins", &pcie->apio_wins))
 		pcie->apio_wins = MAX_PIO_WINDOWS;

@@ -467,12 +461,6 @@ static int mobiveil_pcie_parse_dt(struct

mobiveil_pcie *pcie)

quoted

 	if (of_property_read_u32(node, "ppio-wins", &pcie->ppio_wins))
 		pcie->ppio_wins = MAX_PIO_WINDOWS;

-	rp->irq = platform_get_irq(pdev, 0);
-	if (rp->irq <= 0) {
-		dev_err(dev, "failed to map IRQ: %d\n", rp->irq);
-		return -ENODEV;
-	}
-
 	return 0;
 }

@@ -618,9 +606,6 @@ static int mobiveil_host_init(struct mobiveil_pcie

*pcie)

quoted

 	pab_ctrl |= (1 << AMBA_PIO_ENABLE_SHIFT) | (1 <<

PEX_PIO_ENABLE_SHIFT);

quoted

 	mobiveil_csr_writel(pcie, pab_ctrl, PAB_CTRL);

-	mobiveil_csr_writel(pcie, (PAB_INTP_INTX_MASK |

PAB_INTP_MSI_MASK),

quoted

-			    PAB_INTP_AMBA_MISC_ENB);
-
 	/*
 	 * program PIO Enable Bit to 1 and Config Window Enable Bit to 1 in
 	 * PAB_AXI_PIO_CTRL Register

@@ -670,9 +655,6 @@ static int mobiveil_host_init(struct mobiveil_pcie

*pcie)

quoted

 	value |= (PCI_CLASS_BRIDGE_PCI << 16);
 	mobiveil_csr_writel(pcie, value, PAB_INTP_AXI_PIO_CLASS);

-	/* setup MSI hardware registers */
-	mobiveil_pcie_enable_msi(pcie);
-
 	return 0;
 }

@@ -873,6 +855,46 @@ static int mobiveil_pcie_init_irq_domain(struct

mobiveil_pcie *pcie)

quoted

 	return 0;
 }

+static int mobiveil_pcie_interrupt_init(struct mobiveil_pcie *pcie) {
+	struct platform_device *pdev = pcie->pdev;
+	struct device *dev = &pdev->dev;
+	struct root_port *rp = &pcie->rp;
+	struct resource *res;
+	int ret;
+
+	/* map MSI config resource */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

"apb_csr");

quoted

+	pcie->apb_csr_base = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(pcie->apb_csr_base))
+		return PTR_ERR(pcie->apb_csr_base);
+
+	/* setup MSI hardware registers */
+	mobiveil_pcie_enable_msi(pcie);

Does this need to come after mobiveil_pcie_init_irq_domain - given that this
function sets up the irq domain for MSI?

No, I don't think so, because I didn't change the relative order of the 2 functions
you mentioned.

Thanks,
Zhiqiang

Thanks,

Andrew Murray

quoted

+
+	rp->irq = platform_get_irq(pdev, 0);
+	if (rp->irq <= 0) {
+		dev_err(dev, "failed to map IRQ: %d\n", rp->irq);
+		return -ENODEV;
+	}
+
+	/* initialize the IRQ domains */
+	ret = mobiveil_pcie_init_irq_domain(pcie);
+	if (ret) {
+		dev_err(dev, "Failed creating IRQ Domain\n");
+		return ret;
+	}
+
+	irq_set_chained_handler_and_data(rp->irq, mobiveil_pcie_isr, pcie);
+
+	/* Enable interrupts */
+	mobiveil_csr_writel(pcie, (PAB_INTP_INTX_MASK |

PAB_INTP_MSI_MASK),

quoted

+			    PAB_INTP_AMBA_MISC_ENB);
+
+
+	return 0;
+}
+
 int mobiveil_pcie_host_probe(struct mobiveil_pcie *pcie)  {
 	struct root_port *rp = &pcie->rp;

@@ -906,15 +928,12 @@ int mobiveil_pcie_host_probe(struct

mobiveil_pcie *pcie)

quoted

 		return ret;
 	}

-	/* initialize the IRQ domains */
-	ret = mobiveil_pcie_init_irq_domain(pcie);
+	ret = mobiveil_pcie_interrupt_init(pcie);
 	if (ret) {
-		dev_err(dev, "Failed creating IRQ Domain\n");
+		dev_err(dev, "Interrupt init failed\n");
 		return ret;
 	}

-	irq_set_chained_handler_and_data(rp->irq, mobiveil_pcie_isr, pcie);
-
 	/* Initialize bridge */
 	bridge->dev.parent = dev;
 	bridge->sysdata = pcie;
--
2.17.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel