From: Charles Mirabile <cmirabil@redhat.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.

When claiming an interrupt on the DP1000 PLIC all other interrupts must be
disabled before the claim register is accessed to prevent incorrect
handling of the interrupt.

When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
the enable state of all interrupts is saved and then all interrupts
except for the first pending one are disabled before reading the claim
register. The interrupts are then restored 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@ultrarisc.com>
Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
---
 drivers/irqchip/irq-sifive-plic.c | 83 ++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 9c4af7d58846..a7b51a925e96 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;
@@ -367,6 +370,82 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
 	.free		= irq_domain_free_irqs_top,
 };

+static bool dp1000_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(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(new_mask, enable + j * sizeof(u32));
+	}
+
+	return true;
+}
+
+static irq_hw_number_t dp1000_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;
+	u32 ie[32] = { 0 };
+	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++)
+		ie[i] = readl(enable + i * sizeof(u32));
+
+	if (!dp1000_isolate_pending_irq(nr_irq_groups, ie, pending, enable))
+		goto out;
+
+	hwirq = readl(claim);
+
+	/* Restore previous state */
+	for (i = 0; i < nr_irq_groups; i++)
+		writel(ie[i], enable + i * sizeof(u32));
+out:
+	raw_spin_unlock(&handler->enable_lock);
+	return hwirq;
+}
+
+static irq_hw_number_t plic_get_hwirq(struct plic_handler *handler,
+				      void __iomem *claim)
+{
+	/*
+	 * Due to a hardware bug in the implementation of the claim register
+	 * in the UltraRISC DP1000 platform, other interrupts must be disabled
+	 * before reading the claim register and restored afterwards.
+	 */
+
+	if (test_bit(PLIC_QUIRK_CLAIM_REGISTER, &handler->priv->plic_quirks))
+		return dp1000_get_hwirq(handler, claim);
+
+	return readl(claim);
+}
+
 /*
  * Handling an interrupt is a two-step process: first you claim the interrupt
  * by reading the claim register, then you complete the interrupt by writing
@@ -384,7 +463,7 @@ static void plic_handle_irq(struct irq_desc *desc)

 	chained_irq_enter(chip, desc);

-	while ((hwirq = readl(claim))) {
+	while ((hwirq = plic_get_hwirq(handler, claim))) {
 		int err = generic_handle_domain_irq(handler->priv->irqdomain,
 						    hwirq);
 		if (unlikely(err)) {
@@ -432,6 +511,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) },
 	{}
 };

--
2.51.0

Hi Lucas,

On 2025-10-13 6:15 AM, Lucas Zampieri wrote:
> From: Charles Mirabile <cmirabil@redhat.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.
> 
> When claiming an interrupt on the DP1000 PLIC all other interrupts must be
> disabled before the claim register is accessed to prevent incorrect
> handling of the interrupt.
> 
> When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
> the enable state of all interrupts is saved and then all interrupts
> except for the first pending one are disabled before reading the claim
> register. The interrupts are then restored before further processing of
> the claimed interrupt continues.

Since the workaround requires scanning the pending bits for each interrupt
anyway, it would be simpler and more efficient to ignore the claim register
entirely. Call generic_handle_domain_irq() for each interrupt that is (enabled
AND pending), then clear the pending bit. Then you would not need to save and
restore the enable registers.

> 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@ultrarisc.com>
> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
> Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 83 ++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 9c4af7d58846..a7b51a925e96 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;
> @@ -367,6 +370,82 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
>  	.free		= irq_domain_free_irqs_top,
>  };
> 
> +static bool dp1000_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(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(new_mask, enable + j * sizeof(u32));
> +	}
> +
> +	return true;
> +}
> +
> +static irq_hw_number_t dp1000_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;
> +	u32 ie[32] = { 0 };
> +	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++)
> +		ie[i] = readl(enable + i * sizeof(u32));
> +
> +	if (!dp1000_isolate_pending_irq(nr_irq_groups, ie, pending, enable))
> +		goto out;
> +
> +	hwirq = readl(claim);
> +
> +	/* Restore previous state */
> +	for (i = 0; i < nr_irq_groups; i++)
> +		writel(ie[i], enable + i * sizeof(u32));
> +out:
> +	raw_spin_unlock(&handler->enable_lock);
> +	return hwirq;
> +}
> +
> +static irq_hw_number_t plic_get_hwirq(struct plic_handler *handler,
> +				      void __iomem *claim)
> +{
> +	/*
> +	 * Due to a hardware bug in the implementation of the claim register
> +	 * in the UltraRISC DP1000 platform, other interrupts must be disabled
> +	 * before reading the claim register and restored afterwards.
> +	 */
> +
> +	if (test_bit(PLIC_QUIRK_CLAIM_REGISTER, &handler->priv->plic_quirks))
> +		return dp1000_get_hwirq(handler, claim);
> +
> +	return readl(claim);
> +}
> +
>  /*
>   * Handling an interrupt is a two-step process: first you claim the interrupt
>   * by reading the claim register, then you complete the interrupt by writing
> @@ -384,7 +463,7 @@ static void plic_handle_irq(struct irq_desc *desc)
> 
>  	chained_irq_enter(chip, desc);
> 
> -	while ((hwirq = readl(claim))) {
> +	while ((hwirq = plic_get_hwirq(handler, claim))) {

This is the hot path for interrupt handling. Instead of checking for the quirk
on every interrupt, please create a new function that you conditionally pass to
irq_set_chained_handler(), so the quirk check only happens once at boot.

Regards,
Samuel

>  		int err = generic_handle_domain_irq(handler->priv->irqdomain,
>  						    hwirq);
>  		if (unlikely(err)) {
> @@ -432,6 +511,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) },
>  	{}
>  };
> 
> --
> 2.51.0
>

On 10/13/25 12:00, Samuel Holland wrote:
> Hi Lucas,
> 
> On 2025-10-13 6:15 AM, Lucas Zampieri wrote:
>> From: Charles Mirabile <cmirabil@redhat.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.
>>
>> When claiming an interrupt on the DP1000 PLIC all other interrupts must be
>> disabled before the claim register is accessed to prevent incorrect
>> handling of the interrupt.
>>
>> When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
>> the enable state of all interrupts is saved and then all interrupts
>> except for the first pending one are disabled before reading the claim
>> register. The interrupts are then restored before further processing of
>> the claimed interrupt continues.
> 
> Since the workaround requires scanning the pending bits for each interrupt
> anyway, it would be simpler and more efficient to ignore the claim register
> entirely. Call generic_handle_domain_irq() for each interrupt that is (enabled
> AND pending), then clear the pending bit. Then you would not need to save and
> restore the enable registers.
> 

Is that safe and race-free? Can we guarantee that the enable bits for
different contexts (harts) are disjoint at any given time? I'm a little
bit worried about the scenario where 2+ harts having the same irq enabled
and competing for the same irq claim. Without using the HW claim register,
we may get spurious interrupt, and then wrongly claimed the spurious
interrupt causing the next real one to be delayed indefinitely.

>> 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@ultrarisc.com>
>> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
>> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
>> Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
>> ---
>>   drivers/irqchip/irq-sifive-plic.c | 83 ++++++++++++++++++++++++++++++-
>>   1 file changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
>> index 9c4af7d58846..a7b51a925e96 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;
>> @@ -367,6 +370,82 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
>>   	.free		= irq_domain_free_irqs_top,
>>   };
>>
>> +static bool dp1000_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(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(new_mask, enable + j * sizeof(u32));
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +static irq_hw_number_t dp1000_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;
>> +	u32 ie[32] = { 0 };
>> +	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++)
>> +		ie[i] = readl(enable + i * sizeof(u32));
>> +
>> +	if (!dp1000_isolate_pending_irq(nr_irq_groups, ie, pending, enable))
>> +		goto out;
>> +
>> +	hwirq = readl(claim);
>> +
>> +	/* Restore previous state */
>> +	for (i = 0; i < nr_irq_groups; i++)
>> +		writel(ie[i], enable + i * sizeof(u32));
>> +out:
>> +	raw_spin_unlock(&handler->enable_lock);
>> +	return hwirq;
>> +}
>> +
>> +static irq_hw_number_t plic_get_hwirq(struct plic_handler *handler,
>> +				      void __iomem *claim)
>> +{
>> +	/*
>> +	 * Due to a hardware bug in the implementation of the claim register
>> +	 * in the UltraRISC DP1000 platform, other interrupts must be disabled
>> +	 * before reading the claim register and restored afterwards.
>> +	 */
>> +
>> +	if (test_bit(PLIC_QUIRK_CLAIM_REGISTER, &handler->priv->plic_quirks))
>> +		return dp1000_get_hwirq(handler, claim);
>> +
>> +	return readl(claim);
>> +}
>> +
>>   /*
>>    * Handling an interrupt is a two-step process: first you claim the interrupt
>>    * by reading the claim register, then you complete the interrupt by writing
>> @@ -384,7 +463,7 @@ static void plic_handle_irq(struct irq_desc *desc)
>>
>>   	chained_irq_enter(chip, desc);
>>
>> -	while ((hwirq = readl(claim))) {
>> +	while ((hwirq = plic_get_hwirq(handler, claim))) {
> 
> This is the hot path for interrupt handling. Instead of checking for the quirk
> on every interrupt, please create a new function that you conditionally pass to
> irq_set_chained_handler(), so the quirk check only happens once at boot.
> 
> Regards,
> Samuel
> 
>>   		int err = generic_handle_domain_irq(handler->priv->irqdomain,
>>   						    hwirq);
>>   		if (unlikely(err)) {
>> @@ -432,6 +511,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) },
>>   	{}
>>   };
>>
>> --
>> 2.51.0
>>
> 

Bo

Hi Bo,

On 2025-10-13 4:24 PM, Bo Gan wrote:
> On 10/13/25 12:00, Samuel Holland wrote:
>> Hi Lucas,
>>
>> On 2025-10-13 6:15 AM, Lucas Zampieri wrote:
>>> From: Charles Mirabile <cmirabil@redhat.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.
>>>
>>> When claiming an interrupt on the DP1000 PLIC all other interrupts must be
>>> disabled before the claim register is accessed to prevent incorrect
>>> handling of the interrupt.
>>>
>>> When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
>>> the enable state of all interrupts is saved and then all interrupts
>>> except for the first pending one are disabled before reading the claim
>>> register. The interrupts are then restored before further processing of
>>> the claimed interrupt continues.
>>
>> Since the workaround requires scanning the pending bits for each interrupt
>> anyway, it would be simpler and more efficient to ignore the claim register
>> entirely. Call generic_handle_domain_irq() for each interrupt that is (enabled
>> AND pending), then clear the pending bit. Then you would not need to save and
>> restore the enable registers.
>>
> 
> Is that safe and race-free? Can we guarantee that the enable bits for
> different contexts (harts) are disjoint at any given time? I'm a little
> bit worried about the scenario where 2+ harts having the same irq enabled
> and competing for the same irq claim. Without using the HW claim register,
> we may get spurious interrupt, and then wrongly claimed the spurious
> interrupt causing the next real one to be delayed indefinitely.

Yes, we can guarantee each interrupt is enabled on only one hart at a time.
plic_set_affinity() always chooses a single CPU and gets called from
irq_startup() before the first time the interrupt is enabled.

There are other races to consider (e.g. clearing one pending bit while the
hardware sets an adjacent one), but it looks like this strategy may not work on
the hardware anyway.

Regards,
Samuel

>>> 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@ultrarisc.com>
>>> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
>>> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
>>> Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
>>> ---
>>>   drivers/irqchip/irq-sifive-plic.c | 83 ++++++++++++++++++++++++++++++-
>>>   1 file changed, 82 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-
>>> plic.c
>>> index 9c4af7d58846..a7b51a925e96 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;
>>> @@ -367,6 +370,82 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
>>>       .free        = irq_domain_free_irqs_top,
>>>   };
>>>
>>> +static bool dp1000_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(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(new_mask, enable + j * sizeof(u32));
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +static irq_hw_number_t dp1000_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;
>>> +    u32 ie[32] = { 0 };
>>> +    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++)
>>> +        ie[i] = readl(enable + i * sizeof(u32));
>>> +
>>> +    if (!dp1000_isolate_pending_irq(nr_irq_groups, ie, pending, enable))
>>> +        goto out;
>>> +
>>> +    hwirq = readl(claim);
>>> +
>>> +    /* Restore previous state */
>>> +    for (i = 0; i < nr_irq_groups; i++)
>>> +        writel(ie[i], enable + i * sizeof(u32));
>>> +out:
>>> +    raw_spin_unlock(&handler->enable_lock);
>>> +    return hwirq;
>>> +}
>>> +
>>> +static irq_hw_number_t plic_get_hwirq(struct plic_handler *handler,
>>> +                      void __iomem *claim)
>>> +{
>>> +    /*
>>> +     * Due to a hardware bug in the implementation of the claim register
>>> +     * in the UltraRISC DP1000 platform, other interrupts must be disabled
>>> +     * before reading the claim register and restored afterwards.
>>> +     */
>>> +
>>> +    if (test_bit(PLIC_QUIRK_CLAIM_REGISTER, &handler->priv->plic_quirks))
>>> +        return dp1000_get_hwirq(handler, claim);
>>> +
>>> +    return readl(claim);
>>> +}
>>> +
>>>   /*
>>>    * Handling an interrupt is a two-step process: first you claim the interrupt
>>>    * by reading the claim register, then you complete the interrupt by writing
>>> @@ -384,7 +463,7 @@ static void plic_handle_irq(struct irq_desc *desc)
>>>
>>>       chained_irq_enter(chip, desc);
>>>
>>> -    while ((hwirq = readl(claim))) {
>>> +    while ((hwirq = plic_get_hwirq(handler, claim))) {
>>
>> This is the hot path for interrupt handling. Instead of checking for the quirk
>> on every interrupt, please create a new function that you conditionally pass to
>> irq_set_chained_handler(), so the quirk check only happens once at boot.
>>
>> Regards,
>> Samuel
>>
>>>           int err = generic_handle_domain_irq(handler->priv->irqdomain,
>>>                               hwirq);
>>>           if (unlikely(err)) {
>>> @@ -432,6 +511,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) },
>>>       {}
>>>   };
>>>
>>> -- 
>>> 2.51.0
>>>
>>
> 
> Bo

Hi Samuel - 

On Mon, Oct 13, 2025 at 02:00:29PM -0500, Samuel Holland wrote:
> Hi Lucas,
> 
> On 2025-10-13 6:15 AM, Lucas Zampieri wrote:
> > From: Charles Mirabile <cmirabil@redhat.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.
> > 
> > When claiming an interrupt on the DP1000 PLIC all other interrupts must be
> > disabled before the claim register is accessed to prevent incorrect
> > handling of the interrupt.
> > 
> > When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
> > the enable state of all interrupts is saved and then all interrupts
> > except for the first pending one are disabled before reading the claim
> > register. The interrupts are then restored before further processing of
> > the claimed interrupt continues.
> 
> Since the workaround requires scanning the pending bits for each interrupt
> anyway, it would be simpler and more efficient to ignore the claim register
> entirely. Call generic_handle_domain_irq() for each interrupt that is (enabled
> AND pending), then clear the pending bit. Then you would not need to save and
> restore the enable registers.
> 
> > 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@ultrarisc.com>
> > Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
> > Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
> > Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 83 ++++++++++++++++++++++++++++++-
> >  1 file changed, 82 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 9c4af7d58846..a7b51a925e96 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;
> > @@ -367,6 +370,82 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
> >  	.free		= irq_domain_free_irqs_top,
> >  };
> > 
> > +static bool dp1000_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(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(new_mask, enable + j * sizeof(u32));
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static irq_hw_number_t dp1000_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;
> > +	u32 ie[32] = { 0 };
> > +	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++)
> > +		ie[i] = readl(enable + i * sizeof(u32));
> > +
> > +	if (!dp1000_isolate_pending_irq(nr_irq_groups, ie, pending, enable))
> > +		goto out;
> > +
> > +	hwirq = readl(claim);
> > +
> > +	/* Restore previous state */
> > +	for (i = 0; i < nr_irq_groups; i++)
> > +		writel(ie[i], enable + i * sizeof(u32));
> > +out:
> > +	raw_spin_unlock(&handler->enable_lock);
> > +	return hwirq;
> > +}
> > +
> > +static irq_hw_number_t plic_get_hwirq(struct plic_handler *handler,
> > +				      void __iomem *claim)
> > +{
> > +	/*
> > +	 * Due to a hardware bug in the implementation of the claim register
> > +	 * in the UltraRISC DP1000 platform, other interrupts must be disabled
> > +	 * before reading the claim register and restored afterwards.
> > +	 */
> > +
> > +	if (test_bit(PLIC_QUIRK_CLAIM_REGISTER, &handler->priv->plic_quirks))
> > +		return dp1000_get_hwirq(handler, claim);
> > +
> > +	return readl(claim);
> > +}
> > +
> >  /*
> >   * Handling an interrupt is a two-step process: first you claim the interrupt
> >   * by reading the claim register, then you complete the interrupt by writing
> > @@ -384,7 +463,7 @@ static void plic_handle_irq(struct irq_desc *desc)
> > 
> >  	chained_irq_enter(chip, desc);
> > 
> > -	while ((hwirq = readl(claim))) {
> > +	while ((hwirq = plic_get_hwirq(handler, claim))) {
> 
> This is the hot path for interrupt handling. Instead of checking for the quirk
> on every interrupt, please create a new function that you conditionally pass to
> irq_set_chained_handler(), so the quirk check only happens once at boot.
> 
> Regards,
> Samuel
> 
> >  		int err = generic_handle_domain_irq(handler->priv->irqdomain,
> >  						    hwirq);
> >  		if (unlikely(err)) {
> > @@ -432,6 +511,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) },
> >  	{}
> >  };
> > 
> > --
> > 2.51.0
> > 
> 

Is something like this closer to what you had in mind? I tried it on the
dp1000 and it doesn't work. Obviously it is concievable that I messed up
the logic here, but it also might be the case that reading the claim
register is integral to the proper functioning of the pending bits.

I can confirm that a more minimal change that just moves the quirk check
out of the hot path is fine. Would that be acceptable even if it is not
the most efficient? (in essense take the hunk with new functions from
the original patch but revert the change to `plic_handle_irq` and then add
the hunk that changes probe from this proposed patch and then create
the `plic_handle_irq_dp1000` function as a copy of `plic_handle_irq` where
`dp1000_get_hwirq` is in the loop instead of `readl(claim)`).

Best - Charlie

---
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 9c4af7d58846..fcf520ed33fd 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;
@@ -367,6 +370,53 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
 	.free		= irq_domain_free_irqs_top,
 };
 
+static int dp1000_find_pending_irq(struct plic_handler *handler, void __iomem *pending)
+{
+	void __iomem *enable = handler->enable_base;
+	int nr_irqs = handler->priv->nr_irqs;
+	int nr_irq_groups = DIV_ROUND_UP(nr_irqs, 32);
+	u32 pending_irqs = 0;
+	int i;
+
+	raw_spin_lock(&handler->enable_lock);
+	for (i = 0; i < nr_irq_groups; i++) {
+		u32 enable_mask = readl(enable + i * sizeof(u32));
+		u32 pending_mask = readl(pending + i * sizeof(u32));
+		if ((pending_irqs = enable_mask & pending_mask))
+			break;
+	}
+	raw_spin_unlock(&handler->enable_lock);
+
+	if (!pending_irqs)
+		return 0;
+
+	return 32 * i + __ffs(pending_irqs);
+}
+
+static void plic_handle_irq_dp1000(struct irq_desc *desc)
+{
+	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
+	void __iomem *pending = handler->priv->regs + PENDING_BASE;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	irq_hw_number_t hwirq;
+
+	WARN_ON_ONCE(!handler->present);
+
+	chained_irq_enter(chip, desc);
+
+	while ((hwirq = dp1000_find_pending_irq(handler, pending))) {
+		int err = generic_handle_domain_irq(handler->priv->irqdomain,
+						    hwirq);
+		__plic_toggle(pending, hwirq, 0);
+		if (unlikely(err)) {
+			pr_warn_ratelimited("%pfwP: can't find mapping for hwirq %lu\n",
+					    handler->priv->fwnode, hwirq);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
 /*
  * Handling an interrupt is a two-step process: first you claim the interrupt
  * by reading the claim register, then you complete the interrupt by writing
@@ -432,6 +482,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,dp1000-plic",
+	  .data = (const void *)BIT(PLIC_QUIRK_CLAIM_REGISTER) },
 	{}
 };
 
@@ -666,12 +718,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_dp1000;
+
 			/* 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",
-- 
2.43.0

Hi Charles,

On 2025-10-13 4:03 PM, Charles Mirabile wrote:
> Hi Samuel - 
> 
> On Mon, Oct 13, 2025 at 02:00:29PM -0500, Samuel Holland wrote:
>> Hi Lucas,
>>
>> On 2025-10-13 6:15 AM, Lucas Zampieri wrote:
>>> From: Charles Mirabile <cmirabil@redhat.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.
>>>
>>> When claiming an interrupt on the DP1000 PLIC all other interrupts must be
>>> disabled before the claim register is accessed to prevent incorrect
>>> handling of the interrupt.
>>>
>>> When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
>>> the enable state of all interrupts is saved and then all interrupts
>>> except for the first pending one are disabled before reading the claim
>>> register. The interrupts are then restored before further processing of
>>> the claimed interrupt continues.
>>
>> Since the workaround requires scanning the pending bits for each interrupt
>> anyway, it would be simpler and more efficient to ignore the claim register
>> entirely. Call generic_handle_domain_irq() for each interrupt that is (enabled
>> AND pending), then clear the pending bit. Then you would not need to save and
>> restore the enable registers.
>>
>>> 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@ultrarisc.com>
>>> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
>>> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
>>> Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
>>> ---
>>>  drivers/irqchip/irq-sifive-plic.c | 83 ++++++++++++++++++++++++++++++-
>>>  1 file changed, 82 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
>>> index 9c4af7d58846..a7b51a925e96 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;
>>> @@ -367,6 +370,82 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
>>>  	.free		= irq_domain_free_irqs_top,
>>>  };
>>>
>>> +static bool dp1000_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(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(new_mask, enable + j * sizeof(u32));
>>> +	}
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +static irq_hw_number_t dp1000_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;
>>> +	u32 ie[32] = { 0 };

A couple of comments since we're keeping this algorithm:

There's already an appropriately-sized handler->enable_save array that can be
reused here.

>>> +	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++)
>>> +		ie[i] = readl(enable + i * sizeof(u32));
>>> +
>>> +	if (!dp1000_isolate_pending_irq(nr_irq_groups, ie, pending, enable))
>>> +		goto out;
>>> +
>>> +	hwirq = readl(claim);
>>> +
>>> +	/* Restore previous state */
>>> +	for (i = 0; i < nr_irq_groups; i++)
>>> +		writel(ie[i], enable + i * sizeof(u32));

All of the I/O in these new functions, except the readl(claim), can use the
{readl,writel}_relaxed I/O accessors. They don't have any ordering requirement
with respect to main memory, just other I/O.

>>> +out:
>>> +	raw_spin_unlock(&handler->enable_lock);
>>> +	return hwirq;
>>> +}
>>> +
>>> +static irq_hw_number_t plic_get_hwirq(struct plic_handler *handler,
>>> +				      void __iomem *claim)
>>> +{
>>> +	/*
>>> +	 * Due to a hardware bug in the implementation of the claim register
>>> +	 * in the UltraRISC DP1000 platform, other interrupts must be disabled
>>> +	 * before reading the claim register and restored afterwards.
>>> +	 */
>>> +
>>> +	if (test_bit(PLIC_QUIRK_CLAIM_REGISTER, &handler->priv->plic_quirks))
>>> +		return dp1000_get_hwirq(handler, claim);
>>> +
>>> +	return readl(claim);
>>> +}
>>> +
>>>  /*
>>>   * Handling an interrupt is a two-step process: first you claim the interrupt
>>>   * by reading the claim register, then you complete the interrupt by writing
>>> @@ -384,7 +463,7 @@ static void plic_handle_irq(struct irq_desc *desc)
>>>
>>>  	chained_irq_enter(chip, desc);
>>>
>>> -	while ((hwirq = readl(claim))) {
>>> +	while ((hwirq = plic_get_hwirq(handler, claim))) {
>>
>> This is the hot path for interrupt handling. Instead of checking for the quirk
>> on every interrupt, please create a new function that you conditionally pass to
>> irq_set_chained_handler(), so the quirk check only happens once at boot.
>>
>> Regards,
>> Samuel
>>
>>>  		int err = generic_handle_domain_irq(handler->priv->irqdomain,
>>>  						    hwirq);
>>>  		if (unlikely(err)) {
>>> @@ -432,6 +511,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) },
>>>  	{}
>>>  };
>>>
>>> --
>>> 2.51.0
>>>
>>
> 
> Is something like this closer to what you had in mind? I tried it on the
> dp1000 and it doesn't work. Obviously it is concievable that I messed up
> the logic here, but it also might be the case that reading the claim
> register is integral to the proper functioning of the pending bits.

It's also possible that the pending bits are read only. There are existing
implementations with both RO and RW pending bits. So the claim register might be
the only way to clear the pending bits for edge-triggered interrupts. (For level
interrupts, the pending bits should clear themselves, so it should be fine if
the write is ignored.) I don't see anything wrong with the code below, but we're
working with known-buggy hardware, so who knows.

> I can confirm that a more minimal change that just moves the quirk check
> out of the hot path is fine. Would that be acceptable even if it is not
> the most efficient? (in essense take the hunk with new functions from
> the original patch but revert the change to `plic_handle_irq` and then add
> the hunk that changes probe from this proposed patch and then create
> the `plic_handle_irq_dp1000` function as a copy of `plic_handle_irq` where
> `dp1000_get_hwirq` is in the loop instead of `readl(claim)`).

Yes, this is acceptable. Even if the workaround isn't the most efficient, it's
at least isolated to the affected hardware. So we'll get the hardware working
now, and the efficiency can always be revisited later.

Regards,
Samuel

> Best - Charlie
> 
> ---
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 9c4af7d58846..fcf520ed33fd 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;
> @@ -367,6 +370,53 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
>  	.free		= irq_domain_free_irqs_top,
>  };
>  
> +static int dp1000_find_pending_irq(struct plic_handler *handler, void __iomem *pending)
> +{
> +	void __iomem *enable = handler->enable_base;
> +	int nr_irqs = handler->priv->nr_irqs;
> +	int nr_irq_groups = DIV_ROUND_UP(nr_irqs, 32);
> +	u32 pending_irqs = 0;
> +	int i;
> +
> +	raw_spin_lock(&handler->enable_lock);
> +	for (i = 0; i < nr_irq_groups; i++) {
> +		u32 enable_mask = readl(enable + i * sizeof(u32));
> +		u32 pending_mask = readl(pending + i * sizeof(u32));
> +		if ((pending_irqs = enable_mask & pending_mask))
> +			break;
> +	}
> +	raw_spin_unlock(&handler->enable_lock);
> +
> +	if (!pending_irqs)
> +		return 0;
> +
> +	return 32 * i + __ffs(pending_irqs);
> +}
> +
> +static void plic_handle_irq_dp1000(struct irq_desc *desc)
> +{
> +	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> +	void __iomem *pending = handler->priv->regs + PENDING_BASE;
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	irq_hw_number_t hwirq;
> +
> +	WARN_ON_ONCE(!handler->present);
> +
> +	chained_irq_enter(chip, desc);
> +
> +	while ((hwirq = dp1000_find_pending_irq(handler, pending))) {
> +		int err = generic_handle_domain_irq(handler->priv->irqdomain,
> +						    hwirq);
> +		__plic_toggle(pending, hwirq, 0);
> +		if (unlikely(err)) {
> +			pr_warn_ratelimited("%pfwP: can't find mapping for hwirq %lu\n",
> +					    handler->priv->fwnode, hwirq);
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
>  /*
>   * Handling an interrupt is a two-step process: first you claim the interrupt
>   * by reading the claim register, then you complete the interrupt by writing
> @@ -432,6 +482,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,dp1000-plic",
> +	  .data = (const void *)BIT(PLIC_QUIRK_CLAIM_REGISTER) },
>  	{}
>  };
>  
> @@ -666,12 +718,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_dp1000;
> +
>  			/* 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",

On Mon, Oct 13, 2025 at 12:15:38PM +0100, Lucas Zampieri wrote:
> From: Charles Mirabile <cmirabil@redhat.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.
> 
> When claiming an interrupt on the DP1000 PLIC all other interrupts must be
> disabled before the claim register is accessed to prevent incorrect
> handling of the interrupt.
> 
> When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
> the enable state of all interrupts is saved and then all interrupts
> except for the first pending one are disabled before reading the claim
> register. The interrupts are then restored 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.

Why is that? I expect that you're doing that intentionally given the
ultrarisc employee listed as a co-developer, but with only one SoC using
this IP core it seems possible that this bug in the hardware could be
fixed for other SoCs that are built using this IP core.
Is there a plan to, for example, change the core version to UR-CP101
when the bug is fixed?

Thanks,
Conor.

> This has no impact on other platforms.
> 
> Co-developed-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
> Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 83 ++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 9c4af7d58846..a7b51a925e96 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;
> @@ -367,6 +370,82 @@ static const struct irq_domain_ops plic_irqdomain_ops = {
>  	.free		= irq_domain_free_irqs_top,
>  };
> 
> +static bool dp1000_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(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(new_mask, enable + j * sizeof(u32));
> +	}
> +
> +	return true;
> +}
> +
> +static irq_hw_number_t dp1000_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;
> +	u32 ie[32] = { 0 };
> +	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++)
> +		ie[i] = readl(enable + i * sizeof(u32));
> +
> +	if (!dp1000_isolate_pending_irq(nr_irq_groups, ie, pending, enable))
> +		goto out;
> +
> +	hwirq = readl(claim);
> +
> +	/* Restore previous state */
> +	for (i = 0; i < nr_irq_groups; i++)
> +		writel(ie[i], enable + i * sizeof(u32));
> +out:
> +	raw_spin_unlock(&handler->enable_lock);
> +	return hwirq;
> +}
> +
> +static irq_hw_number_t plic_get_hwirq(struct plic_handler *handler,
> +				      void __iomem *claim)
> +{
> +	/*
> +	 * Due to a hardware bug in the implementation of the claim register
> +	 * in the UltraRISC DP1000 platform, other interrupts must be disabled
> +	 * before reading the claim register and restored afterwards.
> +	 */
> +
> +	if (test_bit(PLIC_QUIRK_CLAIM_REGISTER, &handler->priv->plic_quirks))
> +		return dp1000_get_hwirq(handler, claim);
> +
> +	return readl(claim);
> +}
> +
>  /*
>   * Handling an interrupt is a two-step process: first you claim the interrupt
>   * by reading the claim register, then you complete the interrupt by writing
> @@ -384,7 +463,7 @@ static void plic_handle_irq(struct irq_desc *desc)
> 
>  	chained_irq_enter(chip, desc);
> 
> -	while ((hwirq = readl(claim))) {
> +	while ((hwirq = plic_get_hwirq(handler, claim))) {
>  		int err = generic_handle_domain_irq(handler->priv->irqdomain,
>  						    hwirq);
>  		if (unlikely(err)) {
> @@ -432,6 +511,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) },
>  	{}
>  };
> 
> --
> 2.51.0
> 

Hi Conor,

On 10/14/25 02:30, Conor Dooley wrote:
> On Mon, Oct 13, 2025 at 12:15:38PM +0100, Lucas Zampieri wrote:
>> From: Charles Mirabile <cmirabil@redhat.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.
>>
>> When claiming an interrupt on the DP1000 PLIC all other interrupts must be
>> disabled before the claim register is accessed to prevent incorrect
>> handling of the interrupt.
>>
>> When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
>> the enable state of all interrupts is saved and then all interrupts
>> except for the first pending one are disabled before reading the claim
>> register. The interrupts are then restored 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.
> Why is that? I expect that you're doing that intentionally given the
> ultrarisc employee listed as a co-developer, but with only one SoC using
> this IP core it seems possible that this bug in the hardware could be
> fixed for other SoCs that are built using this IP core.
> Is there a plan to, for example, change the core version to UR-CP101
> when the bug is fixed?

I originally proposed to match on ultrarisc,cp100-plic under the
assumption that it would be the case.

Furthermore, it is my understanding that if the bug is fixed in, say,
UR-DP1001, then the PLIC node can simply be

    compatible = "ultrarisc,dp1001-plic", "sifive,plic-1.0.0";

I meant my reply that I had assumed this bug was associated with the
UR-CP100 core, but I should have stated so more clearly. 

Vivian "dramforever" Wang

Hi Conor and Vivian,

On Tue, Oct 14, 2025 at 10:15 AM Vivian Wang <wangruikang@iscas.ac.cn> wrote:
>
> Hi Conor,
>
> On 10/14/25 02:30, Conor Dooley wrote:
> > On Mon, Oct 13, 2025 at 12:15:38PM +0100, Lucas Zampieri wrote:
> >> From: Charles Mirabile <cmirabil@redhat.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.
> >>
> >> When claiming an interrupt on the DP1000 PLIC all other interrupts must be
> >> disabled before the claim register is accessed to prevent incorrect
> >> handling of the interrupt.
> >>
> >> When the PLIC_QUIRK_CLAIM_REGISTER is present, during plic_handle_irq
> >> the enable state of all interrupts is saved and then all interrupts
> >> except for the first pending one are disabled before reading the claim
> >> register. The interrupts are then restored 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.
> > Why is that? I expect that you're doing that intentionally given the
> > ultrarisc employee listed as a co-developer, but with only one SoC using
> > this IP core it seems possible that this bug in the hardware could be
> > fixed for other SoCs that are built using this IP core.
> > Is there a plan to, for example, change the core version to UR-CP101
> > when the bug is fixed?
>
> I originally proposed to match on ultrarisc,cp100-plic under the
> assumption that it would be the case.
>
As far as I was able to verify with UltraRisc, this is a bug with the
cp100 cores and not the DP-1000 SoC, what that means is that any other
board using those cp100 cores should have the same bug. And I agree
that the function naming in this patch makes it confusing and seem
like this is an issue to the dp1000, I'll reword those in a v3.

> Furthermore, it is my understanding that if the bug is fixed in, say,
> UR-DP1001, then the PLIC node can simply be
>
>     compatible = "ultrarisc,dp1001-plic", "sifive,plic-1.0.0";
>
> I meant my reply that I had assumed this bug was associated with the
> UR-CP100 core, but I should have stated so more clearly.
>
> Vivian "dramforever" Wang
>

Lucas Zampieri