Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
From: Guo Ren
Date: Fri Oct 23 2020 - 23:10:06 EST
On Fri, Oct 23, 2020 at 8:31 PM Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
>
> On Fri, Oct 23, 2020 at 3:48 PM <guoren@xxxxxxxxxx> wrote:
> >
> > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >
> > 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 <guoren@xxxxxxxxxxxxxxxxx>
> > ---
> > 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/