From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752641AbcDCTXp (ORCPT ); Sun, 3 Apr 2016 15:23:45 -0400 Received: from mail-oi0-f51.google.com ([209.85.218.51]:34928 "EHLO mail-oi0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752313AbcDCTXn (ORCPT ); Sun, 3 Apr 2016 15:23:43 -0400 MIME-Version: 1.0 In-Reply-To: <1459589383-16914-3-git-send-email-guodong.xu@linaro.org> References: <1459589383-16914-1-git-send-email-guodong.xu@linaro.org> <1459589383-16914-3-git-send-email-guodong.xu@linaro.org> Date: Sun, 3 Apr 2016 21:23:42 +0200 Message-ID: Subject: Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 From: Linus Walleij To: Guodong Xu Cc: Xu Wei , Mark Rutland , Rob Herring , Grant Likely , Arnd Bergmann , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , XinWei Kong , Leo Yan Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Apr 2, 2016 at 11:29 AM, Guodong Xu wrote: > From: Leo Yan > > Add sp804 timer for hi6220, so it can be used as broadcast timer. > > Signed-off-by: Leo Yan > Signed-off-by: Wei Xu > --- > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > index ad1f1eb..82c4756 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > @@ -209,5 +209,14 @@ > clock-names = "uartclk", "apb_pclk"; > status = "disabled"; > }; > + > + dual_timer0: dual_timer@f8008000 { > + compatible = "arm,sp804", "arm,primecell"; > + reg = <0x0 0xf8008000 0x0 0x1000>; > + interrupts = , > + ; > + clocks = <&ao_ctrl 27>; > + clock-names = "apb_pclk"; How can this work? You only give the apb_pclk for clocking the bus to the timer. Most platforms using this driver has something like this: timer01: timer@10104000 { compatible = "arm,sp804", "arm,primecell"; reg = <0x10104000 0x1000>; interrupt-parent = <&intc_dc1176>; interrupts = <0 8 IRQ_TYPE_LEVEL_HIGH>, <0 9 IRQ_TYPE_LEVEL_HIGH>; clocks = <&timclk>, <&timclk>, <&pclk>; clock-names = "timer1", "timer2", "apb_pclk"; }; It then reads the two clocks in the beginning of clocks() to determine the frequency of each timer. By chance the code in the driver will allow just one clock and will then assume that both the bus to the timer and the timer itself is clocked from the same clock. But I highly doubt that this is the case. Please verify what clocks actually goes into this timer, it should nominally be three of them. Yours, Linus Walleij From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Subject: Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 Date: Sun, 3 Apr 2016 21:23:42 +0200 Message-ID: References: <1459589383-16914-1-git-send-email-guodong.xu@linaro.org> <1459589383-16914-3-git-send-email-guodong.xu@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <1459589383-16914-3-git-send-email-guodong.xu-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Guodong Xu Cc: Xu Wei , Mark Rutland , Rob Herring , Grant Likely , Arnd Bergmann , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , XinWei Kong , Leo Yan List-Id: devicetree@vger.kernel.org On Sat, Apr 2, 2016 at 11:29 AM, Guodong Xu wrote: > From: Leo Yan > > Add sp804 timer for hi6220, so it can be used as broadcast timer. > > Signed-off-by: Leo Yan > Signed-off-by: Wei Xu > --- > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > index ad1f1eb..82c4756 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > @@ -209,5 +209,14 @@ > clock-names = "uartclk", "apb_pclk"; > status = "disabled"; > }; > + > + dual_timer0: dual_timer@f8008000 { > + compatible = "arm,sp804", "arm,primecell"; > + reg = <0x0 0xf8008000 0x0 0x1000>; > + interrupts = , > + ; > + clocks = <&ao_ctrl 27>; > + clock-names = "apb_pclk"; How can this work? You only give the apb_pclk for clocking the bus to the timer. Most platforms using this driver has something like this: timer01: timer@10104000 { compatible = "arm,sp804", "arm,primecell"; reg = <0x10104000 0x1000>; interrupt-parent = <&intc_dc1176>; interrupts = <0 8 IRQ_TYPE_LEVEL_HIGH>, <0 9 IRQ_TYPE_LEVEL_HIGH>; clocks = <&timclk>, <&timclk>, <&pclk>; clock-names = "timer1", "timer2", "apb_pclk"; }; It then reads the two clocks in the beginning of clocks() to determine the frequency of each timer. By chance the code in the driver will allow just one clock and will then assume that both the bus to the timer and the timer itself is clocked from the same clock. But I highly doubt that this is the case. Please verify what clocks actually goes into this timer, it should nominally be three of them. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: linus.walleij@linaro.org (Linus Walleij) Date: Sun, 3 Apr 2016 21:23:42 +0200 Subject: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 In-Reply-To: <1459589383-16914-3-git-send-email-guodong.xu@linaro.org> References: <1459589383-16914-1-git-send-email-guodong.xu@linaro.org> <1459589383-16914-3-git-send-email-guodong.xu@linaro.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sat, Apr 2, 2016 at 11:29 AM, Guodong Xu wrote: > From: Leo Yan > > Add sp804 timer for hi6220, so it can be used as broadcast timer. > > Signed-off-by: Leo Yan > Signed-off-by: Wei Xu > --- > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > index ad1f1eb..82c4756 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > @@ -209,5 +209,14 @@ > clock-names = "uartclk", "apb_pclk"; > status = "disabled"; > }; > + > + dual_timer0: dual_timer at f8008000 { > + compatible = "arm,sp804", "arm,primecell"; > + reg = <0x0 0xf8008000 0x0 0x1000>; > + interrupts = , > + ; > + clocks = <&ao_ctrl 27>; > + clock-names = "apb_pclk"; How can this work? You only give the apb_pclk for clocking the bus to the timer. Most platforms using this driver has something like this: timer01: timer at 10104000 { compatible = "arm,sp804", "arm,primecell"; reg = <0x10104000 0x1000>; interrupt-parent = <&intc_dc1176>; interrupts = <0 8 IRQ_TYPE_LEVEL_HIGH>, <0 9 IRQ_TYPE_LEVEL_HIGH>; clocks = <&timclk>, <&timclk>, <&pclk>; clock-names = "timer1", "timer2", "apb_pclk"; }; It then reads the two clocks in the beginning of clocks() to determine the frequency of each timer. By chance the code in the driver will allow just one clock and will then assume that both the bus to the timer and the timer itself is clocked from the same clock. But I highly doubt that this is the case. Please verify what clocks actually goes into this timer, it should nominally be three of them. Yours, Linus Walleij