Message-ID: <95a777a7-4998-4451-b271-8450a5b674bb@sifive.com>
Date: Thu, 16 Oct 2025 17:01:25 -0500
From: Samuel Holland <samuel.holland@...ive.com>
To: Bo Gan <ganboing@...il.com>, Lucas Zampieri <lzampier@...hat.com>
Cc: Charles Mirabile <cmirabil@...hat.com>,
 Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Alexandre Ghiti <alex@...ti.fr>, Vivian Wang <dramforever@...e.com>,
 devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
 Zhang Xincheng <zhangxincheng@...rarisc.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] irqchip/plic: add support for UltraRISC DP1000
 PLIC

Hi Bo,

On 2025-10-16 4:28 PM, Bo Gan wrote:
> Hi Lucas, Charles,
> 
> I just realized your last reply and sorry about the messy formatting.
> Please disregard the previous one from me and use this one.
> 
> On 10/16/25 01:42, Lucas Zampieri wrote:
>> From: Charles Mirabile <cmirabil@...hat.com>
>>
>> Add a new compatible for the plic found in UltraRISC DP1000 with a quirk to
>> work around a known hardware bug with IRQ claiming in the UR-CP100 cores.
>>
>> When claiming an interrupt on UR-CP100 cores, all other interrupts must be
>> disabled before the claim register is accessed to prevent incorrect
>> handling of the interrupt. This is a hardware bug in the CP100 core
>> implementation, not specific to the DP1000 SoC.
>>
>> When the PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM flag is present, a specialized
>> handler (plic_handle_irq_cp100) saves the enable state of all interrupts,
>> disables all interrupts except for the first pending one before reading the
>> claim register, and then restores the interrupts before further processing of
>> the claimed interrupt continues.
>>
>> The driver matches on "ultrarisc,cp100-plic" to apply the quirk to all
>> SoCs using UR-CP100 cores, regardless of the specific SoC implementation.
>> This has no impact on other platforms.
>>
>> Co-developed-by: Zhang Xincheng <zhangxincheng@...rarisc.com>
>> Signed-off-by: Zhang Xincheng <zhangxincheng@...rarisc.com>
>> Signed-off-by: Charles Mirabile <cmirabil@...hat.com>
>> Acked-by: Samuel Holland <samuel.holland@...ive.com>
>> Signed-off-by: Lucas Zampieri <lzampier@...hat.com>
>> ---
>>   drivers/irqchip/irq-sifive-plic.c | 94 ++++++++++++++++++++++++++++++-
>>   1 file changed, 93 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-
>> plic.c
>> index bf69a4802b71..0428e9f3423d 100644
>> --- a/drivers/irqchip/irq-sifive-plic.c
>> +++ b/drivers/irqchip/irq-sifive-plic.c
>> @@ -49,6 +49,8 @@
>>   #define CONTEXT_ENABLE_BASE        0x2000
>>   #define     CONTEXT_ENABLE_SIZE        0x80
>>   +#define PENDING_BASE                    0x1000
>> +
>>   /*
>>    * Each hart context has a set of control registers associated with it.  Right
>>    * now there's only two: a source priority threshold over which the hart will
>> @@ -63,6 +65,7 @@
>>   #define    PLIC_ENABLE_THRESHOLD        0
>>     #define PLIC_QUIRK_EDGE_INTERRUPT    0
>> +#define PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM    1
>>     struct plic_priv {
>>       struct fwnode_handle *fwnode;
>> @@ -394,6 +397,89 @@ static void plic_handle_irq(struct irq_desc *desc)
>>       chained_irq_exit(chip, desc);
>>   }
>>   +static bool cp100_isolate_pending_irq(int nr_irq_groups, u32 ie[],
>> +                       void __iomem *pending,
>> +                       void __iomem *enable)
>> +{
>> +    u32 pending_irqs = 0;
>> +    int i, j;
>> +
>> +    /* Look for first pending interrupt */
>> +    for (i = 0; i < nr_irq_groups; i++) {
>> +        pending_irqs = ie[i] & readl_relaxed(pending + i * sizeof(u32));
>> +        if (pending_irqs)
>> +            break;
> 
> No need to start from group 0. Only readl on the group with ie[i] != 0
> 
>> +    }
>> +
>> +    if (!pending_irqs)
>> +        return false;
>> +
>> +    /* Disable all interrupts but the first pending one */
>> +    for (j = 0; j < nr_irq_groups; j++) {
>> +        u32 new_mask = 0;
>> +
>> +        if (j == i) {
>> +            /* Extract mask with lowest set bit */
>> +            new_mask = (pending_irqs & -pending_irqs);
>> +        }
>> +
>> +        writel_relaxed(new_mask, enable + j * sizeof(u32));
> 
> 
> There's no need to write the register if the value isn't changing. You can
> check new_mask with the value in ie[].
> 
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +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));
> 
> 
> I see that you start to use handler->enable_save to track HW in the last reply.
> I'm about to suggest that. Please send out a new patch, so people can properly
> review it. There's change to common code path.
> 
>> +
>> +    if (!cp100_isolate_pending_irq(nr_irq_groups, handler->enable_save,
>> pending, enable))
>> +        return 0;
>> +
>> +    hwirq = readl(claim);
> 
> Possibly missing a io barrier. readl isn't going to enforce the ordering of
> readl/writel_relaxed above and itself. There could be other barriers missing.
> Please check.

There is no missing barrier. Linux requires the hardware to enforce this
ordering. See the comment in asm/mmio.h:

/*
 * Relaxed I/O memory access primitives. These follow the Device memory
 * ordering rules but do not guarantee any ordering relative to Normal memory
 * accesses.  These are defined to order the indicated access (either a read or
 * write) with all other I/O memory accesses to the same peripheral. Since the
 * platform specification defines that all I/O regions are strongly ordered on
 * channel 0, no explicit fences are required to enforce this ordering.
 */

where "strongly ordered" is defined by the privileged ISA: "accesses to an I/O
region with strong ordering are generally observed by other harts and bus
mastering devices in program order."

Barriers are only needed if there are ordering requirements between I/O accesses
to multiple MMIO regions, or between I/O and normal memory (e.g. locks and DMA).

Regards,
Samuel

>> +
>> +    /* Restore previous state */
>> +    for (i = 0; i < nr_irq_groups; i++)
>> +        writel_relaxed(handler->enable_save[i], enable + i * sizeof(u32));
>> +
>> +    return hwirq;
>> +}
>> +
>> +static void plic_handle_irq_cp100(struct irq_desc *desc)
>> +{
>> +    struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
>> +    struct irq_chip *chip = irq_desc_get_chip(desc);
>> +    void __iomem *claim = handler->hart_base + CONTEXT_CLAIM;
>> +    irq_hw_number_t hwirq;
>> +
>> +    WARN_ON_ONCE(!handler->present);
>> +
>> +    chained_irq_enter(chip, desc);
>> +
>> +    while ((hwirq = cp100_get_hwirq(handler, claim))) {
>> +        int err = generic_handle_domain_irq(handler->priv->irqdomain, hwirq);
>> +
>> +        if (unlikely(err)) {
>> +            pr_warn_ratelimited("%pfwP: can't find mapping for hwirq %lu\n",
>> +                        handler->priv->fwnode, hwirq);
>> +        }
>> +    }
>> +
>> +    chained_irq_exit(chip, desc);
>> +}
>> +
>>   static void plic_set_threshold(struct plic_handler *handler, u32 threshold)
>>   {
>>       /* priority must be > threshold to trigger an interrupt */
>> @@ -430,6 +516,8 @@ static const struct of_device_id plic_match[] = {
>>         .data = (const void *)BIT(PLIC_QUIRK_EDGE_INTERRUPT) },
>>       { .compatible = "thead,c900-plic",
>>         .data = (const void *)BIT(PLIC_QUIRK_EDGE_INTERRUPT) },
>> +    { .compatible = "ultrarisc,cp100-plic",
>> +      .data = (const void *)BIT(PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM) },
>>       {}
>>   };
>>   @@ -664,12 +752,16 @@ static int plic_probe(struct fwnode_handle *fwnode)
>>           }
>>             if (global_setup) {
>> +            void (*handler_fn)(struct irq_desc *) = plic_handle_irq;
>> +
>> +            if (test_bit(PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM, &handler-
>> >priv->plic_quirks))
>> +                handler_fn = plic_handle_irq_cp100;
>> +
>>               /* Find parent domain and register chained handler */
>>               domain = irq_find_matching_fwnode(riscv_get_intc_hwnode(),
>> DOMAIN_BUS_ANY);
>>               if (domain)
>>                   plic_parent_irq = irq_create_mapping(domain, RV_IRQ_EXT);
>>               if (plic_parent_irq)
>> -                irq_set_chained_handler(plic_parent_irq, plic_handle_irq);
>> +                irq_set_chained_handler(plic_parent_irq, handler_fn);
>>                 cpuhp_setup_state(CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING,
>>                         "irqchip/sifive/plic:starting",
> 
> My rationale of the above comments is to achieve minimal overhead with this
> "read pending[] -> disable IE[] -> claim -> enable IE[]" approach. In general,
> the fewer interrupts enabled on a hart, the lower the overhead. If there's only
> 1 interrupt enabled for a give hart, then there's zero reading/writing of IE[],
> and you can further optimize away the reading of pending register.
> 
> I'd imagine that if the user truly want to avoid the overhead of this quirk,
> they can chose to spread out the irq groups onto different harts to alleviate
> the slow down, or better isolate a single irq to a given hart, and we should
> make it possible.
> 
> Feel free to point out any of my misunderstandings.
> 
> Bo

Powered by blists - more mailing lists