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

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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@wdc.com> wrote:
> >
> > From: Niklas Cassel <niklas.cassel@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@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

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


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


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 ;)


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?


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@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv