* [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
@ 2020-10-23 10:17 guoren
2020-10-23 10:17 ` [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs guoren
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: guoren @ 2020-10-23 10:17 UTC (permalink / raw)
To: palmerdabbelt, paul.walmsley, anup, greentime.hu, zong.li,
atish.patra, tglx, jason, maz, wesley, yash.shah, hch
Cc: linux-riscv, Guo Ren, guoren, linux-kernel
From: Guo Ren <guoren@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 <guoren@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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs
2020-10-23 10:17 [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base guoren
@ 2020-10-23 10:17 ` guoren
2020-10-23 11:27 ` Anup Patel
2020-10-23 10:17 ` [PATCH 3/3] irqchip/irq-sifive-plic: Fixup set_affinity enable irq unexpected guoren
2020-10-23 11:42 ` [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base Anup Patel
2 siblings, 1 reply; 10+ messages in thread
From: guoren @ 2020-10-23 10:17 UTC (permalink / raw)
To: palmerdabbelt, paul.walmsley, anup, greentime.hu, zong.li,
atish.patra, tglx, jason, maz, wesley, yash.shah, hch
Cc: linux-riscv, Guo Ren, guoren, linux-kernel
From: Guo Ren <guoren@linux.alibaba.com>
If "echo 3 > /proc/irq/1/smp_affinity", we want irq 1 could be
broadcast to CPU0 & CPU1 and one of them would pick up the irq
handler.
But current implementation couldn't let multi CPUs process the
same IRQ concurrent.
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
---
drivers/irqchip/irq-sifive-plic.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 2e56576..0003322 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -114,15 +114,12 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
static void plic_irq_unmask(struct irq_data *d)
{
struct cpumask amask;
- unsigned int cpu;
struct plic_priv *priv = irq_get_chip_data(d->irq);
cpumask_and(&amask, &priv->lmask, cpu_online_mask);
- cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
- &amask);
- if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
- return;
- plic_irq_toggle(cpumask_of(cpu), d, 1);
+ cpumask_and(&amask, &amask, irq_data_get_affinity_mask(d));
+
+ plic_irq_toggle(&amask, d, 1);
}
static void plic_irq_mask(struct irq_data *d)
@@ -136,24 +133,16 @@ static void plic_irq_mask(struct irq_data *d)
static int plic_set_affinity(struct irq_data *d,
const struct cpumask *mask_val, bool force)
{
- unsigned int cpu;
struct cpumask amask;
struct plic_priv *priv = irq_get_chip_data(d->irq);
cpumask_and(&amask, &priv->lmask, mask_val);
+ cpumask_and(&amask, &amask, cpu_online_mask);
- if (force)
- cpu = cpumask_first(&amask);
- else
- cpu = cpumask_any_and(&amask, cpu_online_mask);
-
- if (cpu >= nr_cpu_ids)
- return -EINVAL;
+ irq_data_update_effective_affinity(d, &amask);
plic_irq_toggle(&priv->lmask, d, 0);
- plic_irq_toggle(cpumask_of(cpu), d, 1);
-
- irq_data_update_effective_affinity(d, cpumask_of(cpu));
+ plic_irq_toggle(&amask, d, 1);
return IRQ_SET_MASK_OK_DONE;
}
--
2.7.4
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] irqchip/irq-sifive-plic: Fixup set_affinity enable irq unexpected
2020-10-23 10:17 [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base guoren
2020-10-23 10:17 ` [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs guoren
@ 2020-10-23 10:17 ` guoren
2020-10-23 11:33 ` Anup Patel
2020-10-23 11:42 ` [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base Anup Patel
2 siblings, 1 reply; 10+ messages in thread
From: guoren @ 2020-10-23 10:17 UTC (permalink / raw)
To: palmerdabbelt, paul.walmsley, anup, greentime.hu, zong.li,
atish.patra, tglx, jason, maz, wesley, yash.shah, hch
Cc: linux-riscv, Guo Ren, guoren, linux-kernel
From: Guo Ren <guoren@linux.alibaba.com>
For PLIC, we only have enable registers to control per hart's irq
affinity and irq_set_affinity would call plic_irq_toggle to enable
the IRQ's routing. So we can't enable irq in irq_domain_map before
request_irq, it'll let uninitialized devices' irq exception.
The solution is to check the irq has been enabled, just like what
irq-gic-v3 has done in gic_set_affinity.
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
---
drivers/irqchip/irq-sifive-plic.c | 45 ++++++++++++++++++++++++++++++++++++---
1 file changed, 42 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 0003322..1a63859 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -130,6 +130,36 @@ static void plic_irq_mask(struct irq_data *d)
}
#ifdef CONFIG_SMP
+static inline bool plic_toggle_is_enabled(struct plic_handler *handler,
+ int hwirq)
+{
+ u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
+ u32 hwirq_mask = 1 << (hwirq % 32);
+
+ if (readl(reg) & hwirq_mask)
+ return true;
+ else
+ return false;
+}
+
+static inline bool plic_irq_is_enabled(const struct cpumask *mask,
+ struct irq_data *d)
+{
+ int cpu;
+
+ for_each_cpu(cpu, mask) {
+ struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
+
+ if (!handler->present)
+ continue;
+
+ if (plic_toggle_is_enabled(handler, d->hwirq))
+ return true;
+ }
+
+ return false;
+}
+
static int plic_set_affinity(struct irq_data *d,
const struct cpumask *mask_val, bool force)
{
@@ -141,8 +171,10 @@ static int plic_set_affinity(struct irq_data *d,
irq_data_update_effective_affinity(d, &amask);
- plic_irq_toggle(&priv->lmask, d, 0);
- plic_irq_toggle(&amask, d, 1);
+ if (plic_irq_is_enabled(&priv->lmask, d)) {
+ plic_irq_toggle(&priv->lmask, d, 0);
+ plic_irq_toggle(&amask, d, 1);
+ }
return IRQ_SET_MASK_OK_DONE;
}
@@ -168,12 +200,19 @@ static struct irq_chip plic_chip = {
static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hwirq)
{
+ unsigned int cpu;
struct plic_priv *priv = d->host_data;
irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
irq_set_noprobe(irq);
- irq_set_affinity(irq, &priv->lmask);
+
+ cpu = cpumask_any_and(&priv->lmask, cpu_online_mask);
+ if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
+ return -EINVAL;
+
+ irq_set_affinity(irq, cpumask_of(cpu));
+
return 0;
}
--
2.7.4
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs
2020-10-23 10:17 ` [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs guoren
@ 2020-10-23 11:27 ` Anup Patel
2020-10-24 3:38 ` Guo Ren
0 siblings, 1 reply; 10+ messages in thread
From: Anup Patel @ 2020-10-23 11:27 UTC (permalink / raw)
To: Guo Ren
Cc: Guo Ren, Jason Cooper, wesley, Marc Zyngier, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Atish Patra,
Yash Shah, Zong Li, Paul Walmsley, Greentime Hu, Thomas Gleixner,
Christoph Hellwig
On Fri, Oct 23, 2020 at 3:48 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> If "echo 3 > /proc/irq/1/smp_affinity", we want irq 1 could be
> broadcast to CPU0 & CPU1 and one of them would pick up the irq
> handler.
>
> But current implementation couldn't let multi CPUs process the
> same IRQ concurrent.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> ---
> drivers/irqchip/irq-sifive-plic.c | 23 ++++++-----------------
> 1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 2e56576..0003322 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -114,15 +114,12 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
> static void plic_irq_unmask(struct irq_data *d)
> {
> struct cpumask amask;
> - unsigned int cpu;
> struct plic_priv *priv = irq_get_chip_data(d->irq);
>
> cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> - cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> - &amask);
> - if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> - return;
> - plic_irq_toggle(cpumask_of(cpu), d, 1);
> + cpumask_and(&amask, &amask, irq_data_get_affinity_mask(d));
> +
> + plic_irq_toggle(&amask, d, 1);
> }
>
> static void plic_irq_mask(struct irq_data *d)
> @@ -136,24 +133,16 @@ static void plic_irq_mask(struct irq_data *d)
> static int plic_set_affinity(struct irq_data *d,
> const struct cpumask *mask_val, bool force)
> {
> - unsigned int cpu;
> struct cpumask amask;
> struct plic_priv *priv = irq_get_chip_data(d->irq);
>
> cpumask_and(&amask, &priv->lmask, mask_val);
> + cpumask_and(&amask, &amask, cpu_online_mask);
>
> - if (force)
> - cpu = cpumask_first(&amask);
> - else
> - cpu = cpumask_any_and(&amask, cpu_online_mask);
> -
> - if (cpu >= nr_cpu_ids)
> - return -EINVAL;
> + irq_data_update_effective_affinity(d, &amask);
>
> plic_irq_toggle(&priv->lmask, d, 0);
> - plic_irq_toggle(cpumask_of(cpu), d, 1);
> -
> - irq_data_update_effective_affinity(d, cpumask_of(cpu));
> + plic_irq_toggle(&amask, d, 1);
>
> return IRQ_SET_MASK_OK_DONE;
> }
> --
> 2.7.4
>
In the past, a similar patch was proposed by Zong Li (SiFive). We
have good reasons to not allow multiple CPUs handle the same IRQ.
Refer, https://lkml.org/lkml/2020/4/26/201
NACK from my side.
Regards,
Anup
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] irqchip/irq-sifive-plic: Fixup set_affinity enable irq unexpected
2020-10-23 10:17 ` [PATCH 3/3] irqchip/irq-sifive-plic: Fixup set_affinity enable irq unexpected guoren
@ 2020-10-23 11:33 ` Anup Patel
0 siblings, 0 replies; 10+ messages in thread
From: Anup Patel @ 2020-10-23 11:33 UTC (permalink / raw)
To: Guo Ren
Cc: Guo Ren, Jason Cooper, wesley, Marc Zyngier, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Atish Patra,
Yash Shah, Zong Li, Paul Walmsley, Greentime Hu, Thomas Gleixner,
Christoph Hellwig
On Fri, Oct 23, 2020 at 3:48 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> For PLIC, we only have enable registers to control per hart's irq
> affinity and irq_set_affinity would call plic_irq_toggle to enable
> the IRQ's routing. So we can't enable irq in irq_domain_map before
> request_irq, it'll let uninitialized devices' irq exception.
>
> The solution is to check the irq has been enabled, just like what
> irq-gic-v3 has done in gic_set_affinity.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> ---
> drivers/irqchip/irq-sifive-plic.c | 45 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 0003322..1a63859 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -130,6 +130,36 @@ static void plic_irq_mask(struct irq_data *d)
> }
>
> #ifdef CONFIG_SMP
> +static inline bool plic_toggle_is_enabled(struct plic_handler *handler,
> + int hwirq)
> +{
> + u32 __iomem *reg = handler->enable_base + (hwirq / 32) * sizeof(u32);
> + u32 hwirq_mask = 1 << (hwirq % 32);
> +
> + if (readl(reg) & hwirq_mask)
> + return true;
> + else
> + return false;
> +}
> +
> +static inline bool plic_irq_is_enabled(const struct cpumask *mask,
> + struct irq_data *d)
> +{
> + int cpu;
> +
> + for_each_cpu(cpu, mask) {
> + struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
> +
> + if (!handler->present)
> + continue;
> +
> + if (plic_toggle_is_enabled(handler, d->hwirq))
> + return true;
> + }
> +
> + return false;
> +}
> +
> static int plic_set_affinity(struct irq_data *d,
> const struct cpumask *mask_val, bool force)
> {
> @@ -141,8 +171,10 @@ static int plic_set_affinity(struct irq_data *d,
>
> irq_data_update_effective_affinity(d, &amask);
>
> - plic_irq_toggle(&priv->lmask, d, 0);
> - plic_irq_toggle(&amask, d, 1);
> + if (plic_irq_is_enabled(&priv->lmask, d)) {
> + plic_irq_toggle(&priv->lmask, d, 0);
> + plic_irq_toggle(&amask, d, 1);
> + }
>
> return IRQ_SET_MASK_OK_DONE;
> }
> @@ -168,12 +200,19 @@ static struct irq_chip plic_chip = {
> static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> irq_hw_number_t hwirq)
> {
> + unsigned int cpu;
> struct plic_priv *priv = d->host_data;
>
> irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> handle_fasteoi_irq, NULL, NULL);
> irq_set_noprobe(irq);
> - irq_set_affinity(irq, &priv->lmask);
> +
> + cpu = cpumask_any_and(&priv->lmask, cpu_online_mask);
> + if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> + return -EINVAL;
> +
> + irq_set_affinity(irq, cpumask_of(cpu));
> +
> return 0;
> }
>
> --
> 2.7.4
>
Greentime (SiFive) has already proposed a patch to fix this.
Refer, https://lkml.org/lkml/2020/10/20/188
Only the plic_irqdomain_map() change which sets the correct
default affinity looks good to me.
Regards,
Anup
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
2020-10-23 10:17 [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base guoren
2020-10-23 10:17 ` [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs guoren
2020-10-23 10:17 ` [PATCH 3/3] irqchip/irq-sifive-plic: Fixup set_affinity enable irq unexpected guoren
@ 2020-10-23 11:42 ` Anup Patel
2020-10-24 3:09 ` Guo Ren
2 siblings, 1 reply; 10+ messages in thread
From: Anup Patel @ 2020-10-23 11:42 UTC (permalink / raw)
To: Guo Ren
Cc: Guo Ren, Jason Cooper, wesley, Marc Zyngier, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Atish Patra,
Yash Shah, Zong Li, Paul Walmsley, Greentime Hu, Thomas Gleixner,
Christoph Hellwig
On Fri, Oct 23, 2020 at 3:48 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@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 <guoren@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.
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
2020-10-23 11:42 ` [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base Anup Patel
@ 2020-10-24 3:09 ` Guo Ren
2020-10-25 9:17 ` Anup Patel
0 siblings, 1 reply; 10+ messages in thread
From: Guo Ren @ 2020-10-24 3:09 UTC (permalink / raw)
To: Anup Patel
Cc: Guo Ren, Jason Cooper, wesley, Marc Zyngier, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Atish Patra,
Yash Shah, Zong Li, Paul Walmsley, Greentime Hu, Thomas Gleixner,
Christoph Hellwig
On Fri, Oct 23, 2020 at 8:31 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Fri, Oct 23, 2020 at 3:48 PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@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 <guoren@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/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs
2020-10-23 11:27 ` Anup Patel
@ 2020-10-24 3:38 ` Guo Ren
0 siblings, 0 replies; 10+ messages in thread
From: Guo Ren @ 2020-10-24 3:38 UTC (permalink / raw)
To: Anup Patel
Cc: Guo Ren, Jason Cooper, wesley, Marc Zyngier, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Atish Patra,
Yash Shah, Zong Li, Paul Walmsley, Greentime Hu, Thomas Gleixner,
Christoph Hellwig
On Fri, Oct 23, 2020 at 8:17 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Fri, Oct 23, 2020 at 3:48 PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > If "echo 3 > /proc/irq/1/smp_affinity", we want irq 1 could be
> > broadcast to CPU0 & CPU1 and one of them would pick up the irq
> > handler.
> >
> > But current implementation couldn't let multi CPUs process the
> > same IRQ concurrent.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 23 ++++++-----------------
> > 1 file changed, 6 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 2e56576..0003322 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -114,15 +114,12 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
> > static void plic_irq_unmask(struct irq_data *d)
> > {
> > struct cpumask amask;
> > - unsigned int cpu;
> > struct plic_priv *priv = irq_get_chip_data(d->irq);
> >
> > cpumask_and(&amask, &priv->lmask, cpu_online_mask);
> > - cpu = cpumask_any_and(irq_data_get_affinity_mask(d),
> > - &amask);
> > - if (WARN_ON_ONCE(cpu >= nr_cpu_ids))
> > - return;
> > - plic_irq_toggle(cpumask_of(cpu), d, 1);
> > + cpumask_and(&amask, &amask, irq_data_get_affinity_mask(d));
> > +
> > + plic_irq_toggle(&amask, d, 1);
> > }
> >
> > static void plic_irq_mask(struct irq_data *d)
> > @@ -136,24 +133,16 @@ static void plic_irq_mask(struct irq_data *d)
> > static int plic_set_affinity(struct irq_data *d,
> > const struct cpumask *mask_val, bool force)
> > {
> > - unsigned int cpu;
> > struct cpumask amask;
> > struct plic_priv *priv = irq_get_chip_data(d->irq);
> >
> > cpumask_and(&amask, &priv->lmask, mask_val);
> > + cpumask_and(&amask, &amask, cpu_online_mask);
> >
> > - if (force)
> > - cpu = cpumask_first(&amask);
> > - else
> > - cpu = cpumask_any_and(&amask, cpu_online_mask);
> > -
> > - if (cpu >= nr_cpu_ids)
> > - return -EINVAL;
> > + irq_data_update_effective_affinity(d, &amask);
> >
> > plic_irq_toggle(&priv->lmask, d, 0);
> > - plic_irq_toggle(cpumask_of(cpu), d, 1);
> > -
> > - irq_data_update_effective_affinity(d, cpumask_of(cpu));
> > + plic_irq_toggle(&amask, d, 1);
> >
> > return IRQ_SET_MASK_OK_DONE;
> > }
> > --
> > 2.7.4
> >
>
> In the past, a similar patch was proposed by Zong Li (SiFive). We
> have good reasons to not allow multiple CPUs handle the same IRQ.
>
> Refer, https://lkml.org/lkml/2020/4/26/201
>
> NACK from my side.
Thx for sharing the information, I agree with Zong Li & Greentime's
aspect: Don't limit the usage of PLIC! We could let (one hart -> one
irq) as default.
I also agree that using irq broadcast indiscriminately can cause
performance problems. (Anup and Mark Z's view)
I think you've discussed enough at that time and l won't persist the patch.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
2020-10-24 3:09 ` Guo Ren
@ 2020-10-25 9:17 ` Anup Patel
2020-10-26 15:01 ` Guo Ren
0 siblings, 1 reply; 10+ messages in thread
From: Anup Patel @ 2020-10-25 9:17 UTC (permalink / raw)
To: Guo Ren
Cc: Guo Ren, Jason Cooper, wesley, Marc Zyngier, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Atish Patra,
Yash Shah, Zong Li, Paul Walmsley, Greentime Hu, Thomas Gleixner,
Christoph Hellwig
On Sat, Oct 24, 2020 at 8:40 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Fri, Oct 23, 2020 at 8:31 PM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Fri, Oct 23, 2020 at 3:48 PM <guoren@kernel.org> wrote:
> > >
> > > From: Guo Ren <guoren@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 <guoren@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'.
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 = <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;
> };
> };
> }
For above DTS configuration, we need a PLIC with just 4 contexts
(irrespective to the HART IDs).
The PLIC context to HART mapping can be described using the
interrupts-extended DT property of PLIC DT node as follows:
interrupts-extended = <&cpu0_intc 11 &cpu0_intc 9
&cpu1_intc 11 &cpu1_intc 9>;
>
> >
> > 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++)
>
This modified patch still breaks SiFive Unleashed board because
it has following PLIC context to HART mappings:
PLIC_context_0 (offset 0x0000) => HART0_M_mode_irq
PLIC_context_1 (offset 0x1000) => HART1_M_mode_irq
PLIC_context_2 (offset 0x2000) => HART1_S_mode_irq
PLIC_context_3 (offset 0x3000) => HART2_M_mode_irq
PLIC_context_4 (offset 0x4000) => HART2_S_mode_irq
PLIC_context_5 (offset 0x5000) => HART3_M_mode_irq
PLIC_context_6 (offset 0x6000) => HART3_S_mode_irq
PLIC_context_7 (offset 0x7000) => HART4_M_mode_irq
PLIC_context_8 (offset 0x8000) => HART4_S_mode_irq
As-per your patch, the Linux S-mode kernel will use
PLIC_context_3 (offset 0x3000) for HART1 which is in-correct
because PLIC_context_3 maps to HART2 M-mode IRQ.
Further with your patch, the Linux M-mode kernel will use
PLIC_context_2 (offset 0x2000) for HART1 which is in-correct
because PLIC_context_2 maps to HART1 S-mode IRQ.
Clearly your modified patch breaks both Linux S-mode kernel
and Linux M-mode kernel on SiFive Unleashed board.
The current driver handles all above cases correctly by parsing
the PLIC context to HART mappings from the interrupts-extended
DT property. I don't see any bug here.
Regards,
Anup
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base
2020-10-25 9:17 ` Anup Patel
@ 2020-10-26 15:01 ` Guo Ren
0 siblings, 0 replies; 10+ messages in thread
From: Guo Ren @ 2020-10-26 15:01 UTC (permalink / raw)
To: Anup Patel
Cc: Guo Ren, Jason Cooper, wesley, Marc Zyngier, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Atish Patra,
Yash Shah, Zong Li, Paul Walmsley, Greentime Hu, Thomas Gleixner,
Christoph Hellwig
Hi Anup,
On Sun, Oct 25, 2020 at 5:18 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Sat, Oct 24, 2020 at 8:40 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Fri, Oct 23, 2020 at 8:31 PM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Fri, Oct 23, 2020 at 3:48 PM <guoren@kernel.org> wrote:
> > > >
> > > > From: Guo Ren <guoren@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 <guoren@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'.
>
> 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 = <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;
> > };
> > };
> > }
>
> For above DTS configuration, we need a PLIC with just 4 contexts
> (irrespective to the HART IDs).
>
> The PLIC context to HART mapping can be described using the
> interrupts-extended DT property of PLIC DT node as follows:
> interrupts-extended = <&cpu0_intc 11 &cpu0_intc 9
> &cpu1_intc 11 &cpu1_intc 9>;
>
I have 4 harts (0, 1, 2, 3) and I want to run two Linux systems in the
same soc with 0+1 & 2+3.
In this case CPU index of dts are different from hartid.
First dts:
cpu index 0 -> hartid 0
cpu index 1 -> hartid 1
Second dts:
cpu index 0 -> hartid 2
cpu index 1 -> hartid 3
> >
> > >
> > > 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++)
> >
>
> This modified patch still breaks SiFive Unleashed board because
> it has following PLIC context to HART mappings:
> PLIC_context_0 (offset 0x0000) => HART0_M_mode_irq
> PLIC_context_1 (offset 0x1000) => HART1_M_mode_irq
> PLIC_context_2 (offset 0x2000) => HART1_S_mode_irq
> PLIC_context_3 (offset 0x3000) => HART2_M_mode_irq
> PLIC_context_4 (offset 0x4000) => HART2_S_mode_irq
> PLIC_context_5 (offset 0x5000) => HART3_M_mode_irq
> PLIC_context_6 (offset 0x6000) => HART3_S_mode_irq
> PLIC_context_7 (offset 0x7000) => HART4_M_mode_irq
> PLIC_context_8 (offset 0x8000) => HART4_S_mode_irq
Got it, thx, Our system:
PLIC_context_0 (offset 0x0000) => HART0_M_mode_irq
PLIC_context_1 (offset 0x1000) => HART0_S_mode_irq
PLIC_context_2 (offset 0x2000) => HART1_M_mode_irq
PLIC_context_3 (offset 0x3000) => HART1_S_mode_irq
PLIC_context_4 (offset 0x0000) => HART2_M_mode_irq
PLIC_context_5 (offset 0x1000) => HART2_S_mode_irq
PLIC_context_6 (offset 0x2000) => HART3_M_mode_irq
PLIC_context_7 (offset 0x3000) => HART3_S_mode_irq
If I didn't modify PLIC driver, you reconmand me write dts like this?
cpus {
#address-cells = <1>;
#size-cells = <0>;
timebase-frequency = <3000000>;
cpu@0 {
device_type = "cpu";
reg = <0>;
status = "disabled"; // disabled ?
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 = <1>;
status = "disabled"; // disabled ?
compatible = "riscv";
riscv,isa = "rv64imafdcsu";
mmu-type = "riscv,sv39";
cpu1_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu@2 {
device_type = "cpu";
reg = <2>;
status = "ok";
compatible = "riscv";
riscv,isa = "rv64imafdcsu";
mmu-type = "riscv,sv39";
cpu2_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
cpu@3 {
device_type = "cpu";
reg = <3>;
status = "ok";
compatible = "riscv";
riscv,isa = "rv64imafdcsu";
mmu-type = "riscv,sv39";
cpu3_intc: interrupt-controller {
#interrupt-cells = <1>;
compatible = "riscv,cpu-intc";
interrupt-controller;
};
};
};
intc: interrupt-controller@3f0000000 {
#interrupt-cells = <1>;
compatible = "riscv,plic0";
interrupt-controller;
interrupts-extended = <
&cpu0_intc 0xffffffff &cpu0_intc 0xffffffff
&cpu1_intc 0xffffffff &cpu1_intc 0xffffffff
&cpu2_intc 0xffffffff &cpu2_intc 9
&cpu3_intc 0xffffffff &cpu3_intc 9
>;
reg = <0x3 0xf0000000 0x0 0x04000000>;
reg-names = "control";
riscv,max-priority = <7>;
riscv,ndev = <80>;
};
Still use index to address the reg base.
>
> As-per your patch, the Linux S-mode kernel will use
> PLIC_context_3 (offset 0x3000) for HART1 which is in-correct
> because PLIC_context_3 maps to HART2 M-mode IRQ.
>
> Further with your patch, the Linux M-mode kernel will use
> PLIC_context_2 (offset 0x2000) for HART1 which is in-correct
> because PLIC_context_2 maps to HART1 S-mode IRQ.
>
> Clearly your modified patch breaks both Linux S-mode kernel
> and Linux M-mode kernel on SiFive Unleashed board.
>
> The current driver handles all above cases correctly by parsing
> the PLIC context to HART mappings from the interrupts-extended
> DT property. I don't see any bug here.
>
soc {
plic0: interrupt-controller@c000000 {
#interrupt-cells = <1>;
compatible = "sifive,plic-1.0.0";
reg = <0x0 0xc000000 0x0 0x4000000>;
riscv,ndev = <53>;
interrupt-controller;
interrupts-extended = <
&cpu0_intc 0xffffffff
&cpu1_intc 0xffffffff &cpu1_intc 9
&cpu2_intc 0xffffffff &cpu2_intc 9
&cpu3_intc 0xffffffff &cpu3_intc 9
&cpu4_intc 0xffffffff &cpu4_intc 9>;
};
Yes, my patch will break SiFive's u54. The cpu0 hasn't S-mode and it
makes an interrupts-extended odd number. Sorry I haven't seen u54 dts
before and I just think interrupts-extended should be even.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-26 15:01 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-23 10:17 [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base guoren
2020-10-23 10:17 ` [PATCH 2/3] irqchip/irq-sifive-plic: Fixup couldn't broadcast to multi CPUs guoren
2020-10-23 11:27 ` Anup Patel
2020-10-24 3:38 ` Guo Ren
2020-10-23 10:17 ` [PATCH 3/3] irqchip/irq-sifive-plic: Fixup set_affinity enable irq unexpected guoren
2020-10-23 11:33 ` Anup Patel
2020-10-23 11:42 ` [PATCH 1/3] irqchip/irq-sifive-plic: Fixup wrong size of xxx_PER_HART and reg base Anup Patel
2020-10-24 3:09 ` Guo Ren
2020-10-25 9:17 ` Anup Patel
2020-10-26 15:01 ` Guo Ren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).