| 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 |
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
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 > >
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 >> >>
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 >>> >>>
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?
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 >
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. [ ... ]
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 > > [ ... ] > >
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.
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(-)