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
