mbox series

[0/7] add support for clocksource/clockevent DT selection

Message ID 1568123236-767-1-git-send-email-claudiu.beznea@microchip.com (mailing list archive)
Headers show
Series add support for clocksource/clockevent DT selection | expand

Message

Claudiu Beznea Sept. 10, 2019, 1:47 p.m. UTC
Hi,

This series adds support to permit the selection of clocksource/clockevent
via DT.

In [1] I proposed a solution other than the one in this series, with parsing DT
bindings and at probe time and passing it to timer specific probe function.
Looking forward though the clocksource/clockevent drivers implementation and
taking into account the response at [2] I sticked the implementation to
timer-of specific library.

The implementation in this series is using timer-of specific library to parse
the DT bindings related to timers functions: clocksource or clockevent.

With this implementation a timer's driver registers for probing an array of
objects of type struct timer_of. In flags member of struct timer_of object it
has to be passed the following new flags (related to how the driver will behave)
as follows:
- TIMER_OF_TYPE_CS: timer could work only as clocksource at a time
- TIMER_OF_TYPE_CE: timer could work only as clockevent at a time
- TIMER_OF_TYPE_CE_AND_CE: timer could work at a time as both, clocksource and
clockevent.

The timer registration macro (for probing) now gets a new argument which should
be an array of struct timer_of objects:

TIMER_OF_DECLARE(name, compat, handler, to)
CLOCKSOURCE_OF_DECLARE(name, compat, handler, to)

In case driver could work to feed only the clocksource subsystem or only the
clockevent subsystem the struct timer_of array passed to TIMER_OF_DECLARE()/
CLOCKSOURCE_OF_DECLARE() should contain 2 entries: one to be filled if probed
timer works as clocksource device, one to be filled if probed timer works as
clockevent device.

For such a case, the minimal format of struct timer_of array is as follows:
struct timer_of to[] = {
	{ .flags = TIMER_OF_TYPE_CS }
	{ .flags = TIMER_OF_TYPE_CE }
	{ /* sentinel. */
};

If timer could work as both, clocksource and clockevent at the same time,
the struct timer_of array should look as follows:
struct timer_of to[] = {
	{ .flags = TIMER_OF_TYPE_CE_AND_CS }
	{ /* sentinel. */
};

And in device tree there should be added chosen bindings as follows:

chosen {
	linux,clocksource {
		timer = <&timer1>
	};
	
	linux,clockevent {
		timer = <&timer2>;
	}
};

timer1: t1@123 {
	compatible = "timerx-compatible";
	/* the rest of DT bindings here */
};

timer2: t1@234 {
	compatible = "timerx-compatible";
	/* the rest of DT bindings here */
};

At probing time (timer_probe()), timer_of_init() will check the DT bindings
and try to match with one of the entries in struct timer_of array passed at
probe. The used entry will be considered used if the timers' device_node 
pointer is set. If no matching b/w DT and what has been passed for probe
via struct timer_of array then probe should fail.

The patches in this series are organized as follows:
1/7 - avoid calling timer_of_init() for every CPU since it should not be needed
2/7 - changes timer registration macro by adding a new argument (pointer to
      struct timer_of)
3/7 - use BIT() macro
4/7 - add clocksource/clockevent selection documentation [3]
5/7 - implement support described above
6/7 - remove an unnecessary line
7/7 - implement this support for integrator-ap timer

I implemented this support for timer published at [4].

Thank you,
Claudiu Beznea

[1] https://lore.kernel.org/lkml/34574b0f-7d09-eb92-ea62-4199c293b0e7@microchip.com/
[2] https://lore.kernel.org/lkml/1ebaa306-8a7f-fd58-56e0-a61b767357f7@linaro.org/
[3] https://lore.kernel.org/lkml/20171213185313.20017-1-alexandre.belloni@free-electrons.com/
[4] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/

Alexandre Belloni (2):
  dt-bindings: chosen: Add clocksource and clockevent selection
  clocksource/drivers/integrator-ap: parse the chosen node

Claudiu Beznea (5):
  clocksource/drivers/c-sky: request timer_of_init only for probing CPU
  clocksource: change timer registration macros
  clocksource/timer_of: use BIT() macro
  clocksource/drivers/timer-of: add support support for timer's
    functionalities
  drivers/clocksource/timer-of: keep declaration on one line

 Documentation/devicetree/bindings/chosen.txt |  20 +++++
 arch/arm/kernel/smp_twd.c                    |  10 ++-
 arch/arm/mach-davinci/time.c                 |   2 +-
 arch/microblaze/kernel/timer.c               |   2 +-
 arch/mips/ralink/cevt-rt3352.c               |   2 +-
 arch/nios2/kernel/time.c                     |   2 +-
 drivers/clocksource/Kconfig                  |   1 +
 drivers/clocksource/arc_timer.c              |   6 +-
 drivers/clocksource/arm_arch_timer.c         |   6 +-
 drivers/clocksource/arm_global_timer.c       |   2 +-
 drivers/clocksource/armv7m_systick.c         |   2 +-
 drivers/clocksource/asm9260_timer.c          |   2 +-
 drivers/clocksource/bcm2835_timer.c          |   2 +-
 drivers/clocksource/bcm_kona_timer.c         |   4 +-
 drivers/clocksource/clksrc-dbx500-prcmu.c    |   2 +-
 drivers/clocksource/clksrc_st_lpc.c          |   2 +-
 drivers/clocksource/clps711x-timer.c         |   2 +-
 drivers/clocksource/dw_apb_timer_of.c        |   9 ++-
 drivers/clocksource/exynos_mct.c             |   4 +-
 drivers/clocksource/h8300_timer16.c          |   2 +-
 drivers/clocksource/h8300_timer8.c           |   2 +-
 drivers/clocksource/h8300_tpu.c              |   2 +-
 drivers/clocksource/jcore-pit.c              |   2 +-
 drivers/clocksource/mips-gic-timer.c         |   2 +-
 drivers/clocksource/mps2-timer.c             |   2 +-
 drivers/clocksource/mxs_timer.c              |   2 +-
 drivers/clocksource/nomadik-mtu.c            |   2 +-
 drivers/clocksource/renesas-ostm.c           |   2 +-
 drivers/clocksource/samsung_pwm_timer.c      |  12 ++-
 drivers/clocksource/timer-armada-370-xp.c    |   6 +-
 drivers/clocksource/timer-atcpit100.c        |  74 +++++++++---------
 drivers/clocksource/timer-atlas7.c           |   3 +-
 drivers/clocksource/timer-atmel-pit.c        |   2 +-
 drivers/clocksource/timer-atmel-st.c         |   2 +-
 drivers/clocksource/timer-atmel-tcb.c        |   2 +-
 drivers/clocksource/timer-cadence-ttc.c      |   2 +-
 drivers/clocksource/timer-davinci.c          |   3 +-
 drivers/clocksource/timer-digicolor.c        |   2 +-
 drivers/clocksource/timer-efm32.c            |   4 +-
 drivers/clocksource/timer-fsl-ftm.c          |   2 +-
 drivers/clocksource/timer-fttmr010.c         |  10 +--
 drivers/clocksource/timer-gx6605s.c          |  58 +++++++-------
 drivers/clocksource/timer-imx-gpt.c          |  24 +++---
 drivers/clocksource/timer-imx-sysctr.c       |  61 +++++++--------
 drivers/clocksource/timer-imx-tpm.c          |  69 ++++++++---------
 drivers/clocksource/timer-integrator-ap.c    |  21 +++++-
 drivers/clocksource/timer-ixp4xx.c           |   2 +-
 drivers/clocksource/timer-keystone.c         |   2 +-
 drivers/clocksource/timer-lpc32xx.c          |   2 +-
 drivers/clocksource/timer-mediatek.c         | 108 +++++++++++++++------------
 drivers/clocksource/timer-meson6.c           |   2 +-
 drivers/clocksource/timer-milbeaut.c         |  59 ++++++++-------
 drivers/clocksource/timer-mp-csky.c          |  52 ++++++-------
 drivers/clocksource/timer-npcm7xx.c          |  87 +++++++++++----------
 drivers/clocksource/timer-nps.c              |   6 +-
 drivers/clocksource/timer-of.c               |  91 +++++++++++++++++++++-
 drivers/clocksource/timer-of.h               |  36 +++++++--
 drivers/clocksource/timer-orion.c            |   2 +-
 drivers/clocksource/timer-owl.c              |   6 +-
 drivers/clocksource/timer-oxnas-rps.c        |   4 +-
 drivers/clocksource/timer-pistachio.c        |   2 +-
 drivers/clocksource/timer-prima2.c           |   2 +-
 drivers/clocksource/timer-probe.c            |  17 ++++-
 drivers/clocksource/timer-pxa.c              |   2 +-
 drivers/clocksource/timer-qcom.c             |   4 +-
 drivers/clocksource/timer-rda.c              |  66 ++++++++--------
 drivers/clocksource/timer-riscv.c            |   2 +-
 drivers/clocksource/timer-rockchip.c         |   4 +-
 drivers/clocksource/timer-sp804.c            |   4 +-
 drivers/clocksource/timer-sprd.c             |  75 +++++++++----------
 drivers/clocksource/timer-stm32.c            |  39 +++++-----
 drivers/clocksource/timer-sun4i.c            |  78 +++++++++----------
 drivers/clocksource/timer-sun5i.c            |   4 +-
 drivers/clocksource/timer-tango-xtal.c       |   2 +-
 drivers/clocksource/timer-tegra.c            |  31 ++++----
 drivers/clocksource/timer-ti-32k.c           |   2 +-
 drivers/clocksource/timer-u300.c             |   2 +-
 drivers/clocksource/timer-versatile.c        |   4 +-
 drivers/clocksource/timer-vf-pit.c           |   2 +-
 drivers/clocksource/timer-vt8500.c           |   2 +-
 drivers/clocksource/timer-zevio.c            |   2 +-
 include/linux/clocksource.h                  |  30 +++++++-
 82 files changed, 748 insertions(+), 544 deletions(-)

Comments

Daniel Lezcano Sept. 25, 2019, 5:19 p.m. UTC | #1
Hi Claudiu,

On 10/09/2019 15:47, Claudiu Beznea wrote:
> Hi,
> 
> This series adds support to permit the selection of clocksource/clockevent
> via DT.

Thanks for the proposal and taking care of making some progress on this.

I just wanted to let you know I've been traveling but the series is in
my pipe and I did not forget it. I'll comment it next week.

 -- Daniel
Claudiu Beznea Sept. 26, 2019, 8:42 a.m. UTC | #2
On 25.09.2019 20:19, Daniel Lezcano wrote:
> External E-Mail
> 
> 
> Hi Claudiu,
> 
> On 10/09/2019 15:47, Claudiu Beznea wrote:
>> Hi,
>>
>> This series adds support to permit the selection of clocksource/clockevent
>> via DT.
> 
> Thanks for the proposal and taking care of making some progress on this.
> 
> I just wanted to let you know I've been traveling but the series is in
> my pipe and I did not forget it. I'll comment it next week.

Hi Daniel,

No problem. Thank you for letting me know.

Claudiu

> 
>  -- Daniel
> 
>
Claudiu Beznea Oct. 2, 2019, 1:35 p.m. UTC | #3
Hi Daniel,

Taking into account that Rob doesn't agree with the solution proposed in
this series do you think there is a chance to merge this driver as is?

If you have other suggestion I am open to try it.

Thank you,
Claudiu Beznea

On 26.09.2019 11:42, Claudiu Beznea wrote:
> 
> 
> On 25.09.2019 20:19, Daniel Lezcano wrote:
>> External E-Mail
>>
>>
>> Hi Claudiu,
>>
>> On 10/09/2019 15:47, Claudiu Beznea wrote:
>>> Hi,
>>>
>>> This series adds support to permit the selection of clocksource/clockevent
>>> via DT.
>>
>> Thanks for the proposal and taking care of making some progress on this.
>>
>> I just wanted to let you know I've been traveling but the series is in
>> my pipe and I did not forget it. I'll comment it next week.
> 
> Hi Daniel,
> 
> No problem. Thank you for letting me know.
> 
> Claudiu
> 
>>
>>  -- Daniel
>>
>>
Claudiu Beznea Oct. 3, 2019, 10:43 a.m. UTC | #4
On 02.10.2019 16:35, Claudiu Beznea wrote:
> Hi Daniel,
> 
> Taking into account that Rob doesn't agree with the solution proposed in
> this series do you think there is a chance to merge this driver as is?

Sorry, I was talking here about the driver at [1].

[1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/

> 
> If you have other suggestion I am open to try it.
> 
> Thank you,
> Claudiu Beznea
> 
> On 26.09.2019 11:42, Claudiu Beznea wrote:
>>
>>
>> On 25.09.2019 20:19, Daniel Lezcano wrote:
>>> External E-Mail
>>>
>>>
>>> Hi Claudiu,
>>>
>>> On 10/09/2019 15:47, Claudiu Beznea wrote:
>>>> Hi,
>>>>
>>>> This series adds support to permit the selection of clocksource/clockevent
>>>> via DT.
>>>
>>> Thanks for the proposal and taking care of making some progress on this.
>>>
>>> I just wanted to let you know I've been traveling but the series is in
>>> my pipe and I did not forget it. I'll comment it next week.
>>
>> Hi Daniel,
>>
>> No problem. Thank you for letting me know.
>>
>> Claudiu
>>
>>>
>>>  -- Daniel
>>>
>>>
Daniel Lezcano Oct. 13, 2019, 6:16 p.m. UTC | #5
Hi Claudiu,

sorry for the delay, I was OoO again.

On 03/10/2019 12:43, Claudiu.Beznea@microchip.com wrote:
> 
> 
> On 02.10.2019 16:35, Claudiu Beznea wrote:
>> Hi Daniel,
>>
>> Taking into account that Rob doesn't agree with the solution proposed in
>> this series do you think there is a chance to merge this driver as is?
> 
> Sorry, I was talking here about the driver at [1].
> 
> [1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/

Damn! 7 months old. I'm truly sorry we do not have progress on this. Let
fix this once and for all.

In the driver:

ret = of_property_read_u32(node, "clock-frequency", &freq);

It is unclear how is used this property. It should be the frequency
driving the timer, but can we get from a clk_get_rate() and a fixed divider?
Claudiu Beznea Oct. 15, 2019, 9:23 a.m. UTC | #6
Hi Daniel,

On 13.10.2019 21:16, Daniel Lezcano wrote:
> Hi Claudiu,
> 
> sorry for the delay, I was OoO again.

No problem, thank you for your reply.

> 
> On 03/10/2019 12:43, Claudiu.Beznea@microchip.com wrote:
>>
>>
>> On 02.10.2019 16:35, Claudiu Beznea wrote:
>>> Hi Daniel,
>>>
>>> Taking into account that Rob doesn't agree with the solution proposed in
>>> this series do you think there is a chance to merge this driver as is?
>>
>> Sorry, I was talking here about the driver at [1].
>>
>> [1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/
> 
> Damn! 7 months old. I'm truly sorry we do not have progress on this. Let
> fix this once and for all.
> 
> In the driver:
> 
> ret = of_property_read_u32(node, "clock-frequency", &freq);
> 
> It is unclear how is used this property. It should be the frequency
> driving the timer, but can we get from a clk_get_rate() and a fixed divider?
> 

The timer could be driven by 2 clock sources, one is called peripheral
clock and is the clock that is also driving the IP itself, and one is
called generic clock (this could drive only the timer itself) and should be
at least 3 times lower than the peripheral clock.

We could choose the clock driving the timer by setting the PIT64B_MR.SGCLK
bit (0 - means the timer itself is driven by peripheral clock, 1 - means
the timer is driven by the generic clock).

The timer clock source could be divided by MR.PRES + 1.

So, I used the clock-frequency DT binding to let user choose the timer's
frequency. Based on the value provided via this DT binding the best clock
source and prescaler is chosen via mchp_pit64b_pres_prepare() function.

As the datasheet for the product that is using this IP is open now, I'm
inserting here a link to it [1].

Thank you,
Claudiu Beznea

[1]
http://ww1.microchip.com/downloads/en/DeviceDoc/SAM9X60-Data-Sheet-DS60001579A.pdf

>
Daniel Lezcano Oct. 18, 2019, 8:24 p.m. UTC | #7
Hi Claudiu,

On 15/10/2019 11:23, Claudiu.Beznea@microchip.com wrote:

[ ... ]

> The timer clock source could be divided by MR.PRES + 1.
> 
> So, I used the clock-frequency DT binding to let user choose the timer's
> frequency. Based on the value provided via this DT binding the best clock
> source and prescaler is chosen via mchp_pit64b_pres_prepare() function.

I'm willing to take the driver but I doubt the purpose of the
clock-frequency is to let the user choose the frequency.


[ ... ]
Claudiu Beznea Oct. 21, 2019, 8:58 a.m. UTC | #8
Hi Daniel,

On 18.10.2019 23:24, Daniel Lezcano wrote:
> Hi Claudiu,
> 
> On 15/10/2019 11:23, Claudiu.Beznea@microchip.com wrote:
> 
> [ ... ]
> 
>> The timer clock source could be divided by MR.PRES + 1.
>>
>> So, I used the clock-frequency DT binding to let user choose the timer's
>> frequency. Based on the value provided via this DT binding the best clock
>> source and prescaler is chosen via mchp_pit64b_pres_prepare() function.
> 
> I'm willing to take the driver but I doubt the purpose of the
> clock-frequency is to let the user choose the frequency.
> 

I found this approach in the following already integrated drivers:
drivers/clocksource/armv7m_systick.c
drivers/clocksource/bcm2835_timer.c
drivers/clocksource/bcm_kona_timer.c
drivers/clocksource/mips-gic-timer.c
drivers/clocksource/mps2-timer.c
drivers/clocksource/timer-qcom.c
drivers/clocksource/arm_arch_timer.c

Looking through the documentation of these, most of them document this DT
property as the frequency of the clock that drivers the timer, but none of
them seems to have some IP internal dividers so that the timer to tick at
different frequency than the clock that feeds the IP. From the
documentation of the above drivers;
drivers/clocksource/armv7m_systick.c
	- clock-frequency : The rate in HZ in input of the ARM SysTick

drivers/clocksource/bcm2835_timer.c
	- clock-frequency : The frequency of the clock that drives the counter, in
Hz.
drivers/clocksource/bcm_kona_timer.c
	- clock-frequency: frequency that the clock operates

drivers/clocksource/mips-gic-timer.c
	clock-frequency : Clock frequency at which the GIC timers operate.
drivers/clocksource/mps2-timer.c
	- clock-frequency : The rate in HZ in input of the ARM MPS2 timer

drivers/clocksource/timer-qcom.c
	- clock-frequency : The frequency of the debug timer and the general
purpose
                    timer(s) in Hz in that order.


This is why I also chose this DT bindings.

If you want I can stick to a fixed frequency hard coded in the driver.
Please let me know if this would be OK for you.

Thank you,
Claudiu Beznea

> 
> [ ... ]
> 
>
Daniel Lezcano Oct. 21, 2019, 2:17 p.m. UTC | #9
On 21/10/2019 10:58, Claudiu.Beznea@microchip.com wrote:
> Hi Daniel,
> 
> On 18.10.2019 23:24, Daniel Lezcano wrote:
>> Hi Claudiu,
>>
>> On 15/10/2019 11:23, Claudiu.Beznea@microchip.com wrote:
>>
>> [ ... ]
>>
>>> The timer clock source could be divided by MR.PRES + 1.
>>>
>>> So, I used the clock-frequency DT binding to let user choose the timer's
>>> frequency. Based on the value provided via this DT binding the best clock
>>> source and prescaler is chosen via mchp_pit64b_pres_prepare() function.
>>
>> I'm willing to take the driver but I doubt the purpose of the
>> clock-frequency is to let the user choose the frequency.
>>
> 
> I found this approach in the following already integrated drivers:
> drivers/clocksource/armv7m_systick.c
> drivers/clocksource/bcm2835_timer.c
> drivers/clocksource/bcm_kona_timer.c
> drivers/clocksource/mips-gic-timer.c
> drivers/clocksource/mps2-timer.c
> drivers/clocksource/timer-qcom.c
> drivers/clocksource/arm_arch_timer.c
> 
> Looking through the documentation of these, most of them document this DT
> property as the frequency of the clock that drivers the timer, but none of
> them seems to have some IP internal dividers so that the timer to tick at
> different frequency than the clock that feeds the IP. From the
> documentation of the above drivers;
> drivers/clocksource/armv7m_systick.c
> 	- clock-frequency : The rate in HZ in input of the ARM SysTick
> 
> drivers/clocksource/bcm2835_timer.c
> 	- clock-frequency : The frequency of the clock that drives the counter, in
> Hz.
> drivers/clocksource/bcm_kona_timer.c
> 	- clock-frequency: frequency that the clock operates
> 
> drivers/clocksource/mips-gic-timer.c
> 	clock-frequency : Clock frequency at which the GIC timers operate.
> drivers/clocksource/mps2-timer.c
> 	- clock-frequency : The rate in HZ in input of the ARM MPS2 timer
> 
> drivers/clocksource/timer-qcom.c
> 	- clock-frequency : The frequency of the debug timer and the general
> purpose
>                     timer(s) in Hz in that order.
> 
> 
> This is why I also chose this DT bindings.
> 
> If you want I can stick to a fixed frequency hard coded in the driver.
> Please let me know if this would be OK for you.

Yes, the clock-frequency is used to specify the frequency when the
information can not be retrieved from the clock. The goal is not to
specify a frequency and compute from there a prescalar value.

Hardcoding the frequency is fine or hardcode the divider and compute the
frequency from the clock rate.