Hi Daniel, > -----Original Message----- > From: Daniel Lezcano [mailto:daniel.lezcano@xxxxxxxxxx] > Sent: 07 August 2015 11:28 AM > To: Govindraj Raja; linux-kernel@xxxxxxxxxxxxxxx; linux-mips@xxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx > 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 <ezequiel.garcia@xxxxxxxxxx> > > > > 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 <ezequiel.garcia@xxxxxxxxxx> > > Signed-off-by: Govindraj Raja <govindraj.raja@xxxxxxxxxx> > > --- > > > > 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����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
