Re: [PATCH v13 06/13] irqchip: Add RISC-V incoming MSI controller early driver
From: Björn Töpel
Date: Tue Feb 20 2024 - 06:53:06 EST
Anup,
This version is so much easier to follow! Thanks a lot for then
cleanups/design changes.
A bunch of nits, and a major one, below.
Anup Patel <apatel@xxxxxxxxxxxxxxxx> writes:
> The RISC-V advanced interrupt architecture (AIA) specification
> defines a new MSI controller called incoming message signalled
> interrupt controller (IMSIC) which manages MSI on per-HART (or
> per-CPU) basis. It also supports IPIs as software injected MSIs.
> (For more details refer https://github.com/riscv/riscv-aia)
>
> Let us add an early irqchip driver for RISC-V IMSIC which sets
> up the IMSIC state and provide IPIs.
>
> Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> ---
> drivers/irqchip/Kconfig | 7 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-riscv-imsic-early.c | 213 ++++++
> drivers/irqchip/irq-riscv-imsic-state.c | 906 ++++++++++++++++++++++++
> drivers/irqchip/irq-riscv-imsic-state.h | 98 +++
> include/linux/irqchip/riscv-imsic.h | 87 +++
> 6 files changed, 1312 insertions(+)
> create mode 100644 drivers/irqchip/irq-riscv-imsic-early.c
> create mode 100644 drivers/irqchip/irq-riscv-imsic-state.c
> create mode 100644 drivers/irqchip/irq-riscv-imsic-state.h
> create mode 100644 include/linux/irqchip/riscv-imsic.h
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index f7149d0f3d45..85f86e31c996 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -546,6 +546,13 @@ config SIFIVE_PLIC
> select IRQ_DOMAIN_HIERARCHY
> select GENERIC_IRQ_EFFECTIVE_AFF_MASK if SMP
>
> +config RISCV_IMSIC
> + bool
> + depends on RISCV
> + select IRQ_DOMAIN_HIERARCHY
> + select GENERIC_IRQ_MATRIX_ALLOCATOR
> + select GENERIC_MSI_IRQ
> +
> config EXYNOS_IRQ_COMBINER
> bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
> depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index ffd945fe71aa..d714724387ce 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -95,6 +95,7 @@ obj-$(CONFIG_QCOM_MPM) += irq-qcom-mpm.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_IMSIC) += irq-riscv-imsic-state.o irq-riscv-imsic-early.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-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> new file mode 100644
> index 000000000000..32fe428b1c19
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Western Digital Corporation or its affiliates.
> + * Copyright (C) 2022 Ventana Micro Systems Inc.
> + */
> +
> +#define pr_fmt(fmt) "riscv-imsic: " fmt
> +#include <linux/cpu.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +#include <linux/smp.h>
> +
> +#include "irq-riscv-imsic-state.h"
> +
> +static int imsic_parent_irq;
> +
> +#ifdef CONFIG_SMP
> +static void imsic_ipi_send(unsigned int cpu)
> +{
> + struct imsic_local_config *local = per_cpu_ptr(imsic->global.local, cpu);
> +
> + writel_relaxed(IMSIC_IPI_ID, local->msi_va);
> +}
> +
> +static void imsic_ipi_starting_cpu(void)
> +{
> + /* Enable IPIs for current CPU. */
> + __imsic_id_set_enable(IMSIC_IPI_ID);
> +}
> +
> +static void imsic_ipi_dying_cpu(void)
> +{
> + /* Disable IPIs for current CPU. */
> + __imsic_id_clear_enable(IMSIC_IPI_ID);
> +}
> +
> +static int __init imsic_ipi_domain_init(void)
> +{
> + int virq;
> +
> + /* Create IMSIC IPI multiplexing */
> + virq = ipi_mux_create(IMSIC_NR_IPI, imsic_ipi_send);
> + if (virq <= 0)
> + return (virq < 0) ? virq : -ENOMEM;
Nit: No parenthesis need to clutter.
> +
> + /* Set vIRQ range */
> + riscv_ipi_set_virq_range(virq, IMSIC_NR_IPI, true);
> +
> + /* Announce that IMSIC is providing IPIs */
> + pr_info("%pfwP: providing IPIs using interrupt %d\n", imsic->fwnode, IMSIC_IPI_ID);
> +
> + return 0;
> +}
> +#else
> +static void imsic_ipi_starting_cpu(void)
> +{
> +}
> +
> +static void imsic_ipi_dying_cpu(void)
> +{
> +}
> +
> +static int __init imsic_ipi_domain_init(void)
> +{
> + return 0;
> +}
> +#endif
> +
> +/*
> + * To handle an interrupt, we read the TOPEI CSR and write zero in one
> + * instruction. If TOPEI CSR is non-zero then we translate TOPEI.ID to
> + * Linux interrupt number and let Linux IRQ subsystem handle it.
> + */
> +static void imsic_handle_irq(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + int err, cpu = smp_processor_id();
> + struct imsic_vector *vec;
> + unsigned long local_id;
> +
> + chained_irq_enter(chip, desc);
> +
> + while ((local_id = csr_swap(CSR_TOPEI, 0))) {
> + local_id = local_id >> TOPEI_ID_SHIFT;
Nit: Wdyt about moving shift into the loop predicate, or using >>=?
> +
> + if (local_id == IMSIC_IPI_ID) {
> +#ifdef CONFIG_SMP
> + ipi_mux_process();
> +#endif
Is IMSIC_IPI_ID a thing on !IS_ENABLED(CONFIG_SMP)?
> + continue;
> + }
> +
> + if (unlikely(!imsic->base_domain))
> + continue;
> +
> + vec = imsic_vector_from_local_id(cpu, local_id);
> + if (!vec) {
> + pr_warn_ratelimited("vector not found for local ID 0x%lx\n", local_id);
> + continue;
> + }
> +
> + err = generic_handle_domain_irq(imsic->base_domain,
> + vec->hwirq);
Nit: 100 chars
> + if (unlikely(err))
> + pr_warn_ratelimited("hwirq 0x%x mapping not found\n", vec->hwirq);
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static int imsic_starting_cpu(unsigned int cpu)
> +{
> + /* Mark per-CPU IMSIC state as online */
> + imsic_state_online();
> +
> + /* Enable per-CPU parent interrupt */
> + enable_percpu_irq(imsic_parent_irq, irq_get_trigger_type(imsic_parent_irq));
> +
> + /* Setup IPIs */
> + imsic_ipi_starting_cpu();
> +
> + /*
> + * Interrupts identities might have been enabled/disabled while
> + * this CPU was not running so sync-up local enable/disable state.
> + */
> + imsic_local_sync_all();
> +
> + /* Enable local interrupt delivery */
> + imsic_local_delivery(true);
> +
> + return 0;
> +}
> +
> +static int imsic_dying_cpu(unsigned int cpu)
> +{
> + /* Cleanup IPIs */
> + imsic_ipi_dying_cpu();
> +
> + /* Mark per-CPU IMSIC state as offline */
> + imsic_state_offline();
> +
> + return 0;
> +}
> +
> +static int __init imsic_early_probe(struct fwnode_handle *fwnode)
> +{
> + struct irq_domain *domain;
> + int rc;
> +
> + /* Find parent domain and register chained handler */
> + domain = irq_find_matching_fwnode(riscv_get_intc_hwnode(), DOMAIN_BUS_ANY);
> + if (!domain) {
> + pr_err("%pfwP: Failed to find INTC domain\n", fwnode);
> + return -ENOENT;
> + }
> + imsic_parent_irq = irq_create_mapping(domain, RV_IRQ_EXT);
> + if (!imsic_parent_irq) {
> + pr_err("%pfwP: Failed to create INTC mapping\n", fwnode);
> + return -ENOENT;
> + }
> +
> + /* Initialize IPI domain */
> + rc = imsic_ipi_domain_init();
> + if (rc) {
> + pr_err("%pfwP: Failed to initialize IPI domain\n", fwnode);
> + return rc;
> + }
> +
> + /* Setup chained handler to the parent domain interrupt */
> + irq_set_chained_handler(imsic_parent_irq, imsic_handle_irq);
> +
> + /*
> + * Setup cpuhp state (must be done after setting imsic_parent_irq)
> + *
> + * Don't disable per-CPU IMSIC file when CPU goes offline
> + * because this affects IPI and the masking/unmasking of
> + * virtual IPIs is done via generic IPI-Mux
> + */
> + cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "irqchip/riscv/imsic:starting",
> + imsic_starting_cpu, imsic_dying_cpu);
> +
> + return 0;
> +}
> +
> +static int __init imsic_early_dt_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + struct fwnode_handle *fwnode = &node->fwnode;
> + int rc;
> +
> + /* Setup IMSIC state */
> + rc = imsic_setup_state(fwnode);
> + if (rc) {
> + pr_err("%pfwP: failed to setup state (error %d)\n",
> + fwnode, rc);
Nit. 100 chars
> + return rc;
> + }
> +
> + /* Do early setup of IPIs */
> + rc = imsic_early_probe(fwnode);
> + if (rc)
> + return rc;
> +
> + /* Ensure that OF platform device gets probed */
> + of_node_clear_flag(node, OF_POPULATED);
> + return 0;
> +}
> +IRQCHIP_DECLARE(riscv_imsic, "riscv,imsics", imsic_early_dt_init);
> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> new file mode 100644
> index 000000000000..4f347486ec7c
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-imsic-state.c
> @@ -0,0 +1,906 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Western Digital Corporation or its affiliates.
> + * Copyright (C) 2022 Ventana Micro Systems Inc.
> + */
> +
> +#define pr_fmt(fmt) "riscv-imsic: " fmt
> +#include <linux/cpu.h>
> +#include <linux/bitmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/seq_file.h>
> +#include <linux/spinlock.h>
> +#include <linux/smp.h>
> +#include <asm/hwcap.h>
> +
> +#include "irq-riscv-imsic-state.h"
> +
> +#define IMSIC_DISABLE_EIDELIVERY 0
> +#define IMSIC_ENABLE_EIDELIVERY 1
> +#define IMSIC_DISABLE_EITHRESHOLD 1
> +#define IMSIC_ENABLE_EITHRESHOLD 0
> +
> +static inline void imsic_csr_write(unsigned long reg, unsigned long val)
> +{
> + csr_write(CSR_ISELECT, reg);
> + csr_write(CSR_IREG, val);
> +}
> +
> +static inline unsigned long imsic_csr_read(unsigned long reg)
> +{
> + csr_write(CSR_ISELECT, reg);
> + return csr_read(CSR_IREG);
> +}
> +
> +static inline unsigned long imsic_csr_read_clear(unsigned long reg, unsigned long val)
> +{
> + csr_write(CSR_ISELECT, reg);
> + return csr_read_clear(CSR_IREG, val);
> +}
> +
> +static inline void imsic_csr_set(unsigned long reg, unsigned long val)
> +{
> + csr_write(CSR_ISELECT, reg);
> + csr_set(CSR_IREG, val);
> +}
> +
> +static inline void imsic_csr_clear(unsigned long reg, unsigned long val)
> +{
> + csr_write(CSR_ISELECT, reg);
> + csr_clear(CSR_IREG, val);
> +}
> +
> +struct imsic_priv *imsic;
> +
> +const struct imsic_global_config *imsic_get_global_config(void)
> +{
> + return imsic ? &imsic->global : NULL;
> +}
> +EXPORT_SYMBOL_GPL(imsic_get_global_config);
> +
> +static bool __imsic_eix_read_clear(unsigned long id, bool pend)
> +{
> + unsigned long isel, imask;
> +
> + isel = id / BITS_PER_LONG;
> + isel *= BITS_PER_LONG / IMSIC_EIPx_BITS;
> + isel += pend ? IMSIC_EIP0 : IMSIC_EIE0;
> + imask = BIT(id & (__riscv_xlen - 1));
> +
> + return (imsic_csr_read_clear(isel, imask) & imask) ? true : false;
Nit: use return !!(imsic_csr_read_clear(isel, imask) & imask)
> +}
> +
> +static inline bool __imsic_id_read_clear_enabled(unsigned long id)
> +{
> + return __imsic_eix_read_clear(id, false);
> +}
> +
> +static inline bool __imsic_id_read_clear_pending(unsigned long id)
> +{
> + return __imsic_eix_read_clear(id, true);
> +}
> +
> +void __imsic_eix_update(unsigned long base_id, unsigned long num_id, bool pend, bool val)
> +{
> + unsigned long id = base_id, last_id = base_id + num_id;
> + unsigned long i, isel, ireg;
> +
> + while (id < last_id) {
> + isel = id / BITS_PER_LONG;
> + isel *= BITS_PER_LONG / IMSIC_EIPx_BITS;
> + isel += (pend) ? IMSIC_EIP0 : IMSIC_EIE0;
Nit: Redundant parenthesis.
> +
> + /*
> + * Prepare the ID mask to be programmed in the
> + * IMSIC EIEx and EIPx registers. These registers
> + * are XLEN-wide and we must not touch IDs which
> + * are < base_id and >= (base_id + num_id).
> + */
> + ireg = 0;
> + for (i = id & (__riscv_xlen - 1); (id < last_id) && (i < __riscv_xlen); i++) {
Nit: Redundant parenthesis "(id < last_id) && (i < __riscv_xlen)", which
is also inconsistent with other usage in this changeset.
> + ireg |= BIT(i);
> + id++;
> + }
> +
> + /*
> + * The IMSIC EIEx and EIPx registers are indirectly
> + * accessed via using ISELECT and IREG CSRs so we
> + * need to access these CSRs without getting preempted.
> + *
> + * All existing users of this function call this
> + * function with local IRQs disabled so we don't
> + * need to do anything special here.
> + */
> + if (val)
> + imsic_csr_set(isel, ireg);
> + else
> + imsic_csr_clear(isel, ireg);
> + }
> +}
> +
> +/* MUST be called with lpriv->lock held */
> +static void __imsic_local_sync(struct imsic_local_priv *lpriv)
> +{
> + struct imsic_local_config *mlocal;
> + struct imsic_vector *vec, *mvec;
> + int i;
> +
> + /* This pairs with the barrier in __imsic_remote_sync(). */
> + smp_mb();
> +
> + for_each_set_bit(i, lpriv->dirty_bitmap, imsic->global.nr_ids + 1) {
> + if (!i || i == IMSIC_IPI_ID)
> + goto skip;
> + vec = &lpriv->vectors[i];
> +
> + if (vec->enable)
> + __imsic_id_set_enable(i);
> + else
> + __imsic_id_clear_enable(i);
> +
> + /*
> + * If the ID was being moved to a new ID on some other CPU
> + * then we can get a MSI during the movement so check the
> + * ID pending bit and re-trigger the new ID on other CPU
> + * using MMIO write.
> + */
> + mvec = vec->move;
> + vec->move = NULL;
> + if (mvec && mvec != vec) {
> + if (__imsic_id_read_clear_pending(i)) {
> + mlocal = per_cpu_ptr(imsic->global.local, mvec->cpu);
> + writel_relaxed(mvec->local_id, mlocal->msi_va);
> + }
> +
> + imsic_vector_free(&lpriv->vectors[i]);
> + }
> +
> +skip:
> + bitmap_clear(lpriv->dirty_bitmap, i, 1);
> + }
> +}
> +
> +void imsic_local_sync_all(void)
> +{
> + struct imsic_local_priv *lpriv = this_cpu_ptr(imsic->lpriv);
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&lpriv->lock, flags);
> + bitmap_fill(lpriv->dirty_bitmap, imsic->global.nr_ids + 1);
> + __imsic_local_sync(lpriv);
> + raw_spin_unlock_irqrestore(&lpriv->lock, flags);
> +}
> +
> +void imsic_local_delivery(bool enable)
> +{
> + if (enable) {
> + imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_ENABLE_EITHRESHOLD);
> + imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_ENABLE_EIDELIVERY);
> + return;
> + }
> +
> + imsic_csr_write(IMSIC_EIDELIVERY, IMSIC_DISABLE_EIDELIVERY);
> + imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_DISABLE_EITHRESHOLD);
> +}
> +
> +#ifdef CONFIG_SMP
> +static void imsic_local_timer_callback(struct timer_list *timer)
> +{
> + struct imsic_local_priv *lpriv = this_cpu_ptr(imsic->lpriv);
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&lpriv->lock, flags);
> + __imsic_local_sync(lpriv);
> + raw_spin_unlock_irqrestore(&lpriv->lock, flags);
> +}
> +
> +/* MUST be called with lpriv->lock held */
> +static void __imsic_remote_sync(struct imsic_local_priv *lpriv, unsigned int cpu)
> +{
> + /*
> + * Ensure that changes to vector enable, vector move and
> + * dirty bitmap are visible to the target CPU.
> + *
> + * This pairs with the barrier in __imsic_local_sync().
> + */
> + smp_mb();
> +
> + /*
> + * We schedule a timer on the target CPU if the target CPU is not
> + * same as the current CPU. An offline CPU will unconditionally
> + * synchronize IDs through imsic_starting_cpu() when the
> + * CPU is brought up.
> + */
> + if (cpu_online(cpu)) {
> + if (cpu != smp_processor_id()) {
> + if (!timer_pending(&lpriv->timer)) {
> + lpriv->timer.expires = jiffies + 1;
> + add_timer_on(&lpriv->timer, cpu);
> + }
> + } else {
> + __imsic_local_sync(lpriv);
> + }
Nit: Early exit/return vs else-clause for readability
> + }
> +}
> +#else
> +/* MUST be called with lpriv->lock held */
> +static void __imsic_remote_sync(struct imsic_local_priv *lpriv, unsigned int cpu)
> +{
> + __imsic_local_sync(lpriv);
> +}
> +#endif
> +
> +void imsic_vector_mask(struct imsic_vector *vec)
> +{
> + struct imsic_local_priv *lpriv;
> +
> + lpriv = per_cpu_ptr(imsic->lpriv, vec->cpu);
> + if (WARN_ON(&lpriv->vectors[vec->local_id] != vec))
> + return;
> +
> + /*
> + * This function is called through Linux irq subsystem with
> + * irqs disabled so no need to save/restore irq flags.
> + */
> +
> + raw_spin_lock(&lpriv->lock);
> +
> + vec->enable = false;
> + bitmap_set(lpriv->dirty_bitmap, vec->local_id, 1);
> + __imsic_remote_sync(lpriv, vec->cpu);
> +
> + raw_spin_unlock(&lpriv->lock);
> +}
Really nice that you're using a timer for the vector affinity change,
and got rid of the special/weird IMSIC/sync IPI. Can you really use a
timer for mask/unmask? That makes the mask/unmask operation
asynchronous!
That was what I was trying to get though with this comment:
https://lore.kernel.org/linux-riscv/87sf24mo1g.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Also, using the smp_* IPI functions, you can pass arguments, so you
don't need the dirty_bitmap tracking the changes.
> +
> +void imsic_vector_unmask(struct imsic_vector *vec)
> +{
> + struct imsic_local_priv *lpriv;
> +
> + lpriv = per_cpu_ptr(imsic->lpriv, vec->cpu);
> + if (WARN_ON(&lpriv->vectors[vec->local_id] != vec))
> + return;
> +
> + /*
> + * This function is called through Linux irq subsystem with
> + * irqs disabled so no need to save/restore irq flags.
> + */
> +
> + raw_spin_lock(&lpriv->lock);
> +
> + vec->enable = true;
> + bitmap_set(lpriv->dirty_bitmap, vec->local_id, 1);
> + __imsic_remote_sync(lpriv, vec->cpu);
> +
> + raw_spin_unlock(&lpriv->lock);
> +}
> +
> +
> +bool imsic_vector_isenabled(struct imsic_vector *vec)
> +{
> + struct imsic_local_priv *lpriv = per_cpu_ptr(imsic->lpriv, vec->cpu);
> + unsigned long flags;
> + bool ret;
> +
> + raw_spin_lock_irqsave(&lpriv->lock, flags);
> + ret = vec->enable;
> + raw_spin_unlock_irqrestore(&lpriv->lock, flags);
> +
> + return ret;
> +}
> +
> +struct imsic_vector *imsic_vector_get_move(struct imsic_vector *vec)
> +{
> + struct imsic_local_priv *lpriv = per_cpu_ptr(imsic->lpriv, vec->cpu);
> + struct imsic_vector *ret;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&lpriv->lock, flags);
> + ret = vec->move;
> + raw_spin_unlock_irqrestore(&lpriv->lock, flags);
> +
> + return ret;
> +}
> +
> +static bool imsic_vector_move_update(struct imsic_local_priv *lpriv, struct imsic_vector *vec,
> + bool new_enable, struct imsic_vector *new_move)
> +{
> + unsigned long flags;
> + bool enabled;
> +
> + raw_spin_lock_irqsave(&lpriv->lock, flags);
> +
> + /* Update enable and move details */
> + enabled = vec->enable;
> + vec->enable = new_enable;
> + vec->move = new_move;
> +
> + /* Mark the vector as dirty and synchronize */
> + bitmap_set(lpriv->dirty_bitmap, vec->local_id, 1);
> + __imsic_remote_sync(lpriv, vec->cpu);
> +
> + raw_spin_unlock_irqrestore(&lpriv->lock, flags);
> +
> + return enabled;
> +}
> +
> +void imsic_vector_move(struct imsic_vector *old_vec, struct imsic_vector *new_vec)
> +{
> + struct imsic_local_priv *old_lpriv, *new_lpriv;
> + bool enabled;
> +
> + if (WARN_ON(old_vec->cpu == new_vec->cpu))
> + return;
> +
> + old_lpriv = per_cpu_ptr(imsic->lpriv, old_vec->cpu);
> + if (WARN_ON(&old_lpriv->vectors[old_vec->local_id] != old_vec))
> + return;
> +
> + new_lpriv = per_cpu_ptr(imsic->lpriv, new_vec->cpu);
> + if (WARN_ON(&new_lpriv->vectors[new_vec->local_id] != new_vec))
> + return;
> +
> + /*
> + * Move and re-trigger the new vector based on the pending
> + * state of the old vector because we might get a device
> + * interrupt on the old vector while device was being moved
> + * to the new vector.
> + */
> + enabled = imsic_vector_move_update(old_lpriv, old_vec, false, new_vec);
> + imsic_vector_move_update(new_lpriv, new_vec, enabled, new_vec);
> +}
> +
> +#ifdef CONFIG_GENERIC_IRQ_DEBUGFS
> +void imsic_vector_debug_show(struct seq_file *m, struct imsic_vector *vec, int ind)
> +{
> + struct imsic_local_priv *lpriv;
> + struct imsic_vector *mvec;
> + bool is_enabled;
> +
> + lpriv = per_cpu_ptr(imsic->lpriv, vec->cpu);
> + if (WARN_ON(&lpriv->vectors[vec->local_id] != vec))
> + return;
> +
> + is_enabled = imsic_vector_isenabled(vec);
> + mvec = imsic_vector_get_move(vec);
> +
> + seq_printf(m, "%*starget_cpu : %5u\n", ind, "", vec->cpu);
> + seq_printf(m, "%*starget_local_id : %5u\n", ind, "", vec->local_id);
> + seq_printf(m, "%*sis_reserved : %5u\n", ind, "",
> + (vec->local_id <= IMSIC_IPI_ID) ? 1 : 0);
> + seq_printf(m, "%*sis_enabled : %5u\n", ind, "", (is_enabled) ? 1 : 0);
> + seq_printf(m, "%*sis_move_pending : %5u\n", ind, "", (mvec) ? 1 : 0);
Nit: Redundant parenthesis.
> + if (mvec) {
> + seq_printf(m, "%*smove_cpu : %5u\n", ind, "", mvec->cpu);
> + seq_printf(m, "%*smove_local_id : %5u\n", ind, "", mvec->local_id);
> + }
> +}
> +
> +void imsic_vector_debug_show_summary(struct seq_file *m, int ind)
> +{
> + irq_matrix_debug_show(m, imsic->matrix, ind);
> +}
> +#endif
> +
> +struct imsic_vector *imsic_vector_from_local_id(unsigned int cpu, unsigned int local_id)
> +{
> + struct imsic_local_priv *lpriv = per_cpu_ptr(imsic->lpriv, cpu);
> +
> + if (!lpriv || imsic->global.nr_ids < local_id)
> + return NULL;
> +
> + return &lpriv->vectors[local_id];
> +}
> +
> +struct imsic_vector *imsic_vector_alloc(unsigned int hwirq, const struct cpumask *mask)
> +{
> + struct imsic_vector *vec = NULL;
> + struct imsic_local_priv *lpriv;
> + unsigned long flags;
> + unsigned int cpu;
> + int local_id;
> +
> + raw_spin_lock_irqsave(&imsic->matrix_lock, flags);
> + local_id = irq_matrix_alloc(imsic->matrix, mask, false, &cpu);
> + raw_spin_unlock_irqrestore(&imsic->matrix_lock, flags);
> + if (local_id < 0)
> + return NULL;
> +
> + lpriv = per_cpu_ptr(imsic->lpriv, cpu);
> + vec = &lpriv->vectors[local_id];
> + vec->hwirq = hwirq;
> + vec->enable = false;
> + vec->move = NULL;
> +
> + return vec;
> +}
> +
> +void imsic_vector_free(struct imsic_vector *vec)
> +{
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&imsic->matrix_lock, flags);
> + vec->hwirq = UINT_MAX;
> + irq_matrix_free(imsic->matrix, vec->cpu, vec->local_id, false);
> + raw_spin_unlock_irqrestore(&imsic->matrix_lock, flags);
> +}
> +
> +static void __init imsic_local_cleanup(void)
> +{
> + int cpu;
> + struct imsic_local_priv *lpriv;
> +
> + for_each_possible_cpu(cpu) {
> + lpriv = per_cpu_ptr(imsic->lpriv, cpu);
> +
> + bitmap_free(lpriv->dirty_bitmap);
> + kfree(lpriv->vectors);
> + }
> +
> + free_percpu(imsic->lpriv);
> +}
> +
> +static int __init imsic_local_init(void)
> +{
> + struct imsic_global_config *global = &imsic->global;
> + struct imsic_local_priv *lpriv;
> + struct imsic_vector *vec;
> + int cpu, i;
> +
> + /* Allocate per-CPU private state */
> + imsic->lpriv = alloc_percpu(typeof(*(imsic->lpriv)));
> + if (!imsic->lpriv)
> + return -ENOMEM;
> +
> + /* Setup per-CPU private state */
> + for_each_possible_cpu(cpu) {
> + lpriv = per_cpu_ptr(imsic->lpriv, cpu);
> +
> + raw_spin_lock_init(&lpriv->lock);
> +
> + /* Allocate dirty bitmap */
> + lpriv->dirty_bitmap = bitmap_zalloc(global->nr_ids + 1, GFP_KERNEL);
> + if (!lpriv->dirty_bitmap)
> + goto fail_local_cleanup;
> +
> +#ifdef CONFIG_SMP
> + /* Setup lazy timer for synchronization */
> + timer_setup(&lpriv->timer, imsic_local_timer_callback, TIMER_PINNED);
> +#endif
> +
> + /* Allocate vector array */
> + lpriv->vectors = kcalloc(global->nr_ids + 1, sizeof(*lpriv->vectors),
> + GFP_KERNEL);
> + if (!lpriv->vectors)
> + goto fail_local_cleanup;
> +
> + /* Setup vector array */
> + for (i = 0; i <= global->nr_ids; i++) {
> + vec = &lpriv->vectors[i];
> + vec->cpu = cpu;
> + vec->local_id = i;
> + vec->hwirq = UINT_MAX;
> + }
> + }
> +
> + return 0;
> +
> +fail_local_cleanup:
> + imsic_local_cleanup();
> + return -ENOMEM;
> +}
> +
> +void imsic_state_online(void)
> +{
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&imsic->matrix_lock, flags);
> + irq_matrix_online(imsic->matrix);
> + raw_spin_unlock_irqrestore(&imsic->matrix_lock, flags);
> +}
> +
> +void imsic_state_offline(void)
> +{
> +#ifdef CONFIG_SMP
> + struct imsic_local_priv *lpriv = this_cpu_ptr(imsic->lpriv);
> +#endif
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&imsic->matrix_lock, flags);
> + irq_matrix_offline(imsic->matrix);
> + raw_spin_unlock_irqrestore(&imsic->matrix_lock, flags);
> +
> +#ifdef CONFIG_SMP
> + raw_spin_lock_irqsave(&lpriv->lock, flags);
> + WARN_ON_ONCE(try_to_del_timer_sync(&lpriv->timer) < 0);
> + raw_spin_unlock_irqrestore(&lpriv->lock, flags);
> +#endif
> +}
> +
> +static int __init imsic_matrix_init(void)
> +{
> + struct imsic_global_config *global = &imsic->global;
> +
> + raw_spin_lock_init(&imsic->matrix_lock);
> + imsic->matrix = irq_alloc_matrix(global->nr_ids + 1,
> + 0, global->nr_ids + 1);
> + if (!imsic->matrix)
> + return -ENOMEM;
> +
> + /* Reserve ID#0 because it is special and never implemented */
> + irq_matrix_assign_system(imsic->matrix, 0, false);
> +
> + /* Reserve IPI ID because it is special and used internally */
> + irq_matrix_assign_system(imsic->matrix, IMSIC_IPI_ID, false);
> +
> + return 0;
> +}
> +
> +static int __init imsic_get_parent_hartid(struct fwnode_handle *fwnode,
> + u32 index, unsigned long *hartid)
> +{
> + struct of_phandle_args parent;
> + int rc;
> +
> + /*
> + * Currently, only OF fwnode is supported so extend this
> + * function for ACPI support.
> + */
> + if (!is_of_node(fwnode))
> + return -EINVAL;
> +
> + rc = of_irq_parse_one(to_of_node(fwnode), index, &parent);
> + if (rc)
> + return rc;
> +
> + /*
> + * Skip interrupts other than external interrupts for
> + * current privilege level.
> + */
> + if (parent.args[0] != RV_IRQ_EXT)
> + return -EINVAL;
> +
> + return riscv_of_parent_hartid(parent.np, hartid);
> +}
> +
> +static int __init imsic_get_mmio_resource(struct fwnode_handle *fwnode,
> + u32 index, struct resource *res)
> +{
> + /*
> + * Currently, only OF fwnode is supported so extend this
> + * function for ACPI support.
> + */
> + if (!is_of_node(fwnode))
> + return -EINVAL;
> +
> + return of_address_to_resource(to_of_node(fwnode), index, res);
> +}
> +
> +static int __init imsic_parse_fwnode(struct fwnode_handle *fwnode,
> + struct imsic_global_config *global,
> + u32 *nr_parent_irqs,
> + u32 *nr_mmios)
> +{
> + unsigned long hartid;
> + struct resource res;
> + int rc;
> + u32 i;
> +
> + /*
> + * Currently, only OF fwnode is supported so extend this
> + * function for ACPI support.
> + */
> + if (!is_of_node(fwnode))
> + return -EINVAL;
> +
> + *nr_parent_irqs = 0;
> + *nr_mmios = 0;
> +
> + /* Find number of parent interrupts */
> + while (!imsic_get_parent_hartid(fwnode, *nr_parent_irqs, &hartid))
> + (*nr_parent_irqs)++;
> + if (!(*nr_parent_irqs)) {
Nit: Redundant parenthesis
> + pr_err("%pfwP: no parent irqs available\n", fwnode);
> + return -EINVAL;
> + }
> +
> + /* Find number of guest index bits in MSI address */
> + rc = of_property_read_u32(to_of_node(fwnode), "riscv,guest-index-bits",
> + &global->guest_index_bits);
> + if (rc)
> + global->guest_index_bits = 0;
> +
> + /* Find number of HART index bits */
> + rc = of_property_read_u32(to_of_node(fwnode), "riscv,hart-index-bits",
> + &global->hart_index_bits);
> + if (rc) {
> + /* Assume default value */
> + global->hart_index_bits = __fls(*nr_parent_irqs);
> + if (BIT(global->hart_index_bits) < *nr_parent_irqs)
> + global->hart_index_bits++;
> + }
> +
> + /* Find number of group index bits */
> + rc = of_property_read_u32(to_of_node(fwnode), "riscv,group-index-bits",
> + &global->group_index_bits);
> + if (rc)
> + global->group_index_bits = 0;
> +
> + /*
> + * Find first bit position of group index.
> + * If not specified assumed the default APLIC-IMSIC configuration.
> + */
> + rc = of_property_read_u32(to_of_node(fwnode), "riscv,group-index-shift",
> + &global->group_index_shift);
> + if (rc)
> + global->group_index_shift = IMSIC_MMIO_PAGE_SHIFT * 2;
> +
> + /* Find number of interrupt identities */
> + rc = of_property_read_u32(to_of_node(fwnode), "riscv,num-ids",
> + &global->nr_ids);
> + if (rc) {
> + pr_err("%pfwP: number of interrupt identities not found\n",
> + fwnode);
> + return rc;
> + }
> +
> + /* Find number of guest interrupt identities */
> + rc = of_property_read_u32(to_of_node(fwnode), "riscv,num-guest-ids",
> + &global->nr_guest_ids);
> + if (rc)
> + global->nr_guest_ids = global->nr_ids;
> +
> + /* Sanity check guest index bits */
> + i = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT;
> + if (i < global->guest_index_bits) {
> + pr_err("%pfwP: guest index bits too big\n", fwnode);
> + return -EINVAL;
> + }
> +
> + /* Sanity check HART index bits */
> + i = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT - global->guest_index_bits;
> + if (i < global->hart_index_bits) {
> + pr_err("%pfwP: HART index bits too big\n", fwnode);
> + return -EINVAL;
> + }
> +
> + /* Sanity check group index bits */
> + i = BITS_PER_LONG - IMSIC_MMIO_PAGE_SHIFT -
> + global->guest_index_bits - global->hart_index_bits;
> + if (i < global->group_index_bits) {
> + pr_err("%pfwP: group index bits too big\n", fwnode);
> + return -EINVAL;
> + }
> +
> + /* Sanity check group index shift */
> + i = global->group_index_bits + global->group_index_shift - 1;
> + if (i >= BITS_PER_LONG) {
> + pr_err("%pfwP: group index shift too big\n", fwnode);
> + return -EINVAL;
> + }
> +
> + /* Sanity check number of interrupt identities */
> + if ((global->nr_ids < IMSIC_MIN_ID) ||
> + (global->nr_ids >= IMSIC_MAX_ID) ||
> + ((global->nr_ids & IMSIC_MIN_ID) != IMSIC_MIN_ID)) {
> + pr_err("%pfwP: invalid number of interrupt identities\n",
> + fwnode);
Nit: 100 chars
> + return -EINVAL;
> + }
> +
> + /* Sanity check number of guest interrupt identities */
> + if ((global->nr_guest_ids < IMSIC_MIN_ID) ||
> + (global->nr_guest_ids >= IMSIC_MAX_ID) ||
> + ((global->nr_guest_ids & IMSIC_MIN_ID) != IMSIC_MIN_ID)) {
> + pr_err("%pfwP: invalid number of guest interrupt identities\n",
> + fwnode);
Nit: 100 chars
> + return -EINVAL;
> + }
> +
> + /* Compute base address */
> + rc = imsic_get_mmio_resource(fwnode, 0, &res);
> + if (rc) {
> + pr_err("%pfwP: first MMIO resource not found\n", fwnode);
> + return -EINVAL;
> + }
> + global->base_addr = res.start;
> + global->base_addr &= ~(BIT(global->guest_index_bits +
> + global->hart_index_bits +
> + IMSIC_MMIO_PAGE_SHIFT) - 1);
> + global->base_addr &= ~((BIT(global->group_index_bits) - 1) <<
> + global->group_index_shift);
> +
> + /* Find number of MMIO register sets */
> + while (!imsic_get_mmio_resource(fwnode, *nr_mmios, &res))
> + (*nr_mmios)++;
> +
> + return 0;
> +}
> +
> +int __init imsic_setup_state(struct fwnode_handle *fwnode)
> +{
> + u32 i, j, index, nr_parent_irqs, nr_mmios, nr_handlers = 0;
> + struct imsic_global_config *global;
> + struct imsic_local_config *local;
> + void __iomem **mmios_va = NULL;
> + struct resource *mmios = NULL;
> + unsigned long reloff, hartid;
> + phys_addr_t base_addr;
> + int rc, cpu;
> +
> + /*
> + * Only one IMSIC instance allowed in a platform for clean
> + * implementation of SMP IRQ affinity and per-CPU IPIs.
> + *
> + * This means on a multi-socket (or multi-die) platform we
> + * will have multiple MMIO regions for one IMSIC instance.
> + */
> + if (imsic) {
> + pr_err("%pfwP: already initialized hence ignoring\n",
> + fwnode);
Nit: 100 chars
> + return -EALREADY;
> + }
> +
> + if (!riscv_isa_extension_available(NULL, SxAIA)) {
> + pr_err("%pfwP: AIA support not available\n", fwnode);
> + return -ENODEV;
> + }
> +
> + imsic = kzalloc(sizeof(*imsic), GFP_KERNEL);
> + if (!imsic)
> + return -ENOMEM;
> + imsic->fwnode = fwnode;
> + global = &imsic->global;
> +
> + global->local = alloc_percpu(typeof(*(global->local)));
> + if (!global->local) {
> + rc = -ENOMEM;
> + goto out_free_priv;
> + }
> +
> + /* Parse IMSIC fwnode */
> + rc = imsic_parse_fwnode(fwnode, global, &nr_parent_irqs, &nr_mmios);
> + if (rc)
> + goto out_free_local;
> +
> + /* Allocate MMIO resource array */
> + mmios = kcalloc(nr_mmios, sizeof(*mmios), GFP_KERNEL);
> + if (!mmios) {
> + rc = -ENOMEM;
> + goto out_free_local;
> + }
> +
> + /* Allocate MMIO virtual address array */
> + mmios_va = kcalloc(nr_mmios, sizeof(*mmios_va), GFP_KERNEL);
> + if (!mmios_va) {
> + rc = -ENOMEM;
> + goto out_iounmap;
> + }
> +
> + /* Parse and map MMIO register sets */
> + for (i = 0; i < nr_mmios; i++) {
> + rc = imsic_get_mmio_resource(fwnode, i, &mmios[i]);
> + if (rc) {
> + pr_err("%pfwP: unable to parse MMIO regset %d\n",
> + fwnode, i);
Nit: 100 chars
> + goto out_iounmap;
> + }
> +
> + base_addr = mmios[i].start;
> + base_addr &= ~(BIT(global->guest_index_bits +
> + global->hart_index_bits +
> + IMSIC_MMIO_PAGE_SHIFT) - 1);
> + base_addr &= ~((BIT(global->group_index_bits) - 1) <<
> + global->group_index_shift);
> + if (base_addr != global->base_addr) {
> + rc = -EINVAL;
> + pr_err("%pfwP: address mismatch for regset %d\n",
> + fwnode, i);
Nit: 100 chars... and all the places below where applicable.
Björn