[v5,2/9] irqchip/riscv-intc: Add support for RISC-V AIA

  The RISC-V advanced interrupt architecture (AIA) extends the per-HART
local interrupts in following ways:
1. Minimum 64 local interrupts for both RV32 and RV64
2. Ability to process multiple pending local interrupts in same
   interrupt handler
3. Priority configuration for each local interrupts
4. Special CSRs to configure/access the per-HART MSI controller

This patch adds support for RISC-V AIA in the RISC-V intc driver.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 drivers/irqchip/irq-riscv-intc.c | 36 ++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)
  

On Mon, Jul 10, 2023 at 03:13:14PM +0530, Anup Patel wrote:
> The RISC-V advanced interrupt architecture (AIA) extends the per-HART
> local interrupts in following ways:
> 1. Minimum 64 local interrupts for both RV32 and RV64
> 2. Ability to process multiple pending local interrupts in same
>    interrupt handler
> 3. Priority configuration for each local interrupts
> 4. Special CSRs to configure/access the per-HART MSI controller

afaict, we're only doing (1) and (2) from this list in this patch.

> 
> This patch adds support for RISC-V AIA in the RISC-V intc driver.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  drivers/irqchip/irq-riscv-intc.c | 36 ++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index 4adeee1bc391..e235bf1708a4 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/smp.h>
> +#include <asm/hwcap.h>
>  
>  static struct irq_domain *intc_domain;
>  
> @@ -30,6 +31,15 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
>  	generic_handle_domain_irq(intc_domain, cause);
>  }
>  
> +static asmlinkage void riscv_intc_aia_irq(struct pt_regs *regs)
> +{
> +	unsigned long topi;
> +
> +	while ((topi = csr_read(CSR_TOPI)))
> +		generic_handle_domain_irq(intc_domain,
> +					  topi >> TOPI_IID_SHIFT);
> +}
> +
>  /*
>   * On RISC-V systems local interrupts are masked or unmasked by writing
>   * the SIE (Supervisor Interrupt Enable) CSR.  As CSRs can only be written
> @@ -39,12 +49,18 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
>  
>  static void riscv_intc_irq_mask(struct irq_data *d)
>  {
> -	csr_clear(CSR_IE, BIT(d->hwirq));
> +	if (d->hwirq < BITS_PER_LONG)
> +		csr_clear(CSR_IE, BIT(d->hwirq));
> +	else
> +		csr_clear(CSR_IEH, BIT(d->hwirq - BITS_PER_LONG));

We can optimize rv64 by allowing the compiler to remove the branch

 if (IS_ENABLED(CONFIG_32BIT) && d->hwirq >= 32)
    csr_clear(CSR_IEH, BIT(d->hwirq - 32));
 else
    csr_clear(CSR_IE, BIT(d->hwirq));


>  }
>  
>  static void riscv_intc_irq_unmask(struct irq_data *d)
>  {
> -	csr_set(CSR_IE, BIT(d->hwirq));
> +	if (d->hwirq < BITS_PER_LONG)
> +		csr_set(CSR_IE, BIT(d->hwirq));
> +	else
> +		csr_set(CSR_IEH, BIT(d->hwirq - BITS_PER_LONG));

Same comment as above.

>  }
>  
>  static void riscv_intc_irq_eoi(struct irq_data *d)
> @@ -115,16 +131,22 @@ static struct fwnode_handle *riscv_intc_hwnode(void)
>  
>  static int __init riscv_intc_init_common(struct fwnode_handle *fn)
>  {
> -	int rc;
> +	int rc, nr_irqs = BITS_PER_LONG;
> +
> +	if (riscv_isa_extension_available(NULL, SxAIA) && BITS_PER_LONG == 32)
> +		nr_irqs = nr_irqs * 2;

The AIA spec states sie and sip are explicitly 64, so how about writing
this as

 int rc, nr_irqs = BITS_PER_LONG;

 if (riscv_isa_extension_available(NULL, SxAIA))
     nr_irqs = 64;

>  
> -	intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
> +	intc_domain = irq_domain_create_linear(fn, nr_irqs,
>  					       &riscv_intc_domain_ops, NULL);
>  	if (!intc_domain) {
>  		pr_err("unable to add IRQ domain\n");
>  		return -ENXIO;
>  	}
>  
> -	rc = set_handle_irq(&riscv_intc_irq);
> +	if (riscv_isa_extension_available(NULL, SxAIA))
> +		rc = set_handle_irq(&riscv_intc_aia_irq);
> +	else
> +		rc = set_handle_irq(&riscv_intc_irq);

nit: blank line here

>  	if (rc) {
>  		pr_err("failed to set irq handler\n");
>  		return rc;
> @@ -132,7 +154,9 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
>  
>  	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
>  
> -	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> +	pr_info("%d local interrupts mapped%s\n",
> +		nr_irqs, (riscv_isa_extension_available(NULL, SxAIA)) ?

nit: unnecessary ()

> +			 " using AIA" : "");
>  
>  	return 0;
>  }
> -- 
> 2.34.1
>

Thanks,
drew
  

On Tue, Jul 11, 2023 at 7:42 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jul 10, 2023 at 03:13:14PM +0530, Anup Patel wrote:
> > The RISC-V advanced interrupt architecture (AIA) extends the per-HART
> > local interrupts in following ways:
> > 1. Minimum 64 local interrupts for both RV32 and RV64
> > 2. Ability to process multiple pending local interrupts in same
> >    interrupt handler
> > 3. Priority configuration for each local interrupts
> > 4. Special CSRs to configure/access the per-HART MSI controller
>
> afaict, we're only doing (1) and (2) from this list in this patch.

Okay, I will update the commit description.

>
> >
> > This patch adds support for RISC-V AIA in the RISC-V intc driver.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  drivers/irqchip/irq-riscv-intc.c | 36 ++++++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> > index 4adeee1bc391..e235bf1708a4 100644
> > --- a/drivers/irqchip/irq-riscv-intc.c
> > +++ b/drivers/irqchip/irq-riscv-intc.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/smp.h>
> > +#include <asm/hwcap.h>
> >
> >  static struct irq_domain *intc_domain;
> >
> > @@ -30,6 +31,15 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
> >       generic_handle_domain_irq(intc_domain, cause);
> >  }
> >
> > +static asmlinkage void riscv_intc_aia_irq(struct pt_regs *regs)
> > +{
> > +     unsigned long topi;
> > +
> > +     while ((topi = csr_read(CSR_TOPI)))
> > +             generic_handle_domain_irq(intc_domain,
> > +                                       topi >> TOPI_IID_SHIFT);
> > +}
> > +
> >  /*
> >   * On RISC-V systems local interrupts are masked or unmasked by writing
> >   * the SIE (Supervisor Interrupt Enable) CSR.  As CSRs can only be written
> > @@ -39,12 +49,18 @@ static asmlinkage void riscv_intc_irq(struct pt_regs *regs)
> >
> >  static void riscv_intc_irq_mask(struct irq_data *d)
> >  {
> > -     csr_clear(CSR_IE, BIT(d->hwirq));
> > +     if (d->hwirq < BITS_PER_LONG)
> > +             csr_clear(CSR_IE, BIT(d->hwirq));
> > +     else
> > +             csr_clear(CSR_IEH, BIT(d->hwirq - BITS_PER_LONG));
>
> We can optimize rv64 by allowing the compiler to remove the branch
>
>  if (IS_ENABLED(CONFIG_32BIT) && d->hwirq >= 32)
>     csr_clear(CSR_IEH, BIT(d->hwirq - 32));
>  else
>     csr_clear(CSR_IE, BIT(d->hwirq));
>

Makes sense, I will update.

>
> >  }
> >
> >  static void riscv_intc_irq_unmask(struct irq_data *d)
> >  {
> > -     csr_set(CSR_IE, BIT(d->hwirq));
> > +     if (d->hwirq < BITS_PER_LONG)
> > +             csr_set(CSR_IE, BIT(d->hwirq));
> > +     else
> > +             csr_set(CSR_IEH, BIT(d->hwirq - BITS_PER_LONG));
>
> Same comment as above.

Okay, I will update.

>
> >  }
> >
> >  static void riscv_intc_irq_eoi(struct irq_data *d)
> > @@ -115,16 +131,22 @@ static struct fwnode_handle *riscv_intc_hwnode(void)
> >
> >  static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> >  {
> > -     int rc;
> > +     int rc, nr_irqs = BITS_PER_LONG;
> > +
> > +     if (riscv_isa_extension_available(NULL, SxAIA) && BITS_PER_LONG == 32)
> > +             nr_irqs = nr_irqs * 2;
>
> The AIA spec states sie and sip are explicitly 64, so how about writing
> this as
>
>  int rc, nr_irqs = BITS_PER_LONG;
>
>  if (riscv_isa_extension_available(NULL, SxAIA))
>      nr_irqs = 64;

Okay, I will update.

>
> >
> > -     intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
> > +     intc_domain = irq_domain_create_linear(fn, nr_irqs,
> >                                              &riscv_intc_domain_ops, NULL);
> >       if (!intc_domain) {
> >               pr_err("unable to add IRQ domain\n");
> >               return -ENXIO;
> >       }
> >
> > -     rc = set_handle_irq(&riscv_intc_irq);
> > +     if (riscv_isa_extension_available(NULL, SxAIA))
> > +             rc = set_handle_irq(&riscv_intc_aia_irq);
> > +     else
> > +             rc = set_handle_irq(&riscv_intc_irq);
>
> nit: blank line here

I prefer no blank line here because the "if (rc)" below
checks for errors for the above calls.

>
> >       if (rc) {
> >               pr_err("failed to set irq handler\n");
> >               return rc;
> > @@ -132,7 +154,9 @@ static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> >
> >       riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
> >
> > -     pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> > +     pr_info("%d local interrupts mapped%s\n",
> > +             nr_irqs, (riscv_isa_extension_available(NULL, SxAIA)) ?
>
> nit: unnecessary ()

Okay, I will update.

>
> > +                      " using AIA" : "");
> >
> >       return 0;
> >  }
> > --
> > 2.34.1
> >
>
> Thanks,
> drew

Regards,
Anup