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:

quoted

From: Ezequiel Garcia <redacted>

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 <redacted>
Signed-off-by: Govindraj Raja <redacted>
---

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

+++++++++++++++++++++++++++++++++++

quoted

  3 files changed, 222 insertions(+)
  create mode 100644 drivers/clocksource/time-pistachio.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig

[ ... ]

quoted

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.

quoted

+	select CLKSRC_OF

[ ... ]

quoted

+
+struct pistachio_clocksource pcs_gpt;

static.

Ok.

quoted

+#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.

quoted

+}
+
+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.

quoted

+	raw_spin_lock_irqsave(&pcs->lock, flags);
+	overflw = gpt_readl(pcs->base, TIMER_CURRENT_OVERFLOW_VALUE,

0);

quoted

+	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?

quoted

+}
+
+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);

quoted

+	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
��칻
�&�~�&���+-��ݶ��w��˛���m�^�'
����{ay�
ʇڙ�,j��f���h�����/oSc��ڳ9�u�����&jw��(�階�ݢj"���m�����z�ޖ���f���h���~�m�