Hi Felipe,


On 09/29/2015 10:44 PM, Felipe Balbi wrote:

Introduce a new clocksource driver for Texas
Instruments 32.768 Hz device which is available
on most OMAP-like devices.

Signed-off-by: Felipe Balbi <redacted>
---

[ ... ]

+config CLKSRC_TI_32K
+	bool "Texas Instruments 32.768 Hz Clocksource"
+	depends on OF && ARCH_OMAP2PLUS
+	select CLKSRC_OF
+	help
+	  This option enables support for Texas Instruments 32.768 Hz clocksource
+	  available on many OMAP-like platforms.
+

It is the omap's Kconfig which should select the timer, not the user to 
enable the timer.

It should be something like:

config CLKSRC_TI_32K
	bool "Texas Instruments 32.768 Hz Clocksource" if COMPILE_TEST
	select CLKSRC_OF if OF
	help
	  This option enables support for Texas Instruments 32.768 Hz
	  clocksource available on many OMAP-like platforms.

And in the omap's Kconfig:
	select CLKSRC_TI_32K

Can you check all these headers are needed ? I don't think interrupt.h, 
or slab.h or irq.h, etc ... are needed.

A few nits below:

+/* OMAP2_32KSYNCNT_CR_OFF: offset of 32ksync counter register */

I am not sure this comment gives some interesting indication.

+#define OMAP2_32KSYNCNT_REV_OFF		0x0
+#define OMAP2_32KSYNCNT_REV_SCHEME	(0x3 << 30)
+#define OMAP2_32KSYNCNT_CR_OFF_LOW	0x10
+#define OMAP2_32KSYNCNT_CR_OFF_HIGH	0x30
+
+/*
+ * 32KHz clocksource ... always available, on pretty most chips except
+ * OMAP 730 and 1510.  Other timers could be used as clocksources, with
+ * higher resolution in free-running counter modes (e.g. 12 MHz xtal),
+ * but systems won't necessarily want to spend resources that way.
+ */
+static void __iomem *sync32k_cnt_reg;
+
+/**
+ * omap_read_persistent_clock64 -  Return time from a persistent clock.
+ *
+ * Reads the time from a source which isn't disabled during PM, the
+ * 32k sync timer.  Convert the cycles elapsed since last read into
+ * nsecs and adds to a monotonically increasing timespec64.
+ */

This comment above should be moved right before the function it 
describes and in order to comply with the kernel-doc format the 
parameters must be documented also:
...
* @ts:	....
...

+static struct timespec64 persistent_ts;
+static cycles_t cycles;
+static unsigned int persistent_mult, persistent_shift;
+
+static u64 notrace omap_32k_read_sched_clock(void)
+{
+	return readl_relaxed(sync32k_cnt_reg);
+}
+
+static void omap_read_persistent_clock64(struct timespec64 *ts)
+{
+	unsigned long long nsecs;
+	cycles_t last_cycles;
+
+	last_cycles = cycles;
+	cycles = sync32k_cnt_reg ? readl_relaxed(sync32k_cnt_reg) : 0;

This test is pointless because 'sync32k_cnt_reg' is always different 
from zero regarding the init routine, no ?

+
+	nsecs = clocksource_cyc2ns(cycles - last_cycles,
+					persistent_mult, persistent_shift);
+
+	timespec64_add_ns(&persistent_ts, nsecs);
+
+	*ts = persistent_ts;
+}
+
+static void __init ti_32k_timer_init(struct device_node *np)
+{
+	void __iomem *vbase;
+	int ret;
+
+	vbase = of_iomap(np, 0);
+	if (!vbase) {
+		pr_err("Can't ioremap 32k timer base\n");
+		return;
+	}
+
+	/*
+	 * 32k sync Counter IP register offsets vary between the
+	 * highlander version and the legacy ones.
+	 * The 'SCHEME' bits(30-31) of the revision register is used
+	 * to identify the version.
+	 */

Please fix comment length.

+	if (readl_relaxed(vbase + OMAP2_32KSYNCNT_REV_OFF) &
+						OMAP2_32KSYNCNT_REV_SCHEME)
+		sync32k_cnt_reg = vbase + OMAP2_32KSYNCNT_CR_OFF_HIGH;
+	else
+		sync32k_cnt_reg = vbase + OMAP2_32KSYNCNT_CR_OFF_LOW;
+
+	/*
+	 * 120000 rough estimate from the calculations in
+	 * __clocksource_update_freq_scale.
+	 */

Fix comment length also.

+	clocks_calc_mult_shift(&persistent_mult, &persistent_shift,
+			32768, NSEC_PER_SEC, 120000);
+
+	ret = clocksource_mmio_init(sync32k_cnt_reg, "32k_counter", 32768,
+				250, 32, clocksource_mmio_readl_up);
+	if (ret) {
+		pr_err("32k_counter: can't register clocksource\n");
+		return;
+	}
+
+	sched_clock_register(omap_32k_read_sched_clock, 32, 32768);
+	register_persistent_clock(NULL, omap_read_persistent_clock64);

I will let John Stultz to have a look at this part because I have doubt 
regarding the usage of the persistent clock.


   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog