RE: [PATCH v5 6/7] clocksource: Add Pistachio clocksource-only driver

[Govindraj Raja]

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