Message-ID: <87bkttlcfp.wl-maz@kernel.org>
Date:   Wed, 13 Jul 2022 08:05:46 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Konrad Dybcio <konrad.dybcio@...ainline.org>
Cc:     ~postmarketos/upstreaming@...ts.sr.ht, martin.botka@...ainline.org,
        angelogioacchino.delregno@...ainline.org,
        marijn.suijten@...ainline.org, jamipkettunen@...ainline.org,
        Hector Martin <marcan@...can.st>,
        Sven Peter <sven@...npeter.dev>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs

On Tue, 12 Jul 2022 20:23:31 +0100,
Konrad Dybcio <konrad.dybcio@...ainline.org> wrote:
> 
> 
> 
> On 12.07.2022 21:12, Marc Zyngier wrote:
> > 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@...ainline.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@...ainline.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.
> The latter would be true if a bootrom exploit or any equivalent means
> of booting Linux would be found for A12 (M1 is family with A14 for context).
> 
> We can think of 4 feature levels, I think:
> 
> A7-A10: 'nothing fancy'
> A11: fast_ipi (broken currently, need to investigate)
> A12: A11 + UNCORE2 regs
> M1+: A12 + EL2
> 
> We *could* squash the A12-A14 case into M1, but then if a means of booting
> Linux appears, this would have to be untangled again..

We don't add code for systems that could only hypothetically run
Linux. If and when this becomes possible, we'll add support for
them. In the meantime, I suggest you focus on supporting what actually
works.

>
> > 
> >>  };
> >>  
> >>  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.
> This func accesses impl-defined regs that are not present on earlier SoCs.

You're missing my point. Why are you encoding your timer interrupts
with a hwirq that requires you to skip existing code? If you used the
interrupt number that represent a bare-metal interrupt, you'd be just
fine. Specially considering that the interrupt numbers in the DT are
nothing but made-up numbers, as there is no interrupt controller to
speak of.

> >> @@ -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.
> `if (!is_kernel_in_hyp_mode()) {` always evaluates to true, since there's
> no EL2. Hence, accessing AIC_TMR_HV_{VIRT,PHYS} makes this return ENOENT,
> which means timer can't probe and that's no bueno.

Again, you have the wrong end of the stick, and this is about changing
your DT rather than the driver.

> 
> 
> > 
> >>  			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?
> In this peculiar case, enabling support means stripping away the so-called
> 'features', otherwise the interrupt controller won't budge.

What I would like to see is a different compatibility string that
makes the support for a given IP explicit instead of making everything
implicit.

	M.

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

Powered by blists - more mailing lists