From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Mon, 19 Sep 2016 09:11:08 +0100 Subject: [PATCH soc/next] ARM: BCM53573: Add custom init_time with arch timer workaroud In-Reply-To: <20160918193238.21339-1-zajec5@gmail.com> References: <20160918193238.21339-1-zajec5@gmail.com> Message-ID: <57DF9D9C.9020206@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Rafa?, On 18/09/16 20:32, Rafa? Mi?ecki wrote: > From: Rafa? Mi?ecki > > BCM53573 uses ARM architected timer but CFE (bootloader) is bugged and > doesn't setup hardware properly. As the architecture requirement clock > should be enabled and CNTFRQ should be set before starting Linux. > > Unfortunately it's impossible to have CFE fixed and updated on all > released devices. There are plenty of them on the market and updating > bootloader isn't a standard procedure. We also don't have CFE sources > for BCM53573 devices and no replacement bootloader. > > We can't modify arch timer to simply accept specifying clock. Quoting > Mark: "The clock-frequency property is at best a dodgy workaround, and > this is even worse.". Marc also noted that other subsystems may rely on > CNTFRQ as well. > > The only sane workaround seems to be fixing this CFE bug in BCM53573 > arch code. It doesn't require modifying other drivers and should allow > all subsystems to work correctly. > > Signed-off-by: Rafa? Mi?ecki > Cc: Mark Rutland > Cc: Marc Zyngier > --- > MAINTAINERS | 1 + > arch/arm/mach-bcm/Makefile | 3 +++ > arch/arm/mach-bcm/bcm_53573.c | 62 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 66 insertions(+) > create mode 100644 arch/arm/mach-bcm/bcm_53573.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a91bca7..0bb0c4b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2568,6 +2568,7 @@ BROADCOM BCM53573 ARM ARCHITECTURE > M: Rafa? Mi?ecki > L: linux-arm-kernel at lists.infradead.org > S: Maintained > +F: arch/arm/mach-bcm/bcm_53573.c > F: arch/arm/boot/dts/bcm53573* > F: arch/arm/boot/dts/bcm47189* > > diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile > index 980f585..0d64778 100644 > --- a/arch/arm/mach-bcm/Makefile > +++ b/arch/arm/mach-bcm/Makefile > @@ -50,6 +50,9 @@ ifeq ($(CONFIG_ARCH_BCM_5301X),y) > obj-$(CONFIG_SMP) += platsmp.o > endif > > +# BCM53573 > +obj-$(CONFIG_ARCH_BCM_53573) += bcm_53573.o > + > # BCM63XXx > ifeq ($(CONFIG_ARCH_BCM_63XX),y) > obj-y += bcm63xx.o > diff --git a/arch/arm/mach-bcm/bcm_53573.c b/arch/arm/mach-bcm/bcm_53573.c > new file mode 100644 > index 0000000..540a247 > --- /dev/null > +++ b/arch/arm/mach-bcm/bcm_53573.c > @@ -0,0 +1,62 @@ > +/* > + * Copyright (C) 2016 Rafa? Mi?ecki > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include > +#include > +#include > +#include > + > +static inline void arch_timer_set_cntfrq(u32 cntfrq) > +{ > + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (cntfrq)); > +} This is wrong for a number of reasons: This register only writeable from the secure side. Boot this kernel in non-secure mode, and the kernel will explode. Also, you're only setting it from the boot CPU, while it should be set on all of them. So in effect, this is not any better than advertising the timer frequency in the device tree. > + > +/* > + * CFE bootloader doesn't meet arch requirements. It doesn't enable ILP clock > + * which is required for arch timer and doesn't set CNTFRQ. > + * Fix is up here. > + */ > +static void __init bcm_53573_setup_arch_timer(void) > +{ > + struct of_phandle_args out_args = { }; > + struct clk *clk; > + > + out_args.np = of_find_compatible_node(NULL, NULL, "brcm,bcm53573-ilp"); > + if (!out_args.np) { > + pr_warn("Failed to find ILP node\n"); > + return; > + } > + > + clk = of_clk_get_from_provider(&out_args); > + if (!IS_ERR(clk)) { > + if (!clk_prepare_enable(clk)) > + arch_timer_set_cntfrq(clk_get_rate(clk)); > + } > + > + of_node_put(out_args.np); > +} > + > +/* A copy of ARM's time_init with workaround inserted */ > +static void __init bcm_53573_init_time(void) > +{ > +#ifdef CONFIG_COMMON_CLK > + of_clk_init(NULL); > +#endif > + bcm_53573_setup_arch_timer(); > + clocksource_probe(); > +} > + > +static const char *const bcm_53573_dt_compat[] __initconst = { > + "brcm,bcm53573", > + NULL, > +}; > + > +DT_MACHINE_START(BCM5301X, "BCM53573") > + .init_time = bcm_53573_init_time, > + .dt_compat = bcm_53573_dt_compat, > +MACHINE_END > My advise would be to move this to a shim outside of the kernel, which could configure the clocks and set CNTFRQ on all CPUs. As it stands, this patch doesn't really improve the situation. Thanks, M. -- Jazz is not dead. It just smells funny... From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935782AbcISILW (ORCPT ); Mon, 19 Sep 2016 04:11:22 -0400 Received: from foss.arm.com ([217.140.101.70]:54996 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757395AbcISILS (ORCPT ); Mon, 19 Sep 2016 04:11:18 -0400 Subject: Re: [PATCH soc/next] ARM: BCM53573: Add custom init_time with arch timer workaroud To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Florian Fainelli References: <20160918193238.21339-1-zajec5@gmail.com> Cc: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Mark Rutland , Ray Jui , Scott Branden , bcm-kernel-feedback-list@broadcom.com, Russell King , "David S. Miller" , Geert Uytterhoeven , Andrew Morton , Greg Kroah-Hartman , Kalle Valo , Mauro Carvalho Chehab , Guenter Roeck , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org From: Marc Zyngier X-Enigmail-Draft-Status: N1110 Organization: ARM Ltd Message-ID: <57DF9D9C.9020206@arm.com> Date: Mon, 19 Sep 2016 09:11:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.7.0 MIME-Version: 1.0 In-Reply-To: <20160918193238.21339-1-zajec5@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Rafał, On 18/09/16 20:32, Rafał Miłecki wrote: > From: Rafał Miłecki > > BCM53573 uses ARM architected timer but CFE (bootloader) is bugged and > doesn't setup hardware properly. As the architecture requirement clock > should be enabled and CNTFRQ should be set before starting Linux. > > Unfortunately it's impossible to have CFE fixed and updated on all > released devices. There are plenty of them on the market and updating > bootloader isn't a standard procedure. We also don't have CFE sources > for BCM53573 devices and no replacement bootloader. > > We can't modify arch timer to simply accept specifying clock. Quoting > Mark: "The clock-frequency property is at best a dodgy workaround, and > this is even worse.". Marc also noted that other subsystems may rely on > CNTFRQ as well. > > The only sane workaround seems to be fixing this CFE bug in BCM53573 > arch code. It doesn't require modifying other drivers and should allow > all subsystems to work correctly. > > Signed-off-by: Rafał Miłecki > Cc: Mark Rutland > Cc: Marc Zyngier > --- > MAINTAINERS | 1 + > arch/arm/mach-bcm/Makefile | 3 +++ > arch/arm/mach-bcm/bcm_53573.c | 62 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 66 insertions(+) > create mode 100644 arch/arm/mach-bcm/bcm_53573.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a91bca7..0bb0c4b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2568,6 +2568,7 @@ BROADCOM BCM53573 ARM ARCHITECTURE > M: Rafał Miłecki > L: linux-arm-kernel@lists.infradead.org > S: Maintained > +F: arch/arm/mach-bcm/bcm_53573.c > F: arch/arm/boot/dts/bcm53573* > F: arch/arm/boot/dts/bcm47189* > > diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile > index 980f585..0d64778 100644 > --- a/arch/arm/mach-bcm/Makefile > +++ b/arch/arm/mach-bcm/Makefile > @@ -50,6 +50,9 @@ ifeq ($(CONFIG_ARCH_BCM_5301X),y) > obj-$(CONFIG_SMP) += platsmp.o > endif > > +# BCM53573 > +obj-$(CONFIG_ARCH_BCM_53573) += bcm_53573.o > + > # BCM63XXx > ifeq ($(CONFIG_ARCH_BCM_63XX),y) > obj-y += bcm63xx.o > diff --git a/arch/arm/mach-bcm/bcm_53573.c b/arch/arm/mach-bcm/bcm_53573.c > new file mode 100644 > index 0000000..540a247 > --- /dev/null > +++ b/arch/arm/mach-bcm/bcm_53573.c > @@ -0,0 +1,62 @@ > +/* > + * Copyright (C) 2016 Rafał Miłecki > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include > +#include > +#include > +#include > + > +static inline void arch_timer_set_cntfrq(u32 cntfrq) > +{ > + asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (cntfrq)); > +} This is wrong for a number of reasons: This register only writeable from the secure side. Boot this kernel in non-secure mode, and the kernel will explode. Also, you're only setting it from the boot CPU, while it should be set on all of them. So in effect, this is not any better than advertising the timer frequency in the device tree. > + > +/* > + * CFE bootloader doesn't meet arch requirements. It doesn't enable ILP clock > + * which is required for arch timer and doesn't set CNTFRQ. > + * Fix is up here. > + */ > +static void __init bcm_53573_setup_arch_timer(void) > +{ > + struct of_phandle_args out_args = { }; > + struct clk *clk; > + > + out_args.np = of_find_compatible_node(NULL, NULL, "brcm,bcm53573-ilp"); > + if (!out_args.np) { > + pr_warn("Failed to find ILP node\n"); > + return; > + } > + > + clk = of_clk_get_from_provider(&out_args); > + if (!IS_ERR(clk)) { > + if (!clk_prepare_enable(clk)) > + arch_timer_set_cntfrq(clk_get_rate(clk)); > + } > + > + of_node_put(out_args.np); > +} > + > +/* A copy of ARM's time_init with workaround inserted */ > +static void __init bcm_53573_init_time(void) > +{ > +#ifdef CONFIG_COMMON_CLK > + of_clk_init(NULL); > +#endif > + bcm_53573_setup_arch_timer(); > + clocksource_probe(); > +} > + > +static const char *const bcm_53573_dt_compat[] __initconst = { > + "brcm,bcm53573", > + NULL, > +}; > + > +DT_MACHINE_START(BCM5301X, "BCM53573") > + .init_time = bcm_53573_init_time, > + .dt_compat = bcm_53573_dt_compat, > +MACHINE_END > My advise would be to move this to a shim outside of the kernel, which could configure the clocks and set CNTFRQ on all CPUs. As it stands, this patch doesn't really improve the situation. Thanks, M. -- Jazz is not dead. It just smells funny...