On Fri, Oct 23, 2020 at 8:31 PM Anup Patel <a...@brainfault.org> wrote: > > On Fri, Oct 23, 2020 at 3:48 PM <guo...@kernel.org> wrote: > > > > From: Guo Ren <guo...@linux.alibaba.com> > > > > ENABLE and CONTEXT registers contain M & S status for per-hart, so > > ref to the specification the correct definition is double to the > > current value. > > > > The value of hart_base and enable_base should be calculated by real > > physical hartid not software id. Sometimes the CPU node's <reg> > > from dts is not equal to the sequence index. > > > > Signed-off-by: Guo Ren <guo...@linux.alibaba.com> > > --- > > drivers/irqchip/irq-sifive-plic.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/irqchip/irq-sifive-plic.c > > b/drivers/irqchip/irq-sifive-plic.c > > index eaa3e9f..2e56576 100644 > > --- a/drivers/irqchip/irq-sifive-plic.c > > +++ b/drivers/irqchip/irq-sifive-plic.c > > @@ -44,16 +44,16 @@ > > * 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_BASE 0x2080 > > +#define ENABLE_PER_HART 0x100 > > > > /* > > * 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_BASE 0x201000 > > +#define CONTEXT_PER_HART 0x2000 > > #define CONTEXT_THRESHOLD 0x00 > > #define CONTEXT_CLAIM 0x04 > > > > @@ -358,10 +358,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 + hartid * > > CONTEXT_PER_HART; > > raw_spin_lock_init(&handler->enable_lock); > > handler->enable_base = > > - priv->regs + ENABLE_BASE + i * ENABLE_PER_HART; > > + priv->regs + ENABLE_BASE + hartid * ENABLE_PER_HART; > > handler->priv = priv; > > done: > > for (hwirq = 1; hwirq <= nr_irqs; hwirq++) > > -- > > 2.7.4 > > > > There is no one-to-one mapping between PLIC context and HARTID. Instead, > we have many-to-one mapping between PLIC contexts and HARTID. In other > words, we have one PLIC context for each interrupt capable mode (i.e. > M/S-mode) of each HART. > > For example, on SiFive Unleashed we have 5 HARTs but HARTID=0 has > only M-mode capable of taking interrupts so we have total (1 + 2x4) = 9 > PLIC contexts. That's OK, but what the bug I want to point out is enable_base & context_base should be calculated by 'hartid' not 'i'.
For example, how we deal with below dts configuration: cpus { #address-cells = <1>; #size-cells = <0>; timebase-frequency = <3000000>; cpu@0 { device_type = "cpu"; reg = <2>; //********* different from index status = "okay"; compatible = "riscv"; riscv,isa = "rv64imafdcsu"; mmu-type = "riscv,sv39"; cpu0_intc: interrupt-controller { #interrupt-cells = <1>; compatible = "riscv,cpu-intc"; interrupt-controller; }; }; cpu@1 { device_type = "cpu"; reg = <3>; //********* different from index status = "fail"; compatible = "riscv"; riscv,isa = "rv64imafdcsu"; mmu-type = "riscv,sv39"; cpu1_intc: interrupt-controller { #interrupt-cells = <1>; compatible = "riscv,cpu-intc"; interrupt-controller; }; }; } > > I would also like to highlight that this patch is forcing PLIC driver to > always > use PLIC S-mode context for each HART which breaks the Linux RISC-V > NoMMU kernel. Yes, I forgot M-mode and I will correct it. > > There is no issue with the existing defines because these are aligned with > above and latest PLIC spec. > (Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc) > > NACK to this patch from my side. Here is my new patch which fixup m-mode linux: diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index 4048657..e34e1d9 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -45,7 +45,13 @@ * There's one bit for each interrupt source. */ #define ENABLE_BASE 0x2000 -#define ENABLE_PER_HART 0x80 +#define ENABLE_PER_HART 0x100 +#ifdef CONFIG_RISCV_M_MODE +#define ENABLE_OFFSET 0 +#else +#define ENABLE_OFFSET 0x80 +#endif + /* * Each hart context has a set of control registers associated with it. Right @@ -53,9 +59,14 @@ * take an interrupt, and a register to claim interrupts. */ #define CONTEXT_BASE 0x200000 -#define CONTEXT_PER_HART 0x1000 +#define CONTEXT_PER_HART 0x2000 #define CONTEXT_THRESHOLD 0x00 #define CONTEXT_CLAIM 0x04 +#ifdef CONFIG_RISCV_M_MODE +#define CONTEXT_OFFSET 0 +#else +#define CONTEXT_OFFSET 0x1000 +#endif #define PLIC_DISABLE_THRESHOLD 0x7 #define PLIC_ENABLE_THRESHOLD 0 @@ -358,10 +369,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 + hartid * CONTEXT_PER_HART + CONTEXT_OFFSET; raw_spin_lock_init(&handler->enable_lock); handler->enable_base = - priv->regs + ENABLE_BASE + i * ENABLE_PER_HART; + priv->regs + ENABLE_BASE + hartid * ENABLE_PER_HART + ENABLE_OFFSET; handler->priv = priv; done: for (hwirq = 1; hwirq <= nr_irqs; hwirq++) -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/