| Message ID | 20220301005142.3038702-2-Niklas.Cassel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sifive-plic minor improvements | expand |
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. 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
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
On Tue, Mar 1, 2022 at 2:36 PM Niklas Cassel <Niklas.Cassel@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@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 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@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
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 /* + * 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++)