Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver

From: Marc Zyngier
Date: Sun May 31 2020 - 06:53:22 EST


On 2020-05-31 11:06, Anup Patel wrote:
On Sun, May 31, 2020 at 3:03 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:

On 2020-05-31 06:36, Anup Patel wrote:
> On Sat, May 30, 2020 at 5:31 PM Marc Zyngier <maz@xxxxxxxxxx> 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...