[PATCH v2 0/9] PCI: DWC/Keystone: MSI configuration cleanup

[PATCH v2 0/9] PCI: DWC/Keystone: MSI configuration cleanup

[Kishon Vijay Abraham I]

This series tries to address the comments discussed in [1] w.r.t
removing Keystone specific callbacks defined in dw_pcie_host_ops.

This series also tries to cleanup the Keystone interrupt handling
part.

Changes from v1:
*) Removed "PCI: keystone: Use "dummy_irq_chip" instead of new irqchip
for legacy interrupt handling" from the patch series. It should be
handled differently.

*) Added Gustavo's ACKed by and fixed a commit message.

[1] -> https://patchwork.kernel.org/patch/10681587/

Kishon Vijay Abraham I (9):
  PCI: keystone: Cleanup interrupt related macros
  PCI: keystone: Modify legacy_irq_handler to check the IRQ_STATUS of
    INTA/B/C/D
  PCI: keystone: Add separate functions for configuring MSI and legacy
    interrupt
  PCI: keystone: Use hwirq to get the IRQ number offset
  PCI: keystone: Cleanup ks_pcie_msi_irq_handler and
    ks_pcie_legacy_irq_handler
  PCI: dwc: Add support to use non default msi_irq_chip
  PCI: keystone: Use Keystone specific msi_irq_chip
  PCI: dwc: Remove Keystone specific dw_pcie_host_ops
  PCI: dwc: Do not write to MSI control registers if the platform
    doesn't use it

 drivers/pci/controller/dwc/pci-keystone.c     | 406 ++++++++++--------
 .../pci/controller/dwc/pcie-designware-host.c |  74 ++--
 drivers/pci/controller/dwc/pcie-designware.h  |   6 +-
 3 files changed, 256 insertions(+), 230 deletions(-)

-- 
2.17.1

[PATCH v2 1/9] PCI: keystone: Cleanup interrupt related macros

[Kishon Vijay Abraham I]

No functional change. Change both MSI interrupt and legacy interrupt
related macros to take an additional argument in order to return the
correct register offset.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 26 +++++++++++------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 14f2b0b4ed5e..5286a480f76b 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -52,17 +52,17 @@
 
 /* IRQ register defines */
 #define IRQ_EOI				0x050
-#define IRQ_STATUS			0x184
-#define IRQ_ENABLE_SET			0x188
-#define IRQ_ENABLE_CLR			0x18c
 
 #define MSI_IRQ				0x054
-#define MSI0_IRQ_STATUS			0x104
-#define MSI0_IRQ_ENABLE_SET		0x108
-#define MSI0_IRQ_ENABLE_CLR		0x10c
-#define IRQ_STATUS			0x184
+#define MSI_IRQ_STATUS(n)		(0x104 + ((n) << 4))
+#define MSI_IRQ_ENABLE_SET(n)		(0x108 + ((n) << 4))
+#define MSI_IRQ_ENABLE_CLR(n)		(0x10c + ((n) << 4))
 #define MSI_IRQ_OFFSET			4
 
+#define IRQ_STATUS(n)			(0x184 + ((n) << 4))
+#define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
+#define INTx_EN				BIT(0)
+
 #define ERR_IRQ_STATUS			0x1c4
 #define ERR_IRQ_ENABLE_SET		0x1c8
 #define ERR_AER				BIT(5)	/* ECRC error */
@@ -142,7 +142,7 @@ static void ks_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie, int offset)
 	u32 pending, vector;
 	int src, virq;
 
-	pending = ks_pcie_app_readl(ks_pcie, MSI0_IRQ_STATUS + (offset << 4));
+	pending = ks_pcie_app_readl(ks_pcie, MSI_IRQ_STATUS(offset));
 
 	/*
 	 * MSI0 status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit
@@ -169,7 +169,7 @@ static void ks_pcie_msi_irq_ack(int irq, struct pcie_port *pp)
 	ks_pcie = to_keystone_pcie(pci);
 	update_reg_offset_bit_pos(irq, &reg_offset, &bit_pos);
 
-	ks_pcie_app_writel(ks_pcie, MSI0_IRQ_STATUS + (reg_offset << 4),
+	ks_pcie_app_writel(ks_pcie, MSI_IRQ_STATUS(reg_offset),
 			   BIT(bit_pos));
 	ks_pcie_app_writel(ks_pcie, IRQ_EOI, reg_offset + MSI_IRQ_OFFSET);
 }
@@ -181,7 +181,7 @@ static void ks_pcie_msi_set_irq(struct pcie_port *pp, int irq)
 	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
 
 	update_reg_offset_bit_pos(irq, &reg_offset, &bit_pos);
-	ks_pcie_app_writel(ks_pcie, MSI0_IRQ_ENABLE_SET + (reg_offset << 4),
+	ks_pcie_app_writel(ks_pcie, MSI_IRQ_ENABLE_SET(reg_offset),
 			   BIT(bit_pos));
 }
 
@@ -192,7 +192,7 @@ static void ks_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
 	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
 
 	update_reg_offset_bit_pos(irq, &reg_offset, &bit_pos);
-	ks_pcie_app_writel(ks_pcie, MSI0_IRQ_ENABLE_CLR + (reg_offset << 4),
+	ks_pcie_app_writel(ks_pcie, MSI_IRQ_ENABLE_CLR(reg_offset),
 			   BIT(bit_pos));
 }
 
@@ -206,7 +206,7 @@ static void ks_pcie_enable_legacy_irqs(struct keystone_pcie *ks_pcie)
 	int i;
 
 	for (i = 0; i < PCI_NUM_INTX; i++)
-		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET + (i << 4), 0x1);
+		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), 0x1);
 }
 
 static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
@@ -217,7 +217,7 @@ static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
 	u32 pending;
 	int virq;
 
-	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS + (offset << 4));
+	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
 
 	if (BIT(0) & pending) {
 		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
-- 
2.17.1

[PATCH v2 2/9] PCI: keystone: Modify legacy_irq_handler to check the IRQ_STATUS of INTA/B/C/D

[Kishon Vijay Abraham I]

The legacy interrupt handler directly checks the IRQ_STATUS register
corresponding to a interrupt line inorder to invoke generic_handle_irq.

While this is okay for K2G platform which has separate interrupt line for
each of the 4 legacy interrupts, AM654 which uses the same PCIe wrapper
has a single interrupt line for all the legacy interrupts. So for AM654
the interrupt handler won't be able to directly check the IRQ_STATUS
register corresponding to the interrupt line.

Also the legacy interrupt handler uses 'virq' obtained from
irq_of_parse_and_map to find the correct interrupt line which raised the
interrupt. There is no guarantee that virq assigned for contiguous hardware
irq will be contiguous and the interrupt handler might end up checking
the wrong IRQ_STATUS register.

In order to overcome the above issues, read the IRQ_STATUS register of
all the 4 legacy interrupts to determine which interrupt was raised.

Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 5286a480f76b..4cf9849d5a1d 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -214,16 +214,11 @@ static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
 {
 	struct dw_pcie *pci = ks_pcie->pci;
 	struct device *dev = pci->dev;
-	u32 pending;
 	int virq;
 
-	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
-
-	if (BIT(0) & pending) {
-		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
-		dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
-		generic_handle_irq(virq);
-	}
+	virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
+	dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
+	generic_handle_irq(virq);
 
 	/* EOI the INTx interrupt */
 	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
@@ -607,8 +602,9 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
 	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
 	struct dw_pcie *pci = ks_pcie->pci;
 	struct device *dev = pci->dev;
-	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
 	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int irq_no;
+	u32 reg;
 
 	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
 
@@ -618,7 +614,13 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
 	 * ack operation.
 	 */
 	chained_irq_enter(chip, desc);
-	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
+	for (irq_no = 0; irq_no < PCI_NUM_INTX; irq_no++) {
+		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(irq_no));
+		if (!(reg & INTx_EN))
+			continue;
+		ks_pcie_handle_legacy_irq(ks_pcie, irq_no);
+	}
+
 	chained_irq_exit(chip, desc);
 }
 
-- 
2.17.1

[PATCH v2 3/9] PCI: keystone: Add separate functions for configuring MSI and legacy interrupt

[Kishon Vijay Abraham I]

ks_pcie_get_irq_controller_info() was used to configure both MSI and
legacy interrupt. This will prevent MSI or legacy interrupt specific
intializations. Add separate functions to configure MSI and legacy
interrupts.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 188 +++++++++++-----------
 1 file changed, 96 insertions(+), 92 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 4cf9849d5a1d..b1d01751c1af 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -87,11 +87,8 @@ struct keystone_pcie {
 	struct dw_pcie		*pci;
 	/* PCI Device ID */
 	u32			device_id;
-	int			num_legacy_host_irqs;
-	int			legacy_host_irqs[PCI_NUM_INTX];
 	struct			device_node *legacy_intc_np;
 
-	int			num_msi_host_irqs;
 	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
 	int			num_lanes;
 	u32			num_viewport;
@@ -201,14 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp)
 	return dw_pcie_allocate_domains(pp);
 }
 
-static void ks_pcie_enable_legacy_irqs(struct keystone_pcie *ks_pcie)
-{
-	int i;
-
-	for (i = 0; i < PCI_NUM_INTX; i++)
-		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), 0x1);
-}
-
 static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
 				      int offset)
 {
@@ -490,17 +479,6 @@ static int __init ks_pcie_dw_host_init(struct keystone_pcie *ks_pcie)
 
 	ks_pcie->app = *res;
 
-	/* Create legacy IRQ domain */
-	ks_pcie->legacy_irq_domain =
-			irq_domain_add_linear(ks_pcie->legacy_intc_np,
-					      PCI_NUM_INTX,
-					      &ks_pcie_legacy_irq_domain_ops,
-					      NULL);
-	if (!ks_pcie->legacy_irq_domain) {
-		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
-		return -EINVAL;
-	}
-
 	return dw_pcie_host_init(pp);
 }
 
@@ -624,85 +602,117 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie,
-					   char *controller, int *num_irqs)
+static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 {
-	int temp, max_host_irqs, legacy = 1, *host_irqs;
 	struct device *dev = ks_pcie->pci->dev;
-	struct device_node *np_pcie = dev->of_node, **np_temp;
-
-	if (!strcmp(controller, "msi-interrupt-controller"))
-		legacy = 0;
-
-	if (legacy) {
-		np_temp = &ks_pcie->legacy_intc_np;
-		max_host_irqs = PCI_NUM_INTX;
-		host_irqs = &ks_pcie->legacy_host_irqs[0];
-	} else {
-		np_temp = &ks_pcie->msi_intc_np;
-		max_host_irqs = MAX_MSI_HOST_IRQS;
-		host_irqs =  &ks_pcie->msi_host_irqs[0];
-	}
+	struct device_node *np = ks_pcie->np;
+	struct device_node *intc_np;
+	int irq_count;
+	int irq;
+	int ret;
+	int i;
 
-	/* interrupt controller is in a child node */
-	*np_temp = of_get_child_by_name(np_pcie, controller);
-	if (!(*np_temp)) {
-		dev_err(dev, "Node for %s is absent\n", controller);
-		return -EINVAL;
-	}
+	if (!IS_ENABLED(CONFIG_PCI_MSI))
+		return 0;
 
-	temp = of_irq_count(*np_temp);
-	if (!temp) {
-		dev_err(dev, "No IRQ entries in %s\n", controller);
-		of_node_put(*np_temp);
+	intc_np = of_get_child_by_name(np, "msi-interrupt-controller");
+	if (!intc_np) {
+		dev_WARN(dev, "msi-interrupt-controller node is absent\n");
 		return -EINVAL;
 	}
 
-	if (temp > max_host_irqs)
-		dev_warn(dev, "Too many %s interrupts defined %u\n",
-			(legacy ? "legacy" : "MSI"), temp);
+	irq_count = of_irq_count(intc_np);
+	if (!irq_count) {
+		dev_err(dev, "No IRQ entries in msi-interrupt-controller\n");
+		ret = -EINVAL;
+		goto err;
+	}
 
-	/*
-	 * support upto max_host_irqs. In dt from index 0 to 3 (legacy) or 0 to
-	 * 7 (MSI)
-	 */
-	for (temp = 0; temp < max_host_irqs; temp++) {
-		host_irqs[temp] = irq_of_parse_and_map(*np_temp, temp);
-		if (!host_irqs[temp])
-			break;
+	if (irq_count > MAX_MSI_HOST_IRQS) {
+		dev_warn(dev, "Too many MSI interrupt lines defined %u\n",
+			 irq_count);
+		irq_count = MAX_MSI_HOST_IRQS;
 	}
 
-	of_node_put(*np_temp);
+	for (i = 0; i < irq_count; i++) {
+		irq = irq_of_parse_and_map(intc_np, i);
+		if (!irq) {
+			ret = -EINVAL;
+			goto err;
+		}
+		ks_pcie->msi_host_irqs[i] = irq;
 
-	if (temp) {
-		*num_irqs = temp;
-		return 0;
+		irq_set_chained_handler_and_data(irq, ks_pcie_msi_irq_handler,
+						 ks_pcie);
 	}
 
-	return -EINVAL;
+	of_node_put(intc_np);
+	return 0;
+
+err:
+	of_node_put(intc_np);
+	return ret;
 }
 
-static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
+static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 {
+	struct device *dev = ks_pcie->pci->dev;
+	struct irq_domain *legacy_irq_domain;
+	struct device_node *np = ks_pcie->np;
+	struct device_node *intc_np;
+	int irq_count;
+	int irq;
+	int ret;
 	int i;
 
-	/* Legacy IRQ */
-	for (i = 0; i < ks_pcie->num_legacy_host_irqs; i++) {
-		irq_set_chained_handler_and_data(ks_pcie->legacy_host_irqs[i],
+	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
+	if (!intc_np) {
+		dev_WARN(dev, "legacy-interrupt-controller node is absent\n");
+		return -EINVAL;
+	}
+
+	irq_count = of_irq_count(intc_np);
+	if (!irq_count) {
+		dev_err(dev, "No IRQ entries in legacy-interrupt-controller\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	for (i = 0; i < irq_count; i++) {
+		irq = irq_of_parse_and_map(intc_np, i);
+		if (!irq) {
+			ret = -EINVAL;
+			goto err;
+		}
+		irq_set_chained_handler_and_data(irq,
 						 ks_pcie_legacy_irq_handler,
 						 ks_pcie);
 	}
-	ks_pcie_enable_legacy_irqs(ks_pcie);
-
-	/* MSI IRQ */
-	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-		for (i = 0; i < ks_pcie->num_msi_host_irqs; i++) {
-			irq_set_chained_handler_and_data(ks_pcie->msi_host_irqs[i],
-							 ks_pcie_msi_irq_handler,
-							 ks_pcie);
-		}
+
+	legacy_irq_domain =
+		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
+				      &ks_pcie_legacy_irq_domain_ops, NULL);
+	if (!legacy_irq_domain) {
+		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
+		ret = -EINVAL;
+		goto err;
 	}
+	ks_pcie->legacy_irq_domain = legacy_irq_domain;
+
+	for (i = 0; i < PCI_NUM_INTX; i++)
+		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), INTx_EN);
+
+	of_node_put(intc_np);
+
+	return 0;
+
+err:
+	of_node_put(intc_np);
+	return ret;
+}
 
+static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
+{
 	if (ks_pcie->error_irq > 0)
 		ks_pcie_enable_error_irq(ks_pcie);
 }
@@ -756,6 +766,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
 	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
 	int ret;
 
+	ret = ks_pcie_config_legacy_irq(ks_pcie);
+	if (ret)
+		return ret;
+
+	ret = ks_pcie_config_msi_irq(ks_pcie);
+	if (ret)
+		return ret;
+
 	dw_pcie_setup_rc(pp);
 
 	ks_pcie_establish_link(ks_pcie);
@@ -805,20 +823,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	ret = ks_pcie_get_irq_controller_info(ks_pcie,
-					"legacy-interrupt-controller",
-					&ks_pcie->num_legacy_host_irqs);
-	if (ret)
-		return ret;
-
-	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-		ret = ks_pcie_get_irq_controller_info(ks_pcie,
-						"msi-interrupt-controller",
-						&ks_pcie->num_msi_host_irqs);
-		if (ret)
-			return ret;
-	}
-
 	/*
 	 * Index 0 is the platform interrupt for error interrupt
 	 * from RC.  This is optional.
-- 
2.17.1

[PATCH v2 4/9] PCI: keystone: Use hwirq to get the IRQ number offset

[Kishon Vijay Abraham I]

ks_pcie_msi_irq_handler() uses 'virq' to get the IRQ number offset.
This offset is used to get the correct MSI_IRQ_STATUS register
corresponding to the IRQ line that raised the interrupt.
There is no guarantee that 'virq' assigned for consecutive hardware
IRQ will be contiguous. And this might get us an incorrect IRQ number
offset.

Fix it here by using 'hwirq' to get the IRQ number offset. Since we
don't store the 'virq' numbers of all the IRQ numbers, stop checking
if irq count is greater than MAX_MSI_HOST_IRQS and remove
MAX_MSI_HOST_IRQS.

Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 24 ++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index b1d01751c1af..a404fad12bdf 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -74,7 +74,6 @@
 #define ERR_IRQ_ALL			(ERR_AER | ERR_AXI | ERR_CORR | \
 					 ERR_NONFATAL | ERR_FATAL | ERR_SYS)
 
-#define MAX_MSI_HOST_IRQS		8
 /* PCIE controller device IDs */
 #define PCIE_RC_K2HK			0xb008
 #define PCIE_RC_K2E			0xb009
@@ -89,7 +88,7 @@ struct keystone_pcie {
 	u32			device_id;
 	struct			device_node *legacy_intc_np;
 
-	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
+	int			msi_host_irq;
 	int			num_lanes;
 	u32			num_viewport;
 	struct phy		**phy;
@@ -547,9 +546,9 @@ static int ks_pcie_establish_link(struct keystone_pcie *ks_pcie)
 
 static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
 {
-	unsigned int irq = irq_desc_get_irq(desc);
+	unsigned int irq = desc->irq_data.hwirq;
 	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
-	u32 offset = irq - ks_pcie->msi_host_irqs[0];
+	u32 offset = irq - ks_pcie->msi_host_irq;
 	struct dw_pcie *pci = ks_pcie->pci;
 	struct device *dev = pci->dev;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -607,6 +606,7 @@ static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 	struct device *dev = ks_pcie->pci->dev;
 	struct device_node *np = ks_pcie->np;
 	struct device_node *intc_np;
+	struct irq_data *irq_data;
 	int irq_count;
 	int irq;
 	int ret;
@@ -628,19 +628,21 @@ static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 		goto err;
 	}
 
-	if (irq_count > MAX_MSI_HOST_IRQS) {
-		dev_warn(dev, "Too many MSI interrupt lines defined %u\n",
-			 irq_count);
-		irq_count = MAX_MSI_HOST_IRQS;
-	}
-
 	for (i = 0; i < irq_count; i++) {
 		irq = irq_of_parse_and_map(intc_np, i);
 		if (!irq) {
 			ret = -EINVAL;
 			goto err;
 		}
-		ks_pcie->msi_host_irqs[i] = irq;
+
+		if (!ks_pcie->msi_host_irq) {
+			irq_data = irq_get_irq_data(irq);
+			if (!irq_data) {
+				ret = -EINVAL;
+				goto err;
+			}
+			ks_pcie->msi_host_irq = irq_data->hwirq;
+		}
 
 		irq_set_chained_handler_and_data(irq, ks_pcie_msi_irq_handler,
 						 ks_pcie);
-- 
2.17.1

[PATCH v2 5/9] PCI: keystone: Cleanup ks_pcie_msi_irq_handler and ks_pcie_legacy_irq_handler

[Kishon Vijay Abraham I]

Both ks_pcie_msi_irq_handler() and ks_pcie_legacy_irq_handler() invokes
ks_pcie_handle_msi_irq() and ks_pcie_handle_legacy_irq() respectively
for handling the interrupts.

Having two functions for handling the interrupt was used when keystone
PCIe driver was implemented using two files. But with
commit b492aca35c982011500377797d2 ("PCI: keystone: Merge
pci-keystone-dw.c and pci-keystone.c"), which merged the keystone PCIe
driver to use a single file, two functions for handling the
interrupt handler is not required.

Handle both MSI interrupt and legacy interrupt in a single interrupt
handler here.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 92 ++++++++++-------------
 1 file changed, 39 insertions(+), 53 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index a404fad12bdf..3d1abc99fd08 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -104,13 +104,6 @@ struct keystone_pcie {
 	struct resource		app;
 };
 
-static inline void update_reg_offset_bit_pos(u32 offset, u32 *reg_offset,
-					     u32 *bit_pos)
-{
-	*reg_offset = offset % 8;
-	*bit_pos = offset >> 3;
-}
-
 static phys_addr_t ks_pcie_get_msi_addr(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -130,31 +123,6 @@ static void ks_pcie_app_writel(struct keystone_pcie *ks_pcie, u32 offset,
 	writel(val, ks_pcie->va_app_base + offset);
 }
 
-static void ks_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie, int offset)
-{
-	struct dw_pcie *pci = ks_pcie->pci;
-	struct pcie_port *pp = &pci->pp;
-	struct device *dev = pci->dev;
-	u32 pending, vector;
-	int src, virq;
-
-	pending = ks_pcie_app_readl(ks_pcie, MSI_IRQ_STATUS(offset));
-
-	/*
-	 * MSI0 status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit
-	 * shows 1, 9, 17, 25 and so forth
-	 */
-	for (src = 0; src < 4; src++) {
-		if (BIT(src) & pending) {
-			vector = offset + (src << 3);
-			virq = irq_linear_revmap(pp->irq_domain, vector);
-			dev_dbg(dev, "irq: bit %d, vector %d, virq %d\n",
-				src, vector, virq);
-			generic_handle_irq(virq);
-		}
-	}
-}
-
 static void ks_pcie_msi_irq_ack(int irq, struct pcie_port *pp)
 {
 	u32 reg_offset, bit_pos;
@@ -163,7 +131,9 @@ static void ks_pcie_msi_irq_ack(int irq, struct pcie_port *pp)
 
 	pci = to_dw_pcie_from_pp(pp);
 	ks_pcie = to_keystone_pcie(pci);
-	update_reg_offset_bit_pos(irq, &reg_offset, &bit_pos);
+
+	reg_offset = irq % 8;
+	bit_pos = irq >> 3;
 
 	ks_pcie_app_writel(ks_pcie, MSI_IRQ_STATUS(reg_offset),
 			   BIT(bit_pos));
@@ -176,7 +146,9 @@ static void ks_pcie_msi_set_irq(struct pcie_port *pp, int irq)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
 
-	update_reg_offset_bit_pos(irq, &reg_offset, &bit_pos);
+	reg_offset = irq % 8;
+	bit_pos = irq >> 3;
+
 	ks_pcie_app_writel(ks_pcie, MSI_IRQ_ENABLE_SET(reg_offset),
 			   BIT(bit_pos));
 }
@@ -187,7 +159,9 @@ static void ks_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
 
-	update_reg_offset_bit_pos(irq, &reg_offset, &bit_pos);
+	reg_offset = irq % 8;
+	bit_pos = irq >> 3;
+
 	ks_pcie_app_writel(ks_pcie, MSI_IRQ_ENABLE_CLR(reg_offset),
 			   BIT(bit_pos));
 }
@@ -197,21 +171,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp)
 	return dw_pcie_allocate_domains(pp);
 }
 
-static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
-				      int offset)
-{
-	struct dw_pcie *pci = ks_pcie->pci;
-	struct device *dev = pci->dev;
-	int virq;
-
-	virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
-	dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
-	generic_handle_irq(virq);
-
-	/* EOI the INTx interrupt */
-	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
-}
-
 static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie)
 {
 	ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL);
@@ -550,8 +509,13 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
 	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
 	u32 offset = irq - ks_pcie->msi_host_irq;
 	struct dw_pcie *pci = ks_pcie->pci;
+	struct pcie_port *pp = &pci->pp;
 	struct device *dev = pci->dev;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 vector;
+	u32 virq;
+	u32 reg;
+	u32 pos;
 
 	dev_dbg(dev, "%s, irq %d\n", __func__, irq);
 
@@ -561,7 +525,23 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
 	 * ack operation.
 	 */
 	chained_irq_enter(chip, desc);
-	ks_pcie_handle_msi_irq(ks_pcie, offset);
+
+	reg = ks_pcie_app_readl(ks_pcie, MSI_IRQ_STATUS(offset));
+	/*
+	 * MSI0 status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit
+	 * shows 1, 9, 17, 25 and so forth
+	 */
+	for (pos = 0; pos < 4; pos++) {
+		if (!(reg & BIT(pos)))
+			continue;
+
+		vector = offset + (pos << 3);
+		virq = irq_linear_revmap(pp->irq_domain, vector);
+		dev_dbg(dev, "irq: bit %d, vector %d, virq %d\n", pos, vector,
+			virq);
+		generic_handle_irq(virq);
+	}
+
 	chained_irq_exit(chip, desc);
 }
 
@@ -581,6 +561,7 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
 	struct device *dev = pci->dev;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	unsigned int irq_no;
+	u32 virq;
 	u32 reg;
 
 	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
@@ -595,9 +576,14 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
 		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(irq_no));
 		if (!(reg & INTx_EN))
 			continue;
-		ks_pcie_handle_legacy_irq(ks_pcie, irq_no);
-	}
 
+		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, irq_no);
+		dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", irq_no, virq);
+		generic_handle_irq(virq);
+
+		/* EOI the INTx interrupt */
+		ks_pcie_app_writel(ks_pcie, IRQ_EOI, irq_no);
+	}
 	chained_irq_exit(chip, desc);
 }
 
-- 
2.17.1

[PATCH v2 6/9] PCI: dwc: Add support to use non default msi_irq_chip

[Kishon Vijay Abraham I]

Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for
configuring the MSI controller logic within the Designware IP. However
certain platforms like Keystone (K2G) which uses Desingware IP has
it's own MSI controller logic. For handling such platforms,
the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback
functions.

Add support to use different msi_irq_chip with default as
dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off
msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific
dw_pcie_host_ops. This will also help to get rid of get_msi_addr and
get_msi_data ops.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 721d60a5d9e4..042de09b0451 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,
 
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_set_info(domain, virq + i, bit + i,
-				    &dw_pci_msi_bottom_irq_chip,
+				    pp->msi_irq_chip,
 				    pp, handle_edge_irq,
 				    NULL, NULL);
 
@@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);
 
+	if (!pp->msi_irq_chip)
+		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;
+
 	pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors,
 					       &dw_pcie_msi_domain_ops, pp);
 	if (!pp->irq_domain) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 1f56e6ae34ff..95e0c3c93f48 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -176,6 +176,7 @@ struct pcie_port {
 	struct irq_domain	*irq_domain;
 	struct irq_domain	*msi_domain;
 	dma_addr_t		msi_data;
+	struct irq_chip		*msi_irq_chip;
 	u32			num_vectors;
 	u32			irq_status[MAX_MSI_CTRLS];
 	raw_spinlock_t		lock;
-- 
2.17.1

[PATCH v2 7/9] PCI: keystone: Use Keystone specific msi_irq_chip

[Kishon Vijay Abraham I]

Use Keystone specific msi_irq_chip to configure the MSI controller
logic in the PCIe keystone wrapper instead of using the default
Designware msi_irq chip (dw_pci_msi_bottom_irq_chip) with
callback functions for configuring the Keystone MSI controller.
This will help to remove Keystone specific callback functions
added in dw_pcie_host_ops.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 96 +++++++++++++++++------
 1 file changed, 72 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 3d1abc99fd08..94eb5830967e 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -104,14 +104,6 @@ struct keystone_pcie {
 	struct resource		app;
 };
 
-static phys_addr_t ks_pcie_get_msi_addr(struct pcie_port *pp)
-{
-	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
-
-	return ks_pcie->app.start + MSI_IRQ;
-}
-
 static u32 ks_pcie_app_readl(struct keystone_pcie *ks_pcie, u32 offset)
 {
 	return readl(ks_pcie->va_app_base + offset);
@@ -123,11 +115,14 @@ static void ks_pcie_app_writel(struct keystone_pcie *ks_pcie, u32 offset,
 	writel(val, ks_pcie->va_app_base + offset);
 }
 
-static void ks_pcie_msi_irq_ack(int irq, struct pcie_port *pp)
+static void ks_pcie_msi_irq_ack(struct irq_data *data)
 {
-	u32 reg_offset, bit_pos;
+	struct pcie_port *pp  = irq_data_get_irq_chip_data(data);
 	struct keystone_pcie *ks_pcie;
+	u32 irq = data->hwirq;
 	struct dw_pcie *pci;
+	u32 reg_offset;
+	u32 bit_pos;
 
 	pci = to_dw_pcie_from_pp(pp);
 	ks_pcie = to_keystone_pcie(pci);
@@ -140,34 +135,91 @@ static void ks_pcie_msi_irq_ack(int irq, struct pcie_port *pp)
 	ks_pcie_app_writel(ks_pcie, IRQ_EOI, reg_offset + MSI_IRQ_OFFSET);
 }
 
-static void ks_pcie_msi_set_irq(struct pcie_port *pp, int irq)
+static void ks_pcie_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
-	u32 reg_offset, bit_pos;
-	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(data);
+	struct keystone_pcie *ks_pcie;
+	struct dw_pcie *pci;
+	u64 msi_target;
+
+	pci = to_dw_pcie_from_pp(pp);
+	ks_pcie = to_keystone_pcie(pci);
+
+	msi_target = ks_pcie->app.start + MSI_IRQ;
+	msg->address_lo = lower_32_bits(msi_target);
+	msg->address_hi = upper_32_bits(msi_target);
+	msg->data = data->hwirq;
+
+	dev_dbg(pci->dev, "msi#%d address_hi %#x address_lo %#x\n",
+		(int)data->hwirq, msg->address_hi, msg->address_lo);
+}
+
+static int ks_pcie_msi_set_affinity(struct irq_data *irq_data,
+				    const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void ks_pcie_msi_mask(struct irq_data *data)
+{
+	struct pcie_port *pp = irq_data_get_irq_chip_data(data);
+	struct keystone_pcie *ks_pcie;
+	u32 irq = data->hwirq;
+	struct dw_pcie *pci;
+	unsigned long flags;
+	u32 reg_offset;
+	u32 bit_pos;
+
+	raw_spin_lock_irqsave(&pp->lock, flags);
+
+	pci = to_dw_pcie_from_pp(pp);
+	ks_pcie = to_keystone_pcie(pci);
 
 	reg_offset = irq % 8;
 	bit_pos = irq >> 3;
 
-	ks_pcie_app_writel(ks_pcie, MSI_IRQ_ENABLE_SET(reg_offset),
+	ks_pcie_app_writel(ks_pcie, MSI_IRQ_ENABLE_CLR(reg_offset),
 			   BIT(bit_pos));
+
+	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
-static void ks_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
+static void ks_pcie_msi_unmask(struct irq_data *data)
 {
-	u32 reg_offset, bit_pos;
-	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
+	struct pcie_port *pp = irq_data_get_irq_chip_data(data);
+	struct keystone_pcie *ks_pcie;
+	u32 irq = data->hwirq;
+	struct dw_pcie *pci;
+	unsigned long flags;
+	u32 reg_offset;
+	u32 bit_pos;
+
+	raw_spin_lock_irqsave(&pp->lock, flags);
+
+	pci = to_dw_pcie_from_pp(pp);
+	ks_pcie = to_keystone_pcie(pci);
 
 	reg_offset = irq % 8;
 	bit_pos = irq >> 3;
 
-	ks_pcie_app_writel(ks_pcie, MSI_IRQ_ENABLE_CLR(reg_offset),
+	ks_pcie_app_writel(ks_pcie, MSI_IRQ_ENABLE_SET(reg_offset),
 			   BIT(bit_pos));
+
+	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
+static struct irq_chip ks_pcie_msi_irq_chip = {
+	.name = "KEYSTONE-PCI-MSI",
+	.irq_ack = ks_pcie_msi_irq_ack,
+	.irq_compose_msi_msg = ks_pcie_compose_msi_msg,
+	.irq_set_affinity = ks_pcie_msi_set_affinity,
+	.irq_mask = ks_pcie_msi_mask,
+	.irq_unmask = ks_pcie_msi_unmask,
+};
+
 static int ks_pcie_msi_host_init(struct pcie_port *pp)
 {
+	pp->msi_irq_chip = &ks_pcie_msi_irq_chip;
 	return dw_pcie_allocate_domains(pp);
 }
 
@@ -788,11 +840,7 @@ static const struct dw_pcie_host_ops ks_pcie_host_ops = {
 	.rd_other_conf = ks_pcie_rd_other_conf,
 	.wr_other_conf = ks_pcie_wr_other_conf,
 	.host_init = ks_pcie_host_init,
-	.msi_set_irq = ks_pcie_msi_set_irq,
-	.msi_clear_irq = ks_pcie_msi_clear_irq,
-	.get_msi_addr = ks_pcie_get_msi_addr,
 	.msi_host_init = ks_pcie_msi_host_init,
-	.msi_irq_ack = ks_pcie_msi_irq_ack,
 	.scan_bus = ks_pcie_v3_65_scan_bus,
 };
 
-- 
2.17.1

[PATCH v2 8/9] PCI: dwc: Remove Keystone specific dw_pcie_host_ops

[Kishon Vijay Abraham I]

Now that Keystone started using it's own msi_irq_chip, remove
Keystone specific callback function defined in dw_pcie_host_ops.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 .../pci/controller/dwc/pcie-designware-host.c | 45 ++++++-------------
 drivers/pci/controller/dwc/pcie-designware.h  |  5 ---
 2 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 042de09b0451..9492b05e8ff0 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -126,18 +126,12 @@ static void dw_pci_setup_msi_msg(struct irq_data *data, struct msi_msg *msg)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	u64 msi_target;
 
-	if (pp->ops->get_msi_addr)
-		msi_target = pp->ops->get_msi_addr(pp);
-	else
-		msi_target = (u64)pp->msi_data;
+	msi_target = (u64)pp->msi_data;
 
 	msg->address_lo = lower_32_bits(msi_target);
 	msg->address_hi = upper_32_bits(msi_target);
 
-	if (pp->ops->get_msi_data)
-		msg->data = pp->ops->get_msi_data(pp, data->hwirq);
-	else
-		msg->data = data->hwirq;
+	msg->data = data->hwirq;
 
 	dev_dbg(pci->dev, "msi#%d address_hi %#x address_lo %#x\n",
 		(int)data->hwirq, msg->address_hi, msg->address_lo);
@@ -157,17 +151,13 @@ static void dw_pci_bottom_mask(struct irq_data *data)
 
 	raw_spin_lock_irqsave(&pp->lock, flags);
 
-	if (pp->ops->msi_clear_irq) {
-		pp->ops->msi_clear_irq(pp, data->hwirq);
-	} else {
-		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
-		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
-		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
+	ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
+	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
+	bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
 
-		pp->irq_status[ctrl] &= ~(1 << bit);
-		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
-				    ~pp->irq_status[ctrl]);
-	}
+	pp->irq_status[ctrl] &= ~(1 << bit);
+	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
+			    ~pp->irq_status[ctrl]);
 
 	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
@@ -180,17 +170,13 @@ static void dw_pci_bottom_unmask(struct irq_data *data)
 
 	raw_spin_lock_irqsave(&pp->lock, flags);
 
-	if (pp->ops->msi_set_irq) {
-		pp->ops->msi_set_irq(pp, data->hwirq);
-	} else {
-		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
-		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
-		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
+	ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
+	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
+	bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
 
-		pp->irq_status[ctrl] |= 1 << bit;
-		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
-				    ~pp->irq_status[ctrl]);
-	}
+	pp->irq_status[ctrl] |= 1 << bit;
+	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
+			    ~pp->irq_status[ctrl]);
 
 	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
@@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d)
 
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit);
 
-	if (pp->ops->msi_irq_ack)
-		pp->ops->msi_irq_ack(d->hwirq, pp);
-
 	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 95e0c3c93f48..ea4b215b605d 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -142,14 +142,9 @@ struct dw_pcie_host_ops {
 	int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
 			     unsigned int devfn, int where, int size, u32 val);
 	int (*host_init)(struct pcie_port *pp);
-	void (*msi_set_irq)(struct pcie_port *pp, int irq);
-	void (*msi_clear_irq)(struct pcie_port *pp, int irq);
-	phys_addr_t (*get_msi_addr)(struct pcie_port *pp);
-	u32 (*get_msi_data)(struct pcie_port *pp, int pos);
 	void (*scan_bus)(struct pcie_port *pp);
 	void (*set_num_vectors)(struct pcie_port *pp);
 	int (*msi_host_init)(struct pcie_port *pp);
-	void (*msi_irq_ack)(int irq, struct pcie_port *pp);
 };
 
 struct pcie_port {
-- 
2.17.1

[PATCH v2 9/9] PCI: dwc: Do not write to MSI control registers if the platform doesn't use it

[Kishon Vijay Abraham I]

Platforms which populate msi_host_init, has it's own MSI controller
logic. Writing to MSI control registers on platforms which doesn't use
Designware's MSI controller logic might have side effects. To
be safe, do not write to MSI control registers if the platform uses
it's own MSI controller logic instead of Designware's MSI controller
logic.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 .../pci/controller/dwc/pcie-designware-host.c | 24 ++++++++++---------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 9492b05e8ff0..d7184e1a7d92 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -647,17 +647,19 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 
 	dw_pcie_setup(pci);
 
-	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
-
-	/* Initialize IRQ Status array */
-	for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
-		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK +
-					(ctrl * MSI_REG_CTRL_BLOCK_SIZE),
-				    4, ~0);
-		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
-					(ctrl * MSI_REG_CTRL_BLOCK_SIZE),
-				    4, ~0);
-		pp->irq_status[ctrl] = 0;
+	if (!pp->ops->msi_host_init) {
+		num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
+
+		/* Initialize IRQ Status array */
+		for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
+			dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK +
+					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
+					    4, ~0);
+			dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
+					    (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
+					    4, ~0);
+			pp->irq_status[ctrl] = 0;
+		}
 	}
 
 	/* Setup RC BARs */
-- 
2.17.1

Re: [PATCH v2 3/9] PCI: keystone: Add separate functions for configuring MSI and legacy interrupt

[Lorenzo Pieralisi]

On Thu, Feb 07, 2019 at 04:39:18PM +0530, Kishon Vijay Abraham I wrote:
> ks_pcie_get_irq_controller_info() was used to configure both MSI and
> legacy interrupt. This will prevent MSI or legacy interrupt specific
> intializations. Add separate functions to configure MSI and legacy
> interrupts.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 188 +++++++++++-----------
>  1 file changed, 96 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 4cf9849d5a1d..b1d01751c1af 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -87,11 +87,8 @@ struct keystone_pcie {
>  	struct dw_pcie		*pci;
>  	/* PCI Device ID */
>  	u32			device_id;
> -	int			num_legacy_host_irqs;
> -	int			legacy_host_irqs[PCI_NUM_INTX];
>  	struct			device_node *legacy_intc_np;
>  
> -	int			num_msi_host_irqs;
>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
>  	int			num_lanes;
>  	u32			num_viewport;
> @@ -201,14 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp)
>  	return dw_pcie_allocate_domains(pp);
>  }
>  
> -static void ks_pcie_enable_legacy_irqs(struct keystone_pcie *ks_pcie)
> -{
> -	int i;
> -
> -	for (i = 0; i < PCI_NUM_INTX; i++)
> -		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), 0x1);
> -}
> -
>  static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
>  				      int offset)
>  {
> @@ -490,17 +479,6 @@ static int __init ks_pcie_dw_host_init(struct keystone_pcie *ks_pcie)
>  
>  	ks_pcie->app = *res;
>  
> -	/* Create legacy IRQ domain */
> -	ks_pcie->legacy_irq_domain =
> -			irq_domain_add_linear(ks_pcie->legacy_intc_np,
> -					      PCI_NUM_INTX,
> -					      &ks_pcie_legacy_irq_domain_ops,
> -					      NULL);
> -	if (!ks_pcie->legacy_irq_domain) {
> -		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
> -		return -EINVAL;
> -	}
> -
>  	return dw_pcie_host_init(pp);
>  }
>  
> @@ -624,85 +602,117 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> -static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie,
> -					   char *controller, int *num_irqs)
> +static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
>  {
> -	int temp, max_host_irqs, legacy = 1, *host_irqs;
>  	struct device *dev = ks_pcie->pci->dev;
> -	struct device_node *np_pcie = dev->of_node, **np_temp;
> -
> -	if (!strcmp(controller, "msi-interrupt-controller"))
> -		legacy = 0;
> -
> -	if (legacy) {
> -		np_temp = &ks_pcie->legacy_intc_np;
> -		max_host_irqs = PCI_NUM_INTX;
> -		host_irqs = &ks_pcie->legacy_host_irqs[0];
> -	} else {
> -		np_temp = &ks_pcie->msi_intc_np;
> -		max_host_irqs = MAX_MSI_HOST_IRQS;
> -		host_irqs =  &ks_pcie->msi_host_irqs[0];
> -	}
> +	struct device_node *np = ks_pcie->np;
> +	struct device_node *intc_np;
> +	int irq_count;
> +	int irq;
> +	int ret;
> +	int i;

Nit: all int can be in one line.

> -	/* interrupt controller is in a child node */
> -	*np_temp = of_get_child_by_name(np_pcie, controller);
> -	if (!(*np_temp)) {
> -		dev_err(dev, "Node for %s is absent\n", controller);
> -		return -EINVAL;
> -	}
> +	if (!IS_ENABLED(CONFIG_PCI_MSI))
> +		return 0;
>  
> -	temp = of_irq_count(*np_temp);
> -	if (!temp) {
> -		dev_err(dev, "No IRQ entries in %s\n", controller);
> -		of_node_put(*np_temp);
> +	intc_np = of_get_child_by_name(np, "msi-interrupt-controller");
> +	if (!intc_np) {
> +		dev_WARN(dev, "msi-interrupt-controller node is absent\n");

I do not think you can justify a backtrace for this error path, so
convert it to a dev_warn() please.

>  		return -EINVAL;
>  	}
>  
> -	if (temp > max_host_irqs)
> -		dev_warn(dev, "Too many %s interrupts defined %u\n",
> -			(legacy ? "legacy" : "MSI"), temp);
> +	irq_count = of_irq_count(intc_np);
> +	if (!irq_count) {
> +		dev_err(dev, "No IRQ entries in msi-interrupt-controller\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
>  
> -	/*
> -	 * support upto max_host_irqs. In dt from index 0 to 3 (legacy) or 0 to
> -	 * 7 (MSI)
> -	 */
> -	for (temp = 0; temp < max_host_irqs; temp++) {
> -		host_irqs[temp] = irq_of_parse_and_map(*np_temp, temp);
> -		if (!host_irqs[temp])
> -			break;
> +	if (irq_count > MAX_MSI_HOST_IRQS) {
> +		dev_warn(dev, "Too many MSI interrupt lines defined %u\n",
> +			 irq_count);
> +		irq_count = MAX_MSI_HOST_IRQS;
>  	}
>  
> -	of_node_put(*np_temp);
> +	for (i = 0; i < irq_count; i++) {
> +		irq = irq_of_parse_and_map(intc_np, i);
> +		if (!irq) {
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +		ks_pcie->msi_host_irqs[i] = irq;
>  
> -	if (temp) {
> -		*num_irqs = temp;
> -		return 0;
> +		irq_set_chained_handler_and_data(irq, ks_pcie_msi_irq_handler,
> +						 ks_pcie);
>  	}
>  
> -	return -EINVAL;
> +	of_node_put(intc_np);
> +	return 0;
> +
> +err:
> +	of_node_put(intc_np);
> +	return ret;
>  }
>  
> -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
> +static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>  {
> +	struct device *dev = ks_pcie->pci->dev;
> +	struct irq_domain *legacy_irq_domain;
> +	struct device_node *np = ks_pcie->np;
> +	struct device_node *intc_np;
> +	int irq_count;
> +	int irq;
> +	int ret;
>  	int i;
>  
> -	/* Legacy IRQ */
> -	for (i = 0; i < ks_pcie->num_legacy_host_irqs; i++) {
> -		irq_set_chained_handler_and_data(ks_pcie->legacy_host_irqs[i],
> +	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
> +	if (!intc_np) {
> +		dev_WARN(dev, "legacy-interrupt-controller node is absent\n");

Likewise.

Lorenzo

> +		return -EINVAL;
> +	}
> +
> +	irq_count = of_irq_count(intc_np);
> +	if (!irq_count) {
> +		dev_err(dev, "No IRQ entries in legacy-interrupt-controller\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	for (i = 0; i < irq_count; i++) {
> +		irq = irq_of_parse_and_map(intc_np, i);
> +		if (!irq) {
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +		irq_set_chained_handler_and_data(irq,
>  						 ks_pcie_legacy_irq_handler,
>  						 ks_pcie);
>  	}
> -	ks_pcie_enable_legacy_irqs(ks_pcie);
> -
> -	/* MSI IRQ */
> -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> -		for (i = 0; i < ks_pcie->num_msi_host_irqs; i++) {
> -			irq_set_chained_handler_and_data(ks_pcie->msi_host_irqs[i],
> -							 ks_pcie_msi_irq_handler,
> -							 ks_pcie);
> -		}
> +
> +	legacy_irq_domain =
> +		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
> +				      &ks_pcie_legacy_irq_domain_ops, NULL);
> +	if (!legacy_irq_domain) {
> +		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
> +		ret = -EINVAL;
> +		goto err;
>  	}
> +	ks_pcie->legacy_irq_domain = legacy_irq_domain;
> +
> +	for (i = 0; i < PCI_NUM_INTX; i++)
> +		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), INTx_EN);
> +
> +	of_node_put(intc_np);
> +
> +	return 0;
> +
> +err:
> +	of_node_put(intc_np);
> +	return ret;
> +}
>  
> +static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
> +{
>  	if (ks_pcie->error_irq > 0)
>  		ks_pcie_enable_error_irq(ks_pcie);
>  }
> @@ -756,6 +766,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
>  	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
>  	int ret;
>  
> +	ret = ks_pcie_config_legacy_irq(ks_pcie);
> +	if (ret)
> +		return ret;
> +
> +	ret = ks_pcie_config_msi_irq(ks_pcie);
> +	if (ret)
> +		return ret;
> +
>  	dw_pcie_setup_rc(pp);
>  
>  	ks_pcie_establish_link(ks_pcie);
> @@ -805,20 +823,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	ret = ks_pcie_get_irq_controller_info(ks_pcie,
> -					"legacy-interrupt-controller",
> -					&ks_pcie->num_legacy_host_irqs);
> -	if (ret)
> -		return ret;
> -
> -	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> -		ret = ks_pcie_get_irq_controller_info(ks_pcie,
> -						"msi-interrupt-controller",
> -						&ks_pcie->num_msi_host_irqs);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	/*
>  	 * Index 0 is the platform interrupt for error interrupt
>  	 * from RC.  This is optional.
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/9] PCI: keystone: Modify legacy_irq_handler to check the IRQ_STATUS of INTA/B/C/D

[Lorenzo Pieralisi]

On Thu, Feb 07, 2019 at 04:39:17PM +0530, Kishon Vijay Abraham I wrote:
> The legacy interrupt handler directly checks the IRQ_STATUS register
> corresponding to a interrupt line inorder to invoke generic_handle_irq.
> 
> While this is okay for K2G platform which has separate interrupt line for
> each of the 4 legacy interrupts, AM654 which uses the same PCIe wrapper
> has a single interrupt line for all the legacy interrupts. So for AM654
> the interrupt handler won't be able to directly check the IRQ_STATUS
> register corresponding to the interrupt line.
> 
> Also the legacy interrupt handler uses 'virq' obtained from
> irq_of_parse_and_map to find the correct interrupt line which raised the
> interrupt. There is no guarantee that virq assigned for contiguous hardware
> irq will be contiguous and the interrupt handler might end up checking
> the wrong IRQ_STATUS register.
> 
> In order to overcome the above issues, read the IRQ_STATUS register of
> all the 4 legacy interrupts to determine which interrupt was raised.
> 
> Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 5286a480f76b..4cf9849d5a1d 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -214,16 +214,11 @@ static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
>  {
>  	struct dw_pcie *pci = ks_pcie->pci;
>  	struct device *dev = pci->dev;
> -	u32 pending;
>  	int virq;
>  
> -	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
> -
> -	if (BIT(0) & pending) {
> -		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
> -		dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
> -		generic_handle_irq(virq);
> -	}
> +	virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
> +	dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
> +	generic_handle_irq(virq);
>  
>  	/* EOI the INTx interrupt */
>  	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
> @@ -607,8 +602,9 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>  	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
>  	struct dw_pcie *pci = ks_pcie->pci;
>  	struct device *dev = pci->dev;
> -	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int irq_no;
> +	u32 reg;
>  
>  	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
>  
> @@ -618,7 +614,13 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>  	 * ack operation.
>  	 */
>  	chained_irq_enter(chip, desc);
> -	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
> +	for (irq_no = 0; irq_no < PCI_NUM_INTX; irq_no++) {

I understand the aim of this code but now on platforms where there
is a 1:1 relationship between Linux IRQ and INTX this loop has
steps carried out for nothing.

If I understand the code correctly what this code does is force
looping over INTX status regs regardless of what linux IRQ number was
actually active.

You could do something faster by creating a matrix LinuxIRQ x INTx to
detect what INTx status register should actually be checked.

This seems overkill to me but it is not that complicated to implement
and may clarify the code (and avoid reading up to three registers for
nothing on the IRQ code path, which can make things faster too).

Lorenzo

> +		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(irq_no));
> +		if (!(reg & INTx_EN))
> +			continue;
> +		ks_pcie_handle_legacy_irq(ks_pcie, irq_no);
> +	}
> +
>  	chained_irq_exit(chip, desc);
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH v2 6/9] PCI: dwc: Add support to use non default msi_irq_chip

[Lorenzo Pieralisi]

On Thu, Feb 07, 2019 at 04:39:21PM +0530, Kishon Vijay Abraham I wrote:
> Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for
> configuring the MSI controller logic within the Designware IP. However
> certain platforms like Keystone (K2G) which uses Desingware IP has
> it's own MSI controller logic. For handling such platforms,
> the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback
> functions.
> 
> Add support to use different msi_irq_chip with default as
> dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off
> msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific
> dw_pcie_host_ops. This will also help to get rid of get_msi_addr and
> get_msi_data ops.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 721d60a5d9e4..042de09b0451 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,
>  
>  	for (i = 0; i < nr_irqs; i++)
>  		irq_domain_set_info(domain, virq + i, bit + i,
> -				    &dw_pci_msi_bottom_irq_chip,
> +				    pp->msi_irq_chip,
>  				    pp, handle_edge_irq,
>  				    NULL, NULL);
>  
> @@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);
>  
> +	if (!pp->msi_irq_chip)
> +		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;

I think it is better to initialize pp->msi_irq_chip outside
dw_pcie_allocate_domains(), it makes things clearer.

In:

dw_pcie_host_init() for dwc

or

msi_host_init() for platforms with that hook implemented.

Is there any gotcha I am missing ?

Lorenzo

> +
>  	pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors,
>  					       &dw_pcie_msi_domain_ops, pp);
>  	if (!pp->irq_domain) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 1f56e6ae34ff..95e0c3c93f48 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -176,6 +176,7 @@ struct pcie_port {
>  	struct irq_domain	*irq_domain;
>  	struct irq_domain	*msi_domain;
>  	dma_addr_t		msi_data;
> +	struct irq_chip		*msi_irq_chip;
>  	u32			num_vectors;
>  	u32			irq_status[MAX_MSI_CTRLS];
>  	raw_spinlock_t		lock;
> -- 
> 2.17.1
> 

Re: [PATCH v2 8/9] PCI: dwc: Remove Keystone specific dw_pcie_host_ops

[Trent Piepho]

On Thu, 2019-02-07 at 16:39 +0530, Kishon Vijay Abraham I wrote:
> Now that Keystone started using it's own msi_irq_chip, remove
> Keystone specific callback function defined in dw_pcie_host_ops.
> 
> @@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d)
>  
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit);
>  
> -	if (pp->ops->msi_irq_ack)
> -		pp->ops->msi_irq_ack(d->hwirq, pp);
> -
>  	raw_spin_unlock_irqrestore(&pp->lock, flags);
>  }

After this patch, the entire function now looks like:

dw_pci_bottom_ack(struct irq_data *d)
{
        struct pcie_port *pp  = irq_data_get_irq_chip_data(d);
        unsigned int res, bit, ctrl;
        unsigned long flags;

        ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL;
        res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
        bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL;

        raw_spin_lock_irqsave(&pp->lock, flags);

        dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit);

        raw_spin_unlock_irqrestore(&pp->lock, flags);
}

The spin lock isn't necessary anymore since there is nothing but a
single register write inside the critical section.

Re: [PATCH v2 2/9] PCI: keystone: Modify legacy_irq_handler to check the IRQ_STATUS of INTA/B/C/D

[Bjorn Helgaas]

In the subject, "legacy_irq_handler" looks like it's supposed to be a
function name, but it's not.  Maybe something like:

  PCI: keystone: Check INTA/B/C/D IRQ_STATUS in ks_pcie_legacy_irq_handler()

On Thu, Feb 07, 2019 at 04:39:17PM +0530, Kishon Vijay Abraham I wrote:
> The legacy interrupt handler directly checks the IRQ_STATUS register
> corresponding to a interrupt line inorder to invoke generic_handle_irq.

s/The legacy interrupt handler/ks_pcie_handle_legacy_irq()/ ?
s/to a/to an/
s/inorder/in order/
s/generic_handle_irq/generic_handle_irq()/

> While this is okay for K2G platform which has separate interrupt line for
> each of the 4 legacy interrupts, AM654 which uses the same PCIe wrapper
> has a single interrupt line for all the legacy interrupts. So for AM654
> the interrupt handler won't be able to directly check the IRQ_STATUS
> register corresponding to the interrupt line.

s/platform which/platform, which/
s/separate interrupt line/separate interrupt lines/
s/AM654 which/AM654, which/
s/PCIe wrapper/PCIe wrapper,/
s/interrupt line for all/interrupt line shared by all/


> Also the legacy interrupt handler uses 'virq' obtained from
> irq_of_parse_and_map to find the correct interrupt line which raised the
> interrupt. There is no guarantee that virq assigned for contiguous hardware
> irq will be contiguous and the interrupt handler might end up checking
> the wrong IRQ_STATUS register.

s/irq_of_parse_and_map/irq_of_parse_and_map()
s/irq will/IRQ will/

> In order to overcome the above issues, read the IRQ_STATUS register of
> all the 4 legacy interrupts to determine which interrupt was raised.
> 
> Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 5286a480f76b..4cf9849d5a1d 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -214,16 +214,11 @@ static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
>  {
>  	struct dw_pcie *pci = ks_pcie->pci;
>  	struct device *dev = pci->dev;
> -	u32 pending;
>  	int virq;
>  
> -	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
> -
> -	if (BIT(0) & pending) {
> -		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
> -		dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
> -		generic_handle_irq(virq);
> -	}
> +	virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
> +	dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
> +	generic_handle_irq(virq);
>  
>  	/* EOI the INTx interrupt */
>  	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
> @@ -607,8 +602,9 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>  	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
>  	struct dw_pcie *pci = ks_pcie->pci;
>  	struct device *dev = pci->dev;
> -	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int irq_no;
> +	u32 reg;
>  
>  	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
>  
> @@ -618,7 +614,13 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>  	 * ack operation.
>  	 */
>  	chained_irq_enter(chip, desc);
> -	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
> +	for (irq_no = 0; irq_no < PCI_NUM_INTX; irq_no++) {
> +		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(irq_no));
> +		if (!(reg & INTx_EN))
> +			continue;
> +		ks_pcie_handle_legacy_irq(ks_pcie, irq_no);

It's too bad that reading IRQ_STATUS and writing IRQ_EOI are now in
separate functions.  It's nice to have them together for code auditing
purposes.

Could maybe accumulate a mask of which INTx bits are set and call
ks_pcie_handle_legacy_irq() only once with that mask?  Of course, then
you'd need another loop in ks_pcie_handle_legacy_irq().

> +	}
> +
>  	chained_irq_exit(chip, desc);
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH v2 6/9] PCI: dwc: Add support to use non default msi_irq_chip

[Bjorn Helgaas]

On Thu, Feb 07, 2019 at 04:39:21PM +0530, Kishon Vijay Abraham I wrote:
> Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for
> configuring the MSI controller logic within the Designware IP. However
> certain platforms like Keystone (K2G) which uses Desingware IP has
> it's own MSI controller logic. For handling such platforms,
> the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback
> functions.

s/IP uses/IP use/
s/Desingware/DesignWare/
s/Designware/DesignWare/ (several)
s/IP has/IP have/
s/it's own/their own/
s/ops uses/ops use/

> Add support to use different msi_irq_chip with default as
> dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off
> msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific
> dw_pcie_host_ops. This will also help to get rid of get_msi_addr and
> get_msi_data ops.

s/rid off/rid of/

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 721d60a5d9e4..042de09b0451 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,
>  
>  	for (i = 0; i < nr_irqs; i++)
>  		irq_domain_set_info(domain, virq + i, bit + i,
> -				    &dw_pci_msi_bottom_irq_chip,
> +				    pp->msi_irq_chip,
>  				    pp, handle_edge_irq,
>  				    NULL, NULL);
>  
> @@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);
>  
> +	if (!pp->msi_irq_chip)
> +		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;
> +
>  	pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors,
>  					       &dw_pcie_msi_domain_ops, pp);
>  	if (!pp->irq_domain) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 1f56e6ae34ff..95e0c3c93f48 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -176,6 +176,7 @@ struct pcie_port {
>  	struct irq_domain	*irq_domain;
>  	struct irq_domain	*msi_domain;
>  	dma_addr_t		msi_data;
> +	struct irq_chip		*msi_irq_chip;
>  	u32			num_vectors;
>  	u32			irq_status[MAX_MSI_CTRLS];
>  	raw_spinlock_t		lock;
> -- 
> 2.17.1
> 

Re: [PATCH v2 8/9] PCI: dwc: Remove Keystone specific dw_pcie_host_ops

[Bjorn Helgaas]

On Thu, Feb 07, 2019 at 04:39:23PM +0530, Kishon Vijay Abraham I wrote:
> Now that Keystone started using it's own msi_irq_chip, remove
> Keystone specific callback function defined in dw_pcie_host_ops.

s/it's/its/
s/callback function/callback functions/

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 45 ++++++-------------
>  drivers/pci/controller/dwc/pcie-designware.h  |  5 ---
>  2 files changed, 14 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 042de09b0451..9492b05e8ff0 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -126,18 +126,12 @@ static void dw_pci_setup_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	u64 msi_target;
>  
> -	if (pp->ops->get_msi_addr)
> -		msi_target = pp->ops->get_msi_addr(pp);
> -	else
> -		msi_target = (u64)pp->msi_data;
> +	msi_target = (u64)pp->msi_data;
>  
>  	msg->address_lo = lower_32_bits(msi_target);
>  	msg->address_hi = upper_32_bits(msi_target);
>  
> -	if (pp->ops->get_msi_data)
> -		msg->data = pp->ops->get_msi_data(pp, data->hwirq);
> -	else
> -		msg->data = data->hwirq;
> +	msg->data = data->hwirq;
>  
>  	dev_dbg(pci->dev, "msi#%d address_hi %#x address_lo %#x\n",
>  		(int)data->hwirq, msg->address_hi, msg->address_lo);
> @@ -157,17 +151,13 @@ static void dw_pci_bottom_mask(struct irq_data *data)
>  
>  	raw_spin_lock_irqsave(&pp->lock, flags);
>  
> -	if (pp->ops->msi_clear_irq) {
> -		pp->ops->msi_clear_irq(pp, data->hwirq);
> -	} else {
> -		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
> -		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> -		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
> +	ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
> +	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> +	bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
>  
> -		pp->irq_status[ctrl] &= ~(1 << bit);
> -		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
> -				    ~pp->irq_status[ctrl]);
> -	}
> +	pp->irq_status[ctrl] &= ~(1 << bit);
> +	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
> +			    ~pp->irq_status[ctrl]);
>  
>  	raw_spin_unlock_irqrestore(&pp->lock, flags);
>  }
> @@ -180,17 +170,13 @@ static void dw_pci_bottom_unmask(struct irq_data *data)
>  
>  	raw_spin_lock_irqsave(&pp->lock, flags);
>  
> -	if (pp->ops->msi_set_irq) {
> -		pp->ops->msi_set_irq(pp, data->hwirq);
> -	} else {
> -		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
> -		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> -		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
> +	ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
> +	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
> +	bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
>  
> -		pp->irq_status[ctrl] |= 1 << bit;
> -		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
> -				    ~pp->irq_status[ctrl]);
> -	}
> +	pp->irq_status[ctrl] |= 1 << bit;
> +	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
> +			    ~pp->irq_status[ctrl]);
>  
>  	raw_spin_unlock_irqrestore(&pp->lock, flags);
>  }
> @@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d)
>  
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit);
>  
> -	if (pp->ops->msi_irq_ack)
> -		pp->ops->msi_irq_ack(d->hwirq, pp);
> -
>  	raw_spin_unlock_irqrestore(&pp->lock, flags);
>  }
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 95e0c3c93f48..ea4b215b605d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -142,14 +142,9 @@ struct dw_pcie_host_ops {
>  	int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
>  			     unsigned int devfn, int where, int size, u32 val);
>  	int (*host_init)(struct pcie_port *pp);
> -	void (*msi_set_irq)(struct pcie_port *pp, int irq);
> -	void (*msi_clear_irq)(struct pcie_port *pp, int irq);
> -	phys_addr_t (*get_msi_addr)(struct pcie_port *pp);
> -	u32 (*get_msi_data)(struct pcie_port *pp, int pos);

I don't see the whole series on linux-pci (I only see patches 2, 6, 8, 9),
but I expected to somewhere see the removal of assignments to these
pointers.  It would be easier to review if the removal of assignments and
the removal of the function pointers from the structure were in the same
patch, but maybe that's not really feasible.

>  	void (*scan_bus)(struct pcie_port *pp);
>  	void (*set_num_vectors)(struct pcie_port *pp);
>  	int (*msi_host_init)(struct pcie_port *pp);
> -	void (*msi_irq_ack)(int irq, struct pcie_port *pp);
>  };
>  
>  struct pcie_port {
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/9] PCI: keystone: Modify legacy_irq_handler to check the IRQ_STATUS of INTA/B/C/D

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 07/02/19 9:45 PM, Lorenzo Pieralisi wrote:
> On Thu, Feb 07, 2019 at 04:39:17PM +0530, Kishon Vijay Abraham I wrote:
>> The legacy interrupt handler directly checks the IRQ_STATUS register
>> corresponding to a interrupt line inorder to invoke generic_handle_irq.
>>
>> While this is okay for K2G platform which has separate interrupt line for
>> each of the 4 legacy interrupts, AM654 which uses the same PCIe wrapper
>> has a single interrupt line for all the legacy interrupts. So for AM654
>> the interrupt handler won't be able to directly check the IRQ_STATUS
>> register corresponding to the interrupt line.
>>
>> Also the legacy interrupt handler uses 'virq' obtained from
>> irq_of_parse_and_map to find the correct interrupt line which raised the
>> interrupt. There is no guarantee that virq assigned for contiguous hardware
>> irq will be contiguous and the interrupt handler might end up checking
>> the wrong IRQ_STATUS register.
>>
>> In order to overcome the above issues, read the IRQ_STATUS register of
>> all the 4 legacy interrupts to determine which interrupt was raised.
>>
>> Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/dwc/pci-keystone.c | 22 ++++++++++++----------
>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 5286a480f76b..4cf9849d5a1d 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -214,16 +214,11 @@ static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
>>  {
>>  	struct dw_pcie *pci = ks_pcie->pci;
>>  	struct device *dev = pci->dev;
>> -	u32 pending;
>>  	int virq;
>>  
>> -	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
>> -
>> -	if (BIT(0) & pending) {
>> -		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
>> -		dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
>> -		generic_handle_irq(virq);
>> -	}
>> +	virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
>> +	dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
>> +	generic_handle_irq(virq);
>>  
>>  	/* EOI the INTx interrupt */
>>  	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
>> @@ -607,8 +602,9 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>>  	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
>>  	struct dw_pcie *pci = ks_pcie->pci;
>>  	struct device *dev = pci->dev;
>> -	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
>>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	unsigned int irq_no;
>> +	u32 reg;
>>  
>>  	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
>>  
>> @@ -618,7 +614,13 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>>  	 * ack operation.
>>  	 */
>>  	chained_irq_enter(chip, desc);
>> -	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
>> +	for (irq_no = 0; irq_no < PCI_NUM_INTX; irq_no++) {
> 
> I understand the aim of this code but now on platforms where there
> is a 1:1 relationship between Linux IRQ and INTX this loop has
> steps carried out for nothing.
> 
> If I understand the code correctly what this code does is force
> looping over INTX status regs regardless of what linux IRQ number was
> actually active.

right. This driver is used by 2 platforms K2G and AM654 (The patches are there
on the list). K2G has 4 interrupt lines for each of the 4 legacy interrups
while AM654 has a single interrupt line. One of the purpose of this patch is to
have a single legacy interrupt handler for both K2G and AM654.
> 
> You could do something faster by creating a matrix LinuxIRQ x INTx to
> detect what INTx status register should actually be checked.
> 
> This seems overkill to me but it is not that complicated to implement
> and may clarify the code (and avoid reading up to three registers for
> nothing on the IRQ code path, which can make things faster too).

Agreed. But that's not possible for AM654 which has a single interrupt line and
all the registers has to be read to identify the interrupt source.

Thanks
Kishon

Re: [PATCH v2 3/9] PCI: keystone: Add separate functions for configuring MSI and legacy interrupt

[Kishon Vijay Abraham I]

Hi,

On 07/02/19 9:14 PM, Lorenzo Pieralisi wrote:
> On Thu, Feb 07, 2019 at 04:39:18PM +0530, Kishon Vijay Abraham I wrote:
>> ks_pcie_get_irq_controller_info() was used to configure both MSI and
>> legacy interrupt. This will prevent MSI or legacy interrupt specific
>> intializations. Add separate functions to configure MSI and legacy
>> interrupts.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/dwc/pci-keystone.c | 188 +++++++++++-----------
>>  1 file changed, 96 insertions(+), 92 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 4cf9849d5a1d..b1d01751c1af 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -87,11 +87,8 @@ struct keystone_pcie {
>>  	struct dw_pcie		*pci;
>>  	/* PCI Device ID */
>>  	u32			device_id;
>> -	int			num_legacy_host_irqs;
>> -	int			legacy_host_irqs[PCI_NUM_INTX];
>>  	struct			device_node *legacy_intc_np;
>>  
>> -	int			num_msi_host_irqs;
>>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
>>  	int			num_lanes;
>>  	u32			num_viewport;
>> @@ -201,14 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp)
>>  	return dw_pcie_allocate_domains(pp);
>>  }
>>  
>> -static void ks_pcie_enable_legacy_irqs(struct keystone_pcie *ks_pcie)
>> -{
>> -	int i;
>> -
>> -	for (i = 0; i < PCI_NUM_INTX; i++)
>> -		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), 0x1);
>> -}
>> -
>>  static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
>>  				      int offset)
>>  {
>> @@ -490,17 +479,6 @@ static int __init ks_pcie_dw_host_init(struct keystone_pcie *ks_pcie)
>>  
>>  	ks_pcie->app = *res;
>>  
>> -	/* Create legacy IRQ domain */
>> -	ks_pcie->legacy_irq_domain =
>> -			irq_domain_add_linear(ks_pcie->legacy_intc_np,
>> -					      PCI_NUM_INTX,
>> -					      &ks_pcie_legacy_irq_domain_ops,
>> -					      NULL);
>> -	if (!ks_pcie->legacy_irq_domain) {
>> -		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
>> -		return -EINVAL;
>> -	}
>> -
>>  	return dw_pcie_host_init(pp);
>>  }
>>  
>> @@ -624,85 +602,117 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>>  	chained_irq_exit(chip, desc);
>>  }
>>  
>> -static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie,
>> -					   char *controller, int *num_irqs)
>> +static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
>>  {
>> -	int temp, max_host_irqs, legacy = 1, *host_irqs;
>>  	struct device *dev = ks_pcie->pci->dev;
>> -	struct device_node *np_pcie = dev->of_node, **np_temp;
>> -
>> -	if (!strcmp(controller, "msi-interrupt-controller"))
>> -		legacy = 0;
>> -
>> -	if (legacy) {
>> -		np_temp = &ks_pcie->legacy_intc_np;
>> -		max_host_irqs = PCI_NUM_INTX;
>> -		host_irqs = &ks_pcie->legacy_host_irqs[0];
>> -	} else {
>> -		np_temp = &ks_pcie->msi_intc_np;
>> -		max_host_irqs = MAX_MSI_HOST_IRQS;
>> -		host_irqs =  &ks_pcie->msi_host_irqs[0];
>> -	}
>> +	struct device_node *np = ks_pcie->np;
>> +	struct device_node *intc_np;
>> +	int irq_count;
>> +	int irq;
>> +	int ret;
>> +	int i;
> 
> Nit: all int can be in one line.

Okay.
> 
>> -	/* interrupt controller is in a child node */
>> -	*np_temp = of_get_child_by_name(np_pcie, controller);
>> -	if (!(*np_temp)) {
>> -		dev_err(dev, "Node for %s is absent\n", controller);
>> -		return -EINVAL;
>> -	}
>> +	if (!IS_ENABLED(CONFIG_PCI_MSI))
>> +		return 0;
>>  
>> -	temp = of_irq_count(*np_temp);
>> -	if (!temp) {
>> -		dev_err(dev, "No IRQ entries in %s\n", controller);
>> -		of_node_put(*np_temp);
>> +	intc_np = of_get_child_by_name(np, "msi-interrupt-controller");
>> +	if (!intc_np) {
>> +		dev_WARN(dev, "msi-interrupt-controller node is absent\n");
> 
> I do not think you can justify a backtrace for this error path, so
> convert it to a dev_warn() please.

Sure.
> 
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (temp > max_host_irqs)
>> -		dev_warn(dev, "Too many %s interrupts defined %u\n",
>> -			(legacy ? "legacy" : "MSI"), temp);
>> +	irq_count = of_irq_count(intc_np);
>> +	if (!irq_count) {
>> +		dev_err(dev, "No IRQ entries in msi-interrupt-controller\n");
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>>  
>> -	/*
>> -	 * support upto max_host_irqs. In dt from index 0 to 3 (legacy) or 0 to
>> -	 * 7 (MSI)
>> -	 */
>> -	for (temp = 0; temp < max_host_irqs; temp++) {
>> -		host_irqs[temp] = irq_of_parse_and_map(*np_temp, temp);
>> -		if (!host_irqs[temp])
>> -			break;
>> +	if (irq_count > MAX_MSI_HOST_IRQS) {
>> +		dev_warn(dev, "Too many MSI interrupt lines defined %u\n",
>> +			 irq_count);
>> +		irq_count = MAX_MSI_HOST_IRQS;
>>  	}
>>  
>> -	of_node_put(*np_temp);
>> +	for (i = 0; i < irq_count; i++) {
>> +		irq = irq_of_parse_and_map(intc_np, i);
>> +		if (!irq) {
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +		ks_pcie->msi_host_irqs[i] = irq;
>>  
>> -	if (temp) {
>> -		*num_irqs = temp;
>> -		return 0;
>> +		irq_set_chained_handler_and_data(irq, ks_pcie_msi_irq_handler,
>> +						 ks_pcie);
>>  	}
>>  
>> -	return -EINVAL;
>> +	of_node_put(intc_np);
>> +	return 0;
>> +
>> +err:
>> +	of_node_put(intc_np);
>> +	return ret;
>>  }
>>  
>> -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
>> +static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>>  {
>> +	struct device *dev = ks_pcie->pci->dev;
>> +	struct irq_domain *legacy_irq_domain;
>> +	struct device_node *np = ks_pcie->np;
>> +	struct device_node *intc_np;
>> +	int irq_count;
>> +	int irq;
>> +	int ret;
>>  	int i;
>>  
>> -	/* Legacy IRQ */
>> -	for (i = 0; i < ks_pcie->num_legacy_host_irqs; i++) {
>> -		irq_set_chained_handler_and_data(ks_pcie->legacy_host_irqs[i],
>> +	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
>> +	if (!intc_np) {
>> +		dev_WARN(dev, "legacy-interrupt-controller node is absent\n");
> 
> Likewise.

Okay.

Thanks
Kishon

Re: [PATCH v2 6/9] PCI: dwc: Add support to use non default msi_irq_chip

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 07/02/19 10:18 PM, Lorenzo Pieralisi wrote:
> On Thu, Feb 07, 2019 at 04:39:21PM +0530, Kishon Vijay Abraham I wrote:
>> Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for
>> configuring the MSI controller logic within the Designware IP. However
>> certain platforms like Keystone (K2G) which uses Desingware IP has
>> it's own MSI controller logic. For handling such platforms,
>> the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback
>> functions.
>>
>> Add support to use different msi_irq_chip with default as
>> dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off
>> msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific
>> dw_pcie_host_ops. This will also help to get rid of get_msi_addr and
>> get_msi_data ops.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
>>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 721d60a5d9e4..042de09b0451 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,
>>  
>>  	for (i = 0; i < nr_irqs; i++)
>>  		irq_domain_set_info(domain, virq + i, bit + i,
>> -				    &dw_pci_msi_bottom_irq_chip,
>> +				    pp->msi_irq_chip,
>>  				    pp, handle_edge_irq,
>>  				    NULL, NULL);
>>  
>> @@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
>>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>  	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);
>>  
>> +	if (!pp->msi_irq_chip)
>> +		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;
> 
> I think it is better to initialize pp->msi_irq_chip outside
> dw_pcie_allocate_domains(), it makes things clearer.
> 
> In:
> 
> dw_pcie_host_init() for dwc
> 
> or
> 
> msi_host_init() for platforms with that hook implemented.
> 
> Is there any gotcha I am missing ?

I added here only to avoid breaking "git bisect". Next patch adds
ks_pcie_msi_irq_chip in msi_host_init of keystone. However till then it has to
use dw_pci_msi_bottom_irq_chip. Adding anywhere else in dw_pcie_host_init would
mean msi_irq_chip is uninitialized for keystone.

Maybe I can add that in the commit log and move it to dw_pcie_host_init?

Thanks
Kishon

Re: [PATCH v2 8/9] PCI: dwc: Remove Keystone specific dw_pcie_host_ops

[Kishon Vijay Abraham I]

Hi,

On 08/02/19 2:56 AM, Bjorn Helgaas wrote:
> On Thu, Feb 07, 2019 at 04:39:23PM +0530, Kishon Vijay Abraham I wrote:
>> Now that Keystone started using it's own msi_irq_chip, remove
>> Keystone specific callback function defined in dw_pcie_host_ops.
> 
> s/it's/its/
> s/callback function/callback functions/

okay.
> 
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  .../pci/controller/dwc/pcie-designware-host.c | 45 ++++++-------------
>>  drivers/pci/controller/dwc/pcie-designware.h  |  5 ---
>>  2 files changed, 14 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 042de09b0451..9492b05e8ff0 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -126,18 +126,12 @@ static void dw_pci_setup_msi_msg(struct irq_data *data, struct msi_msg *msg)
>>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>  	u64 msi_target;
>>  
>> -	if (pp->ops->get_msi_addr)
>> -		msi_target = pp->ops->get_msi_addr(pp);
>> -	else
>> -		msi_target = (u64)pp->msi_data;
>> +	msi_target = (u64)pp->msi_data;
>>  
>>  	msg->address_lo = lower_32_bits(msi_target);
>>  	msg->address_hi = upper_32_bits(msi_target);
>>  
>> -	if (pp->ops->get_msi_data)
>> -		msg->data = pp->ops->get_msi_data(pp, data->hwirq);
>> -	else
>> -		msg->data = data->hwirq;
>> +	msg->data = data->hwirq;
>>  
>>  	dev_dbg(pci->dev, "msi#%d address_hi %#x address_lo %#x\n",
>>  		(int)data->hwirq, msg->address_hi, msg->address_lo);
>> @@ -157,17 +151,13 @@ static void dw_pci_bottom_mask(struct irq_data *data)
>>  
>>  	raw_spin_lock_irqsave(&pp->lock, flags);
>>  
>> -	if (pp->ops->msi_clear_irq) {
>> -		pp->ops->msi_clear_irq(pp, data->hwirq);
>> -	} else {
>> -		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
>> -		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
>> -		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
>> +	ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
>> +	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
>> +	bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
>>  
>> -		pp->irq_status[ctrl] &= ~(1 << bit);
>> -		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
>> -				    ~pp->irq_status[ctrl]);
>> -	}
>> +	pp->irq_status[ctrl] &= ~(1 << bit);
>> +	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
>> +			    ~pp->irq_status[ctrl]);
>>  
>>  	raw_spin_unlock_irqrestore(&pp->lock, flags);
>>  }
>> @@ -180,17 +170,13 @@ static void dw_pci_bottom_unmask(struct irq_data *data)
>>  
>>  	raw_spin_lock_irqsave(&pp->lock, flags);
>>  
>> -	if (pp->ops->msi_set_irq) {
>> -		pp->ops->msi_set_irq(pp, data->hwirq);
>> -	} else {
>> -		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
>> -		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
>> -		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
>> +	ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
>> +	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
>> +	bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
>>  
>> -		pp->irq_status[ctrl] |= 1 << bit;
>> -		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
>> -				    ~pp->irq_status[ctrl]);
>> -	}
>> +	pp->irq_status[ctrl] |= 1 << bit;
>> +	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4,
>> +			    ~pp->irq_status[ctrl]);
>>  
>>  	raw_spin_unlock_irqrestore(&pp->lock, flags);
>>  }
>> @@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d)
>>  
>>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit);
>>  
>> -	if (pp->ops->msi_irq_ack)
>> -		pp->ops->msi_irq_ack(d->hwirq, pp);
>> -
>>  	raw_spin_unlock_irqrestore(&pp->lock, flags);
>>  }
>>  
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 95e0c3c93f48..ea4b215b605d 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -142,14 +142,9 @@ struct dw_pcie_host_ops {
>>  	int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
>>  			     unsigned int devfn, int where, int size, u32 val);
>>  	int (*host_init)(struct pcie_port *pp);
>> -	void (*msi_set_irq)(struct pcie_port *pp, int irq);
>> -	void (*msi_clear_irq)(struct pcie_port *pp, int irq);
>> -	phys_addr_t (*get_msi_addr)(struct pcie_port *pp);
>> -	u32 (*get_msi_data)(struct pcie_port *pp, int pos);
> 
> I don't see the whole series on linux-pci (I only see patches 2, 6, 8, 9),

I do see all the patches here
https://patchwork.kernel.org/project/linux-pci/list/. Maybe it arrived little late?
> but I expected to somewhere see the removal of assignments to these
> pointers.  It would be easier to review if the removal of assignments and
> the removal of the function pointers from the structure were in the same
> patch, but maybe that's not really feasible.

The removal of the assignments are in 7th patch of the series (PCI: keystone:
Use Keystone specific msi_irq_chip). It's feasible but just wanted to keep the
keystone changes and DesignWare changes in separate patches.

Thanks
Kishon

Re: [PATCH v2 6/9] PCI: dwc: Add support to use non default msi_irq_chip

[Lorenzo Pieralisi]

On Fri, Feb 08, 2019 at 10:16:59AM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 07/02/19 10:18 PM, Lorenzo Pieralisi wrote:
> > On Thu, Feb 07, 2019 at 04:39:21PM +0530, Kishon Vijay Abraham I wrote:
> >> Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for
> >> configuring the MSI controller logic within the Designware IP. However
> >> certain platforms like Keystone (K2G) which uses Desingware IP has
> >> it's own MSI controller logic. For handling such platforms,
> >> the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback
> >> functions.
> >>
> >> Add support to use different msi_irq_chip with default as
> >> dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off
> >> msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific
> >> dw_pcie_host_ops. This will also help to get rid of get_msi_addr and
> >> get_msi_data ops.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
> >>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
> >>  2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> >> index 721d60a5d9e4..042de09b0451 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> >> @@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,
> >>  
> >>  	for (i = 0; i < nr_irqs; i++)
> >>  		irq_domain_set_info(domain, virq + i, bit + i,
> >> -				    &dw_pci_msi_bottom_irq_chip,
> >> +				    pp->msi_irq_chip,
> >>  				    pp, handle_edge_irq,
> >>  				    NULL, NULL);
> >>  
> >> @@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
> >>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>  	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);
> >>  
> >> +	if (!pp->msi_irq_chip)
> >> +		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;
> > 
> > I think it is better to initialize pp->msi_irq_chip outside
> > dw_pcie_allocate_domains(), it makes things clearer.
> > 
> > In:
> > 
> > dw_pcie_host_init() for dwc
> > 
> > or
> > 
> > msi_host_init() for platforms with that hook implemented.
> > 
> > Is there any gotcha I am missing ?
> 
> I added here only to avoid breaking "git bisect". Next patch adds
> ks_pcie_msi_irq_chip in msi_host_init of keystone. However till then it has to
> use dw_pci_msi_bottom_irq_chip. Adding anywhere else in dw_pcie_host_init would
> mean msi_irq_chip is uninitialized for keystone.
> 
> Maybe I can add that in the commit log and move it to dw_pcie_host_init?

I do not think you need to mention that in the commit log but move the
initialization of pp->msi_irq_chip in dw_pcie_host_init() in the next
patch, yes.

It is correct to keep bisectability, I should have read ahead.

Lorenzo

Re: [PATCH v2 2/9] PCI: keystone: Modify legacy_irq_handler to check the IRQ_STATUS of INTA/B/C/D

[Kishon Vijay Abraham I]

Hi Bjorn,

On 08/02/19 2:22 AM, Bjorn Helgaas wrote:
> In the subject, "legacy_irq_handler" looks like it's supposed to be a
> function name, but it's not.  Maybe something like:
> 
>   PCI: keystone: Check INTA/B/C/D IRQ_STATUS in ks_pcie_legacy_irq_handler()
> 
> On Thu, Feb 07, 2019 at 04:39:17PM +0530, Kishon Vijay Abraham I wrote:
>> The legacy interrupt handler directly checks the IRQ_STATUS register
>> corresponding to a interrupt line inorder to invoke generic_handle_irq.
> 
> s/The legacy interrupt handler/ks_pcie_handle_legacy_irq()/ ?
> s/to a/to an/
> s/inorder/in order/
> s/generic_handle_irq/generic_handle_irq()/
> 
>> While this is okay for K2G platform which has separate interrupt line for
>> each of the 4 legacy interrupts, AM654 which uses the same PCIe wrapper
>> has a single interrupt line for all the legacy interrupts. So for AM654
>> the interrupt handler won't be able to directly check the IRQ_STATUS
>> register corresponding to the interrupt line.
> 
> s/platform which/platform, which/
> s/separate interrupt line/separate interrupt lines/
> s/AM654 which/AM654, which/
> s/PCIe wrapper/PCIe wrapper,/
> s/interrupt line for all/interrupt line shared by all/
> 
> 
>> Also the legacy interrupt handler uses 'virq' obtained from
>> irq_of_parse_and_map to find the correct interrupt line which raised the
>> interrupt. There is no guarantee that virq assigned for contiguous hardware
>> irq will be contiguous and the interrupt handler might end up checking
>> the wrong IRQ_STATUS register.
> 
> s/irq_of_parse_and_map/irq_of_parse_and_map()
> s/irq will/IRQ will/
> 
>> In order to overcome the above issues, read the IRQ_STATUS register of
>> all the 4 legacy interrupts to determine which interrupt was raised.
>>
>> Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/dwc/pci-keystone.c | 22 ++++++++++++----------
>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 5286a480f76b..4cf9849d5a1d 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -214,16 +214,11 @@ static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
>>  {
>>  	struct dw_pcie *pci = ks_pcie->pci;
>>  	struct device *dev = pci->dev;
>> -	u32 pending;
>>  	int virq;
>>  
>> -	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
>> -
>> -	if (BIT(0) & pending) {
>> -		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
>> -		dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
>> -		generic_handle_irq(virq);
>> -	}
>> +	virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
>> +	dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
>> +	generic_handle_irq(virq);
>>  
>>  	/* EOI the INTx interrupt */
>>  	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
>> @@ -607,8 +602,9 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>>  	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
>>  	struct dw_pcie *pci = ks_pcie->pci;
>>  	struct device *dev = pci->dev;
>> -	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
>>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	unsigned int irq_no;
>> +	u32 reg;
>>  
>>  	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
>>  
>> @@ -618,7 +614,13 @@ static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
>>  	 * ack operation.
>>  	 */
>>  	chained_irq_enter(chip, desc);
>> -	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
>> +	for (irq_no = 0; irq_no < PCI_NUM_INTX; irq_no++) {
>> +		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(irq_no));
>> +		if (!(reg & INTx_EN))
>> +			continue;
>> +		ks_pcie_handle_legacy_irq(ks_pcie, irq_no);
> 
> It's too bad that reading IRQ_STATUS and writing IRQ_EOI are now in
> separate functions.  It's nice to have them together for code auditing
> purposes.
> 
> Could maybe accumulate a mask of which INTx bits are set and call
> ks_pcie_handle_legacy_irq() only once with that mask?  Of course, then
> you'd need another loop in ks_pcie_handle_legacy_irq().

Patch "5" of this series "PCI: keystone: Cleanup ks_pcie_msi_irq_handler and
ks_pcie_legacy_irq_handler" does more cleanup in irq handler and there
ks_pcie_handle_legacy_irq is removed and everything is done in a single function.

I have to anyway revisit legacy irq handler and have some of the register
writes move to proper irq_chip callbacks as Lorenzo commented in [1]

[1] -> https://lkml.org/lkml/2019/1/24/333

Thanks
Kishon

Re: [PATCH v2 8/9] PCI: dwc: Remove Keystone specific dw_pcie_host_ops

[Bjorn Helgaas]

On Fri, Feb 08, 2019 at 10:43:40AM +0530, Kishon Vijay Abraham I wrote:
> > I don't see the whole series on linux-pci (I only see patches 2, 6, 8, 9),
> 
> I do see all the patches here
> https://patchwork.kernel.org/project/linux-pci/list/. Maybe it arrived little late?

For some reason Gmail decided many of your recent messages were spam.
Sorry for the noise.

> > but I expected to somewhere see the removal of assignments to these
> > pointers.  It would be easier to review if the removal of assignments and
> > the removal of the function pointers from the structure were in the same
> > patch, but maybe that's not really feasible.
> 
> The removal of the assignments are in 7th patch of the series (PCI: keystone:
> Use Keystone specific msi_irq_chip). It's feasible but just wanted to keep the
> keystone changes and DesignWare changes in separate patches.

OK, makes sense.

Bjorn