mbox series

[v5,0/4] stm32 clocksource driver rework

Message ID 1508312614-27750-1-git-send-email-benjamin.gaignard@linaro.org
Headers show
Series stm32 clocksource driver rework | expand

Message

Benjamin Gaignard Oct. 18, 2017, 7:43 a.m. UTC
version 5:
- rebase on top of timer/core branch
- rework commit message of the first patch

version 4:
- split patch in 3 parts
  - convert code to timer_of
  - only use 32 bits timers
  - add clocksource support

version 3:
- fix comments done by Daniel
- use timer_of helper functions

version 2:
- fix uninitialized variable

These patches implements clocksource and clockevent by using only one hardware block.
Getting both clock source and events on the same hardware lead to change quite
a lot driver code.
It also limits usage of clocksource to 32 bits timers because 16 bits ones
aren't enough accurate.
Thanks to timer_fo helpers this series includes minor clean up in structures,
function prototypes and driver name.

Since 16 bits timers become useless it also removes them from stm32f4 and
stm32f7 devicetree.

Increase min delta value to be sure to not have too much interrupts.

Benjamin Gaignard (4):
  clocksource: stm32: convert driver to timer_of
  clocksource: stm32: only use 32 bits timers
  clocksource: stm32: add clocksource support
  arm: dts: stm32: remove useless clocksource nodes

 arch/arm/boot/dts/stm32f429.dtsi  |  32 ------
 arch/arm/boot/dts/stm32f746.dtsi  |  32 ------
 drivers/clocksource/Kconfig       |   1 +
 drivers/clocksource/timer-stm32.c | 229 ++++++++++++++++++--------------------
 4 files changed, 112 insertions(+), 182 deletions(-)

-- 
2.7.4

Comments

Julien Thierry Oct. 18, 2017, 8:21 a.m. UTC | #1
On 18/10/17 08:43, Benjamin Gaignard wrote:
> 16 bits hardware are not enough accure to be used.

> Do no allow them to be probed by tested max counter value.

> 

> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

> ---

>   drivers/clocksource/timer-stm32.c | 23 +++++++++--------------

>   1 file changed, 9 insertions(+), 14 deletions(-)

> 

> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c

> index abff21c..f7e4eec 100644

> --- a/drivers/clocksource/timer-stm32.c

> +++ b/drivers/clocksource/timer-stm32.c

> @@ -81,9 +81,9 @@ static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)

>   static int __init stm32_clockevent_init(struct device_node *node)

>   {

>   	struct reset_control *rstc;

> -	unsigned long max_delta;

> -	int ret, bits, prescaler = 1;

> +	unsigned long max_arr;

>   	struct timer_of *to;

> +	int ret;

>   

>   	to = kzalloc(sizeof(*to), GFP_KERNEL);

>   	if (!to)

> @@ -113,26 +113,21 @@ static int __init stm32_clockevent_init(struct device_node *node)

>   

>   	/* Detect whether the timer is 16 or 32 bits */

>   	writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);

> -	max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);

> -	if (max_delta == ~0U) {

> -		prescaler = 1;

> -		bits = 32;

> -	} else {

> -		prescaler = 1024;

> -		bits = 16;

> +	max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);

> +	if (max_arr != ~0U) {

> +		pr_err("32 bits timer is needed\n");

> +		return -EINVAL;


Same as with previous patch, I think "to" should get freed.

Also, why is there no function to undo what timer_of_init did? Shouldn't 
we be able to free the irqs and unmap the timer io base here?

Cheers,

-- 
Julien Thierry
Benjamin Gaignard Oct. 18, 2017, 12:25 p.m. UTC | #2
2017-10-18 10:21 GMT+02:00 Julien Thierry <julien.thierry@arm.com>:
>

>

> On 18/10/17 08:43, Benjamin Gaignard wrote:

>>

>> 16 bits hardware are not enough accure to be used.

>> Do no allow them to be probed by tested max counter value.

>>

>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

>> ---

>>   drivers/clocksource/timer-stm32.c | 23 +++++++++--------------

>>   1 file changed, 9 insertions(+), 14 deletions(-)

>>

>> diff --git a/drivers/clocksource/timer-stm32.c

>> b/drivers/clocksource/timer-stm32.c

>> index abff21c..f7e4eec 100644

>> --- a/drivers/clocksource/timer-stm32.c

>> +++ b/drivers/clocksource/timer-stm32.c

>> @@ -81,9 +81,9 @@ static irqreturn_t stm32_clock_event_handler(int irq,

>> void *dev_id)

>>   static int __init stm32_clockevent_init(struct device_node *node)

>>   {

>>         struct reset_control *rstc;

>> -       unsigned long max_delta;

>> -       int ret, bits, prescaler = 1;

>> +       unsigned long max_arr;

>>         struct timer_of *to;

>> +       int ret;

>>         to = kzalloc(sizeof(*to), GFP_KERNEL);

>>         if (!to)

>> @@ -113,26 +113,21 @@ static int __init stm32_clockevent_init(struct

>> device_node *node)

>>         /* Detect whether the timer is 16 or 32 bits */

>>         writel_relaxed(~0U, timer_of_base(to) + TIM_ARR);

>> -       max_delta = readl_relaxed(timer_of_base(to) + TIM_ARR);

>> -       if (max_delta == ~0U) {

>> -               prescaler = 1;

>> -               bits = 32;

>> -       } else {

>> -               prescaler = 1024;

>> -               bits = 16;

>> +       max_arr = readl_relaxed(timer_of_base(to) + TIM_ARR);

>> +       if (max_arr != ~0U) {

>> +               pr_err("32 bits timer is needed\n");

>> +               return -EINVAL;

>

>

> Same as with previous patch, I think "to" should get freed.


Yes I could solve that in the first patch

>

> Also, why is there no function to undo what timer_of_init did? Shouldn't we

> be able to free the irqs and unmap the timer io base here?


timer-of doesn't provide deinit function since I will have to do a new version
I will add it.

Benjamin
>

> Cheers,

>

> --

> Julien Thierry