Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 13 Jan 2023 09:39:17 +0000
Message-ID: <868ri6ojsq.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Anup Patel <apatel@ventanamicro.com>
Cc:     Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Atish Patra <atishp@atishpatra.org>,
        Alistair Francis <Alistair.Francis@wdc.com>,
        Anup Patel <anup@brainfault.org>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v2 3/9] irqchip/riscv-intc: Add support for RISC-V AIA
In-Reply-To: <20230103141409.772298-4-apatel@ventanamicro.com>
References: <20230103141409.772298-1-apatel@ventanamicro.com>
        <20230103141409.772298-4-apatel@ventanamicro.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 03 Jan 2023 14:14:03 +0000,
Anup Patel <apatel@ventanamicro.com> 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
> 
> 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 | 37 ++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index f229e3e66387..880d1639aadc 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -16,6 +16,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/smp.h>
> +#include <asm/hwcap.h>
>  
>  static struct irq_domain *intc_domain;
>  
> @@ -29,6 +30,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)

What does "static asmlinkage" in a C file even mean? And clearly, this
isn't the only instance in this file...

> +{
> +	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
> @@ -38,12 +48,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)

And what if BIT_PER_LONG is 32, as I expect it to be on 32bit, which
the commit message says is supported?

> +		csr_clear(CSR_IE, BIT(d->hwirq));
> +	else
> +		csr_clear(CSR_IEH, BIT(d->hwirq - BITS_PER_LONG));
>  }
>  
>  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));
>  }
>  
>  static void riscv_intc_irq_eoi(struct irq_data *d)
> @@ -115,7 +131,7 @@ static struct fwnode_handle *riscv_intc_hwnode(void)
>  static int __init riscv_intc_init(struct device_node *node,
>  				  struct device_node *parent)
>  {
> -	int rc;
> +	int rc, nr_irqs;
>  	unsigned long hartid;
>  
>  	rc = riscv_of_parent_hartid(node, &hartid);
> @@ -133,14 +149,21 @@ 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,
> +	nr_irqs = BITS_PER_LONG;
> +	if (riscv_isa_extension_available(NULL, SxAIA) && BITS_PER_LONG == 32)
> +		nr_irqs = nr_irqs * 2;

Really, please drop this BITS_PER_LONG stuff. Use explicit numbers.

	M.

-- 
Without deviation from the norm, progress is not possible.