From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2C1ECC433EF for ; Tue, 12 Jul 2022 19:14:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=a3SrtbARAmNBrNna/w0DnJ9vTxwxHgWHJNrxpjO+Xg0=; b=znHzKpjEfn1DpZ rqXeyzy/zrOljC1/QDaSIk+5GWWDA/zbqM8fcwxmietVPWWLFARjwFGrUKPRYMv0wM7PxqnI6aQSE vllA7wo9TdplvXQVwwIDz+AIdl9mp1DRSMG1JolrONlBFC/FxIZ3ItOQWiBSMB7BaNyDSzUPLaz0L c8bw4bsmOg3kJr56MS7T/hg1TnNoPYh+UnwZ7IGfRzLNiCVQYSyAHXLbZiSOLby8qVgjTOrJILVvU PdBq3eBsIKgHdLDHkP1yrRE0ulGuteP5MUHdw2+rfXYvl0Cv7Q3ozztPrhHcgMqRCVal8kkYhuXzc QxWOyOjodir79sWbquDA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oBLJj-00E64s-MT; Tue, 12 Jul 2022 19:13:07 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oBLJf-00E63C-KM for linux-arm-kernel@lists.infradead.org; Tue, 12 Jul 2022 19:13:05 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 329CA6189B; Tue, 12 Jul 2022 19:13:02 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 75292C3411C; Tue, 12 Jul 2022 19:13:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1657653181; bh=ZIS/W9+EcBxTcwwCNOkmZaezeeu47J9Ag4z+xMYPzZk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=p5+XnZNmy/Fis86PPOjXFmRJSgFbCZW0te1mtLqxaHH51R8J4BM5mPwvW5/SrVNUL FNUc9M8DIWso6mAY5Ujt3M3EdbzD4Wh0ot+namTh+CR8+8idQDG5q+UrjTkyHDrNVO fbOKZn9b+7g4oh9eodt6UNWyplN8TfFimm2NuhhHtn+DzqkFvH8WKmRV14XQyJSR+z sSZ+Txdi2Tv2HgDGk/C7/Qcl/kYmxOycWnyKsBofwP4hhDH3sFYEf9MnuR7gL48vD2 5ibN9UqkTZpHC+XH/u9+W9kcQ5SPHl2hIIjsXy2wJ4i8lvYr3K1JoxAVLaFDbRfoLR trZgwxK9gyBuA== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oBLJb-0072xT-1p; Tue, 12 Jul 2022 20:12:59 +0100 Date: Tue, 12 Jul 2022 20:12:58 +0100 Message-ID: <87a69e16x1.wl-maz@kernel.org> From: Marc Zyngier To: Konrad Dybcio Cc: ~postmarketos/upstreaming@lists.sr.ht, martin.botka@somainline.org, angelogioacchino.delregno@somainline.org, marijn.suijten@somainline.org, jamipkettunen@somainline.org, Hector Martin , Sven Peter , Alyssa Rosenzweig , Thomas Gleixner , 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") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: konrad.dybcio@somainline.org, ~postmarketos/upstreaming@lists.sr.ht, martin.botka@somainline.org, angelogioacchino.delregno@somainline.org, marijn.suijten@somainline.org, jamipkettunen@somainline.org, marcan@marcan.st, sven@svenpeter.dev, alyssa@rosenzweig.io, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220712_121303_800478_F6A44B00 X-CRM114-Status: GOOD ( 42.21 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 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 > --- > 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel