| Message ID | 1448040129-23869-1-git-send-email-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > Since system clocksource is finally selected by Clocksource core at > fs_initcall stage during boot there are no reasons to initialize > ti_32k_timer at early boot stages. Hence, ti_32k_timer can be > converted to use platform device/driver model and its PM can be > implemented using PM runtime which is common for OMAP devices. > > Platform specific initialization code has to be disabled once as > ti_32k_timer is converted to platform device - otherwise OMAP platform > code will generate boot warnings. > > After this change, all counter_32k's platform code can be removed > once all OMAP boards will be converted to DT. > > Cc: Tony Lindgren <tony@atomide.com> > Cc: Felipe Balbi <balbi@ti.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > arch/arm/mach-omap2/timer.c | 16 +++-------- > drivers/clocksource/timer-ti-32k.c | 58 ++++++++++++++++++++++++++++++++------ > 2 files changed, 53 insertions(+), 21 deletions(-) > > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c > index b18ebbe..3bfde44 100644 > --- a/arch/arm/mach-omap2/timer.c > +++ b/arch/arm/mach-omap2/timer.c > @@ -393,23 +393,15 @@ static const struct of_device_id omap_counter_match[] __initconst = { > static int __init __maybe_unused omap2_sync32k_clocksource_init(void) > { > int ret; > - struct device_node *np = NULL; > struct omap_hwmod *oh; > const char *oh_name = "counter_32k"; > > /* > - * If device-tree is present, then search the DT blob > - * to see if the 32kHz counter is supported. > + * If device-tree is present, then just exit - > + * 32kHz clocksource driver will handle it. > */ > - if (of_have_populated_dt()) { > - np = omap_get_timer_dt(omap_counter_match, NULL); > - if (!np) > - return -ENODEV; > - > - of_property_read_string_index(np, "ti,hwmods", 0, &oh_name); > - if (!oh_name) > - return -ENODEV; > - } > + if (of_have_populated_dt()) > + return 0; > > /* > * First check hwmod data is available for sync32k counter > diff --git a/drivers/clocksource/timer-ti-32k.c b/drivers/clocksource/timer-ti-32k.c > index 8518d9d..e71496f 100644 > --- a/drivers/clocksource/timer-ti-32k.c > +++ b/drivers/clocksource/timer-ti-32k.c > @@ -39,8 +39,11 @@ > #include <linux/time.h> > #include <linux/sched_clock.h> > #include <linux/clocksource.h> > +#include <linux/module.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > > /* > * 32KHz clocksource ... always available, on pretty most chips except > @@ -88,15 +91,28 @@ static u64 notrace omap_32k_read_sched_clock(void) > return ti_32k_read_cycles(&ti_32k_timer.cs); > } > > -static void __init ti_32k_timer_init(struct device_node *np) > +static const struct of_device_id ti_32k_of_table[] = { > + { .compatible = "ti,omap-counter32k" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ti_32k_of_table); > + > +static int __init ti_32k_probe(struct platform_device *pdev) > { > + struct device *dev = &pdev->dev; > + struct resource *res; > int ret; > > - ti_32k_timer.base = of_iomap(np, 0); > - if (!ti_32k_timer.base) { > - pr_err("Can't ioremap 32k timer base\n"); > - return; > - } > + /* Static mapping, never released */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + ti_32k_timer.base = devm_ioremap_resource(dev, res); > + if (IS_ERR(ti_32k_timer.base)) > + return PTR_ERR(ti_32k_timer.base); > + > + pm_runtime_enable(dev); > + ret = pm_runtime_get_sync(dev); > + if (ret < 0) > + goto probe_err; > > ti_32k_timer.counter = ti_32k_timer.base; > > @@ -116,11 +132,35 @@ static void __init ti_32k_timer_init(struct device_node *np) > ret = clocksource_register_hz(&ti_32k_timer.cs, 32768); > if (ret) { > pr_err("32k_counter: can't register clocksource\n"); > - return; > + goto probe_err; > } > > sched_clock_register(omap_32k_read_sched_clock, 32, 32768); > pr_info("OMAP clocksource: 32k_counter at 32768 Hz\n"); > + return 0; > + > +probe_err: > + pm_runtime_put_noidle(dev); > + return ret; > +}; > + > +static struct platform_driver ti_32k_driver __initdata = { > + .probe = ti_32k_probe, > + .driver = { > + .name = "ti_32k_timer", > + .of_match_table = of_match_ptr(ti_32k_of_table), > + } > +}; > + > +static int __init ti_32k_init(void) > +{ > + return platform_driver_register(&ti_32k_driver); > } > -CLOCKSOURCE_OF_DECLARE(ti_32k_timer, "ti,omap-counter32k", > - ti_32k_timer_init); > + > +subsys_initcall(ti_32k_init); > + > +MODULE_AUTHOR("Paul Mundt"); > +MODULE_AUTHOR("Juha Yrjölä"); > +MODULE_DESCRIPTION("OMAP2 32k Timer"); > +MODULE_ALIAS("platform:ti_32k_timer"); > +MODULE_LICENSE("GPL v2"); this will break clksource_of_init(), right ? Eventually, we want that to be the only thing called by our .init_time method. I'll leave it to Tony to decide, but IMO this is not a good path forward for timers.
Hi Felipe, On 11/20/2015 08:21 PM, Felipe Balbi wrote: > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> Since system clocksource is finally selected by Clocksource core at >> fs_initcall stage during boot there are no reasons to initialize >> ti_32k_timer at early boot stages. Hence, ti_32k_timer can be >> converted to use platform device/driver model and its PM can be >> implemented using PM runtime which is common for OMAP devices. >> >> Platform specific initialization code has to be disabled once as >> ti_32k_timer is converted to platform device - otherwise OMAP platform >> code will generate boot warnings. >> >> After this change, all counter_32k's platform code can be removed >> once all OMAP boards will be converted to DT. >> >> Cc: Tony Lindgren <tony@atomide.com> >> Cc: Felipe Balbi <balbi@ti.com> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- [...] >> + >> +static struct platform_driver ti_32k_driver __initdata = { >> + .probe = ti_32k_probe, >> + .driver = { >> + .name = "ti_32k_timer", >> + .of_match_table = of_match_ptr(ti_32k_of_table), >> + } >> +}; >> + >> +static int __init ti_32k_init(void) >> +{ >> + return platform_driver_register(&ti_32k_driver); >> } >> -CLOCKSOURCE_OF_DECLARE(ti_32k_timer, "ti,omap-counter32k", >> - ti_32k_timer_init); >> + >> +subsys_initcall(ti_32k_init); >> + >> +MODULE_AUTHOR("Paul Mundt"); >> +MODULE_AUTHOR("Juha Yrjölä"); >> +MODULE_DESCRIPTION("OMAP2 32k Timer"); >> +MODULE_ALIAS("platform:ti_32k_timer"); >> +MODULE_LICENSE("GPL v2"); > > this will break clksource_of_init(), right ? Eventually, we want that to > be the only thing called by our .init_time method. I'll leave it to Tony > to decide, but IMO this is not a good path forward for timers. > Yeh :(. I did additional tests, and, unfortunately, this can't be used as is. But not because of clocksource_of_init() which will just produce boot warning. It can't be done because of sched_clock_register() which is expected to be called during early boot time only and with disabled IRQs. It was so tempting to try :) Thanks for your comment.
* Grygorii Strashko <grygorii.strashko@ti.com> [151127 12:11]: > Hi Felipe, > > On 11/20/2015 08:21 PM, Felipe Balbi wrote: > > Grygorii Strashko <grygorii.strashko@ti.com> writes: > >> Since system clocksource is finally selected by Clocksource core at > >> fs_initcall stage during boot there are no reasons to initialize > >> ti_32k_timer at early boot stages. Hence, ti_32k_timer can be > >> converted to use platform device/driver model and its PM can be > >> implemented using PM runtime which is common for OMAP devices. > >> > >> Platform specific initialization code has to be disabled once as > >> ti_32k_timer is converted to platform device - otherwise OMAP platform > >> code will generate boot warnings. > >> > >> After this change, all counter_32k's platform code can be removed > >> once all OMAP boards will be converted to DT. > >> > >> Cc: Tony Lindgren <tony@atomide.com> > >> Cc: Felipe Balbi <balbi@ti.com> > >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > >> --- > > [...] > > >> + > >> +static struct platform_driver ti_32k_driver __initdata = { > >> + .probe = ti_32k_probe, > >> + .driver = { > >> + .name = "ti_32k_timer", > >> + .of_match_table = of_match_ptr(ti_32k_of_table), > >> + } > >> +}; > >> + > >> +static int __init ti_32k_init(void) > >> +{ > >> + return platform_driver_register(&ti_32k_driver); > >> } > >> -CLOCKSOURCE_OF_DECLARE(ti_32k_timer, "ti,omap-counter32k", > >> - ti_32k_timer_init); > >> + > >> +subsys_initcall(ti_32k_init); > >> + > >> +MODULE_AUTHOR("Paul Mundt"); > >> +MODULE_AUTHOR("Juha Yrjölä"); > >> +MODULE_DESCRIPTION("OMAP2 32k Timer"); > >> +MODULE_ALIAS("platform:ti_32k_timer"); > >> +MODULE_LICENSE("GPL v2"); > > > > this will break clksource_of_init(), right ? Eventually, we want that to > > be the only thing called by our .init_time method. I'll leave it to Tony > > to decide, but IMO this is not a good path forward for timers. > > > > Yeh :(. I did additional tests, and, unfortunately, this can't be used as is. > But not because of clocksource_of_init() which will just produce boot warning. > It can't be done because of sched_clock_register() which is expected to be > called during early boot time only and with disabled IRQs. > > It was so tempting to try :) We should be able to make this into an early_platform_device and just have it depend on the source clock muxes. See the omap initcall changes patches I just posted. Regards, Tony
Hi Tony, On 11/30/2015 06:28 PM, Tony Lindgren wrote: > * Grygorii Strashko <grygorii.strashko@ti.com> [151127 12:11]: >> On 11/20/2015 08:21 PM, Felipe Balbi wrote: >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> Since system clocksource is finally selected by Clocksource core at >>>> fs_initcall stage during boot there are no reasons to initialize >>>> ti_32k_timer at early boot stages. Hence, ti_32k_timer can be >>>> converted to use platform device/driver model and its PM can be >>>> implemented using PM runtime which is common for OMAP devices. >>>> >>>> Platform specific initialization code has to be disabled once as >>>> ti_32k_timer is converted to platform device - otherwise OMAP platform >>>> code will generate boot warnings. >>>> >>>> After this change, all counter_32k's platform code can be removed >>>> once all OMAP boards will be converted to DT. >>>> >>>> Cc: Tony Lindgren <tony@atomide.com> >>>> Cc: Felipe Balbi <balbi@ti.com> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >> >> [...] >> >>>> + >>>> +static struct platform_driver ti_32k_driver __initdata = { >>>> + .probe = ti_32k_probe, >>>> + .driver = { >>>> + .name = "ti_32k_timer", >>>> + .of_match_table = of_match_ptr(ti_32k_of_table), >>>> + } >>>> +}; >>>> + >>>> +static int __init ti_32k_init(void) >>>> +{ >>>> + return platform_driver_register(&ti_32k_driver); >>>> } >>>> -CLOCKSOURCE_OF_DECLARE(ti_32k_timer, "ti,omap-counter32k", >>>> - ti_32k_timer_init); >>>> + >>>> +subsys_initcall(ti_32k_init); >>>> + >>>> +MODULE_AUTHOR("Paul Mundt"); >>>> +MODULE_AUTHOR("Juha Yrjölä"); >>>> +MODULE_DESCRIPTION("OMAP2 32k Timer"); >>>> +MODULE_ALIAS("platform:ti_32k_timer"); >>>> +MODULE_LICENSE("GPL v2"); >>> >>> this will break clksource_of_init(), right ? Eventually, we want that to >>> be the only thing called by our .init_time method. I'll leave it to Tony >>> to decide, but IMO this is not a good path forward for timers. >>> >> >> Yeh :(. I did additional tests, and, unfortunately, this can't be used as is. >> But not because of clocksource_of_init() which will just produce boot warning. >> It can't be done because of sched_clock_register() which is expected to be >> called during early boot time only and with disabled IRQs. >> >> It was so tempting to try :) > > We should be able to make this into an early_platform_device and just > have it depend on the source clock muxes. See the omap initcall changes > patches I just posted. > Sry, may be I've missed smth, but how early_platform_device will help us to get rid of platform code - We'd still need to power on manually early_platform_device's from platform code :( through hwmod. The main reason why I've tried this is because clocksource will be really selected only at fs_initcall time - and at that time we have no restriction for using platform devices, Pm runtime APIs, etc. (exception/blocker is sched_clock :().
* Grygorii Strashko <grygorii.strashko@ti.com> [151201 07:09]: > On 11/30/2015 06:28 PM, Tony Lindgren wrote: > > > > We should be able to make this into an early_platform_device and just > > have it depend on the source clock muxes. See the omap initcall changes > > patches I just posted. > > > > Sry, may be I've missed smth, but how early_platform_device will help us > to get rid of platform code - We'd still need to power on manually > early_platform_device's from platform code :( through hwmod. Having minimal platform code early is not a problem. The problem is that our early code is not minimal. For the system timers, we should only initialize the mux clocks needed early to select between 32k and hf oscillator source. This needs to be done using the clock framework, but we don't need the other clocks initialized early. The system timers we're using should be in the alwon power domain, if they are not, then we should change the timers around so we're using only timers in the alwon domain for system timers. Typically at least gpt1 and 12 are always powered. That allows us to leave out the hwmod dependency for system timers. Or am I forgetting some other dependency with our system timers? > The main reason why I've tried this is because clocksource will be really selected > only at fs_initcall time - and at that time we have no restriction for using platform > devices, Pm runtime APIs, etc. (exception/blocker is sched_clock :(). Right. But it seems we can leave out quite a bit of the dependencies for system timers. We already have gptimer probe not touching the system timers later on and can use shared gptimer functions after the clock muxing is done. Regards, Tony
On 12/01/2015 06:07 PM, Tony Lindgren wrote: > * Grygorii Strashko <grygorii.strashko@ti.com> [151201 07:09]: >> On 11/30/2015 06:28 PM, Tony Lindgren wrote: >>> >>> We should be able to make this into an early_platform_device and just >>> have it depend on the source clock muxes. See the omap initcall changes >>> patches I just posted. >>> >> >> Sry, may be I've missed smth, but how early_platform_device will help us >> to get rid of platform code - We'd still need to power on manually >> early_platform_device's from platform code :( through hwmod. > > Having minimal platform code early is not a problem. The problem is that > our early code is not minimal. > > For the system timers, we should only initialize the mux clocks needed > early to select between 32k and hf oscillator source. This needs to be done > using the clock framework, but we don't need the other clocks initialized > early. > > The system timers we're using should be in the alwon power domain, if they > are not, then we should change the timers around so we're using only timers > in the alwon domain for system timers. Typically at least gpt1 and 12 are > always powered. That allows us to leave out the hwmod dependency for system > timers. > > Or am I forgetting some other dependency with our system timers? both counter32 and GP timer have to be enabled through sysc registers. They are in "Force idle" state after reset. > >> The main reason why I've tried this is because clocksource will be really selected >> only at fs_initcall time - and at that time we have no restriction for using platform >> devices, Pm runtime APIs, etc. (exception/blocker is sched_clock :(). > > Right. But it seems we can leave out quite a bit of the dependencies > for system timers. We already have gptimer probe not touching the system > timers later on and can use shared gptimer functions after the clock > muxing is done.
* Grygorii Strashko <grygorii.strashko@ti.com> [151201 09:13]: > On 12/01/2015 06:07 PM, Tony Lindgren wrote: > > > >Or am I forgetting some other dependency with our system timers? > > both counter32 and GP timer have to be enabled through sysc registers. > They are in "Force idle" state after reset. That's fine, those are within the timer address space. I'd rather leave out the hwmod dependency from the system timers at the cost of a system timer specific minimal init function. Regards, Tony
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index b18ebbe..3bfde44 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -393,23 +393,15 @@ static const struct of_device_id omap_counter_match[] __initconst = { static int __init __maybe_unused omap2_sync32k_clocksource_init(void) { int ret; - struct device_node *np = NULL; struct omap_hwmod *oh; const char *oh_name = "counter_32k"; /* - * If device-tree is present, then search the DT blob - * to see if the 32kHz counter is supported. + * If device-tree is present, then just exit - + * 32kHz clocksource driver will handle it. */ - if (of_have_populated_dt()) { - np = omap_get_timer_dt(omap_counter_match, NULL); - if (!np) - return -ENODEV; - - of_property_read_string_index(np, "ti,hwmods", 0, &oh_name); - if (!oh_name) - return -ENODEV; - } + if (of_have_populated_dt()) + return 0; /* * First check hwmod data is available for sync32k counter diff --git a/drivers/clocksource/timer-ti-32k.c b/drivers/clocksource/timer-ti-32k.c index 8518d9d..e71496f 100644 --- a/drivers/clocksource/timer-ti-32k.c +++ b/drivers/clocksource/timer-ti-32k.c @@ -39,8 +39,11 @@ #include <linux/time.h> #include <linux/sched_clock.h> #include <linux/clocksource.h> +#include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> /* * 32KHz clocksource ... always available, on pretty most chips except @@ -88,15 +91,28 @@ static u64 notrace omap_32k_read_sched_clock(void) return ti_32k_read_cycles(&ti_32k_timer.cs); } -static void __init ti_32k_timer_init(struct device_node *np) +static const struct of_device_id ti_32k_of_table[] = { + { .compatible = "ti,omap-counter32k" }, + { } +}; +MODULE_DEVICE_TABLE(of, ti_32k_of_table); + +static int __init ti_32k_probe(struct platform_device *pdev) { + struct device *dev = &pdev->dev; + struct resource *res; int ret; - ti_32k_timer.base = of_iomap(np, 0); - if (!ti_32k_timer.base) { - pr_err("Can't ioremap 32k timer base\n"); - return; - } + /* Static mapping, never released */ + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ti_32k_timer.base = devm_ioremap_resource(dev, res); + if (IS_ERR(ti_32k_timer.base)) + return PTR_ERR(ti_32k_timer.base); + + pm_runtime_enable(dev); + ret = pm_runtime_get_sync(dev); + if (ret < 0) + goto probe_err; ti_32k_timer.counter = ti_32k_timer.base; @@ -116,11 +132,35 @@ static void __init ti_32k_timer_init(struct device_node *np) ret = clocksource_register_hz(&ti_32k_timer.cs, 32768); if (ret) { pr_err("32k_counter: can't register clocksource\n"); - return; + goto probe_err; } sched_clock_register(omap_32k_read_sched_clock, 32, 32768); pr_info("OMAP clocksource: 32k_counter at 32768 Hz\n"); + return 0; + +probe_err: + pm_runtime_put_noidle(dev); + return ret; +}; + +static struct platform_driver ti_32k_driver __initdata = { + .probe = ti_32k_probe, + .driver = { + .name = "ti_32k_timer", + .of_match_table = of_match_ptr(ti_32k_of_table), + } +}; + +static int __init ti_32k_init(void) +{ + return platform_driver_register(&ti_32k_driver); } -CLOCKSOURCE_OF_DECLARE(ti_32k_timer, "ti,omap-counter32k", - ti_32k_timer_init); + +subsys_initcall(ti_32k_init); + +MODULE_AUTHOR("Paul Mundt"); +MODULE_AUTHOR("Juha Yrjölä"); +MODULE_DESCRIPTION("OMAP2 32k Timer"); +MODULE_ALIAS("platform:ti_32k_timer"); +MODULE_LICENSE("GPL v2");
Since system clocksource is finally selected by Clocksource core at fs_initcall stage during boot there are no reasons to initialize ti_32k_timer at early boot stages. Hence, ti_32k_timer can be converted to use platform device/driver model and its PM can be implemented using PM runtime which is common for OMAP devices. Platform specific initialization code has to be disabled once as ti_32k_timer is converted to platform device - otherwise OMAP platform code will generate boot warnings. After this change, all counter_32k's platform code can be removed once all OMAP boards will be converted to DT. Cc: Tony Lindgren <tony@atomide.com> Cc: Felipe Balbi <balbi@ti.com> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- arch/arm/mach-omap2/timer.c | 16 +++-------- drivers/clocksource/timer-ti-32k.c | 58 ++++++++++++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 21 deletions(-)