[PATCH v5 0/3] Add UltraRISC DP1000 PLIC support

[PATCH v5 0/3] Add UltraRISC DP1000 PLIC support

[Lucas Zampieri]

This series adds support for the PLIC implementation in the UltraRISC
DP1000 SoC. The UR-CP100 cores used in the DP1000 have a hardware bug in
their PLIC claim register where reading it while multiple interrupts are
pending can return the wrong interrupt ID. The workaround temporarily
disables all interrupts except the first pending one before reading the
claim register, then restores the previous state.

The driver matches on "ultrarisc,cp100-plic" (CPU core compatible), allowing
the quirk to apply to all SoCs using UR-CP100 cores (currently DP1000,
potentially future SoCs).

Charles Mirabile (2):
  dt-bindings: interrupt-controller: add UltraRISC DP1000 PLIC
  irqchip/plic: add support for UltraRISC DP1000 PLIC

Lucas Zampieri (1):
  dt-bindings: vendor-prefixes: add UltraRISC

Changes in v5:
- 0003: Added brackets around conditional in cp100_isolate_pending_irq (feedback from Thomas Gleixner)
- 0003: Reordered variables in reverse fir tree order in cp100_get_hwirq (feedback from Thomas Gleixner)
- 0003: Replaced raw_spin_lock/unlock with guard(raw_spinlock) (feedback from Thomas Gleixner)
- 0003: Added newline between variable declaration and code in plic_probe (feedback from Thomas Gleixner)
- 0003: Extended generic_handle_domain_irq call to single line (feedback from Thomas Gleixner)

Changes in v4:
- 0002: Simplified commit message to focus on hardware bug (feedback from Conor Dooley)
- 0002: Added Conor's Acked-by
- 0003: Renamed PLIC_QUIRK_CLAIM_REGISTER to PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM
  to be more specific (feedback from Samuel Holland)
- 0003: Added Samuel's Acked-by

Changes in v3:
- 0002: Updated commit message to clarify that DP1000 is an SoC and CP100
  is a core (feedback from Conor Dooley)
- 0003: Renamed dp1000_* functions to cp100_* and updated commit message to
  clarify the hardware bug is in the UR-CP100 core implementation, not
  specific to the DP1000 SoC
- 0003: Moved quirk check out of hot interrupt path by creating separate
  plic_handle_irq_cp100() function and selecting handler at probe time
- 0003: Use existing handler->enable_save[] array instead of stack allocation
- 0003: Use readl_relaxed()/writel_relaxed() for better performance

Changes in v2:
- 0002: Changed compatible string pattern to SoC+core: ultrarisc,dp1000-plic
  with ultrarisc,cp100-plic fallback (suggested by Krzysztof and Vivian)
- 0003: Driver now matches on ultrarisc,cp100-plic (core) instead of dp1000 (SoC)
- All patches: Added submitter Signed-off-by to complete DCO chain

 .../sifive,plic-1.0.0.yaml                    |  3 +
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 drivers/irqchip/irq-sifive-plic.c             | 94 ++++++++++++++++++-
 3 files changed, 98 insertions(+), 1 deletion(-)

-- 
2.51.0

[PATCH v5 1/3] dt-bindings: vendor-prefixes: add UltraRISC

[Lucas Zampieri]

Add vendor prefix for UltraRISC Technology Co., Ltd.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 9ec8947dfcad..887bcb792284 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1642,6 +1642,8 @@ patternProperties:
     description: Universal Scientific Industrial Co., Ltd.
   "^usr,.*":
     description: U.S. Robotics Corporation
+  "^ultrarisc,.*":
+    description: UltraRISC Technology Co., Ltd.
   "^ultratronik,.*":
     description: Ultratronik GmbH
   "^utoo,.*":
-- 
2.51.0

[PATCH v5 2/3] dt-bindings: interrupt-controller: add UltraRISC DP1000 PLIC

[Lucas Zampieri]

From: Charles Mirabile <cmirabil@redhat.com>

Add compatible strings for the PLIC found in UltraRISC DP1000 SoC.

The PLIC is part of the UR-CP100 core and has a hardware bug requiring
a workaround.

Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
---
 .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml       | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
index 5b827bc24301..34591d64cca3 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
@@ -74,6 +74,9 @@ properties:
               - sophgo,sg2044-plic
               - thead,th1520-plic
           - const: thead,c900-plic
+      - items:
+          - const: ultrarisc,dp1000-plic
+          - const: ultrarisc,cp100-plic
       - items:
           - const: sifive,plic-1.0.0
           - const: riscv,plic0
-- 
2.51.0

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

[Lucas Zampieri]

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 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@ultrarisc.com>
Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
Acked-by: Samuel Holland <samuel.holland@sifive.com>
Signed-off-by: Lucas Zampieri <lzampier@redhat.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;
+	}
+
+	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)
+{
+	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));
+
+	if (!cp100_isolate_pending_irq(nr_irq_groups, handler->enable_save, pending, enable))
+		return 0;
+
+	hwirq = readl(claim);
+
+	/* 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",
-- 
2.51.0

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

[Thomas Gleixner]

On Thu, Oct 16 2025 at 09:42, Lucas Zampieri wrote:
> @@ -430,6 +516,8 @@ static const struct of_device_id plic_match[] = {
      ^^^^^^^^^^^^
How on earth did you manage to screw up the hunk header?

Applying: irqchip/plic: Add support for UltraRISC DP1000 PLIC
error: corrupt patch at line 116

>  	  .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)
     ^^^^^^^^^^^^^^^
Ditto here.

I fixed it up manually. Please be more careful next time.

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

[Thomas Gleixner]

On Thu, Oct 16 2025 at 09:42, Lucas Zampieri wrote:

After fixing the corrupted patch up I had a closer look and decided not
to merge it. See comments below.
  
> +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)
> +{
> +	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?

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?

But looking at this makes me wonder about the functional correctness of all
this. What happens in this case:

Device A raises an interrupt

    handler()
        ....
        disable_groups();

Device B raises a now disabled interrupt

        restore_groups();

Is the device B interrupt preserved in the interrupt chip and actually
raised when the interrupt enable bit is restored or is it lost?

Thanks,

        tglx

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

[Charles Mirabile]

Hi Thomas—

On Thu, Oct 16, 2025 at 9:17 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, Oct 16 2025 at 09:42, Lucas Zampieri wrote:
>
> After fixing the corrupted patch up I had a closer look and decided not
> to merge it. See comments below.
>
> > +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)
> > +{
> > +     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.

I do agree that it would save this loop, but the way this was written
was intentionally designed to minimize the impact on other platforms
at the expense of this one because it is the platform with the bug.

>
> 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.

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.

I could definitely adjust the clear loop to move the mask preparation out like:

/* extract lowest set bit */
pending &= -pending

for(j = 0; j < nr_groups; ++j)
    writel_relaxed(i == j ? pending : 0, enable + i * sizeof(u32));

but I am quite sure that the compiler is well able to do this exact
transformation as an optimization pass and I am not sure that it is
more readable than what I originally proposed.

>
> But looking at this makes me wonder about the functional correctness of all
> this. What happens in this case:
>
> Device A raises an interrupt
>
>     handler()
>         ....
>         disable_groups();
>
> Device B raises a now disabled interrupt
>
>         restore_groups();
>
> 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.

>
> Thanks,
>
>         tglx
>

I am very sorry about the corrupt patch, I don't know how it happened.
I hope you will reconsider taking this code. We can send a version
that is not corrupt (or with the slight modification to bring the mask
preparation out of the loop) if you would prefer.

Thanks for the review.

Best—Charlie

[1] https://github.com/riscv/riscv-plic-spec/releases/tag/1.0.0

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

[Thomas Gleixner]

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

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

[Charles Mirabile]

Hi Thomas—

On Thu, Oct 16, 2025 at 12:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> 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.

Fair enough, if you insist. This will probably require a second patch
refactoring a decent amount of code (since the suspend / resume path
for which this enable array was added becomes simpler).

>
> >> 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.

Sorry, did you mean "because I had not considered the original
approach carefully enough" or "because this other approach, while
slower, is more self evidently correct."

If you meant the latter, I still want to argue for the bit twiddling
approach. I verified that clang and gcc are both not smart enough to
recognize the pattern of ffs followed by shift and optimize to x & -x
(even on -O3 [1]), so using ffs is definitely slower and relies on a
bunch of machinery (alternatives because risc-v does not include a
dedicated count trailing zeros instruction in the base isa). To me
anyways, the logic of x & -x is pretty obvious, and even more so with
a comment, and this is actually in the hot path.

>
> > 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.

This I still don't understand how this is particuarly helpful. Since
we are doing mmio, this is going to be an explicit loop and not a
memcpy. The code is branchless in either case (set equal for the check
of i against j negate and and with mask before loading into the mmio).


>
> >> 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.

Will do.

>
> Thanks,
>
>         tglx
>

[1] https://godbolt.org/z/eofozYjPo

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

[Thomas Gleixner]

On Thu, Oct 16 2025 at 12:52, Charles Mirabile wrote:
> On Thu, Oct 16, 2025 at 12:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> >>         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.
>
> Sorry, did you mean "because I had not considered the original
> approach carefully enough" or "because this other approach, while
> slower, is more self evidently correct."

I did not think about x & -x :)

>> 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.
>
> This I still don't understand how this is particuarly helpful. Since
> we are doing mmio, this is going to be an explicit loop and not a
> memcpy. The code is branchless in either case (set equal for the check
> of i against j negate and and with mask before loading into the mmio).

Fair enough. I did not think in RISC ASM :)

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

[Charles Mirabile]

Hi Thomas -

On Thu, Oct 16, 2025 at 07:53:26PM +0200, Thomas Gleixner wrote:
> On Thu, Oct 16 2025 at 12:52, Charles Mirabile wrote:
> > On Thu, Oct 16, 2025 at 12:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >>         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.
> >
> > Sorry, did you mean "because I had not considered the original
> > approach carefully enough" or "because this other approach, while
> > slower, is more self evidently correct."
> 
> I did not think about x & -x :)
> 
> >> 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.
> >
> > This I still don't understand how this is particuarly helpful. Since
> > we are doing mmio, this is going to be an explicit loop and not a
> > memcpy. The code is branchless in either case (set equal for the check
> > of i against j negate and and with mask before loading into the mmio).
> 
> Fair enough. I did not think in RISC ASM :)

What do you think about the attached patch (it should be not corrupt :^)
I think I adressed your concerns, and I ran it through clang-format too.

I folded everything into one diff for ease of review, but when we send it
officially there will be a separate patch for the caching refactor.

Best - Charlie
---
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index cbd7697bc148..f8f3384ecaab 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;
@@ -94,15 +97,22 @@ static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
 
 static int plic_irq_set_type(struct irq_data *d, unsigned int type);
 
-static void __plic_toggle(void __iomem *enable_base, int hwirq, int enable)
+static void __plic_toggle(struct plic_handler *handler, int hwirq, int enable)
 {
-	u32 __iomem *reg = enable_base + (hwirq / 32) * sizeof(u32);
+	u32 __iomem *base = handler->enable_base;
 	u32 hwirq_mask = 1 << (hwirq % 32);
+	int group = hwirq / 32;
+	u32 value;
+
+	value = readl(base + group);
 
 	if (enable)
-		writel(readl(reg) | hwirq_mask, reg);
+		value |= hwirq_mask;
 	else
-		writel(readl(reg) & ~hwirq_mask, reg);
+		value &= ~hwirq_mask;
+
+	handler->enable_save[group] = value;
+	writel(value, base + group);
 }
 
 static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
@@ -110,7 +120,7 @@ static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&handler->enable_lock, flags);
-	__plic_toggle(handler->enable_base, hwirq, enable);
+	__plic_toggle(handler, hwirq, enable);
 	raw_spin_unlock_irqrestore(&handler->enable_lock, flags);
 }
 
@@ -247,33 +257,16 @@ static int plic_irq_set_type(struct irq_data *d, unsigned int type)
 
 static int plic_irq_suspend(void)
 {
-	unsigned int i, cpu;
-	unsigned long flags;
-	u32 __iomem *reg;
 	struct plic_priv *priv;
 
 	priv = per_cpu_ptr(&plic_handlers, smp_processor_id())->priv;
 
 	/* irq ID 0 is reserved */
-	for (i = 1; i < priv->nr_irqs; i++) {
+	for (unsigned int i = 1; i < priv->nr_irqs; i++) {
 		__assign_bit(i, priv->prio_save,
 			     readl(priv->regs + PRIORITY_BASE + i * PRIORITY_PER_ID));
 	}
 
-	for_each_present_cpu(cpu) {
-		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
-
-		if (!handler->present)
-			continue;
-
-		raw_spin_lock_irqsave(&handler->enable_lock, flags);
-		for (i = 0; i < DIV_ROUND_UP(priv->nr_irqs, 32); i++) {
-			reg = handler->enable_base + i * sizeof(u32);
-			handler->enable_save[i] = readl(reg);
-		}
-		raw_spin_unlock_irqrestore(&handler->enable_lock, flags);
-	}
-
 	return 0;
 }
 
@@ -398,6 +391,85 @@ 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[],
+				      u32 __iomem *pending,
+				      u32 __iomem *enable)
+{
+	u32 pending_irqs = 0;
+	int i, j;
+
+	/* Look for first pending interrupt */
+	for (i = 0; !pending_irqs && i < nr_irq_groups; i++)
+		pending_irqs = ie[i] & readl_relaxed(pending + i);
+
+	if (!pending_irqs)
+		return false;
+
+	/* Isolate lowest set bit*/
+	pending_irqs &= -pending_irqs;
+
+	/* Disable all interrupts but the first pending one */
+	for (j = 0; j < nr_irq_groups; j++)
+		writel_relaxed(j == i ? pending_irqs : 0, enable + j);
+
+	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);
+	u32 __iomem *pending = handler->priv->regs + PENDING_BASE;
+	u32 __iomem *enable = handler->enable_base;
+	irq_hw_number_t hwirq = 0;
+	int i;
+
+	guard(raw_spinlock)(&handler->enable_lock);
+
+	/* Existing enable state is already cached in enable_save */
+	if (!cp100_isolate_pending_irq(nr_irq_groups, handler->enable_save, pending, enable))
+		return 0;
+
+	/*
+	 * Interrupts delievered to hardware still become pending, but only
+	 * interrupts that are both pending and enabled can be claimed.
+	 * Clearing enable bit for all interrupts but the first pending one
+	 * avoids hardware bug that occurs during read from claim register
+	 * with more than one eligible interrupt.
+	 */
+
+	hwirq = readl(claim);
+
+	/* Restore previous state */
+	for (i = 0; i < nr_irq_groups; i++)
+		writel_relaxed(handler->enable_save[i], enable + i);
+
+	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 */
@@ -434,6 +506,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) },
 	{}
 };
 
@@ -668,12 +742,17 @@ 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",
-- 
2.51.0

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

[Bo Gan]

Hi Lucas,

Please see my comments below.

On 10/16/25 01:42, 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 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@ultrarisc.com>
> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
> Acked-by: Samuel Holland <samuel.holland@sifive.com>
> Signed-off-by: Lucas Zampieri <lzampier@redhat.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));

Here we need to read all the enabled bits from HW register. What about if we 
keep the handler->enable_save in sync with HW? I.e., enable_save tracks the
HW enable registers and we maintain it per each irq toggle. In this way, we
don't need to read them at all. We'll introduce more overhead with irq_toggle,
but they are not frequent. Thus, we can move this reading of ie[] away from
hot-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 
writel_relaxed above and itself. There could be other barriers missing. Please 
check.

> +
> +	/* 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. If you chose
to use enable_save to track HW as I've described above, we should document it
well, because enable_save is only used by suspend/resume without this quirk.

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

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

[Bo Gan]

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@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 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@ultrarisc.com>
> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
> Acked-by: Samuel Holland <samuel.holland@sifive.com>
> Signed-off-by: Lucas Zampieri <lzampier@redhat.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.

> +
> +	/* 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

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

[Samuel Holland]

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@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 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@ultrarisc.com>
>> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
>> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
>> Acked-by: Samuel Holland <samuel.holland@sifive.com>
>> Signed-off-by: Lucas Zampieri <lzampier@redhat.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

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

[Bo Gan]

Hi Samuel,

On 10/16/25 15:01, Samuel Holland wrote:
> 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@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 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@ultrarisc.com>
>>> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
>>> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
>>> Acked-by: Samuel Holland <samuel.holland@sifive.com>
>>> Signed-off-by: Lucas Zampieri <lzampier@redhat.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
> 

Thanks for the clarification. I thought the I/O ordering requirement is more
fine grained in that it only preserves per-register order. Given that it's per
I/O region, my concern was not valid.

>>> +
>>> +    /* 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
> 

Bo

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

[Charles Mirabile]

Hi Bo—

On Thu, Oct 16, 2025 at 5:25 PM Bo Gan <ganboing@gmail.com> 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@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 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@ultrarisc.com>
> > Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
> > Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
> > Acked-by: Samuel Holland <samuel.holland@sifive.com>
> > Signed-off-by: Lucas Zampieri <lzampier@redhat.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

You mean put something like `if (!ie[i]) continue;` to avoid the readl
if the mask is going to obliterate it?

Sounds reasonable.
>
> > +     }
> > +
> > +     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[].

Something similar like `if (!ie[j]) continue;` in this loop too? We
know that this will not interact poorly with the i == j case because
ie[i] is by definition nonzero if we hit this code path and so when i
==j ie[j] == ie[j] != 0 so we will hit the rest of the logic. Also
sounds sane.

>
> > +     }
> > +
> > +     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.

Yes, a proper patch will come soon, just have to respin the whole
series. Two separate commits, one for refactoring the common code,
another for adding the quirk.

The changes do not overlap - the first patch will be hunks 3, 4, & 5
of the tentative diff I sent to Thomas, and patch two will be hunks 1,
2, 6, 7, 8.

If you have any concerns about the changes to common code, do let us know.

I will also pick up your feedback about avoiding the mmio by checking
ie[] in the loops.

>
> > +
> > +     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.
>
> > +
> > +     /* 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
>

Best—Charlie

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

[Lucas Zampieri]

Hi Thomas

I'm really sorry for that.

On Thu, Oct 16, 2025 at 11:16 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, Oct 16 2025 at 09:42, Lucas Zampieri wrote:
> > @@ -430,6 +516,8 @@ static const struct of_device_id plic_match[] = {
>       ^^^^^^^^^^^^
> How on earth did you manage to screw up the hunk header?
>
I was copying hunks over from one file to another to avoid
format-patch as it was screwing the email headers, after that I
learned that there's --[no-]encode-email-headers in format-patch for
that.

> Applying: irqchip/plic: Add support for UltraRISC DP1000 PLIC
> error: corrupt patch at line 116
>
> >         .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)
>      ^^^^^^^^^^^^^^^
> Ditto here.
>
> I fixed it up manually. Please be more careful next time.
>
For sure, wont happen again, thanks for taking the time of manually fixing it.

Lucas Zampieri

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

[Thomas Gleixner]

On Thu, Oct 16 2025 at 15:58, Charles Mirabile wrote:
> On Thu, Oct 16, 2025 at 07:53:26PM +0200, Thomas Gleixner wrote:
> What do you think about the attached patch (it should be not corrupt :^)
> I think I adressed your concerns, and I ran it through clang-format too.
>
> I folded everything into one diff for ease of review, but when we send it
> officially there will be a separate patch for the caching refactor.

Looks about right and it's a net win for everyone due to the suspend
path cleanup.

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

[Bo Gan]

Hi Charles,

On 10/16/25 16:25, Charles Mirabile wrote:
> Hi Bo—
> 
> On Thu, Oct 16, 2025 at 5:25 PM Bo Gan <ganboing@gmail.com> 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@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 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@ultrarisc.com>
>>> Signed-off-by: Zhang Xincheng <zhangxincheng@ultrarisc.com>
>>> Signed-off-by: Charles Mirabile <cmirabil@redhat.com>
>>> Acked-by: Samuel Holland <samuel.holland@sifive.com>
>>> Signed-off-by: Lucas Zampieri <lzampier@redhat.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
> 
> You mean put something like `if (!ie[i]) continue;` to avoid the readl
> if the mask is going to obliterate it?
> 

Yes.

> Sounds reasonable.
>>
>>> +     }
>>> +
>>> +     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[].
> 
> Something similar like `if (!ie[j]) continue;` in this loop too? We

Better to have:

	if (new_mask != ie[j])
		writel_relaxed(...)

> know that this will not interact poorly with the i == j case because
> ie[i] is by definition nonzero if we hit this code path and so when i
> ==j ie[j] == ie[j] != 0 so we will hit the rest of the logic. Also
> sounds sane.
>  >>
>>> +     }
>>> +
>>> +     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.
> 
> Yes, a proper patch will come soon, just have to respin the whole
> series. Two separate commits, one for refactoring the common code,
> another for adding the quirk.
> 
> The changes do not overlap - the first patch will be hunks 3, 4, & 5
> of the tentative diff I sent to Thomas, and patch two will be hunks 1,
> 2, 6, 7, 8.
> 
> If you have any concerns about the changes to common code, do let us know.
> 
> I will also pick up your feedback about avoiding the mmio by checking
> ie[] in the loops.
> 
>>
>>> +
>>> +     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.
>>
>>> +
>>> +     /* Restore previous state */
>>> +     for (i = 0; i < nr_irq_groups; i++)
>>> +             writel_relaxed(handler->enable_save[i], enable + i * sizeof(u32));

You can also:

	if (!handler->enable_save[i])
		// enable_save[i] has never changed, it's 0,
		// so we can't remove any more bits
		continue;

	if (i == hwirq / 32 && handler->enable_save[i] == (1UL << (hwirq %32)))
		// enable_save[i] has never changed,
		// because the enable bit of hwirq must have been enabled to
		// be able to claim this hwirq, and there were no more bits to remove
		continue;

	writel_relaxed(...)

>>> +
>>> +     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
>>
> 
> Best—Charlie
> 

Bo