Received-SPF: pass (google.com: domain of linux-kernel+bounces-72949-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
To: Anup Patel <apatel@ventanamicro.com>, Palmer Dabbelt
 <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Thomas
 Gleixner <tglx@linutronix.de>, Rob Herring <robh+dt@kernel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Frank Rowand
 <frowand.list@gmail.com>, Conor Dooley <conor+dt@kernel.org>
Cc: Anup Patel <apatel@ventanamicro.com>, devicetree@vger.kernel.org,
 Saravana Kannan <saravanak@google.com>, Marc Zyngier <maz@kernel.org>,
 Anup Patel <anup@brainfault.org>, linux-kernel@vger.kernel.org, Atish
 Patra <atishp@atishpatra.org>, linux-riscv@lists.infradead.org,
 linux-arm-kernel@lists.infradead.org, Andrew Jones
 <ajones@ventanamicro.com>
Subject: Re: [PATCH v13 06/13] irqchip: Add RISC-V incoming MSI controller
 early driver
In-Reply-To: <20240220060718.823229-7-apatel@ventanamicro.com>
References: <20240220060718.823229-1-apatel@ventanamicro.com>
 <20240220060718.823229-7-apatel@ventanamicro.com>
Date: Tue, 20 Feb 2024 12:52:42 +0100
Message-ID: <87frxnfj3p.fsf@all.your.base.are.belong.to.us>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

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@ventanamicro.com> 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@ventanamicro.com>
> ---
>  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
>=20=20
> +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)			+=3D irq-qcom-mpm.o
>  obj-$(CONFIG_CSKY_MPINTC)		+=3D irq-csky-mpintc.o
>  obj-$(CONFIG_CSKY_APB_INTC)		+=3D irq-csky-apb-intc.o
>  obj-$(CONFIG_RISCV_INTC)		+=3D irq-riscv-intc.o
> +obj-$(CONFIG_RISCV_IMSIC)		+=3D irq-riscv-imsic-state.o irq-riscv-imsic-=
early.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+=3D irq-sifive-plic.o
>  obj-$(CONFIG_IMX_IRQSTEER)		+=3D irq-imx-irqsteer.o
>  obj-$(CONFIG_IMX_INTMUX)		+=3D irq-imx-intmux.o
> diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/ir=
q-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 =3D per_cpu_ptr(imsic->global.local, c=
pu);
> +
> +	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 =3D ipi_mux_create(IMSIC_NR_IPI, imsic_ipi_send);
> +	if (virq <=3D 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, IM=
SIC_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 =3D irq_desc_get_chip(desc);
> +	int err, cpu =3D smp_processor_id();
> +	struct imsic_vector *vec;
> +	unsigned long local_id;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	while ((local_id =3D csr_swap(CSR_TOPEI, 0))) {
> +		local_id =3D local_id >> TOPEI_ID_SHIFT;

Nit: Wdyt about moving shift into the loop predicate, or using >>=3D?

> +
> +		if (local_id =3D=3D 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 =3D 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 =3D 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_i=
rq));
> +
> +	/* 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 =3D 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 =3D 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 =3D 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 =3D &node->fwnode;
> +	int rc;
> +
> +	/* Setup IMSIC state */
> +	rc =3D 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 =3D 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/ir=
q-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, unsi=
gned 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 =3D id / BITS_PER_LONG;
> +	isel *=3D BITS_PER_LONG / IMSIC_EIPx_BITS;
> +	isel +=3D pend ? IMSIC_EIP0 : IMSIC_EIE0;
> +	imask =3D 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, boo=
l pend, bool val)
> +{
> +	unsigned long id =3D base_id, last_id =3D base_id + num_id;
> +	unsigned long i, isel, ireg;
> +
> +	while (id < last_id) {
> +		isel =3D id / BITS_PER_LONG;
> +		isel *=3D BITS_PER_LONG / IMSIC_EIPx_BITS;
> +		isel +=3D (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 >=3D (base_id + num_id).
> +		 */
> +		ireg =3D 0;
> +		for (i =3D id & (__riscv_xlen - 1); (id < last_id) && (i < __riscv_xle=
n); i++) {

Nit: Redundant parenthesis "(id < last_id) && (i < __riscv_xlen)", which
is also inconsistent with other usage in this changeset.

> +			ireg |=3D 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 =3D=3D IMSIC_IPI_ID)
> +			goto skip;
> +		vec =3D &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 =3D vec->move;
> +		vec->move =3D NULL;
> +		if (mvec && mvec !=3D vec) {
> +			if (__imsic_id_read_clear_pending(i)) {
> +				mlocal =3D 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 =3D 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 =3D 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 !=3D smp_processor_id()) {
> +			if (!timer_pending(&lpriv->timer)) {
> +				lpriv->timer.expires =3D 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 =3D per_cpu_ptr(imsic->lpriv, vec->cpu);
> +	if (WARN_ON(&lpriv->vectors[vec->local_id] !=3D 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 =3D 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@all.your.base.are.belong=
to.us/

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 =3D per_cpu_ptr(imsic->lpriv, vec->cpu);
> +	if (WARN_ON(&lpriv->vectors[vec->local_id] !=3D 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 =3D 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 =3D per_cpu_ptr(imsic->lpriv, vec->cpu);
> +	unsigned long flags;
> +	bool ret;
> +
> +	raw_spin_lock_irqsave(&lpriv->lock, flags);
> +	ret =3D 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 =3D per_cpu_ptr(imsic->lpriv, vec->cpu);
> +	struct imsic_vector *ret;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&lpriv->lock, flags);
> +	ret =3D vec->move;
> +	raw_spin_unlock_irqrestore(&lpriv->lock, flags);
> +
> +	return ret;
> +}
> +
> +static bool imsic_vector_move_update(struct imsic_local_priv *lpriv, str=
uct 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 =3D vec->enable;
> +	vec->enable =3D new_enable;
> +	vec->move =3D 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 =3D=3D new_vec->cpu))
> +		return;
> +
> +	old_lpriv =3D per_cpu_ptr(imsic->lpriv, old_vec->cpu);
> +	if (WARN_ON(&old_lpriv->vectors[old_vec->local_id] !=3D old_vec))
> +		return;
> +
> +	new_lpriv =3D per_cpu_ptr(imsic->lpriv, new_vec->cpu);
> +	if (WARN_ON(&new_lpriv->vectors[new_vec->local_id] !=3D 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 =3D 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 *ve=
c, int ind)
> +{
> +	struct imsic_local_priv *lpriv;
> +	struct imsic_vector *mvec;
> +	bool is_enabled;
> +
> +	lpriv =3D per_cpu_ptr(imsic->lpriv, vec->cpu);
> +	if (WARN_ON(&lpriv->vectors[vec->local_id] !=3D vec))
> +		return;
> +
> +	is_enabled =3D imsic_vector_isenabled(vec);
> +	mvec =3D 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 <=3D 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, unsign=
ed int local_id)
> +{
> +	struct imsic_local_priv *lpriv =3D 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 =3D 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 =3D irq_matrix_alloc(imsic->matrix, mask, false, &cpu);
> +	raw_spin_unlock_irqrestore(&imsic->matrix_lock, flags);
> +	if (local_id < 0)
> +		return NULL;
> +
> +	lpriv =3D per_cpu_ptr(imsic->lpriv, cpu);
> +	vec =3D &lpriv->vectors[local_id];
> +	vec->hwirq =3D hwirq;
> +	vec->enable =3D false;
> +	vec->move =3D NULL;
> +
> +	return vec;
> +}
> +
> +void imsic_vector_free(struct imsic_vector *vec)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&imsic->matrix_lock, flags);
> +	vec->hwirq =3D 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 =3D 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 =3D &imsic->global;
> +	struct imsic_local_priv *lpriv;
> +	struct imsic_vector *vec;
> +	int cpu, i;
> +
> +	/* Allocate per-CPU private state */
> +	imsic->lpriv =3D alloc_percpu(typeof(*(imsic->lpriv)));
> +	if (!imsic->lpriv)
> +		return -ENOMEM;
> +
> +	/* Setup per-CPU private state */
> +	for_each_possible_cpu(cpu) {
> +		lpriv =3D per_cpu_ptr(imsic->lpriv, cpu);
> +
> +		raw_spin_lock_init(&lpriv->lock);
> +
> +		/* Allocate dirty bitmap */
> +		lpriv->dirty_bitmap =3D 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 =3D kcalloc(global->nr_ids + 1, sizeof(*lpriv->vectors),
> +					 GFP_KERNEL);
> +		if (!lpriv->vectors)
> +			goto fail_local_cleanup;
> +
> +		/* Setup vector array */
> +		for (i =3D 0; i <=3D global->nr_ids; i++) {
> +			vec =3D &lpriv->vectors[i];
> +			vec->cpu =3D cpu;
> +			vec->local_id =3D i;
> +			vec->hwirq =3D 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 =3D 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 =3D &imsic->global;
> +
> +	raw_spin_lock_init(&imsic->matrix_lock);
> +	imsic->matrix =3D 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 =3D 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] !=3D 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 =3D 0;
> +	*nr_mmios =3D 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 =3D of_property_read_u32(to_of_node(fwnode), "riscv,guest-index-bits=
",
> +				  &global->guest_index_bits);
> +	if (rc)
> +		global->guest_index_bits =3D 0;
> +
> +	/* Find number of HART index bits */
> +	rc =3D 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 =3D __fls(*nr_parent_irqs);
> +		if (BIT(global->hart_index_bits) < *nr_parent_irqs)
> +			global->hart_index_bits++;
> +	}
> +
> +	/* Find number of group index bits */
> +	rc =3D of_property_read_u32(to_of_node(fwnode), "riscv,group-index-bits=
",
> +				  &global->group_index_bits);
> +	if (rc)
> +		global->group_index_bits =3D 0;
> +
> +	/*
> +	 * Find first bit position of group index.
> +	 * If not specified assumed the default APLIC-IMSIC configuration.
> +	 */
> +	rc =3D of_property_read_u32(to_of_node(fwnode), "riscv,group-index-shif=
t",
> +				  &global->group_index_shift);
> +	if (rc)
> +		global->group_index_shift =3D IMSIC_MMIO_PAGE_SHIFT * 2;
> +
> +	/* Find number of interrupt identities */
> +	rc =3D 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 =3D of_property_read_u32(to_of_node(fwnode), "riscv,num-guest-ids",
> +				  &global->nr_guest_ids);
> +	if (rc)
> +		global->nr_guest_ids =3D global->nr_ids;
> +
> +	/* Sanity check guest index bits */
> +	i =3D 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 =3D 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 =3D 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 =3D global->group_index_bits + global->group_index_shift - 1;
> +	if (i >=3D 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 >=3D IMSIC_MAX_ID) ||
> +	    ((global->nr_ids & IMSIC_MIN_ID) !=3D 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 >=3D IMSIC_MAX_ID) ||
> +	    ((global->nr_guest_ids & IMSIC_MIN_ID) !=3D IMSIC_MIN_ID)) {
> +		pr_err("%pfwP: invalid number of guest interrupt identities\n",
> +			fwnode);

Nit: 100 chars

> +		return -EINVAL;
> +	}
> +
> +	/* Compute base address */
> +	rc =3D imsic_get_mmio_resource(fwnode, 0, &res);
> +	if (rc) {
> +		pr_err("%pfwP: first MMIO resource not found\n", fwnode);
> +		return -EINVAL;
> +	}
> +	global->base_addr =3D res.start;
> +	global->base_addr &=3D ~(BIT(global->guest_index_bits +
> +				   global->hart_index_bits +
> +				   IMSIC_MMIO_PAGE_SHIFT) - 1);
> +	global->base_addr &=3D ~((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 =3D 0;
> +	struct imsic_global_config *global;
> +	struct imsic_local_config *local;
> +	void __iomem **mmios_va =3D NULL;
> +	struct resource *mmios =3D 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 =3D kzalloc(sizeof(*imsic), GFP_KERNEL);
> +	if (!imsic)
> +		return -ENOMEM;
> +	imsic->fwnode =3D fwnode;
> +	global =3D &imsic->global;
> +
> +	global->local =3D alloc_percpu(typeof(*(global->local)));
> +	if (!global->local) {
> +		rc =3D -ENOMEM;
> +		goto out_free_priv;
> +	}
> +
> +	/* Parse IMSIC fwnode */
> +	rc =3D imsic_parse_fwnode(fwnode, global, &nr_parent_irqs, &nr_mmios);
> +	if (rc)
> +		goto out_free_local;
> +
> +	/* Allocate MMIO resource array */
> +	mmios =3D kcalloc(nr_mmios, sizeof(*mmios), GFP_KERNEL);
> +	if (!mmios) {
> +		rc =3D -ENOMEM;
> +		goto out_free_local;
> +	}
> +
> +	/* Allocate MMIO virtual address array */
> +	mmios_va =3D kcalloc(nr_mmios, sizeof(*mmios_va), GFP_KERNEL);
> +	if (!mmios_va) {
> +		rc =3D -ENOMEM;
> +		goto out_iounmap;
> +	}
> +
> +	/* Parse and map MMIO register sets */
> +	for (i =3D 0; i < nr_mmios; i++) {
> +		rc =3D 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 =3D mmios[i].start;
> +		base_addr &=3D ~(BIT(global->guest_index_bits +
> +				   global->hart_index_bits +
> +				   IMSIC_MMIO_PAGE_SHIFT) - 1);
> +		base_addr &=3D ~((BIT(global->group_index_bits) - 1) <<
> +			       global->group_index_shift);
> +		if (base_addr !=3D global->base_addr) {
> +			rc =3D -EINVAL;
> +			pr_err("%pfwP: address mismatch for regset %d\n",
> +				fwnode, i);

Nit: 100 chars... and all the places below where applicable.


Bj=C3=B6rn