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

[Anup Patel]

On Tue, Jul 11, 2023 at 7:42 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > ---
> >  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