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 9BB1AC433F5 for ; Tue, 4 Oct 2022 19:15:39 +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=S6NOBFzazNypI/xeknuy0FIky4dahBWSlfMSr03uhKI=; b=yhB0ElPvCpV+jV qJ9ox5GM9VjTETo12RmwslKBz7DutZfh1hwWT+eCOFd9YwOpSYTc6Aa0miV/Z4bioYJs1pzpcqsNq Cag+925V8tte0Gxx1TKKdPrhl5ROeR4IwbMOrOjD1U4mcYYAqTItGxzDv3iNUKPnhvcQUJQiBwrKA Qf9/CPfU4oqCZuMu1VXh8SflYJSNnY023GHXyqfXegXz1v36wfQdllKto0ZIXkgKhohR9IAIiyF1L hVj8F9soPZP6K6SCloXNdff4vsluzS/JSpV4jUBIhwNk4PEHoUvWYg3A5nW5RFjr4H72mP3dlLxzb tRehOdEnOeiQhPrUfzIA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ofnNB-00AlW0-78; Tue, 04 Oct 2022 19:14:33 +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 1ofnN8-00AlV8-0r for linux-arm-kernel@lists.infradead.org; Tue, 04 Oct 2022 19:14:31 +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 70E1F61505; Tue, 4 Oct 2022 19:14:28 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6B74FC433C1; Tue, 4 Oct 2022 19:14:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1664910867; bh=AI/bhdO9mDt2AJiBTy/DR5WJsm0u8LzDoxfCIICGnNw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=niNm40Ige5jvXdzikYXbvFRhCnsVrejQ9vC/UzB6lHinYg3V18+BeU8W0NpIgvVt0 J4MskZtq62uP1UMptL1pqj/LNZjPvxt5j4RPVu9ePciX1LcFsRMZE0L/vgqWu9dv/t 11Wj9pb7kVH8ti6f4wRcjHQEIXFP5b9RlJuS/W4tSgvhw2ghGOdkji7pj8TcQ6PUNA HVVLvYV958jPMpFwKbjs3Nsa1p2RMv0/vRd5dRNDkcWfRPLv6VE0HzZCiZ62nzUGkk Qa1nhM4ht2qyt9fO5hsPiEQ1EkOmgphAk8l/AAoGsTELQUtv/wpTLjssvroA5AOgxf xOq8d/xjamKYg== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.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 1ofnN3-00EagJ-5M; Tue, 04 Oct 2022 20:14:25 +0100 Date: Tue, 04 Oct 2022 20:14:24 +0100 Message-ID: <86a66b8kq7.wl-maz@kernel.org> From: Marc Zyngier To: Konrad Dybcio Cc: asahi@lists.linux.dev, linux-arm-kernel@lists.infradead.org, towinchenmi@gmail.com, Hector Martin , Sven Peter , Alyssa Rosenzweig , Thomas Gleixner , Rob Herring , Krzysztof Kozlowski , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH v3 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs In-Reply-To: <20221004112724.31621-2-konrad.dybcio@somainline.org> References: <20221004112724.31621-1-konrad.dybcio@somainline.org> <20221004112724.31621-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 (aarch64-unknown-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, asahi@lists.linux.dev, linux-arm-kernel@lists.infradead.org, towinchenmi@gmail.com, marcan@marcan.st, sven@svenpeter.dev, alyssa@rosenzweig.io, tglx@linutronix.de, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, linux-kernel@vger.kernel.org, devicetree@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-20221004_121430_173447_308AECCC X-CRM114-Status: GOOD ( 48.36 ) 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, 04 Oct 2022 12:27:24 +0100, 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. 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 > --- > 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel