Re: [RFC PATCH v2 31/31] kvx: Add IPI driver

[Yann Sionneau]

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,

-- 
Yann