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

[Thomas Gleixner]

On Wed, Feb 18 2026 at 15:07, Meghana Malladi wrote:
> From: Suman Anna <s-anna@xxxxxx>
>
> 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@xxxxxx>
> Signed-off-by: Suman Anna <s-anna@xxxxxx>
> Link: https://lore.kernel.org/all/20230919061900.369300-2-danishanwar@xxxxxx/

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@xxxxxx>
> Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
> Signed-off-by: Meghana Malladi <m-malladi@xxxxxx>

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