| Message ID | 1431005924-21777-4-git-send-email-lee.jones@linaro.org |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, 2015-05-07 at 14:38 +0100, Lee Jones wrote: > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > +config CLKSRC_ST_LPC_TIMER_SCHED_CLOCK > + bool > + depends on CLKSRC_ST_LPC > + default y > + help > + Use Low Power controller timer clock source as sched_clock This adds, as far as I can see, a Kconfig symbol that will always be in lockstep with CLKSRC_ST_LPC. Ie, CLKSRC_ST_LPC and CLKSRC_ST_LPC_TIMER_SCHED_CLOCK will always both be 'n' or both be 'y'. Are there plans to break that lockstep in the future? Thanks, Paul Bolle
On Fri, 08 May 2015, Paul Bolle wrote: > On Thu, 2015-05-07 at 14:38 +0100, Lee Jones wrote: > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > > +config CLKSRC_ST_LPC_TIMER_SCHED_CLOCK > > + bool > > + depends on CLKSRC_ST_LPC > > + default y > > + help > > + Use Low Power controller timer clock source as sched_clock > > This adds, as far as I can see, a Kconfig symbol that will always be in > lockstep with CLKSRC_ST_LPC. Ie, CLKSRC_ST_LPC and > CLKSRC_ST_LPC_TIMER_SCHED_CLOCK will always both be 'n' or both be 'y'. > > Are there plans to break that lockstep in the future? What's stopping CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK=n?
On Fri, 2015-05-08 at 10:36 +0100, Lee Jones wrote: > On Fri, 08 May 2015, Paul Bolle wrote: > > On Thu, 2015-05-07 at 14:38 +0100, Lee Jones wrote: > > > --- a/drivers/clocksource/Kconfig > > > +++ b/drivers/clocksource/Kconfig > > > > > +config CLKSRC_ST_LPC_TIMER_SCHED_CLOCK > > > + bool > > > + depends on CLKSRC_ST_LPC > > > + default y > > > + help > > > + Use Low Power controller timer clock source as sched_clock > > > > This adds, as far as I can see, a Kconfig symbol that will always be in > > lockstep with CLKSRC_ST_LPC. Ie, CLKSRC_ST_LPC and > > CLKSRC_ST_LPC_TIMER_SCHED_CLOCK will always both be 'n' or both be 'y'. > > > > Are there plans to break that lockstep in the future? > > What's stopping CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK=n? If CLKSRC_ST_LPC=y you mean? The lack of a prompt, I'd say. Ie, you would need bool "A very short description" to be able to set these independently. But now I notice that I missed that there appears to be a problem with CLKSRC_ST_LPC too. I'll reply to 2/12 shortly. Thanks, Paul Bolle
On Fri, 08 May 2015, Paul Bolle wrote: > On Fri, 2015-05-08 at 10:36 +0100, Lee Jones wrote: > > On Fri, 08 May 2015, Paul Bolle wrote: > > > On Thu, 2015-05-07 at 14:38 +0100, Lee Jones wrote: > > > > --- a/drivers/clocksource/Kconfig > > > > +++ b/drivers/clocksource/Kconfig > > > > > > > +config CLKSRC_ST_LPC_TIMER_SCHED_CLOCK > > > > + bool > > > > + depends on CLKSRC_ST_LPC > > > > + default y > > > > + help > > > > + Use Low Power controller timer clock source as sched_clock > > > > > > This adds, as far as I can see, a Kconfig symbol that will always be in > > > lockstep with CLKSRC_ST_LPC. Ie, CLKSRC_ST_LPC and > > > CLKSRC_ST_LPC_TIMER_SCHED_CLOCK will always both be 'n' or both be 'y'. > > > > > > Are there plans to break that lockstep in the future? > > > > What's stopping CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK=n? > > If CLKSRC_ST_LPC=y you mean? The lack of a prompt, I'd say. Ie, you > would need > bool "A very short description" > > to be able to set these independently. > > But now I notice that I missed that there appears to be a problem with > CLKSRC_ST_LPC too. I'll reply to 2/12 shortly. Ah, you mean from menuconfig. I almost forgot what that was. ;) I can add a menu option, no problem.
On Fri, 2015-05-08 at 11:29 +0100, Lee Jones wrote: > Ah, you mean from menuconfig. I almost forgot what that was. ;) > > I can add a menu option, no problem. It's not just menuconfig: $ grep -e CONFIG_ARCH_STI -e CONFIG_CLKSRC_ST_LPC .config CONFIG_ARCH_STI=y CONFIG_CLKSRC_ST_LPC=y CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK=y $ vim .config $ grep -e CONFIG_ARCH_STI -e CONFIG_CLKSRC_ST_LPC .config CONFIG_ARCH_STI=y # CONFIG_CLKSRC_ST_LPC is not set # CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK is not set $ make ARCH=arm silentoldconfig scripts/kconfig/conf --silentoldconfig Kconfig # # configuration written to .config # $ grep -e CONFIG_ARCH_STI -e CONFIG_CLKSRC_ST_LPC .config CONFIG_ARCH_STI=y CONFIG_CLKSRC_ST_LPC=y CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK=y So I think that, if you want to be able to set CLKSRC_ST_LPC_TIMER_SCHED_CLOCK to 'n' even though CLKSRC_ST_LPC is 'y', you need to add a prompt. Or have you found a way around this? Thanks, Paul Bolle
On 05/08/2015 01:47 PM, Paul Bolle wrote: > On Fri, 2015-05-08 at 11:29 +0100, Lee Jones wrote: >> Ah, you mean from menuconfig. I almost forgot what that was. ;) >> >> I can add a menu option, no problem. > > It's not just menuconfig: > $ grep -e CONFIG_ARCH_STI -e CONFIG_CLKSRC_ST_LPC .config > CONFIG_ARCH_STI=y > CONFIG_CLKSRC_ST_LPC=y > CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK=y > $ vim .config > $ grep -e CONFIG_ARCH_STI -e CONFIG_CLKSRC_ST_LPC .config > CONFIG_ARCH_STI=y > # CONFIG_CLKSRC_ST_LPC is not set > # CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK is not set > $ make ARCH=arm silentoldconfig > scripts/kconfig/conf --silentoldconfig Kconfig > # > # configuration written to .config > # > $ grep -e CONFIG_ARCH_STI -e CONFIG_CLKSRC_ST_LPC .config > CONFIG_ARCH_STI=y > CONFIG_CLKSRC_ST_LPC=y > CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK=y > > So I think that, if you want to be able to set > CLKSRC_ST_LPC_TIMER_SCHED_CLOCK to 'n' even though CLKSRC_ST_LPC is 'y', > you need to add a prompt. Or have you found a way around this? The general policy is to have non-prompted options in the clocksource's Kconfig and let the platform's config to select the right components.
On Mon, 2015-05-11 at 10:47 +0200, Daniel Lezcano wrote: > The general policy is to have non-prompted options in the clocksource's > Kconfig and let the platform's config to select the right components. If you want to stick to that policy here there's no reason to add CLKSRC_ST_LPC_TIMER_SCHED_CLOCK in this patch. It currently adds nothing, as it will always be equal to whatever value CLKSRC_ST_LPC will have, right? Paul Bolle
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index ac424cf..218daf8 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -258,4 +258,12 @@ config CLKSRC_ST_LPC help Enable this option to use the Low Power controller timer as clocksource. + +config CLKSRC_ST_LPC_TIMER_SCHED_CLOCK + bool + depends on CLKSRC_ST_LPC + default y + help + Use Low Power controller timer clock source as sched_clock + endmenu diff --git a/drivers/clocksource/clksrc_st_lpc.c b/drivers/clocksource/clksrc_st_lpc.c index 18a7dcd0..3d92b26 100644 --- a/drivers/clocksource/clksrc_st_lpc.c +++ b/drivers/clocksource/clksrc_st_lpc.c @@ -16,6 +16,7 @@ #include <linux/clocksource.h> #include <linux/init.h> #include <linux/of_address.h> +#include <linux/sched_clock.h> #include <linux/slab.h> #include <dt-bindings/mfd/st-lpc.h> @@ -38,6 +39,13 @@ static void st_clksrc_reset(void) writel_relaxed(1, ddata.base + LPC_LPT_START_OFF); } +#ifdef CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK +static u64 notrace st_clksrc_sched_clock_read(void) +{ + return (u64)readl_relaxed(ddata.base + LPC_LPT_LSB_OFF); +} +#endif + static int __init st_clksrc_init(void) { unsigned long rate; @@ -47,6 +55,9 @@ static int __init st_clksrc_init(void) rate = clk_get_rate(ddata.clk); +#ifdef CONFIG_CLKSRC_ST_LPC_TIMER_SCHED_CLOCK + sched_clock_register(st_clksrc_sched_clock_read, 32, rate); +#endif ret = clocksource_mmio_init(ddata.base + LPC_LPT_LSB_OFF, "clksrc-st-lpc", rate, 300, 32, clocksource_mmio_readl_up);
Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/clocksource/Kconfig | 8 ++++++++ drivers/clocksource/clksrc_st_lpc.c | 11 +++++++++++ 2 files changed, 19 insertions(+)