From: Guo Ren <guoren@kernel.org> To: Marc Zyngier <maz@kernel.org> Cc: "Anup Patel" <anup@brainfault.org>, "Samuel Holland" <samuel@sholland.org>, "Atish Patra" <atish.patra@wdc.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Heiko Stübner" <heiko@sntech.de>, "Rob Herring" <robh@kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, linux-riscv <linux-riscv@lists.infradead.org>, "Guo Ren" <guoren@linux.alibaba.com> Subject: Re: [PATCH V4 1/3] irqchip/sifive-plic: Add thead,c900-plic support Date: Thu, 21 Oct 2021 17:43:05 +0800 [thread overview] Message-ID: <CAJF2gTSqhFqPf0h-Z_U6OwU8ioPD6RVSzZg9TPoo5nxqfdgK4g@mail.gmail.com> (raw) In-Reply-To: <87v91qbwvr.wl-maz@kernel.org> On Thu, Oct 21, 2021 at 4:33 PM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 21 Oct 2021 03:00:43 +0100, > Guo Ren <guoren@kernel.org> wrote: > > > > On Wed, Oct 20, 2021 at 11:08 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Wed, 20 Oct 2021 15:33:49 +0100, > > > Anup Patel <anup@brainfault.org> wrote: > > > > > > > > On Wed, Oct 20, 2021 at 7:04 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Tue, 19 Oct 2021 14:27:02 +0100, > > > > > Guo Ren <guoren@kernel.org> wrote: > > > > > > > > > > > > On Tue, Oct 19, 2021 at 6:18 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > > > On Tue, 19 Oct 2021 10:33:49 +0100, > > > > > > > Guo Ren <guoren@kernel.org> wrote: > > > > > > > > > > > > > > > > If you have an 'automask' behavior and yet the HW doesn't record this > > > > > > > > > in a separate bit, then you need to track this by yourself in the > > > > > > > > > irq_eoi() callback instead. I guess that you would skip the write to > > > > > > > > > the CLAIM register in this case, though I have no idea whether this > > > > > > > > > breaks > > > > > > > > > the HW interrupt state or not. > > > > > > > > The problem is when enable bit is 0 for that irq_number, > > > > > > > > "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect > > > > > > > > the hw state machine. Then this irq would enter in ack state and no > > > > > > > > continues irqs could come in. > > > > > > > > > > > > > > Really? This means that you cannot mask an interrupt while it is being > > > > > > > handled? How great... > > > > > > If the completion ID does not match an interrupt source that is > > > > > > currently enabled for the target, the completion is silently ignored. > > > > > > So, C9xx completion depends on enable-bit. > > > > > > > > > > Is that what the PLIC spec says? Or what your implementation does? I > > > > > can understand that one implementation would be broken, but if the > > > > > PLIC architecture itself is broken, that's far more concerning. > > > > > > > > Yes, we are dealing with a broken/non-compliant PLIC > > > > implementation. > > > > > > > > The RISC-V PLIC spec defines a very different behaviour for the > > > > interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. > > > > writel(claim)). The T-HEAD PLIC implementation does things > > > > different from what the RISC-V PLIC spec says because it will > > > > mask an interrupt upon interrupt claim whereas PLIC spec says > > > > it should only clear the interrupt pending bit (not mask the interrupt). > > > > > > > > Quoting interrupt claim process (chapter 9) from PLIC spec: > > > > "The PLIC can perform an interrupt claim by reading the claim/complete > > > > register, which returns the ID of the highest priority pending interrupt or > > > > zero if there is no pending interrupt. A successful claim will also atomically > > > > clear the corresponding pending bit on the interrupt source." > > > > > > > > Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc > > > > > > That's not the point I'm making. According to Guo, the PLIC (any > > > implementation of it) will ignore a write to claim on a masked > > > interrupt. > > > > > > If that's indeed correct, then a sequence such as: > > > > > > (1) irq = read(claim) > > > (2) mask from the interrupt handler with the right flags so that it > > > isn't done lazily > > > (3) write(irq, claim) > > > > How about letting the IRQ chip change? > > > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > > index a98bcfc4be7b..ed6ace1058ac 100644 > > --- a/kernel/irq/chip.c > > +++ b/kernel/irq/chip.c > > @@ -444,10 +444,10 @@ void unmask_threaded_irq(struct irq_desc *desc) > > { > > struct irq_chip *chip = desc->irq_data.chip; > > > > + unmask_irq(desc); > > + > > if (chip->flags & IRQCHIP_EOI_THREADED) > > chip->irq_eoi(&desc->irq_data); > > - > > - unmask_irq(desc); > > } > > > > /* > > @@ -673,8 +673,8 @@ static void cond_unmask_eoi_irq(struct irq_desc > > *desc, struct irq_chip *chip) > > */ > > if (!irqd_irq_disabled(&desc->irq_data) && > > irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) { > > - chip->irq_eoi(&desc->irq_data); > > unmask_irq(desc); > > + chip->irq_eoi(&desc->irq_data); > > } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) { > > chip->irq_eoi(&desc->irq_data); > > } > > No, I don't think that's acceptable, and I strongly suspect that other > irqchips have the opposite requirement. You'll have to keep the > workaround in the PLIC code and track the EOI vs unmask to do the > right thing in both callbacks. Okay... > > M. > > -- > Without deviation from the norm, progress is not possible. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
WARNING: multiple messages have this Message-ID (diff)
From: Guo Ren <guoren@kernel.org> To: Marc Zyngier <maz@kernel.org> Cc: "Anup Patel" <anup@brainfault.org>, "Samuel Holland" <samuel@sholland.org>, "Atish Patra" <atish.patra@wdc.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Heiko Stübner" <heiko@sntech.de>, "Rob Herring" <robh@kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, linux-riscv <linux-riscv@lists.infradead.org>, "Guo Ren" <guoren@linux.alibaba.com> Subject: Re: [PATCH V4 1/3] irqchip/sifive-plic: Add thead,c900-plic support Date: Thu, 21 Oct 2021 17:43:05 +0800 [thread overview] Message-ID: <CAJF2gTSqhFqPf0h-Z_U6OwU8ioPD6RVSzZg9TPoo5nxqfdgK4g@mail.gmail.com> (raw) In-Reply-To: <87v91qbwvr.wl-maz@kernel.org> On Thu, Oct 21, 2021 at 4:33 PM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 21 Oct 2021 03:00:43 +0100, > Guo Ren <guoren@kernel.org> wrote: > > > > On Wed, Oct 20, 2021 at 11:08 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Wed, 20 Oct 2021 15:33:49 +0100, > > > Anup Patel <anup@brainfault.org> wrote: > > > > > > > > On Wed, Oct 20, 2021 at 7:04 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Tue, 19 Oct 2021 14:27:02 +0100, > > > > > Guo Ren <guoren@kernel.org> wrote: > > > > > > > > > > > > On Tue, Oct 19, 2021 at 6:18 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > > > On Tue, 19 Oct 2021 10:33:49 +0100, > > > > > > > Guo Ren <guoren@kernel.org> wrote: > > > > > > > > > > > > > > > > If you have an 'automask' behavior and yet the HW doesn't record this > > > > > > > > > in a separate bit, then you need to track this by yourself in the > > > > > > > > > irq_eoi() callback instead. I guess that you would skip the write to > > > > > > > > > the CLAIM register in this case, though I have no idea whether this > > > > > > > > > breaks > > > > > > > > > the HW interrupt state or not. > > > > > > > > The problem is when enable bit is 0 for that irq_number, > > > > > > > > "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect > > > > > > > > the hw state machine. Then this irq would enter in ack state and no > > > > > > > > continues irqs could come in. > > > > > > > > > > > > > > Really? This means that you cannot mask an interrupt while it is being > > > > > > > handled? How great... > > > > > > If the completion ID does not match an interrupt source that is > > > > > > currently enabled for the target, the completion is silently ignored. > > > > > > So, C9xx completion depends on enable-bit. > > > > > > > > > > Is that what the PLIC spec says? Or what your implementation does? I > > > > > can understand that one implementation would be broken, but if the > > > > > PLIC architecture itself is broken, that's far more concerning. > > > > > > > > Yes, we are dealing with a broken/non-compliant PLIC > > > > implementation. > > > > > > > > The RISC-V PLIC spec defines a very different behaviour for the > > > > interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. > > > > writel(claim)). The T-HEAD PLIC implementation does things > > > > different from what the RISC-V PLIC spec says because it will > > > > mask an interrupt upon interrupt claim whereas PLIC spec says > > > > it should only clear the interrupt pending bit (not mask the interrupt). > > > > > > > > Quoting interrupt claim process (chapter 9) from PLIC spec: > > > > "The PLIC can perform an interrupt claim by reading the claim/complete > > > > register, which returns the ID of the highest priority pending interrupt or > > > > zero if there is no pending interrupt. A successful claim will also atomically > > > > clear the corresponding pending bit on the interrupt source." > > > > > > > > Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc > > > > > > That's not the point I'm making. According to Guo, the PLIC (any > > > implementation of it) will ignore a write to claim on a masked > > > interrupt. > > > > > > If that's indeed correct, then a sequence such as: > > > > > > (1) irq = read(claim) > > > (2) mask from the interrupt handler with the right flags so that it > > > isn't done lazily > > > (3) write(irq, claim) > > > > How about letting the IRQ chip change? > > > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > > index a98bcfc4be7b..ed6ace1058ac 100644 > > --- a/kernel/irq/chip.c > > +++ b/kernel/irq/chip.c > > @@ -444,10 +444,10 @@ void unmask_threaded_irq(struct irq_desc *desc) > > { > > struct irq_chip *chip = desc->irq_data.chip; > > > > + unmask_irq(desc); > > + > > if (chip->flags & IRQCHIP_EOI_THREADED) > > chip->irq_eoi(&desc->irq_data); > > - > > - unmask_irq(desc); > > } > > > > /* > > @@ -673,8 +673,8 @@ static void cond_unmask_eoi_irq(struct irq_desc > > *desc, struct irq_chip *chip) > > */ > > if (!irqd_irq_disabled(&desc->irq_data) && > > irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) { > > - chip->irq_eoi(&desc->irq_data); > > unmask_irq(desc); > > + chip->irq_eoi(&desc->irq_data); > > } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) { > > chip->irq_eoi(&desc->irq_data); > > } > > No, I don't think that's acceptable, and I strongly suspect that other > irqchips have the opposite requirement. You'll have to keep the > workaround in the PLIC code and track the EOI vs unmask to do the > right thing in both callbacks. Okay... > > M. > > -- > Without deviation from the norm, progress is not possible. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2021-10-21 9:43 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-16 3:21 [PATCH V4 0/3] irqchip: riscv: Add thead,c900-plic support guoren 2021-10-16 3:21 ` guoren 2021-10-16 3:21 ` [PATCH V4 1/3] irqchip/sifive-plic: " guoren 2021-10-16 3:21 ` guoren 2021-10-18 5:17 ` Samuel Holland 2021-10-18 5:17 ` Samuel Holland 2021-10-18 5:40 ` Anup Patel 2021-10-18 5:40 ` Anup Patel 2021-10-18 7:05 ` Guo Ren 2021-10-18 7:05 ` Guo Ren 2021-10-18 7:21 ` Marc Zyngier 2021-10-18 7:21 ` Marc Zyngier 2021-10-19 9:33 ` Guo Ren 2021-10-19 9:33 ` Guo Ren 2021-10-19 10:18 ` Marc Zyngier 2021-10-19 10:18 ` Marc Zyngier 2021-10-19 13:27 ` Guo Ren 2021-10-19 13:27 ` Guo Ren 2021-10-20 13:34 ` Marc Zyngier 2021-10-20 13:34 ` Marc Zyngier 2021-10-20 14:19 ` Guo Ren 2021-10-20 14:19 ` Guo Ren 2021-10-20 14:59 ` Darius Rad 2021-10-20 14:59 ` Darius Rad 2021-10-20 16:18 ` Anup Patel 2021-10-20 16:18 ` Anup Patel 2021-10-20 18:01 ` Darius Rad 2021-10-20 18:01 ` Darius Rad 2021-10-21 8:47 ` Anup Patel 2021-10-21 8:47 ` Anup Patel 2021-10-20 14:33 ` Anup Patel 2021-10-20 14:33 ` Anup Patel 2021-10-20 15:08 ` Marc Zyngier 2021-10-20 15:08 ` Marc Zyngier 2021-10-20 16:08 ` Anup Patel 2021-10-20 16:08 ` Anup Patel 2021-10-20 16:48 ` Marc Zyngier 2021-10-20 16:48 ` Marc Zyngier 2021-10-21 8:52 ` Anup Patel 2021-10-21 8:52 ` Anup Patel 2021-10-21 1:46 ` Guo Ren 2021-10-21 1:46 ` Guo Ren 2021-10-21 2:00 ` Guo Ren 2021-10-21 2:00 ` Guo Ren 2021-10-21 8:33 ` Marc Zyngier 2021-10-21 8:33 ` Marc Zyngier 2021-10-21 9:43 ` Guo Ren [this message] 2021-10-21 9:43 ` Guo Ren 2021-10-16 3:21 ` [PATCH V4 2/3] dt-bindings: update riscv plic compatible string guoren 2021-10-16 3:21 ` guoren 2021-10-16 7:07 ` Andreas Schwab 2021-10-16 7:07 ` Andreas Schwab 2021-10-16 9:16 ` Guo Ren 2021-10-16 9:16 ` Guo Ren 2021-10-16 10:34 ` Heiko Stuebner 2021-10-16 10:34 ` Heiko Stuebner 2021-10-16 12:56 ` Guo Ren 2021-10-16 12:56 ` Guo Ren 2021-10-16 16:31 ` Heiko Stuebner 2021-10-16 16:31 ` Heiko Stuebner 2021-10-20 12:15 ` Guo Ren 2021-10-20 12:15 ` Guo Ren 2021-10-18 12:02 ` Rob Herring 2021-10-18 12:02 ` Rob Herring 2021-10-19 0:55 ` Guo Ren 2021-10-19 0:55 ` Guo Ren 2021-10-16 3:22 ` [PATCH V4 3/3] dt-bindings: vendor-prefixes: add T-Head Semiconductor guoren 2021-10-16 3:22 ` guoren
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAJF2gTSqhFqPf0h-Z_U6OwU8ioPD6RVSzZg9TPoo5nxqfdgK4g@mail.gmail.com \ --to=guoren@kernel.org \ --cc=anup@brainfault.org \ --cc=atish.patra@wdc.com \ --cc=guoren@linux.alibaba.com \ --cc=heiko@sntech.de \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=maz@kernel.org \ --cc=palmer@dabbelt.com \ --cc=robh@kernel.org \ --cc=samuel@sholland.org \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.