[PATCH v2 0/6] Few more cleanups for tegra-timer

[PATCH v2 0/6] Few more cleanups for tegra-timer

[Dmitry Osipenko]

Hello,

This a followup to [0] that includes some more fixes and further
prettifies the driver's code.

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

Changelog:

v2: Fixed a bug that was introduced by [0] in a newly added patch:
    "Restore timer rate on Tegra210".

    Fixed potential problem in regards to error handling in another new
    patch: "Restore base address before cleanup".

    Added new patch "Add verbose definition for 1MHz constant" as per
    Daniel's Lezcano recommendation.

    Fixed a code typo that was made in "Remove duplicated use of per_cpu_ptr"
    of v1.

Dmitry Osipenko (6):
  clocksource/drivers/tegra: Restore timer rate on Tegra210
  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
  clocksource/drivers/tegra: Add verbose definition for 1MHz constant
  clocksource/drivers/tegra: Restore base address before cleanup

 drivers/clocksource/timer-tegra.c | 59 +++++++++++++++++++------------
 1 file changed, 37 insertions(+), 22 deletions(-)

-- 
2.21.0

[PATCH v2 1/6] clocksource/drivers/tegra: Restore timer rate on Tegra210

[Dmitry Osipenko]

The clocksource rate is initialized only for the first per-CPU clocksource
and then that rate shall be replicated for the rest of clocksource's
because they are initialized manually in the code.

Fixes: 3be2a85a0b61 ("Support per-CPU timers on all Tegra's")
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clocksource/timer-tegra.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index 9406855781ff..830c66e2d927 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -277,6 +277,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 		 */
 		if (tegra20)
 			cpu_to->of_clk.rate = 1000000;
+		else
+			cpu_to->of_clk.rate = timer_of_rate(to);
 
 		cpu_to = per_cpu_ptr(&tegra_to, cpu);
 		cpu_to->of_base.base = timer_reg_base + base;
-- 
2.21.0

Re: [PATCH v2 1/6] clocksource/drivers/tegra: Restore timer rate on Tegra210

[Jon Hunter]

On 10/06/2019 17:43, Dmitry Osipenko wrote:
> The clocksource rate is initialized only for the first per-CPU clocksource
> and then that rate shall be replicated for the rest of clocksource's
> because they are initialized manually in the code.
> 
> Fixes: 3be2a85a0b61 ("Support per-CPU timers on all Tegra's")
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clocksource/timer-tegra.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index 9406855781ff..830c66e2d927 100644
> --- a/drivers/clocksource/timer-tegra.c
> +++ b/drivers/clocksource/timer-tegra.c
> @@ -277,6 +277,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>  		 */
>  		if (tegra20)
>  			cpu_to->of_clk.rate = 1000000;
> +		else
> +			cpu_to->of_clk.rate = timer_of_rate(to);
>  
>  		cpu_to = per_cpu_ptr(&tegra_to, cpu);
>  		cpu_to->of_base.base = timer_reg_base + base;

Thanks. This fixes a boot regression we are seeing on -next with
Tegra210 (introduced by the commit referenced above). So ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

Re: [PATCH v2 1/6] clocksource/drivers/tegra: Restore timer rate on Tegra210

[Dmitry Osipenko]

12.06.2019 11:30, Jon Hunter пишет:
> 
> On 10/06/2019 17:43, Dmitry Osipenko wrote:
>> The clocksource rate is initialized only for the first per-CPU clocksource
>> and then that rate shall be replicated for the rest of clocksource's
>> because they are initialized manually in the code.
>>
>> Fixes: 3be2a85a0b61 ("Support per-CPU timers on all Tegra's")
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/clocksource/timer-tegra.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>> index 9406855781ff..830c66e2d927 100644
>> --- a/drivers/clocksource/timer-tegra.c
>> +++ b/drivers/clocksource/timer-tegra.c
>> @@ -277,6 +277,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>>  		 */
>>  		if (tegra20)
>>  			cpu_to->of_clk.rate = 1000000;
>> +		else
>> +			cpu_to->of_clk.rate = timer_of_rate(to);
>>  
>>  		cpu_to = per_cpu_ptr(&tegra_to, cpu);
>>  		cpu_to->of_base.base = timer_reg_base + base;
> 
> Thanks. This fixes a boot regression we are seeing on -next with
> Tegra210 (introduced by the commit referenced above). So ...
> 
> Acked-by: Jon Hunter <jonathanh@nvidia.com>
> Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks for the testing :)

[PATCH v2 2/6] 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 | 42 ++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index 830c66e2d927..810b4e7435cf 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 timer_of_rate(to);
+}
+
 static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 				   int rating)
 {
@@ -268,32 +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(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;
-		else
-			cpu_to->of_clk.rate = timer_of_rate(to);
-
-		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 v2 2/6] clocksource/drivers/tegra: Remove duplicated use of per_cpu_ptr

[Jon Hunter]

On 10/06/2019 17:43, 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 | 42 ++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index 830c66e2d927..810b4e7435cf 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 timer_of_rate(to);
> +}
> +
>  static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>  				   int rating)
>  {
> @@ -268,32 +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(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;
> -		else
> -			cpu_to->of_clk.rate = timer_of_rate(to);
> -
> -		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",
> 

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH v2 3/6] 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 810b4e7435cf..646b3530c2d2 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

Re: [PATCH v2 3/6] clocksource/drivers/tegra: Set and use timer's period

[Jon Hunter]

On 10/06/2019 17:43, Dmitry Osipenko wrote:
> 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 810b4e7435cf..646b3530c2d2 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);

Any reason you made this a round-up?

Jon

-- 
nvpublic

Re: [PATCH v2 3/6] clocksource/drivers/tegra: Set and use timer's period

[Dmitry Osipenko]

14.06.2019 18:48, Jon Hunter пишет:
> 
> On 10/06/2019 17:43, Dmitry Osipenko wrote:
>> 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 810b4e7435cf..646b3530c2d2 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);
> 
> Any reason you made this a round-up?

That's what timer_of_clk_init() does, I assume it should be a more correct variant.

Re: [PATCH v2 3/6] clocksource/drivers/tegra: Set and use timer's period

[Jon Hunter]

On 14/06/2019 17:45, Dmitry Osipenko wrote:
> 14.06.2019 18:48, Jon Hunter пишет:
>>
>> On 10/06/2019 17:43, Dmitry Osipenko wrote:
>>> 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 810b4e7435cf..646b3530c2d2 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);
>>
>> Any reason you made this a round-up?
> 
> That's what timer_of_clk_init() does, I assume it should be a more correct variant.

Sounds to me like this should be 2 patches, because you are changing the
value. This is not just purely cleanup IMO.

Jon

-- 
nvpublic

Re: [PATCH v2 3/6] clocksource/drivers/tegra: Set and use timer's period

[Dmitry Osipenko]

17.06.2019 13:51, Jon Hunter пишет:
> 
> On 14/06/2019 17:45, Dmitry Osipenko wrote:
>> 14.06.2019 18:48, Jon Hunter пишет:
>>>
>>> On 10/06/2019 17:43, Dmitry Osipenko wrote:
>>>> 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 810b4e7435cf..646b3530c2d2 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);
>>>
>>> Any reason you made this a round-up?
>>
>> That's what timer_of_clk_init() does, I assume it should be a more correct variant.
> 
> Sounds to me like this should be 2 patches, because you are changing the
> value. This is not just purely cleanup IMO.

Indeed, that could be at least mentioned in the commit message. Probably I just
assumed that this is such a minor change that not worth anything. A hundred of
microseconds is hardly noticeable.

I'm not really sure if this really worth a re-spin at this point. Jon, are you insisting?

Also, I now see that some drivers use DIV_ROUND_CLOSEST(), maybe it will be even
better? Not sure.. given that this is still a microseconds difference.

Re: [PATCH v2 3/6] clocksource/drivers/tegra: Set and use timer's period

[Jon Hunter]

On 17/06/2019 15:04, Dmitry Osipenko wrote:
> 17.06.2019 13:51, Jon Hunter пишет:
>>
>> On 14/06/2019 17:45, Dmitry Osipenko wrote:
>>> 14.06.2019 18:48, Jon Hunter пишет:
>>>>
>>>> On 10/06/2019 17:43, Dmitry Osipenko wrote:
>>>>> 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 810b4e7435cf..646b3530c2d2 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);
>>>>
>>>> Any reason you made this a round-up?
>>>
>>> That's what timer_of_clk_init() does, I assume it should be a more correct variant.
>>
>> Sounds to me like this should be 2 patches, because you are changing the
>> value. This is not just purely cleanup IMO.
> 
> Indeed, that could be at least mentioned in the commit message. Probably I just
> assumed that this is such a minor change that not worth anything. A hundred of
> microseconds is hardly noticeable.
> 
> I'm not really sure if this really worth a re-spin at this point. Jon, are you insisting?

At a minimum the changelog needs to be udpated to reflect what is going
on here. Yes it may not be a massive difference, but I prefer not to
change things without any rationale.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v2 3/6] clocksource/drivers/tegra: Set and use timer's period

[Dmitry Osipenko]

18.06.2019 11:40, Jon Hunter пишет:
> 
> On 17/06/2019 15:04, Dmitry Osipenko wrote:
>> 17.06.2019 13:51, Jon Hunter пишет:
>>>
>>> On 14/06/2019 17:45, Dmitry Osipenko wrote:
>>>> 14.06.2019 18:48, Jon Hunter пишет:
>>>>>
>>>>> On 10/06/2019 17:43, Dmitry Osipenko wrote:
>>>>>> 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 810b4e7435cf..646b3530c2d2 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);
>>>>>
>>>>> Any reason you made this a round-up?
>>>>
>>>> That's what timer_of_clk_init() does, I assume it should be a more correct variant.
>>>
>>> Sounds to me like this should be 2 patches, because you are changing the
>>> value. This is not just purely cleanup IMO.
>>
>> Indeed, that could be at least mentioned in the commit message. Probably I just
>> assumed that this is such a minor change that not worth anything. A hundred of
>> microseconds is hardly noticeable.
>>
>> I'm not really sure if this really worth a re-spin at this point. Jon, are you insisting?
> 
> At a minimum the changelog needs to be udpated to reflect what is going
> on here. Yes it may not be a massive difference, but I prefer not to
> change things without any rationale.

Okay, I'll respin this series and probably will just drop the round-up. I'll also
append the other two new patches "cycles can't be 0" and "max limit correction" to
this series.

Daniel, I'll also correct the "Fixes" tag to satisfy the linux-next patch checker.

[PATCH v2 4/6] 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 646b3530c2d2..a7fa12488066 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

Re: [PATCH v2 4/6] clocksource/drivers/tegra: Drop unneeded typecasting in one place

[Jon Hunter]

On 10/06/2019 17:43, Dmitry Osipenko wrote:
> 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 646b3530c2d2..a7fa12488066 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);

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH v2 5/6] clocksource/drivers/tegra: Add verbose definition for 1MHz constant

[Dmitry Osipenko]

Convert all 1MHz literals to a verbose constant for better readability.

Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clocksource/timer-tegra.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index a7fa12488066..2a428fdf702f 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -44,6 +44,8 @@
 #define TIMER1_IRQ_IDX		0
 #define TIMER10_IRQ_IDX		10
 
+#define TIMER_1MHz		1000000
+
 static u32 usec_config;
 static void __iomem *timer_reg_base;
 
@@ -158,7 +160,7 @@ static unsigned long tegra_delay_timer_read_counter_long(void)
 
 static struct delay_timer tegra_delay_timer = {
 	.read_current_timer = tegra_delay_timer_read_counter_long,
-	.freq = 1000000,
+	.freq = TIMER_1MHz,
 };
 #endif
 
@@ -224,7 +226,7 @@ static inline unsigned long tegra_rate_for_timer(struct timer_of *to,
 	 * parent clock.
 	 */
 	if (tegra20)
-		return 1000000;
+		return TIMER_1MHz;
 
 	return timer_of_rate(to);
 }
@@ -313,11 +315,11 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 		}
 	}
 
-	sched_clock_register(tegra_read_sched_clock, 32, 1000000);
+	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
 
 	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
-				    "timer_us", 1000000,
-				    300, 32, clocksource_mmio_readl_up);
+				    "timer_us", TIMER_1MHz, 300, 32,
+				    clocksource_mmio_readl_up);
 	if (ret)
 		pr_err("failed to register clocksource: %d\n", ret);
 
-- 
2.21.0

Re: [PATCH v2 5/6] clocksource/drivers/tegra: Add verbose definition for 1MHz constant

[Jon Hunter]

On 10/06/2019 17:43, Dmitry Osipenko wrote:
> Convert all 1MHz literals to a verbose constant for better readability.
> 
> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clocksource/timer-tegra.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index a7fa12488066..2a428fdf702f 100644
> --- a/drivers/clocksource/timer-tegra.c
> +++ b/drivers/clocksource/timer-tegra.c
> @@ -44,6 +44,8 @@
>  #define TIMER1_IRQ_IDX		0
>  #define TIMER10_IRQ_IDX		10
>  
> +#define TIMER_1MHz		1000000
> +
>  static u32 usec_config;
>  static void __iomem *timer_reg_base;
>  
> @@ -158,7 +160,7 @@ static unsigned long tegra_delay_timer_read_counter_long(void)
>  
>  static struct delay_timer tegra_delay_timer = {
>  	.read_current_timer = tegra_delay_timer_read_counter_long,
> -	.freq = 1000000,
> +	.freq = TIMER_1MHz,
>  };
>  #endif
>  
> @@ -224,7 +226,7 @@ static inline unsigned long tegra_rate_for_timer(struct timer_of *to,
>  	 * parent clock.
>  	 */
>  	if (tegra20)
> -		return 1000000;
> +		return TIMER_1MHz;
>  
>  	return timer_of_rate(to);
>  }
> @@ -313,11 +315,11 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>  		}
>  	}
>  
> -	sched_clock_register(tegra_read_sched_clock, 32, 1000000);
> +	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
>  
>  	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
> -				    "timer_us", 1000000,
> -				    300, 32, clocksource_mmio_readl_up);
> +				    "timer_us", TIMER_1MHz, 300, 32,
> +				    clocksource_mmio_readl_up);
>  	if (ret)
>  		pr_err("failed to register clocksource: %d\n", ret);
>  

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH v2 6/6] clocksource/drivers/tegra: Restore base address before cleanup

[Dmitry Osipenko]

We're adjusting the timer's base for each per-CPU timer to point to the
actual start of the timer since device-tree defines a compound registers
range that includes all of the timers. In this case the original base
need to be restore before calling iounmap to unmap the proper address.

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

diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
index 2a428fdf702f..7be91db98bd7 100644
--- a/drivers/clocksource/timer-tegra.c
+++ b/drivers/clocksource/timer-tegra.c
@@ -345,6 +345,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 			irq_dispose_mapping(cpu_to->clkevt.irq);
 		}
 	}
+
+	to->of_base.base = timer_reg_base;
 out:
 	timer_of_cleanup(to);
 
-- 
2.21.0

Re: [PATCH v2 6/6] clocksource/drivers/tegra: Restore base address before cleanup

[Jon Hunter]

On 10/06/2019 17:44, Dmitry Osipenko wrote:
> We're adjusting the timer's base for each per-CPU timer to point to the
> actual start of the timer since device-tree defines a compound registers
> range that includes all of the timers. In this case the original base
> need to be restore before calling iounmap to unmap the proper address.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clocksource/timer-tegra.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index 2a428fdf702f..7be91db98bd7 100644
> --- a/drivers/clocksource/timer-tegra.c
> +++ b/drivers/clocksource/timer-tegra.c
> @@ -345,6 +345,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>  			irq_dispose_mapping(cpu_to->clkevt.irq);
>  		}
>  	}
> +
> +	to->of_base.base = timer_reg_base;
>  out:
>  	timer_of_cleanup(to);

So what you are saying is that because we don't know which CPU executes
the tegra_init_timer() function, then it is necessary to restore the
base. IOW if it is not CPU0, then the base will be updated and hence,
need to be restored. Correct?

Jon

-- 
nvpublic

Re: [PATCH v2 6/6] clocksource/drivers/tegra: Restore base address before cleanup

[Dmitry Osipenko]

14.06.2019 18:48, Jon Hunter пишет:
> 
> On 10/06/2019 17:44, Dmitry Osipenko wrote:
>> We're adjusting the timer's base for each per-CPU timer to point to the
>> actual start of the timer since device-tree defines a compound registers
>> range that includes all of the timers. In this case the original base
>> need to be restore before calling iounmap to unmap the proper address.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/clocksource/timer-tegra.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
>> index 2a428fdf702f..7be91db98bd7 100644
>> --- a/drivers/clocksource/timer-tegra.c
>> +++ b/drivers/clocksource/timer-tegra.c
>> @@ -345,6 +345,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
>>  			irq_dispose_mapping(cpu_to->clkevt.irq);
>>  		}
>>  	}
>> +
>> +	to->of_base.base = timer_reg_base;
>>  out:
>>  	timer_of_cleanup(to);
> 
> So what you are saying is that because we don't know which CPU executes
> the tegra_init_timer() function, then it is necessary to restore the
> base. IOW if it is not CPU0, then the base will be updated and hence,
> need to be restored. Correct?

We know what the CPU is, it is always CPU0. What we don't know is what TIMER is
assigned for CPU0. On Tegra210 it is TIMER10, for other Tegra's it is TIMER0.