Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753877AbbHGPOo (ORCPT ); Fri, 7 Aug 2015 11:14:44 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:31132 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753241AbbHGPOm (ORCPT ); Fri, 7 Aug 2015 11:14:42 -0400 From: Govindraj Raja To: Daniel Lezcano , "linux-kernel@vger.kernel.org" , "linux-mips@linux-mips.org" , "devicetree@vger.kernel.org" CC: Thomas Gleixner , Andrew Bresticker , James Hartley , "Damien Horsley" , James Hogan , Ezequiel Garcia , Ezequiel Garcia Subject: RE: [PATCH v5 6/7] clocksource: Add Pistachio clocksource-only driver Thread-Topic: [PATCH v5 6/7] clocksource: Add Pistachio clocksource-only driver Thread-Index: AQHQ0DowIj92zDyCIEa7yxK/5E3b8Z4ARlOAgABdXMA= Date: Fri, 7 Aug 2015 15:14:38 +0000 Message-ID: <4BF5E8683E87FC4DA89822A5A3EB60CB6F25D2@hhmail02.hh.imgtec.org> References: <1438860138-31718-1-git-send-email-govindraj.raja@imgtec.com> <55C48841.3050902@linaro.org> In-Reply-To: <55C48841.3050902@linaro.org> Accept-Language: en-US Content-Language: en-AU X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [192.168.167.98] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t77FEnma023405 Content-Length: 5498 Lines: 194 Hi Daniel, > -----Original Message----- > From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] > Sent: 07 August 2015 11:28 AM > To: Govindraj Raja; linux-kernel@vger.kernel.org; linux-mips@linux-mips.org; > devicetree@vger.kernel.org > Cc: Thomas Gleixner; Andrew Bresticker; James Hartley; Damien Horsley; James > Hogan; Ezequiel Garcia; Ezequiel Garcia > Subject: Re: [PATCH v5 6/7] clocksource: Add Pistachio clocksource-only driver > > On 08/06/2015 01:22 PM, Govindraj Raja wrote: > > From: Ezequiel Garcia > > > > The Pistachio SoC provides four general purpose timers, and allow to > > implement a clocksource driver. > > > > This driver can be used as a replacement for the MIPS GIC and MIPS R4K > > clocksources and sched clocks, which are clocked from the CPU clock. > > > > Given the general purpose timers are clocked from an independent > > clock, this new clocksource driver will be useful to introduce CPUFreq > > support for Pistachio machines. > > > > Signed-off-by: Ezequiel Garcia > > Signed-off-by: Govindraj Raja > > --- > > > > changes from v4 > > ---------------- > > Fixes comments from Daniel as dicussed in below thread: > > http://patchwork.linux-mips.org/patch/10784/ > > > > > > drivers/clocksource/Kconfig | 5 + > > drivers/clocksource/Makefile | 1 + > > drivers/clocksource/time-pistachio.c | 216 > +++++++++++++++++++++++++++++++++++ > > 3 files changed, 222 insertions(+) > > create mode 100644 drivers/clocksource/time-pistachio.c > > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > [ ... ] > > > index 4e57730..e642825 100644 > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > @@ -111,6 +111,11 @@ config CLKSRC_LPC32XX > > select CLKSRC_MMIO > > select CLKSRC_OF > > > > +config CLKSRC_PISTACHIO > > + bool > > + default y if MACH_PISTACHIO > > You don't need to add this condition. Ok fine. Will remove it. > > > + select CLKSRC_OF > > [ ... ] > > > + > > +struct pistachio_clocksource pcs_gpt; > > static. > Ok. > > +#define to_pistachio_clocksource(cs) \ > > + container_of(cs, struct pistachio_clocksource, cs) > > + > > +static inline u32 gpt_readl(void __iomem *base, u32 offset, u32 > > +gpt_id) { > > + return readl(base + 0x20 * gpt_id + offset); > > Are you sure of the address computation ? I guess it is correct but just wanted > you to double check. > Yes, tested it on Pistachio bring up board. > > +} > > + > > +static inline void gpt_writel(void __iomem *base, u32 value, u32 offset, > > + u32 gpt_id) > > +{ > > + writel(value, base + 0x20 * gpt_id + offset); } > > + > > +static cycle_t pistachio_clocksource_read_cycles(struct clocksource > > +*cs) { > > + struct pistachio_clocksource *pcs = to_pistachio_clocksource(cs); > > + u32 counter, overflw; > > + unsigned long flags; > > + > > + /* The counter value is only refreshed after the overflow value is read. > > + * And they must be read in strict order, hence raw spin lock added. > > + */ > > nit: extra carriage return and comment format: > > /* > * blabla > */ > Ok. > > + raw_spin_lock_irqsave(&pcs->lock, flags); > > + overflw = gpt_readl(pcs->base, TIMER_CURRENT_OVERFLOW_VALUE, > 0); > > + counter = gpt_readl(pcs->base, TIMER_CURRENT_VALUE, 0); > > + raw_spin_unlock_irqrestore(&pcs->lock, flags); > > + > > + return ~(cycle_t)counter; > > +} > > + > > +static u64 notrace pistachio_read_sched_clock(void) { > > + return pistachio_clocksource_read_cycles(&pcs_gpt.cs); > > Can you double check the u32 cast to cycle_t returning a u64 from this function ? > Sorry I didn't get you over this comment. AFAIU, from include/linux/types.h [..] /* clocksource cycle base type */ typedef u64 cycle_t; [..] Did you mean to check this? > > +} > > + > > +static void pistachio_clksrc_set_mode(struct clocksource *cs, int timeridx, > > + int enable) > > +{ > > + struct pistachio_clocksource *pcs = to_pistachio_clocksource(cs); > > + u32 val; > > + > > + val = gpt_readl(pcs->base, TIMER_CFG, timeridx); > > + if (enable) > > + val |= TIMER_ME_LOCAL; > > + else > > + val &= ~TIMER_ME_LOCAL; > > + > > + gpt_writel(pcs->base, val, TIMER_CFG, timeridx); } > > + > > +static void pistachio_clksrc_enable(struct clocksource *cs, int > > +timeridx) { > > + struct pistachio_clocksource *pcs = to_pistachio_clocksource(cs); > > + > > + /* Disable GPT local before loading reload value */ > > + pistachio_clksrc_set_mode(cs, timeridx, false); > > + gpt_writel(pcs->base, RELOAD_VALUE, TIMER_RELOAD_VALUE, > timeridx); > > + pistachio_clksrc_set_mode(cs, timeridx, true); } > > + > > +static void pistachio_clksrc_disable(struct clocksource *cs, int > > +timeridx) { > > + /* Disable GPT local */ > > + pistachio_clksrc_set_mode(cs, timeridx, false); } > > + > > +static int pistachio_clocksource_enable(struct clocksource *cs) { > > + pistachio_clksrc_enable(cs, 0); > > + return 0; > > +} > > + > > +static void pistachio_clocksource_disable(struct clocksource *cs) { > > + pistachio_clksrc_disable(cs, 0); > > +} > > + > > +/* Desirable clock source for pistachio platform */ struct > > +pistachio_clocksource pcs_gpt = { > > static. Ok. Posting v6 addressing these comments. -- Thanks. Govindraj.R ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?