Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
Hi Anup,
On Sun, Oct 25, 2020 at 5:18 PM Anup Patel wrote:
>
> On Sat, Oct 24, 2020 at 8:40 AM Guo Ren wrote:
> >
> > On Fri, Oct 23, 2020 at 8:31 PM Anup Patel wrote:
> > >
> > > On Fri, Oct 23, 2020 at 3:48 PM wrote:
> > > >
> > > > From: Guo Ren
> > > >
> > > > 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
> > > > from dts is not equal to the sequence index.
> > > >
> > > > Signed-off-by: Guo Ren
> > > > ---
> > > > 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_BASE0x2000
> > > > -#define ENABLE_PER_HART0x80
> > > > +#define ENABLE_BASE0x2080
> > > > +#define ENABLE_PER_HART0x100
> > > >
> > > > /*
> > > > * 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 0x20
> > > > -#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, >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(>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'.
>
> There is no relation between PLIC context number and HART IDs. The
> PLIC context to HART mapping is discovered from the "interrupts-extended"
> DT property of PLIC DT node. The "i" in the loop is PLIC context number.
>
> The PLIC spec does not mandate any ordering/pattern of PLIC context to
> HART mappings. Also, the interrupts-extended DT property is generic
> enough to represent any kind of PLIC context to HART mappings.
>
> Your patch breaks SiFive Unleashed board and NoMMU kernel because
> of incorrect assumptions about PLIC contexts to HART mappings.
>
> >
> > For example, how we deal with below dts configuration:
> > cpus {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > timebase-frequency = <300>;
> > 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 {
Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
On Sat, Oct 24, 2020 at 8:40 AM Guo Ren wrote:
>
> On Fri, Oct 23, 2020 at 8:31 PM Anup Patel wrote:
> >
> > On Fri, Oct 23, 2020 at 3:48 PM wrote:
> > >
> > > From: Guo Ren
> > >
> > > 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
> > > from dts is not equal to the sequence index.
> > >
> > > Signed-off-by: Guo Ren
> > > ---
> > > 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_BASE0x2000
> > > -#define ENABLE_PER_HART0x80
> > > +#define ENABLE_BASE0x2080
> > > +#define ENABLE_PER_HART0x100
> > >
> > > /*
> > > * 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 0x20
> > > -#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, >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(>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'.
There is no relation between PLIC context number and HART IDs. The
PLIC context to HART mapping is discovered from the "interrupts-extended"
DT property of PLIC DT node. The "i" in the loop is PLIC context number.
The PLIC spec does not mandate any ordering/pattern of PLIC context to
HART mappings. Also, the interrupts-extended DT property is generic
enough to represent any kind of PLIC context to HART mappings.
Your patch breaks SiFive Unleashed board and NoMMU kernel because
of incorrect assumptions about PLIC contexts to HART mappings.
>
> For example, how we deal with below dts configuration:
> cpus {
> #address-cells = <1>;
> #size-cells = <0>;
> timebase-frequency = <300>;
> 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";
>
Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
On Fri, Oct 23, 2020 at 8:31 PM Anup Patel wrote:
>
> On Fri, Oct 23, 2020 at 3:48 PM wrote:
> >
> > From: Guo Ren
> >
> > 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
> > from dts is not equal to the sequence index.
> >
> > Signed-off-by: Guo Ren
> > ---
> > 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_BASE0x2000
> > -#define ENABLE_PER_HART0x80
> > +#define ENABLE_BASE0x2080
> > +#define ENABLE_PER_HART0x100
> >
> > /*
> > * 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 0x20
> > -#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, >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(>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 = <300>;
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
Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
On Fri, Oct 23, 2020 at 3:48 PM wrote: > > From: Guo Ren > > 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 > from dts is not equal to the sequence index. > > Signed-off-by: Guo Ren > --- > 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_BASE0x2000 > -#define ENABLE_PER_HART0x80 > +#define ENABLE_BASE0x2080 > +#define ENABLE_PER_HART0x100 > > /* > * 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 0x20 > -#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, >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(>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. 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. 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. Regards, Anup
[PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
From: Guo Ren 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 from dts is not equal to the sequence index. Signed-off-by: Guo Ren --- 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_BASE0x2000 -#define ENABLE_PER_HART0x80 +#define ENABLE_BASE0x2080 +#define ENABLE_PER_HART0x100 /* * 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 0x20 -#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, >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(>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
