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

[PATCH v4 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 v3:
*) Uses hierarchy IRQ domain for legacy interrupts since there is 1:1
   mapping between legacy interrupt and GIC IRQ.
   (MSI still depends on the order of IRQs populated in dt).

Changes from v2:
*) Removed patch that modifies ks_pcie_legacy_irq_handler() to check the
   IRQ_STATUS of INTA/B/C/D. Lorenzo's comment to create a matrix
   LinuxIRQ x INTx will be added in AM654x PCIe support series
*) ks_pcie_legacy_irq_handler() is made to use hwirq to get IRQ offset
   instead of virq.
*) default msi_irq_chip is assigned in dw_pcie_host_init() once keystone
   assigns its msi_irq_chip
*) Fixed other minor comments from Lorenzo and Bjorn

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: Add separate functions for configuring MSI and legacy
    interrupt
  PCI: keystone: Convert to using hierarchy domain for legacy interrupts
  PCI: keystone: Use hwirq to get the MSI IRQ number offset
  PCI: keystone: Cleanup ks_pcie_msi_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     | 520 ++++++++++--------
 .../pci/controller/dwc/pcie-designware-host.c |  78 +--
 drivers/pci/controller/dwc/pcie-designware.h  |   6 +-
 3 files changed, 326 insertions(+), 278 deletions(-)

-- 
2.17.1

[PATCH v4 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 v4 2/9] PCI: keystone: Add separate functions for configuring MSI and legacy interrupt

[Kishon Vijay Abraham I]

ks_pcie_get_irq_controller_info() is 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 | 181 +++++++++++-----------
 1 file changed, 89 insertions(+), 92 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 5286a480f76b..47f0dcf638f2 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -87,11 +87,9 @@ 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 +199,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)
 {
@@ -495,17 +485,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);
 }
 
@@ -622,85 +601,109 @@ 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, irq, ret, 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)
 {
-	int i;
+	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, irq, ret = 0, 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;
+	}
 
-	/* 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],
+	for (i = 0; i < irq_count; i++) {
+		irq = irq_of_parse_and_map(intc_np, i);
+		if (!irq) {
+			ret = -EINVAL;
+			goto err;
+		}
+		ks_pcie->legacy_host_irqs[i] = irq;
+
+		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);
+
+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);
 }
@@ -754,6 +757,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);
@@ -803,20 +814,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 v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Kishon Vijay Abraham I]

K2G provides separate IRQ lines for each of the four legacy interrupts.
Model this using hierarchy domain instead of linear domain with chained
IRQ handler.

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

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 47f0dcf638f2..7f1648453f54 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -61,6 +61,7 @@
 
 #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
 #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
+#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
 #define INTx_EN				BIT(0)
 
 #define ERR_IRQ_STATUS			0x1c4
@@ -87,7 +88,6 @@ struct keystone_pcie {
 	struct dw_pcie		*pci;
 	/* PCI Device ID */
 	u32			device_id;
-	int			legacy_host_irqs[PCI_NUM_INTX];
 	struct			device_node *legacy_intc_np;
 
 	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
@@ -96,7 +96,6 @@ struct keystone_pcie {
 	struct phy		**phy;
 	struct device_link	**link;
 	struct			device_node *msi_intc_np;
-	struct irq_domain	*legacy_irq_domain;
 	struct device_node	*np;
 
 	int error_irq;
@@ -199,26 +198,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;
-	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);
-	}
-
-	/* 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);
@@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
 	return IRQ_HANDLED;
 }
 
-static void ks_pcie_ack_legacy_irq(struct irq_data *d)
+void ks_pcie_irq_eoi(struct irq_data *data)
 {
+	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
+	irq_hw_number_t hwirq = data->hwirq;
+
+	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
+	irq_chip_eoi_parent(data);
 }
 
-static void ks_pcie_mask_legacy_irq(struct irq_data *d)
+void ks_pcie_irq_enable(struct irq_data *data)
 {
+	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
+	irq_hw_number_t hwirq = data->hwirq;
+
+	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
+	irq_chip_enable_parent(data);
 }
 
-static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
+void ks_pcie_irq_disable(struct irq_data *data)
 {
+	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
+	irq_hw_number_t hwirq = data->hwirq;
+
+	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
+	irq_chip_disable_parent(data);
 }
 
 static struct irq_chip ks_pcie_legacy_irq_chip = {
-	.name = "Keystone-PCI-Legacy-IRQ",
-	.irq_ack = ks_pcie_ack_legacy_irq,
-	.irq_mask = ks_pcie_mask_legacy_irq,
-	.irq_unmask = ks_pcie_unmask_legacy_irq,
+	.name			= "Keystone-PCI-Legacy-IRQ",
+	.irq_enable		= ks_pcie_irq_enable,
+	.irq_disable		= ks_pcie_irq_disable,
+	.irq_eoi		= ks_pcie_irq_eoi,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
 };
 
-static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
-				       unsigned int irq,
-				       irq_hw_number_t hw_irq)
+static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
+					   unsigned int virq,
+					   unsigned int nr_irqs, void *data)
 {
-	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
-				 handle_level_irq);
-	irq_set_chip_data(irq, d->host_data);
+	struct keystone_pcie *ks_pcie = domain->host_data;
+	struct device_node *np = ks_pcie->legacy_intc_np;
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct of_phandle_args out_irq;
+	int ret, i;
+
+	if (nr_irqs != 1)
+		return -EINVAL;
+
+	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
+	if (ret < 0) {
+		pr_err("Failed to parse interrupt node\n");
+		return ret;
+	}
+
+	parent_fwspec.fwnode = &out_irq.np->fwnode;
+	parent_fwspec.param_count = out_irq.args_count;
+
+	for (i = 0; i < out_irq.args_count; i++)
+		parent_fwspec.param[i] = out_irq.args[i];
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
+	if (ret < 0) {
+		pr_err("Failed to allocate parent irq %u: %d\n",
+		       parent_fwspec.param[0], ret);
+		return ret;
+	}
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
+					    &ks_pcie_legacy_irq_chip, ks_pcie);
+	if (ret < 0) {
+		pr_err("Failed to set hwirq and chip\n");
+		goto err_set_hwirq_and_chip;
+	}
 
 	return 0;
+
+err_set_hwirq_and_chip:
+	irq_domain_free_irqs_parent(domain, virq, 1);
+
+	return ret;
+}
+
+static int ks_pcie_irq_domain_translate(struct irq_domain *domain,
+					struct irq_fwspec *fwspec,
+					unsigned long *hwirq,
+					unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+
+		if (fwspec->param[0] >= PCI_NUM_INTX)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1];
+
+		return 0;
+	}
+
+	return -EINVAL;
 }
 
 static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = {
-	.map = ks_pcie_init_legacy_irq_map,
-	.xlate = irq_domain_xlate_onetwocell,
+	.alloc		= ks_pcie_legacy_irq_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+	.translate	= ks_pcie_irq_domain_translate,
 };
 
 /**
@@ -572,35 +629,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-/**
- * ks_pcie_legacy_irq_handler() - Handle legacy interrupt
- * @irq: IRQ line for legacy interrupts
- * @desc: Pointer to irq descriptor
- *
- * Traverse through pending legacy interrupts and invoke handler for each. Also
- * takes care of interrupt controller level mask/ack operation.
- */
-static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
-{
-	unsigned int irq = irq_desc_get_irq(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);
-
-	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
-
-	/*
-	 * The chained irq handler installation would have replaced normal
-	 * interrupt driver handler so we need to take care of mask/unmask and
-	 * ack operation.
-	 */
-	chained_irq_enter(chip, desc);
-	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
-	chained_irq_exit(chip, desc);
-}
-
 static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 {
 	struct device *dev = ks_pcie->pci->dev;
@@ -655,14 +683,33 @@ 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 irq_domain *parent_domain;
+	struct device_node *parent_node;
 	struct device_node *intc_np;
-	int irq_count, irq, ret = 0, i;
+	int irq_count, ret = 0;
 
 	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;
 	}
+	ks_pcie->legacy_intc_np = intc_np;
+
+	parent_node = of_irq_find_parent(intc_np);
+	if (!parent_node) {
+		dev_err(dev, "unable to obtain parent node\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain) {
+		dev_err(dev, "unable to obtain parent domain\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	of_node_put(parent_node);
 
 	irq_count = of_irq_count(intc_np);
 	if (!irq_count) {
@@ -671,31 +718,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 		goto err;
 	}
 
-	for (i = 0; i < irq_count; i++) {
-		irq = irq_of_parse_and_map(intc_np, i);
-		if (!irq) {
-			ret = -EINVAL;
-			goto err;
-		}
-		ks_pcie->legacy_host_irqs[i] = irq;
-
-		irq_set_chained_handler_and_data(irq,
-						 ks_pcie_legacy_irq_handler,
-						 ks_pcie);
-	}
-
 	legacy_irq_domain =
-		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
-				      &ks_pcie_legacy_irq_domain_ops, NULL);
+		irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX,
+					 intc_np,
+					 &ks_pcie_legacy_irq_domain_ops,
+					 ks_pcie);
 	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);
 
 err:
 	of_node_put(intc_np);
-- 
2.17.1

[PATCH v4 4/9] PCI: keystone: Use hwirq to get the MSI 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 7f1648453f54..0ebb8622fddb 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -75,7 +75,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
@@ -90,7 +89,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;
@@ -610,9 +609,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);
@@ -634,6 +633,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, irq, ret, i;
 
 	if (!IS_ENABLED(CONFIG_PCI_MSI))
@@ -652,19 +652,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 v4 5/9] PCI: keystone: Cleanup ks_pcie_msi_irq_handler

[Kishon Vijay Abraham I]

ks_pcie_msi_irq_handler() invokes ks_pcie_handle_msi_irq() 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 MSI interrupt in a single interrupt handler here.

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

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 0ebb8622fddb..16feab853a7b 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));
 }
@@ -613,8 +587,10 @@ 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, virq, reg, pos;
 
 	dev_dbg(dev, "%s, irq %d\n", __func__, irq);
 
@@ -624,7 +600,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);
 }
 
-- 
2.17.1

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

[Kishon Vijay Abraham I]

Platforms using DesignWare IP use dw_pci_msi_bottom_irq_chip for
configuring the MSI controller logic within the DesignWare IP. However
certain platforms like Keystone (K2G) which uses DesignWare IP have
their own MSI controller logic. For handling such platforms,
the irqchip ops use 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 of
msi_irq_ack(), msi_set_irq(), msi_clear_irq() and other Keystone
specific dw_pcie_host_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 9943d8c68335..cb6eeb062f47 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 v4 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.

Move the default msi_irq_chip assignment to dw_pcie_host_init since
platforms that doesn't use the default msi_irq_chip will assign
msi_irq_chip in msi_host_init() callback.

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

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 16feab853a7b..88a60e1a1486 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);
 }
 
@@ -816,11 +868,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,
 };
 
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 042de09b0451..72cfedce5af1 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -277,9 +277,6 @@ 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) {
@@ -462,6 +459,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		}
 
 		if (!pp->ops->msi_host_init) {
+			pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;
+
 			ret = dw_pcie_allocate_domains(pp);
 			if (ret)
 				goto error;
-- 
2.17.1

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

[Kishon Vijay Abraham I]

Now that Keystone started using its own msi_irq_chip, remove
Keystone specific callback functions defined in dw_pcie_host_ops.

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

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 72cfedce5af1..781735f06dea 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);
 }
@@ -199,20 +185,12 @@ static void 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);
-
-	if (pp->ops->msi_irq_ack)
-		pp->ops->msi_irq_ack(d->hwirq, pp);
-
-	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
 static struct irq_chip dw_pci_msi_bottom_irq_chip = {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index cb6eeb062f47..ac75cde80a0c 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 v4 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>
---
 .../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 781735f06dea..2bc2fd582124 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -641,17 +641,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 v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Lorenzo Pieralisi]

On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote:
> K2G provides separate IRQ lines for each of the four legacy interrupts.
> Model this using hierarchy domain instead of linear domain with chained
> IRQ handler.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++---------
>  1 file changed, 118 insertions(+), 87 deletions(-)

Hi Kishon,

I CC'ed Marc because you are actually re-writing an interrupt controller
driver so I would be happier to merge this refactoring if Marc can have
a look and he is satisfied with it - more so because most of the code can
be reused by other host bridge drivers with similar behaviour.

I will have a look too, unfortunately it is becoming a bit tight for
v5.1 but let's see how it goes.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 47f0dcf638f2..7f1648453f54 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -61,6 +61,7 @@
>  
>  #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
>  #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
> +#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
>  #define INTx_EN				BIT(0)
>  
>  #define ERR_IRQ_STATUS			0x1c4
> @@ -87,7 +88,6 @@ struct keystone_pcie {
>  	struct dw_pcie		*pci;
>  	/* PCI Device ID */
>  	u32			device_id;
> -	int			legacy_host_irqs[PCI_NUM_INTX];
>  	struct			device_node *legacy_intc_np;
>  
>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
> @@ -96,7 +96,6 @@ struct keystone_pcie {
>  	struct phy		**phy;
>  	struct device_link	**link;
>  	struct			device_node *msi_intc_np;
> -	struct irq_domain	*legacy_irq_domain;
>  	struct device_node	*np;
>  
>  	int error_irq;
> @@ -199,26 +198,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;
> -	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);
> -	}
> -
> -	/* 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);
> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
>  	return IRQ_HANDLED;
>  }
>  
> -static void ks_pcie_ack_legacy_irq(struct irq_data *d)
> +void ks_pcie_irq_eoi(struct irq_data *data)
>  {
> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> +	irq_hw_number_t hwirq = data->hwirq;
> +
> +	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
> +	irq_chip_eoi_parent(data);
>  }
>  
> -static void ks_pcie_mask_legacy_irq(struct irq_data *d)
> +void ks_pcie_irq_enable(struct irq_data *data)
>  {
> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> +	irq_hw_number_t hwirq = data->hwirq;
> +
> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
> +	irq_chip_enable_parent(data);
>  }
>  
> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
> +void ks_pcie_irq_disable(struct irq_data *data)
>  {
> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> +	irq_hw_number_t hwirq = data->hwirq;
> +
> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
> +	irq_chip_disable_parent(data);
>  }
>  
>  static struct irq_chip ks_pcie_legacy_irq_chip = {
> -	.name = "Keystone-PCI-Legacy-IRQ",
> -	.irq_ack = ks_pcie_ack_legacy_irq,
> -	.irq_mask = ks_pcie_mask_legacy_irq,
> -	.irq_unmask = ks_pcie_unmask_legacy_irq,
> +	.name			= "Keystone-PCI-Legacy-IRQ",
> +	.irq_enable		= ks_pcie_irq_enable,
> +	.irq_disable		= ks_pcie_irq_disable,
> +	.irq_eoi		= ks_pcie_irq_eoi,
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_set_type		= irq_chip_set_type_parent,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>  };
>  
> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
> -				       unsigned int irq,
> -				       irq_hw_number_t hw_irq)
> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
> +					   unsigned int virq,
> +					   unsigned int nr_irqs, void *data)
>  {
> -	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
> -				 handle_level_irq);
> -	irq_set_chip_data(irq, d->host_data);
> +	struct keystone_pcie *ks_pcie = domain->host_data;
> +	struct device_node *np = ks_pcie->legacy_intc_np;
> +	struct irq_fwspec parent_fwspec, *fwspec = data;
> +	struct of_phandle_args out_irq;
> +	int ret, i;
> +
> +	if (nr_irqs != 1)
> +		return -EINVAL;
> +
> +	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
> +	if (ret < 0) {
> +		pr_err("Failed to parse interrupt node\n");
> +		return ret;
> +	}
> +
> +	parent_fwspec.fwnode = &out_irq.np->fwnode;
> +	parent_fwspec.param_count = out_irq.args_count;
> +
> +	for (i = 0; i < out_irq.args_count; i++)
> +		parent_fwspec.param[i] = out_irq.args[i];
> +
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> +	if (ret < 0) {
> +		pr_err("Failed to allocate parent irq %u: %d\n",
> +		       parent_fwspec.param[0], ret);
> +		return ret;
> +	}
> +
> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
> +					    &ks_pcie_legacy_irq_chip, ks_pcie);
> +	if (ret < 0) {
> +		pr_err("Failed to set hwirq and chip\n");
> +		goto err_set_hwirq_and_chip;
> +	}
>  
>  	return 0;
> +
> +err_set_hwirq_and_chip:
> +	irq_domain_free_irqs_parent(domain, virq, 1);
> +
> +	return ret;
> +}
> +
> +static int ks_pcie_irq_domain_translate(struct irq_domain *domain,
> +					struct irq_fwspec *fwspec,
> +					unsigned long *hwirq,
> +					unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 2)
> +			return -EINVAL;
> +
> +		if (fwspec->param[0] >= PCI_NUM_INTX)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[0];
> +		*type = fwspec->param[1];
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
>  }
>  
>  static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = {
> -	.map = ks_pcie_init_legacy_irq_map,
> -	.xlate = irq_domain_xlate_onetwocell,
> +	.alloc		= ks_pcie_legacy_irq_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +	.translate	= ks_pcie_irq_domain_translate,
>  };
>  
>  /**
> @@ -572,35 +629,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> -/**
> - * ks_pcie_legacy_irq_handler() - Handle legacy interrupt
> - * @irq: IRQ line for legacy interrupts
> - * @desc: Pointer to irq descriptor
> - *
> - * Traverse through pending legacy interrupts and invoke handler for each. Also
> - * takes care of interrupt controller level mask/ack operation.
> - */
> -static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> -{
> -	unsigned int irq = irq_desc_get_irq(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);
> -
> -	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
> -
> -	/*
> -	 * The chained irq handler installation would have replaced normal
> -	 * interrupt driver handler so we need to take care of mask/unmask and
> -	 * ack operation.
> -	 */
> -	chained_irq_enter(chip, desc);
> -	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
> -	chained_irq_exit(chip, desc);
> -}
> -
>  static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
>  {
>  	struct device *dev = ks_pcie->pci->dev;
> @@ -655,14 +683,33 @@ 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 irq_domain *parent_domain;
> +	struct device_node *parent_node;
>  	struct device_node *intc_np;
> -	int irq_count, irq, ret = 0, i;
> +	int irq_count, ret = 0;
>  
>  	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;
>  	}
> +	ks_pcie->legacy_intc_np = intc_np;
> +
> +	parent_node = of_irq_find_parent(intc_np);
> +	if (!parent_node) {
> +		dev_err(dev, "unable to obtain parent node\n");
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	parent_domain = irq_find_host(parent_node);
> +	if (!parent_domain) {
> +		dev_err(dev, "unable to obtain parent domain\n");
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	of_node_put(parent_node);
>  
>  	irq_count = of_irq_count(intc_np);
>  	if (!irq_count) {
> @@ -671,31 +718,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>  		goto err;
>  	}
>  
> -	for (i = 0; i < irq_count; i++) {
> -		irq = irq_of_parse_and_map(intc_np, i);
> -		if (!irq) {
> -			ret = -EINVAL;
> -			goto err;
> -		}
> -		ks_pcie->legacy_host_irqs[i] = irq;
> -
> -		irq_set_chained_handler_and_data(irq,
> -						 ks_pcie_legacy_irq_handler,
> -						 ks_pcie);
> -	}
> -
>  	legacy_irq_domain =
> -		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
> -				      &ks_pcie_legacy_irq_domain_ops, NULL);
> +		irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX,
> +					 intc_np,
> +					 &ks_pcie_legacy_irq_domain_ops,
> +					 ks_pcie);
>  	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);
>  
>  err:
>  	of_node_put(intc_np);
> -- 
> 2.17.1
> 

Re: [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Marc Zyngier]

On Thu, 21 Feb 2019 16:24:14 +0000
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote:
> > K2G provides separate IRQ lines for each of the four legacy interrupts.
> > Model this using hierarchy domain instead of linear domain with chained
> > IRQ handler.
> > 
> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++---------
> >  1 file changed, 118 insertions(+), 87 deletions(-)  
> 
> Hi Kishon,
> 
> I CC'ed Marc because you are actually re-writing an interrupt controller
> driver so I would be happier to merge this refactoring if Marc can have
> a look and he is satisfied with it - more so because most of the code can
> be reused by other host bridge drivers with similar behaviour.

Cheers Lorenzo.

It doesn't look too bad, but there is a couple of points I'd like to see
clarified. Comments below.

> 
> I will have a look too, unfortunately it is becoming a bit tight for
> v5.1 but let's see how it goes.
> 
> Thanks,
> Lorenzo
> 
> > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> > index 47f0dcf638f2..7f1648453f54 100644
> > --- a/drivers/pci/controller/dwc/pci-keystone.c
> > +++ b/drivers/pci/controller/dwc/pci-keystone.c
> > @@ -61,6 +61,7 @@
> >  
> >  #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
> >  #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
> > +#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
> >  #define INTx_EN				BIT(0)
> >  
> >  #define ERR_IRQ_STATUS			0x1c4
> > @@ -87,7 +88,6 @@ struct keystone_pcie {
> >  	struct dw_pcie		*pci;
> >  	/* PCI Device ID */
> >  	u32			device_id;
> > -	int			legacy_host_irqs[PCI_NUM_INTX];
> >  	struct			device_node *legacy_intc_np;
> >  
> >  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
> > @@ -96,7 +96,6 @@ struct keystone_pcie {
> >  	struct phy		**phy;
> >  	struct device_link	**link;
> >  	struct			device_node *msi_intc_np;
> > -	struct irq_domain	*legacy_irq_domain;
> >  	struct device_node	*np;
> >  
> >  	int error_irq;
> > @@ -199,26 +198,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;
> > -	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);
> > -	}
> > -
> > -	/* 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);
> > @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -static void ks_pcie_ack_legacy_irq(struct irq_data *d)
> > +void ks_pcie_irq_eoi(struct irq_data *data)
> >  {
> > +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> > +	irq_hw_number_t hwirq = data->hwirq;
> > +
> > +	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
> > +	irq_chip_eoi_parent(data);
> >  }
> >  
> > -static void ks_pcie_mask_legacy_irq(struct irq_data *d)
> > +void ks_pcie_irq_enable(struct irq_data *data)
> >  {
> > +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> > +	irq_hw_number_t hwirq = data->hwirq;
> > +
> > +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
> > +	irq_chip_enable_parent(data);
> >  }
> >  
> > -static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
> > +void ks_pcie_irq_disable(struct irq_data *data)
> >  {
> > +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> > +	irq_hw_number_t hwirq = data->hwirq;
> > +
> > +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
> > +	irq_chip_disable_parent(data);
> >  }
> >  
> >  static struct irq_chip ks_pcie_legacy_irq_chip = {
> > -	.name = "Keystone-PCI-Legacy-IRQ",
> > -	.irq_ack = ks_pcie_ack_legacy_irq,
> > -	.irq_mask = ks_pcie_mask_legacy_irq,
> > -	.irq_unmask = ks_pcie_unmask_legacy_irq,
> > +	.name			= "Keystone-PCI-Legacy-IRQ",
> > +	.irq_enable		= ks_pcie_irq_enable,
> > +	.irq_disable		= ks_pcie_irq_disable,
> > +	.irq_eoi		= ks_pcie_irq_eoi,
> > +	.irq_mask		= irq_chip_mask_parent,
> > +	.irq_unmask		= irq_chip_unmask_parent,
> > +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> > +	.irq_set_type		= irq_chip_set_type_parent,
> > +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> >  };
> >  
> > -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
> > -				       unsigned int irq,
> > -				       irq_hw_number_t hw_irq)
> > +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
> > +					   unsigned int virq,
> > +					   unsigned int nr_irqs, void *data)
> >  {
> > -	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
> > -				 handle_level_irq);
> > -	irq_set_chip_data(irq, d->host_data);
> > +	struct keystone_pcie *ks_pcie = domain->host_data;
> > +	struct device_node *np = ks_pcie->legacy_intc_np;
> > +	struct irq_fwspec parent_fwspec, *fwspec = data;
> > +	struct of_phandle_args out_irq;
> > +	int ret, i;
> > +
> > +	if (nr_irqs != 1)
> > +		return -EINVAL;
> > +
> > +	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
> > +	if (ret < 0) {
> > +		pr_err("Failed to parse interrupt node\n");
> > +		return ret;
> > +	}

What it this trying to do? Fishing out the interrupts from DT based on
the legacy pin? This looks at best obscure. I wonder why you don't do
that at probe time instead. Anyway, this requires documenting.

> > +
> > +	parent_fwspec.fwnode = &out_irq.np->fwnode;
> > +	parent_fwspec.param_count = out_irq.args_count;
> > +
> > +	for (i = 0; i < out_irq.args_count; i++)
> > +		parent_fwspec.param[i] = out_irq.args[i];

This feels like a duplicate of of_phandle_args_to_fwspec(). If you need
such a helper, please export it from the irqdomain code.

> > +
> > +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> > +	if (ret < 0) {
> > +		pr_err("Failed to allocate parent irq %u: %d\n",
> > +		       parent_fwspec.param[0], ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
> > +					    &ks_pcie_legacy_irq_chip, ks_pcie);
> > +	if (ret < 0) {
> > +		pr_err("Failed to set hwirq and chip\n");
> > +		goto err_set_hwirq_and_chip;
> > +	}
> >  
> >  	return 0;
> > +
> > +err_set_hwirq_and_chip:
> > +	irq_domain_free_irqs_parent(domain, virq, 1);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ks_pcie_irq_domain_translate(struct irq_domain *domain,
> > +					struct irq_fwspec *fwspec,
> > +					unsigned long *hwirq,
> > +					unsigned int *type)
> > +{
> > +	if (is_of_node(fwspec->fwnode)) {

If you don't plan to support ACPI, you can drop this.

> > +		if (fwspec->param_count != 2)
> > +			return -EINVAL;

From the DT binding:

pcie_intc: Interrupt controller device node for Legacy IRQ chip
        interrupt-cells: should be set to 1

So why do we have 2 cells here?

> > +
> > +		if (fwspec->param[0] >= PCI_NUM_INTX)
> > +			return -EINVAL;

Most of the OF code assumes that the pin number describing the legacy
interrupt is 1-based, while you obviously treat it as 0-based. How does
it work?

> > +
> > +		*hwirq = fwspec->param[0];
> > +		*type = fwspec->param[1];

As far as I remember, PCI legacy interrupts are level triggered, so
there should be no need to advertise the trigger (which is consistent
with the way the binding is written).

> > +
> > +		return 0;
> > +	}
> > +
> > +	return -EINVAL;
> >  }
> >  
> >  static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = {
> > -	.map = ks_pcie_init_legacy_irq_map,
> > -	.xlate = irq_domain_xlate_onetwocell,
> > +	.alloc		= ks_pcie_legacy_irq_domain_alloc,
> > +	.free		= irq_domain_free_irqs_common,
> > +	.translate	= ks_pcie_irq_domain_translate,
> >  };
> >  
> >  /**
> > @@ -572,35 +629,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
> >  	chained_irq_exit(chip, desc);
> >  }
> >  
> > -/**
> > - * ks_pcie_legacy_irq_handler() - Handle legacy interrupt
> > - * @irq: IRQ line for legacy interrupts
> > - * @desc: Pointer to irq descriptor
> > - *
> > - * Traverse through pending legacy interrupts and invoke handler for each. Also
> > - * takes care of interrupt controller level mask/ack operation.
> > - */
> > -static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> > -{
> > -	unsigned int irq = irq_desc_get_irq(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);
> > -
> > -	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
> > -
> > -	/*
> > -	 * The chained irq handler installation would have replaced normal
> > -	 * interrupt driver handler so we need to take care of mask/unmask and
> > -	 * ack operation.
> > -	 */
> > -	chained_irq_enter(chip, desc);
> > -	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
> > -	chained_irq_exit(chip, desc);
> > -}
> > -
> >  static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
> >  {
> >  	struct device *dev = ks_pcie->pci->dev;
> > @@ -655,14 +683,33 @@ 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 irq_domain *parent_domain;
> > +	struct device_node *parent_node;
> >  	struct device_node *intc_np;
> > -	int irq_count, irq, ret = 0, i;
> > +	int irq_count, ret = 0;
> >  
> >  	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;
> >  	}
> > +	ks_pcie->legacy_intc_np = intc_np;
> > +
> > +	parent_node = of_irq_find_parent(intc_np);
> > +	if (!parent_node) {
> > +		dev_err(dev, "unable to obtain parent node\n");
> > +		ret = -ENXIO;
> > +		goto err;
> > +	}
> > +
> > +	parent_domain = irq_find_host(parent_node);
> > +	if (!parent_domain) {
> > +		dev_err(dev, "unable to obtain parent domain\n");
> > +		ret = -ENXIO;
> > +		goto err;
> > +	}
> > +
> > +	of_node_put(parent_node);
> >  
> >  	irq_count = of_irq_count(intc_np);
> >  	if (!irq_count) {
> > @@ -671,31 +718,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
> >  		goto err;
> >  	}
> >  
> > -	for (i = 0; i < irq_count; i++) {
> > -		irq = irq_of_parse_and_map(intc_np, i);
> > -		if (!irq) {
> > -			ret = -EINVAL;
> > -			goto err;
> > -		}
> > -		ks_pcie->legacy_host_irqs[i] = irq;
> > -
> > -		irq_set_chained_handler_and_data(irq,
> > -						 ks_pcie_legacy_irq_handler,
> > -						 ks_pcie);
> > -	}
> > -
> >  	legacy_irq_domain =
> > -		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
> > -				      &ks_pcie_legacy_irq_domain_ops, NULL);
> > +		irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX,
> > +					 intc_np,
> > +					 &ks_pcie_legacy_irq_domain_ops,
> > +					 ks_pcie);
> >  	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);
> >  
> >  err:
> >  	of_node_put(intc_np);
> > -- 
> > 2.17.1
> >   

The structure of the code otherwise looks good. I trust you'd be able
to shed some light on the questions I have.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Kishon Vijay Abraham I]

Hi,

On 23/02/19 5:41 PM, Marc Zyngier wrote:
> On Thu, 21 Feb 2019 16:24:14 +0000
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
>> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote:
>>> K2G provides separate IRQ lines for each of the four legacy interrupts.
>>> Model this using hierarchy domain instead of linear domain with chained
>>> IRQ handler.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++---------
>>>  1 file changed, 118 insertions(+), 87 deletions(-)  
>>
>> Hi Kishon,
>>
>> I CC'ed Marc because you are actually re-writing an interrupt controller
>> driver so I would be happier to merge this refactoring if Marc can have
>> a look and he is satisfied with it - more so because most of the code can
>> be reused by other host bridge drivers with similar behaviour.
> 
> Cheers Lorenzo.
> 
> It doesn't look too bad, but there is a couple of points I'd like to see
> clarified. Comments below.
> 
>>
>> I will have a look too, unfortunately it is becoming a bit tight for
>> v5.1 but let's see how it goes.
>>
>> Thanks,
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>>> index 47f0dcf638f2..7f1648453f54 100644
>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>>> @@ -61,6 +61,7 @@
>>>  
>>>  #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
>>>  #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
>>> +#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
>>>  #define INTx_EN				BIT(0)
>>>  
>>>  #define ERR_IRQ_STATUS			0x1c4
>>> @@ -87,7 +88,6 @@ struct keystone_pcie {
>>>  	struct dw_pcie		*pci;
>>>  	/* PCI Device ID */
>>>  	u32			device_id;
>>> -	int			legacy_host_irqs[PCI_NUM_INTX];
>>>  	struct			device_node *legacy_intc_np;
>>>  
>>>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
>>> @@ -96,7 +96,6 @@ struct keystone_pcie {
>>>  	struct phy		**phy;
>>>  	struct device_link	**link;
>>>  	struct			device_node *msi_intc_np;
>>> -	struct irq_domain	*legacy_irq_domain;
>>>  	struct device_node	*np;
>>>  
>>>  	int error_irq;
>>> @@ -199,26 +198,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;
>>> -	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);
>>> -	}
>>> -
>>> -	/* 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);
>>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
>>>  	return IRQ_HANDLED;
>>>  }
>>>  
>>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d)
>>> +void ks_pcie_irq_eoi(struct irq_data *data)
>>>  {
>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>> +	irq_hw_number_t hwirq = data->hwirq;
>>> +
>>> +	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
>>> +	irq_chip_eoi_parent(data);
>>>  }
>>>  
>>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d)
>>> +void ks_pcie_irq_enable(struct irq_data *data)
>>>  {
>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>> +	irq_hw_number_t hwirq = data->hwirq;
>>> +
>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
>>> +	irq_chip_enable_parent(data);
>>>  }
>>>  
>>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
>>> +void ks_pcie_irq_disable(struct irq_data *data)
>>>  {
>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>> +	irq_hw_number_t hwirq = data->hwirq;
>>> +
>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
>>> +	irq_chip_disable_parent(data);
>>>  }
>>>  
>>>  static struct irq_chip ks_pcie_legacy_irq_chip = {
>>> -	.name = "Keystone-PCI-Legacy-IRQ",
>>> -	.irq_ack = ks_pcie_ack_legacy_irq,
>>> -	.irq_mask = ks_pcie_mask_legacy_irq,
>>> -	.irq_unmask = ks_pcie_unmask_legacy_irq,
>>> +	.name			= "Keystone-PCI-Legacy-IRQ",
>>> +	.irq_enable		= ks_pcie_irq_enable,
>>> +	.irq_disable		= ks_pcie_irq_disable,
>>> +	.irq_eoi		= ks_pcie_irq_eoi,
>>> +	.irq_mask		= irq_chip_mask_parent,
>>> +	.irq_unmask		= irq_chip_unmask_parent,
>>> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>>> +	.irq_set_type		= irq_chip_set_type_parent,
>>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>>  };
>>>  
>>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
>>> -				       unsigned int irq,
>>> -				       irq_hw_number_t hw_irq)
>>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
>>> +					   unsigned int virq,
>>> +					   unsigned int nr_irqs, void *data)
>>>  {
>>> -	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
>>> -				 handle_level_irq);
>>> -	irq_set_chip_data(irq, d->host_data);
>>> +	struct keystone_pcie *ks_pcie = domain->host_data;
>>> +	struct device_node *np = ks_pcie->legacy_intc_np;
>>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>>> +	struct of_phandle_args out_irq;
>>> +	int ret, i;
>>> +
>>> +	if (nr_irqs != 1)
>>> +		return -EINVAL;
>>> +
>>> +	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
>>> +	if (ret < 0) {
>>> +		pr_err("Failed to parse interrupt node\n");
>>> +		return ret;
>>> +	}
> 
> What it this trying to do? Fishing out the interrupts from DT based on
> the legacy pin? This looks at best obscure. I wonder why you don't do
> that at probe time instead. Anyway, this requires documenting.

The device-tree of PCIe node looks something like below.

interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */
		<0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */
		<0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */
		<0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */
pcie_intc0: legacy-interrupt-controller {
	interrupt-controller;
	#interrupt-cells = <2>;
	interrupt-parent = <&gic>;
	interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>,
		     <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>,
		     <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>,
		     <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>;
};

INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this
driver which in turn corresponds to GIC_SPI '48' of GIC.

We could create an array of parent_fwspec for each of the four interrupt lines
in legacy-interrupt-controller during probe and use it directly here while
invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead
of using of_irq_parse_one here?
> 
>>> +
>>> +	parent_fwspec.fwnode = &out_irq.np->fwnode;
>>> +	parent_fwspec.param_count = out_irq.args_count;
>>> +
>>> +	for (i = 0; i < out_irq.args_count; i++)
>>> +		parent_fwspec.param[i] = out_irq.args[i];
> 
> This feels like a duplicate of of_phandle_args_to_fwspec(). If you need
> such a helper, please export it from the irqdomain code.

sure.
> 
>>> +
>>> +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
>>> +	if (ret < 0) {
>>> +		pr_err("Failed to allocate parent irq %u: %d\n",
>>> +		       parent_fwspec.param[0], ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
>>> +					    &ks_pcie_legacy_irq_chip, ks_pcie);
>>> +	if (ret < 0) {
>>> +		pr_err("Failed to set hwirq and chip\n");
>>> +		goto err_set_hwirq_and_chip;
>>> +	}
>>>  
>>>  	return 0;
>>> +
>>> +err_set_hwirq_and_chip:
>>> +	irq_domain_free_irqs_parent(domain, virq, 1);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int ks_pcie_irq_domain_translate(struct irq_domain *domain,
>>> +					struct irq_fwspec *fwspec,
>>> +					unsigned long *hwirq,
>>> +					unsigned int *type)
>>> +{
>>> +	if (is_of_node(fwspec->fwnode)) {
> 
> If you don't plan to support ACPI, you can drop this.

okay.
> 
>>> +		if (fwspec->param_count != 2)
>>> +			return -EINVAL;
> 
> From the DT binding:
> 
> pcie_intc: Interrupt controller device node for Legacy IRQ chip
>         interrupt-cells: should be set to 1
> 
> So why do we have 2 cells here?

With '1' cells, it's not possible to specify irq trigger type. DT binding has
to be fixed. I'll fix this in the next revision of the patch series.
> 
>>> +
>>> +		if (fwspec->param[0] >= PCI_NUM_INTX)
>>> +			return -EINVAL;
> 
> Most of the OF code assumes that the pin number describing the legacy
> interrupt is 1-based, while you obviously treat it as 0-based. How does
> it work?

INTA corresponds to '0' of the hierarchy interrupt domain (using interrupt-map).
> 
>>> +
>>> +		*hwirq = fwspec->param[0];
>>> +		*type = fwspec->param[1];
> 
> As far as I remember, PCI legacy interrupts are level triggered, so
> there should be no need to advertise the trigger (which is consistent
> with the way the binding is written).

It is pulse triggered at subsystem level. Quoting the TRM
"The interrupt request signal at the PCIe SS boundary is a pulse signal that is
triggered each time an assert interrupt message is received." The PCIe
subsystem also has a level signal (interrupt pending signal) but the interrupt
request signal is the one that is connected to GIC.

Thanks
Kishon

Re: [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Marc Zyngier]

On Thu, 07 Mar 2019 09:12:30 +0000,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Hi,
> 
> On 23/02/19 5:41 PM, Marc Zyngier wrote:
> > On Thu, 21 Feb 2019 16:24:14 +0000
> > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > 
> >> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote:
> >>> K2G provides separate IRQ lines for each of the four legacy interrupts.
> >>> Model this using hierarchy domain instead of linear domain with chained
> >>> IRQ handler.
> >>>
> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>> ---
> >>>  drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++---------
> >>>  1 file changed, 118 insertions(+), 87 deletions(-)  
> >>
> >> Hi Kishon,
> >>
> >> I CC'ed Marc because you are actually re-writing an interrupt controller
> >> driver so I would be happier to merge this refactoring if Marc can have
> >> a look and he is satisfied with it - more so because most of the code can
> >> be reused by other host bridge drivers with similar behaviour.
> > 
> > Cheers Lorenzo.
> > 
> > It doesn't look too bad, but there is a couple of points I'd like to see
> > clarified. Comments below.
> > 
> >>
> >> I will have a look too, unfortunately it is becoming a bit tight for
> >> v5.1 but let's see how it goes.
> >>
> >> Thanks,
> >> Lorenzo
> >>
> >>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> >>> index 47f0dcf638f2..7f1648453f54 100644
> >>> --- a/drivers/pci/controller/dwc/pci-keystone.c
> >>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> >>> @@ -61,6 +61,7 @@
> >>>  
> >>>  #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
> >>>  #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
> >>> +#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
> >>>  #define INTx_EN				BIT(0)
> >>>  
> >>>  #define ERR_IRQ_STATUS			0x1c4
> >>> @@ -87,7 +88,6 @@ struct keystone_pcie {
> >>>  	struct dw_pcie		*pci;
> >>>  	/* PCI Device ID */
> >>>  	u32			device_id;
> >>> -	int			legacy_host_irqs[PCI_NUM_INTX];
> >>>  	struct			device_node *legacy_intc_np;
> >>>  
> >>>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
> >>> @@ -96,7 +96,6 @@ struct keystone_pcie {
> >>>  	struct phy		**phy;
> >>>  	struct device_link	**link;
> >>>  	struct			device_node *msi_intc_np;
> >>> -	struct irq_domain	*legacy_irq_domain;
> >>>  	struct device_node	*np;
> >>>  
> >>>  	int error_irq;
> >>> @@ -199,26 +198,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;
> >>> -	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);
> >>> -	}
> >>> -
> >>> -	/* 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);
> >>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
> >>>  	return IRQ_HANDLED;
> >>>  }
> >>>  
> >>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d)
> >>> +void ks_pcie_irq_eoi(struct irq_data *data)
> >>>  {
> >>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> >>> +	irq_hw_number_t hwirq = data->hwirq;
> >>> +
> >>> +	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
> >>> +	irq_chip_eoi_parent(data);
> >>>  }
> >>>  
> >>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d)
> >>> +void ks_pcie_irq_enable(struct irq_data *data)
> >>>  {
> >>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> >>> +	irq_hw_number_t hwirq = data->hwirq;
> >>> +
> >>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
> >>> +	irq_chip_enable_parent(data);
> >>>  }
> >>>  
> >>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
> >>> +void ks_pcie_irq_disable(struct irq_data *data)
> >>>  {
> >>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> >>> +	irq_hw_number_t hwirq = data->hwirq;
> >>> +
> >>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
> >>> +	irq_chip_disable_parent(data);
> >>>  }
> >>>  
> >>>  static struct irq_chip ks_pcie_legacy_irq_chip = {
> >>> -	.name = "Keystone-PCI-Legacy-IRQ",
> >>> -	.irq_ack = ks_pcie_ack_legacy_irq,
> >>> -	.irq_mask = ks_pcie_mask_legacy_irq,
> >>> -	.irq_unmask = ks_pcie_unmask_legacy_irq,
> >>> +	.name			= "Keystone-PCI-Legacy-IRQ",
> >>> +	.irq_enable		= ks_pcie_irq_enable,
> >>> +	.irq_disable		= ks_pcie_irq_disable,
> >>> +	.irq_eoi		= ks_pcie_irq_eoi,
> >>> +	.irq_mask		= irq_chip_mask_parent,
> >>> +	.irq_unmask		= irq_chip_unmask_parent,
> >>> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> >>> +	.irq_set_type		= irq_chip_set_type_parent,
> >>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> >>>  };
> >>>  
> >>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
> >>> -				       unsigned int irq,
> >>> -				       irq_hw_number_t hw_irq)
> >>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
> >>> +					   unsigned int virq,
> >>> +					   unsigned int nr_irqs, void *data)
> >>>  {
> >>> -	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
> >>> -				 handle_level_irq);
> >>> -	irq_set_chip_data(irq, d->host_data);
> >>> +	struct keystone_pcie *ks_pcie = domain->host_data;
> >>> +	struct device_node *np = ks_pcie->legacy_intc_np;
> >>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
> >>> +	struct of_phandle_args out_irq;
> >>> +	int ret, i;
> >>> +
> >>> +	if (nr_irqs != 1)
> >>> +		return -EINVAL;
> >>> +
> >>> +	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
> >>> +	if (ret < 0) {
> >>> +		pr_err("Failed to parse interrupt node\n");
> >>> +		return ret;
> >>> +	}
> > 
> > What it this trying to do? Fishing out the interrupts from DT based on
> > the legacy pin? This looks at best obscure. I wonder why you don't do
> > that at probe time instead. Anyway, this requires documenting.
> 
> The device-tree of PCIe node looks something like below.
> 
> interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */
> 		<0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */
> 		<0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */
> 		<0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */
> pcie_intc0: legacy-interrupt-controller {
> 	interrupt-controller;
> 	#interrupt-cells = <2>;
> 	interrupt-parent = <&gic>;
> 	interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>,
> 		     <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>,
> 		     <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>,
> 		     <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>;
> };
> 
> INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this
> driver which in turn corresponds to GIC_SPI '48' of GIC.
> 
> We could create an array of parent_fwspec for each of the four interrupt lines
> in legacy-interrupt-controller during probe and use it directly here while
> invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead
> of using of_irq_parse_one here?

Up to you. I don't mind either way, but it is the lack of
documentation that annoys me the most here.

The more worrying part is that you are using an EDGE signalling in
these bindings, and that's wrong. PCI legacy interrupts are level,
always.

[...]

> > From the DT binding:
> > 
> > pcie_intc: Interrupt controller device node for Legacy IRQ chip
> >         interrupt-cells: should be set to 1
> > 
> > So why do we have 2 cells here?
> 
> With '1' cells, it's not possible to specify irq trigger type. DT
> binding has to be fixed. I'll fix this in the next revision of the
> patch series.

As I said above, this is wrong. Legacy interrupts are always level, by
definition. Why are you making them edge-triggered?

> > 
> >>> +
> >>> +		if (fwspec->param[0] >= PCI_NUM_INTX)
> >>> +			return -EINVAL;
> > 
> > Most of the OF code assumes that the pin number describing the legacy
> > interrupt is 1-based, while you obviously treat it as 0-based. How does
> > it work?
> 
> INTA corresponds to '0' of the hierarchy interrupt domain (using
> interrupt-map).

I'm not disputing this. It is more that the DT code does number the
pin from 1, if memory serves well. Can you please verify this (see
of_irq_parse_pci).

> >
> >>> +
> >>> +		*hwirq = fwspec->param[0];
> >>> +		*type = fwspec->param[1];
> > 
> > As far as I remember, PCI legacy interrupts are level triggered, so
> > there should be no need to advertise the trigger (which is consistent
> > with the way the binding is written).
> 
> It is pulse triggered at subsystem level. Quoting the TRM
> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is
> triggered each time an assert interrupt message is received." The PCIe
> subsystem also has a level signal (interrupt pending signal) but the interrupt
> request signal is the one that is connected to GIC.

This looks completely wrong. MSIs are always edge, and legacy always
level, end of story. If your HW is any different, then it doesn't seem
to be compliant with PCI.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Kishon Vijay Abraham I]

Hi Marc,

On 07/03/19 5:32 PM, Marc Zyngier wrote:
> On Thu, 07 Mar 2019 09:12:30 +0000,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi,
>>
>> On 23/02/19 5:41 PM, Marc Zyngier wrote:
>>> On Thu, 21 Feb 2019 16:24:14 +0000
>>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>>>
>>>> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote:
>>>>> K2G provides separate IRQ lines for each of the four legacy interrupts.
>>>>> Model this using hierarchy domain instead of linear domain with chained
>>>>> IRQ handler.
>>>>>
>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> ---
>>>>>  drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++---------
>>>>>  1 file changed, 118 insertions(+), 87 deletions(-)  
>>>>
>>>> Hi Kishon,
>>>>
>>>> I CC'ed Marc because you are actually re-writing an interrupt controller
>>>> driver so I would be happier to merge this refactoring if Marc can have
>>>> a look and he is satisfied with it - more so because most of the code can
>>>> be reused by other host bridge drivers with similar behaviour.
>>>
>>> Cheers Lorenzo.
>>>
>>> It doesn't look too bad, but there is a couple of points I'd like to see
>>> clarified. Comments below.
>>>
>>>>
>>>> I will have a look too, unfortunately it is becoming a bit tight for
>>>> v5.1 but let's see how it goes.
>>>>
>>>> Thanks,
>>>> Lorenzo
>>>>
>>>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>>>>> index 47f0dcf638f2..7f1648453f54 100644
>>>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>>>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>>>>> @@ -61,6 +61,7 @@
>>>>>  
>>>>>  #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
>>>>>  #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
>>>>> +#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
>>>>>  #define INTx_EN				BIT(0)
>>>>>  
>>>>>  #define ERR_IRQ_STATUS			0x1c4
>>>>> @@ -87,7 +88,6 @@ struct keystone_pcie {
>>>>>  	struct dw_pcie		*pci;
>>>>>  	/* PCI Device ID */
>>>>>  	u32			device_id;
>>>>> -	int			legacy_host_irqs[PCI_NUM_INTX];
>>>>>  	struct			device_node *legacy_intc_np;
>>>>>  
>>>>>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
>>>>> @@ -96,7 +96,6 @@ struct keystone_pcie {
>>>>>  	struct phy		**phy;
>>>>>  	struct device_link	**link;
>>>>>  	struct			device_node *msi_intc_np;
>>>>> -	struct irq_domain	*legacy_irq_domain;
>>>>>  	struct device_node	*np;
>>>>>  
>>>>>  	int error_irq;
>>>>> @@ -199,26 +198,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;
>>>>> -	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);
>>>>> -	}
>>>>> -
>>>>> -	/* 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);
>>>>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
>>>>>  	return IRQ_HANDLED;
>>>>>  }
>>>>>  
>>>>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d)
>>>>> +void ks_pcie_irq_eoi(struct irq_data *data)
>>>>>  {
>>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>>>> +	irq_hw_number_t hwirq = data->hwirq;
>>>>> +
>>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
>>>>> +	irq_chip_eoi_parent(data);
>>>>>  }
>>>>>  
>>>>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d)
>>>>> +void ks_pcie_irq_enable(struct irq_data *data)
>>>>>  {
>>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>>>> +	irq_hw_number_t hwirq = data->hwirq;
>>>>> +
>>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
>>>>> +	irq_chip_enable_parent(data);
>>>>>  }
>>>>>  
>>>>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
>>>>> +void ks_pcie_irq_disable(struct irq_data *data)
>>>>>  {
>>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>>>> +	irq_hw_number_t hwirq = data->hwirq;
>>>>> +
>>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
>>>>> +	irq_chip_disable_parent(data);
>>>>>  }
>>>>>  
>>>>>  static struct irq_chip ks_pcie_legacy_irq_chip = {
>>>>> -	.name = "Keystone-PCI-Legacy-IRQ",
>>>>> -	.irq_ack = ks_pcie_ack_legacy_irq,
>>>>> -	.irq_mask = ks_pcie_mask_legacy_irq,
>>>>> -	.irq_unmask = ks_pcie_unmask_legacy_irq,
>>>>> +	.name			= "Keystone-PCI-Legacy-IRQ",
>>>>> +	.irq_enable		= ks_pcie_irq_enable,
>>>>> +	.irq_disable		= ks_pcie_irq_disable,
>>>>> +	.irq_eoi		= ks_pcie_irq_eoi,
>>>>> +	.irq_mask		= irq_chip_mask_parent,
>>>>> +	.irq_unmask		= irq_chip_unmask_parent,
>>>>> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>>>>> +	.irq_set_type		= irq_chip_set_type_parent,
>>>>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>>>>  };
>>>>>  
>>>>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
>>>>> -				       unsigned int irq,
>>>>> -				       irq_hw_number_t hw_irq)
>>>>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
>>>>> +					   unsigned int virq,
>>>>> +					   unsigned int nr_irqs, void *data)
>>>>>  {
>>>>> -	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
>>>>> -				 handle_level_irq);
>>>>> -	irq_set_chip_data(irq, d->host_data);
>>>>> +	struct keystone_pcie *ks_pcie = domain->host_data;
>>>>> +	struct device_node *np = ks_pcie->legacy_intc_np;
>>>>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>>>>> +	struct of_phandle_args out_irq;
>>>>> +	int ret, i;
>>>>> +
>>>>> +	if (nr_irqs != 1)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
>>>>> +	if (ret < 0) {
>>>>> +		pr_err("Failed to parse interrupt node\n");
>>>>> +		return ret;
>>>>> +	}
>>>
>>> What it this trying to do? Fishing out the interrupts from DT based on
>>> the legacy pin? This looks at best obscure. I wonder why you don't do
>>> that at probe time instead. Anyway, this requires documenting.
>>
>> The device-tree of PCIe node looks something like below.
>>
>> interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */
>> 		<0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */
>> 		<0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */
>> 		<0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */
>> pcie_intc0: legacy-interrupt-controller {
>> 	interrupt-controller;
>> 	#interrupt-cells = <2>;
>> 	interrupt-parent = <&gic>;
>> 	interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>,
>> 		     <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>,
>> 		     <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>,
>> 		     <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>;
>> };
>>
>> INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this
>> driver which in turn corresponds to GIC_SPI '48' of GIC.
>>
>> We could create an array of parent_fwspec for each of the four interrupt lines
>> in legacy-interrupt-controller during probe and use it directly here while
>> invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead
>> of using of_irq_parse_one here?
> 
> Up to you. I don't mind either way, but it is the lack of
> documentation that annoys me the most here.

Okay, I'll add a comment before invoking of_irq_parse_one.
> 
> The more worrying part is that you are using an EDGE signalling in
> these bindings, and that's wrong. PCI legacy interrupts are level,
> always.

Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message.
> 
> [...]
> 
>>> From the DT binding:
>>>
>>> pcie_intc: Interrupt controller device node for Legacy IRQ chip
>>>         interrupt-cells: should be set to 1
>>>
>>> So why do we have 2 cells here?
>>
>> With '1' cells, it's not possible to specify irq trigger type. DT
>> binding has to be fixed. I'll fix this in the next revision of the
>> patch series.
> 
> As I said above, this is wrong. Legacy interrupts are always level, by
> definition. Why are you making them edge-triggered
> 
>>>
>>>>> +
>>>>> +		if (fwspec->param[0] >= PCI_NUM_INTX)
>>>>> +			return -EINVAL;
>>>
>>> Most of the OF code assumes that the pin number describing the legacy
>>> interrupt is 1-based, while you obviously treat it as 0-based. How does
>>> it work?
>>
>> INTA corresponds to '0' of the hierarchy interrupt domain (using
>> interrupt-map).
> 
> I'm not disputing this. It is more that the DT code does number the
> pin from 1, if memory serves well. Can you please verify this (see
> of_irq_parse_pci).

Yes it does number from '1'. However I use interrupt-map property to map from
legacy interrupt pin to a interrupt number of the legacy interrupt controller
which starts from '0'.
> 
>>>
>>>>> +
>>>>> +		*hwirq = fwspec->param[0];
>>>>> +		*type = fwspec->param[1];
>>>
>>> As far as I remember, PCI legacy interrupts are level triggered, so
>>> there should be no need to advertise the trigger (which is consistent
>>> with the way the binding is written).
>>
>> It is pulse triggered at subsystem level. Quoting the TRM
>> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is
>> triggered each time an assert interrupt message is received." The PCIe
>> subsystem also has a level signal (interrupt pending signal) but the interrupt
>> request signal is the one that is connected to GIC.
> 
> This looks completely wrong. MSIs are always edge, and legacy always
> level, end of story. If your HW is any different, then it doesn't seem
> to be compliant with PCI.

Agree that Legacy interrupts ought to be level-triggered. I checked this with
HW team and I've copy-pasted the response.

"The legacy interrupts from the PCIe controller are still level. The PCIe
controller will hold the legacy interrupt lines high until a de-assert message
is sent back. The pulse interrupt is only to send the interrupt from the PCIe
sub-system to the SoC interrupt controller. This should not impact the
operation of the legacy interrupt on the PCIe side."

Looks like the interrupts are propagated multiple levels and only the last
level to the interrupt controller is pulse.

Thanks
Kishon

Re: [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Marc Zyngier]

On Mon, 18 Mar 2019 11:03:07 +0530
Kishon Vijay Abraham I <kishon@ti.com> wrote:

> Hi Marc,
> 
> On 07/03/19 5:32 PM, Marc Zyngier wrote:
> > On Thu, 07 Mar 2019 09:12:30 +0000,
> > Kishon Vijay Abraham I <kishon@ti.com> wrote:  
> >>
> >> Hi,
> >>
> >> On 23/02/19 5:41 PM, Marc Zyngier wrote:  
> >>> On Thu, 21 Feb 2019 16:24:14 +0000
> >>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> >>>  
> >>>> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote:  
> >>>>> K2G provides separate IRQ lines for each of the four legacy interrupts.
> >>>>> Model this using hierarchy domain instead of linear domain with chained
> >>>>> IRQ handler.
> >>>>>
> >>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>>> ---
> >>>>>  drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++---------
> >>>>>  1 file changed, 118 insertions(+), 87 deletions(-)    
> >>>>
> >>>> Hi Kishon,
> >>>>
> >>>> I CC'ed Marc because you are actually re-writing an interrupt controller
> >>>> driver so I would be happier to merge this refactoring if Marc can have
> >>>> a look and he is satisfied with it - more so because most of the code can
> >>>> be reused by other host bridge drivers with similar behaviour.  
> >>>
> >>> Cheers Lorenzo.
> >>>
> >>> It doesn't look too bad, but there is a couple of points I'd like to see
> >>> clarified. Comments below.
> >>>  
> >>>>
> >>>> I will have a look too, unfortunately it is becoming a bit tight for
> >>>> v5.1 but let's see how it goes.
> >>>>
> >>>> Thanks,
> >>>> Lorenzo
> >>>>  
> >>>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> >>>>> index 47f0dcf638f2..7f1648453f54 100644
> >>>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
> >>>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> >>>>> @@ -61,6 +61,7 @@
> >>>>>  
> >>>>>  #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
> >>>>>  #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
> >>>>> +#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
> >>>>>  #define INTx_EN				BIT(0)
> >>>>>  
> >>>>>  #define ERR_IRQ_STATUS			0x1c4
> >>>>> @@ -87,7 +88,6 @@ struct keystone_pcie {
> >>>>>  	struct dw_pcie		*pci;
> >>>>>  	/* PCI Device ID */
> >>>>>  	u32			device_id;
> >>>>> -	int			legacy_host_irqs[PCI_NUM_INTX];
> >>>>>  	struct			device_node *legacy_intc_np;
> >>>>>  
> >>>>>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
> >>>>> @@ -96,7 +96,6 @@ struct keystone_pcie {
> >>>>>  	struct phy		**phy;
> >>>>>  	struct device_link	**link;
> >>>>>  	struct			device_node *msi_intc_np;
> >>>>> -	struct irq_domain	*legacy_irq_domain;
> >>>>>  	struct device_node	*np;
> >>>>>  
> >>>>>  	int error_irq;
> >>>>> @@ -199,26 +198,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;
> >>>>> -	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);
> >>>>> -	}
> >>>>> -
> >>>>> -	/* 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);
> >>>>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
> >>>>>  	return IRQ_HANDLED;
> >>>>>  }
> >>>>>  
> >>>>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d)
> >>>>> +void ks_pcie_irq_eoi(struct irq_data *data)
> >>>>>  {
> >>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> >>>>> +	irq_hw_number_t hwirq = data->hwirq;
> >>>>> +
> >>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
> >>>>> +	irq_chip_eoi_parent(data);
> >>>>>  }
> >>>>>  
> >>>>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d)
> >>>>> +void ks_pcie_irq_enable(struct irq_data *data)
> >>>>>  {
> >>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> >>>>> +	irq_hw_number_t hwirq = data->hwirq;
> >>>>> +
> >>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
> >>>>> +	irq_chip_enable_parent(data);
> >>>>>  }
> >>>>>  
> >>>>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
> >>>>> +void ks_pcie_irq_disable(struct irq_data *data)
> >>>>>  {
> >>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
> >>>>> +	irq_hw_number_t hwirq = data->hwirq;
> >>>>> +
> >>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
> >>>>> +	irq_chip_disable_parent(data);
> >>>>>  }
> >>>>>  
> >>>>>  static struct irq_chip ks_pcie_legacy_irq_chip = {
> >>>>> -	.name = "Keystone-PCI-Legacy-IRQ",
> >>>>> -	.irq_ack = ks_pcie_ack_legacy_irq,
> >>>>> -	.irq_mask = ks_pcie_mask_legacy_irq,
> >>>>> -	.irq_unmask = ks_pcie_unmask_legacy_irq,
> >>>>> +	.name			= "Keystone-PCI-Legacy-IRQ",
> >>>>> +	.irq_enable		= ks_pcie_irq_enable,
> >>>>> +	.irq_disable		= ks_pcie_irq_disable,
> >>>>> +	.irq_eoi		= ks_pcie_irq_eoi,
> >>>>> +	.irq_mask		= irq_chip_mask_parent,
> >>>>> +	.irq_unmask		= irq_chip_unmask_parent,
> >>>>> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> >>>>> +	.irq_set_type		= irq_chip_set_type_parent,
> >>>>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> >>>>>  };
> >>>>>  
> >>>>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
> >>>>> -				       unsigned int irq,
> >>>>> -				       irq_hw_number_t hw_irq)
> >>>>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
> >>>>> +					   unsigned int virq,
> >>>>> +					   unsigned int nr_irqs, void *data)
> >>>>>  {
> >>>>> -	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
> >>>>> -				 handle_level_irq);
> >>>>> -	irq_set_chip_data(irq, d->host_data);
> >>>>> +	struct keystone_pcie *ks_pcie = domain->host_data;
> >>>>> +	struct device_node *np = ks_pcie->legacy_intc_np;
> >>>>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
> >>>>> +	struct of_phandle_args out_irq;
> >>>>> +	int ret, i;
> >>>>> +
> >>>>> +	if (nr_irqs != 1)
> >>>>> +		return -EINVAL;
> >>>>> +
> >>>>> +	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
> >>>>> +	if (ret < 0) {
> >>>>> +		pr_err("Failed to parse interrupt node\n");
> >>>>> +		return ret;
> >>>>> +	}  
> >>>
> >>> What it this trying to do? Fishing out the interrupts from DT based on
> >>> the legacy pin? This looks at best obscure. I wonder why you don't do
> >>> that at probe time instead. Anyway, this requires documenting.  
> >>
> >> The device-tree of PCIe node looks something like below.
> >>
> >> interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */
> >> 		<0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */
> >> 		<0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */
> >> 		<0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */
> >> pcie_intc0: legacy-interrupt-controller {
> >> 	interrupt-controller;
> >> 	#interrupt-cells = <2>;
> >> 	interrupt-parent = <&gic>;
> >> 	interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>,
> >> 		     <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>,
> >> 		     <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>,
> >> 		     <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>;
> >> };
> >>
> >> INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this
> >> driver which in turn corresponds to GIC_SPI '48' of GIC.
> >>
> >> We could create an array of parent_fwspec for each of the four interrupt lines
> >> in legacy-interrupt-controller during probe and use it directly here while
> >> invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead
> >> of using of_irq_parse_one here?  
> > 
> > Up to you. I don't mind either way, but it is the lack of
> > documentation that annoys me the most here.  
> 
> Okay, I'll add a comment before invoking of_irq_parse_one.
> > 
> > The more worrying part is that you are using an EDGE signalling in
> > these bindings, and that's wrong. PCI legacy interrupts are level,
> > always.  
> 
> Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message.

I'm not convinced... See below.

> > 
> > [...]
> >   
> >>> From the DT binding:
> >>>
> >>> pcie_intc: Interrupt controller device node for Legacy IRQ chip
> >>>         interrupt-cells: should be set to 1
> >>>
> >>> So why do we have 2 cells here?  
> >>
> >> With '1' cells, it's not possible to specify irq trigger type. DT
> >> binding has to be fixed. I'll fix this in the next revision of the
> >> patch series.  
> > 
> > As I said above, this is wrong. Legacy interrupts are always level, by
> > definition. Why are you making them edge-triggered
> >   
> >>>  
> >>>>> +
> >>>>> +		if (fwspec->param[0] >= PCI_NUM_INTX)
> >>>>> +			return -EINVAL;  
> >>>
> >>> Most of the OF code assumes that the pin number describing the legacy
> >>> interrupt is 1-based, while you obviously treat it as 0-based. How does
> >>> it work?  
> >>
> >> INTA corresponds to '0' of the hierarchy interrupt domain (using
> >> interrupt-map).  
> > 
> > I'm not disputing this. It is more that the DT code does number the
> > pin from 1, if memory serves well. Can you please verify this (see
> > of_irq_parse_pci).  
> 
> Yes it does number from '1'. However I use interrupt-map property to map from
> legacy interrupt pin to a interrupt number of the legacy interrupt controller
> which starts from '0'.

If you've verified that it actually works, fine bby me.

> >   
> >>>  
> >>>>> +
> >>>>> +		*hwirq = fwspec->param[0];
> >>>>> +		*type = fwspec->param[1];  
> >>>
> >>> As far as I remember, PCI legacy interrupts are level triggered, so
> >>> there should be no need to advertise the trigger (which is consistent
> >>> with the way the binding is written).  
> >>
> >> It is pulse triggered at subsystem level. Quoting the TRM
> >> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is
> >> triggered each time an assert interrupt message is received." The PCIe
> >> subsystem also has a level signal (interrupt pending signal) but the interrupt
> >> request signal is the one that is connected to GIC.  
> > 
> > This looks completely wrong. MSIs are always edge, and legacy always
> > level, end of story. If your HW is any different, then it doesn't seem
> > to be compliant with PCI.  
> 
> Agree that Legacy interrupts ought to be level-triggered. I checked this with
> HW team and I've copy-pasted the response.
> 
> "The legacy interrupts from the PCIe controller are still level. The PCIe
> controller will hold the legacy interrupt lines high until a de-assert message
> is sent back. The pulse interrupt is only to send the interrupt from the PCIe
> sub-system to the SoC interrupt controller. This should not impact the
> operation of the legacy interrupt on the PCIe side."
> 
> Looks like the interrupts are propagated multiple levels and only the last
> level to the interrupt controller is pulse.

Let's take an example: My device (let's assume a network controller of
some sort) generates an interrupt. Level is high, and you observe an
edge. The driver handles the interrupt, but due to a race, the line
stays high (the device has received a new packet).

How is a *new* edge generated? The line never transited to low, and we
have more packet to process. With a level triggered interrupt, you just
take the interrupt again. With an edge interrupt, you need to actively
resample the level and retrigger the interrupt. How is this done?

Similar issues exist with interrupt sharing, which is a fairly common
thing.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Kishon Vijay Abraham I]

Hi Marc,

On 18/03/19 4:46 PM, Marc Zyngier wrote:
> On Mon, 18 Mar 2019 11:03:07 +0530
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
>> Hi Marc,
>>
>> On 07/03/19 5:32 PM, Marc Zyngier wrote:
>>> On Thu, 07 Mar 2019 09:12:30 +0000,
>>> Kishon Vijay Abraham I <kishon@ti.com> wrote:  
>>>>
>>>> Hi,
>>>>
>>>> On 23/02/19 5:41 PM, Marc Zyngier wrote:  
>>>>> On Thu, 21 Feb 2019 16:24:14 +0000
>>>>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>>>>>  
>>>>>> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote:  
>>>>>>> K2G provides separate IRQ lines for each of the four legacy interrupts.
>>>>>>> Model this using hierarchy domain instead of linear domain with chained
>>>>>>> IRQ handler.
>>>>>>>
>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>> ---
>>>>>>>  drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++---------
>>>>>>>  1 file changed, 118 insertions(+), 87 deletions(-)    
>>>>>>
>>>>>> Hi Kishon,
>>>>>>
>>>>>> I CC'ed Marc because you are actually re-writing an interrupt controller
>>>>>> driver so I would be happier to merge this refactoring if Marc can have
>>>>>> a look and he is satisfied with it - more so because most of the code can
>>>>>> be reused by other host bridge drivers with similar behaviour.  
>>>>>
>>>>> Cheers Lorenzo.
>>>>>
>>>>> It doesn't look too bad, but there is a couple of points I'd like to see
>>>>> clarified. Comments below.
>>>>>  
>>>>>>
>>>>>> I will have a look too, unfortunately it is becoming a bit tight for
>>>>>> v5.1 but let's see how it goes.
>>>>>>
>>>>>> Thanks,
>>>>>> Lorenzo
>>>>>>  
>>>>>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>>>>>>> index 47f0dcf638f2..7f1648453f54 100644
>>>>>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>>>>>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>>>>>>> @@ -61,6 +61,7 @@
>>>>>>>  
>>>>>>>  #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
>>>>>>>  #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
>>>>>>> +#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
>>>>>>>  #define INTx_EN				BIT(0)
>>>>>>>  
>>>>>>>  #define ERR_IRQ_STATUS			0x1c4
>>>>>>> @@ -87,7 +88,6 @@ struct keystone_pcie {
>>>>>>>  	struct dw_pcie		*pci;
>>>>>>>  	/* PCI Device ID */
>>>>>>>  	u32			device_id;
>>>>>>> -	int			legacy_host_irqs[PCI_NUM_INTX];
>>>>>>>  	struct			device_node *legacy_intc_np;
>>>>>>>  
>>>>>>>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
>>>>>>> @@ -96,7 +96,6 @@ struct keystone_pcie {
>>>>>>>  	struct phy		**phy;
>>>>>>>  	struct device_link	**link;
>>>>>>>  	struct			device_node *msi_intc_np;
>>>>>>> -	struct irq_domain	*legacy_irq_domain;
>>>>>>>  	struct device_node	*np;
>>>>>>>  
>>>>>>>  	int error_irq;
>>>>>>> @@ -199,26 +198,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;
>>>>>>> -	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);
>>>>>>> -	}
>>>>>>> -
>>>>>>> -	/* 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);
>>>>>>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
>>>>>>>  	return IRQ_HANDLED;
>>>>>>>  }
>>>>>>>  
>>>>>>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d)
>>>>>>> +void ks_pcie_irq_eoi(struct irq_data *data)
>>>>>>>  {
>>>>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>>>>>> +	irq_hw_number_t hwirq = data->hwirq;
>>>>>>> +
>>>>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
>>>>>>> +	irq_chip_eoi_parent(data);
>>>>>>>  }
>>>>>>>  
>>>>>>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d)
>>>>>>> +void ks_pcie_irq_enable(struct irq_data *data)
>>>>>>>  {
>>>>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>>>>>> +	irq_hw_number_t hwirq = data->hwirq;
>>>>>>> +
>>>>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
>>>>>>> +	irq_chip_enable_parent(data);
>>>>>>>  }
>>>>>>>  
>>>>>>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
>>>>>>> +void ks_pcie_irq_disable(struct irq_data *data)
>>>>>>>  {
>>>>>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>>>>>> +	irq_hw_number_t hwirq = data->hwirq;
>>>>>>> +
>>>>>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
>>>>>>> +	irq_chip_disable_parent(data);
>>>>>>>  }
>>>>>>>  
>>>>>>>  static struct irq_chip ks_pcie_legacy_irq_chip = {
>>>>>>> -	.name = "Keystone-PCI-Legacy-IRQ",
>>>>>>> -	.irq_ack = ks_pcie_ack_legacy_irq,
>>>>>>> -	.irq_mask = ks_pcie_mask_legacy_irq,
>>>>>>> -	.irq_unmask = ks_pcie_unmask_legacy_irq,
>>>>>>> +	.name			= "Keystone-PCI-Legacy-IRQ",
>>>>>>> +	.irq_enable		= ks_pcie_irq_enable,
>>>>>>> +	.irq_disable		= ks_pcie_irq_disable,
>>>>>>> +	.irq_eoi		= ks_pcie_irq_eoi,
>>>>>>> +	.irq_mask		= irq_chip_mask_parent,
>>>>>>> +	.irq_unmask		= irq_chip_unmask_parent,
>>>>>>> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>>>>>>> +	.irq_set_type		= irq_chip_set_type_parent,
>>>>>>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>>>>>>  };
>>>>>>>  
>>>>>>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
>>>>>>> -				       unsigned int irq,
>>>>>>> -				       irq_hw_number_t hw_irq)
>>>>>>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
>>>>>>> +					   unsigned int virq,
>>>>>>> +					   unsigned int nr_irqs, void *data)
>>>>>>>  {
>>>>>>> -	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
>>>>>>> -				 handle_level_irq);
>>>>>>> -	irq_set_chip_data(irq, d->host_data);
>>>>>>> +	struct keystone_pcie *ks_pcie = domain->host_data;
>>>>>>> +	struct device_node *np = ks_pcie->legacy_intc_np;
>>>>>>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>>>>>>> +	struct of_phandle_args out_irq;
>>>>>>> +	int ret, i;
>>>>>>> +
>>>>>>> +	if (nr_irqs != 1)
>>>>>>> +		return -EINVAL;
>>>>>>> +
>>>>>>> +	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
>>>>>>> +	if (ret < 0) {
>>>>>>> +		pr_err("Failed to parse interrupt node\n");
>>>>>>> +		return ret;
>>>>>>> +	}  
>>>>>
>>>>> What it this trying to do? Fishing out the interrupts from DT based on
>>>>> the legacy pin? This looks at best obscure. I wonder why you don't do
>>>>> that at probe time instead. Anyway, this requires documenting.  
>>>>
>>>> The device-tree of PCIe node looks something like below.
>>>>
>>>> interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */
>>>> 		<0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */
>>>> 		<0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */
>>>> 		<0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */
>>>> pcie_intc0: legacy-interrupt-controller {
>>>> 	interrupt-controller;
>>>> 	#interrupt-cells = <2>;
>>>> 	interrupt-parent = <&gic>;
>>>> 	interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>,
>>>> 		     <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>,
>>>> 		     <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>,
>>>> 		     <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>;
>>>> };
>>>>
>>>> INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this
>>>> driver which in turn corresponds to GIC_SPI '48' of GIC.
>>>>
>>>> We could create an array of parent_fwspec for each of the four interrupt lines
>>>> in legacy-interrupt-controller during probe and use it directly here while
>>>> invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead
>>>> of using of_irq_parse_one here?  
>>>
>>> Up to you. I don't mind either way, but it is the lack of
>>> documentation that annoys me the most here.  
>>
>> Okay, I'll add a comment before invoking of_irq_parse_one.
>>>
>>> The more worrying part is that you are using an EDGE signalling in
>>> these bindings, and that's wrong. PCI legacy interrupts are level,
>>> always.  
>>
>> Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message.
> 
> I'm not convinced... See below.
> 
>>>
>>> [...]
>>>   
>>>>> From the DT binding:
>>>>>
>>>>> pcie_intc: Interrupt controller device node for Legacy IRQ chip
>>>>>         interrupt-cells: should be set to 1
>>>>>
>>>>> So why do we have 2 cells here?  
>>>>
>>>> With '1' cells, it's not possible to specify irq trigger type. DT
>>>> binding has to be fixed. I'll fix this in the next revision of the
>>>> patch series.  
>>>
>>> As I said above, this is wrong. Legacy interrupts are always level, by
>>> definition. Why are you making them edge-triggered
>>>   
>>>>>  
>>>>>>> +
>>>>>>> +		if (fwspec->param[0] >= PCI_NUM_INTX)
>>>>>>> +			return -EINVAL;  
>>>>>
>>>>> Most of the OF code assumes that the pin number describing the legacy
>>>>> interrupt is 1-based, while you obviously treat it as 0-based. How does
>>>>> it work?  
>>>>
>>>> INTA corresponds to '0' of the hierarchy interrupt domain (using
>>>> interrupt-map).  
>>>
>>> I'm not disputing this. It is more that the DT code does number the
>>> pin from 1, if memory serves well. Can you please verify this (see
>>> of_irq_parse_pci).  
>>
>> Yes it does number from '1'. However I use interrupt-map property to map from
>> legacy interrupt pin to a interrupt number of the legacy interrupt controller
>> which starts from '0'.
> 
> If you've verified that it actually works, fine bby me.
> 
>>>   
>>>>>  
>>>>>>> +
>>>>>>> +		*hwirq = fwspec->param[0];
>>>>>>> +		*type = fwspec->param[1];  
>>>>>
>>>>> As far as I remember, PCI legacy interrupts are level triggered, so
>>>>> there should be no need to advertise the trigger (which is consistent
>>>>> with the way the binding is written).  
>>>>
>>>> It is pulse triggered at subsystem level. Quoting the TRM
>>>> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is
>>>> triggered each time an assert interrupt message is received." The PCIe
>>>> subsystem also has a level signal (interrupt pending signal) but the interrupt
>>>> request signal is the one that is connected to GIC.  
>>>
>>> This looks completely wrong. MSIs are always edge, and legacy always
>>> level, end of story. If your HW is any different, then it doesn't seem
>>> to be compliant with PCI.  
>>
>> Agree that Legacy interrupts ought to be level-triggered. I checked this with
>> HW team and I've copy-pasted the response.
>>
>> "The legacy interrupts from the PCIe controller are still level. The PCIe
>> controller will hold the legacy interrupt lines high until a de-assert message
>> is sent back. The pulse interrupt is only to send the interrupt from the PCIe
>> sub-system to the SoC interrupt controller. This should not impact the
>> operation of the legacy interrupt on the PCIe side."
>>
>> Looks like the interrupts are propagated multiple levels and only the last
>> level to the interrupt controller is pulse.
> 
> Let's take an example: My device (let's assume a network controller of
> some sort) generates an interrupt. Level is high, and you observe an
> edge. The driver handles the interrupt, but due to a race, the line
> stays high (the device has received a new packet).
> 
> How is a *new* edge generated? The line never transited to low, and we
> have more packet to process. With a level triggered interrupt, you just
> take the interrupt again. With an edge interrupt, you need to actively
> resample the level and retrigger the interrupt. How is this done?

There might be an actual issue in the HW while converting from level to edge
where new edge interrupt might not be generated in the case you just mentioned.
I am checking this with HW folks.

Since this might take time, I'll remove this patch and resend only the MSI
cleanup in order to make progress.

Thanks
Kishon

Re: [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts

[Marc Zyngier]

On Tue, 19 Mar 2019 16:22:39 +0530
Kishon Vijay Abraham I <kishon@ti.com> wrote:

Hi Kishon,

> Hi Marc,
> 
> On 18/03/19 4:46 PM, Marc Zyngier wrote:
> > On Mon, 18 Mar 2019 11:03:07 +0530
> > Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >   
> >> Hi Marc,
> >>
> >> On 07/03/19 5:32 PM, Marc Zyngier wrote:  
> >>> On Thu, 07 Mar 2019 09:12:30 +0000,
> >>> Kishon Vijay Abraham I <kishon@ti.com> wrote:    


[...]

> >>>> It is pulse triggered at subsystem level. Quoting the TRM
> >>>> "The interrupt request signal at the PCIe SS boundary is a pulse signal that is
> >>>> triggered each time an assert interrupt message is received." The PCIe
> >>>> subsystem also has a level signal (interrupt pending signal) but the interrupt
> >>>> request signal is the one that is connected to GIC.    
> >>>
> >>> This looks completely wrong. MSIs are always edge, and legacy always
> >>> level, end of story. If your HW is any different, then it doesn't seem
> >>> to be compliant with PCI.    
> >>
> >> Agree that Legacy interrupts ought to be level-triggered. I checked this with
> >> HW team and I've copy-pasted the response.
> >>
> >> "The legacy interrupts from the PCIe controller are still level. The PCIe
> >> controller will hold the legacy interrupt lines high until a de-assert message
> >> is sent back. The pulse interrupt is only to send the interrupt from the PCIe
> >> sub-system to the SoC interrupt controller. This should not impact the
> >> operation of the legacy interrupt on the PCIe side."
> >>
> >> Looks like the interrupts are propagated multiple levels and only the last
> >> level to the interrupt controller is pulse.  
> > 
> > Let's take an example: My device (let's assume a network controller of
> > some sort) generates an interrupt. Level is high, and you observe an
> > edge. The driver handles the interrupt, but due to a race, the line
> > stays high (the device has received a new packet).
> > 
> > How is a *new* edge generated? The line never transited to low, and we
> > have more packet to process. With a level triggered interrupt, you just
> > take the interrupt again. With an edge interrupt, you need to actively
> > resample the level and retrigger the interrupt. How is this done?  
> 
> There might be an actual issue in the HW while converting from level to edge
> where new edge interrupt might not be generated in the case you just mentioned.
> I am checking this with HW folks.

There is (non-PCI) HW that does that. On EOI, they force the interrupt
controller to resample the level, which results in a new edge to be
generated if the level is high. This of course require some HW, usually
a register taking the ID of the line that needs resampling. I'd be
surprised if there wasn't something along those lines in your HW, at
least as a debug mechanism.

> Since this might take time, I'll remove this patch and resend only the MSI
> cleanup in order to make progress.

OK.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.