Message-ID: <b50745b9-6b45-4364-aff8-3c10c7cadf3c@sifive.com>
Date: Tue, 14 Oct 2025 11:45:50 -0500
From: Samuel Holland <samuel.holland@...ive.com>
To: Charles Mirabile <cmirabil@...hat.com>
Cc: Lucas Zampieri <lzampier@...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 v3 3/3] irqchip/plic: add support for UltraRISC DP1000
 PLIC

Hi Charles,

On 2025-10-14 11:28 AM, Charles Mirabile wrote:
> On Tue, Oct 14, 2025 at 12:09 PM Samuel Holland
> <samuel.holland@...ive.com> wrote:
>>
>> On 2025-10-14 10:40 AM, 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_CLAIM_REGISTER is present, a specialized handler
>>
>> You may want to name this something a bit more specific. Every PLIC has a claim
>> register, so it seems a bit weird saying that this is a quirk :)
> 
> Something like `PLIC_QUIRK_CLAIM_REGISTER_ERRATA`? Or
> `PLIC_QUIRK_CP100_CLAIM_REGISTER`? Or
> `PLIC_QUIRK_MULTIPLE_PENDING_CLAIM`? I guess the trouble is that they
> get pretty long pretty quick. Hard to summarize the issue concisely.

If I had to suggest something, I'd say PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM,
but it's really up to you. The function name indicates it is specific to CP100,
so the flag probably should too. I don't think being long is a problem if it
helps the reader guess the value/effect of the flag at a glance.

Regards,
Samuel

>>
>> Anyway, the code looks good, so:
>>
>> Acked-by: Samuel Holland <samuel.holland@...ive.com>
> 
> Thanks!
> 
>>
>>> (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>
>>> 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..1d528904b353 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_CLAIM_REGISTER    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;
>>> +     }
>>> +
>>> +     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));
>>> +     }
>>> +
>>> +     return true;
>>> +}
>>> +
>>> +static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler,
>>> +                                     void __iomem *claim)
>>> +{
>>> +     void __iomem *enable = handler->enable_base;
>>> +     void __iomem *pending = handler->priv->regs + PENDING_BASE;
>>> +     int nr_irqs = handler->priv->nr_irqs;
>>> +     int nr_irq_groups = DIV_ROUND_UP(nr_irqs, 32);
>>> +     int i;
>>> +     irq_hw_number_t hwirq = 0;
>>> +
>>> +     raw_spin_lock(&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));
>>> +
>>> +     if (!cp100_isolate_pending_irq(nr_irq_groups, handler->enable_save, pending, enable))
>>> +             goto out;
>>> +
>>> +     hwirq = readl(claim);
>>> +
>>> +     /* Restore previous state */
>>> +     for (i = 0; i < nr_irq_groups; i++)
>>> +             writel_relaxed(handler->enable_save[i], enable + i * sizeof(u32));
>>> +out:
>>> +     raw_spin_unlock(&handler->enable_lock);
>>> +     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_CLAIM_REGISTER) },
>>>       {}
>>>  };
>>>
>>> @@ -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_CLAIM_REGISTER, &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",
>>
> 

Powered by blists - more mailing lists