| Message ID | 20130618071756.22598.51046.sendpatchset@w520 (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On 06/18/2013 09:17 AM, Magnus Damm wrote: > From: Magnus Damm <damm@opensource.se> > > Introduce the function tick_device_may_c3stop() that > ignores the C3STOP flag in case CPUIdle is disabled. > > The C3STOP flag tells the system that a clock event > device may be stopped during deep sleep, but if this > will happen or not depends on things like if CPUIdle > is enabled and if a CPUIdle driver is available. > > This patch assumes that if CPUIdle is disabled then > the sleep mode triggering C3STOP will never be entered. > So by ignoring C3STOP when CPUIdle is disabled then it > becomes possible to use high resolution timers with only > per-cpu local timers - regardless if they have the > C3STOP flag set or not. > > Observed on the r8a73a4 SoC that at this point only uses > ARM architected timers for clock event and clock sources. > > Without this patch high resolution timers are run time > disabled on the r8a73a4 SoC - this regardless of CPUIdle > is disabled or not. > > The less short term fix is to add support for more timers > on the r8a73a4 SoC, but until CPUIdle support is enabled > it must be possible to use high resoultion timers without > additional timers. > > I'd like to hear some feedback and also test this on more > systems before merging the code, see the non-SOB below. Do we need a broadcast timer when cpuidle is not compiled in the kernel ? > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> > --- > > An earlier ARM arch timer specific version of this patch was > posted yesterday as: > "[PATCH/RFC] arm: arch_timer: Do not set C3STOP in case CPU_IDLE=n" > > Many thanks to Mark Rutland for his kind feedback. > > kernel/time/tick-broadcast.c | 8 ++++---- > kernel/time/tick-common.c | 2 +- > kernel/time/tick-internal.h | 11 +++++++++++ > 3 files changed, 16 insertions(+), 5 deletions(-) > > --- 0001/kernel/time/tick-broadcast.c > +++ work/kernel/time/tick-broadcast.c 2013-06-18 15:36:21.000000000 +0900 > @@ -71,7 +71,7 @@ int tick_check_broadcast_device(struct c > if ((dev->features & CLOCK_EVT_FEAT_DUMMY) || > (tick_broadcast_device.evtdev && > tick_broadcast_device.evtdev->rating >= dev->rating) || > - (dev->features & CLOCK_EVT_FEAT_C3STOP)) > + tick_device_may_c3stop(dev)) > return 0; > > clockevents_exchange_device(tick_broadcast_device.evtdev, dev); > @@ -146,7 +146,7 @@ int tick_device_uses_broadcast(struct cl > * feature and the cpu is marked in the broadcast mask > * then clear the broadcast bit. > */ > - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) { > + if (!tick_device_may_c3stop(dev)) { > int cpu = smp_processor_id(); > cpumask_clear_cpu(cpu, tick_broadcast_mask); > tick_broadcast_clear_oneshot(cpu); > @@ -270,7 +270,7 @@ static void tick_do_broadcast_on_off(uns > /* > * Is the device not affected by the powerstate ? > */ > - if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP)) > + if (!dev || !tick_device_may_c3stop(dev)) > goto out; > > if (!tick_device_is_functional(dev)) > @@ -568,7 +568,7 @@ void tick_broadcast_oneshot_control(unsi > td = &per_cpu(tick_cpu_device, cpu); > dev = td->evtdev; > > - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) > + if (!tick_device_may_c3stop(dev)) > return; > > bc = tick_broadcast_device.evtdev; > --- 0001/kernel/time/tick-common.c > +++ work/kernel/time/tick-common.c 2013-06-18 15:36:29.000000000 +0900 > @@ -52,7 +52,7 @@ int tick_is_oneshot_available(void) > > if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) > return 0; > - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) > + if (!tick_device_may_c3stop(dev)) > return 1; > return tick_broadcast_oneshot_available(); > } > --- 0001/kernel/time/tick-internal.h > +++ work/kernel/time/tick-internal.h 2013-06-18 15:40:10.000000000 +0900 > @@ -141,6 +141,17 @@ static inline int tick_device_is_functio > return !(dev->features & CLOCK_EVT_FEAT_DUMMY); > } > > +/* > + * Check, if the device has C3STOP behavior and CPU Idle is enabled > + */ > +static inline bool tick_device_may_c3stop(struct clock_event_device *dev) > +{ > + /* The C3 sleep mode can only trigger when CPU Idle is enabled, > + * so if CPU Idle is disabled then the C3STOP flag can be ignored */ > + return (IS_ENABLED(CONFIG_CPU_IDLE) && > + (dev->features & CLOCK_EVT_FEAT_C3STOP)); > +} > + > #endif > > extern void do_timer(unsigned long ticks); > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
On Tue, Jun 18, 2013 at 4:32 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 06/18/2013 09:17 AM, Magnus Damm wrote: >> From: Magnus Damm <damm@opensource.se> >> >> Introduce the function tick_device_may_c3stop() that >> ignores the C3STOP flag in case CPUIdle is disabled. >> >> The C3STOP flag tells the system that a clock event >> device may be stopped during deep sleep, but if this >> will happen or not depends on things like if CPUIdle >> is enabled and if a CPUIdle driver is available. >> >> This patch assumes that if CPUIdle is disabled then >> the sleep mode triggering C3STOP will never be entered. >> So by ignoring C3STOP when CPUIdle is disabled then it >> becomes possible to use high resolution timers with only >> per-cpu local timers - regardless if they have the >> C3STOP flag set or not. >> >> Observed on the r8a73a4 SoC that at this point only uses >> ARM architected timers for clock event and clock sources. >> >> Without this patch high resolution timers are run time >> disabled on the r8a73a4 SoC - this regardless of CPUIdle >> is disabled or not. >> >> The less short term fix is to add support for more timers >> on the r8a73a4 SoC, but until CPUIdle support is enabled >> it must be possible to use high resoultion timers without >> additional timers. >> >> I'd like to hear some feedback and also test this on more >> systems before merging the code, see the non-SOB below. > > Do we need a broadcast timer when cpuidle is not compiled in the kernel ? Yes, if there is no per-cpu timer available. It depends on what the SMP support code for a particular SoC or architecture happen to enable. / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/18/2013 09:39 AM, Magnus Damm wrote: > On Tue, Jun 18, 2013 at 4:32 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> On 06/18/2013 09:17 AM, Magnus Damm wrote: >>> From: Magnus Damm <damm@opensource.se> >>> >>> Introduce the function tick_device_may_c3stop() that >>> ignores the C3STOP flag in case CPUIdle is disabled. >>> >>> The C3STOP flag tells the system that a clock event >>> device may be stopped during deep sleep, but if this >>> will happen or not depends on things like if CPUIdle >>> is enabled and if a CPUIdle driver is available. >>> >>> This patch assumes that if CPUIdle is disabled then >>> the sleep mode triggering C3STOP will never be entered. >>> So by ignoring C3STOP when CPUIdle is disabled then it >>> becomes possible to use high resolution timers with only >>> per-cpu local timers - regardless if they have the >>> C3STOP flag set or not. >>> >>> Observed on the r8a73a4 SoC that at this point only uses >>> ARM architected timers for clock event and clock sources. >>> >>> Without this patch high resolution timers are run time >>> disabled on the r8a73a4 SoC - this regardless of CPUIdle >>> is disabled or not. >>> >>> The less short term fix is to add support for more timers >>> on the r8a73a4 SoC, but until CPUIdle support is enabled >>> it must be possible to use high resoultion timers without >>> additional timers. >>> >>> I'd like to hear some feedback and also test this on more >>> systems before merging the code, see the non-SOB below. >> >> Do we need a broadcast timer when cpuidle is not compiled in the kernel ? > > Yes, if there is no per-cpu timer available. It depends on what the > SMP support code for a particular SoC or architecture happen to > enable. Ok thanks for the information. There is here a multiple occurrence of the information "the timer will stop when power is saved": CLOCK_EVT_FEAT_C3STOP and CPUIDLE_FLAG_TIMER_STOP, so I am wondering if some code simplification couldn't be done before your patch. The function: tick_broadcast_oneshot_control is called from clockevents_notify. This one is called from the cpuidle framework or the back-end cpuidle driver. The caller knows the timer will be stop and this is why it is switching to the broadcast mode. But we have a sanity check in tick_broadcast_oneshot_control function: if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) return; In other words, CPUIDLE_FLAG_TIMER_STOP will tell the framework to call clockevents_notify and the tick broadcast code will re-check the device will effectively go down. IMHO, we can get rid of this check. The same happens for the tick_do_broadcast_on_off function. That reduces the number of C3STOP usage.
On 06/18/2013 09:17 AM, Magnus Damm wrote: > From: Magnus Damm <damm@opensource.se> > > Introduce the function tick_device_may_c3stop() that > ignores the C3STOP flag in case CPUIdle is disabled. > > The C3STOP flag tells the system that a clock event > device may be stopped during deep sleep, but if this > will happen or not depends on things like if CPUIdle > is enabled and if a CPUIdle driver is available. > > This patch assumes that if CPUIdle is disabled then > the sleep mode triggering C3STOP will never be entered. > So by ignoring C3STOP when CPUIdle is disabled then it > becomes possible to use high resolution timers with only > per-cpu local timers - regardless if they have the > C3STOP flag set or not. > > Observed on the r8a73a4 SoC that at this point only uses > ARM architected timers for clock event and clock sources. > > Without this patch high resolution timers are run time > disabled on the r8a73a4 SoC - this regardless of CPUIdle > is disabled or not. > > The less short term fix is to add support for more timers > on the r8a73a4 SoC, but until CPUIdle support is enabled > it must be possible to use high resoultion timers without > additional timers. > > I'd like to hear some feedback and also test this on more > systems before merging the code, see the non-SOB below. > > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> > --- > > An earlier ARM arch timer specific version of this patch was > posted yesterday as: > "[PATCH/RFC] arm: arch_timer: Do not set C3STOP in case CPU_IDLE=n" > > Many thanks to Mark Rutland for his kind feedback. > > kernel/time/tick-broadcast.c | 8 ++++---- > kernel/time/tick-common.c | 2 +- > kernel/time/tick-internal.h | 11 +++++++++++ > 3 files changed, 16 insertions(+), 5 deletions(-) > > --- 0001/kernel/time/tick-broadcast.c > +++ work/kernel/time/tick-broadcast.c 2013-06-18 15:36:21.000000000 +0900 > @@ -71,7 +71,7 @@ int tick_check_broadcast_device(struct c > if ((dev->features & CLOCK_EVT_FEAT_DUMMY) || > (tick_broadcast_device.evtdev && > tick_broadcast_device.evtdev->rating >= dev->rating) || > - (dev->features & CLOCK_EVT_FEAT_C3STOP)) > + tick_device_may_c3stop(dev)) > return 0; > > clockevents_exchange_device(tick_broadcast_device.evtdev, dev); > @@ -146,7 +146,7 @@ int tick_device_uses_broadcast(struct cl > * feature and the cpu is marked in the broadcast mask > * then clear the broadcast bit. > */ > - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) { > + if (!tick_device_may_c3stop(dev)) { > int cpu = smp_processor_id(); > cpumask_clear_cpu(cpu, tick_broadcast_mask); > tick_broadcast_clear_oneshot(cpu); > @@ -270,7 +270,7 @@ static void tick_do_broadcast_on_off(uns > /* > * Is the device not affected by the powerstate ? > */ > - if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP)) > + if (!dev || !tick_device_may_c3stop(dev)) > goto out; > > if (!tick_device_is_functional(dev)) > @@ -568,7 +568,7 @@ void tick_broadcast_oneshot_control(unsi > td = &per_cpu(tick_cpu_device, cpu); > dev = td->evtdev; > > - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) > + if (!tick_device_may_c3stop(dev)) > return; > > bc = tick_broadcast_device.evtdev; > --- 0001/kernel/time/tick-common.c > +++ work/kernel/time/tick-common.c 2013-06-18 15:36:29.000000000 +0900 > @@ -52,7 +52,7 @@ int tick_is_oneshot_available(void) > > if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) > return 0; > - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) > + if (!tick_device_may_c3stop(dev)) > return 1; > return tick_broadcast_oneshot_available(); > } > --- 0001/kernel/time/tick-internal.h > +++ work/kernel/time/tick-internal.h 2013-06-18 15:40:10.000000000 +0900 > @@ -141,6 +141,17 @@ static inline int tick_device_is_functio > return !(dev->features & CLOCK_EVT_FEAT_DUMMY); > } > > +/* > + * Check, if the device has C3STOP behavior and CPU Idle is enabled > + */ > +static inline bool tick_device_may_c3stop(struct clock_event_device *dev) I prefer tick_device_is_reliable(struct clock_event_device *dev). > +{ > + /* The C3 sleep mode can only trigger when CPU Idle is enabled, > + * so if CPU Idle is disabled then the C3STOP flag can be ignored */ > + return (IS_ENABLED(CONFIG_CPU_IDLE) && > + (dev->features & CLOCK_EVT_FEAT_C3STOP)); > +} Preferably you may use the format: #ifdef CONFIG_CPU_IDLE static inline bool tick_device_is_reliable(struct clock_event_device *dev) { return dev->features & CLOCK_EVT_FEAT_C3STOP; } #else static inline bool tick_device_is_reliable(struct clock_event_device *dev) { return true; } #endif to conform the header style format already present in the file. > extern void do_timer(unsigned long ticks); > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
Hi Daniel, On Tue, Jun 18, 2013 at 5:24 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 06/18/2013 09:39 AM, Magnus Damm wrote: >> On Tue, Jun 18, 2013 at 4:32 PM, Daniel Lezcano >> <daniel.lezcano@linaro.org> wrote: >>> On 06/18/2013 09:17 AM, Magnus Damm wrote: >>>> From: Magnus Damm <damm@opensource.se> >>>> >>>> Introduce the function tick_device_may_c3stop() that >>>> ignores the C3STOP flag in case CPUIdle is disabled. >>>> >>>> The C3STOP flag tells the system that a clock event >>>> device may be stopped during deep sleep, but if this >>>> will happen or not depends on things like if CPUIdle >>>> is enabled and if a CPUIdle driver is available. >>>> >>>> This patch assumes that if CPUIdle is disabled then >>>> the sleep mode triggering C3STOP will never be entered. >>>> So by ignoring C3STOP when CPUIdle is disabled then it >>>> becomes possible to use high resolution timers with only >>>> per-cpu local timers - regardless if they have the >>>> C3STOP flag set or not. >>>> >>>> Observed on the r8a73a4 SoC that at this point only uses >>>> ARM architected timers for clock event and clock sources. >>>> >>>> Without this patch high resolution timers are run time >>>> disabled on the r8a73a4 SoC - this regardless of CPUIdle >>>> is disabled or not. >>>> >>>> The less short term fix is to add support for more timers >>>> on the r8a73a4 SoC, but until CPUIdle support is enabled >>>> it must be possible to use high resoultion timers without >>>> additional timers. >>>> >>>> I'd like to hear some feedback and also test this on more >>>> systems before merging the code, see the non-SOB below. >>> >>> Do we need a broadcast timer when cpuidle is not compiled in the kernel ? >> >> Yes, if there is no per-cpu timer available. It depends on what the >> SMP support code for a particular SoC or architecture happen to >> enable. > > Ok thanks for the information. No problem. Thanks for your comments! > There is here a multiple occurrence of the information "the timer will > stop when power is saved": CLOCK_EVT_FEAT_C3STOP and > CPUIDLE_FLAG_TIMER_STOP, so I am wondering if some code simplification > couldn't be done before your patch. I'm sure it's possible to rearrange things in many ways, and the area that you point out indeed seems to have some overlap. Somehow describing which timers that stop during what CPUIdle sleep state would be nice to have. Also, today clock event drivers simply state C3STOP but there may be shallow sleep modes where the timer doesn't have to stop. It all seems a bit coarse grained to me as-is. > The function: > > tick_broadcast_oneshot_control is called from clockevents_notify. This > one is called from the cpuidle framework or the back-end cpuidle driver. > The caller knows the timer will be stop and this is why it is switching > to the broadcast mode. But we have a sanity check in > tick_broadcast_oneshot_control function: > > if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) > return; > > In other words, CPUIDLE_FLAG_TIMER_STOP will tell the framework to call > clockevents_notify and the tick broadcast code will re-check the device > will effectively go down. IMHO, we can get rid of this check. > > The same happens for the tick_do_broadcast_on_off function. > > That reduces the number of C3STOP usage. That may very well be the case. Care to hack up a patch? =) The goal with this patch is simply to make it possible to use high resolution timers if CPUIdle is disabled. Right now the ARM architected timer is sort of optimized for power, so it sets the C3STOP flag to say that on some SoCs during some sleep modes these timers may stop. My point is that this flag doesn't matter as long as CPUIdle is disabled. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Daniel, On Tue, Jun 18, 2013 at 5:30 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 06/18/2013 09:17 AM, Magnus Damm wrote: >> From: Magnus Damm <damm@opensource.se> >> >> Introduce the function tick_device_may_c3stop() that >> ignores the C3STOP flag in case CPUIdle is disabled. >> >> The C3STOP flag tells the system that a clock event >> device may be stopped during deep sleep, but if this >> will happen or not depends on things like if CPUIdle >> is enabled and if a CPUIdle driver is available. >> >> This patch assumes that if CPUIdle is disabled then >> the sleep mode triggering C3STOP will never be entered. >> So by ignoring C3STOP when CPUIdle is disabled then it >> becomes possible to use high resolution timers with only >> per-cpu local timers - regardless if they have the >> C3STOP flag set or not. >> >> Observed on the r8a73a4 SoC that at this point only uses >> ARM architected timers for clock event and clock sources. >> >> Without this patch high resolution timers are run time >> disabled on the r8a73a4 SoC - this regardless of CPUIdle >> is disabled or not. >> >> The less short term fix is to add support for more timers >> on the r8a73a4 SoC, but until CPUIdle support is enabled >> it must be possible to use high resoultion timers without >> additional timers. >> >> I'd like to hear some feedback and also test this on more >> systems before merging the code, see the non-SOB below. >> >> Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> >> --- >> >> An earlier ARM arch timer specific version of this patch was >> posted yesterday as: >> "[PATCH/RFC] arm: arch_timer: Do not set C3STOP in case CPU_IDLE=n" >> >> Many thanks to Mark Rutland for his kind feedback. >> >> kernel/time/tick-broadcast.c | 8 ++++---- >> kernel/time/tick-common.c | 2 +- >> kernel/time/tick-internal.h | 11 +++++++++++ >> 3 files changed, 16 insertions(+), 5 deletions(-) >> >> --- 0001/kernel/time/tick-broadcast.c >> +++ work/kernel/time/tick-broadcast.c 2013-06-18 15:36:21.000000000 +0900 >> @@ -71,7 +71,7 @@ int tick_check_broadcast_device(struct c >> if ((dev->features & CLOCK_EVT_FEAT_DUMMY) || >> (tick_broadcast_device.evtdev && >> tick_broadcast_device.evtdev->rating >= dev->rating) || >> - (dev->features & CLOCK_EVT_FEAT_C3STOP)) >> + tick_device_may_c3stop(dev)) >> return 0; >> >> clockevents_exchange_device(tick_broadcast_device.evtdev, dev); >> @@ -146,7 +146,7 @@ int tick_device_uses_broadcast(struct cl >> * feature and the cpu is marked in the broadcast mask >> * then clear the broadcast bit. >> */ >> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) { >> + if (!tick_device_may_c3stop(dev)) { >> int cpu = smp_processor_id(); >> cpumask_clear_cpu(cpu, tick_broadcast_mask); >> tick_broadcast_clear_oneshot(cpu); >> @@ -270,7 +270,7 @@ static void tick_do_broadcast_on_off(uns >> /* >> * Is the device not affected by the powerstate ? >> */ >> - if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> + if (!dev || !tick_device_may_c3stop(dev)) >> goto out; >> >> if (!tick_device_is_functional(dev)) >> @@ -568,7 +568,7 @@ void tick_broadcast_oneshot_control(unsi >> td = &per_cpu(tick_cpu_device, cpu); >> dev = td->evtdev; >> >> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> + if (!tick_device_may_c3stop(dev)) >> return; >> >> bc = tick_broadcast_device.evtdev; >> --- 0001/kernel/time/tick-common.c >> +++ work/kernel/time/tick-common.c 2013-06-18 15:36:29.000000000 +0900 >> @@ -52,7 +52,7 @@ int tick_is_oneshot_available(void) >> >> if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) >> return 0; >> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> + if (!tick_device_may_c3stop(dev)) >> return 1; >> return tick_broadcast_oneshot_available(); >> } >> --- 0001/kernel/time/tick-internal.h >> +++ work/kernel/time/tick-internal.h 2013-06-18 15:40:10.000000000 +0900 >> @@ -141,6 +141,17 @@ static inline int tick_device_is_functio >> return !(dev->features & CLOCK_EVT_FEAT_DUMMY); >> } >> >> +/* >> + * Check, if the device has C3STOP behavior and CPU Idle is enabled >> + */ >> +static inline bool tick_device_may_c3stop(struct clock_event_device *dev) > > I prefer tick_device_is_reliable(struct clock_event_device *dev). Sure. I took the name from the flag, thought that made it easy to follow. I wonder what the timekeeping maintainers prefer? >> +{ >> + /* The C3 sleep mode can only trigger when CPU Idle is enabled, >> + * so if CPU Idle is disabled then the C3STOP flag can be ignored */ >> + return (IS_ENABLED(CONFIG_CPU_IDLE) && >> + (dev->features & CLOCK_EVT_FEAT_C3STOP)); >> +} > > Preferably you may use the format: > > #ifdef CONFIG_CPU_IDLE > static inline bool tick_device_is_reliable(struct clock_event_device *dev) > { > return dev->features & CLOCK_EVT_FEAT_C3STOP; > } > #else > static inline bool tick_device_is_reliable(struct clock_event_device *dev) > { > return true; > } > #endif > > to conform the header style format already present in the file. I agree with you about following the same style. Actually, I wrote the code to follow the code right above the function, but I decided to return bool instead of int. I don't mind so much in general though, except trying to keep the code at least half well-commented and relatively compact. So regarding stylistic things, sure, we can move around things. Question is just if this is acceptable or not. =) Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/18/2013 10:56 AM, Magnus Damm wrote: > Hi Daniel, > > On Tue, Jun 18, 2013 at 5:30 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> On 06/18/2013 09:17 AM, Magnus Damm wrote: >>> From: Magnus Damm <damm@opensource.se> >>> >>> Introduce the function tick_device_may_c3stop() that >>> ignores the C3STOP flag in case CPUIdle is disabled. >>> >>> The C3STOP flag tells the system that a clock event >>> device may be stopped during deep sleep, but if this >>> will happen or not depends on things like if CPUIdle >>> is enabled and if a CPUIdle driver is available. >>> >>> This patch assumes that if CPUIdle is disabled then >>> the sleep mode triggering C3STOP will never be entered. >>> So by ignoring C3STOP when CPUIdle is disabled then it >>> becomes possible to use high resolution timers with only >>> per-cpu local timers - regardless if they have the >>> C3STOP flag set or not. >>> >>> Observed on the r8a73a4 SoC that at this point only uses >>> ARM architected timers for clock event and clock sources. >>> >>> Without this patch high resolution timers are run time >>> disabled on the r8a73a4 SoC - this regardless of CPUIdle >>> is disabled or not. >>> >>> The less short term fix is to add support for more timers >>> on the r8a73a4 SoC, but until CPUIdle support is enabled >>> it must be possible to use high resoultion timers without >>> additional timers. >>> >>> I'd like to hear some feedback and also test this on more >>> systems before merging the code, see the non-SOB below. >>> >>> Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> >>> --- >>> >>> An earlier ARM arch timer specific version of this patch was >>> posted yesterday as: >>> "[PATCH/RFC] arm: arch_timer: Do not set C3STOP in case CPU_IDLE=n" >>> >>> Many thanks to Mark Rutland for his kind feedback. >>> >>> kernel/time/tick-broadcast.c | 8 ++++---- >>> kernel/time/tick-common.c | 2 +- >>> kernel/time/tick-internal.h | 11 +++++++++++ >>> 3 files changed, 16 insertions(+), 5 deletions(-) >>> >>> --- 0001/kernel/time/tick-broadcast.c >>> +++ work/kernel/time/tick-broadcast.c 2013-06-18 15:36:21.000000000 +0900 >>> @@ -71,7 +71,7 @@ int tick_check_broadcast_device(struct c >>> if ((dev->features & CLOCK_EVT_FEAT_DUMMY) || >>> (tick_broadcast_device.evtdev && >>> tick_broadcast_device.evtdev->rating >= dev->rating) || >>> - (dev->features & CLOCK_EVT_FEAT_C3STOP)) >>> + tick_device_may_c3stop(dev)) >>> return 0; >>> >>> clockevents_exchange_device(tick_broadcast_device.evtdev, dev); >>> @@ -146,7 +146,7 @@ int tick_device_uses_broadcast(struct cl >>> * feature and the cpu is marked in the broadcast mask >>> * then clear the broadcast bit. >>> */ >>> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) { >>> + if (!tick_device_may_c3stop(dev)) { >>> int cpu = smp_processor_id(); >>> cpumask_clear_cpu(cpu, tick_broadcast_mask); >>> tick_broadcast_clear_oneshot(cpu); >>> @@ -270,7 +270,7 @@ static void tick_do_broadcast_on_off(uns >>> /* >>> * Is the device not affected by the powerstate ? >>> */ >>> - if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP)) >>> + if (!dev || !tick_device_may_c3stop(dev)) >>> goto out; >>> >>> if (!tick_device_is_functional(dev)) >>> @@ -568,7 +568,7 @@ void tick_broadcast_oneshot_control(unsi >>> td = &per_cpu(tick_cpu_device, cpu); >>> dev = td->evtdev; >>> >>> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >>> + if (!tick_device_may_c3stop(dev)) >>> return; >>> >>> bc = tick_broadcast_device.evtdev; >>> --- 0001/kernel/time/tick-common.c >>> +++ work/kernel/time/tick-common.c 2013-06-18 15:36:29.000000000 +0900 >>> @@ -52,7 +52,7 @@ int tick_is_oneshot_available(void) >>> >>> if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) >>> return 0; >>> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >>> + if (!tick_device_may_c3stop(dev)) >>> return 1; >>> return tick_broadcast_oneshot_available(); >>> } >>> --- 0001/kernel/time/tick-internal.h >>> +++ work/kernel/time/tick-internal.h 2013-06-18 15:40:10.000000000 +0900 >>> @@ -141,6 +141,17 @@ static inline int tick_device_is_functio >>> return !(dev->features & CLOCK_EVT_FEAT_DUMMY); >>> } >>> >>> +/* >>> + * Check, if the device has C3STOP behavior and CPU Idle is enabled >>> + */ >>> +static inline bool tick_device_may_c3stop(struct clock_event_device *dev) >> >> I prefer tick_device_is_reliable(struct clock_event_device *dev). > > Sure. I took the name from the flag, thought that made it easy to follow. > > I wonder what the timekeeping maintainers prefer? Personally, I would prefer tick_device_is_reliable function instead of c3stop because this one is coming from the C-state Intel semantic. On the other architectures, the C-state does not make sense and, especially for ARM, you can have idle state corresponding to the index #1 where the timer is shutdown. This comment also apply to CLOCK_EVT_FEAT_C3STOP ... but this is diverging from the purpose of your patch. >>> +{ >>> + /* The C3 sleep mode can only trigger when CPU Idle is enabled, >>> + * so if CPU Idle is disabled then the C3STOP flag can be ignored */ >>> + return (IS_ENABLED(CONFIG_CPU_IDLE) && >>> + (dev->features & CLOCK_EVT_FEAT_C3STOP)); >>> +} >> >> Preferably you may use the format: >> >> #ifdef CONFIG_CPU_IDLE >> static inline bool tick_device_is_reliable(struct clock_event_device *dev) >> { >> return dev->features & CLOCK_EVT_FEAT_C3STOP; >> } >> #else >> static inline bool tick_device_is_reliable(struct clock_event_device *dev) >> { >> return true; >> } >> #endif >> >> to conform the header style format already present in the file. > > I agree with you about following the same style. Actually, I wrote > the code to follow the code right above the function, but I decided to > return bool instead of int. I don't mind so much in general though, > except trying to keep the code at least half well-commented and > relatively compact. > > So regarding stylistic things, sure, we can move around things. > Question is just if this is acceptable or not. =)
On 06/18/2013 10:49 AM, Magnus Damm wrote: > Hi Daniel, > > On Tue, Jun 18, 2013 at 5:24 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> On 06/18/2013 09:39 AM, Magnus Damm wrote: >>> On Tue, Jun 18, 2013 at 4:32 PM, Daniel Lezcano >>> <daniel.lezcano@linaro.org> wrote: >>>> On 06/18/2013 09:17 AM, Magnus Damm wrote: >>>>> From: Magnus Damm <damm@opensource.se> >>>>> >>>>> Introduce the function tick_device_may_c3stop() that >>>>> ignores the C3STOP flag in case CPUIdle is disabled. >>>>> >>>>> The C3STOP flag tells the system that a clock event >>>>> device may be stopped during deep sleep, but if this >>>>> will happen or not depends on things like if CPUIdle >>>>> is enabled and if a CPUIdle driver is available. >>>>> >>>>> This patch assumes that if CPUIdle is disabled then >>>>> the sleep mode triggering C3STOP will never be entered. >>>>> So by ignoring C3STOP when CPUIdle is disabled then it >>>>> becomes possible to use high resolution timers with only >>>>> per-cpu local timers - regardless if they have the >>>>> C3STOP flag set or not. >>>>> >>>>> Observed on the r8a73a4 SoC that at this point only uses >>>>> ARM architected timers for clock event and clock sources. >>>>> >>>>> Without this patch high resolution timers are run time >>>>> disabled on the r8a73a4 SoC - this regardless of CPUIdle >>>>> is disabled or not. >>>>> >>>>> The less short term fix is to add support for more timers >>>>> on the r8a73a4 SoC, but until CPUIdle support is enabled >>>>> it must be possible to use high resoultion timers without >>>>> additional timers. >>>>> >>>>> I'd like to hear some feedback and also test this on more >>>>> systems before merging the code, see the non-SOB below. >>>> >>>> Do we need a broadcast timer when cpuidle is not compiled in the kernel ? >>> >>> Yes, if there is no per-cpu timer available. It depends on what the >>> SMP support code for a particular SoC or architecture happen to >>> enable. >> >> Ok thanks for the information. > > No problem. Thanks for your comments! > >> There is here a multiple occurrence of the information "the timer will >> stop when power is saved": CLOCK_EVT_FEAT_C3STOP and >> CPUIDLE_FLAG_TIMER_STOP, so I am wondering if some code simplification >> couldn't be done before your patch. > > I'm sure it's possible to rearrange things in many ways, and the area > that you point out indeed seems to have some overlap. Somehow > describing which timers that stop during what CPUIdle sleep state > would be nice to have. Also, today clock event drivers simply state > C3STOP but there may be shallow sleep modes where the timer doesn't > have to stop. It all seems a bit coarse grained to me as-is. > >> The function: >> >> tick_broadcast_oneshot_control is called from clockevents_notify. This >> one is called from the cpuidle framework or the back-end cpuidle driver. >> The caller knows the timer will be stop and this is why it is switching >> to the broadcast mode. But we have a sanity check in >> tick_broadcast_oneshot_control function: >> >> if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> return; >> >> In other words, CPUIDLE_FLAG_TIMER_STOP will tell the framework to call >> clockevents_notify and the tick broadcast code will re-check the device >> will effectively go down. IMHO, we can get rid of this check. >> >> The same happens for the tick_do_broadcast_on_off function. >> >> That reduces the number of C3STOP usage. > > That may very well be the case. Care to hack up a patch? =) No problem, I will write one as soon as I can. > The goal with this patch is simply to make it possible to use high > resolution timers if CPUIdle is disabled. Right now the ARM > architected timer is sort of optimized for power, so it sets the > C3STOP flag to say that on some SoCs during some sleep modes these > timers may stop. My point is that this flag doesn't matter as long as > CPUIdle is disabled. Yes, I understand. That makes sense. Thanks -- Daniel
--- 0001/kernel/time/tick-broadcast.c +++ work/kernel/time/tick-broadcast.c 2013-06-18 15:36:21.000000000 +0900 @@ -71,7 +71,7 @@ int tick_check_broadcast_device(struct c if ((dev->features & CLOCK_EVT_FEAT_DUMMY) || (tick_broadcast_device.evtdev && tick_broadcast_device.evtdev->rating >= dev->rating) || - (dev->features & CLOCK_EVT_FEAT_C3STOP)) + tick_device_may_c3stop(dev)) return 0; clockevents_exchange_device(tick_broadcast_device.evtdev, dev); @@ -146,7 +146,7 @@ int tick_device_uses_broadcast(struct cl * feature and the cpu is marked in the broadcast mask * then clear the broadcast bit. */ - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) { + if (!tick_device_may_c3stop(dev)) { int cpu = smp_processor_id(); cpumask_clear_cpu(cpu, tick_broadcast_mask); tick_broadcast_clear_oneshot(cpu); @@ -270,7 +270,7 @@ static void tick_do_broadcast_on_off(uns /* * Is the device not affected by the powerstate ? */ - if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP)) + if (!dev || !tick_device_may_c3stop(dev)) goto out; if (!tick_device_is_functional(dev)) @@ -568,7 +568,7 @@ void tick_broadcast_oneshot_control(unsi td = &per_cpu(tick_cpu_device, cpu); dev = td->evtdev; - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) + if (!tick_device_may_c3stop(dev)) return; bc = tick_broadcast_device.evtdev; --- 0001/kernel/time/tick-common.c +++ work/kernel/time/tick-common.c 2013-06-18 15:36:29.000000000 +0900 @@ -52,7 +52,7 @@ int tick_is_oneshot_available(void) if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) return 0; - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) + if (!tick_device_may_c3stop(dev)) return 1; return tick_broadcast_oneshot_available(); } --- 0001/kernel/time/tick-internal.h +++ work/kernel/time/tick-internal.h 2013-06-18 15:40:10.000000000 +0900 @@ -141,6 +141,17 @@ static inline int tick_device_is_functio return !(dev->features & CLOCK_EVT_FEAT_DUMMY); } +/* + * Check, if the device has C3STOP behavior and CPU Idle is enabled + */ +static inline bool tick_device_may_c3stop(struct clock_event_device *dev) +{ + /* The C3 sleep mode can only trigger when CPU Idle is enabled, + * so if CPU Idle is disabled then the C3STOP flag can be ignored */ + return (IS_ENABLED(CONFIG_CPU_IDLE) && + (dev->features & CLOCK_EVT_FEAT_C3STOP)); +} + #endif extern void do_timer(unsigned long ticks);
From: Magnus Damm <damm@opensource.se> Introduce the function tick_device_may_c3stop() that ignores the C3STOP flag in case CPUIdle is disabled. The C3STOP flag tells the system that a clock event device may be stopped during deep sleep, but if this will happen or not depends on things like if CPUIdle is enabled and if a CPUIdle driver is available. This patch assumes that if CPUIdle is disabled then the sleep mode triggering C3STOP will never be entered. So by ignoring C3STOP when CPUIdle is disabled then it becomes possible to use high resolution timers with only per-cpu local timers - regardless if they have the C3STOP flag set or not. Observed on the r8a73a4 SoC that at this point only uses ARM architected timers for clock event and clock sources. Without this patch high resolution timers are run time disabled on the r8a73a4 SoC - this regardless of CPUIdle is disabled or not. The less short term fix is to add support for more timers on the r8a73a4 SoC, but until CPUIdle support is enabled it must be possible to use high resoultion timers without additional timers. I'd like to hear some feedback and also test this on more systems before merging the code, see the non-SOB below. Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> --- An earlier ARM arch timer specific version of this patch was posted yesterday as: "[PATCH/RFC] arm: arch_timer: Do not set C3STOP in case CPU_IDLE=n" Many thanks to Mark Rutland for his kind feedback. kernel/time/tick-broadcast.c | 8 ++++---- kernel/time/tick-common.c | 2 +- kernel/time/tick-internal.h | 11 +++++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html