[PATCH v2 1/3] irqchip/irq-pruss-intc: Fix enabling of intc events

Meghana Malladi posted 3 patches 1 month, 1 week ago
[PATCH v2 1/3] irqchip/irq-pruss-intc: Fix enabling of intc events
Posted by Meghana Malladi 1 month, 1 week ago
From: Suman Anna <s-anna@ti.com>

PRUSS INTC events are enabled by default once IRQ events are mapped to
channel:host pair. This may cause issues with undesirable IRQs triggering
even before a PRU IRQ is requested which are silently processed by
pruss_intc_irq_handler().

Fix it by masking all events by default except those which are routed to
various PRU cores (Host IRQs 0, 1; 10 through 19 on K3 SoCs), and any
other reserved IRQs routed to other processors. The unmasking of IRQs is
the responsibility of Linux IRQ core when IRQ is actually requested.

Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Link: https://lore.kernel.org/all/20230919061900.369300-2-danishanwar@ti.com/
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/irqchip/irq-pruss-intc.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 81078d56f38d..4ec1f4fc491f 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -101,6 +101,7 @@ struct pruss_intc_match_data {
  * @soc_config: cached PRUSS INTC IP configuration data
  * @dev: PRUSS INTC device pointer
  * @lock: mutex to serialize interrupts mapping
+ * @irqs_reserved: bit-mask of reserved host interrupts
  */
 struct pruss_intc {
 	struct pruss_intc_map_record event_channel[MAX_PRU_SYS_EVENTS];
@@ -111,6 +112,7 @@ struct pruss_intc {
 	const struct pruss_intc_match_data *soc_config;
 	struct device *dev;
 	struct mutex lock; /* PRUSS INTC lock */
+	u8 irqs_reserved;
 };
 
 /**
@@ -178,6 +180,7 @@ static void pruss_intc_update_hmr(struct pruss_intc *intc, u8 ch, u8 host)
 static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
 {
 	struct device *dev = intc->dev;
+	bool enable_hwirq = false;
 	u8 ch, host, reg_idx;
 	u32 val;
 
@@ -187,6 +190,9 @@ static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
 
 	ch = intc->event_channel[hwirq].value;
 	host = intc->channel_host[ch].value;
+	enable_hwirq = (host < FIRST_PRU_HOST_INT ||
+			host >= FIRST_PRU_HOST_INT + MAX_NUM_HOST_IRQS ||
+			intc->irqs_reserved & BIT(host - FIRST_PRU_HOST_INT));
 
 	pruss_intc_update_cmr(intc, hwirq, ch);
 
@@ -194,8 +200,10 @@ static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
 	val = BIT(hwirq  % 32);
 
 	/* clear and enable system event */
-	pruss_intc_write_reg(intc, PRU_INTC_ESR(reg_idx), val);
 	pruss_intc_write_reg(intc, PRU_INTC_SECR(reg_idx), val);
+	/* unmask only events going to various PRU and other cores by default */
+	if (enable_hwirq)
+		pruss_intc_write_reg(intc, PRU_INTC_ESR(reg_idx), val);
 
 	if (++intc->channel_host[ch].ref_count == 1) {
 		pruss_intc_update_hmr(intc, ch, host);
@@ -204,7 +212,8 @@ static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
 		pruss_intc_write_reg(intc, PRU_INTC_HIEISR, host);
 	}
 
-	dev_dbg(dev, "mapped system_event = %lu channel = %d host = %d",
+	dev_dbg(dev, "mapped%s system_event = %lu channel = %d host = %d",
+		enable_hwirq ? " and enabled" : "",
 		hwirq, ch, host);
 
 	mutex_unlock(&intc->lock);
@@ -268,11 +277,14 @@ static void pruss_intc_init(struct pruss_intc *intc)
 
 	/*
 	 * configure polarity (SIPR register) to active high and
-	 * type (SITR register) to level interrupt for all system events
+	 * type (SITR register) to level interrupt for all system events,
+	 * and disable and clear all the system events
 	 */
 	for (i = 0; i < num_event_type_regs; i++) {
 		pruss_intc_write_reg(intc, PRU_INTC_SIPR(i), 0xffffffff);
 		pruss_intc_write_reg(intc, PRU_INTC_SITR(i), 0);
+		pruss_intc_write_reg(intc, PRU_INTC_ECR(i), 0xffffffff);
+		pruss_intc_write_reg(intc, PRU_INTC_SECR(i), 0xffffffff);
 	}
 
 	/* clear all interrupt channel map registers, 4 events per register */
@@ -521,7 +533,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
 	struct pruss_intc *intc;
 	struct pruss_host_irq_data *host_data;
 	int i, irq, ret;
-	u8 max_system_events, irqs_reserved = 0;
+	u8 max_system_events;
 
 	data = of_device_get_match_data(dev);
 	if (!data)
@@ -542,7 +554,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
 		return PTR_ERR(intc->base);
 
 	ret = of_property_read_u8(dev->of_node, "ti,irqs-reserved",
-				  &irqs_reserved);
+				  &intc->irqs_reserved);
 
 	/*
 	 * The irqs-reserved is used only for some SoC's therefore not having
@@ -561,7 +573,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
-		if (irqs_reserved & BIT(i))
+		if (intc->irqs_reserved & BIT(i))
 			continue;
 
 		irq = platform_get_irq_byname(pdev, irq_names[i]);
-- 
2.43.0
Re: [PATCH v2 1/3] irqchip/irq-pruss-intc: Fix enabling of intc events
Posted by Thomas Gleixner 1 month, 1 week ago
On Wed, Feb 18 2026 at 15:07, Meghana Malladi wrote:
> From: Suman Anna <s-anna@ti.com>
>
> PRUSS INTC events are enabled by default once IRQ events are mapped to

Please s/IRQ/interrupt/

Change logs are prosa and not twatter messages.

> channel:host pair. This may cause issues with undesirable IRQs triggering
> even before a PRU IRQ is requested which are silently processed by
> pruss_intc_irq_handler().

This sentence does not make any sense. 

> Fix it by masking all events by default except those which are routed to
> various PRU cores (Host IRQs 0, 1; 10 through 19 on K3 SoCs), and any
> other reserved IRQs routed to other processors. The unmasking of IRQs is
> the responsibility of Linux IRQ core when IRQ is actually requested.
>
> Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Link: https://lore.kernel.org/all/20230919061900.369300-2-danishanwar@ti.com/

What's this link for? Put a link into the cover letter which points to
the V1 submission.

> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>

This S-O-B chain is completely broken. Please read:
 
  https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

and the subsequent chapters.

>  struct pruss_intc {
>  	struct pruss_intc_map_record event_channel[MAX_PRU_SYS_EVENTS];
> @@ -111,6 +112,7 @@ struct pruss_intc {
>  	const struct pruss_intc_match_data *soc_config;
>  	struct device *dev;
>  	struct mutex lock; /* PRUSS INTC lock */
> +	u8 irqs_reserved;

In the changelog you explain that host interrupt 0,1 and on K3 SoCs
interrupts 10-19 are reserved and treated differently. How do they fit
into an u8? I'm probably failing to understand the word salad in the
change log, but that doesn't make any better.

> @@ -187,6 +190,9 @@ static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
>  
>  	ch = intc->event_channel[hwirq].value;
>  	host = intc->channel_host[ch].value;
> +	enable_hwirq = (host < FIRST_PRU_HOST_INT ||
> +			host >= FIRST_PRU_HOST_INT + MAX_NUM_HOST_IRQS ||
> +			intc->irqs_reserved & BIT(host - FIRST_PRU_HOST_INT));

Seriously? This is incomprehensible. What's wrong with doing the
obvious:

struct pruss_intc {
       ...
       u32	irqs_reserved;
};

Fill that in the probe function with all bits which are reserved and do

     enable = !!(intc->irqs_reserved & BIT(host_irq));

That'd be not convoluted enough, right?

>  	pruss_intc_update_cmr(intc, hwirq, ch);
>  
> @@ -194,8 +200,10 @@ static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
>  	val = BIT(hwirq  % 32);
>  
>  	/* clear and enable system event */
> -	pruss_intc_write_reg(intc, PRU_INTC_ESR(reg_idx), val);
>  	pruss_intc_write_reg(intc, PRU_INTC_SECR(reg_idx), val);
> +	/* unmask only events going to various PRU and other cores by default */

Sentences start with an upper case letter.

> +	if (enable_hwirq)
> +		pruss_intc_write_reg(intc, PRU_INTC_ESR(reg_idx), val);

Thanks,

        tglx