[V5,3/3] irqchip/sifive-plic: Fixup thead, c900-plic request_threaded_irq with ONESHOT

Commit Message

Guo Ren Oct. 24, 2021, 1:33 a.m. UTC

From: Guo Ren <[e-mail]>

When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver,
only the first interrupt could be handled, and continue irq is blocked by
hw. Because the thead,c900-plic couldn't complete masked irq source which
has been disabled in enable register. Add thead_plic_chip which fix up
c906-plic irq source completion problem by unmask/mask wrapper.

Here is the description of Interrupt Completion in PLIC spec [1]:

The PLIC signals it has completed executing an interrupt handler by
writing the interrupt ID it received from the claim to the claim/complete
register. The PLIC does not check whether the completion ID is the same
as the last claim ID for that target. If the completion ID does not match
an interrupt source that is currently enabled for the target, the
                         ^^ ^^^^^^^^^ ^^^^^^^
completion is silently ignored.

[1] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adoc

Signed-off-by: Guo Ren <[e-mail]>
Cc: Anup Patel <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: Marc Zyngier <[e-mail]>
Cc: Palmer Dabbelt <[e-mail]>
Cc: Atish Patra <[e-mail]>

---

Changes since V5:
 - Move back to mask/unmask
 - Fixup the problem in eoi callback
 - Remove allwinner,sun20i-d1 IRQCHIP_DECLARE
 - Rewrite comment log

Changes since V4:
 - Update comment by Anup

Changes since V3:
 - Rename "c9xx" to "c900"
 - Add sifive_plic_chip and thead_plic_chip for difference

Changes since V2:
 - Add a separate compatible string "thead,c9xx-plic"
 - set irq_mask/unmask of "plic_chip" to NULL and point
   irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
 - Add a detailed comment block in plic_init() about the
   differences in Claim/Completion process of RISC-V PLIC and C9xx
   PLIC.
---
 drivers/irqchip/irq-sifive-plic.c | 44 +++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

Comments

Marc Zyngier Oct. 25, 2021, 10:48 a.m. UTC | #1

On Sun, 24 Oct 2021 02:33:03 +0100,
[e-mail] wrote:
> 
> From: Guo Ren <[e-mail]>
> 
> When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver,
> only the first interrupt could be handled, and continue irq is blocked by
> hw. Because the thead,c900-plic couldn't complete masked irq source which
> has been disabled in enable register. Add thead_plic_chip which fix up
> c906-plic irq source completion problem by unmask/mask wrapper.
> 
> Here is the description of Interrupt Completion in PLIC spec [1]:
> 
> The PLIC signals it has completed executing an interrupt handler by
> writing the interrupt ID it received from the claim to the claim/complete
> register. The PLIC does not check whether the completion ID is the same
> as the last claim ID for that target. If the completion ID does not match
> an interrupt source that is currently enabled for the target, the
>                          ^^ ^^^^^^^^^ ^^^^^^^
> completion is silently ignored.

Given this bit of the spec...

> +static void plic_thead_irq_eoi(struct irq_data *d)
> +{
> +	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> +
> +	if (irqd_irq_masked(d)) {
> +		plic_irq_unmask(d);
> +		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +		plic_irq_mask(d);
> +	} else {
> +		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +	}
> +}
> +

... it isn't obvious to me why this cannot happen on an SiFive PLIC.

And it isn't only for threaded interrupts in oneshot mode. Any driver
can mask an interrupt from its handler after having set the
IRQ_DISABLE_UNLAZY flag, and the interrupt would need the exact same
treatment.

	M.

Guo Ren Oct. 25, 2021, 1:33 p.m. UTC | #2

On Mon, Oct 25, 2021 at 6:48 PM Marc Zyngier <[e-mail]> wrote:
>
> On Sun, 24 Oct 2021 02:33:03 +0100,
> [e-mail] wrote:
> >
> > From: Guo Ren <[e-mail]>
> >
> > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver,
> > only the first interrupt could be handled, and continue irq is blocked by
> > hw. Because the thead,c900-plic couldn't complete masked irq source which
> > has been disabled in enable register. Add thead_plic_chip which fix up
> > c906-plic irq source completion problem by unmask/mask wrapper.
> >
> > Here is the description of Interrupt Completion in PLIC spec [1]:
> >
> > The PLIC signals it has completed executing an interrupt handler by
> > writing the interrupt ID it received from the claim to the claim/complete
> > register. The PLIC does not check whether the completion ID is the same
> > as the last claim ID for that target. If the completion ID does not match
> > an interrupt source that is currently enabled for the target, the
> >                          ^^ ^^^^^^^^^ ^^^^^^^
> > completion is silently ignored.
>
> Given this bit of the spec...
>
> > +static void plic_thead_irq_eoi(struct irq_data *d)
> > +{
> > +     struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> > +
> > +     if (irqd_irq_masked(d)) {
> > +             plic_irq_unmask(d);
> > +             writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> > +             plic_irq_mask(d);
> > +     } else {
> > +             writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> > +     }
> > +}
> > +
>
> ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
I'm not sure about SiFive PLIC. Maybe they didn't follow that to implement.

>
> And it isn't only for threaded interrupts in oneshot mode. Any driver
> can mask an interrupt from its handler after having set the
> IRQ_DISABLE_UNLAZY flag, and the interrupt would need the exact same
> treatment.
Thx for mentioned here, and I'll add it in the comment of next version patch.

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Nikita Shubin Oct. 28, 2021, 10:55 a.m. UTC | #3

Hello Marc and Guo Ren!

On Mon, 25 Oct 2021 11:48:33 +0100
Marc Zyngier <[e-mail]> wrote:

> On Sun, 24 Oct 2021 02:33:03 +0100,
> [e-mail] wrote:
> > 
> > From: Guo Ren <[e-mail]>
> > 
> > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > driver, only the first interrupt could be handled, and continue irq
> > is blocked by hw. Because the thead,c900-plic couldn't complete
> > masked irq source which has been disabled in enable register. Add
> > thead_plic_chip which fix up c906-plic irq source completion
> > problem by unmask/mask wrapper.
> > 
> > Here is the description of Interrupt Completion in PLIC spec [1]:
> > 
> > The PLIC signals it has completed executing an interrupt handler by
> > writing the interrupt ID it received from the claim to the
> > claim/complete register. The PLIC does not check whether the
> > completion ID is the same as the last claim ID for that target. If
> > the completion ID does not match an interrupt source that is
> > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > completion is silently ignored.  
> 
> Given this bit of the spec...
> 
> > +static void plic_thead_irq_eoi(struct irq_data *d)
> > +{
> > +	struct plic_handler *handler =
> > this_cpu_ptr(&plic_handlers); +
> > +	if (irqd_irq_masked(d)) {
> > +		plic_irq_unmask(d);
> > +		writel(d->hwirq, handler->hart_base +
> > CONTEXT_CLAIM);
> > +		plic_irq_mask(d);
> > +	} else {
> > +		writel(d->hwirq, handler->hart_base +
> > CONTEXT_CLAIM);
> > +	}
> > +}
> > +  
> 
> ... it isn't obvious to me why this cannot happen on an SiFive PLIC.

This indeed happens with SiFive PLIC. I am currently tinkering with
da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
begins to work fine.

May be these change should be propagated to plic_irq_eoi instead of
making a new function ?

> 
> And it isn't only for threaded interrupts in oneshot mode. Any driver
> can mask an interrupt from its handler after having set the
> IRQ_DISABLE_UNLAZY flag, and the interrupt would need the exact same
> treatment.
> 
> 	M.
> 

---
diff --git a/drivers/irqchip/irq-sifive-plic.c
b/drivers/irqchip/irq-sifive-plic.c index cf74cfa82045..259065d271ef
100644 --- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d)
 {
        struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
 
-       writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+       if (irqd_irq_masked(d)) {
+               plic_irq_unmask(d);
+               writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+               plic_irq_mask(d);
+       } else {
+               writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+       }
 }
 
 static struct irq_chip plic_chip = {

Marc Zyngier Oct. 28, 2021, 2:58 p.m. UTC | #4

On Thu, 28 Oct 2021 11:55:23 +0100,
Nikita Shubin <[e-mail]> wrote:
> 
> Hello Marc and Guo Ren!
> 
> On Mon, 25 Oct 2021 11:48:33 +0100
> Marc Zyngier <[e-mail]> wrote:
> 
> > On Sun, 24 Oct 2021 02:33:03 +0100,
> > [e-mail] wrote:
> > > 
> > > From: Guo Ren <[e-mail]>
> > > 
> > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > driver, only the first interrupt could be handled, and continue irq
> > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > masked irq source which has been disabled in enable register. Add
> > > thead_plic_chip which fix up c906-plic irq source completion
> > > problem by unmask/mask wrapper.
> > > 
> > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > > 
> > > The PLIC signals it has completed executing an interrupt handler by
> > > writing the interrupt ID it received from the claim to the
> > > claim/complete register. The PLIC does not check whether the
> > > completion ID is the same as the last claim ID for that target. If
> > > the completion ID does not match an interrupt source that is
> > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > completion is silently ignored.  
> > 
> > Given this bit of the spec...
> > 
> > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > +{
> > > +	struct plic_handler *handler =
> > > this_cpu_ptr(&plic_handlers); +
> > > +	if (irqd_irq_masked(d)) {
> > > +		plic_irq_unmask(d);
> > > +		writel(d->hwirq, handler->hart_base +
> > > CONTEXT_CLAIM);
> > > +		plic_irq_mask(d);
> > > +	} else {
> > > +		writel(d->hwirq, handler->hart_base +
> > > CONTEXT_CLAIM);
> > > +	}
> > > +}
> > > +  
> > 
> > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
> 
> This indeed happens with SiFive PLIC. I am currently tinkering with
> da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> begins to work fine.
> 
> May be these change should be propagated to plic_irq_eoi instead of
> making a new function ?

That's my impression too. I think the T-Head defect is pretty much
immaterial when you consider how 'interesting' the PLIC architecture
is. Conflating EOI and masking really is a misfeature...

	M.

Anup Patel Oct. 30, 2021, 10:27 a.m. UTC | #5

On Thu, Oct 28, 2021 at 8:28 PM Marc Zyngier <[e-mail]> wrote:
>
> On Thu, 28 Oct 2021 11:55:23 +0100,
> Nikita Shubin <[e-mail]> wrote:
> >
> > Hello Marc and Guo Ren!
> >
> > On Mon, 25 Oct 2021 11:48:33 +0100
> > Marc Zyngier <[e-mail]> wrote:
> >
> > > On Sun, 24 Oct 2021 02:33:03 +0100,
> > > [e-mail] wrote:
> > > >
> > > > From: Guo Ren <[e-mail]>
> > > >
> > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > > driver, only the first interrupt could be handled, and continue irq
> > > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > > masked irq source which has been disabled in enable register. Add
> > > > thead_plic_chip which fix up c906-plic irq source completion
> > > > problem by unmask/mask wrapper.
> > > >
> > > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > > >
> > > > The PLIC signals it has completed executing an interrupt handler by
> > > > writing the interrupt ID it received from the claim to the
> > > > claim/complete register. The PLIC does not check whether the
> > > > completion ID is the same as the last claim ID for that target. If
> > > > the completion ID does not match an interrupt source that is
> > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > > completion is silently ignored.
> > >
> > > Given this bit of the spec...
> > >
> > > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > > +{
> > > > + struct plic_handler *handler =
> > > > this_cpu_ptr(&plic_handlers); +
> > > > + if (irqd_irq_masked(d)) {
> > > > +         plic_irq_unmask(d);
> > > > +         writel(d->hwirq, handler->hart_base +
> > > > CONTEXT_CLAIM);
> > > > +         plic_irq_mask(d);
> > > > + } else {
> > > > +         writel(d->hwirq, handler->hart_base +
> > > > CONTEXT_CLAIM);
> > > > + }
> > > > +}
> > > > +
> > >
> > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
> >
> > This indeed happens with SiFive PLIC. I am currently tinkering with
> > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> > begins to work fine.
> >
> > May be these change should be propagated to plic_irq_eoi instead of
> > making a new function ?
>
> That's my impression too. I think the T-Head defect is pretty much
> immaterial when you consider how 'interesting' the PLIC architecture
> is. Conflating EOI and masking really is a misfeature...

Unfortunately, the PLIC implementation is the same across existing
boards (except T-HEAD) so this issue is there on all existing boards.

I double checked the upcoming RISC-V AIA specification and this
problem is not there in RISC-V AIA because the interrupt claim/completion
is different.
(Refer, https://github.com/riscv/riscv-aia/releases/download/0.2-draft.27/riscv-interrupts-027.pdf)

I plan to send-out RFC PATCH for AIA soon so that we get early
feedback from everyone on LKML.

Regards,
Anup

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Guo Ren Nov. 1, 2021, 2 a.m. UTC | #6

Thx Nikita,

I would fixup that globally in the plic in next version of patch.

On Thu, Oct 28, 2021 at 7:01 PM Nikita Shubin <[e-mail]> wrote:
>
> Hello Marc and Guo Ren!
>
> On Mon, 25 Oct 2021 11:48:33 +0100
> Marc Zyngier <[e-mail]> wrote:
>
> > On Sun, 24 Oct 2021 02:33:03 +0100,
> > [e-mail] wrote:
> > >
> > > From: Guo Ren <[e-mail]>
> > >
> > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > driver, only the first interrupt could be handled, and continue irq
> > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > masked irq source which has been disabled in enable register. Add
> > > thead_plic_chip which fix up c906-plic irq source completion
> > > problem by unmask/mask wrapper.
> > >
> > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > >
> > > The PLIC signals it has completed executing an interrupt handler by
> > > writing the interrupt ID it received from the claim to the
> > > claim/complete register. The PLIC does not check whether the
> > > completion ID is the same as the last claim ID for that target. If
> > > the completion ID does not match an interrupt source that is
> > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > completion is silently ignored.
> >
> > Given this bit of the spec...
> >
> > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > +{
> > > +   struct plic_handler *handler =
> > > this_cpu_ptr(&plic_handlers); +
> > > +   if (irqd_irq_masked(d)) {
> > > +           plic_irq_unmask(d);
> > > +           writel(d->hwirq, handler->hart_base +
> > > CONTEXT_CLAIM);
> > > +           plic_irq_mask(d);
> > > +   } else {
> > > +           writel(d->hwirq, handler->hart_base +
> > > CONTEXT_CLAIM);
> > > +   }
> > > +}
> > > +
> >
> > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
>
> This indeed happens with SiFive PLIC. I am currently tinkering with
> da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> begins to work fine.
>
> May be these change should be propagated to plic_irq_eoi instead of
> making a new function ?
>
> >
> > And it isn't only for threaded interrupts in oneshot mode. Any driver
> > can mask an interrupt from its handler after having set the
> > IRQ_DISABLE_UNLAZY flag, and the interrupt would need the exact same
> > treatment.
> >
> >       M.
> >
>
> ---
> diff --git a/drivers/irqchip/irq-sifive-plic.c
> b/drivers/irqchip/irq-sifive-plic.c index cf74cfa82045..259065d271ef
> 100644 --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d)
>  {
>         struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
>
> -       writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +       if (irqd_irq_masked(d)) {
> +               plic_irq_unmask(d);
> +               writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +               plic_irq_mask(d);
> +       } else {
> +               writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +       }
>  }
>
>  static struct irq_chip plic_chip = {

Guo Ren Nov. 1, 2021, 2:20 a.m. UTC | #7

On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[e-mail]> wrote:
>
> On Thu, 28 Oct 2021 11:55:23 +0100,
> Nikita Shubin <[e-mail]> wrote:
> >
> > Hello Marc and Guo Ren!
> >
> > On Mon, 25 Oct 2021 11:48:33 +0100
> > Marc Zyngier <[e-mail]> wrote:
> >
> > > On Sun, 24 Oct 2021 02:33:03 +0100,
> > > [e-mail] wrote:
> > > >
> > > > From: Guo Ren <[e-mail]>
> > > >
> > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > > driver, only the first interrupt could be handled, and continue irq
> > > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > > masked irq source which has been disabled in enable register. Add
> > > > thead_plic_chip which fix up c906-plic irq source completion
> > > > problem by unmask/mask wrapper.
> > > >
> > > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > > >
> > > > The PLIC signals it has completed executing an interrupt handler by
> > > > writing the interrupt ID it received from the claim to the
> > > > claim/complete register. The PLIC does not check whether the
> > > > completion ID is the same as the last claim ID for that target. If
> > > > the completion ID does not match an interrupt source that is
> > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > > completion is silently ignored.
> > >
> > > Given this bit of the spec...
> > >
> > > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > > +{
> > > > + struct plic_handler *handler =
> > > > this_cpu_ptr(&plic_handlers); +
> > > > + if (irqd_irq_masked(d)) {
> > > > +         plic_irq_unmask(d);
> > > > +         writel(d->hwirq, handler->hart_base +
> > > > CONTEXT_CLAIM);
> > > > +         plic_irq_mask(d);
> > > > + } else {
> > > > +         writel(d->hwirq, handler->hart_base +
> > > > CONTEXT_CLAIM);
> > > > + }
> > > > +}
> > > > +
> > >
> > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
> >
> > This indeed happens with SiFive PLIC. I am currently tinkering with
> > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> > begins to work fine.
> >
> > May be these change should be propagated to plic_irq_eoi instead of
> > making a new function ?
>
> That's my impression too. I think the T-Head defect is pretty much
> immaterial when you consider how 'interesting' the PLIC architecture
> is.
Which is the "T-Head defect" you mentioned here?
 1. Auto masking with claim + complete (I don't think it's a defect,
right? May I add a new patch to utilize the feature to decrease a
little duplicate mask/unmask operations in the future?)
 2. EOI failed when masked

> Conflating EOI and masking really is a misfeature...
I think the problem is riscv PLIC reuse enable bit as mask bit. I
recommend separating them. That means:
 - EOI still depends on enable bit.
 - Add mask/unmask bit regs to do the right thing.

>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Anup Patel Nov. 1, 2021, 2:53 a.m. UTC | #8

On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <[e-mail]> wrote:
>
> On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[e-mail]> wrote:
> >
> > On Thu, 28 Oct 2021 11:55:23 +0100,
> > Nikita Shubin <[e-mail]> wrote:
> > >
> > > Hello Marc and Guo Ren!
> > >
> > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > Marc Zyngier <[e-mail]> wrote:
> > >
> > > > On Sun, 24 Oct 2021 02:33:03 +0100,
> > > > [e-mail] wrote:
> > > > >
> > > > > From: Guo Ren <[e-mail]>
> > > > >
> > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > > > driver, only the first interrupt could be handled, and continue irq
> > > > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > > > masked irq source which has been disabled in enable register. Add
> > > > > thead_plic_chip which fix up c906-plic irq source completion
> > > > > problem by unmask/mask wrapper.
> > > > >
> > > > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > > > >
> > > > > The PLIC signals it has completed executing an interrupt handler by
> > > > > writing the interrupt ID it received from the claim to the
> > > > > claim/complete register. The PLIC does not check whether the
> > > > > completion ID is the same as the last claim ID for that target. If
> > > > > the completion ID does not match an interrupt source that is
> > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > > > completion is silently ignored.
> > > >
> > > > Given this bit of the spec...
> > > >
> > > > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > > > +{
> > > > > + struct plic_handler *handler =
> > > > > this_cpu_ptr(&plic_handlers); +
> > > > > + if (irqd_irq_masked(d)) {
> > > > > +         plic_irq_unmask(d);
> > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > CONTEXT_CLAIM);
> > > > > +         plic_irq_mask(d);
> > > > > + } else {
> > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > CONTEXT_CLAIM);
> > > > > + }
> > > > > +}
> > > > > +
> > > >
> > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
> > >
> > > This indeed happens with SiFive PLIC. I am currently tinkering with
> > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> > > begins to work fine.
> > >
> > > May be these change should be propagated to plic_irq_eoi instead of
> > > making a new function ?
> >
> > That's my impression too. I think the T-Head defect is pretty much
> > immaterial when you consider how 'interesting' the PLIC architecture
> > is.
> Which is the "T-Head defect" you mentioned here?
>  1. Auto masking with claim + complete (I don't think it's a defect,
> right? May I add a new patch to utilize the feature to decrease a
> little duplicate mask/unmask operations in the future?)

This is definitely a defect and non-compliance for T-HEAD because
no sane interrupt controller would mask interrupt upon claim and this
is not what RISC-V PLIC defines.

>  2. EOI failed when masked

This defect exists for both RISC-V PLIC and T-HEAD PLIC
because of the way interrupt completion is defined.

>
> > Conflating EOI and masking really is a misfeature...
> I think the problem is riscv PLIC reuse enable bit as mask bit. I
> recommend separating them. That means:

There are no per-interrupt mask bits. We only have per-context
and per-interrupt enable bits which is used to provide mask/unmask
functionality expected by the irqchip framework.

I don't see how this is a problem for RISC-V PLIC. The only real
issue with RISC-V PLIC is the fact the interrupt completion will be
ignored for a masked interrupt which is what Marc is pointing at.

Regards,
Anup

>  - EOI still depends on enable bit.
>  - Add mask/unmask bit regs to do the right thing.



>
> >
> >         M.
> >
> > --
> > Without deviation from the norm, progress is not possible.
>
>
>
> --
> Best Regards
>  Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/

Guo Ren Nov. 1, 2021, 3:57 a.m. UTC | #9

On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <[e-mail]> wrote:
>
> On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <[e-mail]> wrote:
> >
> > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[e-mail]> wrote:
> > >
> > > On Thu, 28 Oct 2021 11:55:23 +0100,
> > > Nikita Shubin <[e-mail]> wrote:
> > > >
> > > > Hello Marc and Guo Ren!
> > > >
> > > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > > Marc Zyngier <[e-mail]> wrote:
> > > >
> > > > > On Sun, 24 Oct 2021 02:33:03 +0100,
> > > > > [e-mail] wrote:
> > > > > >
> > > > > > From: Guo Ren <[e-mail]>
> > > > > >
> > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > > > > driver, only the first interrupt could be handled, and continue irq
> > > > > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > > > > masked irq source which has been disabled in enable register. Add
> > > > > > thead_plic_chip which fix up c906-plic irq source completion
> > > > > > problem by unmask/mask wrapper.
> > > > > >
> > > > > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > > > > >
> > > > > > The PLIC signals it has completed executing an interrupt handler by
> > > > > > writing the interrupt ID it received from the claim to the
> > > > > > claim/complete register. The PLIC does not check whether the
> > > > > > completion ID is the same as the last claim ID for that target. If
> > > > > > the completion ID does not match an interrupt source that is
> > > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > > > > completion is silently ignored.
> > > > >
> > > > > Given this bit of the spec...
> > > > >
> > > > > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > > > > +{
> > > > > > + struct plic_handler *handler =
> > > > > > this_cpu_ptr(&plic_handlers); +
> > > > > > + if (irqd_irq_masked(d)) {
> > > > > > +         plic_irq_unmask(d);
> > > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > > CONTEXT_CLAIM);
> > > > > > +         plic_irq_mask(d);
> > > > > > + } else {
> > > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > > CONTEXT_CLAIM);
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > >
> > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
> > > >
> > > > This indeed happens with SiFive PLIC. I am currently tinkering with
> > > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> > > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> > > > begins to work fine.
> > > >
> > > > May be these change should be propagated to plic_irq_eoi instead of
> > > > making a new function ?
> > >
> > > That's my impression too. I think the T-Head defect is pretty much
> > > immaterial when you consider how 'interesting' the PLIC architecture
> > > is.
> > Which is the "T-Head defect" you mentioned here?
> >  1. Auto masking with claim + complete (I don't think it's a defect,
> > right? May I add a new patch to utilize the feature to decrease a
> > little duplicate mask/unmask operations in the future?)
>
> This is definitely a defect and non-compliance for T-HEAD because
I just agree with non-compliance, but what's the defect of
auto-masking? If somebody could explain, I'm very grateful.

> no sane interrupt controller would mask interrupt upon claim and this
> is not what RISC-V PLIC defines.
>
> >  2. EOI failed when masked
>
> This defect exists for both RISC-V PLIC and T-HEAD PLIC
> because of the way interrupt completion is defined.
>
> >
> > > Conflating EOI and masking really is a misfeature...
> > I think the problem is riscv PLIC reuse enable bit as mask bit. I
> > recommend separating them. That means:
>
> There are no per-interrupt mask bits. We only have per-context
> and per-interrupt enable bits which is used to provide mask/unmask
> functionality expected by the irqchip framework.
>
> I don't see how this is a problem for RISC-V PLIC. The only real
> issue with RISC-V PLIC is the fact the interrupt completion will be
> ignored for a masked interrupt which is what Marc is pointing at.
So you are not considering add per-interrupt mask bits to solve the
above problem, right?

I don't think you would keep below codes in AIA eoi.
 +             plic_irq_unmask(d);
 +             writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
 +             plic_irq_mask(d);

>
> Regards,
> Anup
>
> >  - EOI still depends on enable bit.
> >  - Add mask/unmask bit regs to do the right thing.
>
>
>
> >
> > >
> > >         M.
> > >
> > > --
> > > Without deviation from the norm, progress is not possible.
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren
> >
> > ML: https://lore.kernel.org/linux-csky/

Anup Patel Nov. 1, 2021, 4:27 a.m. UTC | #10

On Mon, Nov 1, 2021 at 9:27 AM Guo Ren <[e-mail]> wrote:
>
> On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <[e-mail]> wrote:
> >
> > On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <[e-mail]> wrote:
> > >
> > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[e-mail]> wrote:
> > > >
> > > > On Thu, 28 Oct 2021 11:55:23 +0100,
> > > > Nikita Shubin <[e-mail]> wrote:
> > > > >
> > > > > Hello Marc and Guo Ren!
> > > > >
> > > > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > > > Marc Zyngier <[e-mail]> wrote:
> > > > >
> > > > > > On Sun, 24 Oct 2021 02:33:03 +0100,
> > > > > > [e-mail] wrote:
> > > > > > >
> > > > > > > From: Guo Ren <[e-mail]>
> > > > > > >
> > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > > > > > driver, only the first interrupt could be handled, and continue irq
> > > > > > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > > > > > masked irq source which has been disabled in enable register. Add
> > > > > > > thead_plic_chip which fix up c906-plic irq source completion
> > > > > > > problem by unmask/mask wrapper.
> > > > > > >
> > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > > > > > >
> > > > > > > The PLIC signals it has completed executing an interrupt handler by
> > > > > > > writing the interrupt ID it received from the claim to the
> > > > > > > claim/complete register. The PLIC does not check whether the
> > > > > > > completion ID is the same as the last claim ID for that target. If
> > > > > > > the completion ID does not match an interrupt source that is
> > > > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > > > > > completion is silently ignored.
> > > > > >
> > > > > > Given this bit of the spec...
> > > > > >
> > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > > > > > +{
> > > > > > > + struct plic_handler *handler =
> > > > > > > this_cpu_ptr(&plic_handlers); +
> > > > > > > + if (irqd_irq_masked(d)) {
> > > > > > > +         plic_irq_unmask(d);
> > > > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > > > CONTEXT_CLAIM);
> > > > > > > +         plic_irq_mask(d);
> > > > > > > + } else {
> > > > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > > > CONTEXT_CLAIM);
> > > > > > > + }
> > > > > > > +}
> > > > > > > +
> > > > > >
> > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
> > > > >
> > > > > This indeed happens with SiFive PLIC. I am currently tinkering with
> > > > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> > > > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> > > > > begins to work fine.
> > > > >
> > > > > May be these change should be propagated to plic_irq_eoi instead of
> > > > > making a new function ?
> > > >
> > > > That's my impression too. I think the T-Head defect is pretty much
> > > > immaterial when you consider how 'interesting' the PLIC architecture
> > > > is.
> > > Which is the "T-Head defect" you mentioned here?
> > >  1. Auto masking with claim + complete (I don't think it's a defect,
> > > right? May I add a new patch to utilize the feature to decrease a
> > > little duplicate mask/unmask operations in the future?)
> >
> > This is definitely a defect and non-compliance for T-HEAD because
> I just agree with non-compliance, but what's the defect of
> auto-masking? If somebody could explain, I'm very grateful.
>
> > no sane interrupt controller would mask interrupt upon claim and this
> > is not what RISC-V PLIC defines.
> >
> > >  2. EOI failed when masked
> >
> > This defect exists for both RISC-V PLIC and T-HEAD PLIC
> > because of the way interrupt completion is defined.
> >
> > >
> > > > Conflating EOI and masking really is a misfeature...
> > > I think the problem is riscv PLIC reuse enable bit as mask bit. I
> > > recommend separating them. That means:
> >
> > There are no per-interrupt mask bits. We only have per-context
> > and per-interrupt enable bits which is used to provide mask/unmask
> > functionality expected by the irqchip framework.
> >
> > I don't see how this is a problem for RISC-V PLIC. The only real
> > issue with RISC-V PLIC is the fact the interrupt completion will be
> > ignored for a masked interrupt which is what Marc is pointing at.
> So you are not considering add per-interrupt mask bits to solve the
> above problem, right?

The RISC-V PLIC has several limitations and also lacks a lot of features
hence it's marked as deprecated in RISC-V platform specs and will be
removed eventually from RISC-V platform specs.

The RISC-V AIA will totally replace RISC-V PLIC going forward. In fact,
RISC-V AIA APLIC addresses all limitations of RISC-V PLIC along with
new features additions.

>
> I don't think you would keep below codes in AIA eoi.
>  +             plic_irq_unmask(d);
>  +             writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
>  +             plic_irq_mask(d);

Like I mentioned previously, the AIA APLIC is very different from the
PLIC so we don't need this mask/unmask dance over there. It has global
per-interrupt enable bits in AIA APLIC which is different from PLIC.

Regards,
Anup

>
> >
> > Regards,
> > Anup
> >
> > >  - EOI still depends on enable bit.
> > >  - Add mask/unmask bit regs to do the right thing.
> >
> >
> >
> > >
> > > >
> > > >         M.
> > > >
> > > > --
> > > > Without deviation from the norm, progress is not possible.
> > >
> > >
> > >
> > > --
> > > Best Regards
> > >  Guo Ren
> > >
> > > ML: https://lore.kernel.org/linux-csky/
>
>
>
> --
> Best Regards
>  Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/

Vincent Pelletier Nov. 1, 2021, 5:11 a.m. UTC | #11

On Thu, 28 Oct 2021 13:55:23 +0300, Nikita Shubin <[e-mail]> wrote:
> This indeed happens with SiFive PLIC. I am currently tinkering with
> da9063 RTC on SiFive Unmatched, and ALARM irq fires only once.

Happy to see someone else having this issue. I hit this issue in July
and tried to get feedback, but nothing happened and I gave up:
  http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html

My uneducated guess, by spying on the registers behind the kernel's
back (see the python script I attached), was that this could be
specific to level-signalled interrupts, where the IRQ re-triggers in
the PLIC right after being cleared but after being unbound from any
hart. Then the "IRQ pending" flag is set (causing the IRQ edge which
would normally trigger interrupt handling in associated hart) without
anything noticing, so it will never be cleared and never be handled.

> However
> with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> begins to work fine.

Great news, so this issue will be fixed in a better way than my RFC.
RFC which can hence be discarded in patchwork, I believe:
  https://patchwork.kernel.org/project/linux-riscv/patch/8c3[e-mail]/
(I'm not sure if I can do it myself)

Regards,

Guo Ren Nov. 1, 2021, 7:56 a.m. UTC | #12

On Mon, Nov 1, 2021 at 12:28 PM Anup Patel <[e-mail]> wrote:
>
> On Mon, Nov 1, 2021 at 9:27 AM Guo Ren <[e-mail]> wrote:
> >
> > On Mon, Nov 1, 2021 at 10:53 AM Anup Patel <[e-mail]> wrote:
> > >
> > > On Mon, Nov 1, 2021 at 7:50 AM Guo Ren <[e-mail]> wrote:
> > > >
> > > > On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[e-mail]> wrote:
> > > > >
> > > > > On Thu, 28 Oct 2021 11:55:23 +0100,
> > > > > Nikita Shubin <[e-mail]> wrote:
> > > > > >
> > > > > > Hello Marc and Guo Ren!
> > > > > >
> > > > > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > > > > Marc Zyngier <[e-mail]> wrote:
> > > > > >
> > > > > > > On Sun, 24 Oct 2021 02:33:03 +0100,
> > > > > > > [e-mail] wrote:
> > > > > > > >
> > > > > > > > From: Guo Ren <[e-mail]>
> > > > > > > >
> > > > > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > > > > > > driver, only the first interrupt could be handled, and continue irq
> > > > > > > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > > > > > > masked irq source which has been disabled in enable register. Add
> > > > > > > > thead_plic_chip which fix up c906-plic irq source completion
> > > > > > > > problem by unmask/mask wrapper.
> > > > > > > >
> > > > > > > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > > > > > > >
> > > > > > > > The PLIC signals it has completed executing an interrupt handler by
> > > > > > > > writing the interrupt ID it received from the claim to the
> > > > > > > > claim/complete register. The PLIC does not check whether the
> > > > > > > > completion ID is the same as the last claim ID for that target. If
> > > > > > > > the completion ID does not match an interrupt source that is
> > > > > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > > > > > > completion is silently ignored.
> > > > > > >
> > > > > > > Given this bit of the spec...
> > > > > > >
> > > > > > > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > > > > > > +{
> > > > > > > > + struct plic_handler *handler =
> > > > > > > > this_cpu_ptr(&plic_handlers); +
> > > > > > > > + if (irqd_irq_masked(d)) {
> > > > > > > > +         plic_irq_unmask(d);
> > > > > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > > > > CONTEXT_CLAIM);
> > > > > > > > +         plic_irq_mask(d);
> > > > > > > > + } else {
> > > > > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > > > > CONTEXT_CLAIM);
> > > > > > > > + }
> > > > > > > > +}
> > > > > > > > +
> > > > > > >
> > > > > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
> > > > > >
> > > > > > This indeed happens with SiFive PLIC. I am currently tinkering with
> > > > > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> > > > > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> > > > > > begins to work fine.
> > > > > >
> > > > > > May be these change should be propagated to plic_irq_eoi instead of
> > > > > > making a new function ?
> > > > >
> > > > > That's my impression too. I think the T-Head defect is pretty much
> > > > > immaterial when you consider how 'interesting' the PLIC architecture
> > > > > is.
> > > > Which is the "T-Head defect" you mentioned here?
> > > >  1. Auto masking with claim + complete (I don't think it's a defect,
> > > > right? May I add a new patch to utilize the feature to decrease a
> > > > little duplicate mask/unmask operations in the future?)
> > >
> > > This is definitely a defect and non-compliance for T-HEAD because
> > I just agree with non-compliance, but what's the defect of
> > auto-masking? If somebody could explain, I'm very grateful.
> >
> > > no sane interrupt controller would mask interrupt upon claim and this
> > > is not what RISC-V PLIC defines.
> > >
> > > >  2. EOI failed when masked
> > >
> > > This defect exists for both RISC-V PLIC and T-HEAD PLIC
> > > because of the way interrupt completion is defined.
> > >
> > > >
> > > > > Conflating EOI and masking really is a misfeature...
> > > > I think the problem is riscv PLIC reuse enable bit as mask bit. I
> > > > recommend separating them. That means:
> > >
> > > There are no per-interrupt mask bits. We only have per-context
> > > and per-interrupt enable bits which is used to provide mask/unmask
> > > functionality expected by the irqchip framework.
> > >
> > > I don't see how this is a problem for RISC-V PLIC. The only real
> > > issue with RISC-V PLIC is the fact the interrupt completion will be
> > > ignored for a masked interrupt which is what Marc is pointing at.
> > So you are not considering add per-interrupt mask bits to solve the
> > above problem, right?
>
> The RISC-V PLIC has several limitations and also lacks a lot of features
> hence it's marked as deprecated in RISC-V platform specs and will be
> removed eventually from RISC-V platform specs.
>
> The RISC-V AIA will totally replace RISC-V PLIC going forward. In fact,
> RISC-V AIA APLIC addresses all limitations of RISC-V PLIC along with
> new features additions.
>
> >
> > I don't think you would keep below codes in AIA eoi.
> >  +             plic_irq_unmask(d);
> >  +             writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> >  +             plic_irq_mask(d);
>
> Like I mentioned previously, the AIA APLIC is very different from the
> PLIC so we don't need this mask/unmask dance over there. It has global
> per-interrupt enable bits in AIA APLIC which is different from PLIC.
The ”global per-interrupt enable bits“ is okay.

>
> Regards,
> Anup
>
> >
> > >
> > > Regards,
> > > Anup
> > >
> > > >  - EOI still depends on enable bit.
> > > >  - Add mask/unmask bit regs to do the right thing.
> > >
> > >
> > >
> > > >
> > > > >
> > > > >         M.
> > > > >
> > > > > --
> > > > > Without deviation from the norm, progress is not possible.
> > > >
> > > >
> > > >
> > > > --
> > > > Best Regards
> > > >  Guo Ren
> > > >
> > > > ML: https://lore.kernel.org/linux-csky/
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren
> >
> > ML: https://lore.kernel.org/linux-csky/

Marc Zyngier Nov. 1, 2021, 9:25 a.m. UTC | #13

On Mon, 01 Nov 2021 02:20:21 +0000,
Guo Ren <[e-mail]> wrote:
> 
> On Thu, Oct 28, 2021 at 10:58 PM Marc Zyngier <[e-mail]> wrote:
> >
> > On Thu, 28 Oct 2021 11:55:23 +0100,
> > Nikita Shubin <[e-mail]> wrote:
> > >
> > > Hello Marc and Guo Ren!
> > >
> > > On Mon, 25 Oct 2021 11:48:33 +0100
> > > Marc Zyngier <[e-mail]> wrote:
> > >
> > > > On Sun, 24 Oct 2021 02:33:03 +0100,
> > > > [e-mail] wrote:
> > > > >
> > > > > From: Guo Ren <[e-mail]>
> > > > >
> > > > > When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the
> > > > > driver, only the first interrupt could be handled, and continue irq
> > > > > is blocked by hw. Because the thead,c900-plic couldn't complete
> > > > > masked irq source which has been disabled in enable register. Add
> > > > > thead_plic_chip which fix up c906-plic irq source completion
> > > > > problem by unmask/mask wrapper.
> > > > >
> > > > > Here is the description of Interrupt Completion in PLIC spec [1]:
> > > > >
> > > > > The PLIC signals it has completed executing an interrupt handler by
> > > > > writing the interrupt ID it received from the claim to the
> > > > > claim/complete register. The PLIC does not check whether the
> > > > > completion ID is the same as the last claim ID for that target. If
> > > > > the completion ID does not match an interrupt source that is
> > > > > currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^
> > > > > completion is silently ignored.
> > > >
> > > > Given this bit of the spec...
> > > >
> > > > > +static void plic_thead_irq_eoi(struct irq_data *d)
> > > > > +{
> > > > > + struct plic_handler *handler =
> > > > > this_cpu_ptr(&plic_handlers); +
> > > > > + if (irqd_irq_masked(d)) {
> > > > > +         plic_irq_unmask(d);
> > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > CONTEXT_CLAIM);
> > > > > +         plic_irq_mask(d);
> > > > > + } else {
> > > > > +         writel(d->hwirq, handler->hart_base +
> > > > > CONTEXT_CLAIM);
> > > > > + }
> > > > > +}
> > > > > +
> > > >
> > > > ... it isn't obvious to me why this cannot happen on an SiFive PLIC.
> > >
> > > This indeed happens with SiFive PLIC. I am currently tinkering with
> > > da9063 RTC on SiFive Unmatched, and ALARM irq fires only once. However
> > > with changes proposed by Guo Ren in plic_thead_irq_eoi, everything
> > > begins to work fine.
> > >
> > > May be these change should be propagated to plic_irq_eoi instead of
> > > making a new function ?
> >
> > That's my impression too. I think the T-Head defect is pretty much
> > immaterial when you consider how 'interesting' the PLIC architecture
> > is.
> Which is the "T-Head defect" you mentioned here?
>  1. Auto masking with claim + complete (I don't think it's a defect,
> right? May I add a new patch to utilize the feature to decrease a
> little duplicate mask/unmask operations in the future?)

That *is* a T-Head defect. It may not be material for Linux, but being
a departure from the spec, it is a bug, clear and simple. IMHO, either
you implement the spec to the letter, or you don't. If you deviate,
this is something else.

>  2. EOI failed when masked

This one is a PLIC architecture defect, which seems to plague everyone.

> 
> > Conflating EOI and masking really is a misfeature...
> I think the problem is riscv PLIC reuse enable bit as mask bit. I
> recommend separating them. That means:
>  - EOI still depends on enable bit.
>  - Add mask/unmask bit regs to do the right thing.

Maybe, but that's not the problem at hand. I suggest you move
architectural discussions to a separate thread, and keep this thread
for fixing the mess that plagues existing users.

	M.

Marc Zyngier Nov. 1, 2021, 9:27 a.m. UTC | #14

On Mon, 01 Nov 2021 04:27:50 +0000,
Anup Patel <[e-mail]> wrote:

> The RISC-V AIA will totally replace RISC-V PLIC going forward. In fact,
> RISC-V AIA APLIC addresses all limitations of RISC-V PLIC along with
> new features additions.

Instead of arguing about yet another piece of RISC-V vapourware, how
about you guys propose a patch that would actually make the *current*
HW work to some extent?

	M.

Patch

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index cf74cfa82045..7c64a7ab56f3 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -166,7 +166,7 @@  static void plic_irq_eoi(struct irq_data *d)
 	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
 }
 
-static struct irq_chip plic_chip = {
+static struct irq_chip sifive_plic_chip = {
 	.name		= "SiFive PLIC",
 	.irq_mask	= plic_irq_mask,
 	.irq_unmask	= plic_irq_unmask,
@@ -176,12 +176,43 @@  static struct irq_chip plic_chip = {
 #endif
 };
 
+/*
+ * Current C9xx PLIC can't complete interrupt when the interrupt
+ * source is currently disabled for the target. Re-enable the
+ * interrupt source by unmasking to let hw irq source completion
+ * succeed.
+ */
+static void plic_thead_irq_eoi(struct irq_data *d)
+{
+	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
+
+	if (irqd_irq_masked(d)) {
+		plic_irq_unmask(d);
+		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+		plic_irq_mask(d);
+	} else {
+		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+	}
+}
+
+static struct irq_chip thead_plic_chip = {
+	.name		= "T-Head PLIC",
+	.irq_mask	= plic_irq_mask,
+	.irq_unmask	= plic_irq_unmask,
+	.irq_eoi	= plic_thead_irq_eoi,
+#ifdef CONFIG_SMP
+	.irq_set_affinity = plic_set_affinity,
+#endif
+};
+
+static struct irq_chip *def_plic_chip = &sifive_plic_chip;
+
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
 			      irq_hw_number_t hwirq)
 {
 	struct plic_priv *priv = d->host_data;
 
-	irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
+	irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
 			    handle_fasteoi_irq, NULL, NULL);
 	irq_set_noprobe(irq);
 	irq_set_affinity(irq, &priv->lmask);
@@ -390,5 +421,14 @@  static int __init plic_init(struct device_node *node,
 	return error;
 }
 
+static int __init thead_c900_plic_init(struct device_node *node,
+		struct device_node *parent)
+{
+	def_plic_chip = &thead_plic_chip;
+
+	return plic_init(node, parent);
+}
+
 IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init);
 IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);