| Message ID | 20241004080557.2262872-3-inochiama@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: interrupt-controller: Add T-HEAD C900 ACLINT SSWI | expand |
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
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
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,
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 -- 2.46.2