Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts

From: Marc Zyngier
Date: Thu Jul 02 2020 - 13:44:37 EST

On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
From: Suman Anna <s-anna@xxxxxx>

The PRUSS INTC has a fixed number of output interrupt lines that are
connected to a number of processors or other PRUSS instances or other
devices (like DMA) on the SoC. The output interrupt lines 2 through 9
are usually connected to the main Arm host processor and are referred
to as host interrupts 0 through 7 from ARM/MPU perspective.

All of these 8 host interrupts are not always exclusively connected
to the Arm interrupt controller. Some SoCs have some interrupt lines
not connected to the Arm interrupt controller at all, while a few others
have the interrupt lines connected to multiple processors in which they
need to be partitioned as per SoC integration needs. For example, AM437x
and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 5
connected to the other PRUSS, while AM335x has host interrupt 0 shared
between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU and
a DMA controller.

Add support to the PRUSS INTC driver to allow both these shared and
invalid interrupts by not returning a failure if any of these interrupts
are skipped from the corresponding INTC DT node.

That's not exactly "adding support", is it? It really is "ignore these
interrupts because they are useless from the main CPU's perspective",
right?


Signed-off-by: Suman Anna <s-anna@xxxxxx>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@xxxxxxxxxx>
---
v2->v3:
- Extra checks for (intc->irqs[i]) in error/remove path was moved from
"irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
interrupts" to this patch
v1->v2:
- https://patchwork.kernel.org/patch/11069757/
---
drivers/irqchip/irq-pruss-intc.c | 73 +++++++++++++++++++++++++++++++++++++---
1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index fb3dda3..49c936f 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -65,11 +65,15 @@
* @irqs: kernel irq numbers corresponding to PRUSS host interrupts
* @base: base virtual address of INTC register space
* @domain: irq domain for this interrupt controller
+ * @shared_intr: bit-map denoting if the MPU host interrupt is shared

nit: bitmap

+ * @invalid_intr: bit-map denoting if host interrupt is not connected to MPU
*/
struct pruss_intc {
unsigned int irqs[MAX_NUM_HOST_IRQS];
void __iomem *base;
struct irq_domain *domain;
+ u16 shared_intr;
+ u16 invalid_intr;

Please represent bitmaps as an unsigned long.

};

static inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
unsigned int reg)
@@ -222,7 +226,8 @@ static int pruss_intc_probe(struct platform_device *pdev)
"host_intr4", "host_intr5", "host_intr6", "host_intr7", };
struct device *dev = &pdev->dev;
struct pruss_intc *intc;
- int i, irq;
+ int i, irq, count;
+ u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };

intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
if (!intc)
@@ -235,6 +240,52 @@ static int pruss_intc_probe(struct platform_device *pdev)
return PTR_ERR(intc->base);
}

+ count = of_property_read_variable_u8_array(dev->of_node,
+ "ti,irqs-reserved",
+ temp_intr, 0,
+ MAX_NUM_HOST_IRQS);
+ /*
+ * The irqs-reserved is used only for some SoC's therefore not having
+ * this property is still valid
+ */
+ if (count == -EINVAL)
+ count = 0;
+ if (count < 0)
+ return count;
+
+ for (i = 0; i < count; i++) {
+ if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
+ dev_warn(dev, "ignoring invalid reserved irq %d\n",
+ temp_intr[i]);
+ continue;
+ }
+
+ intc->invalid_intr |= BIT(temp_intr[i]);
+ }
+
+ count = of_property_read_variable_u8_array(dev->of_node,
+ "ti,irqs-shared",
+ temp_intr, 0,
+ MAX_NUM_HOST_IRQS);
+ /*
+ * The irqs-shared is used only for some SoC's therefore not having
+ * this property is still valid
+ */
+ if (count == -EINVAL)
+ count = 0;
+ if (count < 0)
+ return count;
+
+ for (i = 0; i < count; i++) {
+ if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
+ dev_warn(dev, "ignoring invalid shared irq %d\n",
+ temp_intr[i]);
+ continue;
+ }
+
+ intc->shared_intr |= BIT(temp_intr[i]);
+ }
+

You probably want to move this in a separate function, since you populate a
common structure.

pruss_intc_init(intc);

/* always 64 events */
@@ -244,8 +295,14 @@ static int pruss_intc_probe(struct platform_device *pdev)
return -ENOMEM;

for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
+ if (intc->invalid_intr & BIT(i))
+ continue;
+
irq = platform_get_irq_byname(pdev, irq_names[i]);
if (irq <= 0) {
+ if (intc->shared_intr & BIT(i))
+ continue;

I don't really understand why you are treating these "shared" interrupts
differently from the invalid ones. In all cases, they shouldn't be used.

+
dev_err(dev, "platform_get_irq_byname failed for %s : %d\n",
irq_names[i], irq);
goto fail_irq;
@@ -259,8 +316,11 @@ static int pruss_intc_probe(struct platform_device *pdev)
return 0;

fail_irq:
- while (--i >= 0)
- irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
+ while (--i >= 0) {
+ if (intc->irqs[i])
+ irq_set_chained_handler_and_data(intc->irqs[i], NULL,
+ NULL);
+ }

irq_domain_remove(intc->domain);

@@ -273,8 +333,11 @@ static int pruss_intc_remove(struct platform_device *pdev)
unsigned int hwirq;
int i;

- for (i = 0; i < MAX_NUM_HOST_IRQS; i++)
- irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
+ for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
+ if (intc->irqs[i])
+ irq_set_chained_handler_and_data(intc->irqs[i], NULL,
+ NULL);
+ }

for (hwirq = 0; hwirq < MAX_PRU_SYS_EVENTS; hwirq++)
irq_dispose_mapping(irq_find_mapping(intc->domain, hwirq));

Thanks,

M.
--
Jazz is not dead. It just smells funny...