Re: [PATCH v3 2/6] irqchip/riscv-intc: Allow drivers to directly discover INTC hwnode

[Marc Zyngier <maz@kernel.org>]

On Sun, 20 Feb 2022 05:08:50 +0000,
Anup Patel <apatel@ventanamicro.com> wrote:
> 
> Various RISC-V drivers (such as SBI IPI, SBI Timer, SBI PMU, and
> KVM RISC-V) don't have associated DT node but these drivers need
> standard per-CPU (local) interrupts defined by the RISC-V privileged
> specification.
> 
> We add riscv_get_intc_hwnode() in arch/riscv which allows RISC-V
> drivers not having DT node to discover INTC hwnode which in-turn
> helps these drivers to map per-CPU (local) interrupts provided
> by the INTC driver.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  arch/riscv/include/asm/irq.h     |  4 ++++
>  arch/riscv/kernel/irq.c          | 19 +++++++++++++++++++
>  drivers/irqchip/irq-riscv-intc.c |  7 +++++++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
> index e4c435509983..43b9ebfbd943 100644
> --- a/arch/riscv/include/asm/irq.h
> +++ b/arch/riscv/include/asm/irq.h
> @@ -12,6 +12,10 @@
>  
>  #include <asm-generic/irq.h>
>  
> +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void));
> +
> +struct fwnode_handle *riscv_get_intc_hwnode(void);
> +
>  extern void __init init_IRQ(void);
>  
>  #endif /* _ASM_RISCV_IRQ_H */
> diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
> index 7207fa08d78f..ead92432df8c 100644
> --- a/arch/riscv/kernel/irq.c
> +++ b/arch/riscv/kernel/irq.c
> @@ -7,9 +7,28 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
>  #include <linux/seq_file.h>
>  #include <asm/smp.h>
>  
> +static struct fwnode_handle *(*__get_intc_node)(void);
> +
> +void riscv_set_intc_hwnode_fn(struct fwnode_handle *(*fn)(void))
> +{
> +	__get_intc_node = fn;
> +}
> +EXPORT_SYMBOL_GPL(riscv_set_intc_hwnode_fn);

We're talking about the root interrupt controller here. How can this
ever be implemented as a module?

> +
> +struct fwnode_handle *riscv_get_intc_hwnode(void)
> +{
> +	if (__get_intc_node)
> +		return __get_intc_node();
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(riscv_get_intc_hwnode);
> +
>  int arch_show_interrupts(struct seq_file *p, int prec)
>  {
>  	show_ipi_stats(p, prec);
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index b65bd8878d4f..fa24ecd01d39 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -92,6 +92,11 @@ static const struct irq_domain_ops riscv_intc_domain_ops = {
>  	.xlate	= irq_domain_xlate_onecell,
>  };
>  
> +static struct fwnode_handle *riscv_intc_hwnode(void)
> +{
> +	return (intc_domain) ? intc_domain->fwnode : NULL;
> +}

This makes no sense. Either you have found the interrupt controller
and allocated the domain, or you haven't. But you don't register a
callback without having found it.

And you have totally ignored my previous comments about the multitude
of irq domains for the INTC. Either you get rid of all but one and you
can register a single fwnode, or you stay with what you have today,

You can't have it both ways.

	M.

-- 
Without deviation from the norm, progress is not possible.