| Message ID | 20220301005142.3038702-3-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> > > When detecting a hart context for a privilege mode different from the > current running privilege mode, we simply skip to the next hart context > register. > > This means that we never clear the S-mode enable bits when running in > M-mode. > > On canaan k210, a bunch of S-mode interrupts are enabled by the bootrom. > These S-mode specific interrupts should never trigger, since we never set > the mie.SEIE bit in the parent interrupt controller (riscv-intc). > > However, we will be able to see the mip.SEIE bit set as pending. > > This doesn't seem like a good default when CONFIG_RISCV_M_MODE is set, > since in that case we will never enter lower privilege mode (e.g. S-mode). > > Let's clear the S-mode enable bits when running the kernel in M-mode, such > that we won't have a interrupt pending bit set, which we will never clear. > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > drivers/irqchip/irq-sifive-plic.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c > index 211bcb10aa93..46caeb11a114 100644 > --- a/drivers/irqchip/irq-sifive-plic.c > +++ b/drivers/irqchip/irq-sifive-plic.c > @@ -326,8 +326,19 @@ static int __init plic_init(struct device_node *node, > * Skip contexts other than external interrupts for our > * privilege level. > */ > - if (parent.args[0] != RV_IRQ_EXT) > + if (parent.args[0] != RV_IRQ_EXT) { > + /* Disable S-mode enable bits if running in M-mode. */ > + if (IS_ENABLED(CONFIG_RISCV_M_MODE)) { > + struct plic_handler tmp_handler = {}; > + > + raw_spin_lock_init(&tmp_handler.enable_lock); Creating a dummy plic_handler over here is a strange work-around. Please define and use "void __plic_toggle(void __iomem *enable_base, int hwirq, int enable)" over here and in the plic_toggle() function. Regards, Anup > + tmp_handler.enable_base = priv->regs + > + ENABLE_BASE + i * ENABLE_PER_HART_CTX; > + for (hwirq = 1; hwirq <= nr_irqs; hwirq++) > + plic_toggle(&tmp_handler, hwirq, 0); > + } > continue; > + } > > hartid = riscv_of_parent_hartid(parent.np); > if (hartid < 0) { > -- > 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:48:20AM +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> > > > > When detecting a hart context for a privilege mode different from the > > current running privilege mode, we simply skip to the next hart context > > register. > > > > This means that we never clear the S-mode enable bits when running in > > M-mode. > > > > On canaan k210, a bunch of S-mode interrupts are enabled by the bootrom. > > These S-mode specific interrupts should never trigger, since we never set > > the mie.SEIE bit in the parent interrupt controller (riscv-intc). > > > > However, we will be able to see the mip.SEIE bit set as pending. > > > > This doesn't seem like a good default when CONFIG_RISCV_M_MODE is set, > > since in that case we will never enter lower privilege mode (e.g. S-mode). > > > > Let's clear the S-mode enable bits when running the kernel in M-mode, such > > that we won't have a interrupt pending bit set, which we will never clear. > > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > > --- > > drivers/irqchip/irq-sifive-plic.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c > > index 211bcb10aa93..46caeb11a114 100644 > > --- a/drivers/irqchip/irq-sifive-plic.c > > +++ b/drivers/irqchip/irq-sifive-plic.c > > @@ -326,8 +326,19 @@ static int __init plic_init(struct device_node *node, > > * Skip contexts other than external interrupts for our > > * privilege level. > > */ > > - if (parent.args[0] != RV_IRQ_EXT) > > + if (parent.args[0] != RV_IRQ_EXT) { > > + /* Disable S-mode enable bits if running in M-mode. */ > > + if (IS_ENABLED(CONFIG_RISCV_M_MODE)) { > > + struct plic_handler tmp_handler = {}; > > + > > + raw_spin_lock_init(&tmp_handler.enable_lock); > > Creating a dummy plic_handler over here is a strange work-around. > > Please define and use > "void __plic_toggle(void __iomem *enable_base, int hwirq, int enable)" > over here and in the plic_toggle() function. Hello Anup, Your suggestion sounds like a cleaner solution, will fix in V2. Kind regards, Niklas
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index 211bcb10aa93..46caeb11a114 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -326,8 +326,19 @@ static int __init plic_init(struct device_node *node, * Skip contexts other than external interrupts for our * privilege level. */ - if (parent.args[0] != RV_IRQ_EXT) + if (parent.args[0] != RV_IRQ_EXT) { + /* Disable S-mode enable bits if running in M-mode. */ + if (IS_ENABLED(CONFIG_RISCV_M_MODE)) { + struct plic_handler tmp_handler = {}; + + raw_spin_lock_init(&tmp_handler.enable_lock); + tmp_handler.enable_base = priv->regs + + ENABLE_BASE + i * ENABLE_PER_HART_CTX; + for (hwirq = 1; hwirq <= nr_irqs; hwirq++) + plic_toggle(&tmp_handler, hwirq, 0); + } continue; + } hartid = riscv_of_parent_hartid(parent.np); if (hartid < 0) {