From: Marc Zyngier <maz@kernel.org>
To: Anup Patel <anup@brainfault.org>
Cc: Anup Patel <anup.patel@wdc.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Paul Walmsley <paul.walmsley@sifive.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Thomas Gleixner <tglx@linutronix.de>,
Jason Cooper <jason@lakedaemon.net>,
Atish Patra <atish.patra@wdc.com>,
Alistair Francis <Alistair.Francis@wdc.com>,
linux-riscv <linux-riscv@lists.infradead.org>,
"linux-kernel@vger.kernel.org List"
<linux-kernel@vger.kernel.org>,
Palmer Dabbelt <palmerdabbelt@google.com>
Subject: Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver
Date: Sun, 31 May 2020 11:53:17 +0100 [thread overview]
Message-ID: <e315f76b06b7b0935ebee867c04f364e@kernel.org> (raw)
In-Reply-To: <CAAhSdy2fJ1cd2OjAWODOmSbkWUBfvvr4rvsTqh4qNxZjTTKo5A@mail.gmail.com>
On 2020-05-31 11:06, Anup Patel wrote:
> On Sun, May 31, 2020 at 3:03 PM Marc Zyngier <maz@kernel.org> wrote:
>>
>> On 2020-05-31 06:36, Anup Patel wrote:
>> > On Sat, May 30, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote:
>>
>> [...]
>>
>> >> > plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD);
>> >>
>> >> Why do you need to both disable the interrupt *and* change the
>> >> priority
>> >> threshold? It seems to be that one of them should be enough, but my
>> >> kno9wledge of the PLIC is limited. In any case, this would deserve a
>> >> comment.
>> >
>> > Okay, I will test and remove "disable the interrupt" part from
>> > plic_dying_cpu().
>>
>> Be careful, as interrupt enabling/disabling is refcounted in order
>> to allow nesting. If you only enable on CPU_ON and not disable
>> on CPU_OFF, you will end-up with a depth that only increases,
>> up to the point where you hit the roof (it will take a while though).
>>
>> I would keep the enable/disable as is, and drop the priority
>> setting from the CPU_OFF path.
>
> The PLIC threshold is like GICv2 CPU interface enable/disable.
Looking at the documentation[1], that's not really a correct analogy:
- The PLIC is far removed from the CPU, and is more akin a GICv3
distributor. The INTC itself is more like a GICv3 redistributor,
as it deals with local interrupts only. I don't see anything
in the RISC-V architecture that actually behaves like a GIC
CPU interface (not necessarily a bad thing...).
- The threshold register is not an ON/OFF, but a priority mask,
similar to the GIC PMR (except that the PMR lives in the CPU
interface and affects all interrupts targetting this CPU while
this only seem to affect PLIC interrupts and not the INTC interrupts).
You may be using it as an ON/OFF for now as you don't support
multiple priorities yet, but that's just a SW thing.
> Based on your comment, we should only program the PLIC threshold
> in CPU_ON and don't touch the PLIC threshold in CPU_OFF. Right??
This seems like the correct thing to do.
M.
[1]
https://sifive.cdn.prismic.io/sifive%2Fdc4980ff-17db-448b-b521-4c7ab26b7488_sifive+u54-mc+manual+v19.08.pdf
--
Jazz is not dead. It just smells funny...
next prev parent reply other threads:[~2020-05-31 10:53 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-30 10:07 [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver Anup Patel
2020-05-30 10:07 ` [PATCH v6 1/6] RISC-V: self-contained IPI handling routine Anup Patel
2020-05-30 10:07 ` [PATCH v6 2/6] RISC-V: Rename and move plic_find_hart_id() to arch directory Anup Patel
2020-05-30 10:07 ` [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver Anup Patel
2020-05-30 12:01 ` Marc Zyngier
2020-05-31 5:36 ` Anup Patel
2020-05-31 9:33 ` Marc Zyngier
2020-05-31 10:06 ` Anup Patel
2020-05-31 10:53 ` Marc Zyngier [this message]
2020-06-01 4:09 ` Anup Patel
2020-06-01 7:41 ` Marc Zyngier
2020-06-01 9:13 ` Anup Patel
2020-06-01 9:33 ` Guo Ren
2020-05-30 10:07 ` [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt Anup Patel
2020-05-30 11:41 ` Marc Zyngier
2020-05-31 5:52 ` Anup Patel
2020-05-30 10:07 ` [PATCH v6 5/6] RISC-V: Remove do_IRQ() function Anup Patel
2020-05-30 10:07 ` [PATCH v6 6/6] RISC-V: Force select RISCV_INTC for CONFIG_RISCV Anup Patel
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=e315f76b06b7b0935ebee867c04f364e@kernel.org \
--to=maz@kernel.org \
--cc=Alistair.Francis@wdc.com \
--cc=anup.patel@wdc.com \
--cc=anup@brainfault.org \
--cc=aou@eecs.berkeley.edu \
--cc=atish.patra@wdc.com \
--cc=daniel.lezcano@linaro.org \
--cc=jason@lakedaemon.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=palmerdabbelt@google.com \
--cc=paul.walmsley@sifive.com \
--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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).