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 X-Spam-Level: X-Spam-Status: No, score=-13.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EADF5C4338F for ; Tue, 24 Aug 2021 16:20:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D1ED8610A3 for ; Tue, 24 Aug 2021 16:20:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230221AbhHXQVf (ORCPT ); Tue, 24 Aug 2021 12:21:35 -0400 Received: from foss.arm.com ([217.140.110.172]:37876 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229712AbhHXQVb (ORCPT ); Tue, 24 Aug 2021 12:21:31 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0151731B; Tue, 24 Aug 2021 09:20:47 -0700 (PDT) Received: from C02TD0UTHF1T.local (unknown [10.57.90.204]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 04D2E3F766; Tue, 24 Aug 2021 09:20:44 -0700 (PDT) Date: Tue, 24 Aug 2021 17:20:42 +0100 From: Mark Rutland To: Marc Zyngier Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Daniel Lezcano , Thomas Gleixner , Peter Shier , Raghavendra Rao Ananta , Ricardo Koller , Oliver Upton , Will Deacon , Catalin Marinas , Linus Walleij , kernel-team@android.com Subject: Re: [PATCH 01/13] clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors Message-ID: <20210824162042.GF96738@C02TD0UTHF1T.local> References: <20210809152651.2297337-1-maz@kernel.org> <20210809152651.2297337-2-maz@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210809152651.2297337-2-maz@kernel.org> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 09, 2021 at 04:26:39PM +0100, Marc Zyngier wrote: > The arch timer driver never reads the various TVAL registers, only > writes to them. It is thus pointless to provide accessors > for them and to implement errata workarounds. > > Drop these read-side accessors, and add a couple of BUG() statements > for the time being. These statements will be removed further down > the line. > > Signed-off-by: Marc Zyngier > --- > arch/arm/include/asm/arch_timer.h | 6 ++-- > arch/arm64/include/asm/arch_timer.h | 17 ++--------- > drivers/clocksource/arm_arch_timer.c | 44 ++-------------------------- > 3 files changed, 6 insertions(+), 61 deletions(-) > > diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h > index 99175812d903..0c09afaa590d 100644 > --- a/arch/arm/include/asm/arch_timer.h > +++ b/arch/arm/include/asm/arch_timer.h > @@ -60,8 +60,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) > asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val)); > break; > case ARCH_TIMER_REG_TVAL: > - asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val)); > - break; > + BUG(); It would be nice if we had: default: BUG(); // or maybe BUILD_BUG() ... in all these switches to avoid surprises in future. If we did that as a prep patch, this patch would be a pure deletion. Either way, this looks good: Reviewed-by: Mark Rutland Builds cleanly, and boots fine on a fast model (both arm/arm64): Tested-by: Mark Rutland Thanks, Mark. > } > } else if (access == ARCH_TIMER_VIRT_ACCESS) { > switch (reg) { > @@ -69,8 +68,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) > asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val)); > break; > case ARCH_TIMER_REG_TVAL: > - asm volatile("mrc p15, 0, %0, c14, c3, 0" : "=r" (val)); > - break; > + BUG(); > } > } > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index 88d20f04c64a..8e3b2ac60c30 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -52,8 +52,6 @@ struct arch_timer_erratum_workaround { > enum arch_timer_erratum_match_type match_type; > const void *id; > const char *desc; > - u32 (*read_cntp_tval_el0)(void); > - u32 (*read_cntv_tval_el0)(void); > u64 (*read_cntpct_el0)(void); > u64 (*read_cntvct_el0)(void); > int (*set_next_event_phys)(unsigned long, struct clock_event_device *); > @@ -64,17 +62,6 @@ struct arch_timer_erratum_workaround { > DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *, > timer_unstable_counter_workaround); > > -/* inline sysreg accessors that make erratum_handler() work */ > -static inline notrace u32 arch_timer_read_cntp_tval_el0(void) > -{ > - return read_sysreg(cntp_tval_el0); > -} > - > -static inline notrace u32 arch_timer_read_cntv_tval_el0(void) > -{ > - return read_sysreg(cntv_tval_el0); > -} > - > static inline notrace u64 arch_timer_read_cntpct_el0(void) > { > return read_sysreg(cntpct_el0); > @@ -135,14 +122,14 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg) > case ARCH_TIMER_REG_CTRL: > return read_sysreg(cntp_ctl_el0); > case ARCH_TIMER_REG_TVAL: > - return arch_timer_reg_read_stable(cntp_tval_el0); > + break; > } > } else if (access == ARCH_TIMER_VIRT_ACCESS) { > switch (reg) { > case ARCH_TIMER_REG_CTRL: > return read_sysreg(cntv_ctl_el0); > case ARCH_TIMER_REG_TVAL: > - return arch_timer_reg_read_stable(cntv_tval_el0); > + break; > } > } > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index be6d741d404c..9db5c16e31e7 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -141,8 +141,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg, > val = readl_relaxed(timer->base + CNTP_CTL); > break; > case ARCH_TIMER_REG_TVAL: > - val = readl_relaxed(timer->base + CNTP_TVAL); > - break; > + BUG(); > } > } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) { > struct arch_timer *timer = to_arch_timer(clk); > @@ -151,8 +150,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg, > val = readl_relaxed(timer->base + CNTV_CTL); > break; > case ARCH_TIMER_REG_TVAL: > - val = readl_relaxed(timer->base + CNTV_TVAL); > - break; > + BUG(); > } > } else { > val = arch_timer_reg_read_cp15(access, reg); > @@ -239,16 +237,6 @@ struct ate_acpi_oem_info { > _new; \ > }) > > -static u32 notrace fsl_a008585_read_cntp_tval_el0(void) > -{ > - return __fsl_a008585_read_reg(cntp_tval_el0); > -} > - > -static u32 notrace fsl_a008585_read_cntv_tval_el0(void) > -{ > - return __fsl_a008585_read_reg(cntv_tval_el0); > -} > - > static u64 notrace fsl_a008585_read_cntpct_el0(void) > { > return __fsl_a008585_read_reg(cntpct_el0); > @@ -285,16 +273,6 @@ static u64 notrace fsl_a008585_read_cntvct_el0(void) > _new; \ > }) > > -static u32 notrace hisi_161010101_read_cntp_tval_el0(void) > -{ > - return __hisi_161010101_read_reg(cntp_tval_el0); > -} > - > -static u32 notrace hisi_161010101_read_cntv_tval_el0(void) > -{ > - return __hisi_161010101_read_reg(cntv_tval_el0); > -} > - > static u64 notrace hisi_161010101_read_cntpct_el0(void) > { > return __hisi_161010101_read_reg(cntpct_el0); > @@ -379,16 +357,6 @@ static u64 notrace sun50i_a64_read_cntvct_el0(void) > { > return __sun50i_a64_read_reg(cntvct_el0); > } > - > -static u32 notrace sun50i_a64_read_cntp_tval_el0(void) > -{ > - return read_sysreg(cntp_cval_el0) - sun50i_a64_read_cntpct_el0(); > -} > - > -static u32 notrace sun50i_a64_read_cntv_tval_el0(void) > -{ > - return read_sysreg(cntv_cval_el0) - sun50i_a64_read_cntvct_el0(); > -} > #endif > > #ifdef CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND > @@ -438,8 +406,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = { > .match_type = ate_match_dt, > .id = "fsl,erratum-a008585", > .desc = "Freescale erratum a005858", > - .read_cntp_tval_el0 = fsl_a008585_read_cntp_tval_el0, > - .read_cntv_tval_el0 = fsl_a008585_read_cntv_tval_el0, > .read_cntpct_el0 = fsl_a008585_read_cntpct_el0, > .read_cntvct_el0 = fsl_a008585_read_cntvct_el0, > .set_next_event_phys = erratum_set_next_event_tval_phys, > @@ -451,8 +417,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = { > .match_type = ate_match_dt, > .id = "hisilicon,erratum-161010101", > .desc = "HiSilicon erratum 161010101", > - .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0, > - .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0, > .read_cntpct_el0 = hisi_161010101_read_cntpct_el0, > .read_cntvct_el0 = hisi_161010101_read_cntvct_el0, > .set_next_event_phys = erratum_set_next_event_tval_phys, > @@ -462,8 +426,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = { > .match_type = ate_match_acpi_oem_info, > .id = hisi_161010101_oem_info, > .desc = "HiSilicon erratum 161010101", > - .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0, > - .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0, > .read_cntpct_el0 = hisi_161010101_read_cntpct_el0, > .read_cntvct_el0 = hisi_161010101_read_cntvct_el0, > .set_next_event_phys = erratum_set_next_event_tval_phys, > @@ -484,8 +446,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = { > .match_type = ate_match_dt, > .id = "allwinner,erratum-unknown1", > .desc = "Allwinner erratum UNKNOWN1", > - .read_cntp_tval_el0 = sun50i_a64_read_cntp_tval_el0, > - .read_cntv_tval_el0 = sun50i_a64_read_cntv_tval_el0, > .read_cntpct_el0 = sun50i_a64_read_cntpct_el0, > .read_cntvct_el0 = sun50i_a64_read_cntvct_el0, > .set_next_event_phys = erratum_set_next_event_tval_phys, > -- > 2.30.2 >