On Sun, Jun 13, 2021 at 3:11 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Sat, 12 Jun 2021 17:04:17 +0100,
> Anup Patel <anup.patel@xxxxxxx> wrote:
> >
> > The RISC-V ACLINT provides MSWI and SSWI devices for M-mode and
> > S-mode software interrupts respectively. We add irqchip driver
> > which provide IPI operations based on ACLINT [M|S]SWI devices
> > to the Linux RISC-V kernel.
> >
> > Signed-off-by: Anup Patel <anup.patel@xxxxxxx>
> > ---
> > drivers/irqchip/Kconfig | 11 +++
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-aclint-swi.c | 122 +++++++++++++++++++++++++++++++
> > 3 files changed, 134 insertions(+)
> > create mode 100644 drivers/irqchip/irq-aclint-swi.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 62543a4eccc0..2010d493b03b 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -508,6 +508,17 @@ config RISCV_INTC
> >
> > If you don't know what to do here, say Y.
> >
> > +config RISCV_ACLINT_SWI
> > + bool "RISC-V Advanced Core Local Interruptor Software Interrupts"
> > + depends on RISCV
> > + help
> > + This enables support for software interrupts using the Advanced
> > + Core Local Interruptor (ACLINT) found in RISC-V systems. The
> > + RISC-V ACLINT provides devices for inter-process interrupt and
> > + timer functionality.
> > +
> > + If you don't know what to do here, say Y.
> > +
> > config SIFIVE_PLIC
> > bool "SiFive Platform-Level Interrupt Controller"
> > depends on RISCV
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index f88cbf36a9d2..a6edf6733c1d 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -97,6 +97,7 @@ obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o
> > obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o
> > obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o
> > obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o
> > +obj-$(CONFIG_RISCV_ACLINT_SWI) += irq-aclint-swi.o
> > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
> > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o
> > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o
> > diff --git a/drivers/irqchip/irq-aclint-swi.c b/drivers/irqchip/irq-aclint-swi.c
> > new file mode 100644
> > index 000000000000..f9607072cc7b
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-aclint-swi.c
> > @@ -0,0 +1,122 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2021 Western Digital Corporation or its affiliates.
> > + */
> > +
> > +#define pr_fmt(fmt) "aclint-swi: " fmt
> > +#include <linux/cpu.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/smp.h>
> > +
> > +struct aclint_swi {
> > + void __iomem *sip_reg;
> > +};
> > +static DEFINE_PER_CPU(struct aclint_swi, aclint_swis);
> > +
> > +static void aclint_swi_send_ipi(const struct cpumask *target)
> > +{
> > + unsigned int cpu;
> > + struct aclint_swi *swi;
> > +
> > + for_each_cpu(cpu, target) {
> > + swi = per_cpu_ptr(&aclint_swis, cpu);
> > + if (!swi->sip_reg) {
> > + pr_warn("%s: CPU%d SIP register not available\n",
> > + __func__, cpu);
> > + continue;
> > + }
> > +
> > + writel(1, swi->sip_reg);
> > + }
> > +}
> > +
> > +static void aclint_swi_clear_ipi(void)
> > +{
> > + struct aclint_swi *swi = this_cpu_ptr(&aclint_swis);
> > +
> > + if (!swi->sip_reg) {
> > + pr_warn("%s: CPU%d SIP register not available\n",
> > + __func__, smp_processor_id());
> > + return;
> > + }
> > +
> > + writel(0, swi->sip_reg);
> > +}
> > +
> > +static struct riscv_ipi_ops aclint_swi_ipi_ops = {
> > + .name = "ACLINT-SWI",
> > + .use_for_rfence = true,
> > + .ipi_inject = aclint_swi_send_ipi,
> > + .ipi_clear = aclint_swi_clear_ipi,
> > +};
> > +
> > +static int __init aclint_swi_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + void __iomem *base;
> > + struct aclint_swi *swi;
> > + u32 i, nr_irqs, nr_cpus = 0;
> > +
> > + /* Map the registers */
> > + base = of_iomap(node, 0);
> > + if (!base) {
> > + pr_err("%pOFP: could not map registers\n", node);
> > + return -ENODEV;
> > + }
> > +
> > + /* Iterarte over each target CPU connected with this ACLINT */
> > + nr_irqs = of_irq_count(node);
> > + for (i = 0; i < nr_irqs; i++) {
> > + struct of_phandle_args parent;
> > + int cpu, hartid;
> > +
> > + if (of_irq_parse_one(node, i, &parent)) {
> > + pr_err("%pOFP: failed to parse irq %d.\n",
> > + node, i);
> > + continue;
> > + }
> > +
> > + if (parent.args[0] != RV_IRQ_SOFT) {
> > + pr_err("%pOFP: invalid irq %d (hwirq %d)\n",
> > + node, i, parent.args[0]);
> > + continue;
> > + }
> > +
> > + hartid = riscv_of_parent_hartid(parent.np);
> > + if (hartid < 0) {
> > + pr_warn("failed to parse hart ID for irq %d.\n", i);
> > + continue;
> > + }
> > +
> > + cpu = riscv_hartid_to_cpuid(hartid);
> > + if (cpu < 0) {
> > + pr_warn("Invalid cpuid for irq %d\n", i);
> > + continue;
> > + }
> > +
> > + swi = per_cpu_ptr(&aclint_swis, cpu);
> > + swi->sip_reg = base + i * sizeof(u32);
> > + nr_cpus++;
> > + }
> > +
> > + /* Announce the ACLINT SWI device */
> > + pr_info("%pOFP: providing IPIs for %d CPUs\n", node, nr_cpus);
> > +
> > + /* Register the IPI operations */
> > + riscv_set_ipi_ops(&aclint_swi_ipi_ops);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_RISCV_M_MODE
> > +IRQCHIP_DECLARE(riscv_aclint_swi, "riscv,aclint-mswi", aclint_swi_init);
> > +#else
> > +IRQCHIP_DECLARE(riscv_aclint_swi, "riscv,aclint-sswi", aclint_swi_init);
> > +#endif
>
> I'm sorry, but this really isn't an irqchip driver. This is a piece of
> arch-specific code that uses *none* of the irq subsystem abstractions
> apart from the IRQCHIP_DECLARE() macro.
Yes, I was not sure we can call it IRQCHIP hence the RFC PATCH.
Both ACLINT MSWI and SSWI are special devices providing only IPI
support so I will re-think how to fit this.
>
> If you implemented it on top of the IPI irq_domain abstraction, making
> your IPIs actual IRQs, use the proper interrupt flows and accounting,
> then it would make sense to call it an irqchip driver. But as it
> stands, it has no place in drivers/irqchip.
Okay, let me explore IPI irq_domain if it is suitable for this.
Regards,
Anup
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
