Patchwork [v2] RTC: Selectively enable PIE-Hrtimer emulation.

login
register
mail settings
Submitter MyungJoo Ham
Date March 22, 2011, 8:08 a.m.
Message ID <1300781334-30308-1-git-send-email-myungjoo.ham@samsung.com>
Download mbox | patch
Permalink /patch/87879/
State New
Headers show

Comments

MyungJoo Ham - March 22, 2011, 8:08 a.m.
The patch of John Stultz, "RTC: Rework RTC code to use timerqueue for
events", has enabled PIE interrupt emulation with hrtimer unconditionally.
However, we do have RTC devices that support PIE and such devices are
not meant to be emulated by hrtimer. Besides, we sometimes need RTC-PIE
to function while hrtimer does not; e.g., CPU's RTC PIE as a
suspend-to-RAM wakeup source.

This patch allows to selectively use the PIE emulation. If the rtc
driver has implemented PIE related ops (irq_set_state and irq_set_freq),
PIE emulation is not used. Currently, rtc-s3c, rtc-cmos,
rtc-davinci, rtc-pl031, rtc-pxa, rtc-sa1100, rtc-sh, and rtc-vt41xx have
them.

Besides, rtc->pie_enabled is renamed as rtc->pie_emul_enabled. It is
because that variable denotes whether HRTimer-PIE is set or not.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/class.c     |    2 +-
 drivers/rtc/interface.c |   33 +++++++++++++++++++++++++--------
 include/linux/rtc.h     |    2 +-
 3 files changed, 27 insertions(+), 10 deletions(-)
John Stultz - March 22, 2011, 7:44 p.m.
On Tue, 2011-03-22 at 17:08 +0900, MyungJoo Ham wrote:
> The patch of John Stultz, "RTC: Rework RTC code to use timerqueue for
> events", has enabled PIE interrupt emulation with hrtimer unconditionally.
> However, we do have RTC devices that support PIE and such devices are
> not meant to be emulated by hrtimer.

I guess the question I want to ask here, is what is the value of getting
PIE interrupts over the hrtimer? For most cases it is yet another
interrupt. Could you expand a bit more on why you need real PIE irqs?

> Besides, we sometimes need RTC-PIE
> to function while hrtimer does not; e.g., CPU's RTC PIE as a
> suspend-to-RAM wakeup source.

So this indeed is a limitation. hrtimer emulated PIE's won't wake the
system up.  So is this a "Besides...sometimes" sort of case, or is PIE
wakeups really the only issue here?

To better understand the scope of the issue, I'd like to know more about
your uage of PIE mode as a wakeup source. Given PIE mode is for sub 1HZ
irqs, do you really use suspend/resume at a periodic rate multiple times
a second? Is this done with the current mainline kernel? What would be
the penalty of moving to a 1HZ wakeup?

If this indeed a critical limitation, then it needs to be solved.
However, instead of just re-enabling the pie mode access to userland
(which will likely need more then the patch here provides to avoid
breaking the RTC event multiplexing), I'd much prefer to see the the
solution as extending the in-kernel RTC API to allow for sub-second
resolution alarms, then using a periodic mode rtc alarm to emulate PIE
mode irqs out to userland.

Alessandro: Your thoughts here? As it seems most RTC hardware can handle
it, any objections to extending the in-kernel interface to provide
sub-second resolution (it doesn't mean they will provide it, but it
doesn't limit the hardware at the interface level).

> This patch allows to selectively use the PIE emulation. If the rtc
> driver has implemented PIE related ops (irq_set_state and irq_set_freq),

So, both irq_set_state and irq_set_freq were removed upstream in the
2.6.39-rc1 merge window, so this patch def won't work upstream. 

Sorry for the trouble here! Hopefully we can get things resolved and
working shortly here.

thanks
-john
MyungJoo Ham - March 24, 2011, 5:28 a.m.
On Wed, Mar 23, 2011 at 4:44 AM, John Stultz <john.stultz@linaro.org> wrote:
> On Tue, 2011-03-22 at 17:08 +0900, MyungJoo Ham wrote:
>> The patch of John Stultz, "RTC: Rework RTC code to use timerqueue for
>> events", has enabled PIE interrupt emulation with hrtimer unconditionally.
>> However, we do have RTC devices that support PIE and such devices are
>> not meant to be emulated by hrtimer.
>
> I guess the question I want to ask here, is what is the value of getting
> PIE interrupts over the hrtimer? For most cases it is yet another
> interrupt. Could you expand a bit more on why you need real PIE irqs?

I have been implementing charger driver (and then, moving on to
charger "framework") that requires periodic monitoring on the
thermistors and properties of battery chargers that do not have
interrupts. Because the chargers keep charging during suspend, I need
to keep monitoring them during suspend. I have been doing this by
periodically waking up the system during suspend with usually 30 secs.
Although the PIE frequency is over 1Hz, the RTC device in the CPUs we
use (S3Cxxxx, S5Pxxxx) have TICCNT entry, which allows to have PIE
interrupt every TICCNT-th PIE-Tick; e.g., with TICCNT=30 and
PIE-frequency=1Hz, we have an interrupt every 30 seconds. (This TICCNT
feature is not included in mainline rtc-s3c, yet.)

For now, I use an additional EXPORTed functions for setting TICCNT
provided with rtc-s3c.c.

>
>> Besides, we sometimes need RTC-PIE
>> to function while hrtimer does not; e.g., CPU's RTC PIE as a
>> suspend-to-RAM wakeup source.
>
> So this indeed is a limitation. hrtimer emulated PIE's won't wake the
> system up.  So is this a "Besides...sometimes" sort of case, or is PIE
> wakeups really the only issue here?

So far, this is the only issue affecting us with the PIE emulation.

Some potential issues that I'm not concerned for now are;
a. we are blocking a hardware's function with an emulated feature.
b. there could be multiple rtc devices in a system and we may need to
setup them anyway without any s/w interrupt handlers. (RTC PIEs
directly signalling some other H/W pieces?)

>
> To better understand the scope of the issue, I'd like to know more about
> your uage of PIE mode as a wakeup source. Given PIE mode is for sub 1HZ
> irqs, do you really use suspend/resume at a periodic rate multiple times
> a second? Is this done with the current mainline kernel? What would be
> the penalty of moving to a 1HZ wakeup?

No, I don't use it for periodic sub-1HZ wakeups. I use it for
several-seconds periodic wakeups. I have not posted such features to
mainline kernel, yet.


>
> If this indeed a critical limitation, then it needs to be solved.
> However, instead of just re-enabling the pie mode access to userland
> (which will likely need more then the patch here provides to avoid
> breaking the RTC event multiplexing), I'd much prefer to see the the
> solution as extending the in-kernel RTC API to allow for sub-second
> resolution alarms, then using a periodic mode rtc alarm to emulate PIE
> mode irqs out to userland.

If we are to provide in-kernel RTC eumulation API to userland, why
don't we separate it from H/W RTC API dev? (e.g., provide emulated rtc
as /dev/rtc, and provide H/W rtc as /dev/rtcX). We already have
multiple H/W RTC devs (e.g., /dev/rtc0, /dev/rtc1) in some systems. In
the current scheme, we may have multiple H/W RTCs with each having S/W
emulated PIEs (essentially same and redundant.)

>
> Alessandro: Your thoughts here? As it seems most RTC hardware can handle
> it, any objections to extending the in-kernel interface to provide
> sub-second resolution (it doesn't mean they will provide it, but it
> doesn't limit the hardware at the interface level).
>
>> This patch allows to selectively use the PIE emulation. If the rtc
>> driver has implemented PIE related ops (irq_set_state and irq_set_freq),
>
> So, both irq_set_state and irq_set_freq were removed upstream in the
> 2.6.39-rc1 merge window, so this patch def won't work upstream.
>
> Sorry for the trouble here! Hopefully we can get things resolved and
> working shortly here.
>
> thanks
> -john
>
>

I think providing hrtimer-based emulation for RTC PIE interrupts is
fine; however, for RTC devices that want to use their own PIE
features, I think the framework needs not to block them.


Cheers!

- MyungJoo
John Stultz - March 24, 2011, 9:18 p.m.
On Thu, 2011-03-24 at 14:28 +0900, MyungJoo Ham wrote:
> On Wed, Mar 23, 2011 at 4:44 AM, John Stultz <john.stultz@linaro.org> wrote:
> > On Tue, 2011-03-22 at 17:08 +0900, MyungJoo Ham wrote:
> >> The patch of John Stultz, "RTC: Rework RTC code to use timerqueue for
> >> events", has enabled PIE interrupt emulation with hrtimer unconditionally.
> >> However, we do have RTC devices that support PIE and such devices are
> >> not meant to be emulated by hrtimer.
> >
> > I guess the question I want to ask here, is what is the value of getting
> > PIE interrupts over the hrtimer? For most cases it is yet another
> > interrupt. Could you expand a bit more on why you need real PIE irqs?
> 
> I have been implementing charger driver (and then, moving on to
> charger "framework") that requires periodic monitoring on the
> thermistors and properties of battery chargers that do not have
> interrupts. Because the chargers keep charging during suspend, I need
> to keep monitoring them during suspend. I have been doing this by
> periodically waking up the system during suspend with usually 30 secs.
> Although the PIE frequency is over 1Hz, the RTC device in the CPUs we
> use (S3Cxxxx, S5Pxxxx) have TICCNT entry, which allows to have PIE
> interrupt every TICCNT-th PIE-Tick; e.g., with TICCNT=30 and
> PIE-frequency=1Hz, we have an interrupt every 30 seconds. (This TICCNT
> feature is not included in mainline rtc-s3c, yet.)
> 
> For now, I use an additional EXPORTed functions for setting TICCNT
> provided with rtc-s3c.c.

Ok, so this is a special feautre of the s3c hardware and its being
exposed via adding new functionality to the PIE mode in an out of tree
patch.

So, just to further understand what you're doing, is the periodic
monitoring done in userland, or is this all via in-kernel drivers?


> >> Besides, we sometimes need RTC-PIE
> >> to function while hrtimer does not; e.g., CPU's RTC PIE as a
> >> suspend-to-RAM wakeup source.
> >
> > So this indeed is a limitation. hrtimer emulated PIE's won't wake the
> > system up.  So is this a "Besides...sometimes" sort of case, or is PIE
> > wakeups really the only issue here?
> 
> So far, this is the only issue affecting us with the PIE emulation.
> 
> Some potential issues that I'm not concerned for now are;
> a. we are blocking a hardware's function with an emulated feature.

Well, the current rtc interface is poor, as it has limited granularity
and exposes too much hardware detail directly to userland.  We need to
preserve the existing behavior, but before we extend it to enable new
hardware functionality, I think we should consider if the functionality
should be just tacked on, or added in a better way.

> b. there could be multiple rtc devices in a system and we may need to
> setup them anyway without any s/w interrupt handlers. (RTC PIEs
> directly signalling some other H/W pieces?)

Maybe I'm misunderstanding, but this sounds like an abuse of the RTC
interface. If you're using the RTC interface to enable some sort of
inter-hardware signaling, where the kernel itself wouldn't be handling
the irq, you probably really want to use a different driver
all-together. But again, I might just not understand what you're
meaning.


> I think providing hrtimer-based emulation for RTC PIE interrupts is
> fine; however, for RTC devices that want to use their own PIE
> features, I think the framework needs not to block them.

Well, lets take just a small step back. Instead of thinking in specific
hardware features, we need a somewhat generalized abstraction so we can
enable the functionality in a common way across a wide array of
hardware.

For instance, with the current code in 2.6.38, we can easily provide the
behavior you're looking for (ie: 30-second interval RTC triggered
events) in the kernel:

Just create a struct rtc_timer, use rtc_timer_init() to initialize it
with a call back function and ptr, then call rtc_timer_start() with the
expiration time and the 30 second period. The callback will then be
called every 30 seconds, triggered by the RTC alarm.

The benefit here is that since the kernel manages all of this, it will
then work on any RTC hardware that supports alarms, and doesn't need
some hardware-specific PIE mode support. Even better, since the kernel
can allow for multiplexing of events, userland can still set AIE mode
alarms using the legacy interface without affecting your periodic
rtc_timer.

Now, currently this is kernel-internal functionality, and hasn't yet
been exposed to userland, but I'm looking to change that via the posix
alarm timers work. Where one could simply set a standard timer against
CLOCK_REALTIME_ALARM, and that timer will fire regardless of if the
system is in suspend or not.

In the meantime, you might even be able to convert your currently
out-of-tree TICCNT patch to make use of the rtc_timer to provide
equivalent behavior (Having not seen it, I'm not quite sure how the
TICCNT mode is enabled from userland in your code, but I'd be happy to
look at it and make suggestions on how to change it).

thanks
-john
MyungJoo Ham - March 25, 2011, 5:56 a.m.
On Fri, Mar 25, 2011 at 6:18 AM, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, 2011-03-24 at 14:28 +0900, MyungJoo Ham wrote:
>> On Wed, Mar 23, 2011 at 4:44 AM, John Stultz <john.stultz@linaro.org> wrote:
>> > On Tue, 2011-03-22 at 17:08 +0900, MyungJoo Ham wrote:
>> >> The patch of John Stultz, "RTC: Rework RTC code to use timerqueue for
>> >> events", has enabled PIE interrupt emulation with hrtimer unconditionally.
>> >> However, we do have RTC devices that support PIE and such devices are
>> >> not meant to be emulated by hrtimer.
>> >
>> > I guess the question I want to ask here, is what is the value of getting
>> > PIE interrupts over the hrtimer? For most cases it is yet another
>> > interrupt. Could you expand a bit more on why you need real PIE irqs?
>>
>> I have been implementing charger driver (and then, moving on to
>> charger "framework") that requires periodic monitoring on the
>> thermistors and properties of battery chargers that do not have
>> interrupts. Because the chargers keep charging during suspend, I need
>> to keep monitoring them during suspend. I have been doing this by
>> periodically waking up the system during suspend with usually 30 secs.
>> Although the PIE frequency is over 1Hz, the RTC device in the CPUs we
>> use (S3Cxxxx, S5Pxxxx) have TICCNT entry, which allows to have PIE
>> interrupt every TICCNT-th PIE-Tick; e.g., with TICCNT=30 and
>> PIE-frequency=1Hz, we have an interrupt every 30 seconds. (This TICCNT
>> feature is not included in mainline rtc-s3c, yet.)
>>
>> For now, I use an additional EXPORTed functions for setting TICCNT
>> provided with rtc-s3c.c.
>
> Ok, so this is a special feautre of the s3c hardware and its being
> exposed via adding new functionality to the PIE mode in an out of tree
> patch.
>
> So, just to further understand what you're doing, is the periodic
> monitoring done in userland, or is this all via in-kernel drivers?

It is all via in-kernel drivers; i.e., charger-framework,
board-support-files, and rtc-s3c.c. For now, I'm using a function,
"s3c_rtc_tick_setup", which is not in mainline yet.

>
>
>> >> Besides, we sometimes need RTC-PIE
>> >> to function while hrtimer does not; e.g., CPU's RTC PIE as a
>> >> suspend-to-RAM wakeup source.
>> >
>> > So this indeed is a limitation. hrtimer emulated PIE's won't wake the
>> > system up.  So is this a "Besides...sometimes" sort of case, or is PIE
>> > wakeups really the only issue here?
>>
>> So far, this is the only issue affecting us with the PIE emulation.
>>
>> Some potential issues that I'm not concerned for now are;
>> a. we are blocking a hardware's function with an emulated feature.
>
> Well, the current rtc interface is poor, as it has limited granularity
> and exposes too much hardware detail directly to userland.  We need to
> preserve the existing behavior, but before we extend it to enable new
> hardware functionality, I think we should consider if the functionality
> should be just tacked on, or added in a better way.
>
>> b. there could be multiple rtc devices in a system and we may need to
>> setup them anyway without any s/w interrupt handlers. (RTC PIEs
>> directly signalling some other H/W pieces?)
>
> Maybe I'm misunderstanding, but this sounds like an abuse of the RTC
> interface. If you're using the RTC interface to enable some sort of
> inter-hardware signaling, where the kernel itself wouldn't be handling
> the irq, you probably really want to use a different driver
> all-together. But again, I might just not understand what you're
> meaning.

I use RTC-ALARM (not PIE anyway) to boot up the system at a designated
time. I thought someone might need similar behaviors (RTC interrupt
events setup without S/W handlers involved) with RTC PIEs; thus, it is
not an issue I'm concerned with.

>
>
>> I think providing hrtimer-based emulation for RTC PIE interrupts is
>> fine; however, for RTC devices that want to use their own PIE
>> features, I think the framework needs not to block them.
>
> Well, lets take just a small step back. Instead of thinking in specific
> hardware features, we need a somewhat generalized abstraction so we can
> enable the functionality in a common way across a wide array of
> hardware.

Ok. Then, wouldn't this be out of a specific device driver's
interface? For example when "/dev/rtc0" represents "rtc-s3c" and
"/dev/rtc1" represents "rtc-max8998", such in-kernel s/w emulation
needs to have its own interface; e.g., "/dev/rtc2", "/dev/rtc-kernel",
or something not representing a specific h/w piece.

>
> For instance, with the current code in 2.6.38, we can easily provide the
> behavior you're looking for (ie: 30-second interval RTC triggered
> events) in the kernel:
>
> Just create a struct rtc_timer, use rtc_timer_init() to initialize it
> with a call back function and ptr, then call rtc_timer_start() with the
> expiration time and the 30 second period. The callback will then be
> called every 30 seconds, triggered by the RTC alarm.
>
> The benefit here is that since the kernel manages all of this, it will
> then work on any RTC hardware that supports alarms, and doesn't need
> some hardware-specific PIE mode support. Even better, since the kernel
> can allow for multiplexing of events, userland can still set AIE mode
> alarms using the legacy interface without affecting your periodic
> rtc_timer.
>
> Now, currently this is kernel-internal functionality, and hasn't yet
> been exposed to userland, but I'm looking to change that via the posix
> alarm timers work. Where one could simply set a standard timer against
> CLOCK_REALTIME_ALARM, and that timer will fire regardless of if the
> system is in suspend or not.

Cool. I wasn't aware of this feature and this can be one possible way
to replace TICCNT+RTC-PIE feature in the charger-framework/drivers.
However, this rtc_timer is based on rtc-alarm; thus, TICCNT, which is
for PIE (Tick) events, has nothing to do with rtc_timer.

>
> In the meantime, you might even be able to convert your currently
> out-of-tree TICCNT patch to make use of the rtc_timer to provide
> equivalent behavior (Having not seen it, I'm not quite sure how the
> TICCNT mode is enabled from userland in your code, but I'd be happy to
> look at it and make suggestions on how to change it).
>
> thanks
> -john
>
>
>

I still think this hrtimer emulation should be either separated from
rtc-device-interface that is accessed through /dev/rtcX or optional
for each rtc-device. However, I'll keep the recurring wakeup feature
as an in-kernel-drivers only function provided by rtc-s3c.

Thanks a lot.


Cheers! It's Friday! :)
- MyungJoo
Mark Brown - March 25, 2011, noon
On Thu, Mar 24, 2011 at 02:18:18PM -0700, John Stultz wrote:
> On Thu, 2011-03-24 at 14:28 +0900, MyungJoo Ham wrote:

> > b. there could be multiple rtc devices in a system and we may need to
> > setup them anyway without any s/w interrupt handlers. (RTC PIEs
> > directly signalling some other H/W pieces?)

> Maybe I'm misunderstanding, but this sounds like an abuse of the RTC
> interface. If you're using the RTC interface to enable some sort of
> inter-hardware signaling, where the kernel itself wouldn't be handling
> the irq, you probably really want to use a different driver
> all-together. But again, I might just not understand what you're
> meaning.

This sort of thing is fairly common for embedded RTCs.  Usually there's
no fixed connection, it's just that the RTC outputs a signal which can
be wired to anything the system integrator feels like.  It does seem to
make sense for the RTC driver to export this functionality to the rest
of the world.

> The benefit here is that since the kernel manages all of this, it will
> then work on any RTC hardware that supports alarms, and doesn't need
> some hardware-specific PIE mode support. Even better, since the kernel
> can allow for multiplexing of events, userland can still set AIE mode
> alarms using the legacy interface without affecting your periodic
> rtc_timer.

This only works if you're not generating a hardware signal from the RTC
- if it's a hardware signal you may not be able to do it from software
at all.  This may not be relevant for the particular application,
though.

Patch

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index c404b61..102d706 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -164,7 +164,7 @@  struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 	/* Init pie timer */
 	hrtimer_init(&rtc->pie_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	rtc->pie_timer.function = rtc_pie_update_irq;
-	rtc->pie_enabled = 0;
+	rtc->pie_emul_enabled = 0;
 
 	strlcpy(rtc->name, name, RTC_DEVICE_NAME_SIZE);
 	dev_set_name(&rtc->dev, "rtc%d", id);
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index cb2f072..6d5aa3b 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -450,16 +450,25 @@  int rtc_irq_set_state(struct rtc_device *rtc, struct rtc_task *task, int enabled
 		err = -EBUSY;
 	if (rtc->irq_task != task)
 		err = -EACCES;
+	if (err)
+		goto out;
 
-	if (enabled) {
-		ktime_t period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq);
-		hrtimer_start(&rtc->pie_timer, period, HRTIMER_MODE_REL);
+	if (rtc->ops->irq_set_state) {
+		err = rtc->ops->irq_set_state(rtc->dev.parent, enabled);
+		rtc->pie_emul_enabled = 0;
 	} else {
-		hrtimer_cancel(&rtc->pie_timer);
+		if (enabled) {
+			ktime_t period = ktime_set(0, NSEC_PER_SEC /
+					rtc->irq_freq);
+			hrtimer_start(&rtc->pie_timer, period,
+					HRTIMER_MODE_REL);
+		} else {
+			hrtimer_cancel(&rtc->pie_timer);
+		}
+		rtc->pie_emul_enabled = enabled;
 	}
-	rtc->pie_enabled = enabled;
+out:
 	spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
-
 	return err;
 }
 EXPORT_SYMBOL_GPL(rtc_irq_set_state);
@@ -487,9 +496,16 @@  int rtc_irq_set_freq(struct rtc_device *rtc, struct rtc_task *task, int freq)
 		err = -EBUSY;
 	if (rtc->irq_task != task)
 		err = -EACCES;
-	if (err == 0) {
+	if (err)
+		goto out;
+
+	if (rtc->ops->irq_set_freq) {
+		err = rtc->ops->irq_set_freq(rtc->dev.parent, freq);
+		if (err == 0)
+			rtc->irq_freq = freq;
+	} else {
 		rtc->irq_freq = freq;
-		if (rtc->pie_enabled) {
+		if (rtc->pie_emul_enabled) {
 			ktime_t period;
 			hrtimer_cancel(&rtc->pie_timer);
 			period = ktime_set(0, NSEC_PER_SEC/rtc->irq_freq);
@@ -497,6 +513,7 @@  int rtc_irq_set_freq(struct rtc_device *rtc, struct rtc_task *task, int freq)
 					HRTIMER_MODE_REL);
 		}
 	}
+out:
 	spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
 	return err;
 }
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 89c3e51..b2c0a62 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -201,7 +201,7 @@  struct rtc_device
 	struct rtc_timer aie_timer;
 	struct rtc_timer uie_rtctimer;
 	struct hrtimer pie_timer; /* sub second exp, so needs hrtimer */
-	int pie_enabled;
+	int pie_emul_enabled;
 	struct work_struct irqwork;