On Thu, Feb 17, 2022 at 12:40:39AM +0100, Pali Rohár wrote:

On Friday 11 February 2022 18:52:02 Pali Rohár wrote:

quoted

On Friday 11 February 2022 17:19:17 Lorenzo Pieralisi wrote:

quoted

On Wed, Jan 12, 2022 at 04:18:13PM +0100, Pali Rohár wrote:

quoted

This adds support for legacy INTx interrupts received from other PCIe
devices and which are reported by a new INTx irq chip.

With this change, kernel can distinguish between INTA, INTB, INTC and INTD
interrupts.

Note that for this support, device tree files has to be properly adjusted
to provide "interrupts" or "interrupts-extended" property with intx
interrupt source, "interrupt-names" property with "intx" string and also
'interrupt-controller' subnode must be defined.

If device tree files do not provide these nodes then driver would work as
before.

Nit: this information is not useful. DT rules are written in DT
bindings, not in kernel commit logs. All I am saying is that firmware
developers should not have to read this log to write firmware.

It was not intended for firmware developers, but for reviewers of this
patch to understand, what is happening in code and that with old DT
files this patch does not change driver behavior (= work as before).

quoted

quoted

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-mvebu.c | 185 +++++++++++++++++++++++++++--
 1 file changed, 177 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 1e90ab888075..dbb6ecb4cb70 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c

@@ -54,9 +54,10 @@
 	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
 #define PCIE_INT_CAUSE_OFF	0x1900
+#define PCIE_INT_UNMASK_OFF	0x1910

Nit: I understand it is tempting but here you are redefining or better
giving a proper label to a register. Separate patch please.

Ok!

quoted

quoted

+#define  PCIE_INT_INTX(i)		BIT(24+i)
 #define  PCIE_INT_PM_PME		BIT(28)
-#define PCIE_MASK_OFF		0x1910

See above.

quoted

-#define  PCIE_MASK_ENABLE_INTS          0x0f000000
+#define  PCIE_INT_ALL_MASK		GENMASK(31, 0)
 #define PCIE_CTRL_OFF		0x1a00
 #define  PCIE_CTRL_X1_MODE		0x0001
 #define  PCIE_CTRL_RC_MODE		BIT(1)

@@ -110,6 +111,9 @@ struct mvebu_pcie_port {
 	struct mvebu_pcie_window iowin;
 	u32 saved_pcie_stat;
 	struct resource regs;
+	struct irq_domain *intx_irq_domain;
+	raw_spinlock_t irq_lock;
+	int intx_irq;
 };
 
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)

@@ -235,7 +239,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 {
-	u32 ctrl, lnkcap, cmd, dev_rev, mask;
+	u32 ctrl, lnkcap, cmd, dev_rev, unmask;
 
 	/* Setup PCIe controller to Root Complex mode. */
 	ctrl = mvebu_readl(port, PCIE_CTRL_OFF);

@@ -288,10 +292,30 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	/* Point PCIe unit MBUS decode windows to DRAM space. */
 	mvebu_pcie_setup_wins(port);
 
-	/* Enable interrupt lines A-D. */
-	mask = mvebu_readl(port, PCIE_MASK_OFF);
-	mask |= PCIE_MASK_ENABLE_INTS;
-	mvebu_writel(port, mask, PCIE_MASK_OFF);
+	/* Mask all interrupt sources. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OFF);
+
+	/* Clear all interrupt causes. */
+	mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF);
+
+	if (port->intx_irq <= 0) {
+		/*
+		 * When neither "summary" interrupt, nor "intx" interrupt was
+		 * specified in DT then unmask all legacy INTx interrupts as in
+		 * this case driver does not provide a way for masking and
+		 * unmasking of individual legacy INTx interrupts. In this case
+		 * all interrupts, including legacy INTx are reported via one
+		 * shared GIC source and therefore kernel cannot distinguish
+		 * which individual legacy INTx was triggered. These interrupts
+		 * are shared, so it should not cause any issue. Just
+		 * performance penalty as every PCIe interrupt handler needs to
+		 * be called when some interrupt is triggered.
+		 */

This comment applies to current mainline right (ie it describes how
current mainline handles INTx) ? IMO you should split it out in a
separate patch.

This above comment describe what happens in if-branch when intx_irq is
not set (as written in comment "when intx interrupt was not specified in
DT"). You are right that this is also the behavior in the current
mainline.

I'm not sure if this comment can be split out as support for "intx"
interrupt is in this patch.

quoted

I understand it is hard but a patch is a logical _change_, this
comment is a change per se, it is a clarification on current
behaviour.

Ok, I could try to split this comment into two patches, but part about
if-branch comment needs to stay in "this" patch.

I have done it locally.

Let me know when I should resend this patch series and I will include
into it also these changes.

Hi,

yes please resend it and I will merge it.

Thanks,
Lorenzo

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