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=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,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 7C85AC43610 for ; Tue, 20 Nov 2018 08:17:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 429C920831 for ; Tue, 20 Nov 2018 08:17:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 429C920831 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726736AbeKTSoz convert rfc822-to-8bit (ORCPT ); Tue, 20 Nov 2018 13:44:55 -0500 Received: from foss.arm.com ([217.140.101.70]:44388 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725990AbeKTSoz (ORCPT ); Tue, 20 Nov 2018 13:44:55 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 24BC1A78; Tue, 20 Nov 2018 00:17:04 -0800 (PST) Received: from big-swifty.misterjones.org (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C96C53F5A0; Tue, 20 Nov 2018 00:17:00 -0800 (PST) Date: Tue, 20 Nov 2018 08:16:57 +0000 Message-ID: <86pnv0rwna.wl-marc.zyngier@arm.com> From: Marc Zyngier To: Manivannan Sadhasivam Cc: olof@lixom.net, arnd@arndb.de, robh+dt@kernel.org, tglx@linutronix.de, jason@lakedaemon.net, daniel.lezcano@linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, amit.kucheria@linaro.org, linus.walleij@linaro.org, zhao_steven@263.net, Andreas =?UTF-8?B?RsOkcmJl?= =?UTF-8?B?cg==?= Subject: Re: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC In-Reply-To: <20181120050650.GC5885@Mani-XPS-13-9360> References: <20181119170939.19153-1-manivannan.sadhasivam@linaro.org> <20181119170939.19153-13-manivannan.sadhasivam@linaro.org> <20181120050650.GC5885@Mani-XPS-13-9360> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/25.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Organization: ARM Ltd MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 20 Nov 2018 05:06:50 +0000, Manivannan Sadhasivam wrote: > > Hi Marc, > > On Mon, Nov 19, 2018 at 05:57:12PM +0000, Marc Zyngier wrote: > > On 19/11/2018 17:09, Manivannan Sadhasivam wrote: > > > Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER > > > and HWTIMER. > > > > > > Signed-off-by: Andreas Färber > > > Signed-off-by: Manivannan Sadhasivam > > > --- > > > arch/arm/mach-rda/Kconfig | 1 + > > > drivers/clocksource/Kconfig | 7 ++ > > > drivers/clocksource/Makefile | 1 + > > > drivers/clocksource/timer-rda.c | 187 ++++++++++++++++++++++++++++++++ > > > 4 files changed, 196 insertions(+) > > > create mode 100644 drivers/clocksource/timer-rda.c > > > > > > diff --git a/arch/arm/mach-rda/Kconfig b/arch/arm/mach-rda/Kconfig > > > index 29012bc68ca4..1ea753f57b2d 100644 > > > --- a/arch/arm/mach-rda/Kconfig > > > +++ b/arch/arm/mach-rda/Kconfig > > > @@ -4,5 +4,6 @@ menuconfig ARCH_RDA > > > select COMMON_CLK > > > select GENERIC_IRQ_CHIP > > > select RDA_INTC > > > + select RDA_TIMER > > > help > > > This enables support for the RDA Micro 8810PL SoC family. > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > > index 55c77e44bb2d..f51eee3a72ea 100644 > > > --- a/drivers/clocksource/Kconfig > > > +++ b/drivers/clocksource/Kconfig > > > @@ -105,6 +105,13 @@ config OWL_TIMER > > > help > > > Enables the support for the Actions Semi Owl timer driver. > > > > > > +config RDA_TIMER > > > + bool "RDA timer driver" if COMPILE_TEST > > > + depends on GENERIC_CLOCKEVENTS > > > + select CLKSRC_MMIO > > > + help > > > + Enables the support for the RDA Micro timer driver. > > > + > > > config SUN4I_TIMER > > > bool "Sun4i timer driver" if COMPILE_TEST > > > depends on HAS_IOMEM > > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > > > index dd9138104568..150020a90707 100644 > > > --- a/drivers/clocksource/Makefile > > > +++ b/drivers/clocksource/Makefile > > > @@ -57,6 +57,7 @@ obj-$(CONFIG_OXNAS_RPS_TIMER) += timer-oxnas-rps.o > > > obj-$(CONFIG_OWL_TIMER) += timer-owl.o > > > obj-$(CONFIG_SPRD_TIMER) += timer-sprd.o > > > obj-$(CONFIG_NPCM7XX_TIMER) += timer-npcm7xx.o > > > +obj-$(CONFIG_RDA_TIMER) += timer-rda.o > > > > > > obj-$(CONFIG_ARC_TIMERS) += arc_timer.o > > > obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o > > > diff --git a/drivers/clocksource/timer-rda.c b/drivers/clocksource/timer-rda.c > > > new file mode 100644 > > > index 000000000000..3aa684d92c5d > > > --- /dev/null > > > +++ b/drivers/clocksource/timer-rda.c > > > @@ -0,0 +1,187 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * RDA8810PL SoC timer driver > > > + * > > > + * Copyright RDA Microelectronics Company Limited > > > + * Copyright (c) 2017 Andreas Färber > > > + * Copyright (c) 2018 Manivannan Sadhasivam > > > + */ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#define RDA_OSTIMER_LOADVAL_L 0x000 > > > +#define RDA_OSTIMER_CTRL 0x004 > > > +#define RDA_HWTIMER_LOCKVAL_L 0x024 > > > +#define RDA_HWTIMER_LOCKVAL_H 0x028 > > > +#define RDA_TIMER_IRQ_MASK_SET 0x02c > > > +#define RDA_TIMER_IRQ_CLR 0x034 > > > + > > > +#define RDA_OSTIMER_CTRL_ENABLE BIT(24) > > > +#define RDA_OSTIMER_CTRL_REPEAT BIT(28) > > > +#define RDA_OSTIMER_CTRL_LOAD BIT(30) > > > + > > > +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER BIT(0) > > > + > > > +#define RDA_TIMER_IRQ_CLR_OSTIMER BIT(0) > > > + > > > +static void __iomem *rda_timer_base; > > > + > > > +static u64 rda_hwtimer_read(struct clocksource *cs) > > > +{ > > > + u32 lo, hi; > > > + > > > + /* Always read low 32 bits first */ > > > + lo = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_L); > > > + hi = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_H); > > > > Please use the relaxed accessors throughout this driver. There is zero > > reason to use the non-relaxed versions here. > > > > Okay. > > > Now, I'm pretty sure this thing isn't correct. > > > > > > lo = 0xffffffff; > > > > hi = 0x00000001; > > > > Bingo. You cannot read a 64bit counter with only two 32bit accesses. > > > > I think the lack of description makes confusion here. In this SoC, there > are two independent timers available: OSTIMER (56 bit) and HWTIMER (64 bit) > with optional interrupt support. I have used OSTIMER for clockevents and > HWTIMER for clocksource. Will add this information in driver. How does this change anything with the fact that the above code is broken? 56 or 64 bit, you cannot read this counter with a single access, or two. The canonical way of reading such a counter is something like this: do { lo = readl_relaxed(LO); hi = readl_relaxed(HI); } while (hi != read_relaxed(HI)); And yes, please add some documentation, as I have no idea which one is which. Having structure and function names that match the IP blocks used would also help. Thanks, M. -- Jazz is not dead, it just smell funny. From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Tue, 20 Nov 2018 08:16:57 +0000 Subject: [PATCH 12/16] clocksource: Add clock driver for RDA8810PL SoC In-Reply-To: <20181120050650.GC5885@Mani-XPS-13-9360> References: <20181119170939.19153-1-manivannan.sadhasivam@linaro.org> <20181119170939.19153-13-manivannan.sadhasivam@linaro.org> <20181120050650.GC5885@Mani-XPS-13-9360> Message-ID: <86pnv0rwna.wl-marc.zyngier@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 20 Nov 2018 05:06:50 +0000, Manivannan Sadhasivam wrote: > > Hi Marc, > > On Mon, Nov 19, 2018 at 05:57:12PM +0000, Marc Zyngier wrote: > > On 19/11/2018 17:09, Manivannan Sadhasivam wrote: > > > Add clock driver for RDA Micro RDA8810PL SoC supporting OSTIMER > > > and HWTIMER. > > > > > > Signed-off-by: Andreas F?rber > > > Signed-off-by: Manivannan Sadhasivam > > > --- > > > arch/arm/mach-rda/Kconfig | 1 + > > > drivers/clocksource/Kconfig | 7 ++ > > > drivers/clocksource/Makefile | 1 + > > > drivers/clocksource/timer-rda.c | 187 ++++++++++++++++++++++++++++++++ > > > 4 files changed, 196 insertions(+) > > > create mode 100644 drivers/clocksource/timer-rda.c > > > > > > diff --git a/arch/arm/mach-rda/Kconfig b/arch/arm/mach-rda/Kconfig > > > index 29012bc68ca4..1ea753f57b2d 100644 > > > --- a/arch/arm/mach-rda/Kconfig > > > +++ b/arch/arm/mach-rda/Kconfig > > > @@ -4,5 +4,6 @@ menuconfig ARCH_RDA > > > select COMMON_CLK > > > select GENERIC_IRQ_CHIP > > > select RDA_INTC > > > + select RDA_TIMER > > > help > > > This enables support for the RDA Micro 8810PL SoC family. > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > > index 55c77e44bb2d..f51eee3a72ea 100644 > > > --- a/drivers/clocksource/Kconfig > > > +++ b/drivers/clocksource/Kconfig > > > @@ -105,6 +105,13 @@ config OWL_TIMER > > > help > > > Enables the support for the Actions Semi Owl timer driver. > > > > > > +config RDA_TIMER > > > + bool "RDA timer driver" if COMPILE_TEST > > > + depends on GENERIC_CLOCKEVENTS > > > + select CLKSRC_MMIO > > > + help > > > + Enables the support for the RDA Micro timer driver. > > > + > > > config SUN4I_TIMER > > > bool "Sun4i timer driver" if COMPILE_TEST > > > depends on HAS_IOMEM > > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > > > index dd9138104568..150020a90707 100644 > > > --- a/drivers/clocksource/Makefile > > > +++ b/drivers/clocksource/Makefile > > > @@ -57,6 +57,7 @@ obj-$(CONFIG_OXNAS_RPS_TIMER) += timer-oxnas-rps.o > > > obj-$(CONFIG_OWL_TIMER) += timer-owl.o > > > obj-$(CONFIG_SPRD_TIMER) += timer-sprd.o > > > obj-$(CONFIG_NPCM7XX_TIMER) += timer-npcm7xx.o > > > +obj-$(CONFIG_RDA_TIMER) += timer-rda.o > > > > > > obj-$(CONFIG_ARC_TIMERS) += arc_timer.o > > > obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o > > > diff --git a/drivers/clocksource/timer-rda.c b/drivers/clocksource/timer-rda.c > > > new file mode 100644 > > > index 000000000000..3aa684d92c5d > > > --- /dev/null > > > +++ b/drivers/clocksource/timer-rda.c > > > @@ -0,0 +1,187 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * RDA8810PL SoC timer driver > > > + * > > > + * Copyright RDA Microelectronics Company Limited > > > + * Copyright (c) 2017 Andreas F?rber > > > + * Copyright (c) 2018 Manivannan Sadhasivam > > > + */ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#define RDA_OSTIMER_LOADVAL_L 0x000 > > > +#define RDA_OSTIMER_CTRL 0x004 > > > +#define RDA_HWTIMER_LOCKVAL_L 0x024 > > > +#define RDA_HWTIMER_LOCKVAL_H 0x028 > > > +#define RDA_TIMER_IRQ_MASK_SET 0x02c > > > +#define RDA_TIMER_IRQ_CLR 0x034 > > > + > > > +#define RDA_OSTIMER_CTRL_ENABLE BIT(24) > > > +#define RDA_OSTIMER_CTRL_REPEAT BIT(28) > > > +#define RDA_OSTIMER_CTRL_LOAD BIT(30) > > > + > > > +#define RDA_TIMER_IRQ_MASK_SET_OSTIMER BIT(0) > > > + > > > +#define RDA_TIMER_IRQ_CLR_OSTIMER BIT(0) > > > + > > > +static void __iomem *rda_timer_base; > > > + > > > +static u64 rda_hwtimer_read(struct clocksource *cs) > > > +{ > > > + u32 lo, hi; > > > + > > > + /* Always read low 32 bits first */ > > > + lo = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_L); > > > + hi = readl(rda_timer_base + RDA_HWTIMER_LOCKVAL_H); > > > > Please use the relaxed accessors throughout this driver. There is zero > > reason to use the non-relaxed versions here. > > > > Okay. > > > Now, I'm pretty sure this thing isn't correct. > > > > > > lo = 0xffffffff; > > > > hi = 0x00000001; > > > > Bingo. You cannot read a 64bit counter with only two 32bit accesses. > > > > I think the lack of description makes confusion here. In this SoC, there > are two independent timers available: OSTIMER (56 bit) and HWTIMER (64 bit) > with optional interrupt support. I have used OSTIMER for clockevents and > HWTIMER for clocksource. Will add this information in driver. How does this change anything with the fact that the above code is broken? 56 or 64 bit, you cannot read this counter with a single access, or two. The canonical way of reading such a counter is something like this: do { lo = readl_relaxed(LO); hi = readl_relaxed(HI); } while (hi != read_relaxed(HI)); And yes, please add some documentation, as I have no idea which one is which. Having structure and function names that match the IP blocks used would also help. Thanks, M. -- Jazz is not dead, it just smell funny.