[PATCH] clocksource/drivers/timer-imx-tpm: convert the driver to timer-of

[PATCH] clocksource/drivers/timer-imx-tpm: convert the driver to timer-of

[Anson Huang]

Convert the driver to use the timer_of helpers.
This allows to handle timer base, clock and irq using
common timer_of driver and it simplifies the code.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/clocksource/timer-imx-tpm.c | 136 +++++++++++++++---------------------
 1 file changed, 55 insertions(+), 81 deletions(-)

diff --git a/drivers/clocksource/timer-imx-tpm.c b/drivers/clocksource/timer-imx-tpm.c
index b7aa2b8..c3dd4d2 100644
--- a/drivers/clocksource/timer-imx-tpm.c
+++ b/drivers/clocksource/timer-imx-tpm.c
@@ -12,6 +12,8 @@
 #include <linux/of_irq.h>
 #include <linux/sched_clock.h>
 
+#include "timer-of.h"
+
 #define TPM_PARAM			0x4
 #define TPM_PARAM_WIDTH_SHIFT		16
 #define TPM_PARAM_WIDTH_MASK		(0xff << 16)
@@ -33,9 +35,7 @@
 #define TPM_C0V				0x24
 
 static int counter_width;
-static int rating;
 static void __iomem *timer_base;
-static struct clock_event_device clockevent_tpm;
 
 static inline void tpm_timer_disable(void)
 {
@@ -80,19 +80,6 @@ static u64 notrace tpm_read_sched_clock(void)
 	return tpm_read_counter();
 }
 
-static int __init tpm_clocksource_init(unsigned long rate)
-{
-	tpm_delay_timer.read_current_timer = &tpm_read_current_timer;
-	tpm_delay_timer.freq = rate;
-	register_current_timer_delay(&tpm_delay_timer);
-
-	sched_clock_register(tpm_read_sched_clock, counter_width, rate);
-
-	return clocksource_mmio_init(timer_base + TPM_CNT, "imx-tpm",
-				     rate, rating, counter_width,
-				     clocksource_mmio_readl_up);
-}
-
 static int tpm_set_next_event(unsigned long delta,
 				struct clock_event_device *evt)
 {
@@ -137,74 +124,77 @@ static irqreturn_t tpm_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static struct clock_event_device clockevent_tpm = {
-	.name			= "i.MX7ULP TPM Timer",
-	.features		= CLOCK_EVT_FEAT_ONESHOT,
-	.set_state_oneshot	= tpm_set_state_oneshot,
-	.set_next_event		= tpm_set_next_event,
-	.set_state_shutdown	= tpm_set_state_shutdown,
+static struct timer_of to_tpm = {
+	.flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
+	.clkevt = {
+		.name			= "i.MX7ULP TPM Timer",
+		.rating			= 200,
+		.features		= CLOCK_EVT_FEAT_ONESHOT,
+		.set_state_shutdown	= tpm_set_state_shutdown,
+		.set_state_oneshot	= tpm_set_state_oneshot,
+		.set_next_event		= tpm_set_next_event,
+		.cpumask		= cpu_possible_mask,
+	},
+	.of_irq = {
+		.handler		= tpm_timer_interrupt,
+		.flags			= IRQF_TIMER | IRQF_IRQPOLL,
+	},
 };
 
-static int __init tpm_clockevent_init(unsigned long rate, int irq)
+static int __init tpm_clocksource_init(void)
 {
-	int ret;
+	tpm_delay_timer.read_current_timer = &tpm_read_current_timer;
+	tpm_delay_timer.freq = timer_of_rate(&to_tpm) >> 3;
+	register_current_timer_delay(&tpm_delay_timer);
 
-	ret = request_irq(irq, tpm_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
-			  "i.MX7ULP TPM Timer", &clockevent_tpm);
+	sched_clock_register(tpm_read_sched_clock, counter_width,
+			     timer_of_rate(&to_tpm) >> 3);
 
-	clockevent_tpm.rating = rating;
-	clockevent_tpm.cpumask = cpumask_of(0);
-	clockevent_tpm.irq = irq;
-	clockevents_config_and_register(&clockevent_tpm, rate, 300,
-					GENMASK(counter_width - 1, 1));
+	return clocksource_mmio_init(timer_base + TPM_CNT,
+				     "imx-tpm",
+				     timer_of_rate(&to_tpm) >> 3,
+				     to_tpm.clkevt.rating,
+				     counter_width,
+				     clocksource_mmio_readl_up);
+}
 
-	return ret;
+static void __init tpm_clockevent_init(void)
+{
+	clockevents_config_and_register(&to_tpm.clkevt,
+					timer_of_rate(&to_tpm) >> 3,
+					300,
+					GENMASK(counter_width - 1,
+					1));
 }
 
 static int __init tpm_timer_init(struct device_node *np)
 {
-	struct clk *ipg, *per;
-	int irq, ret;
-	u32 rate;
-
-	timer_base = of_iomap(np, 0);
-	if (!timer_base) {
-		pr_err("tpm: failed to get base address\n");
-		return -ENXIO;
-	}
-
-	irq = irq_of_parse_and_map(np, 0);
-	if (!irq) {
-		pr_err("tpm: failed to get irq\n");
-		ret = -ENOENT;
-		goto err_iomap;
-	}
+	struct clk *ipg;
+	int ret;
 
 	ipg = of_clk_get_by_name(np, "ipg");
-	per = of_clk_get_by_name(np, "per");
-	if (IS_ERR(ipg) || IS_ERR(per)) {
-		pr_err("tpm: failed to get ipg or per clk\n");
-		ret = -ENODEV;
-		goto err_clk_get;
+	if (IS_ERR(ipg)) {
+		pr_err("tpm: failed to get ipg clk\n");
+		return -ENODEV;
 	}
-
 	/* enable clk before accessing registers */
 	ret = clk_prepare_enable(ipg);
 	if (ret) {
 		pr_err("tpm: ipg clock enable failed (%d)\n", ret);
-		goto err_clk_get;
+		clk_put(ipg);
+		return ret;
 	}
 
-	ret = clk_prepare_enable(per);
-	if (ret) {
-		pr_err("tpm: per clock enable failed (%d)\n", ret);
-		goto err_per_clk_enable;
-	}
+	ret = timer_of_init(np, &to_tpm);
+	if (ret)
+		return ret;
+
+	timer_base = timer_of_base(&to_tpm);
 
-	counter_width = (readl(timer_base + TPM_PARAM) & TPM_PARAM_WIDTH_MASK)
-		>> TPM_PARAM_WIDTH_SHIFT;
+	counter_width = (readl(timer_base + TPM_PARAM)
+		& TPM_PARAM_WIDTH_MASK) >> TPM_PARAM_WIDTH_SHIFT;
 	/* use rating 200 for 32-bit counter and 150 for 16-bit counter */
-	rating = counter_width == 0x20 ? 200 : 150;
+	to_tpm.clkevt.rating = counter_width == 0x20 ? 200 : 150;
 
 	/*
 	 * Initialize tpm module to a known state
@@ -229,29 +219,13 @@ static int __init tpm_timer_init(struct device_node *np)
 	writel(TPM_SC_CMOD_INC_PER_CNT |
 		(counter_width == 0x20 ?
 		TPM_SC_CMOD_DIV_DEFAULT : TPM_SC_CMOD_DIV_MAX),
-		     timer_base + TPM_SC);
+		timer_base + TPM_SC);
 
 	/* set MOD register to maximum for free running mode */
 	writel(GENMASK(counter_width - 1, 0), timer_base + TPM_MOD);
 
-	rate = clk_get_rate(per) >> 3;
-	ret = tpm_clocksource_init(rate);
-	if (ret)
-		goto err_per_clk_enable;
-
-	ret = tpm_clockevent_init(rate, irq);
-	if (ret)
-		goto err_per_clk_enable;
-
-	return 0;
+	tpm_clockevent_init();
 
-err_per_clk_enable:
-	clk_disable_unprepare(ipg);
-err_clk_get:
-	clk_put(per);
-	clk_put(ipg);
-err_iomap:
-	iounmap(timer_base);
-	return ret;
+	return tpm_clocksource_init();
 }
 TIMER_OF_DECLARE(imx7ulp, "fsl,imx7ulp-tpm", tpm_timer_init);
-- 
2.7.4

RE: [PATCH] clocksource/drivers/timer-imx-tpm: convert the driver to timer-of

[Anson Huang]

Gentle Ping...

Best Regards!
Anson Huang

> -----Original Message-----
> From: Anson Huang
> Sent: 2018年11月6日 13:16
> To: daniel.lezcano@linaro.org; tglx@linutronix.de;
> linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH] clocksource/drivers/timer-imx-tpm: convert the driver to
> timer-of
> 
> Convert the driver to use the timer_of helpers.
> This allows to handle timer base, clock and irq using common timer_of driver
> and it simplifies the code.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/clocksource/timer-imx-tpm.c | 136 +++++++++++++++---------------------
>  1 file changed, 55 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-imx-tpm.c
> b/drivers/clocksource/timer-imx-tpm.c
> index b7aa2b8..c3dd4d2 100644
> --- a/drivers/clocksource/timer-imx-tpm.c
> +++ b/drivers/clocksource/timer-imx-tpm.c
> @@ -12,6 +12,8 @@
>  #include <linux/of_irq.h>
>  #include <linux/sched_clock.h>
> 
> +#include "timer-of.h"
> +
>  #define TPM_PARAM			0x4
>  #define TPM_PARAM_WIDTH_SHIFT		16
>  #define TPM_PARAM_WIDTH_MASK		(0xff << 16)
> @@ -33,9 +35,7 @@
>  #define TPM_C0V				0x24
> 
>  static int counter_width;
> -static int rating;
>  static void __iomem *timer_base;
> -static struct clock_event_device clockevent_tpm;
> 
>  static inline void tpm_timer_disable(void)  { @@ -80,19 +80,6 @@ static
> u64 notrace tpm_read_sched_clock(void)
>  	return tpm_read_counter();
>  }
> 
> -static int __init tpm_clocksource_init(unsigned long rate) -{
> -	tpm_delay_timer.read_current_timer = &tpm_read_current_timer;
> -	tpm_delay_timer.freq = rate;
> -	register_current_timer_delay(&tpm_delay_timer);
> -
> -	sched_clock_register(tpm_read_sched_clock, counter_width, rate);
> -
> -	return clocksource_mmio_init(timer_base + TPM_CNT, "imx-tpm",
> -				     rate, rating, counter_width,
> -				     clocksource_mmio_readl_up);
> -}
> -
>  static int tpm_set_next_event(unsigned long delta,
>  				struct clock_event_device *evt)
>  {
> @@ -137,74 +124,77 @@ static irqreturn_t tpm_timer_interrupt(int irq, void
> *dev_id)
>  	return IRQ_HANDLED;
>  }
> 
> -static struct clock_event_device clockevent_tpm = {
> -	.name			= "i.MX7ULP TPM Timer",
> -	.features		= CLOCK_EVT_FEAT_ONESHOT,
> -	.set_state_oneshot	= tpm_set_state_oneshot,
> -	.set_next_event		= tpm_set_next_event,
> -	.set_state_shutdown	= tpm_set_state_shutdown,
> +static struct timer_of to_tpm = {
> +	.flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
> +	.clkevt = {
> +		.name			= "i.MX7ULP TPM Timer",
> +		.rating			= 200,
> +		.features		= CLOCK_EVT_FEAT_ONESHOT,
> +		.set_state_shutdown	= tpm_set_state_shutdown,
> +		.set_state_oneshot	= tpm_set_state_oneshot,
> +		.set_next_event		= tpm_set_next_event,
> +		.cpumask		= cpu_possible_mask,
> +	},
> +	.of_irq = {
> +		.handler		= tpm_timer_interrupt,
> +		.flags			= IRQF_TIMER | IRQF_IRQPOLL,
> +	},
>  };
> 
> -static int __init tpm_clockevent_init(unsigned long rate, int irq)
> +static int __init tpm_clocksource_init(void)
>  {
> -	int ret;
> +	tpm_delay_timer.read_current_timer = &tpm_read_current_timer;
> +	tpm_delay_timer.freq = timer_of_rate(&to_tpm) >> 3;
> +	register_current_timer_delay(&tpm_delay_timer);
> 
> -	ret = request_irq(irq, tpm_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
> -			  "i.MX7ULP TPM Timer", &clockevent_tpm);
> +	sched_clock_register(tpm_read_sched_clock, counter_width,
> +			     timer_of_rate(&to_tpm) >> 3);
> 
> -	clockevent_tpm.rating = rating;
> -	clockevent_tpm.cpumask = cpumask_of(0);
> -	clockevent_tpm.irq = irq;
> -	clockevents_config_and_register(&clockevent_tpm, rate, 300,
> -					GENMASK(counter_width - 1, 1));
> +	return clocksource_mmio_init(timer_base + TPM_CNT,
> +				     "imx-tpm",
> +				     timer_of_rate(&to_tpm) >> 3,
> +				     to_tpm.clkevt.rating,
> +				     counter_width,
> +				     clocksource_mmio_readl_up);
> +}
> 
> -	return ret;
> +static void __init tpm_clockevent_init(void) {
> +	clockevents_config_and_register(&to_tpm.clkevt,
> +					timer_of_rate(&to_tpm) >> 3,
> +					300,
> +					GENMASK(counter_width - 1,
> +					1));
>  }
> 
>  static int __init tpm_timer_init(struct device_node *np)  {
> -	struct clk *ipg, *per;
> -	int irq, ret;
> -	u32 rate;
> -
> -	timer_base = of_iomap(np, 0);
> -	if (!timer_base) {
> -		pr_err("tpm: failed to get base address\n");
> -		return -ENXIO;
> -	}
> -
> -	irq = irq_of_parse_and_map(np, 0);
> -	if (!irq) {
> -		pr_err("tpm: failed to get irq\n");
> -		ret = -ENOENT;
> -		goto err_iomap;
> -	}
> +	struct clk *ipg;
> +	int ret;
> 
>  	ipg = of_clk_get_by_name(np, "ipg");
> -	per = of_clk_get_by_name(np, "per");
> -	if (IS_ERR(ipg) || IS_ERR(per)) {
> -		pr_err("tpm: failed to get ipg or per clk\n");
> -		ret = -ENODEV;
> -		goto err_clk_get;
> +	if (IS_ERR(ipg)) {
> +		pr_err("tpm: failed to get ipg clk\n");
> +		return -ENODEV;
>  	}
> -
>  	/* enable clk before accessing registers */
>  	ret = clk_prepare_enable(ipg);
>  	if (ret) {
>  		pr_err("tpm: ipg clock enable failed (%d)\n", ret);
> -		goto err_clk_get;
> +		clk_put(ipg);
> +		return ret;
>  	}
> 
> -	ret = clk_prepare_enable(per);
> -	if (ret) {
> -		pr_err("tpm: per clock enable failed (%d)\n", ret);
> -		goto err_per_clk_enable;
> -	}
> +	ret = timer_of_init(np, &to_tpm);
> +	if (ret)
> +		return ret;
> +
> +	timer_base = timer_of_base(&to_tpm);
> 
> -	counter_width = (readl(timer_base + TPM_PARAM) &
> TPM_PARAM_WIDTH_MASK)
> -		>> TPM_PARAM_WIDTH_SHIFT;
> +	counter_width = (readl(timer_base + TPM_PARAM)
> +		& TPM_PARAM_WIDTH_MASK) >> TPM_PARAM_WIDTH_SHIFT;
>  	/* use rating 200 for 32-bit counter and 150 for 16-bit counter */
> -	rating = counter_width == 0x20 ? 200 : 150;
> +	to_tpm.clkevt.rating = counter_width == 0x20 ? 200 : 150;
> 
>  	/*
>  	 * Initialize tpm module to a known state @@ -229,29 +219,13 @@
> static int __init tpm_timer_init(struct device_node *np)
>  	writel(TPM_SC_CMOD_INC_PER_CNT |
>  		(counter_width == 0x20 ?
>  		TPM_SC_CMOD_DIV_DEFAULT : TPM_SC_CMOD_DIV_MAX),
> -		     timer_base + TPM_SC);
> +		timer_base + TPM_SC);
> 
>  	/* set MOD register to maximum for free running mode */
>  	writel(GENMASK(counter_width - 1, 0), timer_base + TPM_MOD);
> 
> -	rate = clk_get_rate(per) >> 3;
> -	ret = tpm_clocksource_init(rate);
> -	if (ret)
> -		goto err_per_clk_enable;
> -
> -	ret = tpm_clockevent_init(rate, irq);
> -	if (ret)
> -		goto err_per_clk_enable;
> -
> -	return 0;
> +	tpm_clockevent_init();
> 
> -err_per_clk_enable:
> -	clk_disable_unprepare(ipg);
> -err_clk_get:
> -	clk_put(per);
> -	clk_put(ipg);
> -err_iomap:
> -	iounmap(timer_base);
> -	return ret;
> +	return tpm_clocksource_init();
>  }
>  TIMER_OF_DECLARE(imx7ulp, "fsl,imx7ulp-tpm", tpm_timer_init);
> --
> 2.7.4

Re: [PATCH] clocksource/drivers/timer-imx-tpm: convert the driver to timer-of

[Daniel Lezcano]

Hi Anson,


On 06/11/2018 06:15, Anson Huang wrote:
> Convert the driver to use the timer_of helpers.
> This allows to handle timer base, clock and irq using
> common timer_of driver and it simplifies the code.

Can you do an extra move by passing the timer_of structure around for:
 - tpm_timer_disable
 - tpm_timer_enable
 - tpm_irq_acknowledge

and use to_timer_of(clkevt) to retrieve the timer_base value for other
clockevent callbacks ?

The clockevent code will be self-encapsulated.



> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/clocksource/timer-imx-tpm.c | 136 +++++++++++++++---------------------
>  1 file changed, 55 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-imx-tpm.c b/drivers/clocksource/timer-imx-tpm.c
> index b7aa2b8..c3dd4d2 100644
> --- a/drivers/clocksource/timer-imx-tpm.c
> +++ b/drivers/clocksource/timer-imx-tpm.c
> @@ -12,6 +12,8 @@
>  #include <linux/of_irq.h>
>  #include <linux/sched_clock.h>
>  
> +#include "timer-of.h"
> +
>  #define TPM_PARAM			0x4
>  #define TPM_PARAM_WIDTH_SHIFT		16
>  #define TPM_PARAM_WIDTH_MASK		(0xff << 16)
> @@ -33,9 +35,7 @@
>  #define TPM_C0V				0x24
>  
>  static int counter_width;
> -static int rating;
>  static void __iomem *timer_base;
> -static struct clock_event_device clockevent_tpm;
>  
>  static inline void tpm_timer_disable(void)
>  {
> @@ -80,19 +80,6 @@ static u64 notrace tpm_read_sched_clock(void)
>  	return tpm_read_counter();
>  }
>  
> -static int __init tpm_clocksource_init(unsigned long rate)
> -{
> -	tpm_delay_timer.read_current_timer = &tpm_read_current_timer;
> -	tpm_delay_timer.freq = rate;
> -	register_current_timer_delay(&tpm_delay_timer);
> -
> -	sched_clock_register(tpm_read_sched_clock, counter_width, rate);
> -
> -	return clocksource_mmio_init(timer_base + TPM_CNT, "imx-tpm",
> -				     rate, rating, counter_width,
> -				     clocksource_mmio_readl_up);
> -}
> -
>  static int tpm_set_next_event(unsigned long delta,
>  				struct clock_event_device *evt)
>  {
> @@ -137,74 +124,77 @@ static irqreturn_t tpm_timer_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static struct clock_event_device clockevent_tpm = {
> -	.name			= "i.MX7ULP TPM Timer",
> -	.features		= CLOCK_EVT_FEAT_ONESHOT,
> -	.set_state_oneshot	= tpm_set_state_oneshot,
> -	.set_next_event		= tpm_set_next_event,
> -	.set_state_shutdown	= tpm_set_state_shutdown,
> +static struct timer_of to_tpm = {
> +	.flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
> +	.clkevt = {
> +		.name			= "i.MX7ULP TPM Timer",
> +		.rating			= 200,
> +		.features		= CLOCK_EVT_FEAT_ONESHOT,
> +		.set_state_shutdown	= tpm_set_state_shutdown,
> +		.set_state_oneshot	= tpm_set_state_oneshot,
> +		.set_next_event		= tpm_set_next_event,
> +		.cpumask		= cpu_possible_mask,
> +	},
> +	.of_irq = {
> +		.handler		= tpm_timer_interrupt,
> +		.flags			= IRQF_TIMER | IRQF_IRQPOLL,
> +	},
>  };
>  
> -static int __init tpm_clockevent_init(unsigned long rate, int irq)
> +static int __init tpm_clocksource_init(void)
>  {
> -	int ret;
> +	tpm_delay_timer.read_current_timer = &tpm_read_current_timer;
> +	tpm_delay_timer.freq = timer_of_rate(&to_tpm) >> 3;
> +	register_current_timer_delay(&tpm_delay_timer);
>  
> -	ret = request_irq(irq, tpm_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
> -			  "i.MX7ULP TPM Timer", &clockevent_tpm);
> +	sched_clock_register(tpm_read_sched_clock, counter_width,
> +			     timer_of_rate(&to_tpm) >> 3);
>  
> -	clockevent_tpm.rating = rating;
> -	clockevent_tpm.cpumask = cpumask_of(0);
> -	clockevent_tpm.irq = irq;
> -	clockevents_config_and_register(&clockevent_tpm, rate, 300,
> -					GENMASK(counter_width - 1, 1));
> +	return clocksource_mmio_init(timer_base + TPM_CNT,
> +				     "imx-tpm",
> +				     timer_of_rate(&to_tpm) >> 3,
> +				     to_tpm.clkevt.rating,
> +				     counter_width,
> +				     clocksource_mmio_readl_up);
> +}
>  
> -	return ret;
> +static void __init tpm_clockevent_init(void)
> +{
> +	clockevents_config_and_register(&to_tpm.clkevt,
> +					timer_of_rate(&to_tpm) >> 3,
> +					300,
> +					GENMASK(counter_width - 1,
> +					1));
>  }
>  
>  static int __init tpm_timer_init(struct device_node *np)
>  {
> -	struct clk *ipg, *per;
> -	int irq, ret;
> -	u32 rate;
> -
> -	timer_base = of_iomap(np, 0);
> -	if (!timer_base) {
> -		pr_err("tpm: failed to get base address\n");
> -		return -ENXIO;
> -	}
> -
> -	irq = irq_of_parse_and_map(np, 0);
> -	if (!irq) {
> -		pr_err("tpm: failed to get irq\n");
> -		ret = -ENOENT;
> -		goto err_iomap;
> -	}
> +	struct clk *ipg;
> +	int ret;
>  
>  	ipg = of_clk_get_by_name(np, "ipg");
> -	per = of_clk_get_by_name(np, "per");
> -	if (IS_ERR(ipg) || IS_ERR(per)) {
> -		pr_err("tpm: failed to get ipg or per clk\n");
> -		ret = -ENODEV;
> -		goto err_clk_get;
> +	if (IS_ERR(ipg)) {
> +		pr_err("tpm: failed to get ipg clk\n");
> +		return -ENODEV;
>  	}
> -
>  	/* enable clk before accessing registers */
>  	ret = clk_prepare_enable(ipg);
>  	if (ret) {
>  		pr_err("tpm: ipg clock enable failed (%d)\n", ret);
> -		goto err_clk_get;
> +		clk_put(ipg);
> +		return ret;
>  	}
>  
> -	ret = clk_prepare_enable(per);
> -	if (ret) {
> -		pr_err("tpm: per clock enable failed (%d)\n", ret);
> -		goto err_per_clk_enable;
> -	}
> +	ret = timer_of_init(np, &to_tpm);
> +	if (ret)
> +		return ret;
> +
> +	timer_base = timer_of_base(&to_tpm);
>  
> -	counter_width = (readl(timer_base + TPM_PARAM) & TPM_PARAM_WIDTH_MASK)
> -		>> TPM_PARAM_WIDTH_SHIFT;
> +	counter_width = (readl(timer_base + TPM_PARAM)
> +		& TPM_PARAM_WIDTH_MASK) >> TPM_PARAM_WIDTH_SHIFT;
>  	/* use rating 200 for 32-bit counter and 150 for 16-bit counter */
> -	rating = counter_width == 0x20 ? 200 : 150;
> +	to_tpm.clkevt.rating = counter_width == 0x20 ? 200 : 150;
>  
>  	/*
>  	 * Initialize tpm module to a known state
> @@ -229,29 +219,13 @@ static int __init tpm_timer_init(struct device_node *np)
>  	writel(TPM_SC_CMOD_INC_PER_CNT |
>  		(counter_width == 0x20 ?
>  		TPM_SC_CMOD_DIV_DEFAULT : TPM_SC_CMOD_DIV_MAX),
> -		     timer_base + TPM_SC);
> +		timer_base + TPM_SC);
>  
>  	/* set MOD register to maximum for free running mode */
>  	writel(GENMASK(counter_width - 1, 0), timer_base + TPM_MOD);
>  
> -	rate = clk_get_rate(per) >> 3;
> -	ret = tpm_clocksource_init(rate);
> -	if (ret)
> -		goto err_per_clk_enable;
> -
> -	ret = tpm_clockevent_init(rate, irq);
> -	if (ret)
> -		goto err_per_clk_enable;
> -
> -	return 0;
> +	tpm_clockevent_init();
>  
> -err_per_clk_enable:
> -	clk_disable_unprepare(ipg);
> -err_clk_get:
> -	clk_put(per);
> -	clk_put(ipg);
> -err_iomap:
> -	iounmap(timer_base);
> -	return ret;
> +	return tpm_clocksource_init();
>  }
>  TIMER_OF_DECLARE(imx7ulp, "fsl,imx7ulp-tpm", tpm_timer_init);
> 


-- 
 <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] clocksource/drivers/timer-imx-tpm: convert the driver to timer-of

[Anson Huang]

Hi, Daniel

Best Regards!
Anson Huang

> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: 2018年11月22日 21:21
> To: Anson Huang <anson.huang@nxp.com>; tglx@linutronix.de;
> linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] clocksource/drivers/timer-imx-tpm: convert the driver to
> timer-of
> 
> 
> Hi Anson,
> 
> 
> On 06/11/2018 06:15, Anson Huang wrote:
> > Convert the driver to use the timer_of helpers.
> > This allows to handle timer base, clock and irq using common timer_of
> > driver and it simplifies the code.
> 
> Can you do an extra move by passing the timer_of structure around for:
>  - tpm_timer_disable
>  - tpm_timer_enable
>  - tpm_irq_acknowledge
> 
> and use to_timer_of(clkevt) to retrieve the timer_base value for other
> clockevent callbacks ?
> 
> The clockevent code will be self-encapsulated.
> 

I remove below inline function, and put the register operation in the clockevent callback using
to_timer_of(clkevt) and timer_of_base(), it make the code more simplified.
	tpm_timer_disable
	tpm_timer_enable
	tpm_irq_acknowledge

For other functions like tpm_read_counter(), if using timer_of_base() to retrieve the timer base, it
needs to_tpm to be defined before this function, but to_tpm need to refer to some clockevent callbacks
which calls tpm_read_counter(), so the function definition sequence is conflict unless we add an function
statement, so I prefer to leave it as what it is now. Please help review V2 patch, thanks.

Anson.

> 
> 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/clocksource/timer-imx-tpm.c | 136
> > +++++++++++++++---------------------
> >  1 file changed, 55 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-imx-tpm.c
> > b/drivers/clocksource/timer-imx-tpm.c
> > index b7aa2b8..c3dd4d2 100644
> > --- a/drivers/clocksource/timer-imx-tpm.c
> > +++ b/drivers/clocksource/timer-imx-tpm.c
> > @@ -12,6 +12,8 @@
> >  #include <linux/of_irq.h>
> >  #include <linux/sched_clock.h>
> >
> > +#include "timer-of.h"
> > +
> >  #define TPM_PARAM			0x4
> >  #define TPM_PARAM_WIDTH_SHIFT		16
> >  #define TPM_PARAM_WIDTH_MASK		(0xff << 16)
> > @@ -33,9 +35,7 @@
> >  #define TPM_C0V				0x24
> >
> >  static int counter_width;
> > -static int rating;
> >  static void __iomem *timer_base;
> > -static struct clock_event_device clockevent_tpm;
> >
> >  static inline void tpm_timer_disable(void)  { @@ -80,19 +80,6 @@
> > static u64 notrace tpm_read_sched_clock(void)
> >  	return tpm_read_counter();
> >  }
> >
> > -static int __init tpm_clocksource_init(unsigned long rate) -{
> > -	tpm_delay_timer.read_current_timer = &tpm_read_current_timer;
> > -	tpm_delay_timer.freq = rate;
> > -	register_current_timer_delay(&tpm_delay_timer);
> > -
> > -	sched_clock_register(tpm_read_sched_clock, counter_width, rate);
> > -
> > -	return clocksource_mmio_init(timer_base + TPM_CNT, "imx-tpm",
> > -				     rate, rating, counter_width,
> > -				     clocksource_mmio_readl_up);
> > -}
> > -
> >  static int tpm_set_next_event(unsigned long delta,
> >  				struct clock_event_device *evt)
> >  {
> > @@ -137,74 +124,77 @@ static irqreturn_t tpm_timer_interrupt(int irq,
> void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >
> > -static struct clock_event_device clockevent_tpm = {
> > -	.name			= "i.MX7ULP TPM Timer",
> > -	.features		= CLOCK_EVT_FEAT_ONESHOT,
> > -	.set_state_oneshot	= tpm_set_state_oneshot,
> > -	.set_next_event		= tpm_set_next_event,
> > -	.set_state_shutdown	= tpm_set_state_shutdown,
> > +static struct timer_of to_tpm = {
> > +	.flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,
> > +	.clkevt = {
> > +		.name			= "i.MX7ULP TPM Timer",
> > +		.rating			= 200,
> > +		.features		= CLOCK_EVT_FEAT_ONESHOT,
> > +		.set_state_shutdown	= tpm_set_state_shutdown,
> > +		.set_state_oneshot	= tpm_set_state_oneshot,
> > +		.set_next_event		= tpm_set_next_event,
> > +		.cpumask		= cpu_possible_mask,
> > +	},
> > +	.of_irq = {
> > +		.handler		= tpm_timer_interrupt,
> > +		.flags			= IRQF_TIMER | IRQF_IRQPOLL,
> > +	},
> >  };
> >
> > -static int __init tpm_clockevent_init(unsigned long rate, int irq)
> > +static int __init tpm_clocksource_init(void)
> >  {
> > -	int ret;
> > +	tpm_delay_timer.read_current_timer = &tpm_read_current_timer;
> > +	tpm_delay_timer.freq = timer_of_rate(&to_tpm) >> 3;
> > +	register_current_timer_delay(&tpm_delay_timer);
> >
> > -	ret = request_irq(irq, tpm_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
> > -			  "i.MX7ULP TPM Timer", &clockevent_tpm);
> > +	sched_clock_register(tpm_read_sched_clock, counter_width,
> > +			     timer_of_rate(&to_tpm) >> 3);
> >
> > -	clockevent_tpm.rating = rating;
> > -	clockevent_tpm.cpumask = cpumask_of(0);
> > -	clockevent_tpm.irq = irq;
> > -	clockevents_config_and_register(&clockevent_tpm, rate, 300,
> > -					GENMASK(counter_width - 1, 1));
> > +	return clocksource_mmio_init(timer_base + TPM_CNT,
> > +				     "imx-tpm",
> > +				     timer_of_rate(&to_tpm) >> 3,
> > +				     to_tpm.clkevt.rating,
> > +				     counter_width,
> > +				     clocksource_mmio_readl_up);
> > +}
> >
> > -	return ret;
> > +static void __init tpm_clockevent_init(void) {
> > +	clockevents_config_and_register(&to_tpm.clkevt,
> > +					timer_of_rate(&to_tpm) >> 3,
> > +					300,
> > +					GENMASK(counter_width - 1,
> > +					1));
> >  }
> >
> >  static int __init tpm_timer_init(struct device_node *np)  {
> > -	struct clk *ipg, *per;
> > -	int irq, ret;
> > -	u32 rate;
> > -
> > -	timer_base = of_iomap(np, 0);
> > -	if (!timer_base) {
> > -		pr_err("tpm: failed to get base address\n");
> > -		return -ENXIO;
> > -	}
> > -
> > -	irq = irq_of_parse_and_map(np, 0);
> > -	if (!irq) {
> > -		pr_err("tpm: failed to get irq\n");
> > -		ret = -ENOENT;
> > -		goto err_iomap;
> > -	}
> > +	struct clk *ipg;
> > +	int ret;
> >
> >  	ipg = of_clk_get_by_name(np, "ipg");
> > -	per = of_clk_get_by_name(np, "per");
> > -	if (IS_ERR(ipg) || IS_ERR(per)) {
> > -		pr_err("tpm: failed to get ipg or per clk\n");
> > -		ret = -ENODEV;
> > -		goto err_clk_get;
> > +	if (IS_ERR(ipg)) {
> > +		pr_err("tpm: failed to get ipg clk\n");
> > +		return -ENODEV;
> >  	}
> > -
> >  	/* enable clk before accessing registers */
> >  	ret = clk_prepare_enable(ipg);
> >  	if (ret) {
> >  		pr_err("tpm: ipg clock enable failed (%d)\n", ret);
> > -		goto err_clk_get;
> > +		clk_put(ipg);
> > +		return ret;
> >  	}
> >
> > -	ret = clk_prepare_enable(per);
> > -	if (ret) {
> > -		pr_err("tpm: per clock enable failed (%d)\n", ret);
> > -		goto err_per_clk_enable;
> > -	}
> > +	ret = timer_of_init(np, &to_tpm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	timer_base = timer_of_base(&to_tpm);
> >
> > -	counter_width = (readl(timer_base + TPM_PARAM) &
> TPM_PARAM_WIDTH_MASK)
> > -		>> TPM_PARAM_WIDTH_SHIFT;
> > +	counter_width = (readl(timer_base + TPM_PARAM)
> > +		& TPM_PARAM_WIDTH_MASK) >> TPM_PARAM_WIDTH_SHIFT;
> >  	/* use rating 200 for 32-bit counter and 150 for 16-bit counter */
> > -	rating = counter_width == 0x20 ? 200 : 150;
> > +	to_tpm.clkevt.rating = counter_width == 0x20 ? 200 : 150;
> >
> >  	/*
> >  	 * Initialize tpm module to a known state @@ -229,29 +219,13 @@
> > static int __init tpm_timer_init(struct device_node *np)
> >  	writel(TPM_SC_CMOD_INC_PER_CNT |
> >  		(counter_width == 0x20 ?
> >  		TPM_SC_CMOD_DIV_DEFAULT : TPM_SC_CMOD_DIV_MAX),
> > -		     timer_base + TPM_SC);
> > +		timer_base + TPM_SC);
> >
> >  	/* set MOD register to maximum for free running mode */
> >  	writel(GENMASK(counter_width - 1, 0), timer_base + TPM_MOD);
> >
> > -	rate = clk_get_rate(per) >> 3;
> > -	ret = tpm_clocksource_init(rate);
> > -	if (ret)
> > -		goto err_per_clk_enable;
> > -
> > -	ret = tpm_clockevent_init(rate, irq);
> > -	if (ret)
> > -		goto err_per_clk_enable;
> > -
> > -	return 0;
> > +	tpm_clockevent_init();
> >
> > -err_per_clk_enable:
> > -	clk_disable_unprepare(ipg);
> > -err_clk_get:
> > -	clk_put(per);
> > -	clk_put(ipg);
> > -err_iomap:
> > -	iounmap(timer_base);
> > -	return ret;
> > +	return tpm_clocksource_init();
> >  }
> >  TIMER_OF_DECLARE(imx7ulp, "fsl,imx7ulp-tpm", tpm_timer_init);
> >
> 
> 
> --
> 
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .linaro.org%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C5ed9d8
> 83d3734ba4140d08d6507d6464%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636784896822292291&amp;sdata=XOymB9k6maxvJhG4%2F2yj
> jBP3YmbdY18mUnFoP9YwKqg%3D&amp;reserved=0> Linaro.org │ Open
> source software for ARM SoCs
> 
> Follow Linaro:
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7Canson.huang%40
> nxp.com%7C5ed9d883d3734ba4140d08d6507d6464%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C636784896822292291&amp;sdata=xUKFR2
> 6E90IoKaRZP5nSk82kn3UT8F5zsBV2cJVfmcs%3D&amp;reserved=0> Facebook
> |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitt
> er.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7Canson.huang%40nxp.c
> om%7C5ed9d883d3734ba4140d08d6507d6464%7C686ea1d3bc2b4c6fa92cd
> 99c5c301635%7C0%7C0%7C636784896822292291&amp;sdata=3iesM9aEJZ0
> QNqcfIdX%2Fe8dgah1SWJaHxIdRQlAqelg%3D&amp;reserved=0> Twitter |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .linaro.org%2Flinaro-blog%2F&amp;data=02%7C01%7Canson.huang%40nxp.c
> om%7C5ed9d883d3734ba4140d08d6507d6464%7C686ea1d3bc2b4c6fa92cd
> 99c5c301635%7C0%7C0%7C636784896822292291&amp;sdata=6e%2FyD7wj
> dyLvmRnOe6G0YlnUr8yQx2z2oniVOsRMmOo%3D&amp;reserved=0> Blog