Re: [PATCH v3 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs

[Marc Zyngier <maz@kernel.org>]

On Tue, 04 Oct 2022 12:27:24 +0100,
Konrad Dybcio <konrad.dybcio@somainline.org> wrote:
> 
> Add support for A7-A11 SoCs by if-ing out some features only present
> on:
> 
> * A11 & newer (implementation-defined IPI & UNCORE registers)
> * A11[1] & newer (fast IPI support).
> 
> UNCORE/UNCORE2 and IPI registers conveniently both first appeared on
> A11, so introduce just one check for that.
> 
> Knowing whether the SoC supports the latter is necessary, as they are
> written to, even if fast IPI is disabled. This in turn causes a crash
> on older platforms, as the implemention-defined registers either do
> something else or are not supposed to be touched - definitely not a
> NOP though.
> 
> [1] A11 is supposed to use this feature, but it currently doesn't work
> for reasons unknown and hence remains disabled. It can easily be enabled
> on A11 only, as there is a SoC-specific compatible in the DT with a
> fallback to apple,aic. That said, it is not yet necessary, especially
> with only one core up, and it has worked a-ok so far.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> ---
> Changes since v2:
> - has_uncore_regs now also reflects whether the soc has IPI regs (A11+)
> - apple,aic is now the default, lowest-common-denominator fallback
> compatible (Sven checked it still works on M1)
> - fixed an error where "Fast IPI fired. Acking." would be unreachable..
> oops..
> - what used to be apple,aic (yes UNCORE/IPI regs no fast IPI) is now
> used for the A11 compatible

I asked for it when I reviewed the first revision of this series, and
I'm going to ask again: please add a cover letter to your patches.
It's not rocket science, and this is the place where you should have
the change log.

> 
>  drivers/irqchip/irq-apple-aic.c | 60 ++++++++++++++++++++++++---------
>  1 file changed, 45 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
> index 1c2813ad8bbe..239115c64340 100644
> --- a/drivers/irqchip/irq-apple-aic.c
> +++ b/drivers/irqchip/irq-apple-aic.c
> @@ -230,6 +230,9 @@
>  
>  static DEFINE_STATIC_KEY_TRUE(use_fast_ipi);
>  
> +/* True if UNCORE/UNCORE2 and Sn_... IPI registers are present (A11+) */
> +static DEFINE_STATIC_KEY_TRUE(has_uncore_ipi_regs);
> +
>  struct aic_info {
>  	int version;
>  
> @@ -246,6 +249,7 @@ struct aic_info {
>  
>  	/* Features */
>  	bool fast_ipi;
> +	bool uncore_ipi_regs;
>  };
>  
>  static const struct aic_info aic1_info = {
> @@ -261,18 +265,33 @@ static const struct aic_info aic1_fipi_info = {
>  	.event		= AIC_EVENT,
>  	.target_cpu	= AIC_TARGET_CPU,
>  
> +	.uncore_ipi_regs	= true,
>  	.fast_ipi	= true,
>  };
>  
> +static const struct aic_info aic1_nofipi_info = {

It is high time that these structures get marked as __initconst, as we
don't need them once the driver is up and running.

> +	.version	= 1,
> +
> +	.event		= AIC_EVENT,
> +	.target_cpu	= AIC_TARGET_CPU,
> +
> +	.uncore_ipi_regs	= true,
> +};
> +
>  static const struct aic_info aic2_info = {
>  	.version	= 2,
>  
>  	.irq_cfg	= AIC2_IRQ_CFG,
>  
> +	.uncore_ipi_regs	= true,
>  	.fast_ipi	= true,

Please initialise the fields in the same order as the declaration.

>  };
>  
>  static const struct of_device_id aic_info_match[] = {

This could also benefit from __initconst.

> +	{
> +		.compatible = "apple,t8015-aic",
> +		.data = &aic1_nofipi_info,
> +	},
>  	{
>  		.compatible = "apple,t8103-aic",
>  		.data = &aic1_fipi_info,
> @@ -524,12 +543,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 (static_branch_likely(&has_uncore_ipi_regs)) {
> +		if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {

What's wrong with:

	if (static_branch_likely(&has_uncore_ipi_regs) &&
	    (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING)) {

which limits the churn and avoids the extra indentation?

> +			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 +587,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 (static_branch_likely(&has_uncore_ipi_regs)) {
> +		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 thing.

> +			/* 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));
> +		}
>  	}
>  }
>  
> @@ -944,7 +967,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 (static_branch_likely(&use_fast_ipi))
> +		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 +989,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 (static_branch_likely(&has_uncore_ipi_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();
> @@ -1125,6 +1150,11 @@ static int __init aic_of_ic_init(struct device_node *node, struct device_node *p
>  	else
>  		static_branch_disable(&use_fast_ipi);
>  
> +	if (irqc->info.uncore_ipi_regs)
> +		static_branch_enable(&has_uncore_ipi_regs);

You initialised the static branch to true, so this does very little.

> +	else
> +		static_branch_disable(&has_uncore_ipi_regs);
> +
>  	irqc->info.die_stride = off - start_off;
>  
>  	irqc->hw_domain = irq_domain_create_tree(of_node_to_fwnode(node),

Thanks,

	M.

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