[PATCH 0/3] riscv: interrupt-controller: Add T-HEAD C900 ACLINT SSWI

[PATCH 0/3] riscv: interrupt-controller: Add T-HEAD C900 ACLINT SSWI

[Inochi Amaoto]

New version of T-HEAD C920 implement a fully featured ACLINT[1] device
(This core is used by Sophgo SG2044). This ACLINT device provides a
SSWI field to support fast S-mode IPI. This SSWI device is like the
MSWI device in CLINT/ACLINT, but for S-mode. The only thing is different
from the draft is that the T-HEAD version SSWI needs to write 0 on the
SSWI address to clear the IPI.

Add full support for T-HEAD C900 SSWI device.

[1] https://github.com/riscv/riscv-aclint

Inochi Amaoto (3):
  dt-bindings: interrupt-controller: Add Sophgo SG2044 ACLINT SSWI
  irqchip: add T-HEAD C900 ACLINT SSWI driver
  riscv: defconfig: Enable T-HEAD C900 ACLINT SSWI drivers

 .../thead,c900-aclint-sswi.yaml               |  58 ++++++
 arch/riscv/configs/defconfig                  |   1 +
 drivers/irqchip/Kconfig                       |  10 ++
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-thead-c900-aclint-sswi.c  | 169 ++++++++++++++++++
 include/linux/cpuhotplug.h                    |   1 +
 6 files changed, 240 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
 create mode 100644 drivers/irqchip/irq-thead-c900-aclint-sswi.c

--
2.46.2

[PATCH 1/3] dt-bindings: interrupt-controller: Add Sophgo SG2044 ACLINT SSWI

[Inochi Amaoto]

Sophgo SG2044 has a new version of T-HEAD C920, which implement
a fully featured ACLINT device. This ACLINT has an extra SSWI
field to support fast S-mode IPI.

Add necessary compatible string for the T-HEAD ACLINT sswi device.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 .../thead,c900-aclint-sswi.yaml               | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml b/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
new file mode 100644
index 000000000000..0106fbf3ea1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/thead,c900-aclint-sswi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo sg2044 ACLINT Supervisor-level Software Interrupt Device
+
+maintainers:
+  - Inochi Amaoto <inochiama@outlook.com>
+
+description:
+  The SSWI device is a part of the riscv ACLINT device. It provides
+  supervisor-level IPI functionality for a set of HARTs on a RISC-V
+  platform. It provides a register to set an IPI (SETSSIP) for each
+  HART connected to the SSWI device.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - sophgo,sg2044-aclint-sswi
+      - const: thead,c900-aclint-sswi
+
+  reg:
+    maxItems: 1
+
+  "#interrupt-cells":
+    const: 0
+
+  interrupt-controller: true
+
+  interrupts-extended:
+    minItems: 1
+    maxItems: 4095
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#interrupt-cells"
+  - interrupt-controller
+  - interrupts-extended
+
+examples:
+  - |
+    interrupt-controller@94000000 {
+      compatible = "sophgo,sg2044-aclint-sswi", "thead,c900-aclint-sswi";
+      reg = <0x94000000 0x00004000>;
+      #interrupt-cells = <0>;
+      interrupt-controller;
+      interrupts-extended = <&cpu1intc 1>,
+                            <&cpu2intc 1>,
+                            <&cpu3intc 1>,
+                            <&cpu4intc 1>;
+    };
+...
--
2.46.2

Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add Sophgo SG2044 ACLINT SSWI

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

On Fri, Oct 04, 2024 at 04:05:55PM +0800, Inochi Amaoto wrote:
> Sophgo SG2044 has a new version of T-HEAD C920, which implement
> a fully featured ACLINT device. This ACLINT has an extra SSWI
> field to support fast S-mode IPI.
> 
> Add necessary compatible string for the T-HEAD ACLINT sswi device.
> 
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> ---
>  .../thead,c900-aclint-sswi.yaml               | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml b/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> new file mode 100644
> index 000000000000..0106fbf3ea1f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/thead,c900-aclint-sswi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo sg2044 ACLINT Supervisor-level Software Interrupt Device
> +
> +maintainers:
> +  - Inochi Amaoto <inochiama@outlook.com>
> +
> +description:
> +  The SSWI device is a part of the riscv ACLINT device. It provides
> +  supervisor-level IPI functionality for a set of HARTs on a RISC-V
> +  platform. It provides a register to set an IPI (SETSSIP) for each
> +  HART connected to the SSWI device.

If it is part of the aclint, why should it have a separate node, rather
than be part of the existing aclint node as a third reg property?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add Sophgo SG2044 ACLINT SSWI

[Inochi Amaoto]

On Fri, Oct 04, 2024 at 04:44:22PM +0100, Conor Dooley wrote:
> On Fri, Oct 04, 2024 at 04:05:55PM +0800, Inochi Amaoto wrote:
> > Sophgo SG2044 has a new version of T-HEAD C920, which implement
> > a fully featured ACLINT device. This ACLINT has an extra SSWI
> > field to support fast S-mode IPI.
> > 
> > Add necessary compatible string for the T-HEAD ACLINT sswi device.
> > 
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > ---
> >  .../thead,c900-aclint-sswi.yaml               | 58 +++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml b/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> > new file mode 100644
> > index 000000000000..0106fbf3ea1f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/thead,c900-aclint-sswi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sophgo sg2044 ACLINT Supervisor-level Software Interrupt Device
> > +
> > +maintainers:
> > +  - Inochi Amaoto <inochiama@outlook.com>
> > +
> > +description:
> > +  The SSWI device is a part of the riscv ACLINT device. It provides
> > +  supervisor-level IPI functionality for a set of HARTs on a RISC-V
> > +  platform. It provides a register to set an IPI (SETSSIP) for each
> > +  HART connected to the SSWI device.
> 
> If it is part of the aclint, why should it have a separate node, rather
> than be part of the existing aclint node as a third reg property?

For aclint, the current nodes that have documented are mswi and mtime.
Since the mtime is a M-mode time source, it is not suitable to add the
sswi reg into this device. For mswi, it is OK to add a sswi reg, but
this will cause problem while checking "interrupt-extend". Do we just
double the maxItem? Or just left it unchanged?

Another reason to add it as a separate node is that the draft says
sswi can be multiple. If we add this device by adding reg. It will be
hard if we have multiple sswi devices but one mswi device.

Regard,
Inochi

Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add Sophgo SG2044 ACLINT SSWI

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

On Sat, Oct 05, 2024 at 08:46:37AM +0800, Inochi Amaoto wrote:
> On Fri, Oct 04, 2024 at 04:44:22PM +0100, Conor Dooley wrote:
> > On Fri, Oct 04, 2024 at 04:05:55PM +0800, Inochi Amaoto wrote:
> > > Sophgo SG2044 has a new version of T-HEAD C920, which implement
> > > a fully featured ACLINT device. This ACLINT has an extra SSWI
> > > field to support fast S-mode IPI.
> > > 
> > > Add necessary compatible string for the T-HEAD ACLINT sswi device.
> > > 
> > > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > > ---
> > >  .../thead,c900-aclint-sswi.yaml               | 58 +++++++++++++++++++
> > >  1 file changed, 58 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml b/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> > > new file mode 100644
> > > index 000000000000..0106fbf3ea1f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/thead,c900-aclint-sswi.yaml
> > > @@ -0,0 +1,58 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/interrupt-controller/thead,c900-aclint-sswi.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Sophgo sg2044 ACLINT Supervisor-level Software Interrupt Device
> > > +
> > > +maintainers:
> > > +  - Inochi Amaoto <inochiama@outlook.com>
> > > +
> > > +description:
> > > +  The SSWI device is a part of the riscv ACLINT device. It provides
> > > +  supervisor-level IPI functionality for a set of HARTs on a RISC-V
> > > +  platform. It provides a register to set an IPI (SETSSIP) for each
> > > +  HART connected to the SSWI device.
> > 
> > If it is part of the aclint, why should it have a separate node, rather
> > than be part of the existing aclint node as a third reg property?
> 
> For aclint, the current nodes that have documented are mswi and mtime.
> Since the mtime is a M-mode time source, it is not suitable to add the
> sswi reg into this device. For mswi, it is OK to add a sswi reg, but
> this will cause problem while checking "interrupt-extend". Do we just
> double the maxItem? Or just left it unchanged?
> 
> Another reason to add it as a separate node is that the draft says
> sswi can be multiple. If we add this device by adding reg. It will be
> hard if we have multiple sswi devices but one mswi device.

Ah, I see we do indeed have 2 devices already for the aclint, one for
mswi and mtimer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/3] irqchip: add T-HEAD C900 ACLINT SSWI driver

[Inochi Amaoto]

Add a driver for the T-HEAD C900 ACLINT SSWI device, which is an
enhanced implementation of the RISC-V ACLINT SSWI specification.
This device allows the system to send ipi via fast device interface.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/irqchip/Kconfig                      |  10 ++
 drivers/irqchip/Makefile                     |   1 +
 drivers/irqchip/irq-thead-c900-aclint-sswi.c | 169 +++++++++++++++++++
 include/linux/cpuhotplug.h                   |   1 +
 4 files changed, 181 insertions(+)
 create mode 100644 drivers/irqchip/irq-thead-c900-aclint-sswi.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 341cd9ca5a05..32671385cbb7 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -611,6 +611,16 @@ config STARFIVE_JH8100_INTC

 	  If you don't know what to do here, say Y.

+config THEAD_C900_ACLINT_SSWI
+	bool "THEAD C9XX ACLINT S-mode IPI Interrupt Controller"
+	depends on RISCV
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  This enables support for T-HEAD specific ACLINT SSWI device
+	  support.
+
+	  If you don't know what to do here, say Y.
+
 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 e3679ec2b9f7..583418261253 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_RISCV_APLIC_MSI)		+= irq-riscv-aplic-msi.o
 obj-$(CONFIG_RISCV_IMSIC)		+= irq-riscv-imsic-state.o irq-riscv-imsic-early.o irq-riscv-imsic-platform.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_STARFIVE_JH8100_INTC)	+= irq-starfive-jh8100-intc.o
+obj-$(CONFIG_THEAD_C900_ACLINT_SSWI)	+= irq-thead-c900-aclint-sswi.o
 obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
 obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
 obj-$(CONFIG_IMX_MU_MSI)		+= irq-imx-mu-msi.o
diff --git a/drivers/irqchip/irq-thead-c900-aclint-sswi.c b/drivers/irqchip/irq-thead-c900-aclint-sswi.c
new file mode 100644
index 000000000000..7bd06369b409
--- /dev/null
+++ b/drivers/irqchip/irq-thead-c900-aclint-sswi.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Inochi Amaoto <inochiama@gmail.com>
+ */
+
+#define pr_fmt(fmt) "thead-c900-aclint-sswi: " fmt
+#include <linux/acpi.h>
+#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/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/pci.h>
+#include <linux/spinlock.h>
+#include <linux/smp.h>
+
+#define ACLINT_xSWI_REGISTER_SIZE	4
+
+struct aclint_sswi_cpu_config {
+	void __iomem	*reg;
+	u32		offset;
+};
+
+static int sswi_ipi_virq __ro_after_init;
+static DEFINE_PER_CPU(struct aclint_sswi_cpu_config, sswi_cpus);
+
+static void thead_aclint_sswi_ipi_send(unsigned int cpu)
+{
+	struct aclint_sswi_cpu_config *config = per_cpu_ptr(&sswi_cpus, cpu);
+
+	writel_relaxed(0x1, config->reg + config->offset);
+}
+
+static void thead_aclint_sswi_ipi_clear(void)
+{
+	unsigned int cpu = smp_processor_id();
+	struct aclint_sswi_cpu_config *config = per_cpu_ptr(&sswi_cpus, cpu);
+
+	writel_relaxed(0x0, config->reg + config->offset);
+}
+
+static void thead_aclint_sswi_ipi_handle(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(chip, desc);
+
+	csr_clear(CSR_IP, IE_SIE);
+	thead_aclint_sswi_ipi_clear();
+
+	ipi_mux_process();
+
+	chained_irq_exit(chip, desc);
+}
+
+static int aclint_sswi_ipi_starting_cpu(unsigned int cpu)
+{
+	enable_percpu_irq(sswi_ipi_virq, irq_get_trigger_type(sswi_ipi_virq));
+	return 0;
+}
+
+static int aclint_sswi_parse_irq(struct fwnode_handle *fwnode,
+				 void __iomem *reg)
+{
+	struct of_phandle_args parent;
+	unsigned long hartid;
+	u32 contexts, i;
+	int rc, cpu;
+	struct aclint_sswi_cpu_config *config;
+
+	contexts = of_irq_count(to_of_node(fwnode));
+	if (WARN_ON(!(contexts))) {
+		pr_err("%pfwP: no ACLINT SSWI context available\n", fwnode);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < contexts; i++) {
+		rc = of_irq_parse_one(to_of_node(fwnode), i, &parent);
+		if (rc)
+			return rc;
+
+		rc = riscv_of_parent_hartid(parent.np, &hartid);
+		if (rc)
+			return rc;
+
+		if (parent.args[0] != RV_IRQ_SOFT)
+			return -ENOTSUPP;
+
+		cpu = riscv_hartid_to_cpuid(hartid);
+		config = per_cpu_ptr(&sswi_cpus, cpu);
+
+		config->offset = i * ACLINT_xSWI_REGISTER_SIZE;
+		config->reg = reg;
+	}
+
+	pr_info("%pfwP: register %u CPU\n", fwnode, contexts);
+
+	return 0;
+}
+
+static int __init aclint_sswi_probe(struct fwnode_handle *fwnode)
+{
+	void __iomem *reg;
+	struct irq_domain *domain;
+	int virq, rc;
+
+	if (!is_of_node(fwnode))
+		return -EINVAL;
+
+	reg = of_iomap(to_of_node(fwnode), 0);
+	if (!reg)
+		return -ENOMEM;
+
+	/* Parse SSWI setting */
+	rc = aclint_sswi_parse_irq(fwnode, reg);
+	if (rc < 0)
+		return rc;
+
+	/* If mulitple SSWI devices are present, do not register irq again */
+	if (sswi_ipi_virq)
+		return 0;
+
+	/* Find and create irq domain */
+	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;
+	}
+
+	sswi_ipi_virq = irq_create_mapping(domain, RV_IRQ_SOFT);
+	if (!sswi_ipi_virq) {
+		pr_err("unable to create ACLINT SSWI IRQ mapping\n");
+		return -ENOMEM;
+	}
+
+	/* Register SSWI irq and handler */
+	virq = ipi_mux_create(BITS_PER_BYTE, thead_aclint_sswi_ipi_send);
+	if (virq <= 0) {
+		pr_err("unable to create muxed IPIs\n");
+		irq_dispose_mapping(sswi_ipi_virq);
+		return virq < 0 ? virq : -ENOMEM;
+	}
+
+	irq_set_chained_handler(sswi_ipi_virq, thead_aclint_sswi_ipi_handle);
+
+	cpuhp_setup_state(CPUHP_AP_IRQ_THEAD_ACLINT_SSWI_STARTING,
+			  "irqchip/thead-aclint-sswi:starting",
+			  aclint_sswi_ipi_starting_cpu, NULL);
+
+	riscv_ipi_set_virq_range(virq, BITS_PER_BYTE);
+
+	/* Announce that SSWI is providing IPIs */
+	pr_info("providing IPIs using THEAD ACLINT SSWI\n");
+
+	return 0;
+}
+
+static int __init aclint_sswi_early_probe(struct device_node *node,
+					  struct device_node *parent)
+{
+	return aclint_sswi_probe(&node->fwnode);
+}
+
+IRQCHIP_DECLARE(thead_aclint_sswi, "thead,c900-aclint-sswi", aclint_sswi_early_probe);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 2361ed4d2b15..799052249c7b 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -147,6 +147,7 @@ enum cpuhp_state {
 	CPUHP_AP_IRQ_EIOINTC_STARTING,
 	CPUHP_AP_IRQ_AVECINTC_STARTING,
 	CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING,
+	CPUHP_AP_IRQ_THEAD_ACLINT_SSWI_STARTING,
 	CPUHP_AP_IRQ_RISCV_IMSIC_STARTING,
 	CPUHP_AP_IRQ_RISCV_SBI_IPI_STARTING,
 	CPUHP_AP_ARM_MVEBU_COHERENCY,
--
2.46.2

Re: [PATCH 2/3] irqchip: add T-HEAD C900 ACLINT SSWI driver

[Thomas Gleixner]

On Fri, Oct 04 2024 at 16:05, Inochi Amaoto wrote:

> +#define pr_fmt(fmt) "thead-c900-aclint-sswi: " fmt
> +#include <linux/acpi.h>

What is this header used for?

> +static void thead_aclint_sswi_ipi_clear(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct aclint_sswi_cpu_config *config = per_cpu_ptr(&sswi_cpus, cpu);

That's an unnecessary indirection.

       *config = __this_cpu_ptr(&sswi_cpus);

is what you want here.

> +	writel_relaxed(0x0, config->reg + config->offset);
> +}

...

> +static int aclint_sswi_parse_irq(struct fwnode_handle *fwnode,
> +				 void __iomem *reg)

Please avoid line breaks and use up to 100 characters per line.

> +{
> +	struct of_phandle_args parent;
> +	unsigned long hartid;
> +	u32 contexts, i;
> +	int rc, cpu;
> +	struct aclint_sswi_cpu_config *config;

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

> +
> +	contexts = of_irq_count(to_of_node(fwnode));
> +	if (WARN_ON(!(contexts))) {

That WARN_ON() is pointless. The call chain is known and the pr_err() is
sufficient.

> +		pr_err("%pfwP: no ACLINT SSWI context available\n", fwnode);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < contexts; i++) {
> +		rc = of_irq_parse_one(to_of_node(fwnode), i, &parent);
> +		if (rc)
> +			return rc;
> +
> +		rc = riscv_of_parent_hartid(parent.np, &hartid);
> +		if (rc)
> +			return rc;
> +
> +		if (parent.args[0] != RV_IRQ_SOFT)
> +			return -ENOTSUPP;
> +
> +		cpu = riscv_hartid_to_cpuid(hartid);
> +		config = per_cpu_ptr(&sswi_cpus, cpu);
> +
> +		config->offset = i * ACLINT_xSWI_REGISTER_SIZE;
> +		config->reg = reg;

Why do you need config->reg and config->offset? All call sites access
the register via:

    config->reg + config->offset

So you can spare the exercise of adding the offset in the hotpath by
adding it at setup time, no?


> +	}
> +
> +	pr_info("%pfwP: register %u CPU\n", fwnode, contexts);

  ...CPU%s\n", fwnode, contexts, str_plural(contexts));

> +
> +	return 0;
> +}
> +
> +static int __init aclint_sswi_probe(struct fwnode_handle *fwnode)
> +{
> +	void __iomem *reg;
> +	struct irq_domain *domain;
> +	int virq, rc;

See above.

> +	if (!is_of_node(fwnode))
> +		return -EINVAL;
> +
> +	reg = of_iomap(to_of_node(fwnode), 0);
> +	if (!reg)
> +		return -ENOMEM;
> +
> +	/* Parse SSWI setting */
> +	rc = aclint_sswi_parse_irq(fwnode, reg);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* If mulitple SSWI devices are present, do not register irq again */
> +	if (sswi_ipi_virq)
> +		return 0;
> +
> +	/* Find and create irq domain */

Which domain is created here?

> +	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;
> +	}
> +
> +	sswi_ipi_virq = irq_create_mapping(domain, RV_IRQ_SOFT);
> +	if (!sswi_ipi_virq) {
> +		pr_err("unable to create ACLINT SSWI IRQ mapping\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Register SSWI irq and handler */
> +	virq = ipi_mux_create(BITS_PER_BYTE, thead_aclint_sswi_ipi_send);
> +	if (virq <= 0) {
> +		pr_err("unable to create muxed IPIs\n");
> +		irq_dispose_mapping(sswi_ipi_virq);
> +		return virq < 0 ? virq : -ENOMEM;
> +	}
> +
> +	irq_set_chained_handler(sswi_ipi_virq, thead_aclint_sswi_ipi_handle);
> +
> +	cpuhp_setup_state(CPUHP_AP_IRQ_THEAD_ACLINT_SSWI_STARTING,
> +			  "irqchip/thead-aclint-sswi:starting",
> +			  aclint_sswi_ipi_starting_cpu, NULL);

The startup callback enables the per CPU interrupt. When a CPU is
offlined then the per CPU interrupt stays enabled because the teardown
callback is NULL. I'm not convinced that this is a good idea.

> +
> +	riscv_ipi_set_virq_range(virq, BITS_PER_BYTE);
> +
> +	/* Announce that SSWI is providing IPIs */
> +	pr_info("providing IPIs using THEAD ACLINT SSWI\n");
> +
> +	return 0;
> +}
> +
> +static int __init aclint_sswi_early_probe(struct device_node *node,
> +					  struct device_node *parent)
> +{
> +	return aclint_sswi_probe(&node->fwnode);
> +}

What's the point of this indirection?

> +

Pointless newline.

> +IRQCHIP_DECLARE(thead_aclint_sswi, "thead,c900-aclint-sswi", aclint_sswi_early_probe);

Thanks,

        tglx

Re: [PATCH 2/3] irqchip: add T-HEAD C900 ACLINT SSWI driver

[Inochi Amaoto]

On Sun, Oct 06, 2024 at 09:50:39PM +0200, Thomas Gleixner wrote:
> On Fri, Oct 04 2024 at 16:05, Inochi Amaoto wrote:
> 
> > +#define pr_fmt(fmt) "thead-c900-aclint-sswi: " fmt
> > +#include <linux/acpi.h>
> 
> What is this header used for?
> 

This is copy-pasted error, I wiil remove it.

> > +static void thead_aclint_sswi_ipi_clear(void)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	struct aclint_sswi_cpu_config *config = per_cpu_ptr(&sswi_cpus, cpu);
> 
> That's an unnecessary indirection.
> 
>        *config = __this_cpu_ptr(&sswi_cpus);
> 
> is what you want here.

Thanks.

> 
> > +	writel_relaxed(0x0, config->reg + config->offset);
> > +}
> 
> ...
> 
> > +static int aclint_sswi_parse_irq(struct fwnode_handle *fwnode,
> > +				 void __iomem *reg)
> 
> Please avoid line breaks and use up to 100 characters per line.
> 
> > +{
> > +	struct of_phandle_args parent;
> > +	unsigned long hartid;
> > +	u32 contexts, i;
> > +	int rc, cpu;
> > +	struct aclint_sswi_cpu_config *config;
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
> 
> > +
> > +	contexts = of_irq_count(to_of_node(fwnode));
> > +	if (WARN_ON(!(contexts))) {
> 
> That WARN_ON() is pointless. The call chain is known and the pr_err() is
> sufficient.
> 
> > +		pr_err("%pfwP: no ACLINT SSWI context available\n", fwnode);
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (i = 0; i < contexts; i++) {
> > +		rc = of_irq_parse_one(to_of_node(fwnode), i, &parent);
> > +		if (rc)
> > +			return rc;
> > +
> > +		rc = riscv_of_parent_hartid(parent.np, &hartid);
> > +		if (rc)
> > +			return rc;
> > +
> > +		if (parent.args[0] != RV_IRQ_SOFT)
> > +			return -ENOTSUPP;
> > +
> > +		cpu = riscv_hartid_to_cpuid(hartid);
> > +		config = per_cpu_ptr(&sswi_cpus, cpu);
> > +
> > +		config->offset = i * ACLINT_xSWI_REGISTER_SIZE;
> > +		config->reg = reg;
> 
> Why do you need config->reg and config->offset? All call sites access
> the register via:
> 
>     config->reg + config->offset
> 
> So you can spare the exercise of adding the offset in the hotpath by
> adding it at setup time, no?

Thanks, I only consider supporting multiple device, but forgot that it
can be computed earily.

> 
> 
> > +	}
> > +
> > +	pr_info("%pfwP: register %u CPU\n", fwnode, contexts);
> 
>   ...CPU%s\n", fwnode, contexts, str_plural(contexts));
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int __init aclint_sswi_probe(struct fwnode_handle *fwnode)
> > +{
> > +	void __iomem *reg;
> > +	struct irq_domain *domain;
> > +	int virq, rc;
> 
> See above.
> 
> > +	if (!is_of_node(fwnode))
> > +		return -EINVAL;
> > +
> > +	reg = of_iomap(to_of_node(fwnode), 0);
> > +	if (!reg)
> > +		return -ENOMEM;
> > +
> > +	/* Parse SSWI setting */
> > +	rc = aclint_sswi_parse_irq(fwnode, reg);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	/* If mulitple SSWI devices are present, do not register irq again */
> > +	if (sswi_ipi_virq)
> > +		return 0;
> > +
> > +	/* Find and create irq domain */
> 
> Which domain is created here?
> 

It will create an IPI domain. I will update the comment.

> > +	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;
> > +	}
> > +
> > +	sswi_ipi_virq = irq_create_mapping(domain, RV_IRQ_SOFT);
> > +	if (!sswi_ipi_virq) {
> > +		pr_err("unable to create ACLINT SSWI IRQ mapping\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Register SSWI irq and handler */
> > +	virq = ipi_mux_create(BITS_PER_BYTE, thead_aclint_sswi_ipi_send);
> > +	if (virq <= 0) {
> > +		pr_err("unable to create muxed IPIs\n");
> > +		irq_dispose_mapping(sswi_ipi_virq);
> > +		return virq < 0 ? virq : -ENOMEM;
> > +	}
> > +
> > +	irq_set_chained_handler(sswi_ipi_virq, thead_aclint_sswi_ipi_handle);
> > +
> > +	cpuhp_setup_state(CPUHP_AP_IRQ_THEAD_ACLINT_SSWI_STARTING,
> > +			  "irqchip/thead-aclint-sswi:starting",
> > +			  aclint_sswi_ipi_starting_cpu, NULL);
> 
> The startup callback enables the per CPU interrupt. When a CPU is
> offlined then the per CPU interrupt stays enabled because the teardown
> callback is NULL. I'm not convinced that this is a good idea.
> 

Yes, I will add the cleanup handle to clear IPI and disable the IPI 
irq for the CPU.

> > +
> > +	riscv_ipi_set_virq_range(virq, BITS_PER_BYTE);
> > +
> > +	/* Announce that SSWI is providing IPIs */
> > +	pr_info("providing IPIs using THEAD ACLINT SSWI\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int __init aclint_sswi_early_probe(struct device_node *node,
> > +					  struct device_node *parent)
> > +{
> > +	return aclint_sswi_probe(&node->fwnode);
> > +}
> 
> What's the point of this indirection?
> 

This is make room for the future ACPI probe.

> > +
> 
> Pointless newline.
> 
> > +IRQCHIP_DECLARE(thead_aclint_sswi, "thead,c900-aclint-sswi", aclint_sswi_early_probe);
> 
> Thanks,
> 
>         tglx

Regards,
Inochi

[PATCH 3/3] riscv: defconfig: Enable T-HEAD C900 ACLINT SSWI drivers

[Inochi Amaoto]

Add support for T-HEAD C900 ACLINT SSWI irqchip.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 arch/riscv/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index 2341393cfac1..5b1d6325df85 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -256,6 +256,7 @@ CONFIG_RPMSG_CTRL=y
 CONFIG_RPMSG_VIRTIO=y
 CONFIG_PM_DEVFREQ=y
 CONFIG_IIO=y
+CONFIG_THEAD_C900_ACLINT_SSWI=y
 CONFIG_PHY_SUN4I_USB=m
 CONFIG_PHY_STARFIVE_JH7110_DPHY_RX=m
 CONFIG_PHY_STARFIVE_JH7110_PCIE=m
--
2.46.2