Message ID | 20130617002056.7646.54131.sendpatchset@w520 (mailing list archive) |
---|---|
State | RFC |
Headers | show |
On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote: > From: Magnus Damm <damm@opensource.se> > > Modify the ARM architected timer driver to not set C3STOP > in case CPU_IDLE is disabled. This is a short term fix that > allows use of high resolution timers even though no additional > clock event is registered. > > Not-really-Signed-off-by: Magnus Damm <damm@opensource.se> > --- > > If someone cares about this case then perhaps it should be > moved up to the clock event main code. The same issue should > in theory trigger on all architectures, perhaps x86 people > hunting for low latency may try to disable CPU_IDLE? > > I propose carrying this patch locally to enable high resolution > timers until CPU_IDLE and an additional clock event is supported. > > Observed on r8a73a4 and APE6EVM. Hi Magnus, Is this patch intended to be picked up by me for the LTSI-3.4.25 based backports that live in my renesas-backports tree? If so, could you clearly state this (below the '---' is fine) and include a proper Sob line to indicate that it is fit to be merged even if that merge is not into mainline. Thanks > > drivers/clocksource/arm_arch_timer.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > --- 0001/drivers/clocksource/arm_arch_timer.c > +++ work/drivers/clocksource/arm_arch_timer.c 2013-06-17 09:03:44.000000000 +0900 > @@ -125,7 +125,23 @@ static int arch_timer_set_next_event_phy > > static int __cpuinit arch_timer_setup(struct clock_event_device *clk) > { > - clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP; > + clk->features = CLOCK_EVT_FEAT_ONESHOT; > +#ifdef CONFIG_CPU_IDLE > + /* By not setting the C3STOP flag it is possible to let the > + * ARM architected timer to be the only clock event installed > + * on the system and have working high resolution timers. > + * > + * If the C3STOP flag is set unconditionally then the kernel > + * will always prevent using the high resoultion timer feature > + * unless an additional clock event is registered. > + * > + * In the case where CPU_IDLE is enabled then there is a chance > + * that deeper sleep states will be handled by software, but > + * if CPU_IDLE is disabled then deep sleep states cannot be > + * entered and the feature flagged by C3STOP is not needed. > + */ > + clk->features |= CLOCK_EVT_FEAT_C3STOP; > +#endif > clk->name = "arch_sys_timer"; > clk->rating = 450; > if (arch_timer_use_virtual) { > -- 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 Simon, On Mon, Jun 17, 2013 at 11:13 AM, Simon Horman <horms@verge.net.au> wrote: > On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote: >> From: Magnus Damm <damm@opensource.se> >> >> Modify the ARM architected timer driver to not set C3STOP >> in case CPU_IDLE is disabled. This is a short term fix that >> allows use of high resolution timers even though no additional >> clock event is registered. >> >> Not-really-Signed-off-by: Magnus Damm <damm@opensource.se> >> --- >> >> If someone cares about this case then perhaps it should be >> moved up to the clock event main code. The same issue should >> in theory trigger on all architectures, perhaps x86 people >> hunting for low latency may try to disable CPU_IDLE? >> >> I propose carrying this patch locally to enable high resolution >> timers until CPU_IDLE and an additional clock event is supported. >> >> Observed on r8a73a4 and APE6EVM. > > Hi Magnus, > > Is this patch intended to be picked up by me for the LTSI-3.4.25 based > backports that live in my renesas-backports tree? Yes, correct. The patch was mainly written to satisfy a feature request for your backports, but I noticed that the same issue exists in upstream as well. Ideally I'd like to use the same code for the backport and upstream, but I am not sure if anyone in upstream really cares. The more long term solution is obviously to install a second clock event, perhaps that's good enough. > If so, could you clearly state this (below the '---' is fine) and > include a proper Sob line to indicate that it is fit to be merged > even if that merge is not into mainline. Sure, but I'd like to hear opinions from other people before resending. I will follow your recommendation in next version. 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 Mon, Jun 17, 2013 at 11:47:11AM +0900, Magnus Damm wrote: > Hi Simon, > > On Mon, Jun 17, 2013 at 11:13 AM, Simon Horman <horms@verge.net.au> wrote: > > On Mon, Jun 17, 2013 at 09:20:56AM +0900, Magnus Damm wrote: > >> From: Magnus Damm <damm@opensource.se> > >> > >> Modify the ARM architected timer driver to not set C3STOP > >> in case CPU_IDLE is disabled. This is a short term fix that > >> allows use of high resolution timers even though no additional > >> clock event is registered. > >> > >> Not-really-Signed-off-by: Magnus Damm <damm@opensource.se> > >> --- > >> > >> If someone cares about this case then perhaps it should be > >> moved up to the clock event main code. The same issue should > >> in theory trigger on all architectures, perhaps x86 people > >> hunting for low latency may try to disable CPU_IDLE? > >> > >> I propose carrying this patch locally to enable high resolution > >> timers until CPU_IDLE and an additional clock event is supported. > >> > >> Observed on r8a73a4 and APE6EVM. > > > > Hi Magnus, > > > > Is this patch intended to be picked up by me for the LTSI-3.4.25 based > > backports that live in my renesas-backports tree? > > Yes, correct. > > The patch was mainly written to satisfy a feature request for your > backports, but I noticed that the same issue exists in upstream as > well. > > Ideally I'd like to use the same code for the backport and upstream, > but I am not sure if anyone in upstream really cares. The more long > term solution is obviously to install a second clock event, perhaps > that's good enough. > > > If so, could you clearly state this (below the '---' is fine) and > > include a proper Sob line to indicate that it is fit to be merged > > even if that merge is not into mainline. > > Sure, but I'd like to hear opinions from other people before > resending. I will follow your recommendation in next version. Thanks, I understand. I'll wait for discussion and a new version. -- 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 Mon, Jun 17, 2013 at 01:20:56AM +0100, Magnus Damm wrote: > From: Magnus Damm <damm@opensource.se> > > Modify the ARM architected timer driver to not set C3STOP > in case CPU_IDLE is disabled. This is a short term fix that > allows use of high resolution timers even though no additional > clock event is registered. > > Not-really-Signed-off-by: Magnus Damm <damm@opensource.se> > --- > > If someone cares about this case then perhaps it should be > moved up to the clock event main code. The same issue should > in theory trigger on all architectures, perhaps x86 people > hunting for low latency may try to disable CPU_IDLE? I think that changing tick_is_oneshot_capable and friends to only worry about C3STOP when CPU_IDLE is enabled would be a nicer solution. That way you enable all clock_event_devices with C3STOP to function as high resolution timers when CPU_IDLE's selected. Presenting the hardware differently depending on CPU_IDLE feels wrong. Having some other clock_event_device would be a nicer solution still. Thanks, Mark. > > I propose carrying this patch locally to enable high resolution > timers until CPU_IDLE and an additional clock event is supported. > > Observed on r8a73a4 and APE6EVM. > > drivers/clocksource/arm_arch_timer.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > --- 0001/drivers/clocksource/arm_arch_timer.c > +++ work/drivers/clocksource/arm_arch_timer.c 2013-06-17 09:03:44.000000000 +0900 > @@ -125,7 +125,23 @@ static int arch_timer_set_next_event_phy > > static int __cpuinit arch_timer_setup(struct clock_event_device *clk) > { > - clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP; > + clk->features = CLOCK_EVT_FEAT_ONESHOT; > +#ifdef CONFIG_CPU_IDLE > + /* By not setting the C3STOP flag it is possible to let the > + * ARM architected timer to be the only clock event installed > + * on the system and have working high resolution timers. > + * > + * If the C3STOP flag is set unconditionally then the kernel > + * will always prevent using the high resoultion timer feature > + * unless an additional clock event is registered. > + * > + * In the case where CPU_IDLE is enabled then there is a chance > + * that deeper sleep states will be handled by software, but > + * if CPU_IDLE is disabled then deep sleep states cannot be > + * entered and the feature flagged by C3STOP is not needed. > + */ > + clk->features |= CLOCK_EVT_FEAT_C3STOP; > +#endif > clk->name = "arch_sys_timer"; > clk->rating = 450; > if (arch_timer_use_virtual) { > -- 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 Mark, On Mon, Jun 17, 2013 at 11:53 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Jun 17, 2013 at 01:20:56AM +0100, Magnus Damm wrote: >> From: Magnus Damm <damm@opensource.se> >> >> Modify the ARM architected timer driver to not set C3STOP >> in case CPU_IDLE is disabled. This is a short term fix that >> allows use of high resolution timers even though no additional >> clock event is registered. >> >> Not-really-Signed-off-by: Magnus Damm <damm@opensource.se> >> --- >> >> If someone cares about this case then perhaps it should be >> moved up to the clock event main code. The same issue should >> in theory trigger on all architectures, perhaps x86 people >> hunting for low latency may try to disable CPU_IDLE? > > I think that changing tick_is_oneshot_capable and friends to only worry about > C3STOP when CPU_IDLE is enabled would be a nicer solution. That way you enable > all clock_event_devices with C3STOP to function as high resolution timers when > CPU_IDLE's selected. Presenting the hardware differently depending on CPU_IDLE > feels wrong. I agree that doing this in the clock event driver is a bit odd. So because of that I just posted this patch: [PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled > Having some other clock_event_device would be a nicer solution still. No doubt that another clock event device helps, but mainly together with CPU Idle IMO. Thanks for your comments! / 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
--- 0001/drivers/clocksource/arm_arch_timer.c +++ work/drivers/clocksource/arm_arch_timer.c 2013-06-17 09:03:44.000000000 +0900 @@ -125,7 +125,23 @@ static int arch_timer_set_next_event_phy static int __cpuinit arch_timer_setup(struct clock_event_device *clk) { - clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP; + clk->features = CLOCK_EVT_FEAT_ONESHOT; +#ifdef CONFIG_CPU_IDLE + /* By not setting the C3STOP flag it is possible to let the + * ARM architected timer to be the only clock event installed + * on the system and have working high resolution timers. + * + * If the C3STOP flag is set unconditionally then the kernel + * will always prevent using the high resoultion timer feature + * unless an additional clock event is registered. + * + * In the case where CPU_IDLE is enabled then there is a chance + * that deeper sleep states will be handled by software, but + * if CPU_IDLE is disabled then deep sleep states cannot be + * entered and the feature flagged by C3STOP is not needed. + */ + clk->features |= CLOCK_EVT_FEAT_C3STOP; +#endif clk->name = "arch_sys_timer"; clk->rating = 450; if (arch_timer_use_virtual) {
From: Magnus Damm <damm@opensource.se> Modify the ARM architected timer driver to not set C3STOP in case CPU_IDLE is disabled. This is a short term fix that allows use of high resolution timers even though no additional clock event is registered. Not-really-Signed-off-by: Magnus Damm <damm@opensource.se> --- If someone cares about this case then perhaps it should be moved up to the clock event main code. The same issue should in theory trigger on all architectures, perhaps x86 people hunting for low latency may try to disable CPU_IDLE? I propose carrying this patch locally to enable high resolution timers until CPU_IDLE and an additional clock event is supported. Observed on r8a73a4 and APE6EVM. drivers/clocksource/arm_arch_timer.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) -- 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