Re: [PATCH] irqchip/ath79-cpu: Flush every write buffer of a shared interrupt

[Thomas Gleixner]

On Fri, Nov 06 2026 at 14:12, Rosen Penev wrote:
>  
> -static unsigned irq_wb_chan[8] = {
> -	-1, -1, -1, -1, -1, -1, -1, -1,
> -};
> +static u32 irq_wb_chans[8];
>  
>  asmlinkage void plat_irq_dispatch(void)
>  {
> -	unsigned long pending;
> -	int irq;
> +	unsigned long pending, chans;
> +	int irq, chan;
>  
>  	pending = read_c0_status() & read_c0_cause() & ST0_IM;
>  
> @@ -46,8 +47,11 @@ asmlinkage void plat_irq_dispatch(void)
>  	pending >>= CAUSEB_IP;
>  	while (pending) {
>  		irq = fls(pending) - 1;
> -		if (irq < ARRAY_SIZE(irq_wb_chan) && irq_wb_chan[irq] != -1)
> -			ath79_ddr_wb_flush(irq_wb_chan[irq]);
> +		if (irq < ARRAY_SIZE(irq_wb_chans)) {
> +			chans = irq_wb_chans[irq];
> +			for_each_set_bit(chan, &chans, 32)
> +				ath79_ddr_wb_flush(chan);

So the CPU provides 8 interrupts and 32 sources, where each source has
its own specific flush register. There is some distribution mechanism
(firmware configured or fixed in hardware) which maps a interrupt source
to a CPU interrupt. When two sources are mapped to the same CPU
interrupt then they share the interrupt line, i.e. the handlers are
requested with IRQF_SHARED, _and_ both flush registers have to be
written to so that the interrupt handler can observe the data and figure
out whether it should handle it or not. Correct?

I kinda decoded this from your change log and tried to map it to the
code. It looks correct, but for heavens sake this needs a bit fat
comment explaining the magic behind all of this. Even for your own sake
as you will have forgotten the gory details three month down the road.

Btw, as this is the interrupt hotpath, this for_each_set_bit() loop is
pretty inefficient when there is more than one interrupt source
associated with a CPU interrupt.

What you want (as a patch on top) is:

ath79_ddr_wb_flush_many(unsigned long sources)
{
	__ath79_ddr_wb_flush_many(sources);
        // Document WHY this needs to be done twice
        // The existing comment is useless...
	__ath79_ddr_wb_flush_many(sources);
}

static __ath79_ddr_wb_flush_many(unsigned long sources)
{
	u32 __iomem *base = ath79_ddr_wb_flush_base;

	for_each_set_bit(idx, &sources, 32)
        	__raw_writel(0x1, base + idx)

	for_each_set_bit(idx, &sources, 32) {
        	while (__raw_readl(base + idx) & 0x1)
                	;
	}
}

or something like that. For a single interrupt it's exactly the same
outcome, but for multiple interrupts mapped to one CPU interrupt there
is an obvious speedup potential, no?

Thanks,

        tglx