[PATCH v2 0/5] clocksource: sun5i: Support parent clock rate changes

[PATCH v2 0/5] clocksource: sun5i: Support parent clock rate changes

[Maxime Ripard]

Hi,

The Allwinner HS timers have the AHB clock as their parent
clock. Since this clock is shared with other devices, we could very
well have another driver requesting a rate change of that clock,
making our timer change frequency at the same time.

This is especially true on the A31, where the DMA controller needs to
do such a rate change, making the HS timer unreliable at the time on
the A31.

This serie makes some cleanups and implements clock notifiers to be
able to reflect such rate changes and make sure that the timer is
always working.

Maxime

Changes from v1:
  - Changed the interrupt name to its previous value

Maxime Ripard (5):
  clocksource: sun5i: Switch to request_irq
  clocksource: sun5i: Use of_io_request_and_map
  clocksource: sun5i: Remove sched_clock
  clocksource: sun5i: Refactor the current code
  clocksource: sun5i: Add clock notifiers

 drivers/clocksource/timer-sun5i.c | 306 +++++++++++++++++++++++++++++---------
 1 file changed, 232 insertions(+), 74 deletions(-)

-- 
2.2.2

[PATCH v2 0/5] clocksource: sun5i: Support parent clock rate changes

[Maxime Ripard]

Hi,

The Allwinner HS timers have the AHB clock as their parent
clock. Since this clock is shared with other devices, we could very
well have another driver requesting a rate change of that clock,
making our timer change frequency at the same time.

This is especially true on the A31, where the DMA controller needs to
do such a rate change, making the HS timer unreliable at the time on
the A31.

This serie makes some cleanups and implements clock notifiers to be
able to reflect such rate changes and make sure that the timer is
always working.

Maxime

Changes from v1:
  - Changed the interrupt name to its previous value

Maxime Ripard (5):
  clocksource: sun5i: Switch to request_irq
  clocksource: sun5i: Use of_io_request_and_map
  clocksource: sun5i: Remove sched_clock
  clocksource: sun5i: Refactor the current code
  clocksource: sun5i: Add clock notifiers

 drivers/clocksource/timer-sun5i.c | 306 +++++++++++++++++++++++++++++---------
 1 file changed, 232 insertions(+), 74 deletions(-)

-- 
2.2.2

[PATCH v2 1/5] clocksource: sun5i: Switch to request_irq

[Maxime Ripard]

The current code uses setup_irq, while it could perfectly use the much simpler
request_irq. Switch to that.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 02268448dc85..836f92ac0ffd 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -130,13 +130,6 @@ static irqreturn_t sun5i_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static struct irqaction sun5i_timer_irq = {
-	.name = "sun5i_timer0",
-	.flags = IRQF_TIMER | IRQF_IRQPOLL,
-	.handler = sun5i_timer_interrupt,
-	.dev_id = &sun5i_clockevent,
-};
-
 static u64 sun5i_timer_sched_read(void)
 {
 	return ~readl(timer_base + TIMER_CNTVAL_LO_REG(1));
@@ -178,7 +171,8 @@ static void __init sun5i_timer_init(struct device_node *node)
 
 	ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
 
-	ret = setup_irq(irq, &sun5i_timer_irq);
+	ret = request_irq(irq, sun5i_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
+			  "sun5i_timer0", &sun5i_clockevent);
 	if (ret)
 		pr_warn("failed to setup irq %d\n", irq);
 
-- 
2.2.2

[PATCH v2 1/5] clocksource: sun5i: Switch to request_irq

[Maxime Ripard]

The current code uses setup_irq, while it could perfectly use the much simpler
request_irq. Switch to that.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 02268448dc85..836f92ac0ffd 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -130,13 +130,6 @@ static irqreturn_t sun5i_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static struct irqaction sun5i_timer_irq = {
-	.name = "sun5i_timer0",
-	.flags = IRQF_TIMER | IRQF_IRQPOLL,
-	.handler = sun5i_timer_interrupt,
-	.dev_id = &sun5i_clockevent,
-};
-
 static u64 sun5i_timer_sched_read(void)
 {
 	return ~readl(timer_base + TIMER_CNTVAL_LO_REG(1));
@@ -178,7 +171,8 @@ static void __init sun5i_timer_init(struct device_node *node)
 
 	ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
 
-	ret = setup_irq(irq, &sun5i_timer_irq);
+	ret = request_irq(irq, sun5i_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
+			  "sun5i_timer0", &sun5i_clockevent);
 	if (ret)
 		pr_warn("failed to setup irq %d\n", irq);
 
-- 
2.2.2

[PATCH v2 2/5] clocksource: sun5i: Use of_io_request_and_map

[Maxime Ripard]

of_iomap doesn't do a request_mem_region on the memory area defined in the DT
it maps. Switch to of_io_request_and_map to make sure we're the only users.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 836f92ac0ffd..2f70ed528174 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -143,7 +143,8 @@ static void __init sun5i_timer_init(struct device_node *node)
 	int ret, irq;
 	u32 val;
 
-	timer_base = of_iomap(node, 0);
+	timer_base = of_io_request_and_map(node, 0,
+					   of_node_full_name(node));
 	if (!timer_base)
 		panic("Can't map registers");
 
-- 
2.2.2

[PATCH v2 2/5] clocksource: sun5i: Use of_io_request_and_map

[Maxime Ripard]

of_iomap doesn't do a request_mem_region on the memory area defined in the DT
it maps. Switch to of_io_request_and_map to make sure we're the only users.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 836f92ac0ffd..2f70ed528174 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -143,7 +143,8 @@ static void __init sun5i_timer_init(struct device_node *node)
 	int ret, irq;
 	u32 val;
 
-	timer_base = of_iomap(node, 0);
+	timer_base = of_io_request_and_map(node, 0,
+					   of_node_full_name(node));
 	if (!timer_base)
 		panic("Can't map registers");
 
-- 
2.2.2

[PATCH v2 3/5] clocksource: sun5i: Remove sched_clock

[Maxime Ripard]

It's not possible to remove a sched_clock once it has been added, nor is it
possible to change its rate.

Since we will need to support a rate change, and that we have other
sched_clocks in the system anyway, remove it.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 2f70ed528174..295bb5e1010c 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -130,11 +130,6 @@ static irqreturn_t sun5i_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static u64 sun5i_timer_sched_read(void)
-{
-	return ~readl(timer_base + TIMER_CNTVAL_LO_REG(1));
-}
-
 static void __init sun5i_timer_init(struct device_node *node)
 {
 	struct reset_control *rstc;
@@ -166,7 +161,6 @@ static void __init sun5i_timer_init(struct device_node *node)
 	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
 	       timer_base + TIMER_CTL_REG(1));
 
-	sched_clock_register(sun5i_timer_sched_read, 32, rate);
 	clocksource_mmio_init(timer_base + TIMER_CNTVAL_LO_REG(1), node->name,
 			      rate, 340, 32, clocksource_mmio_readl_down);
 
-- 
2.2.2

[PATCH v2 3/5] clocksource: sun5i: Remove sched_clock

[Maxime Ripard]

It's not possible to remove a sched_clock once it has been added, nor is it
possible to change its rate.

Since we will need to support a rate change, and that we have other
sched_clocks in the system anyway, remove it.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 2f70ed528174..295bb5e1010c 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -130,11 +130,6 @@ static irqreturn_t sun5i_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static u64 sun5i_timer_sched_read(void)
-{
-	return ~readl(timer_base + TIMER_CNTVAL_LO_REG(1));
-}
-
 static void __init sun5i_timer_init(struct device_node *node)
 {
 	struct reset_control *rstc;
@@ -166,7 +161,6 @@ static void __init sun5i_timer_init(struct device_node *node)
 	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
 	       timer_base + TIMER_CTL_REG(1));
 
-	sched_clock_register(sun5i_timer_sched_read, 32, rate);
 	clocksource_mmio_init(timer_base + TIMER_CNTVAL_LO_REG(1), node->name,
 			      rate, 340, 32, clocksource_mmio_readl_down);
 
-- 
2.2.2

[PATCH v2 4/5] clocksource: sun5i: Refactor the current code

[Maxime Ripard]

Refactor the code in order to remove the global variables and split the clock
source and clock events registration in order to ease the addition of the clock
notifiers needed to handle the parent clock rate changes.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 231 +++++++++++++++++++++++++++-----------
 1 file changed, 166 insertions(+), 65 deletions(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 295bb5e1010c..377e50450781 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -18,6 +18,7 @@
 #include <linux/irqreturn.h>
 #include <linux/reset.h>
 #include <linux/sched_clock.h>
+#include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -37,8 +38,27 @@
 
 #define TIMER_SYNC_TICKS	3
 
-static void __iomem *timer_base;
-static u32 ticks_per_jiffy;
+struct sun5i_timer {
+	void __iomem		*base;
+	struct clk		*clk;
+	u32			ticks_per_jiffy;
+};
+
+struct sun5i_timer_clksrc {
+	struct sun5i_timer	timer;
+	struct clocksource	clksrc;
+};
+
+#define to_sun5i_timer_clksrc(x) \
+	container_of(x, struct sun5i_timer_clksrc, clksrc)
+
+struct sun5i_timer_clkevt {
+	struct sun5i_timer		timer;
+	struct clock_event_device	clkevt;
+};
+
+#define to_sun5i_timer_clkevt(x) \
+	container_of(x, struct sun5i_timer_clkevt, clkevt)
 
 /*
  * When we disable a timer, we need to wait at least for 2 cycles of
@@ -46,30 +66,30 @@ static u32 ticks_per_jiffy;
  * that is already setup and runs at the same frequency than the other
  * timers, and we never will be disabled.
  */
-static void sun5i_clkevt_sync(void)
+static void sun5i_clkevt_sync(struct sun5i_timer_clkevt *ce)
 {
-	u32 old = readl(timer_base + TIMER_CNTVAL_LO_REG(1));
+	u32 old = readl(ce->timer.base + TIMER_CNTVAL_LO_REG(1));
 
-	while ((old - readl(timer_base + TIMER_CNTVAL_LO_REG(1))) < TIMER_SYNC_TICKS)
+	while ((old - readl(ce->timer.base + TIMER_CNTVAL_LO_REG(1))) < TIMER_SYNC_TICKS)
 		cpu_relax();
 }
 
-static void sun5i_clkevt_time_stop(u8 timer)
+static void sun5i_clkevt_time_stop(struct sun5i_timer_clkevt *ce, u8 timer)
 {
-	u32 val = readl(timer_base + TIMER_CTL_REG(timer));
-	writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(timer));
+	u32 val = readl(ce->timer.base + TIMER_CTL_REG(timer));
+	writel(val & ~TIMER_CTL_ENABLE, ce->timer.base + TIMER_CTL_REG(timer));
 
-	sun5i_clkevt_sync();
+	sun5i_clkevt_sync(ce);
 }
 
-static void sun5i_clkevt_time_setup(u8 timer, u32 delay)
+static void sun5i_clkevt_time_setup(struct sun5i_timer_clkevt *ce, u8 timer, u32 delay)
 {
-	writel(delay, timer_base + TIMER_INTVAL_LO_REG(timer));
+	writel(delay, ce->timer.base + TIMER_INTVAL_LO_REG(timer));
 }
 
-static void sun5i_clkevt_time_start(u8 timer, bool periodic)
+static void sun5i_clkevt_time_start(struct sun5i_timer_clkevt *ce, u8 timer, bool periodic)
 {
-	u32 val = readl(timer_base + TIMER_CTL_REG(timer));
+	u32 val = readl(ce->timer.base + TIMER_CTL_REG(timer));
 
 	if (periodic)
 		val &= ~TIMER_CTL_ONESHOT;
@@ -77,66 +97,170 @@ static void sun5i_clkevt_time_start(u8 timer, bool periodic)
 		val |= TIMER_CTL_ONESHOT;
 
 	writel(val | TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
-	       timer_base + TIMER_CTL_REG(timer));
+	       ce->timer.base + TIMER_CTL_REG(timer));
 }
 
 static void sun5i_clkevt_mode(enum clock_event_mode mode,
-			      struct clock_event_device *clk)
+			      struct clock_event_device *clkevt)
 {
+	struct sun5i_timer_clkevt *ce = to_sun5i_timer_clkevt(clkevt);
+
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		sun5i_clkevt_time_stop(0);
-		sun5i_clkevt_time_setup(0, ticks_per_jiffy);
-		sun5i_clkevt_time_start(0, true);
+		sun5i_clkevt_time_stop(ce, 0);
+		sun5i_clkevt_time_setup(ce, 0, ce->timer.ticks_per_jiffy);
+		sun5i_clkevt_time_start(ce, 0, true);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
-		sun5i_clkevt_time_stop(0);
-		sun5i_clkevt_time_start(0, false);
+		sun5i_clkevt_time_stop(ce, 0);
+		sun5i_clkevt_time_start(ce, 0, false);
 		break;
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
 	default:
-		sun5i_clkevt_time_stop(0);
+		sun5i_clkevt_time_stop(ce, 0);
 		break;
 	}
 }
 
 static int sun5i_clkevt_next_event(unsigned long evt,
-				   struct clock_event_device *unused)
+				   struct clock_event_device *clkevt)
 {
-	sun5i_clkevt_time_stop(0);
-	sun5i_clkevt_time_setup(0, evt - TIMER_SYNC_TICKS);
-	sun5i_clkevt_time_start(0, false);
+	struct sun5i_timer_clkevt *ce = to_sun5i_timer_clkevt(clkevt);
+
+	sun5i_clkevt_time_stop(ce, 0);
+	sun5i_clkevt_time_setup(ce, 0, evt - TIMER_SYNC_TICKS);
+	sun5i_clkevt_time_start(ce, 0, false);
 
 	return 0;
 }
 
-static struct clock_event_device sun5i_clockevent = {
-	.name = "sun5i_tick",
-	.rating = 340,
-	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
-	.set_mode = sun5i_clkevt_mode,
-	.set_next_event = sun5i_clkevt_next_event,
-};
-
-
 static irqreturn_t sun5i_timer_interrupt(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	struct sun5i_timer_clkevt *ce = (struct sun5i_timer_clkevt *)dev_id;
 
-	writel(0x1, timer_base + TIMER_IRQ_ST_REG);
-	evt->event_handler(evt);
+	writel(0x1, ce->timer.base + TIMER_IRQ_ST_REG);
+	ce->clkevt.event_handler(&ce->clkevt);
 
 	return IRQ_HANDLED;
 }
 
+static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
+{
+	struct sun5i_timer_clksrc *cs = to_sun5i_timer_clksrc(clksrc);
+
+	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
+}
+
+static int __init sun5i_setup_clocksource(struct device_node *node,
+					  void __iomem *base,
+					  struct clk *clk, int irq)
+{
+	struct sun5i_timer_clksrc *cs;
+	unsigned long rate;
+	int ret;
+
+	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
+	if (!cs)
+		return -ENOMEM;
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("Couldn't enable parent clock\n");
+		goto err_free;
+	}
+
+	rate = clk_get_rate(clk);
+
+	cs->timer.base = base;
+	cs->timer.clk = clk;
+
+	writel(~0, base + TIMER_INTVAL_LO_REG(1));
+	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
+	       base + TIMER_CTL_REG(1));
+
+	cs->clksrc.name = node->name;
+	cs->clksrc.rating = 340;
+	cs->clksrc.read = sun5i_clksrc_read;
+	cs->clksrc.mask = CLOCKSOURCE_MASK(32);
+	cs->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS;
+
+	ret = clocksource_register_hz(&cs->clksrc, rate);
+	if (ret) {
+		pr_err("Couldn't register clock source.\n");
+		goto err_disable_clk;
+	}
+
+	return 0;
+
+err_disable_clk:
+	clk_disable_unprepare(clk);
+err_free:
+	kfree(cs);
+	return ret;
+}
+
+static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem *base,
+					 struct clk *clk, int irq)
+{
+	struct sun5i_timer_clkevt *ce;
+	unsigned long rate;
+	int ret;
+	u32 val;
+
+	ce = kzalloc(sizeof(*ce), GFP_KERNEL);
+	if (!ce)
+		return -ENOMEM;
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("Couldn't enable parent clock\n");
+		goto err_free;
+	}
+
+	rate = clk_get_rate(clk);
+
+	ce->timer.base = base;
+	ce->timer.ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
+	ce->timer.clk = clk;
+
+	ce->clkevt.name = node->name;
+	ce->clkevt.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
+	ce->clkevt.set_next_event = sun5i_clkevt_next_event;
+	ce->clkevt.set_mode = sun5i_clkevt_mode;
+	ce->clkevt.rating = 340;
+	ce->clkevt.irq = irq;
+	ce->clkevt.cpumask = cpu_possible_mask;
+
+	/* Enable timer0 interrupt */
+	val = readl(base + TIMER_IRQ_EN_REG);
+	writel(val | TIMER_IRQ_EN(0), base + TIMER_IRQ_EN_REG);
+
+	ret = request_irq(irq, sun5i_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
+			  "sun5i_timer0", ce);
+	if (ret) {
+		pr_err("Unable to register interrupt\n");
+		goto err_disable_clk;
+	}
+
+	clockevents_config_and_register(&ce->clkevt, rate,
+					TIMER_SYNC_TICKS, 0xffffffff);
+
+	return 0;
+
+err_disable_clk:
+	clk_disable_unprepare(clk);
+err_free:
+	kfree(ce);
+	return ret;
+}
+
 static void __init sun5i_timer_init(struct device_node *node)
 {
 	struct reset_control *rstc;
-	unsigned long rate;
+	void __iomem *timer_base;
 	struct clk *clk;
-	int ret, irq;
-	u32 val;
+	int irq;
 
 	timer_base = of_io_request_and_map(node, 0,
 					   of_node_full_name(node));
@@ -150,36 +274,13 @@ static void __init sun5i_timer_init(struct device_node *node)
 	clk = of_clk_get(node, 0);
 	if (IS_ERR(clk))
 		panic("Can't get timer clock");
-	clk_prepare_enable(clk);
-	rate = clk_get_rate(clk);
 
 	rstc = of_reset_control_get(node, NULL);
 	if (!IS_ERR(rstc))
 		reset_control_deassert(rstc);
 
-	writel(~0, timer_base + TIMER_INTVAL_LO_REG(1));
-	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
-	       timer_base + TIMER_CTL_REG(1));
-
-	clocksource_mmio_init(timer_base + TIMER_CNTVAL_LO_REG(1), node->name,
-			      rate, 340, 32, clocksource_mmio_readl_down);
-
-	ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
-
-	ret = request_irq(irq, sun5i_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
-			  "sun5i_timer0", &sun5i_clockevent);
-	if (ret)
-		pr_warn("failed to setup irq %d\n", irq);
-
-	/* Enable timer0 interrupt */
-	val = readl(timer_base + TIMER_IRQ_EN_REG);
-	writel(val | TIMER_IRQ_EN(0), timer_base + TIMER_IRQ_EN_REG);
-
-	sun5i_clockevent.cpumask = cpu_possible_mask;
-	sun5i_clockevent.irq = irq;
-
-	clockevents_config_and_register(&sun5i_clockevent, rate,
-					TIMER_SYNC_TICKS, 0xffffffff);
+	sun5i_setup_clocksource(node, timer_base, clk, irq);
+	sun5i_setup_clockevent(node, timer_base, clk, irq);
 }
 CLOCKSOURCE_OF_DECLARE(sun5i_a13, "allwinner,sun5i-a13-hstimer",
 		       sun5i_timer_init);
-- 
2.2.2

[PATCH v2 4/5] clocksource: sun5i: Refactor the current code

[Maxime Ripard]

Refactor the code in order to remove the global variables and split the clock
source and clock events registration in order to ease the addition of the clock
notifiers needed to handle the parent clock rate changes.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 231 +++++++++++++++++++++++++++-----------
 1 file changed, 166 insertions(+), 65 deletions(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 295bb5e1010c..377e50450781 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -18,6 +18,7 @@
 #include <linux/irqreturn.h>
 #include <linux/reset.h>
 #include <linux/sched_clock.h>
+#include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -37,8 +38,27 @@
 
 #define TIMER_SYNC_TICKS	3
 
-static void __iomem *timer_base;
-static u32 ticks_per_jiffy;
+struct sun5i_timer {
+	void __iomem		*base;
+	struct clk		*clk;
+	u32			ticks_per_jiffy;
+};
+
+struct sun5i_timer_clksrc {
+	struct sun5i_timer	timer;
+	struct clocksource	clksrc;
+};
+
+#define to_sun5i_timer_clksrc(x) \
+	container_of(x, struct sun5i_timer_clksrc, clksrc)
+
+struct sun5i_timer_clkevt {
+	struct sun5i_timer		timer;
+	struct clock_event_device	clkevt;
+};
+
+#define to_sun5i_timer_clkevt(x) \
+	container_of(x, struct sun5i_timer_clkevt, clkevt)
 
 /*
  * When we disable a timer, we need to wait at least for 2 cycles of
@@ -46,30 +66,30 @@ static u32 ticks_per_jiffy;
  * that is already setup and runs at the same frequency than the other
  * timers, and we never will be disabled.
  */
-static void sun5i_clkevt_sync(void)
+static void sun5i_clkevt_sync(struct sun5i_timer_clkevt *ce)
 {
-	u32 old = readl(timer_base + TIMER_CNTVAL_LO_REG(1));
+	u32 old = readl(ce->timer.base + TIMER_CNTVAL_LO_REG(1));
 
-	while ((old - readl(timer_base + TIMER_CNTVAL_LO_REG(1))) < TIMER_SYNC_TICKS)
+	while ((old - readl(ce->timer.base + TIMER_CNTVAL_LO_REG(1))) < TIMER_SYNC_TICKS)
 		cpu_relax();
 }
 
-static void sun5i_clkevt_time_stop(u8 timer)
+static void sun5i_clkevt_time_stop(struct sun5i_timer_clkevt *ce, u8 timer)
 {
-	u32 val = readl(timer_base + TIMER_CTL_REG(timer));
-	writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(timer));
+	u32 val = readl(ce->timer.base + TIMER_CTL_REG(timer));
+	writel(val & ~TIMER_CTL_ENABLE, ce->timer.base + TIMER_CTL_REG(timer));
 
-	sun5i_clkevt_sync();
+	sun5i_clkevt_sync(ce);
 }
 
-static void sun5i_clkevt_time_setup(u8 timer, u32 delay)
+static void sun5i_clkevt_time_setup(struct sun5i_timer_clkevt *ce, u8 timer, u32 delay)
 {
-	writel(delay, timer_base + TIMER_INTVAL_LO_REG(timer));
+	writel(delay, ce->timer.base + TIMER_INTVAL_LO_REG(timer));
 }
 
-static void sun5i_clkevt_time_start(u8 timer, bool periodic)
+static void sun5i_clkevt_time_start(struct sun5i_timer_clkevt *ce, u8 timer, bool periodic)
 {
-	u32 val = readl(timer_base + TIMER_CTL_REG(timer));
+	u32 val = readl(ce->timer.base + TIMER_CTL_REG(timer));
 
 	if (periodic)
 		val &= ~TIMER_CTL_ONESHOT;
@@ -77,66 +97,170 @@ static void sun5i_clkevt_time_start(u8 timer, bool periodic)
 		val |= TIMER_CTL_ONESHOT;
 
 	writel(val | TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
-	       timer_base + TIMER_CTL_REG(timer));
+	       ce->timer.base + TIMER_CTL_REG(timer));
 }
 
 static void sun5i_clkevt_mode(enum clock_event_mode mode,
-			      struct clock_event_device *clk)
+			      struct clock_event_device *clkevt)
 {
+	struct sun5i_timer_clkevt *ce = to_sun5i_timer_clkevt(clkevt);
+
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		sun5i_clkevt_time_stop(0);
-		sun5i_clkevt_time_setup(0, ticks_per_jiffy);
-		sun5i_clkevt_time_start(0, true);
+		sun5i_clkevt_time_stop(ce, 0);
+		sun5i_clkevt_time_setup(ce, 0, ce->timer.ticks_per_jiffy);
+		sun5i_clkevt_time_start(ce, 0, true);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
-		sun5i_clkevt_time_stop(0);
-		sun5i_clkevt_time_start(0, false);
+		sun5i_clkevt_time_stop(ce, 0);
+		sun5i_clkevt_time_start(ce, 0, false);
 		break;
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
 	default:
-		sun5i_clkevt_time_stop(0);
+		sun5i_clkevt_time_stop(ce, 0);
 		break;
 	}
 }
 
 static int sun5i_clkevt_next_event(unsigned long evt,
-				   struct clock_event_device *unused)
+				   struct clock_event_device *clkevt)
 {
-	sun5i_clkevt_time_stop(0);
-	sun5i_clkevt_time_setup(0, evt - TIMER_SYNC_TICKS);
-	sun5i_clkevt_time_start(0, false);
+	struct sun5i_timer_clkevt *ce = to_sun5i_timer_clkevt(clkevt);
+
+	sun5i_clkevt_time_stop(ce, 0);
+	sun5i_clkevt_time_setup(ce, 0, evt - TIMER_SYNC_TICKS);
+	sun5i_clkevt_time_start(ce, 0, false);
 
 	return 0;
 }
 
-static struct clock_event_device sun5i_clockevent = {
-	.name = "sun5i_tick",
-	.rating = 340,
-	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
-	.set_mode = sun5i_clkevt_mode,
-	.set_next_event = sun5i_clkevt_next_event,
-};
-
-
 static irqreturn_t sun5i_timer_interrupt(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	struct sun5i_timer_clkevt *ce = (struct sun5i_timer_clkevt *)dev_id;
 
-	writel(0x1, timer_base + TIMER_IRQ_ST_REG);
-	evt->event_handler(evt);
+	writel(0x1, ce->timer.base + TIMER_IRQ_ST_REG);
+	ce->clkevt.event_handler(&ce->clkevt);
 
 	return IRQ_HANDLED;
 }
 
+static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
+{
+	struct sun5i_timer_clksrc *cs = to_sun5i_timer_clksrc(clksrc);
+
+	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
+}
+
+static int __init sun5i_setup_clocksource(struct device_node *node,
+					  void __iomem *base,
+					  struct clk *clk, int irq)
+{
+	struct sun5i_timer_clksrc *cs;
+	unsigned long rate;
+	int ret;
+
+	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
+	if (!cs)
+		return -ENOMEM;
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("Couldn't enable parent clock\n");
+		goto err_free;
+	}
+
+	rate = clk_get_rate(clk);
+
+	cs->timer.base = base;
+	cs->timer.clk = clk;
+
+	writel(~0, base + TIMER_INTVAL_LO_REG(1));
+	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
+	       base + TIMER_CTL_REG(1));
+
+	cs->clksrc.name = node->name;
+	cs->clksrc.rating = 340;
+	cs->clksrc.read = sun5i_clksrc_read;
+	cs->clksrc.mask = CLOCKSOURCE_MASK(32);
+	cs->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS;
+
+	ret = clocksource_register_hz(&cs->clksrc, rate);
+	if (ret) {
+		pr_err("Couldn't register clock source.\n");
+		goto err_disable_clk;
+	}
+
+	return 0;
+
+err_disable_clk:
+	clk_disable_unprepare(clk);
+err_free:
+	kfree(cs);
+	return ret;
+}
+
+static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem *base,
+					 struct clk *clk, int irq)
+{
+	struct sun5i_timer_clkevt *ce;
+	unsigned long rate;
+	int ret;
+	u32 val;
+
+	ce = kzalloc(sizeof(*ce), GFP_KERNEL);
+	if (!ce)
+		return -ENOMEM;
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("Couldn't enable parent clock\n");
+		goto err_free;
+	}
+
+	rate = clk_get_rate(clk);
+
+	ce->timer.base = base;
+	ce->timer.ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
+	ce->timer.clk = clk;
+
+	ce->clkevt.name = node->name;
+	ce->clkevt.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
+	ce->clkevt.set_next_event = sun5i_clkevt_next_event;
+	ce->clkevt.set_mode = sun5i_clkevt_mode;
+	ce->clkevt.rating = 340;
+	ce->clkevt.irq = irq;
+	ce->clkevt.cpumask = cpu_possible_mask;
+
+	/* Enable timer0 interrupt */
+	val = readl(base + TIMER_IRQ_EN_REG);
+	writel(val | TIMER_IRQ_EN(0), base + TIMER_IRQ_EN_REG);
+
+	ret = request_irq(irq, sun5i_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
+			  "sun5i_timer0", ce);
+	if (ret) {
+		pr_err("Unable to register interrupt\n");
+		goto err_disable_clk;
+	}
+
+	clockevents_config_and_register(&ce->clkevt, rate,
+					TIMER_SYNC_TICKS, 0xffffffff);
+
+	return 0;
+
+err_disable_clk:
+	clk_disable_unprepare(clk);
+err_free:
+	kfree(ce);
+	return ret;
+}
+
 static void __init sun5i_timer_init(struct device_node *node)
 {
 	struct reset_control *rstc;
-	unsigned long rate;
+	void __iomem *timer_base;
 	struct clk *clk;
-	int ret, irq;
-	u32 val;
+	int irq;
 
 	timer_base = of_io_request_and_map(node, 0,
 					   of_node_full_name(node));
@@ -150,36 +274,13 @@ static void __init sun5i_timer_init(struct device_node *node)
 	clk = of_clk_get(node, 0);
 	if (IS_ERR(clk))
 		panic("Can't get timer clock");
-	clk_prepare_enable(clk);
-	rate = clk_get_rate(clk);
 
 	rstc = of_reset_control_get(node, NULL);
 	if (!IS_ERR(rstc))
 		reset_control_deassert(rstc);
 
-	writel(~0, timer_base + TIMER_INTVAL_LO_REG(1));
-	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
-	       timer_base + TIMER_CTL_REG(1));
-
-	clocksource_mmio_init(timer_base + TIMER_CNTVAL_LO_REG(1), node->name,
-			      rate, 340, 32, clocksource_mmio_readl_down);
-
-	ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
-
-	ret = request_irq(irq, sun5i_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
-			  "sun5i_timer0", &sun5i_clockevent);
-	if (ret)
-		pr_warn("failed to setup irq %d\n", irq);
-
-	/* Enable timer0 interrupt */
-	val = readl(timer_base + TIMER_IRQ_EN_REG);
-	writel(val | TIMER_IRQ_EN(0), timer_base + TIMER_IRQ_EN_REG);
-
-	sun5i_clockevent.cpumask = cpu_possible_mask;
-	sun5i_clockevent.irq = irq;
-
-	clockevents_config_and_register(&sun5i_clockevent, rate,
-					TIMER_SYNC_TICKS, 0xffffffff);
+	sun5i_setup_clocksource(node, timer_base, clk, irq);
+	sun5i_setup_clockevent(node, timer_base, clk, irq);
 }
 CLOCKSOURCE_OF_DECLARE(sun5i_a13, "allwinner,sun5i-a13-hstimer",
 		       sun5i_timer_init);
-- 
2.2.2

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Maxime Ripard]

The parent clock of the sun5i timer is the AHB clock, which rate might change
because of other devices requirements.

This is for example the case on the Allwinner A31, where the DMA controller
needs a minimum rate higher than the default, that is enforced after the timer
driver has probed.

Add clock notifiers to make sure we reflect the clock rate changes in the timer
rates.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 72 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 377e50450781..5841a956f5f0 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -41,9 +41,13 @@
 struct sun5i_timer {
 	void __iomem		*base;
 	struct clk		*clk;
+	struct notifier_block	clk_rate_cb;
 	u32			ticks_per_jiffy;
 };
 
+#define to_sun5i_timer(x) \
+	container_of(x, struct sun5i_timer, clk_rate_cb)
+
 struct sun5i_timer_clksrc {
 	struct sun5i_timer	timer;
 	struct clocksource	clksrc;
@@ -152,6 +156,30 @@ static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
 	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
 }
 
+static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
+				unsigned long event, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct sun5i_timer *timer = to_sun5i_timer(nb);
+	struct sun5i_timer_clksrc *cs = container_of(timer,
+						     struct sun5i_timer_clksrc, timer);
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		clocksource_unregister(&cs->clksrc);
+		break;
+
+	case POST_RATE_CHANGE:
+		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
+		break;
+
+	default:
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int __init sun5i_setup_clocksource(struct device_node *node,
 					  void __iomem *base,
 					  struct clk *clk, int irq)
@@ -174,6 +202,14 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
 
 	cs->timer.base = base;
 	cs->timer.clk = clk;
+	cs->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clksrc;
+	cs->timer.clk_rate_cb.next = NULL;
+
+	ret = clk_notifier_register(clk, &cs->timer.clk_rate_cb);
+	if (ret) {
+		pr_err("Unable to register clock notifier.\n");
+		goto err_disable_clk;
+	}
 
 	writel(~0, base + TIMER_INTVAL_LO_REG(1));
 	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
@@ -188,11 +224,13 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
 	ret = clocksource_register_hz(&cs->clksrc, rate);
 	if (ret) {
 		pr_err("Couldn't register clock source.\n");
-		goto err_disable_clk;
+		goto err_remove_notifier;
 	}
 
 	return 0;
 
+err_remove_notifier:
+	clk_notifier_unregister(clk, &cs->timer.clk_rate_cb);
 err_disable_clk:
 	clk_disable_unprepare(clk);
 err_free:
@@ -200,6 +238,26 @@ err_free:
 	return ret;
 }
 
+static int sun5i_rate_cb_clkevt(struct notifier_block *nb,
+				unsigned long event, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct sun5i_timer *timer = to_sun5i_timer(nb);
+	struct sun5i_timer_clkevt *ce = container_of(timer,
+						     struct sun5i_timer_clkevt, timer);
+	unsigned long flags;
+
+	if (event == POST_RATE_CHANGE) {
+		local_irq_save(flags);
+		clockevents_update_freq(&ce->clkevt, ndata->new_rate);
+		local_irq_restore(flags);
+
+		ce->timer.ticks_per_jiffy = DIV_ROUND_UP(ndata->new_rate, HZ);
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem *base,
 					 struct clk *clk, int irq)
 {
@@ -223,6 +281,14 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
 	ce->timer.base = base;
 	ce->timer.ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
 	ce->timer.clk = clk;
+	ce->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clkevt;
+	ce->timer.clk_rate_cb.next = NULL;
+
+	ret = clk_notifier_register(clk, &ce->timer.clk_rate_cb);
+	if (ret) {
+		pr_err("Unable to register clock notifier.\n");
+		goto err_disable_clk;
+	}
 
 	ce->clkevt.name = node->name;
 	ce->clkevt.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
@@ -240,7 +306,7 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
 			  "sun5i_timer0", ce);
 	if (ret) {
 		pr_err("Unable to register interrupt\n");
-		goto err_disable_clk;
+		goto err_remove_notifier;
 	}
 
 	clockevents_config_and_register(&ce->clkevt, rate,
@@ -248,6 +314,8 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
 
 	return 0;
 
+err_remove_notifier:
+	clk_notifier_unregister(clk, &ce->timer.clk_rate_cb);
 err_disable_clk:
 	clk_disable_unprepare(clk);
 err_free:
-- 
2.2.2

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Maxime Ripard]

The parent clock of the sun5i timer is the AHB clock, which rate might change
because of other devices requirements.

This is for example the case on the Allwinner A31, where the DMA controller
needs a minimum rate higher than the default, that is enforced after the timer
driver has probed.

Add clock notifiers to make sure we reflect the clock rate changes in the timer
rates.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/timer-sun5i.c | 72 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
index 377e50450781..5841a956f5f0 100644
--- a/drivers/clocksource/timer-sun5i.c
+++ b/drivers/clocksource/timer-sun5i.c
@@ -41,9 +41,13 @@
 struct sun5i_timer {
 	void __iomem		*base;
 	struct clk		*clk;
+	struct notifier_block	clk_rate_cb;
 	u32			ticks_per_jiffy;
 };
 
+#define to_sun5i_timer(x) \
+	container_of(x, struct sun5i_timer, clk_rate_cb)
+
 struct sun5i_timer_clksrc {
 	struct sun5i_timer	timer;
 	struct clocksource	clksrc;
@@ -152,6 +156,30 @@ static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
 	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
 }
 
+static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
+				unsigned long event, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct sun5i_timer *timer = to_sun5i_timer(nb);
+	struct sun5i_timer_clksrc *cs = container_of(timer,
+						     struct sun5i_timer_clksrc, timer);
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		clocksource_unregister(&cs->clksrc);
+		break;
+
+	case POST_RATE_CHANGE:
+		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
+		break;
+
+	default:
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int __init sun5i_setup_clocksource(struct device_node *node,
 					  void __iomem *base,
 					  struct clk *clk, int irq)
@@ -174,6 +202,14 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
 
 	cs->timer.base = base;
 	cs->timer.clk = clk;
+	cs->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clksrc;
+	cs->timer.clk_rate_cb.next = NULL;
+
+	ret = clk_notifier_register(clk, &cs->timer.clk_rate_cb);
+	if (ret) {
+		pr_err("Unable to register clock notifier.\n");
+		goto err_disable_clk;
+	}
 
 	writel(~0, base + TIMER_INTVAL_LO_REG(1));
 	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
@@ -188,11 +224,13 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
 	ret = clocksource_register_hz(&cs->clksrc, rate);
 	if (ret) {
 		pr_err("Couldn't register clock source.\n");
-		goto err_disable_clk;
+		goto err_remove_notifier;
 	}
 
 	return 0;
 
+err_remove_notifier:
+	clk_notifier_unregister(clk, &cs->timer.clk_rate_cb);
 err_disable_clk:
 	clk_disable_unprepare(clk);
 err_free:
@@ -200,6 +238,26 @@ err_free:
 	return ret;
 }
 
+static int sun5i_rate_cb_clkevt(struct notifier_block *nb,
+				unsigned long event, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct sun5i_timer *timer = to_sun5i_timer(nb);
+	struct sun5i_timer_clkevt *ce = container_of(timer,
+						     struct sun5i_timer_clkevt, timer);
+	unsigned long flags;
+
+	if (event == POST_RATE_CHANGE) {
+		local_irq_save(flags);
+		clockevents_update_freq(&ce->clkevt, ndata->new_rate);
+		local_irq_restore(flags);
+
+		ce->timer.ticks_per_jiffy = DIV_ROUND_UP(ndata->new_rate, HZ);
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem *base,
 					 struct clk *clk, int irq)
 {
@@ -223,6 +281,14 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
 	ce->timer.base = base;
 	ce->timer.ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
 	ce->timer.clk = clk;
+	ce->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clkevt;
+	ce->timer.clk_rate_cb.next = NULL;
+
+	ret = clk_notifier_register(clk, &ce->timer.clk_rate_cb);
+	if (ret) {
+		pr_err("Unable to register clock notifier.\n");
+		goto err_disable_clk;
+	}
 
 	ce->clkevt.name = node->name;
 	ce->clkevt.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
@@ -240,7 +306,7 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
 			  "sun5i_timer0", ce);
 	if (ret) {
 		pr_err("Unable to register interrupt\n");
-		goto err_disable_clk;
+		goto err_remove_notifier;
 	}
 
 	clockevents_config_and_register(&ce->clkevt, rate,
@@ -248,6 +314,8 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
 
 	return 0;
 
+err_remove_notifier:
+	clk_notifier_unregister(clk, &ce->timer.clk_rate_cb);
 err_disable_clk:
 	clk_disable_unprepare(clk);
 err_free:
-- 
2.2.2

Re: [PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Daniel Lezcano]

On 01/26/2015 10:50 AM, Maxime Ripard wrote:
> The parent clock of the sun5i timer is the AHB clock, which rate might change
> because of other devices requirements.
>
> This is for example the case on the Allwinner A31, where the DMA controller
> needs a minimum rate higher than the default, that is enforced after the timer
> driver has probed.
>
> Add clock notifiers to make sure we reflect the clock rate changes in the timer
> rates.

Mmh, I am wondering if that shouldn't go to the time framework ...

Looking at the notifier callbacks that call generic time framework 
functions.

Can you have a look at commit b3e90722 ? and double check your changes ?

A couple of comments below.

> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>   drivers/clocksource/timer-sun5i.c | 72 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
> index 377e50450781..5841a956f5f0 100644
> --- a/drivers/clocksource/timer-sun5i.c
> +++ b/drivers/clocksource/timer-sun5i.c
> @@ -41,9 +41,13 @@
>   struct sun5i_timer {
>   	void __iomem		*base;
>   	struct clk		*clk;
> +	struct notifier_block	clk_rate_cb;
>   	u32			ticks_per_jiffy;
>   };
>
> +#define to_sun5i_timer(x) \
> +	container_of(x, struct sun5i_timer, clk_rate_cb)
> +
>   struct sun5i_timer_clksrc {
>   	struct sun5i_timer	timer;
>   	struct clocksource	clksrc;
> @@ -152,6 +156,30 @@ static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
>   	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
>   }
>
> +static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
> +				unsigned long event, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
> +	struct sun5i_timer_clksrc *cs = container_of(timer,
> +						     struct sun5i_timer_clksrc, timer);
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		clocksource_unregister(&cs->clksrc);
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
> +		break;

Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?

> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static int __init sun5i_setup_clocksource(struct device_node *node,
>   					  void __iomem *base,
>   					  struct clk *clk, int irq)
> @@ -174,6 +202,14 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
>
>   	cs->timer.base = base;
>   	cs->timer.clk = clk;
> +	cs->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clksrc;
> +	cs->timer.clk_rate_cb.next = NULL;
> +
> +	ret = clk_notifier_register(clk, &cs->timer.clk_rate_cb);
> +	if (ret) {
> +		pr_err("Unable to register clock notifier.\n");
> +		goto err_disable_clk;
> +	}
>
>   	writel(~0, base + TIMER_INTVAL_LO_REG(1));
>   	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
> @@ -188,11 +224,13 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
>   	ret = clocksource_register_hz(&cs->clksrc, rate);
>   	if (ret) {
>   		pr_err("Couldn't register clock source.\n");
> -		goto err_disable_clk;
> +		goto err_remove_notifier;
>   	}
>
>   	return 0;
>
> +err_remove_notifier:
> +	clk_notifier_unregister(clk, &cs->timer.clk_rate_cb);
>   err_disable_clk:
>   	clk_disable_unprepare(clk);
>   err_free:
> @@ -200,6 +238,26 @@ err_free:
>   	return ret;
>   }
>
> +static int sun5i_rate_cb_clkevt(struct notifier_block *nb,
> +				unsigned long event, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
> +	struct sun5i_timer_clkevt *ce = container_of(timer,
> +						     struct sun5i_timer_clkevt, timer);
> +	unsigned long flags;
> +
> +	if (event == POST_RATE_CHANGE) {
> +		local_irq_save(flags);
> +		clockevents_update_freq(&ce->clkevt, ndata->new_rate);
> +		local_irq_restore(flags);

local_irq_save and local_irq_restore are already in the 
clockevents_update_freq function.

> +		ce->timer.ticks_per_jiffy = DIV_ROUND_UP(ndata->new_rate, HZ);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem *base,
>   					 struct clk *clk, int irq)
>   {
> @@ -223,6 +281,14 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
>   	ce->timer.base = base;
>   	ce->timer.ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
>   	ce->timer.clk = clk;
> +	ce->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clkevt;
> +	ce->timer.clk_rate_cb.next = NULL;
> +
> +	ret = clk_notifier_register(clk, &ce->timer.clk_rate_cb);
> +	if (ret) {
> +		pr_err("Unable to register clock notifier.\n");
> +		goto err_disable_clk;
> +	}
>
>   	ce->clkevt.name = node->name;
>   	ce->clkevt.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> @@ -240,7 +306,7 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
>   			  "sun5i_timer0", ce);
>   	if (ret) {
>   		pr_err("Unable to register interrupt\n");
> -		goto err_disable_clk;
> +		goto err_remove_notifier;
>   	}
>
>   	clockevents_config_and_register(&ce->clkevt, rate,
> @@ -248,6 +314,8 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
>
>   	return 0;
>
> +err_remove_notifier:
> +	clk_notifier_unregister(clk, &ce->timer.clk_rate_cb);
>   err_disable_clk:
>   	clk_disable_unprepare(clk);
>   err_free:
>


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

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

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Daniel Lezcano]

On 01/26/2015 10:50 AM, Maxime Ripard wrote:
> The parent clock of the sun5i timer is the AHB clock, which rate might change
> because of other devices requirements.
>
> This is for example the case on the Allwinner A31, where the DMA controller
> needs a minimum rate higher than the default, that is enforced after the timer
> driver has probed.
>
> Add clock notifiers to make sure we reflect the clock rate changes in the timer
> rates.

Mmh, I am wondering if that shouldn't go to the time framework ...

Looking at the notifier callbacks that call generic time framework 
functions.

Can you have a look at commit b3e90722 ? and double check your changes ?

A couple of comments below.

> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>   drivers/clocksource/timer-sun5i.c | 72 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
> index 377e50450781..5841a956f5f0 100644
> --- a/drivers/clocksource/timer-sun5i.c
> +++ b/drivers/clocksource/timer-sun5i.c
> @@ -41,9 +41,13 @@
>   struct sun5i_timer {
>   	void __iomem		*base;
>   	struct clk		*clk;
> +	struct notifier_block	clk_rate_cb;
>   	u32			ticks_per_jiffy;
>   };
>
> +#define to_sun5i_timer(x) \
> +	container_of(x, struct sun5i_timer, clk_rate_cb)
> +
>   struct sun5i_timer_clksrc {
>   	struct sun5i_timer	timer;
>   	struct clocksource	clksrc;
> @@ -152,6 +156,30 @@ static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
>   	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
>   }
>
> +static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
> +				unsigned long event, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
> +	struct sun5i_timer_clksrc *cs = container_of(timer,
> +						     struct sun5i_timer_clksrc, timer);
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		clocksource_unregister(&cs->clksrc);
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
> +		break;

Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?

> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static int __init sun5i_setup_clocksource(struct device_node *node,
>   					  void __iomem *base,
>   					  struct clk *clk, int irq)
> @@ -174,6 +202,14 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
>
>   	cs->timer.base = base;
>   	cs->timer.clk = clk;
> +	cs->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clksrc;
> +	cs->timer.clk_rate_cb.next = NULL;
> +
> +	ret = clk_notifier_register(clk, &cs->timer.clk_rate_cb);
> +	if (ret) {
> +		pr_err("Unable to register clock notifier.\n");
> +		goto err_disable_clk;
> +	}
>
>   	writel(~0, base + TIMER_INTVAL_LO_REG(1));
>   	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
> @@ -188,11 +224,13 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
>   	ret = clocksource_register_hz(&cs->clksrc, rate);
>   	if (ret) {
>   		pr_err("Couldn't register clock source.\n");
> -		goto err_disable_clk;
> +		goto err_remove_notifier;
>   	}
>
>   	return 0;
>
> +err_remove_notifier:
> +	clk_notifier_unregister(clk, &cs->timer.clk_rate_cb);
>   err_disable_clk:
>   	clk_disable_unprepare(clk);
>   err_free:
> @@ -200,6 +238,26 @@ err_free:
>   	return ret;
>   }
>
> +static int sun5i_rate_cb_clkevt(struct notifier_block *nb,
> +				unsigned long event, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
> +	struct sun5i_timer_clkevt *ce = container_of(timer,
> +						     struct sun5i_timer_clkevt, timer);
> +	unsigned long flags;
> +
> +	if (event == POST_RATE_CHANGE) {
> +		local_irq_save(flags);
> +		clockevents_update_freq(&ce->clkevt, ndata->new_rate);
> +		local_irq_restore(flags);

local_irq_save and local_irq_restore are already in the 
clockevents_update_freq function.

> +		ce->timer.ticks_per_jiffy = DIV_ROUND_UP(ndata->new_rate, HZ);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem *base,
>   					 struct clk *clk, int irq)
>   {
> @@ -223,6 +281,14 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
>   	ce->timer.base = base;
>   	ce->timer.ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
>   	ce->timer.clk = clk;
> +	ce->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clkevt;
> +	ce->timer.clk_rate_cb.next = NULL;
> +
> +	ret = clk_notifier_register(clk, &ce->timer.clk_rate_cb);
> +	if (ret) {
> +		pr_err("Unable to register clock notifier.\n");
> +		goto err_disable_clk;
> +	}
>
>   	ce->clkevt.name = node->name;
>   	ce->clkevt.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> @@ -240,7 +306,7 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
>   			  "sun5i_timer0", ce);
>   	if (ret) {
>   		pr_err("Unable to register interrupt\n");
> -		goto err_disable_clk;
> +		goto err_remove_notifier;
>   	}
>
>   	clockevents_config_and_register(&ce->clkevt, rate,
> @@ -248,6 +314,8 @@ static int __init sun5i_setup_clockevent(struct device_node *node, void __iomem
>
>   	return 0;
>
> +err_remove_notifier:
> +	clk_notifier_unregister(clk, &ce->timer.clk_rate_cb);
>   err_disable_clk:
>   	clk_disable_unprepare(clk);
>   err_free:
>


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

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

Re: [PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5186 bytes --]

Hi Daniel,

On Mon, Jan 26, 2015 at 12:22:16PM +0100, Daniel Lezcano wrote:
> On 01/26/2015 10:50 AM, Maxime Ripard wrote:
> >The parent clock of the sun5i timer is the AHB clock, which rate might change
> >because of other devices requirements.
> >
> >This is for example the case on the Allwinner A31, where the DMA controller
> >needs a minimum rate higher than the default, that is enforced after the timer
> >driver has probed.
> >
> >Add clock notifiers to make sure we reflect the clock rate changes in the timer
> >rates.
> 
> Mmh, I am wondering if that shouldn't go to the time framework ...
> 
> Looking at the notifier callbacks that call generic time framework
> functions.

I don't think the framework currently has a clock handle so far?

> Can you have a look at commit b3e90722 ? and double check your
> changes ?

I'm exactly in the opposite situation.

The parent clock of this timer is *not* the CPU clock, and it might
even be in a completely different clock tree.

However, our DMA controller has the same parent clock than the timer,
and it needs some frequency adjustement. Otherwise, DMA would just not
work.

So I'm completely fine with any rate change as far as the timer is
concerned, the driver just need to be notified to be able to reflect
this rate change.

> A couple of comments below.
> 
> >Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >---
> >  drivers/clocksource/timer-sun5i.c | 72 +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 70 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
> >index 377e50450781..5841a956f5f0 100644
> >--- a/drivers/clocksource/timer-sun5i.c
> >+++ b/drivers/clocksource/timer-sun5i.c
> >@@ -41,9 +41,13 @@
> >  struct sun5i_timer {
> >  	void __iomem		*base;
> >  	struct clk		*clk;
> >+	struct notifier_block	clk_rate_cb;
> >  	u32			ticks_per_jiffy;
> >  };
> >
> >+#define to_sun5i_timer(x) \
> >+	container_of(x, struct sun5i_timer, clk_rate_cb)
> >+
> >  struct sun5i_timer_clksrc {
> >  	struct sun5i_timer	timer;
> >  	struct clocksource	clksrc;
> >@@ -152,6 +156,30 @@ static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
> >  	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
> >  }
> >
> >+static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
> >+				unsigned long event, void *data)
> >+{
> >+	struct clk_notifier_data *ndata = data;
> >+	struct sun5i_timer *timer = to_sun5i_timer(nb);
> >+	struct sun5i_timer_clksrc *cs = container_of(timer,
> >+						     struct sun5i_timer_clksrc, timer);
> >+
> >+	switch (event) {
> >+	case PRE_RATE_CHANGE:
> >+		clocksource_unregister(&cs->clksrc);
> >+		break;
> >+
> >+	case POST_RATE_CHANGE:
> >+		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
> >+		break;
> 
> Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?

Wouldn't that leave a (small, I agree) window where the timer would
run at a rate different to the one it has been registered with?

> >+	default:
> >+		break;
> >+	}
> >+
> >+	return NOTIFY_DONE;
> >+}
> >+
> >  static int __init sun5i_setup_clocksource(struct device_node *node,
> >  					  void __iomem *base,
> >  					  struct clk *clk, int irq)
> >@@ -174,6 +202,14 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
> >
> >  	cs->timer.base = base;
> >  	cs->timer.clk = clk;
> >+	cs->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clksrc;
> >+	cs->timer.clk_rate_cb.next = NULL;
> >+
> >+	ret = clk_notifier_register(clk, &cs->timer.clk_rate_cb);
> >+	if (ret) {
> >+		pr_err("Unable to register clock notifier.\n");
> >+		goto err_disable_clk;
> >+	}
> >
> >  	writel(~0, base + TIMER_INTVAL_LO_REG(1));
> >  	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
> >@@ -188,11 +224,13 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
> >  	ret = clocksource_register_hz(&cs->clksrc, rate);
> >  	if (ret) {
> >  		pr_err("Couldn't register clock source.\n");
> >-		goto err_disable_clk;
> >+		goto err_remove_notifier;
> >  	}
> >
> >  	return 0;
> >
> >+err_remove_notifier:
> >+	clk_notifier_unregister(clk, &cs->timer.clk_rate_cb);
> >  err_disable_clk:
> >  	clk_disable_unprepare(clk);
> >  err_free:
> >@@ -200,6 +238,26 @@ err_free:
> >  	return ret;
> >  }
> >
> >+static int sun5i_rate_cb_clkevt(struct notifier_block *nb,
> >+				unsigned long event, void *data)
> >+{
> >+	struct clk_notifier_data *ndata = data;
> >+	struct sun5i_timer *timer = to_sun5i_timer(nb);
> >+	struct sun5i_timer_clkevt *ce = container_of(timer,
> >+						     struct sun5i_timer_clkevt, timer);
> >+	unsigned long flags;
> >+
> >+	if (event == POST_RATE_CHANGE) {
> >+		local_irq_save(flags);
> >+		clockevents_update_freq(&ce->clkevt, ndata->new_rate);
> >+		local_irq_restore(flags);
> 
> local_irq_save and local_irq_restore are already in the
> clockevents_update_freq function.

Ok.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Maxime Ripard]

Hi Daniel,

On Mon, Jan 26, 2015 at 12:22:16PM +0100, Daniel Lezcano wrote:
> On 01/26/2015 10:50 AM, Maxime Ripard wrote:
> >The parent clock of the sun5i timer is the AHB clock, which rate might change
> >because of other devices requirements.
> >
> >This is for example the case on the Allwinner A31, where the DMA controller
> >needs a minimum rate higher than the default, that is enforced after the timer
> >driver has probed.
> >
> >Add clock notifiers to make sure we reflect the clock rate changes in the timer
> >rates.
> 
> Mmh, I am wondering if that shouldn't go to the time framework ...
> 
> Looking at the notifier callbacks that call generic time framework
> functions.

I don't think the framework currently has a clock handle so far?

> Can you have a look at commit b3e90722 ? and double check your
> changes ?

I'm exactly in the opposite situation.

The parent clock of this timer is *not* the CPU clock, and it might
even be in a completely different clock tree.

However, our DMA controller has the same parent clock than the timer,
and it needs some frequency adjustement. Otherwise, DMA would just not
work.

So I'm completely fine with any rate change as far as the timer is
concerned, the driver just need to be notified to be able to reflect
this rate change.

> A couple of comments below.
> 
> >Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >---
> >  drivers/clocksource/timer-sun5i.c | 72 +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 70 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
> >index 377e50450781..5841a956f5f0 100644
> >--- a/drivers/clocksource/timer-sun5i.c
> >+++ b/drivers/clocksource/timer-sun5i.c
> >@@ -41,9 +41,13 @@
> >  struct sun5i_timer {
> >  	void __iomem		*base;
> >  	struct clk		*clk;
> >+	struct notifier_block	clk_rate_cb;
> >  	u32			ticks_per_jiffy;
> >  };
> >
> >+#define to_sun5i_timer(x) \
> >+	container_of(x, struct sun5i_timer, clk_rate_cb)
> >+
> >  struct sun5i_timer_clksrc {
> >  	struct sun5i_timer	timer;
> >  	struct clocksource	clksrc;
> >@@ -152,6 +156,30 @@ static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
> >  	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
> >  }
> >
> >+static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
> >+				unsigned long event, void *data)
> >+{
> >+	struct clk_notifier_data *ndata = data;
> >+	struct sun5i_timer *timer = to_sun5i_timer(nb);
> >+	struct sun5i_timer_clksrc *cs = container_of(timer,
> >+						     struct sun5i_timer_clksrc, timer);
> >+
> >+	switch (event) {
> >+	case PRE_RATE_CHANGE:
> >+		clocksource_unregister(&cs->clksrc);
> >+		break;
> >+
> >+	case POST_RATE_CHANGE:
> >+		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
> >+		break;
> 
> Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?

Wouldn't that leave a (small, I agree) window where the timer would
run at a rate different to the one it has been registered with?

> >+	default:
> >+		break;
> >+	}
> >+
> >+	return NOTIFY_DONE;
> >+}
> >+
> >  static int __init sun5i_setup_clocksource(struct device_node *node,
> >  					  void __iomem *base,
> >  					  struct clk *clk, int irq)
> >@@ -174,6 +202,14 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
> >
> >  	cs->timer.base = base;
> >  	cs->timer.clk = clk;
> >+	cs->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clksrc;
> >+	cs->timer.clk_rate_cb.next = NULL;
> >+
> >+	ret = clk_notifier_register(clk, &cs->timer.clk_rate_cb);
> >+	if (ret) {
> >+		pr_err("Unable to register clock notifier.\n");
> >+		goto err_disable_clk;
> >+	}
> >
> >  	writel(~0, base + TIMER_INTVAL_LO_REG(1));
> >  	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
> >@@ -188,11 +224,13 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
> >  	ret = clocksource_register_hz(&cs->clksrc, rate);
> >  	if (ret) {
> >  		pr_err("Couldn't register clock source.\n");
> >-		goto err_disable_clk;
> >+		goto err_remove_notifier;
> >  	}
> >
> >  	return 0;
> >
> >+err_remove_notifier:
> >+	clk_notifier_unregister(clk, &cs->timer.clk_rate_cb);
> >  err_disable_clk:
> >  	clk_disable_unprepare(clk);
> >  err_free:
> >@@ -200,6 +238,26 @@ err_free:
> >  	return ret;
> >  }
> >
> >+static int sun5i_rate_cb_clkevt(struct notifier_block *nb,
> >+				unsigned long event, void *data)
> >+{
> >+	struct clk_notifier_data *ndata = data;
> >+	struct sun5i_timer *timer = to_sun5i_timer(nb);
> >+	struct sun5i_timer_clkevt *ce = container_of(timer,
> >+						     struct sun5i_timer_clkevt, timer);
> >+	unsigned long flags;
> >+
> >+	if (event == POST_RATE_CHANGE) {
> >+		local_irq_save(flags);
> >+		clockevents_update_freq(&ce->clkevt, ndata->new_rate);
> >+		local_irq_restore(flags);
> 
> local_irq_save and local_irq_restore are already in the
> clockevents_update_freq function.

Ok.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150126/f9c789d8/attachment-0001.sig>

Re: [PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 994 bytes --]

On Mon, Jan 26, 2015 at 03:35:41PM +0100, Maxime Ripard wrote:
> > >+static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
> > >+				unsigned long event, void *data)
> > >+{
> > >+	struct clk_notifier_data *ndata = data;
> > >+	struct sun5i_timer *timer = to_sun5i_timer(nb);
> > >+	struct sun5i_timer_clksrc *cs = container_of(timer,
> > >+						     struct sun5i_timer_clksrc, timer);
> > >+
> > >+	switch (event) {
> > >+	case PRE_RATE_CHANGE:
> > >+		clocksource_unregister(&cs->clksrc);
> > >+		break;
> > >+
> > >+	case POST_RATE_CHANGE:
> > >+		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
> > >+		break;
> > 
> > Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
> 
> Wouldn't that leave a (small, I agree) window where the timer would
> run at a rate different to the one it has been registered with?

Ping?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Maxime Ripard]

On Mon, Jan 26, 2015 at 03:35:41PM +0100, Maxime Ripard wrote:
> > >+static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
> > >+				unsigned long event, void *data)
> > >+{
> > >+	struct clk_notifier_data *ndata = data;
> > >+	struct sun5i_timer *timer = to_sun5i_timer(nb);
> > >+	struct sun5i_timer_clksrc *cs = container_of(timer,
> > >+						     struct sun5i_timer_clksrc, timer);
> > >+
> > >+	switch (event) {
> > >+	case PRE_RATE_CHANGE:
> > >+		clocksource_unregister(&cs->clksrc);
> > >+		break;
> > >+
> > >+	case POST_RATE_CHANGE:
> > >+		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
> > >+		break;
> > 
> > Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
> 
> Wouldn't that leave a (small, I agree) window where the timer would
> run at a rate different to the one it has been registered with?

Ping?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150303/8f71e32a/attachment-0001.sig>

Re: [PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Daniel Lezcano]

On 03/03/2015 09:52 AM, Maxime Ripard wrote:
> On Mon, Jan 26, 2015 at 03:35:41PM +0100, Maxime Ripard wrote:
>>>> +static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
>>>> +				unsigned long event, void *data)
>>>> +{
>>>> +	struct clk_notifier_data *ndata = data;
>>>> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
>>>> +	struct sun5i_timer_clksrc *cs = container_of(timer,
>>>> +						     struct sun5i_timer_clksrc, timer);
>>>> +
>>>> +	switch (event) {
>>>> +	case PRE_RATE_CHANGE:
>>>> +		clocksource_unregister(&cs->clksrc);
>>>> +		break;
>>>> +
>>>> +	case POST_RATE_CHANGE:
>>>> +		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
>>>> +		break;
>>>
>>> Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
>>
>> Wouldn't that leave a (small, I agree) window where the timer would
>> run at a rate different to the one it has been registered with?
>
> Ping?

Hi Maxime,

except I missed something, there were a couple of comments and I was 
waiting a V3.

   -- Daniel


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

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

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Daniel Lezcano]

On 03/03/2015 09:52 AM, Maxime Ripard wrote:
> On Mon, Jan 26, 2015 at 03:35:41PM +0100, Maxime Ripard wrote:
>>>> +static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
>>>> +				unsigned long event, void *data)
>>>> +{
>>>> +	struct clk_notifier_data *ndata = data;
>>>> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
>>>> +	struct sun5i_timer_clksrc *cs = container_of(timer,
>>>> +						     struct sun5i_timer_clksrc, timer);
>>>> +
>>>> +	switch (event) {
>>>> +	case PRE_RATE_CHANGE:
>>>> +		clocksource_unregister(&cs->clksrc);
>>>> +		break;
>>>> +
>>>> +	case POST_RATE_CHANGE:
>>>> +		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
>>>> +		break;
>>>
>>> Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
>>
>> Wouldn't that leave a (small, I agree) window where the timer would
>> run at a rate different to the one it has been registered with?
>
> Ping?

Hi Maxime,

except I missed something, there were a couple of comments and I was 
waiting a V3.

   -- Daniel


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

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

Re: [PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

On Tue, Mar 03, 2015 at 12:16:57PM +0100, Daniel Lezcano wrote:
> On 03/03/2015 09:52 AM, Maxime Ripard wrote:
> >On Mon, Jan 26, 2015 at 03:35:41PM +0100, Maxime Ripard wrote:
> >>>>+static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
> >>>>+				unsigned long event, void *data)
> >>>>+{
> >>>>+	struct clk_notifier_data *ndata = data;
> >>>>+	struct sun5i_timer *timer = to_sun5i_timer(nb);
> >>>>+	struct sun5i_timer_clksrc *cs = container_of(timer,
> >>>>+						     struct sun5i_timer_clksrc, timer);
> >>>>+
> >>>>+	switch (event) {
> >>>>+	case PRE_RATE_CHANGE:
> >>>>+		clocksource_unregister(&cs->clksrc);
> >>>>+		break;
> >>>>+
> >>>>+	case POST_RATE_CHANGE:
> >>>>+		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
> >>>>+		break;
> >>>
> >>>Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
> >>
> >>Wouldn't that leave a (small, I agree) window where the timer would
> >>run at a rate different to the one it has been registered with?
> >
> >Ping?
> 
> Hi Maxime,
> 
> except I missed something, there were a couple of comments and I was waiting
> a V3.

Indeed, but one of these comments (the one above) was still under
discussion.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Maxime Ripard]

On Tue, Mar 03, 2015 at 12:16:57PM +0100, Daniel Lezcano wrote:
> On 03/03/2015 09:52 AM, Maxime Ripard wrote:
> >On Mon, Jan 26, 2015 at 03:35:41PM +0100, Maxime Ripard wrote:
> >>>>+static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
> >>>>+				unsigned long event, void *data)
> >>>>+{
> >>>>+	struct clk_notifier_data *ndata = data;
> >>>>+	struct sun5i_timer *timer = to_sun5i_timer(nb);
> >>>>+	struct sun5i_timer_clksrc *cs = container_of(timer,
> >>>>+						     struct sun5i_timer_clksrc, timer);
> >>>>+
> >>>>+	switch (event) {
> >>>>+	case PRE_RATE_CHANGE:
> >>>>+		clocksource_unregister(&cs->clksrc);
> >>>>+		break;
> >>>>+
> >>>>+	case POST_RATE_CHANGE:
> >>>>+		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
> >>>>+		break;
> >>>
> >>>Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
> >>
> >>Wouldn't that leave a (small, I agree) window where the timer would
> >>run at a rate different to the one it has been registered with?
> >
> >Ping?
> 
> Hi Maxime,
> 
> except I missed something, there were a couple of comments and I was waiting
> a V3.

Indeed, but one of these comments (the one above) was still under
discussion.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150304/5fe478a3/attachment.sig>

Re: [PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Daniel Lezcano]

On 03/04/2015 10:32 AM, Maxime Ripard wrote:
> On Tue, Mar 03, 2015 at 12:16:57PM +0100, Daniel Lezcano wrote:
>> On 03/03/2015 09:52 AM, Maxime Ripard wrote:
>>> On Mon, Jan 26, 2015 at 03:35:41PM +0100, Maxime Ripard wrote:
>>>>>> +static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
>>>>>> +				unsigned long event, void *data)
>>>>>> +{
>>>>>> +	struct clk_notifier_data *ndata = data;
>>>>>> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
>>>>>> +	struct sun5i_timer_clksrc *cs = container_of(timer,
>>>>>> +						     struct sun5i_timer_clksrc, timer);
>>>>>> +
>>>>>> +	switch (event) {
>>>>>> +	case PRE_RATE_CHANGE:
>>>>>> +		clocksource_unregister(&cs->clksrc);
>>>>>> +		break;
>>>>>> +
>>>>>> +	case POST_RATE_CHANGE:
>>>>>> +		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
>>>>>> +		break;
>>>>>
>>>>> Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
>>>>
>>>> Wouldn't that leave a (small, I agree) window where the timer would
>>>> run at a rate different to the one it has been registered with?
>>>
>>> Ping?
>>
>> Hi Maxime,
>>
>> except I missed something, there were a couple of comments and I was waiting
>> a V3.
>
> Indeed, but one of these comments (the one above) was still under
> discussion.

Ah, yeah. Fair enough :)



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

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

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Daniel Lezcano]

On 03/04/2015 10:32 AM, Maxime Ripard wrote:
> On Tue, Mar 03, 2015 at 12:16:57PM +0100, Daniel Lezcano wrote:
>> On 03/03/2015 09:52 AM, Maxime Ripard wrote:
>>> On Mon, Jan 26, 2015 at 03:35:41PM +0100, Maxime Ripard wrote:
>>>>>> +static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
>>>>>> +				unsigned long event, void *data)
>>>>>> +{
>>>>>> +	struct clk_notifier_data *ndata = data;
>>>>>> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
>>>>>> +	struct sun5i_timer_clksrc *cs = container_of(timer,
>>>>>> +						     struct sun5i_timer_clksrc, timer);
>>>>>> +
>>>>>> +	switch (event) {
>>>>>> +	case PRE_RATE_CHANGE:
>>>>>> +		clocksource_unregister(&cs->clksrc);
>>>>>> +		break;
>>>>>> +
>>>>>> +	case POST_RATE_CHANGE:
>>>>>> +		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
>>>>>> +		break;
>>>>>
>>>>> Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
>>>>
>>>> Wouldn't that leave a (small, I agree) window where the timer would
>>>> run at a rate different to the one it has been registered with?
>>>
>>> Ping?
>>
>> Hi Maxime,
>>
>> except I missed something, there were a couple of comments and I was waiting
>> a V3.
>
> Indeed, but one of these comments (the one above) was still under
> discussion.

Ah, yeah. Fair enough :)



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

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

Re: [PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Daniel Lezcano]

On 01/26/2015 03:35 PM, Maxime Ripard wrote:
> Hi Daniel,
>
> On Mon, Jan 26, 2015 at 12:22:16PM +0100, Daniel Lezcano wrote:
>> On 01/26/2015 10:50 AM, Maxime Ripard wrote:
>>> The parent clock of the sun5i timer is the AHB clock, which rate might change
>>> because of other devices requirements.
>>>
>>> This is for example the case on the Allwinner A31, where the DMA controller
>>> needs a minimum rate higher than the default, that is enforced after the timer
>>> driver has probed.
>>>
>>> Add clock notifiers to make sure we reflect the clock rate changes in the timer
>>> rates.
>>
>> Mmh, I am wondering if that shouldn't go to the time framework ...
>>
>> Looking at the notifier callbacks that call generic time framework
>> functions.
>
> I don't think the framework currently has a clock handle so far?
>
>> Can you have a look at commit b3e90722 ? and double check your
>> changes ?
>
> I'm exactly in the opposite situation.
>
> The parent clock of this timer is *not* the CPU clock, and it might
> even be in a completely different clock tree.
>
> However, our DMA controller has the same parent clock than the timer,
> and it needs some frequency adjustement. Otherwise, DMA would just not
> work.
>
> So I'm completely fine with any rate change as far as the timer is
> concerned, the driver just need to be notified to be able to reflect
> this rate change.
>
>> A couple of comments below.
>>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>>   drivers/clocksource/timer-sun5i.c | 72 +++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 70 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
>>> index 377e50450781..5841a956f5f0 100644
>>> --- a/drivers/clocksource/timer-sun5i.c
>>> +++ b/drivers/clocksource/timer-sun5i.c
>>> @@ -41,9 +41,13 @@
>>>   struct sun5i_timer {
>>>   	void __iomem		*base;
>>>   	struct clk		*clk;
>>> +	struct notifier_block	clk_rate_cb;
>>>   	u32			ticks_per_jiffy;
>>>   };
>>>
>>> +#define to_sun5i_timer(x) \
>>> +	container_of(x, struct sun5i_timer, clk_rate_cb)
>>> +
>>>   struct sun5i_timer_clksrc {
>>>   	struct sun5i_timer	timer;
>>>   	struct clocksource	clksrc;
>>> @@ -152,6 +156,30 @@ static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
>>>   	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
>>>   }
>>>
>>> +static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
>>> +				unsigned long event, void *data)
>>> +{
>>> +	struct clk_notifier_data *ndata = data;
>>> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
>>> +	struct sun5i_timer_clksrc *cs = container_of(timer,
>>> +						     struct sun5i_timer_clksrc, timer);
>>> +
>>> +	switch (event) {
>>> +	case PRE_RATE_CHANGE:
>>> +		clocksource_unregister(&cs->clksrc);
>>> +		break;
>>> +
>>> +	case POST_RATE_CHANGE:
>>> +		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
>>> +		break;
>>
>> Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
>
> Wouldn't that leave a (small, I agree) window where the timer would
> run at a rate different to the one it has been registered with?

Mmh, yes, that's correct.

>>> +	default:
>>> +		break;
>>> +	}
>>> +
>>> +	return NOTIFY_DONE;
>>> +}
>>> +
>>>   static int __init sun5i_setup_clocksource(struct device_node *node,
>>>   					  void __iomem *base,
>>>   					  struct clk *clk, int irq)
>>> @@ -174,6 +202,14 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
>>>
>>>   	cs->timer.base = base;
>>>   	cs->timer.clk = clk;
>>> +	cs->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clksrc;
>>> +	cs->timer.clk_rate_cb.next = NULL;
>>> +
>>> +	ret = clk_notifier_register(clk, &cs->timer.clk_rate_cb);
>>> +	if (ret) {
>>> +		pr_err("Unable to register clock notifier.\n");
>>> +		goto err_disable_clk;
>>> +	}
>>>
>>>   	writel(~0, base + TIMER_INTVAL_LO_REG(1));
>>>   	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
>>> @@ -188,11 +224,13 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
>>>   	ret = clocksource_register_hz(&cs->clksrc, rate);
>>>   	if (ret) {
>>>   		pr_err("Couldn't register clock source.\n");
>>> -		goto err_disable_clk;
>>> +		goto err_remove_notifier;
>>>   	}
>>>
>>>   	return 0;
>>>
>>> +err_remove_notifier:
>>> +	clk_notifier_unregister(clk, &cs->timer.clk_rate_cb);
>>>   err_disable_clk:
>>>   	clk_disable_unprepare(clk);
>>>   err_free:
>>> @@ -200,6 +238,26 @@ err_free:
>>>   	return ret;
>>>   }
>>>
>>> +static int sun5i_rate_cb_clkevt(struct notifier_block *nb,
>>> +				unsigned long event, void *data)
>>> +{
>>> +	struct clk_notifier_data *ndata = data;
>>> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
>>> +	struct sun5i_timer_clkevt *ce = container_of(timer,
>>> +						     struct sun5i_timer_clkevt, timer);
>>> +	unsigned long flags;
>>> +
>>> +	if (event == POST_RATE_CHANGE) {
>>> +		local_irq_save(flags);
>>> +		clockevents_update_freq(&ce->clkevt, ndata->new_rate);
>>> +		local_irq_restore(flags);
>>
>> local_irq_save and local_irq_restore are already in the
>> clockevents_update_freq function.
>
> Ok.
>
> Thanks!
> Maxime
>


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

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

[PATCH v2 5/5] clocksource: sun5i: Add clock notifiers

[Daniel Lezcano]

On 01/26/2015 03:35 PM, Maxime Ripard wrote:
> Hi Daniel,
>
> On Mon, Jan 26, 2015 at 12:22:16PM +0100, Daniel Lezcano wrote:
>> On 01/26/2015 10:50 AM, Maxime Ripard wrote:
>>> The parent clock of the sun5i timer is the AHB clock, which rate might change
>>> because of other devices requirements.
>>>
>>> This is for example the case on the Allwinner A31, where the DMA controller
>>> needs a minimum rate higher than the default, that is enforced after the timer
>>> driver has probed.
>>>
>>> Add clock notifiers to make sure we reflect the clock rate changes in the timer
>>> rates.
>>
>> Mmh, I am wondering if that shouldn't go to the time framework ...
>>
>> Looking at the notifier callbacks that call generic time framework
>> functions.
>
> I don't think the framework currently has a clock handle so far?
>
>> Can you have a look at commit b3e90722 ? and double check your
>> changes ?
>
> I'm exactly in the opposite situation.
>
> The parent clock of this timer is *not* the CPU clock, and it might
> even be in a completely different clock tree.
>
> However, our DMA controller has the same parent clock than the timer,
> and it needs some frequency adjustement. Otherwise, DMA would just not
> work.
>
> So I'm completely fine with any rate change as far as the timer is
> concerned, the driver just need to be notified to be able to reflect
> this rate change.
>
>> A couple of comments below.
>>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>>   drivers/clocksource/timer-sun5i.c | 72 +++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 70 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-sun5i.c b/drivers/clocksource/timer-sun5i.c
>>> index 377e50450781..5841a956f5f0 100644
>>> --- a/drivers/clocksource/timer-sun5i.c
>>> +++ b/drivers/clocksource/timer-sun5i.c
>>> @@ -41,9 +41,13 @@
>>>   struct sun5i_timer {
>>>   	void __iomem		*base;
>>>   	struct clk		*clk;
>>> +	struct notifier_block	clk_rate_cb;
>>>   	u32			ticks_per_jiffy;
>>>   };
>>>
>>> +#define to_sun5i_timer(x) \
>>> +	container_of(x, struct sun5i_timer, clk_rate_cb)
>>> +
>>>   struct sun5i_timer_clksrc {
>>>   	struct sun5i_timer	timer;
>>>   	struct clocksource	clksrc;
>>> @@ -152,6 +156,30 @@ static cycle_t sun5i_clksrc_read(struct clocksource *clksrc)
>>>   	return ~readl(cs->timer.base + TIMER_CNTVAL_LO_REG(1));
>>>   }
>>>
>>> +static int sun5i_rate_cb_clksrc(struct notifier_block *nb,
>>> +				unsigned long event, void *data)
>>> +{
>>> +	struct clk_notifier_data *ndata = data;
>>> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
>>> +	struct sun5i_timer_clksrc *cs = container_of(timer,
>>> +						     struct sun5i_timer_clksrc, timer);
>>> +
>>> +	switch (event) {
>>> +	case PRE_RATE_CHANGE:
>>> +		clocksource_unregister(&cs->clksrc);
>>> +		break;
>>> +
>>> +	case POST_RATE_CHANGE:
>>> +		clocksource_register_hz(&cs->clksrc, ndata->new_rate);
>>> +		break;
>>
>> Why clocksource_unregister couldn't be in the POST_RATE_CHANGE ?
>
> Wouldn't that leave a (small, I agree) window where the timer would
> run at a rate different to the one it has been registered with?

Mmh, yes, that's correct.

>>> +	default:
>>> +		break;
>>> +	}
>>> +
>>> +	return NOTIFY_DONE;
>>> +}
>>> +
>>>   static int __init sun5i_setup_clocksource(struct device_node *node,
>>>   					  void __iomem *base,
>>>   					  struct clk *clk, int irq)
>>> @@ -174,6 +202,14 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
>>>
>>>   	cs->timer.base = base;
>>>   	cs->timer.clk = clk;
>>> +	cs->timer.clk_rate_cb.notifier_call = sun5i_rate_cb_clksrc;
>>> +	cs->timer.clk_rate_cb.next = NULL;
>>> +
>>> +	ret = clk_notifier_register(clk, &cs->timer.clk_rate_cb);
>>> +	if (ret) {
>>> +		pr_err("Unable to register clock notifier.\n");
>>> +		goto err_disable_clk;
>>> +	}
>>>
>>>   	writel(~0, base + TIMER_INTVAL_LO_REG(1));
>>>   	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
>>> @@ -188,11 +224,13 @@ static int __init sun5i_setup_clocksource(struct device_node *node,
>>>   	ret = clocksource_register_hz(&cs->clksrc, rate);
>>>   	if (ret) {
>>>   		pr_err("Couldn't register clock source.\n");
>>> -		goto err_disable_clk;
>>> +		goto err_remove_notifier;
>>>   	}
>>>
>>>   	return 0;
>>>
>>> +err_remove_notifier:
>>> +	clk_notifier_unregister(clk, &cs->timer.clk_rate_cb);
>>>   err_disable_clk:
>>>   	clk_disable_unprepare(clk);
>>>   err_free:
>>> @@ -200,6 +238,26 @@ err_free:
>>>   	return ret;
>>>   }
>>>
>>> +static int sun5i_rate_cb_clkevt(struct notifier_block *nb,
>>> +				unsigned long event, void *data)
>>> +{
>>> +	struct clk_notifier_data *ndata = data;
>>> +	struct sun5i_timer *timer = to_sun5i_timer(nb);
>>> +	struct sun5i_timer_clkevt *ce = container_of(timer,
>>> +						     struct sun5i_timer_clkevt, timer);
>>> +	unsigned long flags;
>>> +
>>> +	if (event == POST_RATE_CHANGE) {
>>> +		local_irq_save(flags);
>>> +		clockevents_update_freq(&ce->clkevt, ndata->new_rate);
>>> +		local_irq_restore(flags);
>>
>> local_irq_save and local_irq_restore are already in the
>> clockevents_update_freq function.
>
> Ok.
>
> Thanks!
> Maxime
>


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

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