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:   Tue, 12 Jul 2022 20:12:58 +0100
Message-ID: <87a69e16x1.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Konrad Dybcio <konrad.dybcio@somainline.org>
Cc:     ~postmarketos/upstreaming@lists.sr.ht, martin.botka@somainline.org,
        angelogioacchino.delregno@somainline.org,
        marijn.suijten@somainline.org, jamipkettunen@somainline.org,
        Hector Martin <marcan@marcan.st>,
        Sven Peter <sven@svenpeter.dev>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs
In-Reply-To: <20220712160919.740878-2-konrad.dybcio@somainline.org>
References: <20220712160919.740878-1-konrad.dybcio@somainline.org>
        <20220712160919.740878-2-konrad.dybcio@somainline.org>
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/27.1
 (x86_64-pc-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

Hi Konrad,

Please add a cover letter when sending more than a single patch.

On Tue, 12 Jul 2022 17:09:19 +0100,
Konrad Dybcio <konrad.dybcio@somainline.org> wrote:
> 
> Add support for A7-A11 SoCs by if-ing out some features only present on
> A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the
> older SoCs don't implement EL2).
> 
> Also, annotate IPI regs support (A11 and newer*) so that the driver can
> tell whether the SoC supports these (they are written to even if fast
> IPI is disabled, when the registers are there of course).
> 
> *A11 is supposed to use this feature, but it is currently not working.
> That said, it is not yet necessary, especially with only one core up,
> and it works a-ok using the same featureset as earlier SoCs.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> ---
>  drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
> index 12dd48727a15..36f4b52addc2 100644
> --- a/drivers/irqchip/irq-apple-aic.c
> +++ b/drivers/irqchip/irq-apple-aic.c
> @@ -245,7 +245,10 @@ struct aic_info {
>  	u32 die_stride;
>  
>  	/* Features */
> +	bool el2_regs;
>  	bool fast_ipi;
> +	bool ipi_regs;
> +	bool uncore2_regs;
>  };
>  
>  static const struct aic_info aic1_info = {
> @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = {
>  	.event		= AIC_EVENT,
>  	.target_cpu	= AIC_TARGET_CPU,
>  
> +	.el2_regs	= true,
>  	.fast_ipi	= true,
> +	.ipi_regs	= true,
> +	.uncore2_regs	= true,
>  };
>  
>  static const struct aic_info aic2_info = {
> @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = {
>  
>  	.irq_cfg	= AIC2_IRQ_CFG,
>  
> +	.el2_regs	= true,
>  	.fast_ipi	= true,
> +	.ipi_regs	= true,
> +	.uncore2_regs	= true,

So to sum it up, all recent cores have all the cool features, and the
older ones have none of them. Surely we can do better than adding 3
fields that have the same value. Turn 'fast_ipi' into something that
means 'full_fat', and key everything on that.

And if this is meant to evolve into a more differentiated set of
features, the usual idiom is to have a set of flags as part of an
unsigned long instead of a set of booleans.

>  };
>  
>  static const struct of_device_id aic_info_match[] = {
> @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d)
>  
>  static void aic_fiq_set_mask(struct irq_data *d)
>  {
> +	if (!aic_irqc->info.el2_regs)
> +		return;

Why? AIC_TMR_EL02_PHYS is defined as the interrupt that fires in the
context of a guest. There is no guest here (no EL2 either), so what
you should have as interrupt number is AIC_TMR_EL0_{PHYS,VIRT}, and
this change becomes irrelevant (nothing to mask). Which is also what
happens when running an M1 under the m1n1 hypervisor.

> +
>  	/* Only the guest timers have real mask bits, unfortunately. */
>  	switch (aic_fiq_get_idx(d)) {
>  	case AIC_TMR_EL02_PHYS:
> @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d)
>  
>  static void aic_fiq_clear_mask(struct irq_data *d)
>  {
> +	if (!aic_irqc->info.el2_regs)
> +		return;
> +
>  	switch (aic_fiq_get_idx(d)) {
>  	case AIC_TMR_EL02_PHYS:
>  		sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, VM_TMR_FIQ_ENABLE_P);
> @@ -524,12 +539,14 @@ static void __exception_irq_entry aic_handle_fiq(struct pt_regs *regs)
>  	 * we check for everything here, even things we don't support yet.
>  	 */
>  
> -	if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
> -		if (static_branch_likely(&use_fast_ipi)) {
> -			aic_handle_ipi(regs);
> -		} else {
> -			pr_err_ratelimited("Fast IPI fired. Acking.\n");
> -			write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
> +	if (aic_irqc->info.ipi_regs) {

This is probably the hottest path in the whole kernel. Do we want an
extra read here? Absolutely not. At the very least, this should be a
static key.

> +		if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
> +			if (static_branch_likely(&use_fast_ipi)) {
> +				aic_handle_ipi(regs);
> +			} else {
> +				pr_err_ratelimited("Fast IPI fired. Acking.\n");
> +				write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
> +			}
>  		}
>  	}
>  
> @@ -566,12 +583,14 @@ static void __exception_irq_entry aic_handle_fiq(struct pt_regs *regs)
>  					  AIC_FIQ_HWIRQ(irq));
>  	}
>  
> -	if (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ &&
> -			(read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
> -		/* Same story with uncore PMCs */
> -		pr_err_ratelimited("Uncore PMC FIQ fired. Masking.\n");
> -		sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> -				   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
> +	if (aic_irqc->info.uncore2_regs) {

Same thing.

> +		if (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ &&
> +				(read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
> +			/* Same story with uncore PMCs */
> +			pr_err_ratelimited("Uncore PMC FIQ fired. Masking.\n");
> +			sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> +					FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
> +		}
>  	}
>  }
>  
> @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id,
>  				break;
>  			case AIC_TMR_HV_PHYS:
>  			case AIC_TMR_HV_VIRT:
> -				return -ENOENT;
> +				if (aic_irqc->info.el2_regs)
> +					return -ENOENT;

See my comment above about the use of these interrupt numbers.

>  			default:
>  				break;
>  			}
> @@ -944,7 +964,8 @@ static int aic_init_cpu(unsigned int cpu)
>  	/* Mask all hard-wired per-CPU IRQ/FIQ sources */
>  
>  	/* Pending Fast IPI FIQs */
> -	write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
> +	if (aic_irqc->info.ipi_regs)
> +		write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>  
>  	/* Timer FIQs */
>  	sysreg_clear_set(cntp_ctl_el0, 0, ARCH_TIMER_CTRL_IT_MASK);
> @@ -965,8 +986,9 @@ static int aic_init_cpu(unsigned int cpu)
>  			   FIELD_PREP(PMCR0_IMODE, PMCR0_IMODE_OFF));
>  
>  	/* Uncore PMC FIQ */
> -	sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> -			   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
> +	if (aic_irqc->info.uncore2_regs)
> +		sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> +				   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>  
>  	/* Commit all of the above */
>  	isb();

I must be missing something though. Where is the code that actually
enables support for the SoCs mentioned in $SUBJECT?

Thanks,

	M.

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