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 E7AEDC433EF for ; Wed, 13 Jul 2022 07:07:16 +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=vfz1ChyK2zUn13zQGXu0664RcLXlW4LevCjYfGTrGX8=; b=FxWUZsmZxiPNNW Gnb9606E4jbUrNHnPjrtkMrRmOTNb7zvUqhauq5fHmlzSJ3+CaHD7eZVw5ESD4uOdhl3pV9aqSaAU nE9R6kfZTQWrL6MLrEmRC7g8cjRSY0/SUwLpX79CByG6tr0v6X6Xws/6jmPoSr0hL8LmgLn5ZI3Zn vIb/CG9Y6p1yaHPe3GZqm2oSGdcaWQsI1k7cuWq+kRhB8kjLhQCexd3WnlgMLc2FEwbbmyypLtfJo c7JxaMTt2BIa2T235O2hsNMTYdUv6wY2D7P+DCOFgpzdT+t+hhgGKyysZZPXm2JJLA4TflmbJogTn yBRZe9NOPU7FKQitStmQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oBWRk-0012rE-CK; Wed, 13 Jul 2022 07:06:08 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oBWRg-0012q6-02 for linux-arm-kernel@lists.infradead.org; Wed, 13 Jul 2022 07:06: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 ams.source.kernel.org (Postfix) with ESMTPS id 711FFB81D41; Wed, 13 Jul 2022 07:06:02 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0157AC34114; Wed, 13 Jul 2022 07:06:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1657695961; bh=il/VUoRRK8tAkt4DY/I12EVvlEk0AWNjjZ+AkvPGREY=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=C3sCQk3F5wKR1xGe06jgExzccla0ZvHUVpVfRXeNhEmUto/4q3vTS2KuLglP4UK89 QMiyoL1oeHfh+6phDdZmGAGJ+5rnfIo7rW8BBvAcIkRLOOGMxG9rO9GhGBp/XYGXTU CzzeYO++MNVVJhIdTm47YKmK5UTWEA5jgnBSfgt0yGWRLDc03TGkGPauR+DwqjmTq4 zm7e2cdqE779ww3ckraYbmcOfhiXCR8PwMffqIlGCwJnjyCS/iHmC2cEaYzUGcHYhX PezGnqFnubIIhkjZ9LR/iFQ0TMqW8U16Iop5XoMN2DFaW8n4jULNj7VdElxRrNqPar BzvrWKhYHM8PQ== Received: from ip-185-104-136-29.ptr.icomera.net ([185.104.136.29] helo=wait-a-minute.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 1oBWRa-0078mQ-An; Wed, 13 Jul 2022 08:05:58 +0100 Date: Wed, 13 Jul 2022 08:05:46 +0100 Message-ID: <87bkttlcfp.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: References: <20220712160919.740878-1-konrad.dybcio@somainline.org> <20220712160919.740878-2-konrad.dybcio@somainline.org> <87a69e16x1.wl-maz@kernel.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.104.136.29 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-20220713_000604_353498_C1CB7FE3 X-CRM114-Status: GOOD ( 53.77 ) 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 On Tue, 12 Jul 2022 20:23:31 +0100, Konrad Dybcio 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 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. > 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel