| 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 |
On Tue, Sep 10, 2019 at 6:47 AM Claudiu Beznea <claudiu.beznea@microchip.com> wrote: > > This series adds support to permit the selection of clocksource/clockevent > via DT. Sorry about this, but could you try to include more of a rational for *why* this would be useful in your cover-letter/commit messages? I'm not sure I understand the limitation that requires such an option to be added to the dts. thanks -john
On 10.09.2019 19:05, John Stultz wrote: > External E-Mail > > > On Tue, Sep 10, 2019 at 6:47 AM Claudiu Beznea > <claudiu.beznea@microchip.com> wrote: >> >> This series adds support to permit the selection of clocksource/clockevent >> via DT. > > Sorry about this, but could you try to include more of a rational for > *why* this would be useful in your cover-letter/commit messages? > Sorry for not being to clear in the cover letter. The case I am trying to solve here is as follows: The timer hardware for which I publish a driver at [1] cannot work at the same time as a clocksource and clockevent. On some of our platforms we have more than one such a timer. So we could use one hardware resource as clocksource and one as clockevent but not one for both. Due to this, I proposed in the driver at [1] to have 1st probed hardware to work as clocksource and the 2nd one to work as clockevent. There are also other timer drivers that uses this approach. While working on this series I noticed that there are others that are using even different compatibles (although it looks to be related to the same hardware). Due to this Daniel proposed to have an unified mechanism for this scenario, see [2], (something like what I proposed in this series), such that to have a determinism b/w the function that the hardware resources would behave (either clocksource or clockevent or both). The description I gave in cover letter was not the best one. Because, actually, at this time, the clocksource/clockevent of the system would not be the one pointed by DT bindings, these DT bindings would chose only the function that a timer would have. Because if more than one clocksource/clockevent would be registered in a system the rating field of struct clocksource/struct clockevent would be the one that would decide the chosen clocksource/clockevent. [1] https://lore.kernel.org/lkml/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/ [2] https://lore.kernel.org/lkml/2f831f1b-c87d-48bd-cf02-2ebb334b964c@linaro.org/ > I'm not sure I understand the limitation that requires such an option > to be added to the dts. > > thanks > -john >
On Tue, Sep 10, 2019 at 11:52 PM <Claudiu.Beznea@microchip.com> wrote: > On 10.09.2019 19:05, John Stultz wrote: > > External E-Mail > > On Tue, Sep 10, 2019 at 6:47 AM Claudiu Beznea > > <claudiu.beznea@microchip.com> wrote: > >> > >> This series adds support to permit the selection of clocksource/clockevent > >> via DT. > > > > Sorry about this, but could you try to include more of a rational for > > *why* this would be useful in your cover-letter/commit messages? > > > > Sorry for not being to clear in the cover letter. > > The case I am trying to solve here is as follows: > The timer hardware for which I publish a driver at [1] cannot work at the > same time as a clocksource and clockevent. On some of our platforms we have > more than one such a timer. So we could use one hardware resource as > clocksource and one as clockevent but not one for both. > Due to this, I proposed in the driver at [1] to have 1st probed hardware to > work as clocksource and the 2nd one to work as clockevent. [snip] > Due to this Daniel proposed to have an unified mechanism for this scenario, > see [2], (something like what I proposed in this series), such that to have > a determinism b/w the function that the hardware resources would behave > (either clocksource or clockevent or both). Thanks for the additional explanation! I'd suggest adding something like it to the commit logs for next time. Personally, I tend to think of DT bindings as a big harry ABI, and as such avoid it as much as possible. :) I'd probably favor driver tweaks to ensure the hardware only gets registered once for each, using a minimal DT compatible or flag on the hardware to better describe the underlying hardware quirk that keeps it from being usable for both clocksource and clockevent usage. That way the DT sticks to accurately describing hardware, instead of system/software-abstraction configuration details that just apply for Linux. But, again, I'm not really a DT person, so I'll defer to Rob and Daniel. thanks -john
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(-)