Re: [PATCH v5 3/3] irqchip/plic: add support for UltraRISC DP1000 PLIC

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Oct 16 2025 at 11:54, Charles Mirabile wrote:
> On Thu, Oct 16, 2025 at 9:17 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> > +static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler,
>> > +                                     void __iomem *claim)
>> > +{
>> > +     int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32);
>> > +     void __iomem *pending = handler->priv->regs + PENDING_BASE;
>> > +     void __iomem *enable = handler->enable_base;
>> > +     irq_hw_number_t hwirq = 0;
>> > +     int i;
>> > +
>> > +     guard(raw_spinlock)(&handler->enable_lock);
>> > +
>> > +     /* Save current interrupt enable state */
>> > +     for (i = 0; i < nr_irq_groups; i++)
>> > +             handler->enable_save[i] = readl_relaxed(enable + i * sizeof(u32));
>>
>> This is truly the most inefficient way to solve that problem. The enable
>> registers are modified with enabled_lock held, so you can just cache the
>> value in plic_handler::enabled_save and avoid this read loop completely.
>> After claiming the interrupt you restore from that cache, no?
>
> You mean touch the other functions where the enable bits are modified
> to keep the cache in sync so that we don't need to do this read loop
> and can have a proper set of values cached?
>
> My concern is that this obviously has an impact on other platforms
> which do not have this quirk since keeping the cache in sync would get
> pushed all throughout the driver.

The irq_enable()/disable() callbacks are not really hotpath and caching
the bit in plic_toggle() or such is just not measurable overhead
compared to the register access.

>> Now for the search and disable mechanism. Of course you need to search
>> for th pending interrupt first, but then you can make that masking loop
>> very simple by having a plic_handler::enabled_clear[] array which is
>> zeroed on initialization:
>>
>>         unsigned long pending = 0;
>>
>>         for (group = 0; !pending && group < nr_irq_groups; group++) {
>>                 pending = handler->enabled_save[i];
>>                 pending =& readl_relaxed(pending + group * sizeof(u32));
>>         }
>>         if (!pending)
>>                 return false;
>>
>>         bit = ffs(pending) - 1;
>>         handler->enabled_clear[group] |= BIT(bit);
>>         for (int i = 0; i < nr_irq_groups; i++)
>>                 writel_relaxed(handler->enabled_clear[i], enable + i * sizeof(u32));
>>         handler->enabled_clear[group] = 0;
>>
>> No?
>
> Sure that would also work, but why are we using ffs (slow) only to
> shift the result back to make a new mask when (x & -x) is faster and
> skips the intermediate step delivering immediately the mask of the
> lowest bit.

Because I did not spend time thinking about it. 

> As for making another caching array, I guess, but again that is just a
> time vs space trade off with its own invariants to maintain that would
> also impact other platforms.

It's a pointer in struct plic_handler (or whatever it's named) and you
can allocate it when the quirk is required. The pointer is definitely
not a burden for anyone else.

>> Is the device B interrupt preserved in the interrupt chip and actually
>> raised when the interrupt enable bit is restored or is it lost?
>
> I am not sure how to verify this other than to tell you that without
> this quirk (i.e. trying to use normal plic behavior) the device does
> not work, but with this quirk I can boot to a desktop with a pcie
> graphics card and storage, use networking etc that all obviously
> depend on the correct functioning of the interrupt controller.
>
> My reading of the spec for PLIC also suggests (but does not explicitly
> confirm) that the pending bits function irrespective of the state of
> the corresponding enable bit: "A pending bit in the PLIC core can be
> cleared by setting the associated enable bit then performing a claim."
> (page 14 plic spec 1.0.0 [1]).
>
> This sentence implies to me that it is possible for a pending bit to
> be set even though the corresponding enable bit is not, which lends
> credence to the idea that the pending bits operate independently.

Looks like that. Please add a comment to that effect then.

Thanks,

        tglx