[PATCH 1/2] irqchip/sifive-plic: Improve naming scheme for per context offsets

Anup Patel apatel at ventanamicro.com
Tue Mar 1 03:10:19 PST 2022 

On Tue, Mar 1, 2022 at 2:36 PM Niklas Cassel <Niklas.Cassel at wdc.com> wrote:
>
> On Tue, Mar 01, 2022 at 09:42:46AM +0530, Anup Patel wrote:
> > On Tue, Mar 1, 2022 at 6:22 AM Niklas Cassel <Niklas.Cassel at wdc.com> wrote:
> > >
> > > From: Niklas Cassel <niklas.cassel at wdc.com>
> > >
> > > A hart context is a given privilege mode on a given hart.
> > > The PLIC supports a fixed number of hart contexts (15872).
> > > Each hart context has fixed register offsets in PLIC.
> > >
> > > The number of hart contexts for each hart depends on the privilege modes
> > > supported by each hart. Therefore, this mapping between hart context to
> > > hart id is platform specific, and is currently supplied via device tree.
> > >
> > > For example, canaan,k210 has the following mapping:
> > > Context0: hart0 M-mode
> > > Context1: hart0 S-mode
> > > Context2: hart1 M-mode
> > > Context3: hart1 S-mode
> > >
> > > While sifive,fu540 has the following mapping:
> > > Context0: hart0 M-mode
> > > Context1: hart1 M-mode
> > > Context2: hart1 S-mode
> > >
> > > Because the number of hart contexts per hart is not fixed, the names
> > > ENABLE_PER_HART and CONTEXT_PER_HART for the register offsets are quite
> > > confusing and might mislead the reader to think that these are fixed
> > > register offsets per hart.
> > >
> > > Rename the offsets to more clearly highlight that they are per hart
> > > _context_ and not per hart.
> > >
> > > Signed-off-by: Niklas Cassel <niklas.cassel at wdc.com>
> > > ---
> > >  drivers/irqchip/irq-sifive-plic.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > index 09cc98266d30..211bcb10aa93 100644
> > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > @@ -41,19 +41,21 @@
> > >  #define     PRIORITY_PER_ID            4
> > >
> > >  /*
> > > + * A hart context is a given privilege mode on a given hart.
> > >   * Each hart context has a vector of interrupt enable bits associated with it.
> > >   * There's one bit for each interrupt source.
> > >   */
> > >  #define ENABLE_BASE                    0x2000
> > > -#define     ENABLE_PER_HART            0x80
> > > +#define     ENABLE_PER_HART_CTX                0x80
> >
> > These are enable registers for each plic-context and we have multiple
> > plic-context associated with each HART.
> > (Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc)
> >
> > Correct name would be ENABLE_PER_CONTEXT instead of
> > ENABLE_PER_HART_CTX.
>
> Hello Anup,
>
> If you look at the RISC-V Privileged Spec v1.11-draft:
> https://github.com/riscv/riscv-isa-manual/releases/download/draft-20181201-5449851/riscv-privileged.pdf

Clearly, you are referring to a very old draft version of the specification.

>
> They do use the wording "hart context" all the time.
>
> See e.g. 7.3 Interrupt Targets and Hart Contexts
>
> "Interrupt targets are usually hart contexts, where a hart context is a given privilege mode on a
> given hart (though there are other possible interrupt targets, such as DMA engines). Not all hart
> contexts need be interrupt targets, in particular, if a processor core does not support delegating
> external interrupts to lower-privilege modes, then the lower-privilege hart contexts will not be
> interrupt targets."

The latest ratified RISC-V Privileged Spec v1.12 has no such section.

>
>
> Also see the DT binding, which also uses the term hart context:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml?h=v5.17-rc6

I am referring to the RISC-V PLIC spec which simply names these
registers as "interrupt enable" registers.

>
>
> And the existing comments in the driver also uses hart context:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-sifive-plic.c?h=v5.17-rc6#n44
>
>
> I'm not sure why:
> https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc
> Seems to have done search/replace on "hart context" with "context"
> in all places except one. Almost looks like some kind of revisionism ;)

The PLIC context output connects to the external interrupt of a specific
privilege mode in HART. If a HART supports both M and S modes then
we will have separate PLIC contexts for HART M-mode external interrupt
and HART S-mode external interrupt.

Clearly, the term "context" is defined by PLIC spec and not the RISC-V
privilege spec.

We also have a new RISC-V AIA specification which has totally different
terminology.

>
>
> I would say that "hart context" seems to be slightly more correct,
> and it is already used in many places, dt binding, comments, etc.
>
>
> However, if you feel that I should search/replace "hart context" with
> "context" inside the PLIC driver, to better match the current github spec,
> I can do that. It is only used in two places.
> (I don't think we should touch the DT binding though. It defines "hart
> context", then uses "context".)
>
>
> If we use ENABLE_PER_CONTEXT, like you suggest, do you have a better
> suggestion for CONTEXT_PER_HART_CTX as well?
> I don't think we can keep the CONTEXT_ prefix.
> And in that case, we probably shouldn't keep the ENABLE_ prefix either.
>
> How about:
> PER_CONTEXT_ENABLE_OFFSET and PER_CONTEXT_CTRL_OFFSET?

We just need three renaming:
1) Rename ENABLE_BASE to CONTEXT_ENABLE_BASE
2) Rename ENABLE_PER_HART to CONTEXT_ENABLE_SIZE
3) Rename CONTEXT_PER_HART to CONTEXT_SIZE

Regards,
Anup

>
>
> Kind regards,
> Niklas
>
>
> >
> > Regards,
> > Anup
> >
> > >
> > >  /*
> > > + * A hart context is a given privilege mode on a given hart.
> > >   * Each hart context has a set of control registers associated with it.  Right
> > >   * now there's only two: a source priority threshold over which the hart will
> > >   * take an interrupt, and a register to claim interrupts.
> > >   */
> > >  #define CONTEXT_BASE                   0x200000
> > > -#define     CONTEXT_PER_HART           0x1000
> > > +#define     CONTEXT_PER_HART_CTX       0x1000
> > >  #define     CONTEXT_THRESHOLD          0x00
> > >  #define     CONTEXT_CLAIM              0x04
> > >
> > > @@ -362,10 +364,10 @@ static int __init plic_init(struct device_node *node,
> > >                 cpumask_set_cpu(cpu, &priv->lmask);
> > >                 handler->present = true;
> > >                 handler->hart_base =
> > > -                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART;
> > > +                       priv->regs + CONTEXT_BASE + i * CONTEXT_PER_HART_CTX;
> > >                 raw_spin_lock_init(&handler->enable_lock);
> > >                 handler->enable_base =
> > > -                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART;
> > > +                       priv->regs + ENABLE_BASE + i * ENABLE_PER_HART_CTX;
> > >                 handler->priv = priv;
> > >  done:
> > >                 for (hwirq = 1; hwirq <= nr_irqs; hwirq++)
> > > --
> > > 2.35.1
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

More information about the linux-riscv mailing list