Re: [PATCH v2 2/6] irqchip/riscv-intc: Create domain using named fwnode

[Marc Zyngier <maz@kernel.org>]

On 2022-02-19 03:38, Anup Patel wrote:
> On Thu, Feb 17, 2022 at 8:42 PM Marc Zyngier <maz@kernel.org> wrote:
>> 
>> On 2022-01-28 05:25, Anup Patel wrote:
>> > We should create INTC domain using a synthetic fwnode which will allow
>> > drivers (such as RISC-V SBI IPI driver, RISC-V timer driver, RISC-V
>> > PMU driver, etc) not having dedicated DT/ACPI node to directly create
>> > interrupt mapping for standard local interrupt numbers defined by the
>> > RISC-V privileged specification.
>> >
>> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>> > ---
>> >  arch/riscv/include/asm/irq.h      |  2 ++
>> >  arch/riscv/kernel/irq.c           | 13 +++++++++++++
>> >  drivers/clocksource/timer-clint.c | 13 +++++++------
>> >  drivers/clocksource/timer-riscv.c | 11 ++---------
>> >  drivers/irqchip/irq-riscv-intc.c  | 12 ++++++++++--
>> >  drivers/irqchip/irq-sifive-plic.c | 19 +++++++++++--------
>> >  6 files changed, 45 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/arch/riscv/include/asm/irq.h
>> > b/arch/riscv/include/asm/irq.h
>> > index e4c435509983..f85ebaf07505 100644
>> > --- a/arch/riscv/include/asm/irq.h
>> > +++ b/arch/riscv/include/asm/irq.h
>> > @@ -12,6 +12,8 @@
>> >
>> >  #include <asm-generic/irq.h>
>> >
>> > +extern struct fwnode_handle *riscv_intc_fwnode(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..f2fed78ab659 100644
>> > --- a/arch/riscv/kernel/irq.c
>> > +++ b/arch/riscv/kernel/irq.c
>> > @@ -7,9 +7,22 @@
>> >
>> >  #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 *intc_fwnode;
>> > +
>> > +struct fwnode_handle *riscv_intc_fwnode(void)
>> > +{
>> > +     if (!intc_fwnode)
>> > +             intc_fwnode = irq_domain_alloc_named_fwnode("RISCV-INTC");
>> > +
>> > +     return intc_fwnode;
>> > +}
>> > +EXPORT_SYMBOL_GPL(riscv_intc_fwnode);
>> 
>> Why is this created outside of the root interrupt controller driver?
>> Furthermore, why do you need to create a new fwnode the first place?
>> As far as I can tell, the INTC does have a node, and what you don't
>> have is the firmware linkage between PMU (an others) and the INTC.
> 
> Fair enough, I will update this patch to not create a synthetic fwnode.
> 
> The issue is not with INTC driver. We have other drivers and places
> (such as SBI IPI driver, SBI PMU driver, and KVM RISC-V AIA support)
> where we don't have a way to locate INTC fwnode.

And that's exactly what I am talking about: The INTC is OK (sort of),
but the firmware is too crap for words, and isn't even able to expose
where the various endpoints route their interrupts to.

Yes, this is probably fine today because you can describe the topology
of RISC-V systems on the surface of a post stamp. Once you get to the
complexity of a server-grade SoC (or worse, a mobile phone style SoC),
this *implicit topology* stuff doesn't fly, because there is no 
guarantee
that all endpoints will always all point to the same controller.

>> what you should have instead is something like:
>> 
>> static struct fwnode_handle *(*__get_root_intc_node)(void);
>> struct fwnode_handle *riscv_get_root_intc_hwnode(void)
>> {
>>          if (__get_root_intc_node)
>>                  return __get_root_intc_node();
>> 
>>          return NULL;
>> }
>> 
>> and the corresponding registration interface.
> 
> Thanks, I will follow this suggestion. This is a much better approach
> and it will avoid touching existing drivers.
> 
>> 
>> But either way, something breaks: the INTC has one node per CPU, and
>> expect one irqdomain per CPU. Having a single fwnode completely breaks
>> the INTC driver (and probably the irqdomain list, as we don't check 
>> for
>> duplicate entries).
>> 
>> > diff --git a/drivers/irqchip/irq-riscv-intc.c
>> > b/drivers/irqchip/irq-riscv-intc.c
>> > index b65bd8878d4f..26ed62c11768 100644
>> > --- a/drivers/irqchip/irq-riscv-intc.c
>> > +++ b/drivers/irqchip/irq-riscv-intc.c
>> > @@ -112,8 +112,16 @@ static int __init riscv_intc_init(struct
>> > device_node *node,
>> >       if (riscv_hartid_to_cpuid(hartid) != smp_processor_id())
>> >               return 0;
>> >
>> > -     intc_domain = irq_domain_add_linear(node, BITS_PER_LONG,
>> > -                                         &riscv_intc_domain_ops, NULL);
>> > +     /*
>> > +      * Create INTC domain using a synthetic fwnode which will allow
>> > +      * drivers (such as RISC-V SBI IPI driver, RISC-V timer driver,
>> > +      * RISC-V PMU driver, etc) not having dedicated DT/ACPI node to
>> > +      * directly create interrupt mapping for standard local interrupt
>> > +      * numbers defined by the RISC-V privileged specification.
>> > +      */
>> > +     intc_domain = irq_domain_create_linear(riscv_intc_fwnode(),
>> > +                                            BITS_PER_LONG,
>> > +                                            &riscv_intc_domain_ops, NULL);
>> 
>> This is what I'm talking about. It is simply broken. So either you 
>> don't
>> need a per-CPU node (and the DT was bad the first place), or you
>> absolutely need
>> one (and the whole 'well-known/default domain' doesn't work at all).
>> 
>> Either way, this patch is plain wrong.
> 
> Okay, I will update this patch with the new approach which you 
> suggested.

But how do you plan to work around the fact that everything is currently
build around having a node (and an irqdomain) per CPU? The PLIC, for 
example,
clearly has one parent per CPU, not one global parent.

I'm sure there was a good reason for this, and I suspect merging the 
domains
will simply end up breaking things.

         M.
-- 
Jazz is not dead. It just smells funny...