[PATCH 1/3] powerpc/mpic: add irq_set_wake support

[PATCH 1/3] powerpc/mpic: add irq_set_wake support

[Wang Dongsheng]

Add irq_set_wake support. Just add IRQF_NO_SUSPEND to desc->action->flag.
So the wake up interrupt will not be disable in suspend_device_irqs.

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
---
 arch/powerpc/sysdev/mpic.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 3b2efd4..10e474e 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -920,6 +920,18 @@ int mpic_set_irq_type(struct irq_data *d, unsigned int flow_type)
 	return IRQ_SET_MASK_OK_NOCOPY;
 }
 
+static int mpic_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct irq_desc *desc = container_of(d, struct irq_desc, irq_data);
+
+	if (on)
+		desc->action->flags |= IRQF_NO_SUSPEND;
+	else
+		desc->action->flags &= ~IRQF_NO_SUSPEND;
+
+	return 0;
+}
+
 void mpic_set_vector(unsigned int virq, unsigned int vector)
 {
 	struct mpic *mpic = mpic_from_irq(virq);
@@ -957,6 +969,7 @@ static struct irq_chip mpic_irq_chip = {
 	.irq_unmask	= mpic_unmask_irq,
 	.irq_eoi	= mpic_end_irq,
 	.irq_set_type	= mpic_set_irq_type,
+	.irq_set_wake	= mpic_irq_set_wake,
 };
 
 #ifdef CONFIG_SMP
@@ -971,6 +984,7 @@ static struct irq_chip mpic_tm_chip = {
 	.irq_mask	= mpic_mask_tm,
 	.irq_unmask	= mpic_unmask_tm,
 	.irq_eoi	= mpic_end_irq,
+	.irq_set_wake	= mpic_irq_set_wake,
 };
 
 #ifdef CONFIG_MPIC_U3_HT_IRQS
@@ -981,6 +995,7 @@ static struct irq_chip mpic_irq_ht_chip = {
 	.irq_unmask	= mpic_unmask_ht_irq,
 	.irq_eoi	= mpic_end_ht_irq,
 	.irq_set_type	= mpic_set_irq_type,
+	.irq_set_wake	= mpic_irq_set_wake,
 };
 #endif /* CONFIG_MPIC_U3_HT_IRQS */
 
-- 
1.7.5.1

[PATCH 2/3] powerpc/mpic: add global timer support

[Wang Dongsheng]

The MPIC global timer is a hardware timer inside the Freescale PIC comply
to Open-PIC standard. When the timer is timeout of the specified interval,
the hardware timer generates an interrupt. The driver currently is only
tested on fsl chip, but it can potentially support other global timers
complying to Open-PIC standard.

The two independent groups of global timer on fsl chip, group A and group B,
are identical in their functionality, except that they appear at different
locations within the PIC register map. The hardware timer can be cascaded to
create timers larger than the default 31-bit global timers. Timer cascade
fields allow configuration of up to two 63-bit timers. But These two groups
of timers cannot be cascaded together.

It can be used as a wakeup source for low power modes. It also could be used
as periodical timer for protocols, drivers and etc.

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/include/asm/mpic_timer.h |   46 +++
 arch/powerpc/platforms/Kconfig        |   12 +
 arch/powerpc/sysdev/Makefile          |    1 +
 arch/powerpc/sysdev/mpic_timer.c      |  606 +++++++++++++++++++++++++++++++++
 4 files changed, 665 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/include/asm/mpic_timer.h
 create mode 100644 arch/powerpc/sysdev/mpic_timer.c

diff --git a/arch/powerpc/include/asm/mpic_timer.h b/arch/powerpc/include/asm/mpic_timer.h
new file mode 100644
index 0000000..0e23cd4
--- /dev/null
+++ b/arch/powerpc/include/asm/mpic_timer.h
@@ -0,0 +1,46 @@
+/*
+ * arch/powerpc/include/asm/mpic_timer.h
+ *
+ * Header file for Mpic Global Timer
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * Author: Wang Dongsheng <Dongsheng.Wang@freescale.com>
+ *	   Li Yang <leoli@freescale.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#ifndef __MPIC_TIMER__
+#define __MPIC_TIMER__
+
+#include <linux/interrupt.h>
+#include <linux/time.h>
+
+struct mpic_timer {
+	void			*dev;
+	struct cascade_priv	*cascade_handle;
+	unsigned int		num;
+	unsigned int		irq;
+};
+
+#ifdef CONFIG_MPIC_TIMER
+struct mpic_timer *mpic_request_timer(irq_handler_t fn,  void *dev,
+		const struct timeval *time);
+void mpic_start_timer(struct mpic_timer *handle);
+void mpic_stop_timer(struct mpic_timer *handle);
+void mpic_get_remain_time(struct mpic_timer *handle, struct timeval *time);
+void mpic_free_timer(struct mpic_timer *handle);
+#else
+struct mpic_timer *mpic_request_timer(irq_handler_t fn,  void *dev,
+		const struct timeval *time) { return NULL; }
+void mpic_start_timer(struct mpic_timer *handle) { }
+void mpic_stop_timer(struct mpic_timer *handle) { }
+void mpic_get_remain_time(struct mpic_timer *handle, struct timeval *time) { }
+void mpic_free_timer(struct mpic_timer *handle) { }
+#endif
+
+#endif
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index 48a920d..5af04fa 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -87,6 +87,18 @@ config MPIC
 	bool
 	default n
 
+config MPIC_TIMER
+	bool "MPIC Global Timer"
+	depends on MPIC && FSL_SOC
+	default n
+	help
+	  The MPIC global timer is a hardware timer inside the
+	  Freescale PIC comply to Open-PIC standard. When the
+	  timer is timeout of the specified interval, the hardware
+	  timer generates an interrupt. The driver currently is
+	  only tested on fsl chip, but it can potentially support
+	  other global timers complying to Open-PIC standard.
+
 config PPC_EPAPR_HV_PIC
 	bool
 	default n
diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index a57600b..ff6184a 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -4,6 +4,7 @@ ccflags-$(CONFIG_PPC64)		:= -mno-minimal-toc
 
 mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
 obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
+obj-$(CONFIG_MPIC_TIMER)        += mpic_timer.o
 mpic-msgr-obj-$(CONFIG_MPIC_MSGR)	+= mpic_msgr.o
 obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y) $(mpic-msgr-obj-y)
 obj-$(CONFIG_PPC_EPAPR_HV_PIC)	+= ehv_pic.o
diff --git a/arch/powerpc/sysdev/mpic_timer.c b/arch/powerpc/sysdev/mpic_timer.c
new file mode 100644
index 0000000..3aaa3d4
--- /dev/null
+++ b/arch/powerpc/sysdev/mpic_timer.c
@@ -0,0 +1,606 @@
+/*
+ * MPIC timer driver
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ * Author: Dongsheng Wang <Dongsheng.Wang@freescale.com>
+ *	   Li Yang <leoli@freescale.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/mm.h>
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/syscore_ops.h>
+#include <sysdev/fsl_soc.h>
+#include <asm/io.h>
+
+#include <asm/mpic_timer.h>
+
+#define FSL_GLOBAL_TIMER		0x1
+
+#define MPIC_TIMER_TCR_CLKDIV_64	0x00000300
+#define MPIC_TIMER_TCR_ROVR_OFFSET	24
+
+#define TIMER_STOP			0x80000000
+#define TIMERS_PER_GROUP		4
+#define MAX_TICKS			(~0U >> 1)
+#define MAX_TICKS_CASCADE		(~0U)
+#define TIMER_OFFSET(num)		(1 << (TIMERS_PER_GROUP - 1 - num))
+
+/* tv_usec should be less than ONE_SECOND, otherwise use tv_sec */
+#define ONE_SECOND			1000000
+
+struct timer_regs {
+	u32	gtccr;
+	u32	res0[3];
+	u32	gtbcr;
+	u32	res1[3];
+	u32	gtvpr;
+	u32	res2[3];
+	u32	gtdr;
+	u32	res3[3];
+};
+
+struct cascade_priv {
+	u32 tcr_value;			/* TCR register: CASC & ROVR value */
+	unsigned int cascade_map;	/* cascade map */
+	unsigned int timer_num;		/* cascade control timer */
+};
+
+struct timer_group_priv {
+	struct timer_regs __iomem	*regs;
+	struct mpic_timer		timer[TIMERS_PER_GROUP];
+	struct list_head		node;
+	unsigned int			timerfreq;
+	unsigned int			idle;
+	unsigned int			flags;
+	spinlock_t			lock;
+	void __iomem			*group_tcr;
+};
+
+static struct cascade_priv cascade_timer[] = {
+	/* cascade timer 0 and 1 */
+	{0x1, 0xc, 0x1},
+	/* cascade timer 1 and 2 */
+	{0x2, 0x6, 0x2},
+	/* cascade timer 2 and 3 */
+	{0x4, 0x3, 0x3}
+};
+
+static LIST_HEAD(timer_group_list);
+
+static void convert_ticks_to_time(struct timer_group_priv *priv,
+		const u64 ticks, struct timeval *time)
+{
+	u64 tmp_sec;
+	u32 rem_us;
+	u32 div;
+
+	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
+		time->tv_sec = (__kernel_time_t)
+			div_u64_rem(ticks, priv->timerfreq, &rem_us);
+		tmp_sec = (u64)time->tv_sec * (u64)priv->timerfreq;
+		time->tv_usec = (__kernel_suseconds_t)
+			div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq);
+
+		return;
+	}
+
+	div = (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
+
+	time->tv_sec = (__kernel_time_t)div_u64(ticks, priv->timerfreq / div);
+	tmp_sec = div_u64((u64)time->tv_sec * (u64)priv->timerfreq, div);
+
+	time->tv_usec = (__kernel_suseconds_t)
+		div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq / div);
+
+	return;
+}
+
+/* the time set by the user is converted to "ticks" */
+static int convert_time_to_ticks(struct timer_group_priv *priv,
+		const struct timeval *time, u64 *ticks)
+{
+	u64 max_value;		/* prevent u64 overflow */
+	u64 tmp = 0;
+
+	u64 tmp_sec;
+	u64 tmp_ms;
+	u64 tmp_us;
+	u32 div;
+
+	max_value = div_u64(ULLONG_MAX, priv->timerfreq);
+
+	if (time->tv_sec > max_value ||
+			(time->tv_sec == max_value && time->tv_usec > 0))
+		return -EINVAL;
+
+	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
+		tmp_sec = time->tv_sec * priv->timerfreq;
+		tmp_ms = time->tv_usec / 1000 * priv->timerfreq / 1000;
+		tmp_us = time->tv_usec % 1000 * priv->timerfreq / 1000000;
+
+		*ticks = tmp_sec + tmp_ms + tmp_us;
+
+		return 0;
+	}
+
+	div = (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
+
+	tmp_sec = div_u64((u64)time->tv_sec * (u64)priv->timerfreq, div);
+	tmp += tmp_sec;
+
+	tmp_ms = time->tv_usec / 1000;
+	tmp_ms = div_u64((u64)tmp_ms * (u64)priv->timerfreq, div * 1000);
+	tmp += tmp_ms;
+
+	tmp_us = time->tv_usec % 1000;
+	tmp_us = div_u64((u64)tmp_us * (u64)priv->timerfreq, div * 1000000);
+	tmp += tmp_us;
+
+	*ticks = tmp;
+
+	return 0;
+}
+
+/* detect whether there is a cascade timer available */
+static struct mpic_timer *detect_idle_cascade_timer(
+					struct timer_group_priv *priv)
+{
+	struct cascade_priv *casc_priv;
+	unsigned int map;
+	unsigned int array_size = ARRAY_SIZE(cascade_timer);
+	unsigned int num;
+	unsigned int i;
+	unsigned long flags;
+
+	casc_priv = cascade_timer;
+	for (i = 0; i < array_size; i++) {
+		spin_lock_irqsave(&priv->lock, flags);
+		map = casc_priv->cascade_map & priv->idle;
+		if (map == casc_priv->cascade_map) {
+			num = casc_priv->timer_num;
+			priv->timer[num].cascade_handle = casc_priv;
+
+			/* set timer busy */
+			priv->idle &= ~casc_priv->cascade_map;
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return &priv->timer[num];
+		}
+		spin_unlock_irqrestore(&priv->lock, flags);
+		casc_priv++;
+	}
+
+	return NULL;
+}
+
+static int set_cascade_timer(struct timer_group_priv *priv, u64 ticks,
+		unsigned int num)
+{
+	struct cascade_priv *casc_priv;
+	u32 tcr;
+	u32 tmp_ticks;
+	u32 rem_ticks;
+
+	/* set group tcr reg for cascade */
+	casc_priv = priv->timer[num].cascade_handle;
+	if (!casc_priv)
+		return -EINVAL;
+
+	tcr = casc_priv->tcr_value |
+		(casc_priv->tcr_value << MPIC_TIMER_TCR_ROVR_OFFSET);
+	setbits32(priv->group_tcr, tcr);
+
+	tmp_ticks = div_u64_rem(ticks, MAX_TICKS_CASCADE, &rem_ticks);
+
+	out_be32(&priv->regs[num].gtccr, 0);
+	out_be32(&priv->regs[num].gtbcr, tmp_ticks | TIMER_STOP);
+
+	out_be32(&priv->regs[num - 1].gtccr, 0);
+	out_be32(&priv->regs[num - 1].gtbcr, rem_ticks);
+
+	return 0;
+}
+
+static struct mpic_timer *get_cascade_timer(struct timer_group_priv *priv,
+					u64 ticks)
+{
+	struct mpic_timer *allocated_timer;
+
+	/* Two cascade timers: Support the maximum time */
+	const u64 max_ticks = (u64)MAX_TICKS * (u64)MAX_TICKS_CASCADE;
+	int ret;
+
+	if (ticks > max_ticks)
+		return NULL;
+
+	/* detect idle timer */
+	allocated_timer = detect_idle_cascade_timer(priv);
+	if (!allocated_timer)
+		return NULL;
+
+	/* set ticks to timer */
+	ret = set_cascade_timer(priv, ticks, allocated_timer->num);
+	if (ret < 0)
+		return NULL;
+
+	return allocated_timer;
+}
+
+static struct mpic_timer *get_timer(const struct timeval *time)
+{
+	struct timer_group_priv *priv;
+	struct mpic_timer *timer;
+
+	u64 ticks;
+	unsigned int num;
+	unsigned int i;
+	unsigned long flags;
+	int ret;
+
+	list_for_each_entry(priv, &timer_group_list, node) {
+		ret = convert_time_to_ticks(priv, time, &ticks);
+		if (ret < 0)
+			return NULL;
+
+		if (ticks > MAX_TICKS) {
+			if (!(priv->flags & FSL_GLOBAL_TIMER))
+				return NULL;
+
+			timer = get_cascade_timer(priv, ticks);
+			if (!timer)
+				continue;
+			else
+				return timer;
+		}
+
+		for (i = 0; i < TIMERS_PER_GROUP; i++) {
+			/* one timer: Reverse allocation */
+			num = TIMERS_PER_GROUP - 1 - i;
+			spin_lock_irqsave(&priv->lock, flags);
+			if (priv->idle & (1 << i)) {
+				/* set timer busy */
+				priv->idle &= ~(1 << i);
+				/* set ticks & stop timer */
+				out_be32(&priv->regs[num].gtbcr,
+					ticks | TIMER_STOP);
+				out_be32(&priv->regs[num].gtccr, 0);
+				priv->timer[num].cascade_handle = NULL;
+				spin_unlock_irqrestore(&priv->lock, flags);
+				return &priv->timer[num];
+			}
+			spin_unlock_irqrestore(&priv->lock, flags);
+		}
+	}
+
+	return NULL;
+}
+
+/**
+ * mpic_start_timer - start hardware timer
+ * @handle: the timer to be started.
+ *
+ * It will do ->fn(->dev) callback from the hardware interrupt at
+ * the ->timeval point in the future.
+ */
+void mpic_start_timer(struct mpic_timer *handle)
+{
+	struct timer_group_priv *priv = container_of(handle,
+			struct timer_group_priv, timer[handle->num]);
+
+	clrbits32(&priv->regs[handle->num].gtbcr, TIMER_STOP);
+}
+EXPORT_SYMBOL(mpic_start_timer);
+
+/**
+ * mpic_stop_timer - stop hardware timer
+ * @handle: the timer to be stoped
+ *
+ * The timer periodically generates an interrupt. Unless user stops the timer.
+ */
+void mpic_stop_timer(struct mpic_timer *handle)
+{
+	struct timer_group_priv *priv = container_of(handle,
+			struct timer_group_priv, timer[handle->num]);
+	struct cascade_priv *casc_priv;
+
+	setbits32(&priv->regs[handle->num].gtbcr, TIMER_STOP);
+
+	casc_priv = priv->timer[handle->num].cascade_handle;
+	if (casc_priv) {
+		out_be32(&priv->regs[handle->num].gtccr, 0);
+		out_be32(&priv->regs[handle->num - 1].gtccr, 0);
+	} else {
+		out_be32(&priv->regs[handle->num].gtccr, 0);
+	}
+}
+EXPORT_SYMBOL(mpic_stop_timer);
+
+/**
+ * mpic_get_remain_time - get timer time
+ * @handle: the timer to be selected.
+ * @time: time for timer
+ *
+ * Query timer remaining time.
+ */
+void mpic_get_remain_time(struct mpic_timer *handle, struct timeval *time)
+{
+	struct timer_group_priv *priv = container_of(handle,
+			struct timer_group_priv, timer[handle->num]);
+	struct cascade_priv *casc_priv;
+
+	u64 ticks;
+	u32 tmp_ticks;
+
+	casc_priv = priv->timer[handle->num].cascade_handle;
+	if (casc_priv) {
+		tmp_ticks = in_be32(&priv->regs[handle->num].gtccr);
+		ticks = ((u64)tmp_ticks & UINT_MAX) * (u64)MAX_TICKS_CASCADE;
+		tmp_ticks = in_be32(&priv->regs[handle->num - 1].gtccr);
+		ticks += tmp_ticks;
+	} else {
+		ticks = in_be32(&priv->regs[handle->num].gtccr);
+	}
+
+	convert_ticks_to_time(priv, ticks, time);
+}
+EXPORT_SYMBOL(mpic_get_remain_time);
+
+/**
+ * mpic_free_timer - free hardware timer
+ * @handle: the timer to be removed.
+ *
+ * Free the timer.
+ *
+ * Note: can not be used in interrupt context.
+ */
+void mpic_free_timer(struct mpic_timer *handle)
+{
+	struct timer_group_priv *priv = container_of(handle,
+			struct timer_group_priv, timer[handle->num]);
+
+	struct cascade_priv *casc_priv;
+	unsigned long flags;
+
+	mpic_stop_timer(handle);
+
+	casc_priv = priv->timer[handle->num].cascade_handle;
+
+	free_irq(priv->timer[handle->num].irq, priv->timer[handle->num].dev);
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (casc_priv) {
+		u32 tcr;
+		tcr = casc_priv->tcr_value | (casc_priv->tcr_value <<
+					MPIC_TIMER_TCR_ROVR_OFFSET);
+		clrbits32(priv->group_tcr, tcr);
+		priv->idle |= casc_priv->cascade_map;
+		priv->timer[handle->num].cascade_handle = NULL;
+	} else {
+		priv->idle |= TIMER_OFFSET(handle->num);
+	}
+	spin_unlock_irqrestore(&priv->lock, flags);
+}
+EXPORT_SYMBOL(mpic_free_timer);
+
+/**
+ * mpic_request_timer - get a hardware timer
+ * @fn: interrupt handler function
+ * @dev: callback function of the data
+ * @time: time for timer
+ *
+ * This executes the "request_irq", returning NULL
+ * else "handle" on success.
+ */
+struct mpic_timer *mpic_request_timer(irq_handler_t fn, void *dev,
+					const struct timeval *time)
+{
+	struct mpic_timer *allocated_timer;
+	int ret;
+
+	if (list_empty(&timer_group_list))
+		return NULL;
+
+	if (!(time->tv_sec + time->tv_usec) ||
+			time->tv_sec < 0 || time->tv_usec < 0)
+		return NULL;
+
+	if (time->tv_usec > ONE_SECOND)
+		return NULL;
+
+	allocated_timer = get_timer(time);
+	if (!allocated_timer)
+		return NULL;
+
+	ret = request_irq(allocated_timer->irq, fn,
+			IRQF_TRIGGER_LOW, "global-timer", dev);
+	if (ret) {
+		mpic_free_timer(allocated_timer);
+		return NULL;
+	}
+
+	allocated_timer->dev = dev;
+
+	return allocated_timer;
+}
+EXPORT_SYMBOL(mpic_request_timer);
+
+static int timer_group_get_freq(struct device_node *np,
+			struct timer_group_priv *priv)
+{
+	if (priv->flags & FSL_GLOBAL_TIMER) {
+		struct device_node *dn;
+
+		dn = of_find_compatible_node(NULL, NULL, "fsl,mpic");
+		if (dn) {
+			of_property_read_u32(dn, "clock-frequency",
+					&priv->timerfreq);
+			of_node_put(dn);
+		}
+	}
+
+	if (priv->timerfreq <= 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int timer_group_get_irq(struct device_node *np,
+		struct timer_group_priv *priv)
+{
+	const u32 all_timer[] = { 0, TIMERS_PER_GROUP };
+	const u32 *p;
+	u32 offset;
+	u32 count;
+
+	unsigned int i;
+	unsigned int j;
+	unsigned int irq_index = 0;
+	unsigned int irq;
+	int len;
+
+	p = of_get_property(np, "fsl,available-ranges", &len);
+	if (p && len % (2 * sizeof(u32)) != 0) {
+		pr_err("%s: malformed available-ranges property.\n",
+				np->full_name);
+		return -EINVAL;
+	}
+
+	if (!p) {
+		p = all_timer;
+		len = sizeof(all_timer);
+	}
+
+	len /= 2 * sizeof(u32);
+
+	for (i = 0; i < len; i++) {
+		offset = p[i * 2];
+		count = p[i * 2 + 1];
+		for (j = 0; j < count; j++) {
+			irq = irq_of_parse_and_map(np, irq_index);
+			if (!irq) {
+				pr_err("%s: irq parse and map failed.\n",
+						np->full_name);
+				return -EINVAL;
+			}
+
+			/* Set timer idle */
+			priv->idle |= TIMER_OFFSET((offset + j));
+			priv->timer[offset + j].irq = irq;
+			priv->timer[offset + j].num = offset + j;
+			irq_index++;
+		}
+	}
+
+	return 0;
+}
+
+static void timer_group_init(struct device_node *np)
+{
+	struct timer_group_priv *priv;
+	unsigned int i = 0;
+	int ret;
+
+	priv = kzalloc(sizeof(struct timer_group_priv), GFP_KERNEL);
+	if (!priv) {
+		pr_err("%s: cannot allocate memory for group.\n",
+				np->full_name);
+		return;
+	}
+
+	if (of_device_is_compatible(np, "fsl,mpic-global-timer"))
+		priv->flags |= FSL_GLOBAL_TIMER;
+
+	priv->regs = of_iomap(np, i++);
+	if (!priv->regs) {
+		pr_err("%s: cannot ioremap timer register address.\n",
+				np->full_name);
+		goto out;
+	}
+
+	if (priv->flags & FSL_GLOBAL_TIMER) {
+		priv->group_tcr = of_iomap(np, i++);
+		if (!priv->group_tcr) {
+			pr_err("%s: cannot ioremap tcr address.\n",
+					np->full_name);
+			goto out;
+		}
+	}
+
+	ret = timer_group_get_freq(np, priv);
+	if (ret < 0) {
+		pr_err("%s: cannot get timer frequency.\n", np->full_name);
+		goto out;
+	}
+
+	ret = timer_group_get_irq(np, priv);
+	if (ret < 0) {
+		pr_err("%s: cannot get timer irqs.\n", np->full_name);
+		goto out;
+	}
+
+	spin_lock_init(&priv->lock);
+
+	/* Init FSL timer hardware */
+	if (priv->flags & FSL_GLOBAL_TIMER)
+		setbits32(priv->group_tcr, MPIC_TIMER_TCR_CLKDIV_64);
+
+	list_add_tail(&priv->node, &timer_group_list);
+
+	return;
+
+out:
+	if (priv->regs)
+		iounmap(priv->regs);
+
+	if (priv->group_tcr)
+		iounmap(priv->group_tcr);
+
+	kfree(priv);
+}
+
+static void mpic_timer_resume(void)
+{
+	struct timer_group_priv *priv;
+
+	list_for_each_entry(priv, &timer_group_list, node) {
+		/* Init FSL timer hardware */
+		if (priv->flags & FSL_GLOBAL_TIMER)
+			setbits32(priv->group_tcr, MPIC_TIMER_TCR_CLKDIV_64);
+	}
+}
+
+static const struct of_device_id mpic_timer_ids[] = {
+	{ .compatible = "fsl,mpic-global-timer", },
+	{},
+};
+
+static struct syscore_ops mpic_timer_syscore_ops = {
+	.resume = mpic_timer_resume,
+};
+
+static int __init mpic_timer_init(void)
+{
+	struct device_node *np = NULL;
+
+	for_each_matching_node(np, mpic_timer_ids)
+		timer_group_init(np);
+
+	register_syscore_ops(&mpic_timer_syscore_ops);
+
+	if (list_empty(&timer_group_list))
+		return -ENODEV;
+
+	return 0;
+}
+subsys_initcall(mpic_timer_init);
-- 
1.7.5.1

[PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Wang Dongsheng]

The driver provides a way to wake up the system by the MPIC timer.

For example,
echo 5 > /sys/devices/system/mpic/timer_wakeup
echo standby > /sys/power/state

After 5 seconds the MPIC timer will generate an interrupt to wake up
the system.

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
 arch/powerpc/platforms/Kconfig              |    9 ++
 arch/powerpc/sysdev/Makefile                |    1 +
 arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c |  185 +++++++++++++++++++++++++++
 3 files changed, 195 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c

diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index 5af04fa..487c37f 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -99,6 +99,15 @@ config MPIC_TIMER
 	  only tested on fsl chip, but it can potentially support
 	  other global timers complying to Open-PIC standard.
 
+config FSL_MPIC_TIMER_WAKEUP
+	tristate "Freescale MPIC global timer wakeup driver"
+	depends on FSL_SOC &&  MPIC_TIMER
+	default n
+	help
+	  This is only for freescale powerpc platform. The driver
+	  provides a way to wake up the system by MPIC timer,
+	  e.g. "echo 5 > /sys/devices/system/mpic/timer_wakeup"
+
 config PPC_EPAPR_HV_PIC
 	bool
 	default n
diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index ff6184a..e1b8a80 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -5,6 +5,7 @@ ccflags-$(CONFIG_PPC64)		:= -mno-minimal-toc
 mpic-msi-obj-$(CONFIG_PCI_MSI)	+= mpic_msi.o mpic_u3msi.o mpic_pasemi_msi.o
 obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y)
 obj-$(CONFIG_MPIC_TIMER)        += mpic_timer.o
+obj-$(CONFIG_FSL_MPIC_TIMER_WAKEUP)	+= fsl_mpic_timer_wakeup.o
 mpic-msgr-obj-$(CONFIG_MPIC_MSGR)	+= mpic_msgr.o
 obj-$(CONFIG_MPIC)		+= mpic.o $(mpic-msi-obj-y) $(mpic-msgr-obj-y)
 obj-$(CONFIG_PPC_EPAPR_HV_PIC)	+= ehv_pic.o
diff --git a/arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c b/arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
new file mode 100644
index 0000000..e94ba65
--- /dev/null
+++ b/arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
@@ -0,0 +1,185 @@
+/*
+ * MPIC timer wakeup driver
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+
+#include <asm/mpic_timer.h>
+
+struct fsl_mpic_timer_wakeup {
+	struct mpic_timer *timer;
+	struct work_struct free_work;
+};
+
+static struct fsl_mpic_timer_wakeup *fsl_wakeup;
+static DEFINE_MUTEX(sysfs_lock);
+
+static void fsl_free_resource(struct work_struct *ws)
+{
+	struct fsl_mpic_timer_wakeup *wakeup =
+		container_of(ws, struct fsl_mpic_timer_wakeup, free_work);
+
+	mutex_lock(&sysfs_lock);
+
+	if (wakeup->timer) {
+		disable_irq_wake(wakeup->timer->irq);
+		mpic_free_timer(wakeup->timer);
+	}
+
+	wakeup->timer = NULL;
+	mutex_unlock(&sysfs_lock);
+}
+
+static irqreturn_t fsl_mpic_timer_irq(int irq, void *dev_id)
+{
+	struct fsl_mpic_timer_wakeup *wakeup = dev_id;
+
+	schedule_work(&wakeup->free_work);
+	return IRQ_HANDLED;
+}
+
+static ssize_t fsl_timer_wakeup_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct timeval interval;
+	int val = 0;
+
+	mutex_lock(&sysfs_lock);
+	if (fsl_wakeup->timer) {
+		mpic_get_remain_time(fsl_wakeup->timer, &interval);
+		val = interval.tv_sec + 1;
+	}
+	mutex_unlock(&sysfs_lock);
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t fsl_timer_wakeup_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf,
+				size_t count)
+{
+	struct timeval interval;
+	int ret;
+
+	interval.tv_usec = 0;
+	if (kstrtol(buf, 0, &interval.tv_sec))
+		return -EINVAL;
+
+	mutex_lock(&sysfs_lock);
+
+	if (fsl_wakeup->timer && !interval.tv_sec) {
+		disable_irq_wake(fsl_wakeup->timer->irq);
+		mpic_free_timer(fsl_wakeup->timer);
+		fsl_wakeup->timer = NULL;
+		mutex_unlock(&sysfs_lock);
+
+		return count;
+	}
+
+	if (fsl_wakeup->timer) {
+		mutex_unlock(&sysfs_lock);
+		return -EBUSY;
+	}
+
+	fsl_wakeup->timer = mpic_request_timer(fsl_mpic_timer_irq,
+						fsl_wakeup, &interval);
+	if (!fsl_wakeup->timer) {
+		mutex_unlock(&sysfs_lock);
+		return -EINVAL;
+	}
+
+	ret = enable_irq_wake(fsl_wakeup->timer->irq);
+	if (ret) {
+		mpic_free_timer(fsl_wakeup->timer);
+		fsl_wakeup->timer = NULL;
+		mutex_unlock(&sysfs_lock);
+
+		return ret;
+	}
+	mpic_start_timer(fsl_wakeup->timer);
+
+	mutex_unlock(&sysfs_lock);
+
+	return count;
+}
+
+static struct bus_type mpic_subsys = {
+	.name = "mpic",
+	.dev_name = "mpic",
+};
+
+static DEVICE_ATTR(timer_wakeup, 0644,
+			fsl_timer_wakeup_show, fsl_timer_wakeup_store);
+
+static struct device_attribute *mpic_attributes[] = {
+	&dev_attr_timer_wakeup,
+	NULL
+};
+
+static int __init fsl_wakeup_sys_init(void)
+{
+	int ret;
+	int i;
+
+	fsl_wakeup = kzalloc(sizeof(struct fsl_mpic_timer_wakeup), GFP_KERNEL);
+	if (!fsl_wakeup)
+		return -ENOMEM;
+
+	INIT_WORK(&fsl_wakeup->free_work, fsl_free_resource);
+
+	ret = subsys_system_register(&mpic_subsys, NULL);
+	if (ret)
+		goto err;
+
+	for (i = 0; mpic_attributes[i]; i++) {
+		ret = device_create_file(mpic_subsys.dev_root,
+					mpic_attributes[i]);
+		if (ret)
+			goto err2;
+	}
+
+	return ret;
+
+err2:
+	while (--i >= 0)
+		device_remove_file(mpic_subsys.dev_root, mpic_attributes[i]);
+
+	bus_unregister(&mpic_subsys);
+
+err:
+	kfree(fsl_wakeup);
+
+	return ret;
+}
+
+static void __exit fsl_wakeup_sys_exit(void)
+{
+	int i;
+
+	for (i = 0; mpic_attributes[i]; i++)
+		device_remove_file(mpic_subsys.dev_root,
+				mpic_attributes[i]);
+	bus_unregister(&mpic_subsys);
+	kfree(fsl_wakeup);
+}
+
+module_init(fsl_wakeup_sys_init);
+module_exit(fsl_wakeup_sys_exit);
+
+MODULE_DESCRIPTION("Freescale MPIC global timer wakeup driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Wang Dongsheng <dongsheng.wang@freescale.com>");
-- 
1.7.5.1

RE: [PATCH 1/3] powerpc/mpic: add irq_set_wake support

[Wang Dongsheng-B40534]

Hi Benjamin & Kumar,

I am not sure who can apply these patches...

Could you apply these patches?

Thanks.

[1/3] powerpc/mpic: add irq_set_wake support
http://patchwork.ozlabs.org/patch/226034/

[2/3] powerpc/mpic: add global timer support
http://patchwork.ozlabs.org/patch/226035/

[3/3] powerpc/fsl: add MPIC timer wakeup support
http://patchwork.ozlabs.org/patch/226036/

> -----Original Message-----
> From: Wang Dongsheng-B40534
> Sent: Friday, March 08, 2013 3:39 PM
> To: Wood Scott-B07421; Gala Kumar-B11780
> Cc: linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-B40534
> Subject: [PATCH 1/3] powerpc/mpic: add irq_set_wake support
>=20
> Add irq_set_wake support. Just add IRQF_NO_SUSPEND to desc->action->flag.
> So the wake up interrupt will not be disable in suspend_device_irqs.
>=20
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
>  arch/powerpc/sysdev/mpic.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>=20
> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> index 3b2efd4..10e474e 100644
> --- a/arch/powerpc/sysdev/mpic.c
> +++ b/arch/powerpc/sysdev/mpic.c
> @@ -920,6 +920,18 @@ int mpic_set_irq_type(struct irq_data *d, unsigned
> int flow_type)
>  	return IRQ_SET_MASK_OK_NOCOPY;
>  }
>=20
> +static int mpic_irq_set_wake(struct irq_data *d, unsigned int on) {
> +	struct irq_desc *desc =3D container_of(d, struct irq_desc, irq_data);
> +
> +	if (on)
> +		desc->action->flags |=3D IRQF_NO_SUSPEND;
> +	else
> +		desc->action->flags &=3D ~IRQF_NO_SUSPEND;
> +
> +	return 0;
> +}
> +
>  void mpic_set_vector(unsigned int virq, unsigned int vector)  {
>  	struct mpic *mpic =3D mpic_from_irq(virq); @@ -957,6 +969,7 @@ static
> struct irq_chip mpic_irq_chip =3D {
>  	.irq_unmask	=3D mpic_unmask_irq,
>  	.irq_eoi	=3D mpic_end_irq,
>  	.irq_set_type	=3D mpic_set_irq_type,
> +	.irq_set_wake	=3D mpic_irq_set_wake,
>  };
>=20
>  #ifdef CONFIG_SMP
> @@ -971,6 +984,7 @@ static struct irq_chip mpic_tm_chip =3D {
>  	.irq_mask	=3D mpic_mask_tm,
>  	.irq_unmask	=3D mpic_unmask_tm,
>  	.irq_eoi	=3D mpic_end_irq,
> +	.irq_set_wake	=3D mpic_irq_set_wake,
>  };
>=20
>  #ifdef CONFIG_MPIC_U3_HT_IRQS
> @@ -981,6 +995,7 @@ static struct irq_chip mpic_irq_ht_chip =3D {
>  	.irq_unmask	=3D mpic_unmask_ht_irq,
>  	.irq_eoi	=3D mpic_end_ht_irq,
>  	.irq_set_type	=3D mpic_set_irq_type,
> +	.irq_set_wake	=3D mpic_irq_set_wake,
>  };
>  #endif /* CONFIG_MPIC_U3_HT_IRQS */
>=20
> --
> 1.7.5.1

Re: [PATCH 1/3] powerpc/mpic: add irq_set_wake support

[Benjamin Herrenschmidt]

On Mon, 2013-03-18 at 09:28 +0000, Wang Dongsheng-B40534 wrote:
> Hi Benjamin & Kumar,
> 
> I am not sure who can apply these patches...
> 
> Could you apply these patches?

I can but I need somebody to review them, I don't have the bandwidth nor
am I familiar with the FSL changes to the MPIC.

Cheers,
Ben.

> Thanks.
> 
> [1/3] powerpc/mpic: add irq_set_wake support
> http://patchwork.ozlabs.org/patch/226034/
> 
> [2/3] powerpc/mpic: add global timer support
> http://patchwork.ozlabs.org/patch/226035/
> 
> [3/3] powerpc/fsl: add MPIC timer wakeup support
> http://patchwork.ozlabs.org/patch/226036/
> 
> > -----Original Message-----
> > From: Wang Dongsheng-B40534
> > Sent: Friday, March 08, 2013 3:39 PM
> > To: Wood Scott-B07421; Gala Kumar-B11780
> > Cc: linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-B40534
> > Subject: [PATCH 1/3] powerpc/mpic: add irq_set_wake support
> > 
> > Add irq_set_wake support. Just add IRQF_NO_SUSPEND to desc->action->flag.
> > So the wake up interrupt will not be disable in suspend_device_irqs.
> > 
> > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > ---
> >  arch/powerpc/sysdev/mpic.c |   15 +++++++++++++++
> >  1 files changed, 15 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > index 3b2efd4..10e474e 100644
> > --- a/arch/powerpc/sysdev/mpic.c
> > +++ b/arch/powerpc/sysdev/mpic.c
> > @@ -920,6 +920,18 @@ int mpic_set_irq_type(struct irq_data *d, unsigned
> > int flow_type)
> >  	return IRQ_SET_MASK_OK_NOCOPY;
> >  }
> > 
> > +static int mpic_irq_set_wake(struct irq_data *d, unsigned int on) {
> > +	struct irq_desc *desc = container_of(d, struct irq_desc, irq_data);
> > +
> > +	if (on)
> > +		desc->action->flags |= IRQF_NO_SUSPEND;
> > +	else
> > +		desc->action->flags &= ~IRQF_NO_SUSPEND;
> > +
> > +	return 0;
> > +}
> > +
> >  void mpic_set_vector(unsigned int virq, unsigned int vector)  {
> >  	struct mpic *mpic = mpic_from_irq(virq); @@ -957,6 +969,7 @@ static
> > struct irq_chip mpic_irq_chip = {
> >  	.irq_unmask	= mpic_unmask_irq,
> >  	.irq_eoi	= mpic_end_irq,
> >  	.irq_set_type	= mpic_set_irq_type,
> > +	.irq_set_wake	= mpic_irq_set_wake,
> >  };
> > 
> >  #ifdef CONFIG_SMP
> > @@ -971,6 +984,7 @@ static struct irq_chip mpic_tm_chip = {
> >  	.irq_mask	= mpic_mask_tm,
> >  	.irq_unmask	= mpic_unmask_tm,
> >  	.irq_eoi	= mpic_end_irq,
> > +	.irq_set_wake	= mpic_irq_set_wake,
> >  };
> > 
> >  #ifdef CONFIG_MPIC_U3_HT_IRQS
> > @@ -981,6 +995,7 @@ static struct irq_chip mpic_irq_ht_chip = {
> >  	.irq_unmask	= mpic_unmask_ht_irq,
> >  	.irq_eoi	= mpic_end_ht_irq,
> >  	.irq_set_type	= mpic_set_irq_type,
> > +	.irq_set_wake	= mpic_irq_set_wake,
> >  };
> >  #endif /* CONFIG_MPIC_U3_HT_IRQS */
> > 
> > --
> > 1.7.5.1
> 

Re: [PATCH 1/3] powerpc/mpic: add irq_set_wake support

[Gala Kumar-B11780]

On Mar 18, 2013, at 9:41 AM, Benjamin Herrenschmidt wrote:

> On Mon, 2013-03-18 at 09:28 +0000, Wang Dongsheng-B40534 wrote:
>> Hi Benjamin & Kumar,
>>=20
>> I am not sure who can apply these patches...
>>=20
>> Could you apply these patches?
>=20
> I can but I need somebody to review them, I don't have the bandwidth nor
> am I familiar with the FSL changes to the MPIC.
>=20
> Cheers,
> Ben.

I'd ask for Scott's ack/signoff on these patches for ben or I to accept the=
m.

- k

>=20
>> Thanks.
>>=20
>> [1/3] powerpc/mpic: add irq_set_wake support
>> http://patchwork.ozlabs.org/patch/226034/
>>=20
>> [2/3] powerpc/mpic: add global timer support
>> http://patchwork.ozlabs.org/patch/226035/
>>=20
>> [3/3] powerpc/fsl: add MPIC timer wakeup support
>> http://patchwork.ozlabs.org/patch/226036/
>>=20
>>> -----Original Message-----
>>> From: Wang Dongsheng-B40534
>>> Sent: Friday, March 08, 2013 3:39 PM
>>> To: Wood Scott-B07421; Gala Kumar-B11780
>>> Cc: linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-B40534
>>> Subject: [PATCH 1/3] powerpc/mpic: add irq_set_wake support
>>>=20
>>> Add irq_set_wake support. Just add IRQF_NO_SUSPEND to desc->action->fla=
g.
>>> So the wake up interrupt will not be disable in suspend_device_irqs.
>>>=20
>>> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
>>> ---
>>> arch/powerpc/sysdev/mpic.c |   15 +++++++++++++++
>>> 1 files changed, 15 insertions(+), 0 deletions(-)
>>>=20
>>> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
>>> index 3b2efd4..10e474e 100644
>>> --- a/arch/powerpc/sysdev/mpic.c
>>> +++ b/arch/powerpc/sysdev/mpic.c
>>> @@ -920,6 +920,18 @@ int mpic_set_irq_type(struct irq_data *d, unsigned
>>> int flow_type)
>>> 	return IRQ_SET_MASK_OK_NOCOPY;
>>> }
>>>=20
>>> +static int mpic_irq_set_wake(struct irq_data *d, unsigned int on) {
>>> +	struct irq_desc *desc =3D container_of(d, struct irq_desc, irq_data);
>>> +
>>> +	if (on)
>>> +		desc->action->flags |=3D IRQF_NO_SUSPEND;
>>> +	else
>>> +		desc->action->flags &=3D ~IRQF_NO_SUSPEND;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> void mpic_set_vector(unsigned int virq, unsigned int vector)  {
>>> 	struct mpic *mpic =3D mpic_from_irq(virq); @@ -957,6 +969,7 @@ static
>>> struct irq_chip mpic_irq_chip =3D {
>>> 	.irq_unmask	=3D mpic_unmask_irq,
>>> 	.irq_eoi	=3D mpic_end_irq,
>>> 	.irq_set_type	=3D mpic_set_irq_type,
>>> +	.irq_set_wake	=3D mpic_irq_set_wake,
>>> };
>>>=20
>>> #ifdef CONFIG_SMP
>>> @@ -971,6 +984,7 @@ static struct irq_chip mpic_tm_chip =3D {
>>> 	.irq_mask	=3D mpic_mask_tm,
>>> 	.irq_unmask	=3D mpic_unmask_tm,
>>> 	.irq_eoi	=3D mpic_end_irq,
>>> +	.irq_set_wake	=3D mpic_irq_set_wake,
>>> };
>>>=20
>>> #ifdef CONFIG_MPIC_U3_HT_IRQS
>>> @@ -981,6 +995,7 @@ static struct irq_chip mpic_irq_ht_chip =3D {
>>> 	.irq_unmask	=3D mpic_unmask_ht_irq,
>>> 	.irq_eoi	=3D mpic_end_ht_irq,
>>> 	.irq_set_type	=3D mpic_set_irq_type,
>>> +	.irq_set_wake	=3D mpic_irq_set_wake,
>>> };
>>> #endif /* CONFIG_MPIC_U3_HT_IRQS */
>>>=20
>>> --
>>> 1.7.5.1
>>=20
>=20
>=20
>=20

Re: [PATCH 2/3] powerpc/mpic: add global timer support

[Scott Wood]

On 03/08/2013 01:38:46 AM, Wang Dongsheng wrote:
> The MPIC global timer is a hardware timer inside the Freescale PIC =20
> comply
> to Open-PIC standard. When the timer is timeout of the specified =20
> interval,
> the hardware timer generates an interrupt. The driver currently is =20
> only
> tested on fsl chip, but it can potentially support other global timers
> complying to Open-PIC standard.
>=20
> The two independent groups of global timer on fsl chip, group A and =20
> group B,
> are identical in their functionality, except that they appear at =20
> different
> locations within the PIC register map. The hardware timer can be =20
> cascaded to
> create timers larger than the default 31-bit global timers. Timer =20
> cascade
> fields allow configuration of up to two 63-bit timers. But These two =20
> groups
> of timers cannot be cascaded together.
>=20
> It can be used as a wakeup source for low power modes. It also could =20
> be used
> as periodical timer for protocols, drivers and etc.
>=20
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
>  arch/powerpc/include/asm/mpic_timer.h |   46 +++
>  arch/powerpc/platforms/Kconfig        |   12 +
>  arch/powerpc/sysdev/Makefile          |    1 +
>  arch/powerpc/sysdev/mpic_timer.c      |  606 =20
> +++++++++++++++++++++++++++++++++
>  4 files changed, 665 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/include/asm/mpic_timer.h
>  create mode 100644 arch/powerpc/sysdev/mpic_timer.c
>=20
> diff --git a/arch/powerpc/include/asm/mpic_timer.h =20
> b/arch/powerpc/include/asm/mpic_timer.h
> new file mode 100644
> index 0000000..0e23cd4
> --- /dev/null
> +++ b/arch/powerpc/include/asm/mpic_timer.h
> @@ -0,0 +1,46 @@
> +/*
> + * arch/powerpc/include/asm/mpic_timer.h
> + *
> + * Header file for Mpic Global Timer
> + *
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * Author: Wang Dongsheng <Dongsheng.Wang@freescale.com>
> + *	   Li Yang <leoli@freescale.com>
> + *
> + * This program is free software; you can redistribute it and/or =20
> modify it
> + * under the terms of the GNU General Public License as published by =20
> the
> + * Free Software Foundation; either version 2 of the License, or (at =20
> your
> + * option) any later version.
> + */
> +
> +#ifndef __MPIC_TIMER__
> +#define __MPIC_TIMER__
> +
> +#include <linux/interrupt.h>
> +#include <linux/time.h>
> +
> +struct mpic_timer {
> +	void			*dev;
> +	struct cascade_priv	*cascade_handle;
> +	unsigned int		num;
> +	unsigned int		irq;
> +};
> +
> +#ifdef CONFIG_MPIC_TIMER
> +struct mpic_timer *mpic_request_timer(irq_handler_t fn,  void *dev,
> +		const struct timeval *time);
> +void mpic_start_timer(struct mpic_timer *handle);
> +void mpic_stop_timer(struct mpic_timer *handle);
> +void mpic_get_remain_time(struct mpic_timer *handle, struct timeval =20
> *time);
> +void mpic_free_timer(struct mpic_timer *handle);
> +#else
> +struct mpic_timer *mpic_request_timer(irq_handler_t fn,  void *dev,
> +		const struct timeval *time) { return NULL; }
> +void mpic_start_timer(struct mpic_timer *handle) { }
> +void mpic_stop_timer(struct mpic_timer *handle) { }
> +void mpic_get_remain_time(struct mpic_timer *handle, struct timeval =20
> *time) { }
> +void mpic_free_timer(struct mpic_timer *handle) { }
> +#endif

I'm not sure how useful these stubs are...

> +config MPIC_TIMER
> +	bool "MPIC Global Timer"
> +	depends on MPIC && FSL_SOC
> +	default n
> +	help
> +	  The MPIC global timer is a hardware timer inside the
> +	  Freescale PIC comply to Open-PIC standard. When the

s/comply to/complying with/
s/Open-PIC/OpenPIC/

> +	  timer is timeout of the specified interval, the hardware

s/timer is timeout of the specified interval/specified interval times =20
out/

> +	  timer generates an interrupt. The driver currently is
> +	  only tested on fsl chip, but it can potentially support
> +	  other global timers complying to Open-PIC standard.

s/to Open-PIC/with the OpenPIC/

> +static void convert_ticks_to_time(struct timer_group_priv *priv,
> +		const u64 ticks, struct timeval *time)
> +{
> +	u64 tmp_sec;
> +	u32 rem_us;
> +	u32 div;
> +
> +	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
> +		time->tv_sec =3D (__kernel_time_t)
> +			div_u64_rem(ticks, priv->timerfreq, &rem_us);
> +		tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> +		time->tv_usec =3D (__kernel_suseconds_t)
> +			div_u64((ticks - tmp_sec) * 1000000, =20
> priv->timerfreq);
> +
> +		return;
> +	}
> +
> +	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> +
> +	time->tv_sec =3D (__kernel_time_t)div_u64(ticks, priv->timerfreq =20
> / div);
> +	tmp_sec =3D div_u64((u64)time->tv_sec * (u64)priv->timerfreq, =20
> div);
> +
> +	time->tv_usec =3D (__kernel_suseconds_t)
> +		div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq / =20
> div);
> +
> +	return;

Why don't you just adjust the clock frequency up front for CLKDIV_64,
rather than introduce alternate (and untested!) code paths throughout =20
the
driver?

> +	list_for_each_entry(priv, &timer_group_list, node) {
> +		ret =3D convert_time_to_ticks(priv, time, &ticks);
> +		if (ret < 0)
> +			return NULL;
> +
> +		if (ticks > MAX_TICKS) {
> +			if (!(priv->flags & FSL_GLOBAL_TIMER))
> +				return NULL;
> +
> +			timer =3D get_cascade_timer(priv, ticks);
> +			if (!timer)
> +				continue;
> +			else
> +				return timer;
> +		}

	if (!timer)
		continue;

	return timer;

-Scott=

Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Scott Wood]

On 03/08/2013 01:38:47 AM, Wang Dongsheng wrote:
> The driver provides a way to wake up the system by the MPIC timer.
>=20
> For example,
> echo 5 > /sys/devices/system/mpic/timer_wakeup
> echo standby > /sys/power/state
>=20
> After 5 seconds the MPIC timer will generate an interrupt to wake up
> the system.
>=20
> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>

Does this work with deep sleep (echo mem > /sys/power/state on mpc8536, =20
p1022, etc) or just regular sleep?

> ---
>  arch/powerpc/platforms/Kconfig              |    9 ++
>  arch/powerpc/sysdev/Makefile                |    1 +
>  arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c |  185 =20
> +++++++++++++++++++++++++++
>  3 files changed, 195 insertions(+), 0 deletions(-)
>  create mode 100644 arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
>=20
> diff --git a/arch/powerpc/platforms/Kconfig =20
> b/arch/powerpc/platforms/Kconfig
> index 5af04fa..487c37f 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -99,6 +99,15 @@ config MPIC_TIMER
>  	  only tested on fsl chip, but it can potentially support
>  	  other global timers complying to Open-PIC standard.
>=20
> +config FSL_MPIC_TIMER_WAKEUP
> +	tristate "Freescale MPIC global timer wakeup driver"
> +	depends on FSL_SOC &&  MPIC_TIMER
> +	default n
> +	help
> +	  This is only for freescale powerpc platform.

This sentence is redundant... It already says "Freescale MPIC" in the =20
name and depends on "FSL_SOC && MPIC_TIMER".

> +static irqreturn_t fsl_mpic_timer_irq(int irq, void *dev_id)
> +{
> +	struct fsl_mpic_timer_wakeup *wakeup =3D dev_id;
> +
> +	schedule_work(&wakeup->free_work);
> +	return IRQ_HANDLED;
> +}

return wakeup->timer ? IRQ_HANDLED : IRQ_NONE;

> +
> +static ssize_t fsl_timer_wakeup_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct timeval interval;
> +	int val =3D 0;
> +
> +	mutex_lock(&sysfs_lock);
> +	if (fsl_wakeup->timer) {
> +		mpic_get_remain_time(fsl_wakeup->timer, &interval);
> +		val =3D interval.tv_sec + 1;
> +	}
> +	mutex_unlock(&sysfs_lock);
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t fsl_timer_wakeup_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf,
> +				size_t count)
> +{
> +	struct timeval interval;
> +	int ret;
> +
> +	interval.tv_usec =3D 0;
> +	if (kstrtol(buf, 0, &interval.tv_sec))
> +		return -EINVAL;

I don't think the buffer will NUL-terminated...  Ordinarily there'll be
an LF terminator, but you can't rely on that (many other sysfs =20
attributes
seem to, though...).

> +	mutex_lock(&sysfs_lock);
> +
> +	if (fsl_wakeup->timer && !interval.tv_sec) {
> +		disable_irq_wake(fsl_wakeup->timer->irq);
> +		mpic_free_timer(fsl_wakeup->timer);
> +		fsl_wakeup->timer =3D NULL;
> +		mutex_unlock(&sysfs_lock);
> +
> +		return count;
> +	}
> +
> +	if (fsl_wakeup->timer) {
> +		mutex_unlock(&sysfs_lock);
> +		return -EBUSY;
> +	}

So to change an already-set timer you have to set it to zero and then to
what you want?  Why not just do:

	if (fsl_wakeup->timer) {
		disable_irq_wake(...);
		mpic_free_timer(...);
		fsl_wakeup_timer =3D NULL;
	}

	if (!interval.tv_sec) {
		mutex_unlock(&sysfs_lock);
		return count;
	}

> +	ret =3D subsys_system_register(&mpic_subsys, NULL);
> +	if (ret)
> +		goto err;

Maybe arch/powerpc/sysdev/mpic.c should be doing this?

> +
> +	for (i =3D 0; mpic_attributes[i]; i++) {
> +		ret =3D device_create_file(mpic_subsys.dev_root,
> +					mpic_attributes[i]);
> +		if (ret)
> +			goto err2;
> +	}

Is this code ever going to register more than one?

-Scott=

RE: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Wang Dongsheng-B40534]

Hi Scoot,

Thanks for reviewing.

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, March 19, 2013 8:31 AM
> To: Wang Dongsheng-B40534
> Cc: Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-
> B40534; Zhao Chenhui-B35336; Li Yang-R58472
> Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
>=20
> On 03/08/2013 01:38:47 AM, Wang Dongsheng wrote:
> > The driver provides a way to wake up the system by the MPIC timer.
> >
> > For example,
> > echo 5 > /sys/devices/system/mpic/timer_wakeup
> > echo standby > /sys/power/state
> >
> > After 5 seconds the MPIC timer will generate an interrupt to wake up
> > the system.
> >
> > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> > Signed-off-by: Li Yang <leoli@freescale.com>
>=20
> Does this work with deep sleep (echo mem > /sys/power/state on mpc8536,
> p1022, etc) or just regular sleep?
>=20
The deep sleep can also work.

> > ---
> >  arch/powerpc/platforms/Kconfig              |    9 ++
> >  arch/powerpc/sysdev/Makefile                |    1 +
> >  arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c |  185
> > +++++++++++++++++++++++++++
> >  3 files changed, 195 insertions(+), 0 deletions(-)  create mode
> > 100644 arch/powerpc/sysdev/fsl_mpic_timer_wakeup.c
> >
> > diff --git a/arch/powerpc/platforms/Kconfig
> > b/arch/powerpc/platforms/Kconfig index 5af04fa..487c37f 100644
> > --- a/arch/powerpc/platforms/Kconfig
> > +++ b/arch/powerpc/platforms/Kconfig
> > @@ -99,6 +99,15 @@ config MPIC_TIMER
> >  	  only tested on fsl chip, but it can potentially support
> >  	  other global timers complying to Open-PIC standard.
> >
> > +config FSL_MPIC_TIMER_WAKEUP
> > +	tristate "Freescale MPIC global timer wakeup driver"
> > +	depends on FSL_SOC &&  MPIC_TIMER
> > +	default n
> > +	help
> > +	  This is only for freescale powerpc platform.
>=20
> This sentence is redundant... It already says "Freescale MPIC" in the
> name and depends on "FSL_SOC && MPIC_TIMER".
>=20
Um...yes, I will remove it.
=20
> > +static irqreturn_t fsl_mpic_timer_irq(int irq, void *dev_id) {
> > +	struct fsl_mpic_timer_wakeup *wakeup =3D dev_id;
> > +
> > +	schedule_work(&wakeup->free_work);
> > +	return IRQ_HANDLED;
> > +}
>=20
> return wakeup->timer ? IRQ_HANDLED : IRQ_NONE;
>=20
Looks better, thanks.

> > +
> > +static ssize_t fsl_timer_wakeup_show(struct device *dev,
> > +				struct device_attribute *attr,
> > +				char *buf)
> > +{
> > +	struct timeval interval;
> > +	int val =3D 0;
> > +
> > +	mutex_lock(&sysfs_lock);
> > +	if (fsl_wakeup->timer) {
> > +		mpic_get_remain_time(fsl_wakeup->timer, &interval);
> > +		val =3D interval.tv_sec + 1;
> > +	}
> > +	mutex_unlock(&sysfs_lock);
> > +
> > +	return sprintf(buf, "%d\n", val);
> > +}
> > +
> > +static ssize_t fsl_timer_wakeup_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf,
> > +				size_t count)
> > +{
> > +	struct timeval interval;
> > +	int ret;
> > +
> > +	interval.tv_usec =3D 0;
> > +	if (kstrtol(buf, 0, &interval.tv_sec))
> > +		return -EINVAL;
>=20
> I don't think the buffer will NUL-terminated...  Ordinarily there'll be
> an LF terminator, but you can't rely on that (many other sysfs attributes
> seem to, though...).
>=20
I think we don't need to care about LF terminator.
The kstrtol--> _kstrtoull has been done.

> > +	mutex_lock(&sysfs_lock);
> > +
> > +	if (fsl_wakeup->timer && !interval.tv_sec) {
> > +		disable_irq_wake(fsl_wakeup->timer->irq);
> > +		mpic_free_timer(fsl_wakeup->timer);
> > +		fsl_wakeup->timer =3D NULL;
> > +		mutex_unlock(&sysfs_lock);
> > +
> > +		return count;
> > +	}
> > +
> > +	if (fsl_wakeup->timer) {
> > +		mutex_unlock(&sysfs_lock);
> > +		return -EBUSY;
> > +	}
>=20
> So to change an already-set timer you have to set it to zero and then to
> what you want?  Why not just do:
>=20
> 	if (fsl_wakeup->timer) {
> 		disable_irq_wake(...);
> 		mpic_free_timer(...);
> 		fsl_wakeup_timer =3D NULL;
> 	}
>=20
> 	if (!interval.tv_sec) {
> 		mutex_unlock(&sysfs_lock);
> 		return count;
> 	}
>=20
You can't break up the it.
if echo zero the code will cancel the timer that is currently running.
Not echo non-zero value just zero to cancel.

> > +	ret =3D subsys_system_register(&mpic_subsys, NULL);
> > +	if (ret)
> > +		goto err;
>=20
> Maybe arch/powerpc/sysdev/mpic.c should be doing this?
>=20
This can be registered by mpic. Maybe better than here.

> > +
> > +	for (i =3D 0; mpic_attributes[i]; i++) {
> > +		ret =3D device_create_file(mpic_subsys.dev_root,
> > +					mpic_attributes[i]);
> > +		if (ret)
> > +			goto err2;
> > +	}
>=20
> Is this code ever going to register more than one?
>=20
No, just one. I only keep the style here.
If you don't think it's necessary I can remove this loop.

RE: [PATCH 2/3] powerpc/mpic: add global timer support

[Wang Dongsheng-B40534]

> > +static void convert_ticks_to_time(struct timer_group_priv *priv,
> > +		const u64 ticks, struct timeval *time) {
> > +	u64 tmp_sec;
> > +	u32 rem_us;
> > +	u32 div;
> > +
> > +	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
> > +		time->tv_sec =3D (__kernel_time_t)
> > +			div_u64_rem(ticks, priv->timerfreq, &rem_us);
> > +		tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> > +		time->tv_usec =3D (__kernel_suseconds_t)
> > +			div_u64((ticks - tmp_sec) * 1000000,
> > priv->timerfreq);
> > +
> > +		return;
> > +	}
> > +
> > +	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > +
> > +	time->tv_sec =3D (__kernel_time_t)div_u64(ticks, priv->timerfreq
> > / div);
> > +	tmp_sec =3D div_u64((u64)time->tv_sec * (u64)priv->timerfreq,
> > div);
> > +
> > +	time->tv_usec =3D (__kernel_suseconds_t)
> > +		div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq /
> > div);
> > +
> > +	return;
>=20
> Why don't you just adjust the clock frequency up front for CLKDIV_64,
> rather than introduce alternate (and untested!) code paths throughout the
> driver?
>=20
No, It cannot be integrated. The div cannot be removed.
Because if do priv->timerfreq /=3D div, that will affect the accuracy.

Like:
3 * 5 / 2 =3D 7;
3 / 2 * 5 =3D 5;

BTW
if (!(priv->flags & FSL_GLOBAL_TIMER)) {
	time->tv_sec =3D (__kernel_time_t)
		div_u64_rem(ticks, priv->timerfreq, &rem_us);
	tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
	time->tv_usec =3D (__kernel_suseconds_t)
		div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq);

	return;
}
This branch I has been tested.

Test methods:
1. Get timerfreq and set timerfreq.
   timerfreq /=3D 64;(Clock ratio is divide by 64)

2. Clear FSL_GLOBAL_TIMER flag.

Test Log:
[root@p5020 root]# echo 20 > /sys/devices/system/mpic/timer_wakeup
[root@p5020 root]# cat /sys/devices/system/mpic/timer_wakeup
sec =3D 18, ticks =3D 118295518.., timerfreq =3D 6249999..
19
[root@p5020 root]# cat /sys/devices/system/mpic/timer_wakeup
sec =3D 17, ticks =3D 110095766.., timerfreq =3D 6249999..
18
[root@p5020 root]# cat /sys/devices/system/mpic/timer_wakeup
sec =3D 16, ticks =3D 105095737.., timerfreq =3D 6249999..
17
[root@p5020 root]# cat /sys/devices/system/mpic/timer_wakeup
sec =3D 15, ticks =3D 99495711.., timerfreq =3D 6249999..
16

Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Scott Wood]

On 03/19/2013 01:25:42 AM, Wang Dongsheng-B40534 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, March 19, 2013 8:31 AM
> > To: Wang Dongsheng-B40534
> > Cc: Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org; Wang =20
> Dongsheng-
> > B40534; Zhao Chenhui-B35336; Li Yang-R58472
> > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> >
> > On 03/08/2013 01:38:47 AM, Wang Dongsheng wrote:
> > > +static ssize_t fsl_timer_wakeup_store(struct device *dev,
> > > +				struct device_attribute *attr,
> > > +				const char *buf,
> > > +				size_t count)
> > > +{
> > > +	struct timeval interval;
> > > +	int ret;
> > > +
> > > +	interval.tv_usec =3D 0;
> > > +	if (kstrtol(buf, 0, &interval.tv_sec))
> > > +		return -EINVAL;
> >
> > I don't think the buffer will NUL-terminated...  Ordinarily =20
> there'll be
> > an LF terminator, but you can't rely on that (many other sysfs =20
> attributes
> > seem to, though...).
> >
> I think we don't need to care about LF terminator.
> The kstrtol--> _kstrtoull has been done.

My point is, what happens if userspace passes in a buffer that has no =20
terminator of any sort?  kstrtol will continue reading beyond the end =20
of the buffer.

> > > +	mutex_lock(&sysfs_lock);
> > > +
> > > +	if (fsl_wakeup->timer && !interval.tv_sec) {
> > > +		disable_irq_wake(fsl_wakeup->timer->irq);
> > > +		mpic_free_timer(fsl_wakeup->timer);
> > > +		fsl_wakeup->timer =3D NULL;
> > > +		mutex_unlock(&sysfs_lock);
> > > +
> > > +		return count;
> > > +	}
> > > +
> > > +	if (fsl_wakeup->timer) {
> > > +		mutex_unlock(&sysfs_lock);
> > > +		return -EBUSY;
> > > +	}
> >
> > So to change an already-set timer you have to set it to zero and =20
> then to
> > what you want?  Why not just do:
> >
> > 	if (fsl_wakeup->timer) {
> > 		disable_irq_wake(...);
> > 		mpic_free_timer(...);
> > 		fsl_wakeup_timer =3D NULL;
> > 	}
> >
> > 	if (!interval.tv_sec) {
> > 		mutex_unlock(&sysfs_lock);
> > 		return count;
> > 	}
> >
> You can't break up the it.
> if echo zero the code will cancel the timer that is currently running.
> Not echo non-zero value just zero to cancel.

Echoing a nonzero value wouldn't just be to cancel, it would be to set =20
a new timer after cancelling the old.

> > > +	for (i =3D 0; mpic_attributes[i]; i++) {
> > > +		ret =3D device_create_file(mpic_subsys.dev_root,
> > > +					mpic_attributes[i]);
> > > +		if (ret)
> > > +			goto err2;
> > > +	}
> >
> > Is this code ever going to register more than one?
> >
> No, just one. I only keep the style here.
> If you don't think it's necessary I can remove this loop.

I don't think it's necessary.

-Scott=

Re: [PATCH 2/3] powerpc/mpic: add global timer support

[Scott Wood]

On 03/19/2013 02:55:58 AM, Wang Dongsheng-B40534 wrote:
> > > +static void convert_ticks_to_time(struct timer_group_priv *priv,
> > > +		const u64 ticks, struct timeval *time) {
> > > +	u64 tmp_sec;
> > > +	u32 rem_us;
> > > +	u32 div;
> > > +
> > > +	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
> > > +		time->tv_sec =3D (__kernel_time_t)
> > > +			div_u64_rem(ticks, priv->timerfreq, &rem_us);
> > > +		tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> > > +		time->tv_usec =3D (__kernel_suseconds_t)
> > > +			div_u64((ticks - tmp_sec) * 1000000,
> > > priv->timerfreq);
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > +
> > > +	time->tv_sec =3D (__kernel_time_t)div_u64(ticks, priv->timerfreq
> > > / div);
> > > +	tmp_sec =3D div_u64((u64)time->tv_sec * (u64)priv->timerfreq,
> > > div);
> > > +
> > > +	time->tv_usec =3D (__kernel_suseconds_t)
> > > +		div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq /
> > > div);
> > > +
> > > +	return;
> >
> > Why don't you just adjust the clock frequency up front for =20
> CLKDIV_64,
> > rather than introduce alternate (and untested!) code paths =20
> throughout the
> > driver?
> >
> No, It cannot be integrated. The div cannot be removed.
> Because if do priv->timerfreq /=3D div, that will affect the accuracy.
>=20
> Like:
> 3 * 5 / 2 =3D 7;
> 3 / 2 * 5 =3D 5;

I don't follow -- a change in the clock speed is a change in the clock =20
speed, no matter how you accomplish it.

How you round is a different question.  You should probably be rounding =20
up always, based on the final clock frequency -- though it's unlikely =20
to matter much given the high precision of the timer relative to the =20
input granularity.

> BTW
> if (!(priv->flags & FSL_GLOBAL_TIMER)) {
> 	time->tv_sec =3D (__kernel_time_t)
> 		div_u64_rem(ticks, priv->timerfreq, &rem_us);
> 	tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> 	time->tv_usec =3D (__kernel_suseconds_t)
> 		div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq);
>=20
> 	return;
> }
> This branch I has been tested.
>=20
> Test methods:
> 1. Get timerfreq and set timerfreq.
>    timerfreq /=3D 64;(Clock ratio is divide by 64)
>=20
> 2. Clear FSL_GLOBAL_TIMER flag.

Even if it was tested once, it's unlikely to continue to be tested =20
without a user.

-Scott=

RE: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, March 20, 2013 6:55 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Zhao Chenhui-B35336; Li Yang-R58472
> Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
>=20
> On 03/19/2013 01:25:42 AM, Wang Dongsheng-B40534 wrote:
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, March 19, 2013 8:31 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org; Wang
> > Dongsheng-
> > > B40534; Zhao Chenhui-B35336; Li Yang-R58472
> > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> > >
> > > On 03/08/2013 01:38:47 AM, Wang Dongsheng wrote:
> > > > +static ssize_t fsl_timer_wakeup_store(struct device *dev,
> > > > +				struct device_attribute *attr,
> > > > +				const char *buf,
> > > > +				size_t count)
> > > > +{
> > > > +	struct timeval interval;
> > > > +	int ret;
> > > > +
> > > > +	interval.tv_usec =3D 0;
> > > > +	if (kstrtol(buf, 0, &interval.tv_sec))
> > > > +		return -EINVAL;
> > >
> > > I don't think the buffer will NUL-terminated...  Ordinarily
> > there'll be
> > > an LF terminator, but you can't rely on that (many other sysfs
> > attributes
> > > seem to, though...).
> > >
> > I think we don't need to care about LF terminator.
> > The kstrtol--> _kstrtoull has been done.
>=20
> My point is, what happens if userspace passes in a buffer that has no
> terminator of any sort?  kstrtol will continue reading beyond the end of
> the buffer.
>=20
Do not care about terminator.

kstrtol--> _kstrtoull--> _parse_integer

_kstrtoull(...) {
	...
	rv =3D _parse_integer(s, base, &_res);
	if (rv & KSTRTOX_OVERFLOW)
		return -ERANGE;
	rv &=3D ~KSTRTOX_OVERFLOW;
	if (rv =3D=3D 0)
		return -EINVAL;
	s +=3D rv;

	if (*s =3D=3D '\n')
		s++;
	if (*s)
		return -EINVAL;
	...
}

_parse_integer(...) {
	...
	while (*s) {
		if ('0' <=3D *s && *s <=3D '9')
			val =3D *s - '0';
		else if ('a' <=3D _tolower(*s) && _tolower(*s) <=3D 'f')
			val =3D _tolower(*s) - 'a' + 10;
		else
			break;	//this will break out to convert.

		if (val >=3D base)
			break;
	}
	...
}

> > > > +	mutex_lock(&sysfs_lock);
> > > > +
> > > > +	if (fsl_wakeup->timer && !interval.tv_sec) {
> > > > +		disable_irq_wake(fsl_wakeup->timer->irq);
> > > > +		mpic_free_timer(fsl_wakeup->timer);
> > > > +		fsl_wakeup->timer =3D NULL;
> > > > +		mutex_unlock(&sysfs_lock);
> > > > +
> > > > +		return count;
> > > > +	}
> > > > +
> > > > +	if (fsl_wakeup->timer) {
> > > > +		mutex_unlock(&sysfs_lock);
> > > > +		return -EBUSY;
> > > > +	}
> > >
> > > So to change an already-set timer you have to set it to zero and
> > then to
> > > what you want?  Why not just do:
> > >
> > > 	if (fsl_wakeup->timer) {
> > > 		disable_irq_wake(...);
> > > 		mpic_free_timer(...);
> > > 		fsl_wakeup_timer =3D NULL;
> > > 	}
> > >
> > > 	if (!interval.tv_sec) {
> > > 		mutex_unlock(&sysfs_lock);
> > > 		return count;
> > > 	}
> > >
> > You can't break up the it.
> > if echo zero the code will cancel the timer that is currently running.
> > Not echo non-zero value just zero to cancel.
>=20
> Echoing a nonzero value wouldn't just be to cancel, it would be to set a
> new timer after cancelling the old.
>=20
If you think this way is better, I can change.
But why should do it?=20
Explicitly stop the timer (echo 0) before reuse it is more reasonable for m=
e.=20

> > > > +	for (i =3D 0; mpic_attributes[i]; i++) {
> > > > +		ret =3D device_create_file(mpic_subsys.dev_root,
> > > > +					mpic_attributes[i]);
> > > > +		if (ret)
> > > > +			goto err2;
> > > > +	}
> > >
> > > Is this code ever going to register more than one?
> > >
> > No, just one. I only keep the style here.
> > If you don't think it's necessary I can remove this loop.
>=20
> I don't think it's necessary.
>=20
Ok, I will remove this loop.

RE: [PATCH 2/3] powerpc/mpic: add global timer support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, March 20, 2013 6:59 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Li Yang-R58472
> Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
>=20
> On 03/19/2013 02:55:58 AM, Wang Dongsheng-B40534 wrote:
> > > > +static void convert_ticks_to_time(struct timer_group_priv *priv,
> > > > +		const u64 ticks, struct timeval *time) {
> > > > +	u64 tmp_sec;
> > > > +	u32 rem_us;
> > > > +	u32 div;
> > > > +
> > > > +	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
> > > > +		time->tv_sec =3D (__kernel_time_t)
> > > > +			div_u64_rem(ticks, priv->timerfreq, &rem_us);
> > > > +		tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> > > > +		time->tv_usec =3D (__kernel_suseconds_t)
> > > > +			div_u64((ticks - tmp_sec) * 1000000,
> > > > priv->timerfreq);
> > > > +
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > > +
> > > > +	time->tv_sec =3D (__kernel_time_t)div_u64(ticks, priv-
> >timerfreq
> > > > / div);
> > > > +	tmp_sec =3D div_u64((u64)time->tv_sec * (u64)priv->timerfreq,
> > > > div);
> > > > +
> > > > +	time->tv_usec =3D (__kernel_suseconds_t)
> > > > +		div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq /
> > > > div);
> > > > +
> > > > +	return;
> > >
> > > Why don't you just adjust the clock frequency up front for
> > CLKDIV_64,
> > > rather than introduce alternate (and untested!) code paths
> > throughout the
> > > driver?
> > >
> > No, It cannot be integrated. The div cannot be removed.
> > Because if do priv->timerfreq /=3D div, that will affect the accuracy.
> >
> > Like:
> > 3 * 5 / 2 =3D 7;
> > 3 / 2 * 5 =3D 5;
>=20
> I don't follow -- a change in the clock speed is a change in the clock
> speed, no matter how you accomplish it.
>=20
This is not change hardware clock frequency. The mpic timer hardware clock
is not be changed after initialization. This is just conversion ticks.
These calculated ticks will be set to the hardware.

> How you round is a different question.  You should probably be rounding
> up always, based on the final clock frequency -- though it's unlikely to
> matter much given the high precision of the timer relative to the input
> granularity.
>=20
Each ticks are based on the mpic timer hardware clock frequency.
The conversion and calculation are in order to make the tick value is more
accurate, more close to real time.
If echo 40 seconds may be difference is not obvious. But echo 315360000(10 =
years)
difference is obvious.

Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Scott Wood]

On 03/19/2013 10:48:53 PM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, March 20, 2013 6:55 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Zhao Chenhui-B35336; Li Yang-R58472
> > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> >
> > On 03/19/2013 01:25:42 AM, Wang Dongsheng-B40534 wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Tuesday, March 19, 2013 8:31 AM
> > > > To: Wang Dongsheng-B40534
> > > > Cc: Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org; Wang
> > > Dongsheng-
> > > > B40534; Zhao Chenhui-B35336; Li Yang-R58472
> > > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup =20
> support
> > > >
> > > > On 03/08/2013 01:38:47 AM, Wang Dongsheng wrote:
> > > > > +static ssize_t fsl_timer_wakeup_store(struct device *dev,
> > > > > +				struct device_attribute *attr,
> > > > > +				const char *buf,
> > > > > +				size_t count)
> > > > > +{
> > > > > +	struct timeval interval;
> > > > > +	int ret;
> > > > > +
> > > > > +	interval.tv_usec =3D 0;
> > > > > +	if (kstrtol(buf, 0, &interval.tv_sec))
> > > > > +		return -EINVAL;
> > > >
> > > > I don't think the buffer will NUL-terminated...  Ordinarily
> > > there'll be
> > > > an LF terminator, but you can't rely on that (many other sysfs
> > > attributes
> > > > seem to, though...).
> > > >
> > > I think we don't need to care about LF terminator.
> > > The kstrtol--> _kstrtoull has been done.
> >
> > My point is, what happens if userspace passes in a buffer that has =20
> no
> > terminator of any sort?  kstrtol will continue reading beyond the =20
> end of
> > the buffer.
> >
> Do not care about terminator.

kstrtol() obviously *does* because it doesn't take the buffer length as =20
a parameter.

> kstrtol--> _kstrtoull--> _parse_integer
>=20
> _kstrtoull(...) {
> 	...
> 	rv =3D _parse_integer(s, base, &_res);
> 	if (rv & KSTRTOX_OVERFLOW)
> 		return -ERANGE;
> 	rv &=3D ~KSTRTOX_OVERFLOW;
> 	if (rv =3D=3D 0)
> 		return -EINVAL;
> 	s +=3D rv;
>=20
> 	if (*s =3D=3D '\n')
> 		s++;
> 	if (*s)
> 		return -EINVAL;
> 	...
> }
>=20
> _parse_integer(...) {
> 	...
> 	while (*s) {
> 		if ('0' <=3D *s && *s <=3D '9')
> 			val =3D *s - '0';
> 		else if ('a' <=3D _tolower(*s) && _tolower(*s) <=3D 'f')
> 			val =3D _tolower(*s) - 'a' + 10;
> 		else
> 			break;	//this will break out to convert.

Really?  How do you know that the next byte after the buffer isn't a =20
valid hex digit?  How do you even know that we won't take a fault =20
accessing it?

> > Echoing a nonzero value wouldn't just be to cancel, it would be to =20
> set a
> > new timer after cancelling the old.
> >
> If you think this way is better, I can change.

I do.

> But why should do it?
> Explicitly stop the timer (echo 0) before reuse it is more reasonable =20
> for me.

It's an unnecessary restriction, and eliminating it doesn't make =20
anything simpler.

-Scott=

Re: [PATCH 2/3] powerpc/mpic: add global timer support

[Scott Wood]

On 03/20/2013 01:45:03 AM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, March 20, 2013 6:59 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Li Yang-R58472
> > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> >
> > On 03/19/2013 02:55:58 AM, Wang Dongsheng-B40534 wrote:
> > > > > +static void convert_ticks_to_time(struct timer_group_priv =20
> *priv,
> > > > > +		const u64 ticks, struct timeval *time) {
> > > > > +	u64 tmp_sec;
> > > > > +	u32 rem_us;
> > > > > +	u32 div;
> > > > > +
> > > > > +	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
> > > > > +		time->tv_sec =3D (__kernel_time_t)
> > > > > +			div_u64_rem(ticks, priv->timerfreq, =20
> &rem_us);
> > > > > +		tmp_sec =3D (u64)time->tv_sec * =20
> (u64)priv->timerfreq;
> > > > > +		time->tv_usec =3D (__kernel_suseconds_t)
> > > > > +			div_u64((ticks - tmp_sec) * 1000000,
> > > > > priv->timerfreq);
> > > > > +
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > > > +
> > > > > +	time->tv_sec =3D (__kernel_time_t)div_u64(ticks, priv-
> > >timerfreq
> > > > > / div);
> > > > > +	tmp_sec =3D div_u64((u64)time->tv_sec * =20
> (u64)priv->timerfreq,
> > > > > div);
> > > > > +
> > > > > +	time->tv_usec =3D (__kernel_suseconds_t)
> > > > > +		div_u64((ticks - tmp_sec) * 1000000, =20
> priv->timerfreq /
> > > > > div);
> > > > > +
> > > > > +	return;
> > > >
> > > > Why don't you just adjust the clock frequency up front for
> > > CLKDIV_64,
> > > > rather than introduce alternate (and untested!) code paths
> > > throughout the
> > > > driver?
> > > >
> > > No, It cannot be integrated. The div cannot be removed.
> > > Because if do priv->timerfreq /=3D div, that will affect the =20
> accuracy.
> > >
> > > Like:
> > > 3 * 5 / 2 =3D 7;
> > > 3 / 2 * 5 =3D 5;
> >
> > I don't follow -- a change in the clock speed is a change in the =20
> clock
> > speed, no matter how you accomplish it.
> >
> This is not change hardware clock frequency.

Citation needed.  It looks like a change in the timer frequency to me:

   Clock ratio. Specifies the ratio of the timer frequency to the MPIC =20
input clock (platform clock/2) . The
   following clock ratios are supported:
   00
   01
   10
   11
   Default. Divide by 8
   Divide by 16
   Divide by 32
   Divide by 64

The end result is that the counter in the timer register changes only
1/64 as often as the input clock.  There's nothing special about that,
compared to having an input clock that is 1/64 the speed.

> The mpic timer hardware clock is not be changed after initialization. =20
> This is just conversion ticks.
> These calculated ticks will be set to the hardware.
>=20
> > How you round is a different question.  You should probably be =20
> rounding
> > up always, based on the final clock frequency -- though it's =20
> unlikely to
> > matter much given the high precision of the timer relative to the =20
> input
> > granularity.
> >
> Each ticks are based on the mpic timer hardware clock frequency.
> The conversion and calculation are in order to make the tick value is =20
> more
> accurate, more close to real time.
> If echo 40 seconds may be difference is not obvious. But echo =20
> 315360000(10 years)
> difference is obvious.

So basically you're taking advantage of the fact that you have what
appears to be a more precise value of the frequency than is expressible
in integer Hz -- but I think that's false precision; odds are the
frequency is not accurate to 1 Hz to begin with.  Even if it is, I doubt
it's worth worrying about.  The error as a percentage will still be very
small with an input frequency of many MHz.  Does an error of a few
minutes really matter if you're delaying for 10 years?  That's =20
acceptable
clock drift for something not synced to network time.  The main thing is
to ensure that you round up, not down, so that software doesn't see an
early wakeup as measured by its own timers.

BTW, the input clock frequency has been similarly scaled, yet you don't
try to scrounge up that information to get further precision...

-Scott=

RE: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, March 21, 2013 5:49 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Zhao Chenhui-B35336; Li Yang-R58472
> Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
>=20
> On 03/19/2013 10:48:53 PM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, March 20, 2013 6:55 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Zhao Chenhui-B35336; Li Yang-R58472
> > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> > >
> > > On 03/19/2013 01:25:42 AM, Wang Dongsheng-B40534 wrote:
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Tuesday, March 19, 2013 8:31 AM
> > > > > To: Wang Dongsheng-B40534
> > > > > Cc: Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org; Wang
> > > > Dongsheng-
> > > > > B40534; Zhao Chenhui-B35336; Li Yang-R58472
> > > > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup
> > support
> > > > >
> > > > > On 03/08/2013 01:38:47 AM, Wang Dongsheng wrote:
> > > > > > +static ssize_t fsl_timer_wakeup_store(struct device *dev,
> > > > > > +				struct device_attribute *attr,
> > > > > > +				const char *buf,
> > > > > > +				size_t count)
> > > > > > +{
> > > > > > +	struct timeval interval;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	interval.tv_usec =3D 0;
> > > > > > +	if (kstrtol(buf, 0, &interval.tv_sec))
> > > > > > +		return -EINVAL;
> > > > >
> > > > > I don't think the buffer will NUL-terminated...  Ordinarily
> > > > there'll be
> > > > > an LF terminator, but you can't rely on that (many other sysfs
> > > > attributes
> > > > > seem to, though...).
> > > > >
> > > > I think we don't need to care about LF terminator.
> > > > The kstrtol--> _kstrtoull has been done.
> > >
> > > My point is, what happens if userspace passes in a buffer that has
> > no
> > > terminator of any sort?  kstrtol will continue reading beyond the
> > end of
> > > the buffer.
> > >
> > Do not care about terminator.
>=20
> kstrtol() obviously *does* because it doesn't take the buffer length as
> a parameter.
>=20
> > kstrtol--> _kstrtoull--> _parse_integer
> >
> > _kstrtoull(...) {
> > 	...
> > 	rv =3D _parse_integer(s, base, &_res);
> > 	if (rv & KSTRTOX_OVERFLOW)
> > 		return -ERANGE;
> > 	rv &=3D ~KSTRTOX_OVERFLOW;
> > 	if (rv =3D=3D 0)
> > 		return -EINVAL;
> > 	s +=3D rv;
> >
> > 	if (*s =3D=3D '\n')
> > 		s++;
> > 	if (*s)
> > 		return -EINVAL;
> > 	...
> > }
> >
> > _parse_integer(...) {
> > 	...
> > 	while (*s) {
> > 		if ('0' <=3D *s && *s <=3D '9')
> > 			val =3D *s - '0';
> > 		else if ('a' <=3D _tolower(*s) && _tolower(*s) <=3D 'f')
> > 			val =3D _tolower(*s) - 'a' + 10;
> > 		else
> > 			break;	//this will break out to convert.
>=20
> Really?  How do you know that the next byte after the buffer isn't a
> valid hex digit?  How do you even know that we won't take a fault
> accessing it?
>=20
Under what case is unsafe, please make sense.

"kstrtol" is used in almost of sysfs interface, I think it should be accept=
ed in defaule :).

RE: [PATCH 2/3] powerpc/mpic: add global timer support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, March 21, 2013 7:00 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Li Yang-R58472
> Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
>=20
> On 03/20/2013 01:45:03 AM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, March 20, 2013 6:59 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Li Yang-R58472
> > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > >
> > > On 03/19/2013 02:55:58 AM, Wang Dongsheng-B40534 wrote:
> > > > > > +static void convert_ticks_to_time(struct timer_group_priv
> > *priv,
> > > > > > +		const u64 ticks, struct timeval *time) {
> > > > > > +	u64 tmp_sec;
> > > > > > +	u32 rem_us;
> > > > > > +	u32 div;
> > > > > > +
> > > > > > +	if (!(priv->flags & FSL_GLOBAL_TIMER)) {
> > > > > > +		time->tv_sec =3D (__kernel_time_t)
> > > > > > +			div_u64_rem(ticks, priv->timerfreq,
> > &rem_us);
> > > > > > +		tmp_sec =3D (u64)time->tv_sec *
> > (u64)priv->timerfreq;
> > > > > > +		time->tv_usec =3D (__kernel_suseconds_t)
> > > > > > +			div_u64((ticks - tmp_sec) * 1000000,
> > > > > > priv->timerfreq);
> > > > > > +
> > > > > > +		return;
> > > > > > +	}
> > > > > > +
> > > > > > +	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > > > > +
> > > > > > +	time->tv_sec =3D (__kernel_time_t)div_u64(ticks, priv-
> > > >timerfreq
> > > > > > / div);
> > > > > > +	tmp_sec =3D div_u64((u64)time->tv_sec *
> > (u64)priv->timerfreq,
> > > > > > div);
> > > > > > +
> > > > > > +	time->tv_usec =3D (__kernel_suseconds_t)
> > > > > > +		div_u64((ticks - tmp_sec) * 1000000,
> > priv->timerfreq /
> > > > > > div);
> > > > > > +
> > > > > > +	return;
> > > > >
> > > > > Why don't you just adjust the clock frequency up front for
> > > > CLKDIV_64,
> > > > > rather than introduce alternate (and untested!) code paths
> > > > throughout the
> > > > > driver?
> > > > >
> > > > No, It cannot be integrated. The div cannot be removed.
> > > > Because if do priv->timerfreq /=3D div, that will affect the
> > accuracy.
> > > >
> > > > Like:
> > > > 3 * 5 / 2 =3D 7;
> > > > 3 / 2 * 5 =3D 5;
> > >
> > > I don't follow -- a change in the clock speed is a change in the
> > clock
> > > speed, no matter how you accomplish it.
> > >
> > This is not change hardware clock frequency.
>=20
> Citation needed.  It looks like a change in the timer frequency to me:
>=20
>    Clock ratio. Specifies the ratio of the timer frequency to the MPIC
> input clock (platform clock/2) . The
>    following clock ratios are supported:
>    00
>    01
>    10
>    11
>    Default. Divide by 8
>    Divide by 16
>    Divide by 32
>    Divide by 64
>=20
> The end result is that the counter in the timer register changes only
> 1/64 as often as the input clock.  There's nothing special about that,
> compared to having an input clock that is 1/64 the speed.
>=20
> > The mpic timer hardware clock is not be changed after initialization.
> > This is just conversion ticks.
> > These calculated ticks will be set to the hardware.
> >
> > > How you round is a different question.  You should probably be
> > rounding
> > > up always, based on the final clock frequency -- though it's
> > unlikely to
> > > matter much given the high precision of the timer relative to the
> > input
> > > granularity.
> > >
> > Each ticks are based on the mpic timer hardware clock frequency.
> > The conversion and calculation are in order to make the tick value is
> > more
> > accurate, more close to real time.
> > If echo 40 seconds may be difference is not obvious. But echo
> > 315360000(10 years)
> > difference is obvious.
>=20
> So basically you're taking advantage of the fact that you have what
> appears to be a more precise value of the frequency than is expressible
> in integer Hz -- but I think that's false precision; odds are the
> frequency is not accurate to 1 Hz to begin with.  Even if it is, I doubt
> it's worth worrying about.  The error as a percentage will still be very
> small with an input frequency of many MHz.  Does an error of a few
> minutes really matter if you're delaying for 10 years?  That's
> acceptable
> clock drift for something not synced to network time.  The main thing is
> to ensure that you round up, not down, so that software doesn't see an
> early wakeup as measured by its own timers.
>=20
> BTW, the input clock frequency has been similarly scaled, yet you don't
> try to scrounge up that information to get further precision...
>=20
Let's go back patch, do you think the code is repeated?
I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" branch, there will b=
e no redundant code.

Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Scott Wood]

On 03/22/2013 12:46:24 AM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, March 21, 2013 5:49 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Zhao Chenhui-B35336; Li Yang-R58472
> > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> >
> > On 03/19/2013 10:48:53 PM, Wang Dongsheng-B40534 wrote:
> > > 	while (*s) {
> > > 		if ('0' <=3D *s && *s <=3D '9')
> > > 			val =3D *s - '0';
> > > 		else if ('a' <=3D _tolower(*s) && _tolower(*s) <=3D 'f')
> > > 			val =3D _tolower(*s) - 'a' + 10;
> > > 		else
> > > 			break;	//this will break out to convert.
> >
> > Really?  How do you know that the next byte after the buffer isn't a
> > valid hex digit?  How do you even know that we won't take a fault
> > accessing it?
> >
> Under what case is unsafe, please make sense.

char buffer[1] =3D { '5' };
write(fd, &buffer, 1);

What comes after that '5' byte in the pointer you pass to kstrtol?

> "kstrtol" is used in almost of sysfs interface, I think it should be =20
> accepted in defaule :).

Just because a lot of other people copy blindly doesn't make it right.  =20
Most of the examples I found use sscanf instead, though that has the =20
same problem.

I do see a few instances of the "strings from sysfs write are not 0 =20
terminated!" in the comments, though (kernel/time/clocksource.c and =20
kernel/rtmutex-tester.c).

Also "words written to sysfs files may, or may not, be \n terminated" =20
in drivers/md/md.c.

-Scott=

Re: [PATCH 2/3] powerpc/mpic: add global timer support

[Scott Wood]

On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, March 21, 2013 7:00 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Li Yang-R58472
> > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> >
> > BTW, the input clock frequency has been similarly scaled, yet you =20
> don't
> > try to scrounge up that information to get further precision...
> >
> Let's go back patch, do you think the code is repeated?
> I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" branch, there =20
> will be no redundant code.

I'd rather that branch be kept and the more complicated branch deleted, =20
and priv->timerfreq frequency be adjusted on initialization to account =20
for the scaler.

-Scott=

RE: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Saturday, March 23, 2013 6:11 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Zhao Chenhui-B35336; Li Yang-R58472
> Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
>=20
> On 03/22/2013 12:46:24 AM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Thursday, March 21, 2013 5:49 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Zhao Chenhui-B35336; Li Yang-R58472
> > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> > >
> > > On 03/19/2013 10:48:53 PM, Wang Dongsheng-B40534 wrote:
> > > > 	while (*s) {
> > > > 		if ('0' <=3D *s && *s <=3D '9')
> > > > 			val =3D *s - '0';
> > > > 		else if ('a' <=3D _tolower(*s) && _tolower(*s) <=3D 'f')
> > > > 			val =3D _tolower(*s) - 'a' + 10;
> > > > 		else
> > > > 			break;	//this will break out to convert.
> > >
> > > Really?  How do you know that the next byte after the buffer isn't a
> > > valid hex digit?  How do you even know that we won't take a fault
> > > accessing it?
> > >
> > Under what case is unsafe, please make sense.
>=20
> char buffer[1] =3D { '5' };
> write(fd, &buffer, 1);
>=20
> What comes after that '5' byte in the pointer you pass to kstrtol?
>=20
The buffer is userspace. It will fall in the kernel space.
Kernel will get a free page, and copy the buffer to page.
This page has been cleared before copy to page.
The page has already have null-terminated.

> > "kstrtol" is used in almost of sysfs interface, I think it should be
> > accepted in defaule :).
>=20
> Just because a lot of other people copy blindly doesn't make it right.
> Most of the examples I found use sscanf instead, though that has the same
> problem.
>=20
> I do see a few instances of the "strings from sysfs write are not 0
> terminated!" in the comments, though (kernel/time/clocksource.c and
> kernel/rtmutex-tester.c).
>=20
> Also "words written to sysfs files may, or may not, be \n terminated"
> in drivers/md/md.c.
>=20
It's not "kstrtol" doesn't work as well, They do not belong to this kind
of scenarios.

RE: [PATCH 2/3] powerpc/mpic: add global timer support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Saturday, March 23, 2013 6:30 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Li Yang-R58472
> Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
>=20
> On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Thursday, March 21, 2013 7:00 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Li Yang-R58472
> > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > >
> > > BTW, the input clock frequency has been similarly scaled, yet you
> > don't
> > > try to scrounge up that information to get further precision...
> > >
> > Let's go back patch, do you think the code is repeated?
> > I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" branch, there
> > will be no redundant code.
>=20
> I'd rather that branch be kept and the more complicated branch deleted,
> and priv->timerfreq frequency be adjusted on initialization to account
> for the scaler.

static void convert_ticks_to_time(struct timer_group_priv *priv,
                const u64 ticks, struct timeval *time)
{
        u64 tmp_sec;

        time->tv_sec =3D (__kernel_time_t)div_u64(ticks, priv->timerfreq);
        tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;

        time->tv_usec =3D (__kernel_suseconds_t)
                div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq);

        return;
}

timer_group_get_freq() {
	...
	if (priv->flags & FSL_GLOBAL_TIMER) {
		div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
		priv->timerfreq /=3D div;
	}
	...
}
Do you want to do that?

Re: [PATCH 2/3] powerpc/mpic: add global timer support

[Scott Wood]

On 03/25/2013 10:29:58 PM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Saturday, March 23, 2013 6:30 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Li Yang-R58472
> > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> >
> > On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Thursday, March 21, 2013 7:00 AM
> > > > To: Wang Dongsheng-B40534
> > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > linuxppc-dev@lists.ozlabs.org;
> > > > Li Yang-R58472
> > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > > >
> > > > BTW, the input clock frequency has been similarly scaled, yet =20
> you
> > > don't
> > > > try to scrounge up that information to get further precision...
> > > >
> > > Let's go back patch, do you think the code is repeated?
> > > I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" branch, =20
> there
> > > will be no redundant code.
> >
> > I'd rather that branch be kept and the more complicated branch =20
> deleted,
> > and priv->timerfreq frequency be adjusted on initialization to =20
> account
> > for the scaler.
>=20
> static void convert_ticks_to_time(struct timer_group_priv *priv,
>                 const u64 ticks, struct timeval *time)
> {
>         u64 tmp_sec;
>=20
>         time->tv_sec =3D (__kernel_time_t)div_u64(ticks, =20
> priv->timerfreq);
>         tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
>=20
>         time->tv_usec =3D (__kernel_suseconds_t)
>                 div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq);
>=20
>         return;
> }
>=20
> timer_group_get_freq() {
> 	...
> 	if (priv->flags & FSL_GLOBAL_TIMER) {
> 		div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> 		priv->timerfreq /=3D div;
> 	}
> 	...
> }
> Do you want to do that?

	if (priv->flags & FSL_GLOBAL_TIMER)
		priv->timerfreq /=3D 64;

...but otherwise yes.

-Scott=

Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Scott Wood]

On 03/25/2013 10:27:24 PM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Saturday, March 23, 2013 6:11 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Zhao Chenhui-B35336; Li Yang-R58472
> > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> >
> > On 03/22/2013 12:46:24 AM, Wang Dongsheng-B40534 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Thursday, March 21, 2013 5:49 AM
> > > > To: Wang Dongsheng-B40534
> > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > linuxppc-dev@lists.ozlabs.org;
> > > > Zhao Chenhui-B35336; Li Yang-R58472
> > > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup =20
> support
> > > >
> > > > On 03/19/2013 10:48:53 PM, Wang Dongsheng-B40534 wrote:
> > > > > 	while (*s) {
> > > > > 		if ('0' <=3D *s && *s <=3D '9')
> > > > > 			val =3D *s - '0';
> > > > > 		else if ('a' <=3D _tolower(*s) && _tolower(*s) <=3D =20
> 'f')
> > > > > 			val =3D _tolower(*s) - 'a' + 10;
> > > > > 		else
> > > > > 			break;	//this will break out to =20
> convert.
> > > >
> > > > Really?  How do you know that the next byte after the buffer =20
> isn't a
> > > > valid hex digit?  How do you even know that we won't take a =20
> fault
> > > > accessing it?
> > > >
> > > Under what case is unsafe, please make sense.
> >
> > char buffer[1] =3D { '5' };
> > write(fd, &buffer, 1);
> >
> > What comes after that '5' byte in the pointer you pass to kstrtol?
> >
> The buffer is userspace. It will fall in the kernel space.
> Kernel will get a free page, and copy the buffer to page.
> This page has been cleared before copy to page.
> The page has already have null-terminated.

It doesn't allocate a whole page, it uses kmalloc (not kzalloc!).  Even =20
if kzalloc were used, a larger user buffer could be the exact size of =20
the region that was allocated.

See memdup_user() in mm/util.c

-Scott=

RE: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, March 27, 2013 1:36 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Zhao Chenhui-B35336; Li Yang-R58472
> Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
>=20
> On 03/25/2013 10:27:24 PM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Saturday, March 23, 2013 6:11 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Zhao Chenhui-B35336; Li Yang-R58472
> > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> > >
> > > On 03/22/2013 12:46:24 AM, Wang Dongsheng-B40534 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Thursday, March 21, 2013 5:49 AM
> > > > > To: Wang Dongsheng-B40534
> > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > linuxppc-dev@lists.ozlabs.org;
> > > > > Zhao Chenhui-B35336; Li Yang-R58472
> > > > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup
> > support
> > > > >
> > > > > On 03/19/2013 10:48:53 PM, Wang Dongsheng-B40534 wrote:
> > > > > > 	while (*s) {
> > > > > > 		if ('0' <=3D *s && *s <=3D '9')
> > > > > > 			val =3D *s - '0';
> > > > > > 		else if ('a' <=3D _tolower(*s) && _tolower(*s) <=3D
> > 'f')
> > > > > > 			val =3D _tolower(*s) - 'a' + 10;
> > > > > > 		else
> > > > > > 			break;	//this will break out to
> > convert.
> > > > >
> > > > > Really?  How do you know that the next byte after the buffer
> > isn't a
> > > > > valid hex digit?  How do you even know that we won't take a
> > fault
> > > > > accessing it?
> > > > >
> > > > Under what case is unsafe, please make sense.
> > >
> > > char buffer[1] =3D { '5' };
> > > write(fd, &buffer, 1);
> > >
> > > What comes after that '5' byte in the pointer you pass to kstrtol?
> > >
> > The buffer is userspace. It will fall in the kernel space.
> > Kernel will get a free page, and copy the buffer to page.
> > This page has been cleared before copy to page.
> > The page has already have null-terminated.
>=20
> It doesn't allocate a whole page, it uses kmalloc (not kzalloc!).  Even
> if kzalloc were used, a larger user buffer could be the exact size of the
> region that was allocated.
>=20
> See memdup_user() in mm/util.c
>=20
Did you miss something?

See fill_write_buffer() in fs/sysfs/file.c. It's used get_zeroed_page()...

See SYSCALL_DEFINE3(write,...) in fs/read_write.c

[c0000000f1ff3a60] [c000000000008224] .show_stack+0x74/0x1b0 (unreliable)
[c0000000f1ff3b10] [c00000000002f370] .fsl_timer_wakeup_store+0x30/0x200
[c0000000f1ff3bc0] [c00000000030accc] .dev_attr_store+0x3c/0x50
[c0000000f1ff3c30] [c00000000018c47c] .sysfs_write_file+0xec/0x1f0
[c0000000f1ff3ce0] [c00000000010dfb4] .vfs_write+0xf4/0x1b0
[c0000000f1ff3d80] [c00000000010e360] .SyS_write+0x60/0xe0
[c0000000f1ff3e30] [c000000000000590] syscall_exit+0x0/0x80

RE: [PATCH 2/3] powerpc/mpic: add global timer support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, March 27, 2013 1:32 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Li Yang-R58472
> Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
>=20
> On 03/25/2013 10:29:58 PM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Saturday, March 23, 2013 6:30 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Li Yang-R58472
> > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > >
> > > On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Thursday, March 21, 2013 7:00 AM
> > > > > To: Wang Dongsheng-B40534
> > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > linuxppc-dev@lists.ozlabs.org;
> > > > > Li Yang-R58472
> > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > > > >
> > > > > BTW, the input clock frequency has been similarly scaled, yet
> > you
> > > > don't
> > > > > try to scrounge up that information to get further precision...
> > > > >
> > > > Let's go back patch, do you think the code is repeated?
> > > > I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" branch,
> > there
> > > > will be no redundant code.
> > >
> > > I'd rather that branch be kept and the more complicated branch
> > deleted,
> > > and priv->timerfreq frequency be adjusted on initialization to
> > account
> > > for the scaler.
> >
> > static void convert_ticks_to_time(struct timer_group_priv *priv,
> >                 const u64 ticks, struct timeval *time) {
> >         u64 tmp_sec;
> >
> >         time->tv_sec =3D (__kernel_time_t)div_u64(ticks,
> > priv->timerfreq);
> >         tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> >
> >         time->tv_usec =3D (__kernel_suseconds_t)
> >                 div_u64((ticks - tmp_sec) * 1000000, priv->timerfreq);
> >
> >         return;
> > }
> >
> > timer_group_get_freq() {
> > 	...
> > 	if (priv->flags & FSL_GLOBAL_TIMER) {
> > 		div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > 		priv->timerfreq /=3D div;
> > 	}
> > 	...
> > }
> > Do you want to do that?
>=20
> 	if (priv->flags & FSL_GLOBAL_TIMER)
> 		priv->timerfreq /=3D 64;
>=20
> ...but otherwise yes.
Ok, I would like do this.

if (priv->flags & FSL_GLOBAL_TIMER) {
	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
	priv->timerfreq /=3D div;
}

Re: [PATCH 2/3] powerpc/mpic: add global timer support

[Scott Wood]

On 03/26/2013 10:23:38 PM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, March 27, 2013 1:32 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Li Yang-R58472
> > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> >
> > On 03/25/2013 10:29:58 PM, Wang Dongsheng-B40534 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Saturday, March 23, 2013 6:30 AM
> > > > To: Wang Dongsheng-B40534
> > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > linuxppc-dev@lists.ozlabs.org;
> > > > Li Yang-R58472
> > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > > >
> > > > On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Thursday, March 21, 2013 7:00 AM
> > > > > > To: Wang Dongsheng-B40534
> > > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > Li Yang-R58472
> > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer =20
> support
> > > > > >
> > > > > > BTW, the input clock frequency has been similarly scaled, =20
> yet
> > > you
> > > > > don't
> > > > > > try to scrounge up that information to get further =20
> precision...
> > > > > >
> > > > > Let's go back patch, do you think the code is repeated?
> > > > > I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" branch,
> > > there
> > > > > will be no redundant code.
> > > >
> > > > I'd rather that branch be kept and the more complicated branch
> > > deleted,
> > > > and priv->timerfreq frequency be adjusted on initialization to
> > > account
> > > > for the scaler.
> > >
> > > static void convert_ticks_to_time(struct timer_group_priv *priv,
> > >                 const u64 ticks, struct timeval *time) {
> > >         u64 tmp_sec;
> > >
> > >         time->tv_sec =3D (__kernel_time_t)div_u64(ticks,
> > > priv->timerfreq);
> > >         tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> > >
> > >         time->tv_usec =3D (__kernel_suseconds_t)
> > >                 div_u64((ticks - tmp_sec) * 1000000, =20
> priv->timerfreq);
> > >
> > >         return;
> > > }
> > >
> > > timer_group_get_freq() {
> > > 	...
> > > 	if (priv->flags & FSL_GLOBAL_TIMER) {
> > > 		div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > 		priv->timerfreq /=3D div;
> > > 	}
> > > 	...
> > > }
> > > Do you want to do that?
> >
> > 	if (priv->flags & FSL_GLOBAL_TIMER)
> > 		priv->timerfreq /=3D 64;
> >
> > ...but otherwise yes.
> Ok, I would like do this.
>=20
> if (priv->flags & FSL_GLOBAL_TIMER) {
> 	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> 	priv->timerfreq /=3D div;

Why?  What do you get out of that obfuscation?

-Scott=

Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Scott Wood]

On 03/26/2013 10:21:04 PM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, March 27, 2013 1:36 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Zhao Chenhui-B35336; Li Yang-R58472
> > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> >
> > On 03/25/2013 10:27:24 PM, Wang Dongsheng-B40534 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Saturday, March 23, 2013 6:11 AM
> > > > To: Wang Dongsheng-B40534
> > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > linuxppc-dev@lists.ozlabs.org;
> > > > Zhao Chenhui-B35336; Li Yang-R58472
> > > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup =20
> support
> > > >
> > > > On 03/22/2013 12:46:24 AM, Wang Dongsheng-B40534 wrote:
> > > > > Under what case is unsafe, please make sense.
> > > >
> > > > char buffer[1] =3D { '5' };
> > > > write(fd, &buffer, 1);
> > > >
> > > > What comes after that '5' byte in the pointer you pass to =20
> kstrtol?
> > > >
> > > The buffer is userspace. It will fall in the kernel space.
> > > Kernel will get a free page, and copy the buffer to page.
> > > This page has been cleared before copy to page.
> > > The page has already have null-terminated.
> >
> > It doesn't allocate a whole page, it uses kmalloc (not kzalloc!).  =20
> Even
> > if kzalloc were used, a larger user buffer could be the exact size =20
> of the
> > region that was allocated.
> >
> > See memdup_user() in mm/util.c
> >
> Did you miss something?
> See fill_write_buffer() in fs/sysfs/file.c. It's used =20
> get_zeroed_page()...

OK, I was looking at fs/sysfs/bin.c which is something slightly =20
different.

fill_write_buffer() forces the size to be no more than "PAGE_SIZE - 1" =20
so we know there's a terminator.

Perhaps kernel/rtmutex-tester.c and kernel/time/clocksource.c are =20
similarly confused?

-Scott=

RE: [PATCH 2/3] powerpc/mpic: add global timer support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, March 28, 2013 1:12 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Li Yang-R58472
> Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
>=20
> On 03/26/2013 10:23:38 PM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, March 27, 2013 1:32 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Li Yang-R58472
> > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > >
> > > On 03/25/2013 10:29:58 PM, Wang Dongsheng-B40534 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Saturday, March 23, 2013 6:30 AM
> > > > > To: Wang Dongsheng-B40534
> > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > linuxppc-dev@lists.ozlabs.org;
> > > > > Li Yang-R58472
> > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > > > >
> > > > > On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wood Scott-B07421
> > > > > > > Sent: Thursday, March 21, 2013 7:00 AM
> > > > > > > To: Wang Dongsheng-B40534
> > > > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > > Li Yang-R58472
> > > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer
> > support
> > > > > > >
> > > > > > > BTW, the input clock frequency has been similarly scaled,
> > yet
> > > > you
> > > > > > don't
> > > > > > > try to scrounge up that information to get further
> > precision...
> > > > > > >
> > > > > > Let's go back patch, do you think the code is repeated?
> > > > > > I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" branch,
> > > > there
> > > > > > will be no redundant code.
> > > > >
> > > > > I'd rather that branch be kept and the more complicated branch
> > > > deleted,
> > > > > and priv->timerfreq frequency be adjusted on initialization to
> > > > account
> > > > > for the scaler.
> > > >
> > > > static void convert_ticks_to_time(struct timer_group_priv *priv,
> > > >                 const u64 ticks, struct timeval *time) {
> > > >         u64 tmp_sec;
> > > >
> > > >         time->tv_sec =3D (__kernel_time_t)div_u64(ticks,
> > > > priv->timerfreq);
> > > >         tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> > > >
> > > >         time->tv_usec =3D (__kernel_suseconds_t)
> > > >                 div_u64((ticks - tmp_sec) * 1000000,
> > priv->timerfreq);
> > > >
> > > >         return;
> > > > }
> > > >
> > > > timer_group_get_freq() {
> > > > 	...
> > > > 	if (priv->flags & FSL_GLOBAL_TIMER) {
> > > > 		div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > > 		priv->timerfreq /=3D div;
> > > > 	}
> > > > 	...
> > > > }
> > > > Do you want to do that?
> > >
> > > 	if (priv->flags & FSL_GLOBAL_TIMER)
> > > 		priv->timerfreq /=3D 64;
> > >
> > > ...but otherwise yes.
> > Ok, I would like do this.
> >
> > if (priv->flags & FSL_GLOBAL_TIMER) {
> > 	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > 	priv->timerfreq /=3D div;
>=20
> Why?  What do you get out of that obfuscation?
>=20
Change MPIC_TIMER_TCR_CLKDIV_64 to MPIC_TIMER_TCR_CLKDIV

/* Clock Ratio
 * Divide by 64 0x00000300
 * Divide by 32 0x00000200
 * Divide by 16 0x00000100
 * Divide by  8 0x00000000 (Hardware default div)
 */
#define MPIC_TIMER_TCR_CLKDIV 0x00000300

timer_group_init() {
	...
	/* Init FSL timer hardware */
      if (priv->flags & FSL_GLOBAL_TIMER)
      	setbits32(priv->group_tcr, MPIC_TIMER_TCR_CLKDIV);
	...
}

mpic_timer_resume() {
	...
	/* Init FSL timer hardware */
      if (priv->flags & FSL_GLOBAL_TIMER)
      	setbits32(priv->group_tcr, MPIC_TIMER_TCR_CLKDIV);
	...
}

Because macro is friendly, and other functions also used the macro.

RE: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, March 28, 2013 4:26 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Zhao Chenhui-B35336; Li Yang-R58472
> Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
>=20
> On 03/26/2013 10:21:04 PM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Wednesday, March 27, 2013 1:36 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Zhao Chenhui-B35336; Li Yang-R58472
> > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup support
> > >
> > > On 03/25/2013 10:27:24 PM, Wang Dongsheng-B40534 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Saturday, March 23, 2013 6:11 AM
> > > > > To: Wang Dongsheng-B40534
> > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > linuxppc-dev@lists.ozlabs.org;
> > > > > Zhao Chenhui-B35336; Li Yang-R58472
> > > > > Subject: Re: [PATCH 3/3] powerpc/fsl: add MPIC timer wakeup
> > support
> > > > >
> > > > > On 03/22/2013 12:46:24 AM, Wang Dongsheng-B40534 wrote:
> > > > > > Under what case is unsafe, please make sense.
> > > > >
> > > > > char buffer[1] =3D { '5' };
> > > > > write(fd, &buffer, 1);
> > > > >
> > > > > What comes after that '5' byte in the pointer you pass to
> > kstrtol?
> > > > >
> > > > The buffer is userspace. It will fall in the kernel space.
> > > > Kernel will get a free page, and copy the buffer to page.
> > > > This page has been cleared before copy to page.
> > > > The page has already have null-terminated.
> > >
> > > It doesn't allocate a whole page, it uses kmalloc (not kzalloc!).
> > Even
> > > if kzalloc were used, a larger user buffer could be the exact size
> > of the
> > > region that was allocated.
> > >
> > > See memdup_user() in mm/util.c
> > >
> > Did you miss something?
> > See fill_write_buffer() in fs/sysfs/file.c. It's used
> > get_zeroed_page()...
>=20
> OK, I was looking at fs/sysfs/bin.c which is something slightly different=
.
>=20
> fill_write_buffer() forces the size to be no more than "PAGE_SIZE - 1"
> so we know there's a terminator.
>=20
> Perhaps kernel/rtmutex-tester.c and kernel/time/clocksource.c are
> similarly confused?
>=20
Yes. But its depends on file->f_op.
See vfs_write in fs/read_write.c.

Re: [PATCH 2/3] powerpc/mpic: add global timer support

[Scott Wood]

On 03/27/2013 09:29:26 PM, Wang Dongsheng-B40534 wrote:
>=20
>=20
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, March 28, 2013 1:12 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Gala Kumar-B11780; =20
> linuxppc-dev@lists.ozlabs.org;
> > Li Yang-R58472
> > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> >
> > On 03/26/2013 10:23:38 PM, Wang Dongsheng-B40534 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, March 27, 2013 1:32 AM
> > > > To: Wang Dongsheng-B40534
> > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > linuxppc-dev@lists.ozlabs.org;
> > > > Li Yang-R58472
> > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > > >
> > > > On 03/25/2013 10:29:58 PM, Wang Dongsheng-B40534 wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Saturday, March 23, 2013 6:30 AM
> > > > > > To: Wang Dongsheng-B40534
> > > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > Li Yang-R58472
> > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer =20
> support
> > > > > >
> > > > > > On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Wood Scott-B07421
> > > > > > > > Sent: Thursday, March 21, 2013 7:00 AM
> > > > > > > > To: Wang Dongsheng-B40534
> > > > > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > > > Li Yang-R58472
> > > > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer
> > > support
> > > > > > > >
> > > > > > > > BTW, the input clock frequency has been similarly =20
> scaled,
> > > yet
> > > > > you
> > > > > > > don't
> > > > > > > > try to scrounge up that information to get further
> > > precision...
> > > > > > > >
> > > > > > > Let's go back patch, do you think the code is repeated?
> > > > > > > I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))" =20
> branch,
> > > > > there
> > > > > > > will be no redundant code.
> > > > > >
> > > > > > I'd rather that branch be kept and the more complicated =20
> branch
> > > > > deleted,
> > > > > > and priv->timerfreq frequency be adjusted on initialization =20
> to
> > > > > account
> > > > > > for the scaler.
> > > > >
> > > > > static void convert_ticks_to_time(struct timer_group_priv =20
> *priv,
> > > > >                 const u64 ticks, struct timeval *time) {
> > > > >         u64 tmp_sec;
> > > > >
> > > > >         time->tv_sec =3D (__kernel_time_t)div_u64(ticks,
> > > > > priv->timerfreq);
> > > > >         tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> > > > >
> > > > >         time->tv_usec =3D (__kernel_suseconds_t)
> > > > >                 div_u64((ticks - tmp_sec) * 1000000,
> > > priv->timerfreq);
> > > > >
> > > > >         return;
> > > > > }
> > > > >
> > > > > timer_group_get_freq() {
> > > > > 	...
> > > > > 	if (priv->flags & FSL_GLOBAL_TIMER) {
> > > > > 		div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * =20
> 8;
> > > > > 		priv->timerfreq /=3D div;
> > > > > 	}
> > > > > 	...
> > > > > }
> > > > > Do you want to do that?
> > > >
> > > > 	if (priv->flags & FSL_GLOBAL_TIMER)
> > > > 		priv->timerfreq /=3D 64;
> > > >
> > > > ...but otherwise yes.
> > > Ok, I would like do this.
> > >
> > > if (priv->flags & FSL_GLOBAL_TIMER) {
> > > 	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > 	priv->timerfreq /=3D div;
> >
> > Why?  What do you get out of that obfuscation?
> >
> Change MPIC_TIMER_TCR_CLKDIV_64 to MPIC_TIMER_TCR_CLKDIV

OK, that would at least provide the ability to adjust the clock divider =20
in one place rather than two -- though I don't know why we'd ever need =20
to change the divider.

> Because macro is friendly, and other functions also used the macro.

Using the macro rather than hardcoding register bit encodings is =20
friendly.  The calculation to turn the bit encoding into an actual =20
divider value is not particularly friendly.

-Scott=

RE: [PATCH 2/3] powerpc/mpic: add global timer support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, March 29, 2013 3:48 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; Gala Kumar-B11780; linuxppc-dev@lists.ozlabs.org;
> Li Yang-R58472
> Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
>=20
> On 03/27/2013 09:29:26 PM, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Thursday, March 28, 2013 1:12 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > linuxppc-dev@lists.ozlabs.org;
> > > Li Yang-R58472
> > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > >
> > > On 03/26/2013 10:23:38 PM, Wang Dongsheng-B40534 wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Wednesday, March 27, 2013 1:32 AM
> > > > > To: Wang Dongsheng-B40534
> > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > linuxppc-dev@lists.ozlabs.org;
> > > > > Li Yang-R58472
> > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer support
> > > > >
> > > > > On 03/25/2013 10:29:58 PM, Wang Dongsheng-B40534 wrote:
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wood Scott-B07421
> > > > > > > Sent: Saturday, March 23, 2013 6:30 AM
> > > > > > > To: Wang Dongsheng-B40534
> > > > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > > Li Yang-R58472
> > > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer
> > support
> > > > > > >
> > > > > > > On 03/22/2013 01:14:51 AM, Wang Dongsheng-B40534 wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Wood Scott-B07421
> > > > > > > > > Sent: Thursday, March 21, 2013 7:00 AM
> > > > > > > > > To: Wang Dongsheng-B40534
> > > > > > > > > Cc: Wood Scott-B07421; Gala Kumar-B11780;
> > > > > > > > linuxppc-dev@lists.ozlabs.org;
> > > > > > > > > Li Yang-R58472
> > > > > > > > > Subject: Re: [PATCH 2/3] powerpc/mpic: add global timer
> > > > support
> > > > > > > > >
> > > > > > > > > BTW, the input clock frequency has been similarly
> > scaled,
> > > > yet
> > > > > > you
> > > > > > > > don't
> > > > > > > > > try to scrounge up that information to get further
> > > > precision...
> > > > > > > > >
> > > > > > > > Let's go back patch, do you think the code is repeated?
> > > > > > > > I will remove "if (!(priv->flags & FSL_GLOBAL_TIMER))"
> > branch,
> > > > > > there
> > > > > > > > will be no redundant code.
> > > > > > >
> > > > > > > I'd rather that branch be kept and the more complicated
> > branch
> > > > > > deleted,
> > > > > > > and priv->timerfreq frequency be adjusted on initialization
> > to
> > > > > > account
> > > > > > > for the scaler.
> > > > > >
> > > > > > static void convert_ticks_to_time(struct timer_group_priv
> > *priv,
> > > > > >                 const u64 ticks, struct timeval *time) {
> > > > > >         u64 tmp_sec;
> > > > > >
> > > > > >         time->tv_sec =3D (__kernel_time_t)div_u64(ticks,
> > > > > > priv->timerfreq);
> > > > > >         tmp_sec =3D (u64)time->tv_sec * (u64)priv->timerfreq;
> > > > > >
> > > > > >         time->tv_usec =3D (__kernel_suseconds_t)
> > > > > >                 div_u64((ticks - tmp_sec) * 1000000,
> > > > priv->timerfreq);
> > > > > >
> > > > > >         return;
> > > > > > }
> > > > > >
> > > > > > timer_group_get_freq() {
> > > > > > 	...
> > > > > > 	if (priv->flags & FSL_GLOBAL_TIMER) {
> > > > > > 		div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) *
> > 8;
> > > > > > 		priv->timerfreq /=3D div;
> > > > > > 	}
> > > > > > 	...
> > > > > > }
> > > > > > Do you want to do that?
> > > > >
> > > > > 	if (priv->flags & FSL_GLOBAL_TIMER)
> > > > > 		priv->timerfreq /=3D 64;
> > > > >
> > > > > ...but otherwise yes.
> > > > Ok, I would like do this.
> > > >
> > > > if (priv->flags & FSL_GLOBAL_TIMER) {
> > > > 	div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8;
> > > > 	priv->timerfreq /=3D div;
> > >
> > > Why?  What do you get out of that obfuscation?
> > >
> > Change MPIC_TIMER_TCR_CLKDIV_64 to MPIC_TIMER_TCR_CLKDIV
>=20
> OK, that would at least provide the ability to adjust the clock divider
> in one place rather than two -- though I don't know why we'd ever need to
> change the divider.
>=20
Thanks, next patch MPIC_TIMER_TCR_CLKDIV_64 will change to MPIC_TIMER_TCR_C=
LKDIV. :)

div =3D (1 << (MPIC_TIMER_TCR_CLKDIV_64 >> 8)) * 8 --->> div =3D (1 << (MPI=
C_TIMER_TCR_CLKDIV >> 8)) * 8;