Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752049AbaK1PDo (ORCPT ); Fri, 28 Nov 2014 10:03:44 -0500 Received: from foss-mx-na.foss.arm.com ([217.140.108.86]:41491 "EHLO foss-mx-na.foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751941AbaK1PDk (ORCPT ); Fri, 28 Nov 2014 10:03:40 -0500 Date: Fri, 28 Nov 2014 15:03:26 +0000 From: Catalin Marinas To: Will Deacon Cc: Mark Rutland , "andrew@lunn.ch" , "heiko@sntech.de" , "gnomes@lxorguk.ukuu.org.uk" , Chunyan Zhang , "jslaby@suse.cz" , "jason@lakedaemon.net" , "lanqing.liu@spreadtrum.com" , Pawel Moll , "corbet@lwn.net" , "zhang.lyra@gmail.com" , "zhizhou.zhang@spreadtrum.com" , "geng.ren@spreadtrum.com" , "m-karicheri2@ti.com" , "shawn.guo@freescale.com" , "linux-serial@vger.kernel.org" , "grant.likely@linaro.org" , "orsonzhai@gmail.com" , "florian.vaussard@epfl.ch" , "devicetree@vger.kernel.org" , "artagnon@gmail.com" , "arnd@arndb.de" , "ijc+devicetree@hellion.org.uk" , Marc Zyngier , "galak@codeaurora.org" , "hytszk@gmail.com" , "rrichter@cavium.com" , "broonie@kernel.org" , "wei.qiao@spreadtrum.com" , "sprdlinux@freelists.org" , "linux-arm-kernel@lists.infradead.org" , "linux-api@vger.kernel.org" , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "robh+dt@kernel.org" , "gregkh@linuxfoundation.org" , "broonie@linaro.org" Subject: Re: [PATCH v3 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile Message-ID: <20141128150325.GC24370@e104818-lin.cambridge.arm.com> References: <1416917818-10506-1-git-send-email-chunyan.zhang@spreadtrum.com> <1416917818-10506-4-git-send-email-chunyan.zhang@spreadtrum.com> <20141127115042.GE857@leverpostej> <20141127121214.GF11511@e104818-lin.cambridge.arm.com> <20141127134309.GJ857@leverpostej> <20141128142913.GB5393@localhost> <20141128143532.GI25883@leverpostej> <20141128144412.GG7144@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141128144412.GG7144@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Nov 28, 2014 at 02:44:12PM +0000, Will Deacon wrote: > On Fri, Nov 28, 2014 at 02:35:32PM +0000, Mark Rutland wrote: > > On Fri, Nov 28, 2014 at 02:29:13PM +0000, Catalin Marinas wrote: > > > On Thu, Nov 27, 2014 at 01:43:09PM +0000, Mark Rutland wrote: > > > > On Thu, Nov 27, 2014 at 12:12:15PM +0000, Catalin Marinas wrote: > > > > > On Thu, Nov 27, 2014 at 11:50:43AM +0000, Mark Rutland wrote: > > > > > > On Tue, Nov 25, 2014 at 12:16:56PM +0000, Chunyan Zhang wrote: > > > > > > > + > > > > > > > + timer { > > > > > > > + compatible = "arm,armv8-timer"; > > > > > > > + interrupts = <1 13 0xff01>, > > > > > > > + <1 14 0xff01>, > > > > > > > + <1 11 0xff01>, > > > > > > > + <1 10 0xff01>; > > > > > > > + clock-frequency = <26000000>; > > > > > > > > > > > > Please remove the clock-frequency property. Your FW should initialise > > > > > > CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this). > > > > > > > > > > Since this comes up regularly, I think we need a dev_warn() in the arch > > > > > timer driver when CONFIG_ARM64. > > > > > > > > I'll ack such a patch ;) > > > > > > How rude would this be? > > > > > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > > > index 2133f9d59d06..aaaf3433ccb9 100644 > > > --- a/drivers/clocksource/arm_arch_timer.c > > > +++ b/drivers/clocksource/arm_arch_timer.c > > > @@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) > > > return; > > > > > > /* Try to determine the frequency from the device tree or CNTFRQ */ > > > - if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) { > > > + if (IS_ENABLED(CONFIG_ARM64) || > > > + of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) { > > > if (cntbase) > > > arch_timer_rate = readl_relaxed(cntbase + CNTFRQ); > > > else > > > > > > > Probably too rude, given it doesn't WARN() the user. > > We override broken hardware ID registers all the time in device-tree without > dumping stack. Why is this any different? I'm not for dumping the stack, it's not relevant (just more noise). > > We should be extremely loud if we see the clock-frequency property on an > > arm64 system. Whether or not we should ignore the property is another > > matter. > > I don't really see the point in ignoring it. We will see broken hardware > [1] and this is just preventing ourselves from working around it. I'd much > rather have arch-timers with a "clock-frequence" property than have some > other timer instead because the kernel driver is being stubborn. I agree that sooner or later we'll need a workaround (we already did for Juno). My point is that many consider such overriding behaviour to be the default - i.e. don't bother writing any sane value in CNTFRQ in firmware at boot because Linux can cope without. It gets worse when companies develop their firmware long before starting to upstream kernel patches, so too late to fix it. > [1] A previous version of the Juno firmware, for example. What about CONFIG_BROKEN_FIRMWARE, default off? In the meantime I think we can be more tolerant: diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 2133f9d59d06..87f67a93fcc7 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -376,6 +376,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) arch_timer_rate = readl_relaxed(cntbase + CNTFRQ); else arch_timer_rate = arch_timer_get_cntfrq(); + } else if (IS_ENABLED(CONFIG_ARM64)) { + pr_warn("Architected timer frequency overridden by DT (broken firmware?)\n"); } /* Check the timer frequency. */ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/