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

Commit Message

Anup Patel Jan. 28, 2022, 5:25 a.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Marc Zyngier Feb. 17, 2022, 3:12 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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.

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.

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.


         M.

Anup Patel Feb. 19, 2022, 3:38 a.m. UTC | #2

On Thu, Feb 17, 2022 at 8:42 PM Marc Zyngier <[e-mail]> 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 <[e-mail]>
> > ---
> >  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.

>
> 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.

Regards,
Anup

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

Marc Zyngier Feb. 19, 2022, 9:32 a.m. UTC | #3

On 2022-02-19 03:38, Anup Patel wrote:
> On Thu, Feb 17, 2022 at 8:42 PM Marc Zyngier <[e-mail]> 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 <[e-mail]>
>> > ---
>> >  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.

Anup Patel Feb. 19, 2022, 1:03 p.m. UTC | #4

On Sat, Feb 19, 2022 at 3:02 PM Marc Zyngier <[e-mail]> wrote:
>
> On 2022-02-19 03:38, Anup Patel wrote:
> > On Thu, Feb 17, 2022 at 8:42 PM Marc Zyngier <[e-mail]> 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 <[e-mail]>
> >> > ---
> >> >  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.

The firmware is always present at a higher privilege-level hence there
is no DT node to discover the presence of firmware. The local interrupts
used by the firmware for IPI, Timer and PMU are defined by the RISC-V
ISA specification.

>
> 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.

The local interrupts (per-CPU) are always managed by the INTC. The
interrupt controllers to manage device interrupts (such as PLIC) can
vary from platform to platform and have INTC as the parent domain.

We already have high-end interrupt controllers (such as AIA) under
development which are scalable for server-grade SoC, mobile SoC
and various other types of SoCs.

We are able to describe the topology of different types of interrupt
controllers (PLIC as well as AIA) in DT.

The only issue is for drivers which do not have dedicated DT node
(such as SBI IPI, SBI Timer, SBI PMU, or KVM RISC-V) but the
upside is that local interrupt numbers used by these drivers is
clearly defined by the RISC-V ISA specification:

Here are the local interrupts defined by the RISC-V ISA spec:
IRQ13 => PMU overflow interrupt (used by SBI PMU driver)
IRQ12 => S-mode guest external interrupt (to be used by KVM RISC-V)
IRQ11 => M-mode external interrupt (used by firmware)
IRQ9  => S-mode external interrupt (used by PLIC driver)
IRQ7  => M-mode timer interrupt
IRQ5  => S-mode timer interrupt (used by SBI Timer driver)
IRQ3  => M-mode software interrupt (used by firmware)
IRQ1 =>  S-mode software interrupt (used by SBI IPI driver)

>
> >> 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.

We can have multiple PLIC instances in a platform and each PLIC
instance targets a subset of CPUs. Further, each PLIC instance has
multiple PLIC contexts for associated CPUs.

The per-CPU INTC DT nodes and the "interrupts-extended" DT
property of PLIC DT node helps us describe the association
between various PLIC contexts and CPUs.

Here's an example PLIC DT node:

    plic: interrupt-controller@c000000 {
      #address-cells = <0>;
      #interrupt-cells = <1>;
      compatible = "sifive,fu540-c000-plic", "sifive,plic-1.0.0";
      interrupt-controller;
      interrupts-extended = <&cpu0_intc 11>,
                            <&cpu1_intc 11>, <&cpu1_intc 9>,
                            <&cpu2_intc 11>, <&cpu2_intc 9>,
                            <&cpu3_intc 11>, <&cpu3_intc 9>,
                            <&cpu4_intc 11>, <&cpu4_intc 9>;
      reg = <0xc000000 0x4000000>;
      riscv,ndev = <10>;
    };

In above above example, PLIC has 9 contexts and context
to CPU connections are as follows:
PLIC context0 => CPU0 M-mode external interrupt
PLIC context1 => CPU1 M-mode external interrupt
PLIC context2 => CPU1 S-mode external interrupt
PLIC context3 => CPU2 M-mode external interrupt
PLIC context4 => CPU2 S-mode external interrupt
....

This is just one example and we can describe any kind of
PLIC context to CPU connections using "interrupts-extended"
DT property.

The same level of flexibility is provided by AIA interrupt
controllers which are under development.

Regards,
Anup

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

Jessica Clarke Feb. 19, 2022, 2:51 p.m. UTC | #5

On 19 Feb 2022, at 09:32, Marc Zyngier <[e-mail]> wrote:
> 
> On 2022-02-19 03:38, Anup Patel wrote:
>> On Thu, Feb 17, 2022 at 8:42 PM Marc Zyngier <[e-mail]> 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 <[e-mail]>
>>> > ---
>>> >  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.

On the contrary, the drivers rely on the controller being the same
across all harts, with riscv_intc_init skipping initialisation for all
but the boot hart’s controller. The bindings are a complete pain to
deal with as a result, what you *want* is like you have in the Arm
world where there is just one interrupt controller in the device tree
with some of the interrupts per-processor, but instead we have this
overengineered nuisance. The only reason there are per-hart interrupt
controllers is because that’s how the contexts for the CLINT/PLIC are
specified, but that really should have been done another way rather
than abusing the interrupts-extended property for that. In the FreeBSD
world we’ve been totally ignoring the device tree nodes for the local
interrupt controllers but for my AIA and ACLINT branch I started a few
months ago (though ACLINT's now been completely screwed up by RVI
politics, things have been renamed and split up differently in the past
few days and software interrupts de-prioritised with no current path to
ratification, so that was a waste of my time) I just hang the driver
off the boot hart’s node and leave all the others as totally ignored
and a waste of space other than to figure out the contexts for the PLIC
etc.

TL;DR yes the bindings are awful, no there’s no issue with merging the
domains.

Jess

Marc Zyngier Feb. 21, 2022, 9:07 a.m. UTC | #6

On 2022-02-19 13:03, Anup Patel wrote:
> On Sat, Feb 19, 2022 at 3:02 PM Marc Zyngier <[e-mail]> wrote:
>> 
>> On 2022-02-19 03:38, Anup Patel wrote:
>> > On Thu, Feb 17, 2022 at 8:42 PM Marc Zyngier <[e-mail]> 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 <[e-mail]>
>> >> > ---
>> >> >  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.
> 
> The firmware is always present at a higher privilege-level hence there
> is no DT node to discover the presence of firmware. The local 
> interrupts
> used by the firmware for IPI, Timer and PMU are defined by the RISC-V
> ISA specification.
> 
>> 
>> 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.
> 
> The local interrupts (per-CPU) are always managed by the INTC. The
> interrupt controllers to manage device interrupts (such as PLIC) can
> vary from platform to platform and have INTC as the parent domain.

I don't know how to make it clearer: this isn't about the situation
*today*. It is about what you will have two or five years from now.

Relying on a default topology is stupidly bad, and you will end-up
in a terrible corner eventually, because you can't have *two*
defaults.

> 
> We already have high-end interrupt controllers (such as AIA) under
> development which are scalable for server-grade SoC, mobile SoC
> and various other types of SoCs.
> 
> We are able to describe the topology of different types of interrupt
> controllers (PLIC as well as AIA) in DT.
> 
> The only issue is for drivers which do not have dedicated DT node
> (such as SBI IPI, SBI Timer, SBI PMU, or KVM RISC-V) but the
> upside is that local interrupt numbers used by these drivers is
> clearly defined by the RISC-V ISA specification:
> 
> Here are the local interrupts defined by the RISC-V ISA spec:
> IRQ13 => PMU overflow interrupt (used by SBI PMU driver)
> IRQ12 => S-mode guest external interrupt (to be used by KVM RISC-V)
> IRQ11 => M-mode external interrupt (used by firmware)
> IRQ9  => S-mode external interrupt (used by PLIC driver)
> IRQ7  => M-mode timer interrupt
> IRQ5  => S-mode timer interrupt (used by SBI Timer driver)
> IRQ3  => M-mode software interrupt (used by firmware)
> IRQ1 =>  S-mode software interrupt (used by SBI IPI driver)

Again, you are missing the point. It isn't about the interrupt
number (nobody gives a crap about them). It is about the entity
the device is connected to. No amount of copy pasting of the
spec changes that.

>> 
>> >> 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.
> 
> We can have multiple PLIC instances in a platform and each PLIC
> instance targets a subset of CPUs. Further, each PLIC instance has
> multiple PLIC contexts for associated CPUs.
> 
> The per-CPU INTC DT nodes and the "interrupts-extended" DT
> property of PLIC DT node helps us describe the association
> between various PLIC contexts and CPUs.
> 
> Here's an example PLIC DT node:
> 
>     plic: interrupt-controller@c000000 {
>       #address-cells = <0>;
>       #interrupt-cells = <1>;
>       compatible = "sifive,fu540-c000-plic", "sifive,plic-1.0.0";
>       interrupt-controller;
>       interrupts-extended = <&cpu0_intc 11>,
>                             <&cpu1_intc 11>, <&cpu1_intc 9>,
>                             <&cpu2_intc 11>, <&cpu2_intc 9>,
>                             <&cpu3_intc 11>, <&cpu3_intc 9>,
>                             <&cpu4_intc 11>, <&cpu4_intc 9>;
>       reg = <0xc000000 0x4000000>;
>       riscv,ndev = <10>;
>     };
> 
> In above above example, PLIC has 9 contexts and context
> to CPU connections are as follows:
> PLIC context0 => CPU0 M-mode external interrupt
> PLIC context1 => CPU1 M-mode external interrupt
> PLIC context2 => CPU1 S-mode external interrupt
> PLIC context3 => CPU2 M-mode external interrupt
> PLIC context4 => CPU2 S-mode external interrupt
> ....

Asymmetric interrupt routing. How lovely. How broken.

> 
> This is just one example and we can describe any kind of
> PLIC context to CPU connections using "interrupts-extended"
> DT property.
> 
> The same level of flexibility is provided by AIA interrupt
> controllers which are under development.

How promising. I really hope someone will eventually barge in
and clean this mess.

           M.

Marc Zyngier Feb. 21, 2022, 9:25 a.m. UTC | #7

On Sat, 19 Feb 2022 14:51:22 +0000,
Jessica Clarke <[e-mail]> wrote:
> 
> On 19 Feb 2022, at 09:32, Marc Zyngier <[e-mail]> wrote:
> > 
> > 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.
> 
> On the contrary, the drivers rely on the controller being the same
> across all harts, with riscv_intc_init skipping initialisation for all
> but the boot hart’s controller. The bindings are a complete pain to
> deal with as a result, what you *want* is like you have in the Arm
> world where there is just one interrupt controller in the device tree
> with some of the interrupts per-processor, but instead we have this
> overengineered nuisance. The only reason there are per-hart interrupt
> controllers is because that’s how the contexts for the CLINT/PLIC are
> specified, but that really should have been done another way rather
> than abusing the interrupts-extended property for that. In the FreeBSD
> world we’ve been totally ignoring the device tree nodes for the local
> interrupt controllers but for my AIA and ACLINT branch I started a few
> months ago (though ACLINT's now been completely screwed up by RVI
> politics, things have been renamed and split up differently in the past
> few days and software interrupts de-prioritised with no current path to
> ratification, so that was a waste of my time) I just hang the driver
> off the boot hart’s node and leave all the others as totally ignored
> and a waste of space other than to figure out the contexts for the PLIC
> etc.
> 
> TL;DR yes the bindings are awful, no there’s no issue with merging the
> domains.

I don't know how that flies with something like[1], where CPU0 only
gets interrupts in M-Mode and not S-Mode. Maybe it doesn't really
matter, but this sort of asymmetric routing is totally backward.

It sometime feels like the RV folks are actively trying to make this
architecture a mess... :-/

	M.

[1] [e-mail]

Anup Patel Feb. 21, 2022, 9:38 a.m. UTC | #8

On Mon, Feb 21, 2022 at 2:37 PM Marc Zyngier <[e-mail]> wrote:
>
> On 2022-02-19 13:03, Anup Patel wrote:
> > On Sat, Feb 19, 2022 at 3:02 PM Marc Zyngier <[e-mail]> wrote:
> >>
> >> On 2022-02-19 03:38, Anup Patel wrote:
> >> > On Thu, Feb 17, 2022 at 8:42 PM Marc Zyngier <[e-mail]> 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 <[e-mail]>
> >> >> > ---
> >> >> >  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.
> >
> > The firmware is always present at a higher privilege-level hence there
> > is no DT node to discover the presence of firmware. The local
> > interrupts
> > used by the firmware for IPI, Timer and PMU are defined by the RISC-V
> > ISA specification.
> >
> >>
> >> 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.
> >
> > The local interrupts (per-CPU) are always managed by the INTC. The
> > interrupt controllers to manage device interrupts (such as PLIC) can
> > vary from platform to platform and have INTC as the parent domain.
>
> I don't know how to make it clearer: this isn't about the situation
> *today*. It is about what you will have two or five years from now.
>
> Relying on a default topology is stupidly bad, and you will end-up
> in a terrible corner eventually, because you can't have *two*
> defaults.
>
> >
> > We already have high-end interrupt controllers (such as AIA) under
> > development which are scalable for server-grade SoC, mobile SoC
> > and various other types of SoCs.
> >
> > We are able to describe the topology of different types of interrupt
> > controllers (PLIC as well as AIA) in DT.
> >
> > The only issue is for drivers which do not have dedicated DT node
> > (such as SBI IPI, SBI Timer, SBI PMU, or KVM RISC-V) but the
> > upside is that local interrupt numbers used by these drivers is
> > clearly defined by the RISC-V ISA specification:
> >
> > Here are the local interrupts defined by the RISC-V ISA spec:
> > IRQ13 => PMU overflow interrupt (used by SBI PMU driver)
> > IRQ12 => S-mode guest external interrupt (to be used by KVM RISC-V)
> > IRQ11 => M-mode external interrupt (used by firmware)
> > IRQ9  => S-mode external interrupt (used by PLIC driver)
> > IRQ7  => M-mode timer interrupt
> > IRQ5  => S-mode timer interrupt (used by SBI Timer driver)
> > IRQ3  => M-mode software interrupt (used by firmware)
> > IRQ1 =>  S-mode software interrupt (used by SBI IPI driver)
>
> Again, you are missing the point. It isn't about the interrupt
> number (nobody gives a crap about them). It is about the entity
> the device is connected to. No amount of copy pasting of the
> spec changes that.

The INTC CSRs are part of the RISC-V privileged specification
and maintaining backward compatibility is of utmost importance in
RISC-V ISA so I don't see why per-CPU INTC will be replaced
by something totally incompatible in future.

>
> >>
> >> >> 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.
> >
> > We can have multiple PLIC instances in a platform and each PLIC
> > instance targets a subset of CPUs. Further, each PLIC instance has
> > multiple PLIC contexts for associated CPUs.
> >
> > The per-CPU INTC DT nodes and the "interrupts-extended" DT
> > property of PLIC DT node helps us describe the association
> > between various PLIC contexts and CPUs.
> >
> > Here's an example PLIC DT node:
> >
> >     plic: interrupt-controller@c000000 {
> >       #address-cells = <0>;
> >       #interrupt-cells = <1>;
> >       compatible = "sifive,fu540-c000-plic", "sifive,plic-1.0.0";
> >       interrupt-controller;
> >       interrupts-extended = <&cpu0_intc 11>,
> >                             <&cpu1_intc 11>, <&cpu1_intc 9>,
> >                             <&cpu2_intc 11>, <&cpu2_intc 9>,
> >                             <&cpu3_intc 11>, <&cpu3_intc 9>,
> >                             <&cpu4_intc 11>, <&cpu4_intc 9>;
> >       reg = <0xc000000 0x4000000>;
> >       riscv,ndev = <10>;
> >     };
> >
> > In above above example, PLIC has 9 contexts and context
> > to CPU connections are as follows:
> > PLIC context0 => CPU0 M-mode external interrupt
> > PLIC context1 => CPU1 M-mode external interrupt
> > PLIC context2 => CPU1 S-mode external interrupt
> > PLIC context3 => CPU2 M-mode external interrupt
> > PLIC context4 => CPU2 S-mode external interrupt
> > ....
>
> Asymmetric interrupt routing. How lovely. How broken.

This is a known issue with PLIC where contexts from the same
PLIC can target both M-mode and S-mode. This asymmetric
interrupt routing is not there in under-development AIA.

>
> >
> > This is just one example and we can describe any kind of
> > PLIC context to CPU connections using "interrupts-extended"
> > DT property.
> >
> > The same level of flexibility is provided by AIA interrupt
> > controllers which are under development.
>
> How promising. I really hope someone will eventually barge in
> and clean this mess.

The INTC DT node being per-CPU is the only strange thing
in RISC-V but we can't change INTC DT bindings at this stage.

Apart from this, I still don't see anything messy here.

Regards,
Anup

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

Anup Patel Feb. 21, 2022, 9:44 a.m. UTC | #9

On Mon, Feb 21, 2022 at 2:55 PM Marc Zyngier <[e-mail]> wrote:
>
> On Sat, 19 Feb 2022 14:51:22 +0000,
> Jessica Clarke <[e-mail]> wrote:
> >
> > On 19 Feb 2022, at 09:32, Marc Zyngier <[e-mail]> wrote:
> > >
> > > 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.
> >
> > On the contrary, the drivers rely on the controller being the same
> > across all harts, with riscv_intc_init skipping initialisation for all
> > but the boot hart’s controller. The bindings are a complete pain to
> > deal with as a result, what you *want* is like you have in the Arm
> > world where there is just one interrupt controller in the device tree
> > with some of the interrupts per-processor, but instead we have this
> > overengineered nuisance. The only reason there are per-hart interrupt
> > controllers is because that’s how the contexts for the CLINT/PLIC are
> > specified, but that really should have been done another way rather
> > than abusing the interrupts-extended property for that. In the FreeBSD
> > world we’ve been totally ignoring the device tree nodes for the local
> > interrupt controllers but for my AIA and ACLINT branch I started a few
> > months ago (though ACLINT's now been completely screwed up by RVI
> > politics, things have been renamed and split up differently in the past
> > few days and software interrupts de-prioritised with no current path to
> > ratification, so that was a waste of my time) I just hang the driver
> > off the boot hart’s node and leave all the others as totally ignored
> > and a waste of space other than to figure out the contexts for the PLIC
> > etc.
> >
> > TL;DR yes the bindings are awful, no there’s no issue with merging the
> > domains.
>
> I don't know how that flies with something like[1], where CPU0 only
> gets interrupts in M-Mode and not S-Mode. Maybe it doesn't really
> matter, but this sort of asymmetric routing is totally backward.

The example PLIC DT node which I provided is from SiFive FU540, where
we have 5 CPUs. The CPU0 in FU540 is a cache-coherent microcontroller
having only M-mode (i.e. No MMU hence not Linux capable).

>
> It sometime feels like the RV folks are actively trying to make this
> architecture a mess... :-/

Well, I still fail to understand what is messy here.

Regards,
Anup

>
>         M.
>
> [1] [e-mail]
>
> --
> Without deviation from the norm, progress is not possible.

Patch

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);
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 	show_ipi_stats(p, prec);
diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
index 6cfe2ab73eb0..6e5624989525 100644
--- a/drivers/clocksource/timer-clint.c
+++ b/drivers/clocksource/timer-clint.c
@@ -149,6 +149,7 @@  static int __init clint_timer_init_dt(struct device_node *np)
 	int rc;
 	u32 i, nr_irqs;
 	void __iomem *base;
+	struct irq_domain *domain;
 	struct of_phandle_args oirq;
 
 	/*
@@ -169,14 +170,14 @@  static int __init clint_timer_init_dt(struct device_node *np)
 			       np, i, oirq.args[0]);
 			return -ENODEV;
 		}
-
-		/* Find parent irq domain and map timer irq */
-		if (!clint_timer_irq &&
-		    oirq.args[0] == RV_IRQ_TIMER &&
-		    irq_find_host(oirq.np))
-			clint_timer_irq = irq_of_parse_and_map(np, i);
 	}
 
+	/* Find parent irq domain and map timer irq */
+	domain = irq_find_matching_fwnode(riscv_intc_fwnode(),
+					  DOMAIN_BUS_ANY);
+	if (!clint_timer_irq && domain)
+		clint_timer_irq = irq_create_mapping(domain, RV_IRQ_TIMER);
+
 	/* If CLINT timer irq not found then fail */
 	if (!clint_timer_irq) {
 		pr_err("%pOFP: timer irq not found\n", np);
diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index 1767f8bf2013..a98f5d18bab9 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -102,7 +102,6 @@  static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id)
 static int __init riscv_timer_init_dt(struct device_node *n)
 {
 	int cpuid, hartid, error;
-	struct device_node *child;
 	struct irq_domain *domain;
 
 	hartid = riscv_of_processor_hartid(n);
@@ -121,14 +120,8 @@  static int __init riscv_timer_init_dt(struct device_node *n)
 	if (cpuid != smp_processor_id())
 		return 0;
 
-	domain = NULL;
-	child = of_get_compatible_child(n, "riscv,cpu-intc");
-	if (!child) {
-		pr_err("Failed to find INTC node [%pOF]\n", n);
-		return -ENODEV;
-	}
-	domain = irq_find_host(child);
-	of_node_put(child);
+	domain = irq_find_matching_fwnode(riscv_intc_fwnode(),
+					  DOMAIN_BUS_ANY);
 	if (!domain) {
 		pr_err("Failed to find IRQ domain for node [%pOF]\n", n);
 		return -ENODEV;
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);
 	if (!intc_domain) {
 		pr_err("unable to add IRQ domain\n");
 		return -ENXIO;
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 259065d271ef..2c43ab77c014 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -284,6 +284,7 @@  static int __init plic_init(struct device_node *node,
 	u32 nr_irqs;
 	struct plic_priv *priv;
 	struct plic_handler *handler;
+	struct irq_domain *domain;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -339,14 +340,6 @@  static int __init plic_init(struct device_node *node,
 			continue;
 		}
 
-		/* Find parent domain and register chained handler */
-		if (!plic_parent_irq && irq_find_host(parent.np)) {
-			plic_parent_irq = irq_of_parse_and_map(node, i);
-			if (plic_parent_irq)
-				irq_set_chained_handler(plic_parent_irq,
-							plic_handle_irq);
-		}
-
 		/*
 		 * When running in M-mode we need to ignore the S-mode handler.
 		 * Here we assume it always comes later, but that might be a
@@ -373,6 +366,16 @@  static int __init plic_init(struct device_node *node,
 		nr_handlers++;
 	}
 
+	/* Find parent domain and register chained handler */
+	domain = irq_find_matching_fwnode(riscv_intc_fwnode(),
+					  DOMAIN_BUS_ANY);
+	if (!plic_parent_irq && domain) {
+		plic_parent_irq = irq_create_mapping(domain, RV_IRQ_EXT);
+		if (plic_parent_irq)
+			irq_set_chained_handler(plic_parent_irq,
+						plic_handle_irq);
+	}
+
 	/*
 	 * We can have multiple PLIC instances so setup cpuhp state only
 	 * when context handler for current/boot CPU is present.