| Message ID | 1385085414-9034-6-git-send-email-joelf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Nov 21, 2013 at 07:56:51PM -0600, Joel Fernandes wrote: [...] > New functions for OF-only boot are introduced, and we can soon delete the old > versions once we migrate all platforms. Currently only AM335x is migrated and actually, you don't need to initialize .init_timer at all in DT boot. Just use CLKSOURCE_OF_DECLARE() and pass your omap_generic_timer_init() as argument (although, I'd call it omap_of_timer_init()). That will put of_device_id structure on a special section (__clksource_of_table) and pass your function as a data argument. That function will be called automatically during init.
On 11/21/2013 09:51 PM, Felipe Balbi wrote: > Hi, > > On Thu, Nov 21, 2013 at 07:56:51PM -0600, Joel Fernandes wrote: > > [...] > >> New functions for OF-only boot are introduced, and we can soon delete the old >> versions once we migrate all platforms. Currently only AM335x is migrated and > > actually, you don't need to initialize .init_timer at all in DT boot. Actually we still do, because the plan is to keep the hwmod stuff that's required in timer.c in a custom .init_time, and then of_clocksource_init maybe called to do what you're suggesting but (not yet) more on that below. > Just use CLKSOURCE_OF_DECLARE() and pass your omap_generic_timer_init() > as argument (although, I'd call it omap_of_timer_init()). > That will put of_device_id structure on a special section > (__clksource_of_table) and pass your function as a data argument. That > function will be called automatically during init. > I thought of doing that, but currently the timer selection for clocksource is not a simple matching of compatible string, rather it is selecting the timer based on properties such as ti,timer-alwon and such. In omap3 for example, there are needs for specific timers and such have been provided with the macros passing in timer id etc. Right now, this can be forced through DT with the "ti,timer-clocksource" property I introduced. All this selection algorithm is too complex to be handle directly by the CLOCKSOURCE_OF_DECLARE / clocksource_of_init matching mechanism. thanks, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote: > This work is a migration effort of OMAP system timers to the > clocksource/clockevent framework. Consider this as a first-pass in this effort. > There are few cleanups that need to be done first. The HWMOD code is > intertwined with the timer code. HWMOD code cleanups in the future will > hopefully make most of this code go away, so till then we separate out the > power/clocks portion of the code from the actual timer bits. This will > facilitate near-future work of adapting the system timer as a clocksource. > > New functions for OF-only boot are introduced, and we can soon delete the old > versions once we migrate all platforms. Currently only AM335x is migrated and > testedA new omap_generic_timer_init function is introduced for DT platforms. > Code required earlier for non-DT platforms such as setup of timer IDs and timer > parent clock is not required. parent clocks are automatically setup by the mux > clock driver through DT so they no longer need to be hardcoded. > > The init code will try to pick the best timer for clocksource and clockevent > however bindings are added to force a particular timer as clocksource or > clockevent through DT. > > Signed-off-by: Joel Fernandes <joelf@ti.com> > --- > .../devicetree/bindings/arm/omap/timer.txt | 12 ++ > arch/arm/mach-omap2/common.h | 1 + > arch/arm/mach-omap2/timer.c | 235 +++++++++++++++++++++ > 3 files changed, 248 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt > index d02e27c..6cf7a75 100644 > --- a/Documentation/devicetree/bindings/arm/omap/timer.txt > +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt > @@ -32,6 +32,18 @@ Optional properties: > - ti,timer-secure: Indicates the timer is reserved on a secure OMAP device > and therefore cannot be used by the kernel. > > +- ti,timer-clockevent, > + ti,timer-clocksource These properties force the system timer code to choose > + the particular timer as a clockevent or clocksource. > + If these properties are not specified, the timer code > + picks up a "ti,timer-alwon" as the clocksource and a > + timer containing one of the following properties as > + the clockevent in the following order: > + ti,timer-alwon > + ti,timer-dsp > + ti,timer-pwm > + ti,timer-secure These properties were added specifically for the reason of avoiding linux specific properties like these. When is this not sufficient? And I agree with the comment to use OF_CLKSRC. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/22/2013 09:58 AM, Rob Herring wrote: > On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote: >> This work is a migration effort of OMAP system timers to the >> clocksource/clockevent framework. Consider this as a first-pass in this effort. >> There are few cleanups that need to be done first. The HWMOD code is >> intertwined with the timer code. HWMOD code cleanups in the future will >> hopefully make most of this code go away, so till then we separate out the >> power/clocks portion of the code from the actual timer bits. This will >> facilitate near-future work of adapting the system timer as a clocksource. >> >> New functions for OF-only boot are introduced, and we can soon delete the old >> versions once we migrate all platforms. Currently only AM335x is migrated and >> testedA new omap_generic_timer_init function is introduced for DT platforms. >> Code required earlier for non-DT platforms such as setup of timer IDs and timer >> parent clock is not required. parent clocks are automatically setup by the mux >> clock driver through DT so they no longer need to be hardcoded. >> >> The init code will try to pick the best timer for clocksource and clockevent >> however bindings are added to force a particular timer as clocksource or >> clockevent through DT. >> >> Signed-off-by: Joel Fernandes <joelf@ti.com> >> --- >> .../devicetree/bindings/arm/omap/timer.txt | 12 ++ >> arch/arm/mach-omap2/common.h | 1 + >> arch/arm/mach-omap2/timer.c | 235 +++++++++++++++++++++ >> 3 files changed, 248 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt >> index d02e27c..6cf7a75 100644 >> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt >> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt >> @@ -32,6 +32,18 @@ Optional properties: >> - ti,timer-secure: Indicates the timer is reserved on a secure OMAP device >> and therefore cannot be used by the kernel. >> >> +- ti,timer-clockevent, >> + ti,timer-clocksource These properties force the system timer code to choose >> + the particular timer as a clockevent or clocksource. >> + If these properties are not specified, the timer code >> + picks up a "ti,timer-alwon" as the clocksource and a >> + timer containing one of the following properties as >> + the clockevent in the following order: >> + ti,timer-alwon >> + ti,timer-dsp >> + ti,timer-pwm >> + ti,timer-secure > > These properties were added specifically for the reason of avoiding > linux specific properties like these. When is this not sufficient? Some platforms cannot use certain timers as clockevents and clocksource, to keep this code functionally equivalent and working, I added these properties so that its possible to select specific timers as clockevents/sources as was being done in the non-DT case. I'm open to suggestions for doing this in a better way for DT. > > And I agree with the comment to use OF_CLKSRC. > There are difficulties I mentioned in a previous post [1] stating why it may not be possible to use OF_CLKSRC macros, and still keep the code functionally equivalent to when it was non-DT. thanks, -Joel [1] http://marc.info/?l=linux-arm-kernel&m=138513299917850&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 22, 2013 at 10:42 AM, Joel Fernandes <joelf@ti.com> wrote: > On 11/22/2013 09:58 AM, Rob Herring wrote: >> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote: >>> This work is a migration effort of OMAP system timers to the >>> clocksource/clockevent framework. Consider this as a first-pass in this effort. >>> There are few cleanups that need to be done first. The HWMOD code is >>> intertwined with the timer code. HWMOD code cleanups in the future will >>> hopefully make most of this code go away, so till then we separate out the >>> power/clocks portion of the code from the actual timer bits. This will >>> facilitate near-future work of adapting the system timer as a clocksource. >>> >>> New functions for OF-only boot are introduced, and we can soon delete the old >>> versions once we migrate all platforms. Currently only AM335x is migrated and >>> testedA new omap_generic_timer_init function is introduced for DT platforms. >>> Code required earlier for non-DT platforms such as setup of timer IDs and timer >>> parent clock is not required. parent clocks are automatically setup by the mux >>> clock driver through DT so they no longer need to be hardcoded. >>> >>> The init code will try to pick the best timer for clocksource and clockevent >>> however bindings are added to force a particular timer as clocksource or >>> clockevent through DT. >>> >>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>> --- >>> .../devicetree/bindings/arm/omap/timer.txt | 12 ++ >>> arch/arm/mach-omap2/common.h | 1 + >>> arch/arm/mach-omap2/timer.c | 235 +++++++++++++++++++++ >>> 3 files changed, 248 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt >>> index d02e27c..6cf7a75 100644 >>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt >>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt >>> @@ -32,6 +32,18 @@ Optional properties: >>> - ti,timer-secure: Indicates the timer is reserved on a secure OMAP device >>> and therefore cannot be used by the kernel. >>> >>> +- ti,timer-clockevent, >>> + ti,timer-clocksource These properties force the system timer code to choose >>> + the particular timer as a clockevent or clocksource. >>> + If these properties are not specified, the timer code >>> + picks up a "ti,timer-alwon" as the clocksource and a >>> + timer containing one of the following properties as >>> + the clockevent in the following order: >>> + ti,timer-alwon >>> + ti,timer-dsp >>> + ti,timer-pwm >>> + ti,timer-secure >> >> These properties were added specifically for the reason of avoiding >> linux specific properties like these. When is this not sufficient? > > Some platforms cannot use certain timers as clockevents and clocksource, to keep > this code functionally equivalent and working, I added these properties so that > its possible to select specific timers as clockevents/sources as was being done > in the non-DT case. I'm open to suggestions for doing this in a better way for DT. There has to be a defined reason why a given timer cannot be used. You are not explaining what that reason is. Define a property or set of properties that describe the h/w feature or quirk. >> >> And I agree with the comment to use OF_CLKSRC. >> > > There are difficulties I mentioned in a previous post [1] stating why it may not > be possible to use OF_CLKSRC macros, and still keep the code functionally > equivalent to when it was non-DT. Sorry, I don't buy it. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Adding Thomas to the thread since discussion is about clocksource, and Mark Rutland as discussion is related to timers and DT, thanks. On 11/22/2013 02:01 PM, Rob Herring wrote: > On Fri, Nov 22, 2013 at 10:42 AM, Joel Fernandes <joelf@ti.com> wrote: >> On 11/22/2013 09:58 AM, Rob Herring wrote: >>> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote: >>>> This work is a migration effort of OMAP system timers to the >>>> clocksource/clockevent framework. Consider this as a first-pass in this effort. >>>> There are few cleanups that need to be done first. The HWMOD code is >>>> intertwined with the timer code. HWMOD code cleanups in the future will >>>> hopefully make most of this code go away, so till then we separate out the >>>> power/clocks portion of the code from the actual timer bits. This will >>>> facilitate near-future work of adapting the system timer as a clocksource. >>>> >>>> New functions for OF-only boot are introduced, and we can soon delete the old >>>> versions once we migrate all platforms. Currently only AM335x is migrated and >>>> testedA new omap_generic_timer_init function is introduced for DT platforms. >>>> Code required earlier for non-DT platforms such as setup of timer IDs and timer >>>> parent clock is not required. parent clocks are automatically setup by the mux >>>> clock driver through DT so they no longer need to be hardcoded. >>>> >>>> The init code will try to pick the best timer for clocksource and clockevent >>>> however bindings are added to force a particular timer as clocksource or >>>> clockevent through DT. >>>> >>>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>>> --- >>>> .../devicetree/bindings/arm/omap/timer.txt | 12 ++ >>>> arch/arm/mach-omap2/common.h | 1 + >>>> arch/arm/mach-omap2/timer.c | 235 +++++++++++++++++++++ >>>> 3 files changed, 248 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt >>>> index d02e27c..6cf7a75 100644 >>>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt >>>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt >>>> @@ -32,6 +32,18 @@ Optional properties: >>>> - ti,timer-secure: Indicates the timer is reserved on a secure OMAP device >>>> and therefore cannot be used by the kernel. >>>> >>>> +- ti,timer-clockevent, >>>> + ti,timer-clocksource These properties force the system timer code to choose >>>> + the particular timer as a clockevent or clocksource. >>>> + If these properties are not specified, the timer code >>>> + picks up a "ti,timer-alwon" as the clocksource and a >>>> + timer containing one of the following properties as >>>> + the clockevent in the following order: >>>> + ti,timer-alwon >>>> + ti,timer-dsp >>>> + ti,timer-pwm >>>> + ti,timer-secure >>> >>> These properties were added specifically for the reason of avoiding >>> linux specific properties like these. When is this not sufficient? >> >> Some platforms cannot use certain timers as clockevents and clocksource, to keep >> this code functionally equivalent and working, I added these properties so that >> its possible to select specific timers as clockevents/sources as was being done >> in the non-DT case. I'm open to suggestions for doing this in a better way for DT. > > There has to be a defined reason why a given timer cannot be used. You > are not explaining what that reason is. Define a property or set of > properties that describe the h/w feature or quirk. Reason? There are HW bugs that prevent a timer from being used. See [1]. And there may be other reasons, I haven't written this code but I know that there are other HW bugs that made authors pick a specific timer such as board issues or accuracy. This is nothing new, just that I'm trying to find a way to do it from DT. You can see the ifdef'ry below in mach-omap2/timer.c. All I'm trying to do is make it simpler and cleaner by adding these properties to DT so that we can delete this code entirely. #ifdef CONFIG_ARCH_OMAP2 OMAP_SYS_32K_TIMER_INIT(2, 1, "timer_32k_ck", "ti,timer-alwon", 2, "timer_sys_ck", NULL); #endif /* CONFIG_ARCH_OMAP2 */ #if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM43XX) OMAP_SYS_32K_TIMER_INIT(3, 1, "timer_32k_ck", "ti,timer-alwon", 2, "timer_sys_ck", NULL); OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure", 2, "timer_sys_ck", NULL); #endif /* CONFIG_ARCH_OMAP3 */ etc... >>> >>> And I agree with the comment to use OF_CLKSRC. >>> >> >> There are difficulties I mentioned in a previous post [1] stating why it may not >> be possible to use OF_CLKSRC macros, and still keep the code functionally >> equivalent to when it was non-DT. > > Sorry, I don't buy it. > Ofcourse even I want to use OF_CLOCKSOURCE macros for registering DT clocksources, but re-iterating, the selection of timer is not a simple match of compatible property as you may see in my patch. Some platforms need specific timer as clocksource, all timers have same "compatible" flag. How would you differentiate? Refer to ifdef's above to see how timer IDs change across platforms in the non-DT code that's being converted. I think clocksource_of_init has been written such that any timer having a matching compatible can be enumerated. I can change the compatible to something unique in the DT like: timer1:timer@... { compatible = "ti,dmtimer-clocksource" .... } and that may make clocksource work. But then such a similar approach to clockevent is not possible. What I observed in other platforms is a *single* DT node represents both clocksource and clockevent. For example, in zevio-timer: drivers/clocksource/zevio-timer.c does this: CLOCKSOURCE_OF_DECLARE(zevio_timer, "lsi,zevio-timer", zevio_timer_add); In the body of zevio_timer_add(struct device_node *node), they do this: timer->base = of_iomap(node, 0); timer->timer1 = timer->base + IO_TIMER1; // USED AS CLOCKSOURCE timer->timer2 = timer->base + IO_TIMER2; // USED AS CLOCKEVENT What's happening here is a single device node (DT node) represents both clockevent and source. But for OMAP, we use different timers for clockevent and clocksource. They are physically 2 different timer DT nodes. How do we register something like this with the CLOCKSOURCE_OF_DECLARE macro? Looking forward to your suggestions. thanks, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry, resending due to an MUA issue. Adding Thomas to the thread since discussion is about clocksource, and Mark Rutland as discussion is related to timers and DT, thanks. On 11/22/2013 02:01 PM, Rob Herring wrote: > On Fri, Nov 22, 2013 at 10:42 AM, Joel Fernandes <joelf@ti.com> wrote: >> On 11/22/2013 09:58 AM, Rob Herring wrote: >>> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote: >>>> This work is a migration effort of OMAP system timers to the >>>> clocksource/clockevent framework. Consider this as a first-pass in this effort. >>>> There are few cleanups that need to be done first. The HWMOD code is >>>> intertwined with the timer code. HWMOD code cleanups in the future will >>>> hopefully make most of this code go away, so till then we separate out the >>>> power/clocks portion of the code from the actual timer bits. This will >>>> facilitate near-future work of adapting the system timer as a clocksource. >>>> >>>> New functions for OF-only boot are introduced, and we can soon delete the old >>>> versions once we migrate all platforms. Currently only AM335x is migrated and >>>> testedA new omap_generic_timer_init function is introduced for DT platforms. >>>> Code required earlier for non-DT platforms such as setup of timer IDs and timer >>>> parent clock is not required. parent clocks are automatically setup by the mux >>>> clock driver through DT so they no longer need to be hardcoded. >>>> >>>> The init code will try to pick the best timer for clocksource and clockevent >>>> however bindings are added to force a particular timer as clocksource or >>>> clockevent through DT. >>>> >>>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>>> --- >>>> .../devicetree/bindings/arm/omap/timer.txt | 12 ++ >>>> arch/arm/mach-omap2/common.h | 1 + >>>> arch/arm/mach-omap2/timer.c | 235 +++++++++++++++++++++ >>>> 3 files changed, 248 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt >>>> index d02e27c..6cf7a75 100644 >>>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt >>>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt >>>> @@ -32,6 +32,18 @@ Optional properties: >>>> - ti,timer-secure: Indicates the timer is reserved on a secure OMAP device >>>> and therefore cannot be used by the kernel. >>>> >>>> +- ti,timer-clockevent, >>>> + ti,timer-clocksource These properties force the system timer code to choose >>>> + the particular timer as a clockevent or clocksource. >>>> + If these properties are not specified, the timer code >>>> + picks up a "ti,timer-alwon" as the clocksource and a >>>> + timer containing one of the following properties as >>>> + the clockevent in the following order: >>>> + ti,timer-alwon >>>> + ti,timer-dsp >>>> + ti,timer-pwm >>>> + ti,timer-secure >>> >>> These properties were added specifically for the reason of avoiding >>> linux specific properties like these. When is this not sufficient? >> >> Some platforms cannot use certain timers as clockevents and clocksource, to keep >> this code functionally equivalent and working, I added these properties so that >> its possible to select specific timers as clockevents/sources as was being done >> in the non-DT case. I'm open to suggestions for doing this in a better way for DT. > > There has to be a defined reason why a given timer cannot be used. You > are not explaining what that reason is. Define a property or set of > properties that describe the h/w feature or quirk. Reason? There are HW bugs that prevent a timer from being used. See [1]. And there may be other reasons, I haven't written this code but I know that there are other HW bugs that made authors pick a specific timer such as board issues or accuracy. This is nothing new, just that I'm trying to find a way to do it from DT. You can see the ifdef'ry below in mach-omap2/timer.c. All I'm trying to do is make it simpler and cleaner by adding these properties to DT so that we can delete this code entirely. #ifdef CONFIG_ARCH_OMAP2 OMAP_SYS_32K_TIMER_INIT(2, 1, "timer_32k_ck", "ti,timer-alwon", 2, "timer_sys_ck", NULL); #endif /* CONFIG_ARCH_OMAP2 */ #if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM43XX) OMAP_SYS_32K_TIMER_INIT(3, 1, "timer_32k_ck", "ti,timer-alwon", 2, "timer_sys_ck", NULL); OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure", 2, "timer_sys_ck", NULL); #endif /* CONFIG_ARCH_OMAP3 */ etc... >>> >>> And I agree with the comment to use OF_CLKSRC. >>> >> >> There are difficulties I mentioned in a previous post [1] stating why it may not >> be possible to use OF_CLKSRC macros, and still keep the code functionally >> equivalent to when it was non-DT. > > Sorry, I don't buy it. > Ofcourse even I want to use OF_CLOCKSOURCE macros for registering DT clocksources, but re-iterating, the selection of timer is not a simple match of compatible property as you may see in my patch. Some platforms need specific timer as clocksource, all timers have same "compatible" flag. How would you differentiate? Refer to ifdef's above to see how timer IDs change across platforms in the non-DT code that's being converted. I think clocksource_of_init has been written such that any timer having a matching compatible can be enumerated. I can change the compatible to something unique in the DT like: timer1:timer@... { compatible = "ti,dmtimer-clocksource" .... } and that may make clocksource work. But then such a similar approach to clockevent is not possible. What I observed in other platforms is a *single* DT node represents both clocksource and clockevent. For example, in zevio-timer: drivers/clocksource/zevio-timer.c does this: CLOCKSOURCE_OF_DECLARE(zevio_timer, "lsi,zevio-timer", zevio_timer_add); In the body of zevio_timer_add(struct device_node *node), they do this: timer->base = of_iomap(node, 0); timer->timer1 = timer->base + IO_TIMER1; // USED AS CLOCKSOURCE timer->timer2 = timer->base + IO_TIMER2; // USED AS CLOCKEVENT What's happening here is a single device node (DT node) represents both clockevent and source. But for OMAP, we use different timers for clockevent and clocksource. They are physically 2 different timer DT nodes. How do we register something like this with the CLOCKSOURCE_OF_DECLARE macro? Looking forward to your suggestions. thanks, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 22, 2013 at 7:12 PM, Joel Fernandes <joelf@ti.com> wrote: > Adding Thomas to the thread since discussion is about clocksource, and Mark > Rutland as discussion is related to timers and DT, thanks. > > On 11/22/2013 02:01 PM, Rob Herring wrote: >> On Fri, Nov 22, 2013 at 10:42 AM, Joel Fernandes <joelf@ti.com> wrote: >>> On 11/22/2013 09:58 AM, Rob Herring wrote: >>>> On Thu, Nov 21, 2013 at 7:56 PM, Joel Fernandes <joelf@ti.com> wrote: >>>>> This work is a migration effort of OMAP system timers to the >>>>> clocksource/clockevent framework. Consider this as a first-pass in this effort. >>>>> There are few cleanups that need to be done first. The HWMOD code is >>>>> intertwined with the timer code. HWMOD code cleanups in the future will >>>>> hopefully make most of this code go away, so till then we separate out the >>>>> power/clocks portion of the code from the actual timer bits. This will >>>>> facilitate near-future work of adapting the system timer as a clocksource. >>>>> >>>>> New functions for OF-only boot are introduced, and we can soon delete the old >>>>> versions once we migrate all platforms. Currently only AM335x is migrated and >>>>> testedA new omap_generic_timer_init function is introduced for DT platforms. >>>>> Code required earlier for non-DT platforms such as setup of timer IDs and timer >>>>> parent clock is not required. parent clocks are automatically setup by the mux >>>>> clock driver through DT so they no longer need to be hardcoded. >>>>> >>>>> The init code will try to pick the best timer for clocksource and clockevent >>>>> however bindings are added to force a particular timer as clocksource or >>>>> clockevent through DT. >>>>> >>>>> Signed-off-by: Joel Fernandes <joelf@ti.com> >>>>> --- >>>>> .../devicetree/bindings/arm/omap/timer.txt | 12 ++ >>>>> arch/arm/mach-omap2/common.h | 1 + >>>>> arch/arm/mach-omap2/timer.c | 235 +++++++++++++++++++++ >>>>> 3 files changed, 248 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt >>>>> index d02e27c..6cf7a75 100644 >>>>> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt >>>>> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt >>>>> @@ -32,6 +32,18 @@ Optional properties: >>>>> - ti,timer-secure: Indicates the timer is reserved on a secure OMAP device >>>>> and therefore cannot be used by the kernel. >>>>> >>>>> +- ti,timer-clockevent, >>>>> + ti,timer-clocksource These properties force the system timer code to choose >>>>> + the particular timer as a clockevent or clocksource. >>>>> + If these properties are not specified, the timer code >>>>> + picks up a "ti,timer-alwon" as the clocksource and a >>>>> + timer containing one of the following properties as >>>>> + the clockevent in the following order: >>>>> + ti,timer-alwon >>>>> + ti,timer-dsp >>>>> + ti,timer-pwm >>>>> + ti,timer-secure >>>> >>>> These properties were added specifically for the reason of avoiding >>>> linux specific properties like these. When is this not sufficient? >>> >>> Some platforms cannot use certain timers as clockevents and clocksource, to keep >>> this code functionally equivalent and working, I added these properties so that >>> its possible to select specific timers as clockevents/sources as was being done >>> in the non-DT case. I'm open to suggestions for doing this in a better way for DT. >> >> There has to be a defined reason why a given timer cannot be used. You >> are not explaining what that reason is. Define a property or set of >> properties that describe the h/w feature or quirk. > > Reason? There are HW bugs that prevent a timer from being used. See [1]. And > there may be other reasons, I haven't written this code but I know that there > are other HW bugs that made authors pick a specific timer such as board issues > or accuracy. Bugs are a feature of the h/w. Add a "ti,timer-broken" property if you don't know the specific bug. Accuracy is a function of counter bit size and frequency. The only board issues for a timer I can imagine is you want to reserve timers with output compare or input capture pins for other uses. These are all properties that you can describe in the binding. If you just don't want to use a timer for some arbitrary reason, then add a 'status = "disabled";' property. > > This is nothing new, just that I'm trying to find a way to do it from DT. > > You can see the ifdef'ry below in mach-omap2/timer.c. All I'm trying to do is > make it simpler and cleaner by adding these properties to DT so that we can > delete this code entirely. > > > #ifdef CONFIG_ARCH_OMAP2 > OMAP_SYS_32K_TIMER_INIT(2, 1, "timer_32k_ck", "ti,timer-alwon", > 2, "timer_sys_ck", NULL); > #endif /* CONFIG_ARCH_OMAP2 */ > > #if defined(CONFIG_ARCH_OMAP3) || defined(CONFIG_SOC_AM43XX) > OMAP_SYS_32K_TIMER_INIT(3, 1, "timer_32k_ck", "ti,timer-alwon", > 2, "timer_sys_ck", NULL); > OMAP_SYS_32K_TIMER_INIT(3_secure, 12, "secure_32k_fck", "ti,timer-secure", > 2, "timer_sys_ck", NULL); > #endif /* CONFIG_ARCH_OMAP3 */ Hence why compatible properties are supposed to be specific. You should have ti,omap2-timer, ti,omap3-timer, and ti,am43xx-timer as compatible properties. > etc... > >>>> >>>> And I agree with the comment to use OF_CLKSRC. >>>> >>> >>> There are difficulties I mentioned in a previous post [1] stating why it may not >>> be possible to use OF_CLKSRC macros, and still keep the code functionally >>> equivalent to when it was non-DT. >> >> Sorry, I don't buy it. >> > > Ofcourse even I want to use OF_CLOCKSOURCE macros for registering DT > clocksources, but re-iterating, the selection of timer is not a simple match of > compatible property as you may see in my patch. Some platforms need specific > timer as clocksource, all timers have same "compatible" flag. How would you > differentiate? Refer to ifdef's above to see how timer IDs change across > platforms in the non-DT code that's being converted. > > I think clocksource_of_init has been written such that any timer having a > matching compatible can be enumerated. > > I can change the compatible to something unique in the DT like: > > timer1:timer@... { > compatible = "ti,dmtimer-clocksource" > .... > } > > and that may make clocksource work. But then such a similar approach to > clockevent is not possible. What I observed in other platforms is a *single* DT > node represents both clocksource and clockevent. Because the timer h/w represents a single IP block in these cases. This is the case for sp804 which I wrote. And I also care which timer is which function because only 1 timer has an interrupt connected. All this was handled by describing the h/w and leaving it to the kernel to figure out which timer is which. > > For example, in zevio-timer: > drivers/clocksource/zevio-timer.c > > does this: > CLOCKSOURCE_OF_DECLARE(zevio_timer, "lsi,zevio-timer", zevio_timer_add); > > In the body of zevio_timer_add(struct device_node *node), they do this: > timer->base = of_iomap(node, 0); > timer->timer1 = timer->base + IO_TIMER1; // USED AS CLOCKSOURCE > timer->timer2 = timer->base + IO_TIMER2; // USED AS CLOCKEVENT > > What's happening here is a single device node (DT node) represents both > clockevent and source. But for OMAP, we use different timers for clockevent and > clocksource. They are physically 2 different timer DT nodes. How do we register > something like this with the CLOCKSOURCE_OF_DECLARE macro? To start with, declare a macro and init function for each SOC and add an of_machine_is_compatible check if you need to different selection logic for each SOC. You might look at integrator_cp_of_init which deals with having 3 different timers. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt index d02e27c..6cf7a75 100644 --- a/Documentation/devicetree/bindings/arm/omap/timer.txt +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt @@ -32,6 +32,18 @@ Optional properties: - ti,timer-secure: Indicates the timer is reserved on a secure OMAP device and therefore cannot be used by the kernel. +- ti,timer-clockevent, + ti,timer-clocksource These properties force the system timer code to choose + the particular timer as a clockevent or clocksource. + If these properties are not specified, the timer code + picks up a "ti,timer-alwon" as the clocksource and a + timer containing one of the following properties as + the clockevent in the following order: + ti,timer-alwon + ti,timer-dsp + ti,timer-pwm + ti,timer-secure + Example: timer12: timer@48304000 { diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h index 4a5684b..2a6b588 100644 --- a/arch/arm/mach-omap2/common.h +++ b/arch/arm/mach-omap2/common.h @@ -86,6 +86,7 @@ extern void omap3_secure_sync32k_timer_init(void); extern void omap3_gptimer_timer_init(void); extern void omap4_local_timer_init(void); extern void omap5_realtime_timer_init(void); +void omap_generic_timer_init(void); void omap2420_init_early(void); void omap2430_init_early(void); diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index dd41f57..8ee2de0 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -318,6 +318,89 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer, return r; } +static int __init omap_dmtimer_power_init(struct omap_dm_timer *timer, + struct device_node *np, + bool is_counter) { + struct omap_hwmod *oh; + const char *oh_name = NULL; + struct clk *src; + const char *parent; + int r; + + of_property_read_string_index(np, "ti,hwmods", 0, &oh_name); + if (!oh_name) + return -ENODEV; + + oh = omap_hwmod_lookup(oh_name); + if (!oh) + return -ENODEV; + + omap_hwmod_setup_one(oh_name); + + if (!is_counter) { + if (oh->_clk) + timer->fclk = oh->_clk; + else + timer->fclk = clk_get(NULL, + omap_hwmod_get_main_clk(oh)); + if (IS_ERR(timer->fclk)) + return PTR_ERR(timer->fclk); + + parent = of_get_property(np, "ti,timer-parent", NULL); + if (!parent) { + pr_err("ti,timer-parent required for system timer\n"); + return -1; + } + + src = clk_get(NULL, parent); + if (IS_ERR(src)) { + pr_err("Couldn't clk_get timer parent\n"); + return PTR_ERR(src); + } + + if (clk_get_parent(timer->fclk) != src) { + r = clk_set_parent(timer->fclk, src); + if (r < 0) { + pr_err("%s: %s cannot set source\n", __func__, + oh->name); + clk_put(src); + return r; + } + } + clk_put(src); + } + + omap_hwmod_enable(oh); + return 0; +} + +static int __init omap_dmtimer_init_one(struct omap_dm_timer *timer, + struct device_node *np, + int posted) +{ + timer->irq = irq_of_parse_and_map(np, 0); + if (!timer->irq) + return -ENXIO; + + timer->io_base = of_iomap(np, 0); + if (!timer->io_base) + return -ENXIO; + + __omap_dm_timer_init_regs(timer); + + if (posted) + __omap_dm_timer_enable_posted(timer); + + /* Check that the intended posted configuration matches the actual */ + if (posted != timer->posted) + return -EINVAL; + + timer->rate = clk_get_rate(timer->fclk); + timer->reserved = 1; + + return 0; +} + static void __init omap2_gp_clockevent_init(int gptimer_id, const char *fck_source, const char *property) @@ -353,6 +436,41 @@ static void __init omap2_gp_clockevent_init(int gptimer_id, clkev.rate); } +static int __init omap_clockevent_init(struct device_node *np) +{ + int res; + + clkev.errata = omap_dm_timer_get_errata(); + + /* + * For clock-event timers we never read the timer counter and + * so we are not impacted by errata i103 and i767. Therefore, + * we can safely ignore this errata for clock-event timers. + */ + __omap_dm_timer_override_errata(&clkev, OMAP_TIMER_ERRATA_I103_I767); + + clockevent_gpt.name = "timer_clkev"; + + res = omap_dmtimer_init_one(&clkev, np, OMAP_TIMER_POSTED); + if (res) + return res; + + omap2_gp_timer_irq.dev_id = &clkev; + setup_irq(clkev.irq, &omap2_gp_timer_irq); + + __omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW); + + clockevent_gpt.cpumask = cpu_possible_mask; + clockevent_gpt.irq = omap_dm_timer_get_irq(&clkev); + clockevents_config_and_register(&clockevent_gpt, clkev.rate, + 3, /* Timer internal resynch latency */ + 0xffffffff); + + pr_info("OMAP clockevent source: %s at %lu Hz\n", clockevent_gpt.name, + clkev.rate); + return 0; +} + /* Clocksource code */ static struct omap_dm_timer clksrc; static bool use_gptimer_clksrc; @@ -448,6 +566,31 @@ static int __init __maybe_unused omap2_sync32k_clocksource_init(void) return ret; } +/* Setup free-running counter for clocksource */ +static int __init omap_sync32k_clocksource_init(struct device_node *np) +{ + int ret = 0; + void __iomem *vbase; + + vbase = of_iomap(np, 0); + if (!vbase) { + pr_err("%s: failed to get counter_32k resource\n", __func__); + return -ENXIO; + } + + ret = omap_init_clocksource_32k(vbase); + if (ret) { + pr_err("%s: failed to initialize counter_32k clocksource(%d)\n", + __func__, ret); + return ret; + } + + pr_err("Sync32k clocksource initialized\n"); + + return ret; +} + + static void __init omap2_gptimer_clocksource_init(int gptimer_id, const char *fck_source, const char *property) @@ -475,6 +618,39 @@ static void __init omap2_gptimer_clocksource_init(int gptimer_id, clocksource_gpt.name, clksrc.rate); } +static int __init omap_clocksource_init(struct device_node *np) +{ + int res = 0; + + clksrc.errata = omap_dm_timer_get_errata(); + + if (strlen(np->name) > 7) { + pr_err("%s: OF node name too big\n", __func__); + return -ENODEV; + } + clocksource_gpt.name = "timer_clksrc"; + + res = omap_dmtimer_init_one(&clksrc, np, OMAP_TIMER_NONPOSTED); + if (res) + return res; + + __omap_dm_timer_load_start(&clksrc, + OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, + OMAP_TIMER_NONPOSTED); + setup_sched_clock(dmtimer_read_sched_clock, 32, clksrc.rate); + + res = clocksource_register_hz(&clocksource_gpt, clksrc.rate); + if (res) { + pr_err("Could not register clocksource %s\n", + clocksource_gpt.name); + return res; + } else { + pr_info("OMAP clocksource: %s at %lu Hz\n", + clocksource_gpt.name, clksrc.rate); + } + return 0; +} + #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER /* * The realtime counter also called master counter, is a free-running @@ -604,6 +780,65 @@ static OMAP_SYS_32K_TIMER_INIT(4, 1, "timer_32k_ck", "ti,timer-alwon", 2, "sys_clkin_ck", NULL); #endif +void omap_generic_timer_init(void) +{ + struct device_node *np_clksrc = NULL, *np_clkev = NULL; + int res, using_counter = false; + + if (!of_have_populated_dt()) + BUG_ON("Generic timer init should only be used for DT boot\n"); + + if (omap_clk_init) + omap_clk_init(); + + omap_dmtimer_init(); + + /* + * Search for a clocksource: + * Check if dmtimer with property timer-clocksource is in DT, + * If not, by default see if a counter is available unless the + * clocksource=use_gptimer is present on boot command line. + * If no counter on platform, or use_gptimer=1, search for + * a dmtimer with ti,timer-alwon property (timers that don't + * turn off during suspend state). If not found, then fail. + */ + np_clksrc = omap_get_timer_dt(omap_timer_match, "ti,timer-clocksource"); + if (!np_clksrc && !use_gptimer_clksrc) { + np_clksrc = omap_get_timer_dt(omap_counter_match, NULL); + if (np_clksrc) + using_counter = true; + } + + if (!np_clksrc) + np_clksrc = omap_get_timer_dt(omap_timer_match, + "ti,timer-alwon"); + + BUG_ON(!np_clksrc); + + np_clkev = omap_get_timer_dt(omap_timer_match, "ti,timer-clockevent"); + if (!np_clkev) { + np_clkev = omap_get_timer_dt(omap_timer_match, NULL); + BUG_ON(!np_clkev); + } + + res = omap_dmtimer_power_init(&clkev, np_clkev, false); + BUG_ON(res); + + res = omap_clockevent_init(np_clkev); + BUG_ON(res); + + res = omap_dmtimer_power_init(&clksrc, np_clksrc, using_counter); + BUG_ON(res); + + if (using_counter) + res = omap_sync32k_clocksource_init(np_clksrc); + else + res = omap_clocksource_init(np_clksrc); + BUG_ON(res); + + of_node_put(np_clksrc); +} + #ifdef CONFIG_ARCH_OMAP4 void __init omap4_local_timer_init(void) {
This work is a migration effort of OMAP system timers to the clocksource/clockevent framework. Consider this as a first-pass in this effort. There are few cleanups that need to be done first. The HWMOD code is intertwined with the timer code. HWMOD code cleanups in the future will hopefully make most of this code go away, so till then we separate out the power/clocks portion of the code from the actual timer bits. This will facilitate near-future work of adapting the system timer as a clocksource. New functions for OF-only boot are introduced, and we can soon delete the old versions once we migrate all platforms. Currently only AM335x is migrated and testedA new omap_generic_timer_init function is introduced for DT platforms. Code required earlier for non-DT platforms such as setup of timer IDs and timer parent clock is not required. parent clocks are automatically setup by the mux clock driver through DT so they no longer need to be hardcoded. The init code will try to pick the best timer for clocksource and clockevent however bindings are added to force a particular timer as clocksource or clockevent through DT. Signed-off-by: Joel Fernandes <joelf@ti.com> --- .../devicetree/bindings/arm/omap/timer.txt | 12 ++ arch/arm/mach-omap2/common.h | 1 + arch/arm/mach-omap2/timer.c | 235 +++++++++++++++++++++ 3 files changed, 248 insertions(+)