From: Yann Sionneau <ysionneau@kalrayinc.com>

Each Cluster of the Coolidge SoC includes an Advanced Programmable
Interrupt Controller (APIC) and Generic Interrupt Controller (GIC).

The APIC GIC acts as an intermediary interrupt controller, muxing/routing
incoming interrupts to cores in the cluster.

The 139 possible input interrupt lines are organized as follows:
 - 128 from the mailbox controller (one it per mailboxes)
 - 1   from the NoC router
 - 5   from IOMMUs
 - 1   from L2 cache DMA job FIFO
 - 1   from cluster watchdog
 - 2   for SECC, DECC
 - 1   from Data NoC

The 72 possible output interrupt lines:
 -  68: 4 interrupts per core (17 cores)
 -  1 for L2 cache controller
 -  3 extra that are for padding

Co-developed-by: Clement Leger <clement@clement-leger.fr>
Signed-off-by: Clement Leger <clement@clement-leger.fr>
Co-developed-by: Julian Vetter <jvetter@kalrayinc.com>
Signed-off-by: Julian Vetter <jvetter@kalrayinc.com>
Co-developed-by: Vincent Chardon <vincent.chardon@elsys-design.com>
Signed-off-by: Vincent Chardon <vincent.chardon@elsys-design.com>
Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>
---

Notes:
V1 -> V2:
- removed irq-kvx-itgen driver (moved in its own patch)
- removed irq-kvx-apic-mailbox driver (moved in its own patch)
- removed irq-kvx-core-intc driver (moved in its own patch)
- removed print on probe success

V2 -> V3: update compatible
---
 drivers/irqchip/Kconfig            |   6 +
 drivers/irqchip/Makefile           |   1 +
 drivers/irqchip/irq-kvx-apic-gic.c | 356 +++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 drivers/irqchip/irq-kvx-apic-gic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 14464716bacbb..566425731b757 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -332,6 +332,12 @@ config MIPS_GIC
 	select IRQ_DOMAIN_HIERARCHY
 	select MIPS_CM
 
+config KVX_APIC_GIC
+	bool
+	depends on KVX
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+
 config INGENIC_IRQ
 	bool
 	depends on MACH_INGENIC
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index d9dc3d99aaa86..f59255947c8ed 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
 obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
 obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
 obj-$(CONFIG_MIPS_GIC)			+= irq-mips-gic.o
+obj-$(CONFIG_KVX_APIC_GIC)		+= irq-kvx-apic-gic.o
 obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o irq-mtk-cirq.o
 obj-$(CONFIG_ARCH_DIGICOLOR)		+= irq-digicolor.o
 obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
diff --git a/drivers/irqchip/irq-kvx-apic-gic.c b/drivers/irqchip/irq-kvx-apic-gic.c
new file mode 100644
index 0000000000000..95196bd7ef058
--- /dev/null
+++ b/drivers/irqchip/irq-kvx-apic-gic.c
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2017 - 2022 Kalray Inc.
+ * Author(s): Clement Leger
+ *            Julian Vetter
+ */
+
+#define pr_fmt(fmt)	"kvx_apic_gic: " fmt
+
+#include <linux/of_address.h>
+#include <linux/cpuhotplug.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/spinlock.h>
+#include <linux/irqchip.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/of.h>
+
+/* APIC is organized in 18 groups of 4 output lines
+ * However, the two upper lines are for Secure RM and DMA engine
+ * Thus, we do not have to use them
+ */
+#define KVX_GIC_PER_CPU_IT_COUNT	4
+#define KVX_GIC_INPUT_IT_COUNT		0x9D
+#define KVX_GIC_OUTPUT_IT_COUNT		0x10
+
+/* GIC enable register definitions */
+#define KVX_GIC_ENABLE_OFFSET		0x0
+#define KVX_GIC_ENABLE_ELEM_SIZE	0x1
+#define KVX_GIC_ELEM_SIZE		0x400
+
+/* GIC status lac register definitions */
+#define KVX_GIC_STATUS_LAC_OFFSET	0x120
+#define KVX_GIC_STATUS_LAC_ELEM_SIZE	0x8
+#define KVX_GIC_STATUS_LAC_ARRAY_SIZE	0x3
+
+/**
+ * For each CPU, there is 4 output lines coming from the apic GIC.
+ * We only use 1 line and this structure represent this line.
+ * @base Output line base address
+ * @cpu CPU associated to this line
+ */
+struct gic_out_irq_line {
+	void __iomem *base;
+	unsigned int cpu;
+};
+
+/**
+ * Input irq line.
+ * This structure is used to store the status of the input line and the
+ * associated output line.
+ * @enabled Boolean for line status
+ * @cpu CPU currently receiving this interrupt
+ * @it_num Interrupt number
+ */
+struct gic_in_irq_line {
+	bool enabled;
+	struct gic_out_irq_line *out_line;
+	unsigned int it_num;
+};
+
+/**
+ * struct kvx_apic_gic - kvx apic gic
+ * @base: Base address of the controller
+ * @domain Domain for this controller
+ * @input_nr_irqs: maximum number of supported input interrupts
+ * @cpus: Per cpu interrupt configuration
+ * @output_irq: Array of output irq lines
+ * @input_irq: Array of input irq lines
+ */
+struct kvx_apic_gic {
+	raw_spinlock_t lock;
+	void __iomem *base;
+	struct irq_domain *domain;
+	uint32_t input_nr_irqs;
+	/* For each cpu, there is an output IT line */
+	struct gic_out_irq_line output_irq[KVX_GIC_OUTPUT_IT_COUNT];
+	/* Input interrupt status */
+	struct gic_in_irq_line input_irq[KVX_GIC_INPUT_IT_COUNT];
+};
+
+static int gic_parent_irq;
+
+/**
+ * Enable/Disable an output irq line
+ * This function is used by both mask/unmask to disable/enable the line.
+ */
+static void irq_line_set_enable(struct gic_out_irq_line *irq_line,
+				struct gic_in_irq_line *in_irq_line,
+				int enable)
+{
+	void __iomem *enable_line_addr = irq_line->base +
+	       KVX_GIC_ENABLE_OFFSET +
+	       in_irq_line->it_num * KVX_GIC_ENABLE_ELEM_SIZE;
+
+	writeb((uint8_t) enable ? 1 : 0, enable_line_addr);
+	in_irq_line->enabled = enable;
+}
+
+static void kvx_apic_gic_set_line(struct irq_data *data, int enable)
+{
+	struct kvx_apic_gic *gic = irq_data_get_irq_chip_data(data);
+	unsigned int in_irq = irqd_to_hwirq(data);
+	struct gic_in_irq_line *in_line = &gic->input_irq[in_irq];
+	struct gic_out_irq_line *out_line = in_line->out_line;
+
+	raw_spin_lock(&gic->lock);
+	/* Set line enable on currently assigned cpu */
+	irq_line_set_enable(out_line, in_line, enable);
+	raw_spin_unlock(&gic->lock);
+}
+
+static void kvx_apic_gic_mask(struct irq_data *data)
+{
+	kvx_apic_gic_set_line(data, 0);
+}
+
+static void kvx_apic_gic_unmask(struct irq_data *data)
+{
+	kvx_apic_gic_set_line(data, 1);
+}
+
+#ifdef CONFIG_SMP
+
+static int kvx_apic_gic_set_affinity(struct irq_data *d,
+				     const struct cpumask *cpumask,
+				     bool force)
+{
+	struct kvx_apic_gic *gic = irq_data_get_irq_chip_data(d);
+	unsigned int new_cpu;
+	unsigned int hw_irq = irqd_to_hwirq(d);
+	struct gic_in_irq_line *input_line = &gic->input_irq[hw_irq];
+	struct gic_out_irq_line *new_out_line;
+
+	/* We assume there is only one cpu in the mask */
+	new_cpu = cpumask_first(cpumask);
+	new_out_line = &gic->output_irq[new_cpu];
+
+	raw_spin_lock(&gic->lock);
+
+	/* Nothing to do, line is the same */
+	if (new_out_line == input_line->out_line)
+		goto out;
+
+	/* If old line was enabled, enable the new one before disabling
+	 * the old one
+	 */
+	if (input_line->enabled)
+		irq_line_set_enable(new_out_line, input_line, 1);
+
+	/* Disable it on old line */
+	irq_line_set_enable(input_line->out_line, input_line, 0);
+
+	/* Assign new output line to input IRQ */
+	input_line->out_line = new_out_line;
+
+out:
+	raw_spin_unlock(&gic->lock);
+
+	irq_data_update_effective_affinity(d, cpumask_of(new_cpu));
+
+	return IRQ_SET_MASK_OK;
+}
+#endif
+
+static struct irq_chip kvx_apic_gic_chip = {
+	.name           = "kvx apic gic",
+	.irq_mask	= kvx_apic_gic_mask,
+	.irq_unmask	= kvx_apic_gic_unmask,
+#ifdef CONFIG_SMP
+	.irq_set_affinity = kvx_apic_gic_set_affinity,
+#endif
+};
+
+static int kvx_apic_gic_alloc(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs, void *args)
+{
+	int i;
+	struct irq_fwspec *fwspec = args;
+	int hwirq = fwspec->param[0];
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_domain_set_info(domain, virq + i, hwirq + i,
+				    &kvx_apic_gic_chip,
+				    domain->host_data, handle_simple_irq,
+				    NULL, NULL);
+	}
+
+	return 0;
+}
+
+static const struct irq_domain_ops kvx_apic_gic_domain_ops = {
+	.alloc  = kvx_apic_gic_alloc,
+	.free   = irq_domain_free_irqs_common,
+};
+
+static void irq_line_get_status_lac(struct gic_out_irq_line *out_irq_line,
+			uint64_t status[KVX_GIC_STATUS_LAC_ARRAY_SIZE])
+{
+	int i;
+
+	for (i = 0; i < KVX_GIC_STATUS_LAC_ARRAY_SIZE; i++) {
+		status[i] = readq(out_irq_line->base +
+				  KVX_GIC_STATUS_LAC_OFFSET +
+				  i * KVX_GIC_STATUS_LAC_ELEM_SIZE);
+	}
+}
+
+static void kvx_apic_gic_handle_irq(struct irq_desc *desc)
+{
+	struct kvx_apic_gic *gic_data = irq_desc_get_handler_data(desc);
+	struct gic_out_irq_line *out_line;
+	uint64_t status[KVX_GIC_STATUS_LAC_ARRAY_SIZE];
+	unsigned long irqn, cascade_irq;
+	unsigned long cpu = smp_processor_id();
+
+	out_line = &gic_data->output_irq[cpu];
+
+	irq_line_get_status_lac(out_line, status);
+
+	for_each_set_bit(irqn, (unsigned long *) status,
+			KVX_GIC_STATUS_LAC_ARRAY_SIZE * BITS_PER_LONG) {
+
+		cascade_irq = irq_find_mapping(gic_data->domain, irqn);
+
+		generic_handle_irq(cascade_irq);
+	}
+}
+
+static void __init apic_gic_init(struct kvx_apic_gic *gic)
+{
+	unsigned int cpu, line;
+	struct gic_in_irq_line *input_irq_line;
+	struct gic_out_irq_line *output_irq_line;
+	uint64_t status[KVX_GIC_STATUS_LAC_ARRAY_SIZE];
+
+	/* Initialize all input lines (device -> )*/
+	for (line = 0; line < KVX_GIC_INPUT_IT_COUNT; line++) {
+		input_irq_line = &gic->input_irq[line];
+		input_irq_line->enabled = false;
+		/* All input lines map on output 0 */
+		input_irq_line->out_line = &gic->output_irq[0];
+		input_irq_line->it_num = line;
+	}
+
+	/* Clear all output lines (-> cpus) */
+	for (cpu = 0; cpu < KVX_GIC_OUTPUT_IT_COUNT; cpu++) {
+		output_irq_line = &gic->output_irq[cpu];
+		output_irq_line->cpu = cpu;
+		output_irq_line->base = gic->base +
+			cpu * (KVX_GIC_ELEM_SIZE * KVX_GIC_PER_CPU_IT_COUNT);
+
+		/* Disable all external lines on this core */
+		for (line = 0; line < KVX_GIC_INPUT_IT_COUNT; line++)
+			irq_line_set_enable(output_irq_line,
+					&gic->input_irq[line], 0x0);
+
+		irq_line_get_status_lac(output_irq_line, status);
+	}
+}
+
+static int kvx_gic_starting_cpu(unsigned int cpu)
+{
+	enable_percpu_irq(gic_parent_irq, IRQ_TYPE_NONE);
+
+	return 0;
+}
+
+static int kvx_gic_dying_cpu(unsigned int cpu)
+{
+	disable_percpu_irq(gic_parent_irq);
+
+	return 0;
+}
+
+static int __init kvx_init_apic_gic(struct device_node *node,
+				    struct device_node *parent)
+{
+	struct kvx_apic_gic *gic;
+	int ret;
+	unsigned int irq;
+
+	if (!parent) {
+		pr_err("kvx apic gic does not have parent\n");
+		return -EINVAL;
+	}
+
+	gic = kzalloc(sizeof(*gic), GFP_KERNEL);
+	if (!gic)
+		return -ENOMEM;
+
+	if (of_property_read_u32(node, "kalray,intc-nr-irqs",
+						&gic->input_nr_irqs))
+		gic->input_nr_irqs = KVX_GIC_INPUT_IT_COUNT;
+
+	if (WARN_ON(gic->input_nr_irqs > KVX_GIC_INPUT_IT_COUNT)) {
+		ret = -EINVAL;
+		goto err_kfree;
+	}
+
+	gic->base = of_io_request_and_map(node, 0, node->name);
+	if (!gic->base) {
+		ret = -EINVAL;
+		goto err_kfree;
+	}
+
+	raw_spin_lock_init(&gic->lock);
+	apic_gic_init(gic);
+
+	gic->domain = irq_domain_add_linear(node,
+					gic->input_nr_irqs,
+					&kvx_apic_gic_domain_ops,
+					gic);
+	if (!gic->domain) {
+		pr_err("Failed to add IRQ domain\n");
+		ret = -EINVAL;
+		goto err_iounmap;
+	}
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0) {
+		pr_err("unable to parse irq\n");
+		ret = -EINVAL;
+		goto err_irq_domain_remove;
+	}
+
+	irq_set_chained_handler_and_data(irq, kvx_apic_gic_handle_irq,
+								gic);
+
+	gic_parent_irq = irq;
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+				"kvx/gic:online",
+				kvx_gic_starting_cpu,
+				kvx_gic_dying_cpu);
+	if (ret < 0) {
+		pr_err("Failed to setup hotplug state");
+		goto err_irq_unmap;
+	}
+
+	return 0;
+
+err_irq_unmap:
+	irq_dispose_mapping(irq);
+err_irq_domain_remove:
+	irq_domain_remove(gic->domain);
+err_iounmap:
+	iounmap(gic->base);
+err_kfree:
+	kfree(gic);
+
+	return ret;
+}
+
+IRQCHIP_DECLARE(kvx_apic_gic, "kalray,coolidge-apic-gic", kvx_init_apic_gic);
-- 
2.45.2

Yann!

On Mon, Jul 22 2024 at 11:41, ysionneau@kalrayinc.com wrote:
> +
> +/**
> + * For each CPU, there is 4 output lines coming from the apic GIC.
> + * We only use 1 line and this structure represent this line.
> + * @base Output line base address
> + * @cpu CPU associated to this line

This is not valid kernel doc. 

> + */
> +struct gic_out_irq_line {
> +	void __iomem *base;
> +	unsigned int cpu;
> +};

Please read

       https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

which contains coding style information. Fix it up all over the place.

> +struct gic_in_irq_line {
> +	bool enabled;
> +	struct gic_out_irq_line *out_line;
> +	unsigned int it_num;
> +};

Please use proper ordering so that you don't have gaps in the
structure. pahole helps you to look at that.

> +/**
> + * Enable/Disable an output irq line
> + * This function is used by both mask/unmask to disable/enable the line.

Again invalid kernel doc

> + */
> +static void irq_line_set_enable(struct gic_out_irq_line *irq_line,
> +				struct gic_in_irq_line *in_irq_line,
> +				int enable)

bool enable or 'u8 val' and then supply the proper constants from the
call sites: LINE_ENABLE, LINE_DISABLE or such.

> +{
> +	void __iomem *enable_line_addr = irq_line->base +
> +	       KVX_GIC_ENABLE_OFFSET +
> +	       in_irq_line->it_num * KVX_GIC_ENABLE_ELEM_SIZE;

This is really unreadable.

	void __iomem *enable_line_addr = irq_line->base + KVX_GIC_ENABLE_OFFSET +
					 in_irq_line->it_num * KVX_GIC_ENABLE_ELEM_SIZE;

Also line->it_num is only used here, so you can do the multiplication at
the site which initializes 'line', no?

> +	writeb((uint8_t) enable ? 1 : 0, enable_line_addr);

The above spares you the typecast and please use 'u8' and friends for
kernel code.

> +	in_irq_line->enabled = enable;
> +}
> +
> +static void kvx_apic_gic_set_line(struct irq_data *data, int enable)
> +{
> +	struct kvx_apic_gic *gic = irq_data_get_irq_chip_data(data);
> +	unsigned int in_irq = irqd_to_hwirq(data);
> +	struct gic_in_irq_line *in_line = &gic->input_irq[in_irq];
> +	struct gic_out_irq_line *out_line = in_line->out_line;
> +
> +	raw_spin_lock(&gic->lock);

Please use guard() 

> +	/* Set line enable on currently assigned cpu */
> +	irq_line_set_enable(out_line, in_line, enable);
> +	raw_spin_unlock(&gic->lock);
> +}
> +
> +static void kvx_apic_gic_mask(struct irq_data *data)
> +{
> +	kvx_apic_gic_set_line(data, 0);
> +}
> +
> +static void kvx_apic_gic_unmask(struct irq_data *data)
> +{
> +	kvx_apic_gic_set_line(data, 1);
> +}
> +
> +#ifdef CONFIG_SMP
> +
> +static int kvx_apic_gic_set_affinity(struct irq_data *d,
> +				     const struct cpumask *cpumask,
> +				     bool force)
> +{
> +	struct kvx_apic_gic *gic = irq_data_get_irq_chip_data(d);
> +	unsigned int new_cpu;
> +	unsigned int hw_irq = irqd_to_hwirq(d);
> +	struct gic_in_irq_line *input_line = &gic->input_irq[hw_irq];
> +	struct gic_out_irq_line *new_out_line;
> +
> +	/* We assume there is only one cpu in the mask */

That's an invalid assumption. The mask can contain multiple CPUs. It's a
different story whether you select a single target CPU in your code.

> +	new_cpu = cpumask_first(cpumask);

Also cpumask_first() is wrong as the mask can contain offline CPUs. You
want to use cpumask_any_and(mask, cpu_online_mask) or such.

That still has the problem that this will select the first CPU for all
interrupts by default, so on boot you end up with all interrupts on
CPU0. You might want to use something like cpumask_pick_least_loaded().

> +	new_out_line = &gic->output_irq[new_cpu];
> +
> +	raw_spin_lock(&gic->lock);
> +
> +	/* Nothing to do, line is the same */
> +	if (new_out_line == input_line->out_line)
> +		goto out;
> +
> +	/* If old line was enabled, enable the new one before disabling
> +	 * the old one
> +	 */
> +	if (input_line->enabled)
> +		irq_line_set_enable(new_out_line, input_line, 1);
> +
> +	/* Disable it on old line */
> +	irq_line_set_enable(input_line->out_line, input_line, 0);
> +
> +	/* Assign new output line to input IRQ */
> +	input_line->out_line = new_out_line;
> +
> +out:
> +	raw_spin_unlock(&gic->lock);

        scoped_guard(raw_spinlock)(&gic->lock) {
                if (outline != input_line->out_line) {
                   	.....
                }
        }

which spares the goto and has a well defined scope.

> +	irq_data_update_effective_affinity(d, cpumask_of(new_cpu));
> +
> +	return IRQ_SET_MASK_OK;
> +}
> +#endif
> +
> +static struct irq_chip kvx_apic_gic_chip = {
> +	.name           = "kvx apic gic",
> +	.irq_mask	= kvx_apic_gic_mask,
> +	.irq_unmask	= kvx_apic_gic_unmask,
> +#ifdef CONFIG_SMP
> +	.irq_set_affinity = kvx_apic_gic_set_affinity,
> +#endif
> +};
> +
> +static int kvx_apic_gic_alloc(struct irq_domain *domain, unsigned int virq,
> +				   unsigned int nr_irqs, void *args)
> +{
> +	int i;
> +	struct irq_fwspec *fwspec = args;
> +	int hwirq = fwspec->param[0];
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irq_domain_set_info(domain, virq + i, hwirq + i,
> +				    &kvx_apic_gic_chip,
> +				    domain->host_data, handle_simple_irq,
> +				    NULL, NULL);

Please use the full 100 character width.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops kvx_apic_gic_domain_ops = {
> +	.alloc  = kvx_apic_gic_alloc,
> +	.free   = irq_domain_free_irqs_common,
> +};
> +
> +static void irq_line_get_status_lac(struct gic_out_irq_line *out_irq_line,
> +			uint64_t status[KVX_GIC_STATUS_LAC_ARRAY_SIZE])
> +{
> +	int i;
> +
> +	for (i = 0; i < KVX_GIC_STATUS_LAC_ARRAY_SIZE; i++) {
> +		status[i] = readq(out_irq_line->base +
> +				  KVX_GIC_STATUS_LAC_OFFSET +
> +				  i * KVX_GIC_STATUS_LAC_ELEM_SIZE);
> +	}
> +}
> +
> +static void kvx_apic_gic_handle_irq(struct irq_desc *desc)
> +{
> +	struct kvx_apic_gic *gic_data = irq_desc_get_handler_data(desc);
> +	struct gic_out_irq_line *out_line;
> +	uint64_t status[KVX_GIC_STATUS_LAC_ARRAY_SIZE];
> +	unsigned long irqn, cascade_irq;
> +	unsigned long cpu = smp_processor_id();
> +
> +	out_line = &gic_data->output_irq[cpu];
> +
> +	irq_line_get_status_lac(out_line, status);
> +
> +	for_each_set_bit(irqn, (unsigned long *) status,
> +			KVX_GIC_STATUS_LAC_ARRAY_SIZE * BITS_PER_LONG) {

  sizeof(status) * 8 ?

> +
> +		cascade_irq = irq_find_mapping(gic_data->domain, irqn);
> +
> +		generic_handle_irq(cascade_irq);
> +	}
> +}
> +
> +static void __init apic_gic_init(struct kvx_apic_gic *gic)
> +{
> +	unsigned int cpu, line;
> +	struct gic_in_irq_line *input_irq_line;
> +	struct gic_out_irq_line *output_irq_line;
> +	uint64_t status[KVX_GIC_STATUS_LAC_ARRAY_SIZE];
> +
> +	/* Initialize all input lines (device -> )*/
> +	for (line = 0; line < KVX_GIC_INPUT_IT_COUNT; line++) {
> +		input_irq_line = &gic->input_irq[line];
> +		input_irq_line->enabled = false;
> +		/* All input lines map on output 0 */
> +		input_irq_line->out_line = &gic->output_irq[0];
> +		input_irq_line->it_num = line;
> +	}
> +
> +	/* Clear all output lines (-> cpus) */
> +	for (cpu = 0; cpu < KVX_GIC_OUTPUT_IT_COUNT; cpu++) {
> +		output_irq_line = &gic->output_irq[cpu];
> +		output_irq_line->cpu = cpu;
> +		output_irq_line->base = gic->base +
> +			cpu * (KVX_GIC_ELEM_SIZE * KVX_GIC_PER_CPU_IT_COUNT);
> +
> +		/* Disable all external lines on this core */
> +		for (line = 0; line < KVX_GIC_INPUT_IT_COUNT; line++)
> +			irq_line_set_enable(output_irq_line,
> +					&gic->input_irq[line], 0x0);

See bracket rules

> +		irq_line_get_status_lac(output_irq_line, status);
> +	}
> +}

A general observation. I understand that you want to use self
explanatory variable names, but some of them are really overdone for my
taste.

Thanks,

        tglx

On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote:
> From: Yann Sionneau <ysionneau@kalrayinc.com>
> 

...

> +
> +static int __init kvx_init_apic_gic(struct device_node *node,
> +				    struct device_node *parent)
> +{
> +	struct kvx_apic_gic *gic;
> +	int ret;
> +	unsigned int irq;
> +
> +	if (!parent) {
> +		pr_err("kvx apic gic does not have parent\n");

How is this possible? Aren't you controlling the code being executed?

> +		return -EINVAL;
> +	}
> +
> +	gic = kzalloc(sizeof(*gic), GFP_KERNEL);
> +	if (!gic)
> +		return -ENOMEM;
> +
> +	if (of_property_read_u32(node, "kalray,intc-nr-irqs",
> +						&gic->input_nr_irqs))

There is no such property. Also, there shouldn't be anyway...

> +		gic->input_nr_irqs = KVX_GIC_INPUT_IT_COUNT;
> +
> +	if (WARN_ON(gic->input_nr_irqs > KVX_GIC_INPUT_IT_COUNT)) {

Why? Please, drop all these WARN_ON from here and other patches. WARN_ON
is for cases which cannot happen, as it might panic entire system.

Instead, handle the case properly.

> +		ret = -EINVAL;
> +		goto err_kfree;
> +	}
> +
> +	gic->base = of_io_request_and_map(node, 0, node->name);
> +	if (!gic->base) {
> +		ret = -EINVAL;
> +		goto err_kfree;
> +	}
> +
> +	raw_spin_lock_init(&gic->lock);
> +	apic_gic_init(gic);


Best regards,
Krzysztof

Hello Krzysztof,

On 22/07/2024 14:28, Krzysztof Kozlowski wrote:
> On 22/07/2024 11:41, ysionneau@kalrayinc.com wrote:
>> From: Yann Sionneau <ysionneau@kalrayinc.com>
>>
> ...
>
>> +
>> +static int __init kvx_init_apic_gic(struct device_node *node,
>> +				    struct device_node *parent)
>> +{
>> +	struct kvx_apic_gic *gic;
>> +	int ret;
>> +	unsigned int irq;
>> +
>> +	if (!parent) {
>> +		pr_err("kvx apic gic does not have parent\n");
> How is this possible? Aren't you controlling the code being executed?

I think this is called from generic code with values from the DT.

If this node does not have any interrupt-parent I guess parent is NULL.

DT is user controlled, not controlled by the driver code.

Also, I can see such tests in several irqchip drivers: irq-imx-gpcv2, irq-sni-exiu, irq-crossbar, irq-meson-gpio, irq-al-fic, irq-tegra, irq-mmp, etc

Isn't it ok?

>> +		return -EINVAL;
>> +	}
>> +
>> +	gic = kzalloc(sizeof(*gic), GFP_KERNEL);
>> +	if (!gic)
>> +		return -ENOMEM;
>> +
>> +	if (of_property_read_u32(node, "kalray,intc-nr-irqs",
>> +						&gic->input_nr_irqs))
> There is no such property. Also, there shouldn't be anyway...
Ok I'll remove this property from the code and just use KVX_GIC_INPUT_IT_COUNT renamed as KVX_GIC_MAX_INPUT_IT_COUNT everywhere with the maximum possible value.
>
>> +		gic->input_nr_irqs = KVX_GIC_INPUT_IT_COUNT;
>> +
>> +	if (WARN_ON(gic->input_nr_irqs > KVX_GIC_INPUT_IT_COUNT)) {
> Why? Please, drop all these WARN_ON from here and other patches. WARN_ON
> is for cases which cannot happen, as it might panic entire system.
Ok, I'll replace with pr_warn().
> Instead, handle the case properly.
>
>> +		ret = -EINVAL;
>> +		goto err_kfree;
>> +	}
>> +
>> +	gic->base = of_io_request_and_map(node, 0, node->name);
>> +	if (!gic->base) {
>> +		ret = -EINVAL;
>> +		goto err_kfree;
>> +	}
>> +
>> +	raw_spin_lock_init(&gic->lock);
>> +	apic_gic_init(gic);