diff mbox series

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

Message ID 20221004112724.31621-2-konrad.dybcio@somainline.org (mailing list archive)
State New, archived
Headers show
Series [v3,1/2] dt-bindings: apple,aic: Document A7-A11 compatibles | expand

Commit Message

Konrad Dybcio Oct. 4, 2022, 11:27 a.m. UTC 
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

 drivers/irqchip/irq-apple-aic.c | 60 ++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 15 deletions(-)

Comments

Sven Peter Oct. 4, 2022, 3:56 p.m. UTC | #1 
Hi,


On Tue, Oct 4, 2022, at 13:27, Konrad Dybcio 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. 

AFAIK that's only an artifact in this driver: It was added to prevent an FIQ
storm in case there were pending fast ipis (i.e. the bootloader was broken ;))
when this driver didn't support fast ipis yet.

> 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.

Just to make sure I understand this correctly - we have the following three situations:

- base: no fastipi, no uncore, will work on A11 and M1 though
- A11: fastipi and uncore but fastipi is broken (possibly due to HW errata or some bug in this driver that only happens on A11)
- M1 (or maybe even A12 already, doesn't matter though): fastipi and uncore support

If we figured out _why_ fastipi is broken on A11 we would only need a single
feature flag to enable both uncore and fastipi but for now we need two to
disable fastipi for A11.

I'm also curious: What are the symptoms when you enable fastipi on A11?


Sven
Marc Zyngier Oct. 4, 2022, 7:14 p.m. UTC | #2 
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.
Nick Chan Oct. 5, 2022, 4:43 p.m. UTC | #3 
On 4/10/2022 23:56, Sven Peter wrote:
> Hi,
> 
> 
> On Tue, Oct 4, 2022, at 13:27, Konrad Dybcio 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.
> 
> AFAIK that's only an artifact in this driver: It was added to prevent an FIQ
> storm in case there were pending fast ipis (i.e. the bootloader was broken ;))
> when this driver didn't support fast ipis yet.
> 
>> 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.
> 
> Just to make sure I understand this correctly - we have the following three situations:
> 
> - base: no fastipi, no uncore, will work on A11 and M1 though
> - A11: fastipi and uncore but fastipi is broken (possibly due to HW errata or some bug in this driver that only happens on A11)
> - M1 (or maybe even A12 already, doesn't matter though): fastipi and uncore support
> 
> If we figured out _why_ fastipi is broken on A11 we would only need a single
> feature flag to enable both uncore and fastipi but for now we need two to
> disable fastipi for A11

The previous issues with fast IPI does not seem to be reproducible with thisversion of patches anymore, at least when modifying the device tree to useapple,t8103-aic the device still boots. Tested on iPhone X.
> 
> I'm also curious: What are the symptoms when you enable fastipi on A11?
> 
> 
> Sven


Nick Chan
diff mbox series

Patch

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 = {
+	.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,
 };
 
 static const struct of_device_id aic_info_match[] = {
+	{
+		.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) {
+			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 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);
+	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),