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

[Marc Zyngier]

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