[PATCH v1 0/3] Few more cleanups for tegra-timer

[PATCH v1 0/3] Few more cleanups for tegra-timer

[Dmitry Osipenko]

Hello,

I took a look at tegra-timer once again and spotted few more things that
could be improved in addition to [0] which is already in linux-next.

Please apply this small series in addition to [0], thanks in advance!

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=111529

Dmitry Osipenko (3):
  clocksource/drivers/tegra: Remove duplicated use of per_cpu_ptr
  clocksource/drivers/tegra: Set and use timer's period
  clocksource/drivers/tegra: Drop unneeded typecasting in one place

 drivers/clocksource/timer-tegra.c | 47 +++++++++++++++++++------------
 1 file changed, 29 insertions(+), 18 deletions(-)

-- 
2.21.0

[PATCH v1 1/3] clocksource/drivers/tegra: Remove duplicated use of per_cpu_ptr

[Dmitry Osipenko]

It was left unnoticed by accident, which means that the code could be
cleaned up a tad more.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clocksource/timer-tegra.c | 40 +++++++++++++++++++------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index 9406855781ff..6da169de47f9 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -216,6 +216,19 @@ static inline unsigned int tegra_irq_idx_for_cpu(int cpu, bool tegra20)
 	return TIMER10_IRQ_IDX + cpu;
 }
 
+static inline unsigned long tegra_rate_for_timer(struct timer_of *to,
+						 bool tegra20)
+{
+	/*
+	 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
+	 * parent clock.
+	 */
+	if (tegra20)
+		return 1000000;
+
+	return to->of_clk.rate;
+}
+
 static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 				   int rating)
 {
@@ -268,30 +281,27 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 
 	for_each_possible_cpu(cpu) {
 		struct timer_of *cpu_to = per_cpu_ptr(&tegra_to, cpu);
+		unsigned long flags = IRQF_TIMER | IRQF_NOBALANCING;
+		unsigned long rate = tegra_rate_for_timer(&tegra_to, tegra20);
 		unsigned int base = tegra_base_for_cpu(cpu, tegra20);
 		unsigned int idx = tegra_irq_idx_for_cpu(cpu, tegra20);
+		unsigned int irq = irq_of_parse_and_map(np, idx);
 
-		/*
-		 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
-		 * parent clock.
-		 */
-		if (tegra20)
-			cpu_to->of_clk.rate = 1000000;
-
-		cpu_to = per_cpu_ptr(&tegra_to, cpu);
-		cpu_to->of_base.base = timer_reg_base + base;
-		cpu_to->clkevt.rating = rating;
-		cpu_to->clkevt.cpumask = cpumask_of(cpu);
-		cpu_to->clkevt.irq = irq_of_parse_and_map(np, idx);
-		if (!cpu_to->clkevt.irq) {
+		if (!irq) {
 			pr_err("failed to map irq for cpu%d\n", cpu);
 			ret = -EINVAL;
 			goto out_irq;
 		}
 
+		cpu_to->clkevt.irq = irq;
+		cpu_to->clkevt.rating = rating;
+		cpu_to->clkevt.cpumask = cpumask_of(cpu);
+		cpu_to->of_base.base = timer_reg_base + base;
+		cpu_to->of_clk.rate = rate;
+
 		irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);
-		ret = request_irq(cpu_to->clkevt.irq, tegra_timer_isr,
-				  IRQF_TIMER | IRQF_NOBALANCING,
+
+		ret = request_irq(cpu_to->clkevt.irq, tegra_timer_isr, flags,
 				  cpu_to->clkevt.name, &cpu_to->clkevt);
 		if (ret) {
 			pr_err("failed to set up irq for cpu%d: %d\n",
-- 
2.21.0

Re: [PATCH v1 1/3] clocksource/drivers/tegra: Remove duplicated use of per_cpu_ptr

[Daniel Lezcano]

Hi Dmitry,


On 09/06/2019 21:27, Dmitry Osipenko wrote:
> It was left unnoticed by accident, which means that the code could be
> cleaned up a tad more.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clocksource/timer-tegra.c | 40 +++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index 9406855781ff..6da169de47f9 100644
> --- a/drivers/clocksource/timer-tegra.c
> +++ b/drivers/clocksource/timer-tegra.c
> @@ -216,6 +216,19 @@ static inline unsigned int tegra_irq_idx_for_cpu(int cpu, bool tegra20)
>  	return TIMER10_IRQ_IDX + cpu;
>  }
>  
> +static inline unsigned long tegra_rate_for_timer(struct timer_of *to,
> +						 bool tegra20)
> +{
> +	/*
> +	 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
> +	 * parent clock.
> +	 */
> +	if (tegra20)
> +		return 1000000;

Mind to take the opportunity to convert the literal value to a constant?

> +
> +	return to->of_clk.rate;
> +}
> +
>  static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>  				   int rating)
>  {
> @@ -268,30 +281,27 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>  
>  	for_each_possible_cpu(cpu) {
>  		struct timer_of *cpu_to = per_cpu_ptr(&tegra_to, cpu);
> +		unsigned long flags = IRQF_TIMER | IRQF_NOBALANCING;
> +		unsigned long rate = tegra_rate_for_timer(&tegra_to, tegra20);
>  		unsigned int base = tegra_base_for_cpu(cpu, tegra20);
>  		unsigned int idx = tegra_irq_idx_for_cpu(cpu, tegra20);
> +		unsigned int irq = irq_of_parse_and_map(np, idx);
>  
> -		/*
> -		 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
> -		 * parent clock.
> -		 */
> -		if (tegra20)
> -			cpu_to->of_clk.rate = 1000000;
> -
> -		cpu_to = per_cpu_ptr(&tegra_to, cpu);
> -		cpu_to->of_base.base = timer_reg_base + base;
> -		cpu_to->clkevt.rating = rating;
> -		cpu_to->clkevt.cpumask = cpumask_of(cpu);
> -		cpu_to->clkevt.irq = irq_of_parse_and_map(np, idx);
> -		if (!cpu_to->clkevt.irq) {
> +		if (!irq) {
>  			pr_err("failed to map irq for cpu%d\n", cpu);
>  			ret = -EINVAL;
>  			goto out_irq;
>  		}
>  
> +		cpu_to->clkevt.irq = irq;
> +		cpu_to->clkevt.rating = rating;
> +		cpu_to->clkevt.cpumask = cpumask_of(cpu);
> +		cpu_to->of_base.base = timer_reg_base + base;
> +		cpu_to->of_clk.rate = rate;
> +
>  		irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);
> -		ret = request_irq(cpu_to->clkevt.irq, tegra_timer_isr,
> -				  IRQF_TIMER | IRQF_NOBALANCING,
> +
> +		ret = request_irq(cpu_to->clkevt.irq, tegra_timer_isr, flags,
>  				  cpu_to->clkevt.name, &cpu_to->clkevt);
>  		if (ret) {
>  			pr_err("failed to set up irq for cpu%d: %d\n",
> 


-- 
 <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 v1 1/3] clocksource/drivers/tegra: Remove duplicated use of per_cpu_ptr

[Dmitry Osipenko]

10.06.2019 11:11, Daniel Lezcano пишет:
> 
> Hi Dmitry,
> 
> 
> On 09/06/2019 21:27, Dmitry Osipenko wrote:
>> It was left unnoticed by accident, which means that the code could be
>> cleaned up a tad more.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/clocksource/timer-tegra.c | 40 +++++++++++++++++++------------
>>  1 file changed, 25 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>> index 9406855781ff..6da169de47f9 100644
>> --- a/drivers/clocksource/timer-tegra.c
>> +++ b/drivers/clocksource/timer-tegra.c
>> @@ -216,6 +216,19 @@ static inline unsigned int tegra_irq_idx_for_cpu(int cpu, bool tegra20)
>>  	return TIMER10_IRQ_IDX + cpu;
>>  }
>>  
>> +static inline unsigned long tegra_rate_for_timer(struct timer_of *to,
>> +						 bool tegra20)
>> +{
>> +	/*
>> +	 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
>> +	 * parent clock.
>> +	 */
>> +	if (tegra20)
>> +		return 1000000;
> 
> Mind to take the opportunity to convert the literal value to a constant?

Sure!

>> +
>> +	return to->of_clk.rate;
>> +}
>> +
>>  static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>>  				   int rating)
>>  {
>> @@ -268,30 +281,27 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>>  
>>  	for_each_possible_cpu(cpu) {
>>  		struct timer_of *cpu_to = per_cpu_ptr(&tegra_to, cpu);
>> +		unsigned long flags = IRQF_TIMER | IRQF_NOBALANCING;
>> +		unsigned long rate = tegra_rate_for_timer(&tegra_to, tegra20);

Oh, this actually shall be (to, tegra20). Will correct this in v2!

>>  		unsigned int base = tegra_base_for_cpu(cpu, tegra20);
>>  		unsigned int idx = tegra_irq_idx_for_cpu(cpu, tegra20);
>> +		unsigned int irq = irq_of_parse_and_map(np, idx);
>>  
>> -		/*
>> -		 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
>> -		 * parent clock.
>> -		 */
>> -		if (tegra20)
>> -			cpu_to->of_clk.rate = 1000000;

I also spotted that there is a bug here that I introduced in a previous
patch. The of_clk.rate is initialized only for the first per-cpu
clocksource and then we need to replicate it for the rest of CPU's in a
case of T210. I'll add an explicit fixup for this.

[PATCH v1 2/3] clocksource/drivers/tegra: Set and use timer's period

[Dmitry Osipenko]

The of_clk structure has a period field that is set up initially by
timer_of_clk_init(), that period value need to be adjusted for a case of
TIMER1-9 that are running at a fixed rate that doesn't match the clock's
rate. Note that the period value is currently used only by some of the
clocksource drivers internally and hence this is just a minor cleanup
change that doesn't fix anything.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clocksource/timer-tegra.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index 6da169de47f9..089c2f51ed40 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -71,9 +71,9 @@ static int tegra_timer_shutdown(struct clock_event_device *evt)
 static int tegra_timer_set_periodic(struct clock_event_device *evt)
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
+	unsigned long period = timer_of_period(to_timer_of(evt));
 
-	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER |
-		       ((timer_of_rate(to_timer_of(evt)) / HZ) - 1),
+	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER | (period - 1),
 		       reg_base + TIMER_PTV);
 
 	return 0;
@@ -297,6 +297,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 		cpu_to->clkevt.rating = rating;
 		cpu_to->clkevt.cpumask = cpumask_of(cpu);
 		cpu_to->of_base.base = timer_reg_base + base;
+		cpu_to->of_clk.period = DIV_ROUND_UP(rate, HZ);
 		cpu_to->of_clk.rate = rate;
 
 		irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);
-- 
2.21.0

[PATCH v1 3/3] clocksource/drivers/tegra: Drop unneeded typecasting in one place

[Dmitry Osipenko]

There is no need to cast void because kernel allows to do that without
a warning message from a compiler.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clocksource/timer-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index 089c2f51ed40..c208908fa288 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -81,7 +81,7 @@ static int tegra_timer_set_periodic(struct clock_event_device *evt)
 
 static irqreturn_t tegra_timer_isr(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	struct clock_event_device *evt = dev_id;
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
 	writel_relaxed(TIMER_PCR_INTR_CLR, reg_base + TIMER_PCR);
-- 
2.21.0