On Tue, 04 Oct 2022 12:27:24 +0100,
Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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.
