| Message ID | 20230120141002.2442-32-ysionneau@kalray.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Upstream kvx Linux port | expand |
On 20/01/2023 15:10, Yann Sionneau wrote: > + > +int __init kvx_ipi_ctrl_probe(irqreturn_t (*ipi_irq_handler)(int, void *)) > +{ > + struct device_node *np; > + int ret; > + unsigned int ipi_irq; > + void __iomem *ipi_base; > + > + np = of_find_compatible_node(NULL, NULL, "kalray,kvx-ipi-ctrl"); Nope, big no. Drivers go to drivers, not to arch code. Use proper driver infrastructure. Best regards, Krzysztof
Hello Krzysztof, On 22/01/2023 12:54, Krzysztof Kozlowski wrote: > On 20/01/2023 15:10, Yann Sionneau wrote: >> + >> +int __init kvx_ipi_ctrl_probe(irqreturn_t (*ipi_irq_handler)(int, void *)) >> +{ >> + struct device_node *np; >> + int ret; >> + unsigned int ipi_irq; >> + void __iomem *ipi_base; >> + >> + np = of_find_compatible_node(NULL, NULL, "kalray,kvx-ipi-ctrl"); > Nope, big no. > > Drivers go to drivers, not to arch code. Use proper driver infrastructure. Thank you for your review. It raises questions on our side about how to handle this change. First let me describe the hardware: The coolidge ipi controller device handles IPI communication between cpus inside a cluster. Each cpu has 8 of its dedicated irq lines (24 to 31) hard-wired to the ipi. The ipi controller has 8 sets of 2 registers: - a 17-bit "interrupt" register - a 17-bit "mask" register Each couple of register is dedicated to 1 of the 8 irqlines. Each of the 17 bits of interrupt/mask register identifies a cpu (cores 0 to 15 + secure_core). Writing bit i in interrupt register sends an irq to cpu i, according to the mask in mask register. Writing in interrupt/mask register couple N targets irq line N of the core. - Ipi generates an interrupt to the cpu when message is ready. - Messages are delivered via Axi. - Ipi does not have any interrupt input lines. +---------------+ irq axi_w | | i |<--/--- ipi <------ | CPU | n | x8 | core0 | t | | | c | irq irq msi | | t |<--/--- apic <----- mbox <------- | | l | x4 +---------------+ with intctl = core-irq controller We analyzed how other Linux ports are handling this situation (IPI) and here are several possible solutions: 1/ put everything in smp.c like what longarch is doing. * Except that IPI in longarch seems to involve writing to a special purpose CPU register and not doing a memory mapped write like kvx. 2/ write a device driver in drivers/xxx/ with the content from ipi.c * the probe would just ioremap the reg from DT and register the irq using request_percpu_irq() * it would export a function "kvx_ipi_send()" that would directly be called by smp.c * Question : where would this driver be placed in drivers/ ? drivers/irqchip/ ? Even if this is not per-se an interrupt-controller driver? 3/ write a "dummy" interrupt-controller driver in drivers/irqchip/: * it would create a dummy irq_domain, ioremap the reg, request per_cpu irq * declare a struct irq_chip with only ipi_send_mask() callback declared so that generic IPI code in kernel/irq/ipi.c (__ipi_send_single()) would work. * This would make use of the generic IPI code like what mips and risc-v are doing. 4/ consider our "ipi device" as a mailbox and write a mailbox driver in drivers/mailbox/ 5/ consider it as an msi-controller since it transforms an AXI write into IRQ. The solution would look a bit like 3/ 6/ consider the ipi as "part of the core_intc" and add the content of ipi.c in drivers/irqchip/irq-kvx-core-intc.c 7/ Do like OpenRISC and CSKY: * declare a function pointer in smp.c (see smp_cross_call() from OpenRISC https://elixir.bootlin.com/linux/latest/source/arch/openrisc/kernel/smp.c#L28) * declare a "setter" function in smp.c (see set_send_ipi() from OpenRISC https://elixir.bootlin.com/linux/latest/source/arch/openrisc/kernel/smp.c#L202) * write a driver in drivers/irqchip/ which ends up calling the setter function (see irq-ompic.c: https://elixir.bootlin.com/linux/latest/source/drivers/irqchip/irq-ompic.c#L191) I would tend to exclude solution 1/ because it does not fit exactly our arch (core reg vs AXI write), or we would have to do an ioremap() from inside smp.c, is this acceptable? I would exclude 3/ because it feels a bit dirty to hack a dummy interrupt-controller... our IPI is not an interrupt-controller, there are no input irqs. It's more like a device generating an IRQ. 4/ and 5/ feel a bit over-engineered. 6/ I guess this would work since irqchips are initialized early from init_IRQ(), but it does not reflect very much our hardware since each CPU has one core_intc but the IPI is global to each cluster and is accessed over AXI. Having considered all of this, I would tend to end up with solution 7/ but it honestly does not feel much cleaner than our current proposition. The function pointer dance feels a bit hackish. What would you prefer? Regards,
On 31/01/2024 10:52, Yann Sionneau wrote: > Hello Krzysztof, > > On 22/01/2023 12:54, Krzysztof Kozlowski wrote: >> On 20/01/2023 15:10, Yann Sionneau wrote: >>> + >>> +int __init kvx_ipi_ctrl_probe(irqreturn_t (*ipi_irq_handler)(int, void *)) >>> +{ >>> + struct device_node *np; >>> + int ret; >>> + unsigned int ipi_irq; >>> + void __iomem *ipi_base; >>> + >>> + np = of_find_compatible_node(NULL, NULL, "kalray,kvx-ipi-ctrl"); >> Nope, big no. >> >> Drivers go to drivers, not to arch code. Use proper driver infrastructure. > Thank you for your review. > > It raises questions on our side about how to handle this change. I am sorry, but responding with one page of hardware description is totally unrelated to the code I am questioning here and does not make it easier for me to respond. I understand that you want me to learn entire new KVX architecture to be able to provide good review, but it is just not possible, sorry. We all have quite limited time around here, so we all expect concise and precise answers. Best regards, Krzysztof
On Wed, Jan 31, 2024, at 10:52, Yann Sionneau wrote: > On 22/01/2023 12:54, Krzysztof Kozlowski wrote: >> On 20/01/2023 15:10, Yann Sionneau wrote: >>> + >>> +int __init kvx_ipi_ctrl_probe(irqreturn_t (*ipi_irq_handler)(int, void *)) >>> +{ >>> + struct device_node *np; >>> + int ret; >>> + unsigned int ipi_irq; >>> + void __iomem *ipi_base; >>> + >>> + np = of_find_compatible_node(NULL, NULL, "kalray,kvx-ipi-ctrl"); >> Nope, big no. >> >> Drivers go to drivers, not to arch code. Use proper driver infrastructure. > Thank you for your review. > > It raises questions on our side about how to handle this change. > > First let me describe the hardware: > > The coolidge ipi controller device handles IPI communication between cpus > inside a cluster. > > Each cpu has 8 of its dedicated irq lines (24 to 31) hard-wired to the ipi. > The ipi controller has 8 sets of 2 registers: > - a 17-bit "interrupt" register > - a 17-bit "mask" register > > Each couple of register is dedicated to 1 of the 8 irqlines. > Each of the 17 bits of interrupt/mask register > identifies a cpu (cores 0 to 15 + secure_core). > Writing bit i in interrupt register sends an irq to cpu i, according to the mask > in mask register. > Writing in interrupt/mask register couple N targets irq line N of the core. > > - Ipi generates an interrupt to the cpu when message is ready. > - Messages are delivered via Axi. > - Ipi does not have any interrupt input lines. > > > +---------------+ irq axi_w > | | i |<--/--- ipi <------ > | CPU | n | x8 > | core0 | t | > | | c | irq irq msi > | | t |<--/--- apic <----- mbox <------- > | | l | x4 > +---------------+ > with intctl = core-irq controller > > > We analyzed how other Linux ports are handling this situation (IPI) and > here are several possible solutions: > > 1/ put everything in smp.c like what longarch is doing. > * Except that IPI in longarch seems to involve writing to a special > purpose CPU register and not doing a memory mapped write like kvx. > > 2/ write a device driver in drivers/xxx/ with the content from ipi.c > * the probe would just ioremap the reg from DT and register the irq > using request_percpu_irq() > * it would export a function "kvx_ipi_send()" that would directly be > called by smp.c > * Question : where would this driver be placed in drivers/ ? > drivers/irqchip/ ? Even if this is not per-se an interrupt-controller > driver? This looks like it's close enough to the irqchip driver that you can just have it in the same file as the 'intctl' portion. Top-level irqchip implementations tend to be rather architecture specific, as does the IPI mechanism. Depending on the register layout, I think you can have a single devicetree node for the combination of the core-irq (for managing your own interrupts) and the ipi (for receiving interrupts from others), and then have a driver in drivers/irqchip to deal with both. For the ipi mechanism, trying to abstract it too much generally makes it too slow, so I would not go through a nested irqchip or a mailbox driver etc. I don't know what the 'apic' in your diagram is, so that would be either a nested irqchip or could be part of the same driver as well. Arnd
diff --git a/arch/kvx/include/asm/ipi.h b/arch/kvx/include/asm/ipi.h new file mode 100644 index 000000000000..137407a075e6 --- /dev/null +++ b/arch/kvx/include/asm/ipi.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2017-2023 Kalray Inc. + * Author(s): Clement Leger + */ + +#ifndef _ASM_KVX_IPI_H +#define _ASM_KVX_IPI_H + +#include <linux/irqreturn.h> + +int kvx_ipi_ctrl_probe(irqreturn_t (*ipi_irq_handler)(int, void *)); + +void kvx_ipi_send(const struct cpumask *mask); + +#endif /* _ASM_KVX_IPI_H */ diff --git a/arch/kvx/platform/Makefile b/arch/kvx/platform/Makefile index c7d0abb15c27..27f0914e0de5 100644 --- a/arch/kvx/platform/Makefile +++ b/arch/kvx/platform/Makefile @@ -4,3 +4,4 @@ # obj-$(CONFIG_SMP) += pwr_ctrl.o +obj-$(CONFIG_SMP) += ipi.o diff --git a/arch/kvx/platform/ipi.c b/arch/kvx/platform/ipi.c new file mode 100644 index 000000000000..a471039b1643 --- /dev/null +++ b/arch/kvx/platform/ipi.c @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2017-2023 Kalray Inc. + * Author(s): Clement Leger + * Luc Michel + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/module.h> +#include <linux/of_irq.h> +#include <linux/cpumask.h> +#include <linux/interrupt.h> +#include <linux/cpuhotplug.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> + +#define IPI_INTERRUPT_OFFSET 0x0 +#define IPI_MASK_OFFSET 0x20 + +/* + * IPI controller can signal RM and PE0 -> 15 + * In order to restrict that to the PE, write the corresponding mask + */ +#define KVX_IPI_CPU_MASK (~0xFFFF) + +struct kvx_ipi_ctrl { + void __iomem *regs; + unsigned int ipi_irq; +}; + +static struct kvx_ipi_ctrl kvx_ipi_controller; + +/** + * @kvx_pwr_ctrl_cpu_poweron Wakeup a cpu + * + * cpu: cpu to wakeup + */ +void kvx_ipi_send(const struct cpumask *mask) +{ + const unsigned long *maskb = cpumask_bits(mask); + + WARN_ON(*maskb & KVX_IPI_CPU_MASK); + writel(*maskb, kvx_ipi_controller.regs + IPI_INTERRUPT_OFFSET); +} + +static int kvx_ipi_starting_cpu(unsigned int cpu) +{ + enable_percpu_irq(kvx_ipi_controller.ipi_irq, IRQ_TYPE_NONE); + + return 0; +} + +static int kvx_ipi_dying_cpu(unsigned int cpu) +{ + disable_percpu_irq(kvx_ipi_controller.ipi_irq); + + return 0; +} + +int __init kvx_ipi_ctrl_probe(irqreturn_t (*ipi_irq_handler)(int, void *)) +{ + struct device_node *np; + int ret; + unsigned int ipi_irq; + void __iomem *ipi_base; + + np = of_find_compatible_node(NULL, NULL, "kalray,kvx-ipi-ctrl"); + BUG_ON(!np); + + ipi_base = of_iomap(np, 0); + BUG_ON(!ipi_base); + + kvx_ipi_controller.regs = ipi_base; + + /* Init mask for interrupts to PE0 -> PE15 */ + writel(KVX_IPI_CPU_MASK, kvx_ipi_controller.regs + IPI_MASK_OFFSET); + + ipi_irq = irq_of_parse_and_map(np, 0); + of_node_put(np); + if (!ipi_irq) { + pr_err("Failed to parse irq: %d\n", ipi_irq); + return -EINVAL; + } + + ret = request_percpu_irq(ipi_irq, ipi_irq_handler, + "kvx_ipi", &kvx_ipi_controller); + if (ret) { + pr_err("can't register interrupt %d (%d)\n", + ipi_irq, ret); + return ret; + } + kvx_ipi_controller.ipi_irq = ipi_irq; + + ret = cpuhp_setup_state(CPUHP_AP_IRQ_KVX_STARTING, + "kvx/ipi:online", + kvx_ipi_starting_cpu, + kvx_ipi_dying_cpu); + if (ret < 0) { + pr_err("Failed to setup hotplug state"); + return ret; + } + + return 0; +}