diff mbox series

[5/6] PCI: keystone: Add PCI legacy interrupt support for AM654

Message ID 20210325090026.8843-6-kishon@ti.com (mailing list archive)
State Changes Requested
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: Add legacy interrupt support in Keystone | expand

Commit Message

Kishon Vijay Abraham I March 25, 2021, 9 a.m. UTC
Add PCI legacy interrupt support for AM654. AM654 has a single HW
interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD.
The HW interrupt line connected to GIC is a pulse interrupt whereas
the legacy interrupts by definition is level interrupt. In order to
provide level interrupt functionality to edge interrupt line, PCIe
in AM654 has provided IRQ_EOI register. When the SW writes to IRQ_EOI
register after handling the interrupt, the IP checks the state of
legacy interrupt and re-triggers pulse interrupt invoking the handler
again.

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

Comments

Krzysztof WilczyƄski March 26, 2021, 7:14 a.m. UTC | #1
Hi Kishon,

[...]
> +	if (!legacy_irq_domain) {
> +		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
> +		return -EINVAL;
> +	}
[...]

It would be "IRQ" and "IRQs" in the message above.

[...]
> -	ret = ks_pcie_config_legacy_irq(ks_pcie);
> -	if (ret)
> -		return ret;
> +	if (!ks_pcie->is_am6) {
> +		pp->bridge->child_ops = &ks_child_pcie_ops;
> +		ret = ks_pcie_config_legacy_irq(ks_pcie);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = ks_pcie_am654_config_legacy_irq(ks_pcie);
> +		if (ret)
> +			return ret;
> +	}
[...]

What if we change this to the following:

	if (!ks_pcie->is_am6) {
		pp->bridge->child_ops = &ks_child_pcie_ops;
		ret = ks_pcie_config_legacy_irq(ks_pcie);
	} else {
		ret = ks_pcie_am654_config_legacy_irq(ks_pcie);
	}
	
	if (ret)
	  	return ret;

Not sure if this is something you would prefer, but it seems that either
of the functions can set "ret", so checking immediately after would be
the same as checking in either of the branches.  But, this is a matter
of style, so it would be up to you - not sure what do you prefer.

Krzysztof
Marc Zyngier April 2, 2021, 11:11 a.m. UTC | #2
On Thu, 25 Mar 2021 09:00:25 +0000,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Add PCI legacy interrupt support for AM654. AM654 has a single HW
> interrupt line for all the four legacy interrupts INTA/INTB/INTC/INTD.
> The HW interrupt line connected to GIC is a pulse interrupt whereas
> the legacy interrupts by definition is level interrupt. In order to
> provide level interrupt functionality to edge interrupt line, PCIe
> in AM654 has provided IRQ_EOI register. When the SW writes to IRQ_EOI
> register after handling the interrupt, the IP checks the state of
> legacy interrupt and re-triggers pulse interrupt invoking the handler
> again.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 87 +++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index dfa9a7fcf9b7..84a25207d0d3 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -118,6 +118,7 @@ struct keystone_pcie {
>  	/* PCI Device ID */
>  	u32			device_id;
>  	struct			device_node *legacy_intc_np;
> +	struct irq_domain	*legacy_irq_domain;
>  
>  	int			msi_host_irq;
>  	int			num_lanes;
> @@ -289,6 +290,29 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
>  	return IRQ_HANDLED;
>  }
>  
> +static void ks_pcie_am654_legacy_irq_handler(struct irq_desc *desc)
> +{
> +	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	int virq, i;
> +	u32 reg;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	for (i = 0; i < PCI_NUM_INTX; i++) {
> +		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(i));
> +		if (!(reg & INTx_EN))
> +			continue;
> +
> +		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, i);
> +		generic_handle_irq(virq);

I'm on my way to kill irq_linear_revmap(), so I'd rather you didn't
add more instances. Consider using irq_find_mapping() instead.

> +		ks_pcie_app_writel(ks_pcie, IRQ_STATUS(i), INTx_EN);
> +		ks_pcie_app_writel(ks_pcie, IRQ_EOI, i);

What are these writes for? The first one feels like an Ack, and the
second one has EOI written over it.

If that's what they are, llease move these to a proper irq_chip
structure and use the appropriate flow handler, instead of
dummy_irq_chip and handle_simple_irq.

Thanks,

	M.
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index dfa9a7fcf9b7..84a25207d0d3 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -118,6 +118,7 @@  struct keystone_pcie {
 	/* PCI Device ID */
 	u32			device_id;
 	struct			device_node *legacy_intc_np;
+	struct irq_domain	*legacy_irq_domain;
 
 	int			msi_host_irq;
 	int			num_lanes;
@@ -289,6 +290,29 @@  static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
 	return IRQ_HANDLED;
 }
 
+static void ks_pcie_am654_legacy_irq_handler(struct irq_desc *desc)
+{
+	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int virq, i;
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		reg = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(i));
+		if (!(reg & INTx_EN))
+			continue;
+
+		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, i);
+		generic_handle_irq(virq);
+		ks_pcie_app_writel(ks_pcie, IRQ_STATUS(i), INTx_EN);
+		ks_pcie_app_writel(ks_pcie, IRQ_EOI, i);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
 void ks_pcie_irq_eoi(struct irq_data *data)
 {
 	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
@@ -728,6 +752,54 @@  static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 	return ret;
 }
 
+static int ks_pcie_am654_intx_map(struct irq_domain *domain, unsigned int irq,
+				  irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops ks_pcie_am654_irq_domain_ops = {
+	.map = ks_pcie_am654_intx_map,
+};
+
+static int ks_pcie_am654_config_legacy_irq(struct keystone_pcie *ks_pcie)
+{
+	struct device *dev = ks_pcie->pci->dev;
+	struct irq_domain *legacy_irq_domain;
+	struct device_node *np = ks_pcie->np;
+	struct device_node *intc_np;
+	int ret = 0;
+	int irq;
+	int i;
+
+	intc_np = of_get_child_by_name(np, "interrupt-controller");
+	if (!intc_np) {
+		dev_warn(dev, "legacy interrupt-controller node is absent\n");
+		return -EINVAL;
+	}
+
+	irq = irq_of_parse_and_map(intc_np, 0);
+	if (!irq)
+		return -EINVAL;
+
+	irq_set_chained_handler_and_data(irq, ks_pcie_am654_legacy_irq_handler, ks_pcie);
+	legacy_irq_domain = irq_domain_add_linear(intc_np, PCI_NUM_INTX,
+						  &ks_pcie_am654_irq_domain_ops, ks_pcie);
+	if (!legacy_irq_domain) {
+		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
+		return -EINVAL;
+	}
+	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);
+
+	return ret;
+}
+
 static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 {
 	struct device *dev = ks_pcie->pci->dev;
@@ -835,12 +907,17 @@  static int __init ks_pcie_host_init(struct pcie_port *pp)
 	int ret;
 
 	pp->bridge->ops = &ks_pcie_ops;
-	if (!ks_pcie->is_am6)
-		pp->bridge->child_ops = &ks_child_pcie_ops;
 
-	ret = ks_pcie_config_legacy_irq(ks_pcie);
-	if (ret)
-		return ret;
+	if (!ks_pcie->is_am6) {
+		pp->bridge->child_ops = &ks_child_pcie_ops;
+		ret = ks_pcie_config_legacy_irq(ks_pcie);
+		if (ret)
+			return ret;
+	} else {
+		ret = ks_pcie_am654_config_legacy_irq(ks_pcie);
+		if (ret)
+			return ret;
+	}
 
 	ret = ks_pcie_config_msi_irq(ks_pcie);
 	if (ret)