| Message ID | 1385085414-9034-7-git-send-email-joelf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Joel Fernandes <joelf@ti.com> [131121 18:00]: > +Required properties for system timers (clockevents/clocksource): > +- ti,timer-parent: System timer's parent mux clock needs to be setup. > + This is currently hardcoded in code, for DT boot we > + move this to DT. > + This can be replaced with just clk_set_rate, or clk_set_parent if needed. Or by having a clocks = <&32k_clk> property in the dmtimer node in the .dts file. > Optional properties: > - ti,timer-alwon: Indicates the timer is in an alway-on power domain. Hmm this we may not need, this can probably be deciphered from the compatible flag already? Then for the users of a specific dmtimer, they can select the right one using the interrupt-parent property: timer1: timer@0x4800abcd { compatible = "ti,omap5430-timer"; #interrupt-cells = <1>; /* needs irqchip implemented for dmtimer */ interrupt-controller; #clock-cells = <1>; /* needs clocksource implemented for dmtimer */ clock-output-names = "32k", "sys_ck"; ... }; counter32k: counter@4ae04000 { compatible = "ti,omap-counter32k"; #clock-cells = <1>; /* needs clocksource implemented for 32k counter */ clock-output-names = "32k"; ... }; timer { compatible = "ti,omap5-timer"; interrupt-parent = <&timer1>; interrupts = <1>; clocks = <&timer1 0>, /* use timer1 as clockevent, clock index 0 = 32k, 1 = sys_ck ... */ <&counter32k 0>; /* use 32k counter as clocksource */ }; > - ti,timer-dsp: Indicates the timer can interrupt the on-chip DSP in This can be probably also be mapped based on the compatible property? Regards, Tony
Hi Tony, Thanks for your comments, few replies inline below.. On 11/22/2013 09:58 AM, Tony Lindgren wrote: > * Joel Fernandes <joelf@ti.com> [131121 18:00]: >> +Required properties for system timers (clockevents/clocksource): >> +- ti,timer-parent: System timer's parent mux clock needs to be setup. >> + This is currently hardcoded in code, for DT boot we >> + move this to DT. >> + > > This can be replaced with just clk_set_rate, or clk_set_parent if needed. > Or by having a clocks = <&32k_clk> property in the dmtimer node in the > .dts file. > Sure, but clock-data is still not available to make this possible in mainline. We also discussed earlier right that we don't want dependencies as much as possible to get one chunk in and working at a time. I was thinking like for a first-pass since there's a lot of unrelated code that doesn't have dependencies, but needs this to work, we can introduce this property for now and drop it later as a "cost of migration"? >> Optional properties: >> - ti,timer-alwon: Indicates the timer is in an alway-on power domain. > > Hmm this we may not need, this can probably be deciphered from the compatible > flag already? How? Compatible contains the same string, for example for OMAP4: timer8 has: compatible = "ti,omap4430-timer"; ti,timer-pwm; ti,timer-dsp; and timer9 has: compatible = "ti,omap4430-timer"; ti,hwmods = "timer9"; ti,timer-pwm; > > Then for the users of a specific dmtimer, they can select the right one using > the interrupt-parent property: > > timer1: timer@0x4800abcd { > compatible = "ti,omap5430-timer"; > #interrupt-cells = <1>; /* needs irqchip implemented for dmtimer */ > interrupt-controller; > #clock-cells = <1>; /* needs clocksource implemented for dmtimer */ > clock-output-names = "32k", "sys_ck"; > ... > }; In reference to my last thread reply, irqchip may not be available early in the boot process (.init_time) for system timer usage? thanks, -Joel
On 11/22/2013 09:58 AM, Tony Lindgren wrote: > * Joel Fernandes <joelf@ti.com> [131121 18:00]: >> +Required properties for system timers (clockevents/clocksource): >> +- ti,timer-parent: System timer's parent mux clock needs to be setup. >> + This is currently hardcoded in code, for DT boot we >> + move this to DT. >> + > > This can be replaced with just clk_set_rate, or clk_set_parent if needed. > Or by having a clocks = <&32k_clk> property in the dmtimer node in the > .dts file. > >> Optional properties: >> - ti,timer-alwon: Indicates the timer is in an alway-on power domain. > > Hmm this we may not need, this can probably be deciphered from the compatible > flag already? I was thinking maybe we can improve the of_clocksource_init matching capabilities to match better, not just on compatible but other properties for better selection? For example, a generic clocksource DT property to say that the timer is in the wakeup domain and so should be preferred as the clocksource? Also additional properties to force its selection? Then we can switch to clocksource_of_init and have it do the selection. Not sure if there is such a need or value for other ARM platforms so we may be over complicating the framework to handle these quirks. Comments welcomed about it. thanks, -Joel > > Then for the users of a specific dmtimer, they can select the right one using > the interrupt-parent property: > > timer1: timer@0x4800abcd { > compatible = "ti,omap5430-timer"; > #interrupt-cells = <1>; /* needs irqchip implemented for dmtimer */ > interrupt-controller; > #clock-cells = <1>; /* needs clocksource implemented for dmtimer */ > clock-output-names = "32k", "sys_ck"; > ... > }; > > counter32k: counter@4ae04000 { > compatible = "ti,omap-counter32k"; > #clock-cells = <1>; /* needs clocksource implemented for 32k counter */ > clock-output-names = "32k"; > ... > }; > > timer { > compatible = "ti,omap5-timer"; > interrupt-parent = <&timer1>; > interrupts = <1>; > clocks = <&timer1 0>, /* use timer1 as clockevent, clock index 0 = 32k, 1 = sys_ck ... */ > <&counter32k 0>; /* use 32k counter as clocksource */ > }; > >> - ti,timer-dsp: Indicates the timer can interrupt the on-chip DSP in > > This can be probably also be mapped based on the compatible property? > > Regards, > > Tony >
* Joel Fernandes <joelf@ti.com> [131122 08:37]: > Hi Tony, > > Thanks for your comments, few replies inline below.. > > On 11/22/2013 09:58 AM, Tony Lindgren wrote: > > * Joel Fernandes <joelf@ti.com> [131121 18:00]: > >> +Required properties for system timers (clockevents/clocksource): > >> +- ti,timer-parent: System timer's parent mux clock needs to be setup. > >> + This is currently hardcoded in code, for DT boot we > >> + move this to DT. > >> + > > > > This can be replaced with just clk_set_rate, or clk_set_parent if needed. > > Or by having a clocks = <&32k_clk> property in the dmtimer node in the > > .dts file. > > > > Sure, but clock-data is still not available to make this possible in mainline. > We also discussed earlier right that we don't want dependencies as much as > possible to get one chunk in and working at a time. I was thinking like for a > first-pass since there's a lot of unrelated code that doesn't have dependencies, > but needs this to work, we can introduce this property for now and drop it later > as a "cost of migration"? I don't think there's a dependency here to the omap clocks as the dmtimer can implement the clocksource separately and internally still use clk_get using the clock alias table. > >> Optional properties: > >> - ti,timer-alwon: Indicates the timer is in an alway-on power domain. > > > > Hmm this we may not need, this can probably be deciphered from the compatible > > flag already? > > How? Compatible contains the same string, for example for OMAP4: > > timer8 has: > compatible = "ti,omap4430-timer"; > ti,timer-pwm; > ti,timer-dsp; > > and timer9 has: > compatible = "ti,omap4430-timer"; > ti,hwmods = "timer9"; > ti,timer-pwm; > Some of these features are always hardwired certain way and could be mapped to the right timer based on the timer offset and the compatible flag if we want to avoid adding the ti,timer-alwon property. It seems that most omaps have just one always on timer that's the first timer, and only on am33xx it does not exist? I'm fine adding ti,timer-alwon if it help to leave out static data in the driver and avoid patching the driver for every new SoC. But sounds like in this case we really have just the am33xx exception to the rule? > > Then for the users of a specific dmtimer, they can select the right one using > > the interrupt-parent property: > > > > timer1: timer@0x4800abcd { > > compatible = "ti,omap5430-timer"; > > #interrupt-cells = <1>; /* needs irqchip implemented for dmtimer */ > > interrupt-controller; > > #clock-cells = <1>; /* needs clocksource implemented for dmtimer */ > > clock-output-names = "32k", "sys_ck"; > > ... > > }; > > In reference to my last thread reply, irqchip may not be available early in the > boot process (.init_time) for system timer usage? Hmm it should be, looks like we have in arch/arm/kernel/irq.c: void __init init_IRQ(void) { if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq) irqchip_init(); else machine_desc->init_irq(); } Then in init/main.c has: ... early_irq_init(); init_IRQ(); tick_init(); init_timers(); hrtimers_init(); softirq_init(); timekeeping_init(); time_init(); ... So looks like we should have irqchip available? Regards, Tony
* Joel Fernandes <joelf@ti.com> [131122 09:06]: > On 11/22/2013 09:58 AM, Tony Lindgren wrote: > > * Joel Fernandes <joelf@ti.com> [131121 18:00]: > >> +Required properties for system timers (clockevents/clocksource): > >> +- ti,timer-parent: System timer's parent mux clock needs to be setup. > >> + This is currently hardcoded in code, for DT boot we > >> + move this to DT. > >> + > > > > This can be replaced with just clk_set_rate, or clk_set_parent if needed. > > Or by having a clocks = <&32k_clk> property in the dmtimer node in the > > .dts file. > > > >> Optional properties: > >> - ti,timer-alwon: Indicates the timer is in an alway-on power domain. > > > > Hmm this we may not need, this can probably be deciphered from the compatible > > flag already? > > I was thinking maybe we can improve the of_clocksource_init matching > capabilities to match better, not just on compatible but other properties for > better selection? > > For example, a generic clocksource DT property to say that the timer is in the > wakeup domain and so should be preferred as the clocksource? Also additional > properties to force its selection? > > Then we can switch to clocksource_of_init and have it do the selection. Not sure > if there is such a need or value for other ARM platforms so we may be over > complicating the framework to handle these quirks. Comments welcomed about it. Maybe, but so far it seems that it can all be specified in the board specific .dts file using standard properties, so I'd first play with that. Regards, Tony
On 11/22/2013 11:08 AM, Tony Lindgren wrote: > * Joel Fernandes <joelf@ti.com> [131122 08:37]: >> Hi Tony, >> >> Thanks for your comments, few replies inline below.. >> >> On 11/22/2013 09:58 AM, Tony Lindgren wrote: >>> * Joel Fernandes <joelf@ti.com> [131121 18:00]: >>>> +Required properties for system timers (clockevents/clocksource): >>>> +- ti,timer-parent: System timer's parent mux clock needs to be setup. >>>> + This is currently hardcoded in code, for DT boot we >>>> + move this to DT. >>>> + >>> >>> This can be replaced with just clk_set_rate, or clk_set_parent if needed. >>> Or by having a clocks = <&32k_clk> property in the dmtimer node in the >>> .dts file. >>> >> >> Sure, but clock-data is still not available to make this possible in mainline. >> We also discussed earlier right that we don't want dependencies as much as >> possible to get one chunk in and working at a time. I was thinking like for a >> first-pass since there's a lot of unrelated code that doesn't have dependencies, >> but needs this to work, we can introduce this property for now and drop it later >> as a "cost of migration"? > > I don't think there's a dependency here to the omap clocks as the dmtimer > can implement the clocksource separately and internally still use clk_get > using the clock alias table. You mean implement clock-tree separately? Sorry I'm confused can you clarify what you mean? In clock tree data (not upstream), here is the system clock for am335x for example: sys_clkin_ck: sys_clkin_ck@44e10040 { #clock-cells = <0>; compatible = "mux-clock"; It uses the mux-clock driver. Are you saying we duplicate clock-tree stuff? I don't think that's a good idea specially since once clock dt data is available, we will switch to using it. >>>> Optional properties: >>>> - ti,timer-alwon: Indicates the timer is in an alway-on power domain. >>> >>> Hmm this we may not need, this can probably be deciphered from the compatible >>> flag already? >> >> How? Compatible contains the same string, for example for OMAP4: >> >> timer8 has: >> compatible = "ti,omap4430-timer"; >> ti,timer-pwm; >> ti,timer-dsp; >> >> and timer9 has: >> compatible = "ti,omap4430-timer"; >> ti,hwmods = "timer9"; >> ti,timer-pwm; >> > > Some of these features are always hardwired certain way and could be mapped to > the right timer based on the timer offset and the compatible flag if we want > to avoid adding the ti,timer-alwon property. It seems that most omaps have > just one always on timer that's the first timer, and only on am33xx it does > not exist? > > I'm fine adding ti,timer-alwon if it help to leave out static data in the > driver and avoid patching the driver for every new SoC. But sounds like in > this case we really have just the am33xx exception to the rule? ti,timer-alwon may not be needed yes, since on all platforms I've observed first timer has this property. However from OMAP3 dt, timer12 has it too. Not sure what that implies. I think what Jon was trying to do is to find a DT node by property, he had no other way of getting a device_node * otherwise. But notice this macro used for HS (secure devices): OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure", 2, "timer_sys_ck", NULL); How would you specify in DT that you want a node with the timer-secure property as the clocksource if we drop these kind of properties? >>> Then for the users of a specific dmtimer, they can select the right one using >>> the interrupt-parent property: >>> >>> timer1: timer@0x4800abcd { >>> compatible = "ti,omap5430-timer"; >>> #interrupt-cells = <1>; /* needs irqchip implemented for dmtimer */ >>> interrupt-controller; >>> #clock-cells = <1>; /* needs clocksource implemented for dmtimer */ >>> clock-output-names = "32k", "sys_ck"; >>> ... >>> }; >> >> In reference to my last thread reply, irqchip may not be available early in the >> boot process (.init_time) for system timer usage? > > Hmm it should be, looks like we have in arch/arm/kernel/irq.c: > > void __init init_IRQ(void) > { > if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq) > irqchip_init(); > else > machine_desc->init_irq(); > } > > Then in init/main.c has: > ... > early_irq_init(); > init_IRQ(); > tick_init(); > init_timers(); > hrtimers_init(); > softirq_init(); > timekeeping_init(); > time_init(); > ... > > So looks like we should have irqchip available? Right. I think your idea of using irqchip is certainly a clean way. Let me go back to the drawing board and try to see if we have all the pieces we need and there are no surprises in doing it this way. Then we can have a general purpose clocksource driver that uses the irqchip driver. I still worry about things like hwmod that may be need to be called on specific timer, and runtime PM is not available that early in the boot process. thanks, -Joel
* Joel Fernandes <joelf@ti.com> [131122 16:32]: > On 11/22/2013 11:08 AM, Tony Lindgren wrote: > > > > I don't think there's a dependency here to the omap clocks as the dmtimer > > can implement the clocksource separately and internally still use clk_get > > using the clock alias table. > > You mean implement clock-tree separately? Sorry I'm confused can you clarify > what you mean? You could implement the needed clocks for client drivers to use in dmtimer.c directly if dmtimer.c is the gating those clocks. > In clock tree data (not upstream), here is the system clock for am335x for example: > sys_clkin_ck: sys_clkin_ck@44e10040 { > #clock-cells = <0>; > compatible = "mux-clock"; > > It uses the mux-clock driver. Are you saying we duplicate clock-tree stuff? I > don't think that's a good idea specially since once clock dt data is available, > we will switch to using it. Oh OK, then that naturally could be used directly too. > >>>> Optional properties: > >>>> - ti,timer-alwon: Indicates the timer is in an alway-on power domain. > >>> > >>> Hmm this we may not need, this can probably be deciphered from the compatible > >>> flag already? > >> > >> How? Compatible contains the same string, for example for OMAP4: > >> > >> timer8 has: > >> compatible = "ti,omap4430-timer"; > >> ti,timer-pwm; > >> ti,timer-dsp; > >> > >> and timer9 has: > >> compatible = "ti,omap4430-timer"; > >> ti,hwmods = "timer9"; > >> ti,timer-pwm; > >> > > > > Some of these features are always hardwired certain way and could be mapped to > > the right timer based on the timer offset and the compatible flag if we want > > to avoid adding the ti,timer-alwon property. It seems that most omaps have > > just one always on timer that's the first timer, and only on am33xx it does > > not exist? > > > > I'm fine adding ti,timer-alwon if it help to leave out static data in the > > driver and avoid patching the driver for every new SoC. But sounds like in > > this case we really have just the am33xx exception to the rule? > > ti,timer-alwon may not be needed yes, since on all platforms I've observed first > timer has this property. However from OMAP3 dt, timer12 has it too. Not sure > what that implies. I guess you could mark timer1 and 12 as always on if the compatible flag matches ti,omap3430-timer. > I think what Jon was trying to do is to find a DT node by property, he had no > other way of getting a device_node * otherwise. > > But notice this macro used for HS (secure devices): > OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure", > 2, "timer_sys_ck", NULL); > > How would you specify in DT that you want a node with the timer-secure property > as the clocksource if we drop these kind of properties? timer { interrupt-parent = <&timer12>; ... } Should do the trick I think :) > >>> Then for the users of a specific dmtimer, they can select the right one using > >>> the interrupt-parent property: > >>> > >>> timer1: timer@0x4800abcd { > >>> compatible = "ti,omap5430-timer"; > >>> #interrupt-cells = <1>; /* needs irqchip implemented for dmtimer */ > >>> interrupt-controller; > >>> #clock-cells = <1>; /* needs clocksource implemented for dmtimer */ > >>> clock-output-names = "32k", "sys_ck"; > >>> ... > >>> }; > >> > >> In reference to my last thread reply, irqchip may not be available early in the > >> boot process (.init_time) for system timer usage? > > > > Hmm it should be, looks like we have in arch/arm/kernel/irq.c: > > > > void __init init_IRQ(void) > > { > > if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq) > > irqchip_init(); > > else > > machine_desc->init_irq(); > > } > > > > Then in init/main.c has: > > ... > > early_irq_init(); > > init_IRQ(); > > tick_init(); > > init_timers(); > > hrtimers_init(); > > softirq_init(); > > timekeeping_init(); > > time_init(); > > ... > > > > So looks like we should have irqchip available? > > Right. I think your idea of using irqchip is certainly a clean way. Let me go > back to the drawing board and try to see if we have all the pieces we need and > there are no surprises in doing it this way. OK cool. > Then we can have a general purpose clocksource driver that uses the irqchip > driver. I still worry about things like hwmod that may be need to be called on > specific timer, and runtime PM is not available that early in the boot process. Yeah we may still need a piece of code in mach-omap2 for now if runtime PM is not available at that point yet. Regards, Tony
diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt index 6cf7a75..644bc61 100644 --- a/Documentation/devicetree/bindings/arm/omap/timer.txt +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt @@ -24,6 +24,11 @@ Required properties: where <X> is the instance number of the timer from the HW spec. +Required properties for system timers (clockevents/clocksource): +- ti,timer-parent: System timer's parent mux clock needs to be setup. + This is currently hardcoded in code, for DT boot we + move this to DT. + Optional properties: - ti,timer-alwon: Indicates the timer is in an alway-on power domain. - ti,timer-dsp: Indicates the timer can interrupt the on-chip DSP in
Timer's parent mux clocks require a parent clock alias, provide the same from device tree. Ultimately this will be provided from DT as clock node phandles but clk-mux driver DT bindings series is still under going review. So to keep things working during the timer migration, we add this property. Originally this was hardcoded in timer.c, now we do the same in DT and would be removing the hardcoded names from timer.c Signed-off-by: Joel Fernandes <joelf@ti.com> --- Documentation/devicetree/bindings/arm/omap/timer.txt | 5 +++++ 1 file changed, 5 insertions(+)