rtc: Set wakeup capability for I2C and SPI RTC drivers
PATCH 1/1 rtc: Set wakeup capability for I2C and SPI RTC drivers
RTC core won't allow wakeup alarms to be set if RTC devices' parent (i.e. i2c_client or spi_device) isn't wakeup capable.
For I2C devices there is I2C_CLIENT_WAKE flag exists that we can pass via board info, and if set, I2C core will initialize wakeup capability. For SPI devices there is no such flag at all.
I believe that it's not platform code responsibility to allow or disallow wakeups, instead, drivers themselves should set the capability if a device can trigger wakeups.
That's what drivers/base/power/sysfs.c says:
* It is the responsibility of device drivers to enable (or disable)
* wakeup signaling as part of changing device power states, respecting
* the policy choices provided through the driver model.
I2C and SPI RTC devices send wakeup events via interrupt lines, so we should set the wakeup capability if IRQ is routed.
Ideally we should also check irq for wakeup capability before setting device's capability, i.e.
if (can_irq_wake(irq)) device_set_wakeup_capable(&client->dev, 1);
But there is no can_irq_wake() call exist, and it is not that trivial to implement it for all interrupts controllers and complex/cascaded setups.
drivers/base/power/sysfs.c also covers these cases:
* Devices may not be able to generate wakeup events from all power
* states. Also, the events may be ignored in some configurations;
* for example, they might need help from other devices that aren't
* active
So there is no guarantee that wakeup will actually work, and so I think there is no point in being pedantic wrt checking IRQ wakeup capability.
--- drivers/rtc/rtc-ds1305.c | 2 ++ drivers/rtc/rtc-ds1307.c | 2 ++ drivers/rtc/rtc-ds1374.c | 2 ++ 3 files changed, 6 insertions(+), 0 deletions(-)
drivers/rtc/rtc-ds1305.c +2 -0
@@ -780,6 +780,8 @@ static int __devinit ds1305_probe(struct spi_device *spi) spi->irq, status); goto fail1; } + +device_set_wakeup_capable (&spi->dev,1 ); }/* export NVRAM */
drivers/rtc/rtc-ds1307.c +2 -0
@@ -881,6 +881,8 @@ read_rtc: "unable to request IRQ!\n"); goto exit_irq; } + +device_set_wakeup_capable (&client->dev,1 );set_bit (HAS_ALARM , &ds1307->flags);dev_dbg (&client->dev,"got IRQ %d\n" , client->irq); }
drivers/rtc/rtc-ds1374.c +2 -0
@@ -383,6 +383,8 @@ static int ds1374_probe(struct i2c_client *client,dev_err (&client->dev,"unable to request IRQ\n" ); goto out_free; } + +device_set_wakeup_capable (&client->dev,1 ); } ds1374->rtc = rtc_device_register(client->name, &client->dev,
Signature · 1 line
1.6.3.3
NAK; see details below
Quoted 2 lines · Anton Vorontsov
RTC core won't allow wakeup alarms to be set if RTC devices' parent (i.e.
i2c_clientorspi_device) isn't wakeup capable.
Quite rightly so ... being wakeup-capable is config-specific.
Quoted 3 lines
For I2C devices there is
I2C_CLIENT_WAKEflag exists that we can pass via board info, and if set, I2C core will initialize wakeup capability. For SPI devices there is no such flag at all.
So why not add it for SPI? If it's an issue, it's not unique to RTC devices.
Quoted 3 lines
I believe that it's not platform code responsibility to allow or disallow wakeups, instead, drivers themselves should set the capability if a device can trigger wakeups.
Drivers can't generally know if that's possible though. They need to be told that it is, by platform code.
Most devices can't issue wakeup events.
Quoted 8 lines
That's what
drivers/base/power/sysfs.csays:* It is the responsibility of device drivers to enable (or disable)
* wakeup signaling as part of changing device power states, respecting
* the policy choices provided through the driver model.I2C and SPI RTC devices send wakeup events via interrupt lines, so we should set the wakeup capability if IRQ is routed.
Re-read the quoted sentence. You're saying that policy choices should be hard-wired into the driver; contrary to that quote. (In this case the choice is one that platform code must report, and which the hardware designer made: if the device can issue wakeup events.)
Quoted 9 lines
Ideally we should also check irq for wakeup capability before setting device's capability, i.e.
if (can_irq_wake(irq)) device_set_wakeup_capable(&client->dev, 1);But there is no
can_irq_wake()call exist, and it is not that trivial to implement it for all interrupts controllers and complex/cascaded setups.
That is why platform code should device_init_wakeup() and drivers should check device_can_wakeup(dev) ...
Quoted 8 lines
drivers/base/power/sysfs.calso covers these cases:* Devices may not be able to generate wakeup events from all power
* states. Also, the events may be ignored in some configurations;
* for example, they might need help from other devices that aren't
* activeSo there is no guarantee that wakeup will actually work,
Yes there is ... it's only **exceptional** cases where it can't work. Your patch would make it routine that those flags be unreliable; pretty nasty.
On Thu, Aug 27, 2009 at 02:52:36PM -0700, David Brownell wrote:
[...]
Quoted 6 lines
Quoted 3 lines
I believe that it's not platform code responsibility to allow or disallow wakeups, instead, drivers themselves should set the capability if a device can trigger wakeups.
Drivers can't generally know if that's possible though. They need to be told that it is, by platform code.
Um? Of course they know, DS1374 device driver knows that DS1374 chips can issue wakeups, what it doesn't know is if that wakeup event will trigger CPU resume/power-up.
And even platform code doesn't know that. For example, on PowerPC CPUs there could be several 'suspend' states, some allow wakeup on particular IRQs, some don't. In some cases wakeup event will be ignored, in other cases it will take effect. The suspend mode depends on user's decision ('echo <mode> > /sys/power/state').
Quoted 1 line
Most devices can't issue wakeup events.
The device drivers I modified know that devices can issue wakeup events.
Quoted 6 lines
Quoted 6 lines
That's what
drivers/base/power/sysfs.csays:* It is the responsibility of device drivers to enable (or disable)
* wakeup signaling as part of changing device power states, respecting
* the policy choices provided through the driver model.
Quoted 8 lines
Quoted 2 lines
I2C and SPI RTC devices send wakeup events via interrupt lines, so we should set the wakeup capability if IRQ is routed.
Re-read the quoted sentence. You're saying that policy choices should be hard-wired into the driver; contrary to that quote. (In this case the choice is one that platform code must report, and which the hardware designer made: if the device can issue wakeup events.)
The patch doesn't hard-wire the policy, quite the contrary: it removes the "can't do wakeup" policy.
I'm just adding device_set_wakeup_capable() calls, telling everybody that the devices can issue wakeup events (and they truly can).
Yes, it could be that the IRQ line is wired not to a CPU, but to some power switch, and so nobody pass any IRQs to the drivers. In that case platform-specific I2C_CLIENT_WAKE may be useful.
Quoted 6 lines
Quoted 6 lines
Ideally we should also check irq for wakeup capability before setting device's capability, i.e.
if (can_irq_wake(irq)) device_set_wakeup_capable(&client->dev, 1);
Quoted 6 lines
Quoted 3 lines
But there is no
can_irq_wake()call exist, and it is not that trivial to implement it for all interrupts controllers and complex/cascaded setups.That is why platform code should
device_init_wakeup()and drivers should checkdevice_can_wakeup(dev)...
They should (and do) check may_wakeup() (i.e. should_wakeup) before suspending, not can_wakeup().
static int ds1374_suspend(struct i2c_client *client, pm_message_t state)
{
if (client->irq >= 0 && device_may_wakeup(&client->dev))
enable_irq_wake(client->irq);
return 0;
}(quite funny, they issue enable_irq_wake(), assuming that otherwise IRQ line won't trigger CPU wakeup. But in reality, there are interrupt controllers that you can't control in that regard: any IRQ activity will always resume CPU. And so 'echo disable > /sys/.../wakeup' won't guarantee anything. Unreliable, nasty? Could be.)
Quoted 7 lines
Quoted 7 lines
drivers/base/power/sysfs.calso covers these cases:* Devices may not be able to generate wakeup events from all power
* states. Also, the events may be ignored in some configurations;
* for example, they might need help from other devices that aren't
* active
Quoted 5 lines
Quoted 1 line
So there is no guarantee that wakeup will actually work,
Yes there is ... it's only **exceptional** cases where it can't work. Your patch would make it routine that those flags be unreliable; pretty nasty.
It's not exceptional at all, you really can't tell if device's wakeup event will take any effect. It depends on so many factors that it's virtually impossible to account them all. And hiding wakeup capability only because you aren't sure _is_ policy hard-wiring, no?
Thanks,
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
On Thu, Aug 27, 2009 at 02:52:36PM -0700, David Brownell wrote:
[...]
Quoted 6 lines
Quoted 3 lines
I believe that it's not platform code responsibility to allow or disallow wakeups, instead, drivers themselves should set the capability if a device can trigger wakeups.
Drivers can't generally know if that's possible though. They need to be told that it is, by platform code.
Um? Of course they know, DS1374 device driver knows that DS1374 chips can issue wakeups, what it doesn't know is if that wakeup event will trigger CPU resume/power-up.
And even platform code doesn't know that. For example, on PowerPC CPUs there could be several 'suspend' states, some allow wakeup on particular IRQs, some don't. In some cases wakeup event will be ignored, in other cases it will take effect. The suspend mode depends on user's decision ('echo <mode> > /sys/power/state').
Quoted 1 line
Most devices can't issue wakeup events.
The device drivers I modified know that devices can issue wakeup events.
Quoted 6 lines
Quoted 6 lines
That's what
drivers/base/power/sysfs.csays:* It is the responsibility of device drivers to enable (or disable)
* wakeup signaling as part of changing device power states, respecting
* the policy choices provided through the driver model.
Quoted 8 lines
Quoted 2 lines
I2C and SPI RTC devices send wakeup events via interrupt lines, so we should set the wakeup capability if IRQ is routed.
Re-read the quoted sentence. You're saying that policy choices should be hard-wired into the driver; contrary to that quote. (In this case the choice is one that platform code must report, and which the hardware designer made: if the device can issue wakeup events.)
The patch doesn't hard-wire the policy, quite the contrary: it removes the "can't do wakeup" policy.
I'm just adding device_set_wakeup_capable() calls, telling everybody that the devices can issue wakeup events (and they truly can).
Yes, it could be that the IRQ line is wired not to a CPU, but to some power switch, and so nobody pass any IRQs to the drivers. In that case platform-specific I2C_CLIENT_WAKE may be useful.
Quoted 6 lines
Quoted 6 lines
Ideally we should also check irq for wakeup capability before setting device's capability, i.e.
if (can_irq_wake(irq)) device_set_wakeup_capable(&client->dev, 1);
Quoted 6 lines
Quoted 3 lines
But there is no
can_irq_wake()call exist, and it is not that trivial to implement it for all interrupts controllers and complex/cascaded setups.That is why platform code should
device_init_wakeup()and drivers should checkdevice_can_wakeup(dev)...
They should (and do) check may_wakeup() (i.e. should_wakeup) before suspending, not can_wakeup().
static int ds1374_suspend(struct i2c_client *client, pm_message_t state)
{
if (client->irq >= 0 && device_may_wakeup(&client->dev))
enable_irq_wake(client->irq);
return 0;
}(quite funny, they issue enable_irq_wake(), assuming that otherwise IRQ line won't trigger CPU wakeup. But in reality, there are interrupt controllers that you can't control in that regard: any IRQ activity will always resume CPU. And so 'echo disable > /sys/.../wakeup' won't guarantee anything. Unreliable, nasty? Could be.)
Quoted 7 lines
Quoted 7 lines
drivers/base/power/sysfs.calso covers these cases:* Devices may not be able to generate wakeup events from all power
* states. Also, the events may be ignored in some configurations;
* for example, they might need help from other devices that aren't
* active
Quoted 5 lines
Quoted 1 line
So there is no guarantee that wakeup will actually work,
Yes there is ... it's only **exceptional** cases where it can't work. Your patch would make it routine that those flags be unreliable; pretty nasty.
It's not exceptional at all, you really can't tell if device's wakeup event will take any effect. It depends on so many factors that it's virtually impossible to account them all. And hiding wakeup capability only because you aren't sure _is_ policy hard-wiring, no?
Thanks,
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
On Fri, Aug 28, 2009 at 03:19:25AM +0400, Anton Vorontsov wrote:
[...]
Quoted 13 lines
Quoted 2 lines
That is why platform code should
device_init_wakeup()and drivers should checkdevice_can_wakeup(dev)...They should (and do) check
may_wakeup()(i.e.should_wakeup) before suspending, notcan_wakeup().static int
ds1374_suspend(struct i2c_client *client, pm_message_t state){ if (client->irq >= 0 && device_may_wakeup(&client->dev)) enable_irq_wake(client->irq); return 0; }
Quoted 5 lines
(quite funny, they issue
enable_irq_wake(), assuming that otherwise IRQ line won't trigger CPU wakeup. But in reality, there are interrupt controllers that you can't control in that regard: any IRQ activity will always resume CPU. And so 'echo disable >/sys/.../wakeup' won't guarantee anything. Unreliable, nasty? Could be.)
BTW, of course we can fix this by masking interrupts before suspending, but nobody actually do this (but should, I think).
And if RTC's IRQ is wired to power switch you're in trouble without any way to fix this.
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
On Fri, Aug 28, 2009 at 03:19:25AM +0400, Anton Vorontsov wrote:
[...]
Quoted 13 lines
Quoted 2 lines
That is why platform code should
device_init_wakeup()and drivers should checkdevice_can_wakeup(dev)...They should (and do) check
may_wakeup()(i.e.should_wakeup) before suspending, notcan_wakeup().static int
ds1374_suspend(struct i2c_client *client, pm_message_t state){ if (client->irq >= 0 && device_may_wakeup(&client->dev)) enable_irq_wake(client->irq); return 0; }
Quoted 5 lines
(quite funny, they issue
enable_irq_wake(), assuming that otherwise IRQ line won't trigger CPU wakeup. But in reality, there are interrupt controllers that you can't control in that regard: any IRQ activity will always resume CPU. And so 'echo disable >/sys/.../wakeup' won't guarantee anything. Unreliable, nasty? Could be.)
BTW, of course we can fix this by masking interrupts before suspending, but nobody actually do this (but should, I think).
And if RTC's IRQ is wired to power switch you're in trouble without any way to fix this.
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2
Quoted 21 lines · On Fri, 28 Aug 2009 04:30:10 +0400 Anton Vorontsov <[email protected]>…
On Fri, Aug 28, 2009 at 03:19:25AM +0400, Anton Vorontsov wrote:
[...]Quoted 13 lines
Quoted 2 lines
That is why platform code should
device_init_wakeup()and drivers should checkdevice_can_wakeup(dev)...They should (and do) check
may_wakeup()(i.e.should_wakeup) before suspending, notcan_wakeup().static int
ds1374_suspend(struct i2c_client *client, pm_message_t state){ if (client->irq >= 0 && device_may_wakeup(&client->dev)) enable_irq_wake(client->irq); return 0; }Quoted 5 lines
(quite funny, they issue
enable_irq_wake(), assuming that otherwise IRQ line won't trigger CPU wakeup. But in reality, there are interrupt controllers that you can't control in that regard: any IRQ activity will always resume CPU. And so 'echo disable >/sys/.../wakeup' won't guarantee anything. Unreliable, nasty? Could be.)
Quoted 6 lines
BTW, of course we can fix this by masking interrupts before suspending, but nobody actually do this (but should, I think).
And if RTC's IRQ is wired to power switch you're in trouble without any way to fix this.
afaik nothing got resolved here, so this patch is floating about wondering what its future will be.
From: Anton Vorontsov <[email protected]>
RTC core won't allow wakeup alarms to be set if RTC devices' parent (i.e. i2c_client or spi_device) isn't wakeup capable.
For I2C devices there is I2C_CLIENT_WAKE flag exists that we can pass via board info, and if set, I2C core will initialize wakeup capability. For SPI devices there is no such flag at all.
I believe that it's not platform code responsibility to allow or disallow wakeups, instead, drivers themselves should set the capability if a device can trigger wakeups.
That's what drivers/base/power/sysfs.c says:
* It is the responsibility of device drivers to enable (or disable)
* wakeup signaling as part of changing device power states, respecting
* the policy choices provided through the driver model.
I2C and SPI RTC devices send wakeup events via interrupt lines, so we should set the wakeup capability if IRQ is routed.
Ideally we should also check irq for wakeup capability before setting device's capability, i.e.
if (can_irq_wake(irq)) device_set_wakeup_capable(&client->dev, 1);
But there is no can_irq_wake() call exist, and it is not that trivial to implement it for all interrupts controllers and complex/cascaded setups.
drivers/base/power/sysfs.c also covers these cases:
* Devices may not be able to generate wakeup events from all power
* states. Also, the events may be ignored in some configurations;
* for example, they might need help from other devices that aren't
* active
So there is no guarantee that wakeup will actually work, and so I think there is no point in being pedantic wrt checking IRQ wakeup capability.
---
drivers/rtc/rtc-ds1305.c | 2 ++ drivers/rtc/rtc-ds1307.c | 2 ++ drivers/rtc/rtc-ds1374.c | 2 ++ 3 files changed, 6 insertions(+)
drivers/rtc/rtc-ds1305.c renamed from drivers/rtc/rtc-ds1305.c~rtc-set-wakeup-capability-for-i2c-and-spi-rtc-drivers +2 -0
@@ -780,6 +780,8 @@ static int __devinit ds1305_probe(struct spi->irq, status); goto fail1; } + +device_set_wakeup_capable (&spi->dev,1 ); }/* export NVRAM */
drivers/rtc/rtc-ds1307.c renamed from drivers/rtc/rtc-ds1307.c~rtc-set-wakeup-capability-for-i2c-and-spi-rtc-drivers +2 -0
@@ -881,6 +881,8 @@ read_rtc: "unable to request IRQ!\n"); goto exit_irq; } + +device_set_wakeup_capable (&client->dev,1 );set_bit (HAS_ALARM , &ds1307->flags);dev_dbg (&client->dev,"got IRQ %d\n" , client->irq); }
drivers/rtc/rtc-ds1374.c renamed from drivers/rtc/rtc-ds1374.c~rtc-set-wakeup-capability-for-i2c-and-spi-rtc-drivers +2 -0
@@ -383,6 +383,8 @@ static int ds1374_probe(struct i2c_cliendev_err (&client->dev,"unable to request IRQ\n" ); goto out_free; } + +device_set_wakeup_capable (&client->dev,1 ); } ds1374->rtc = rtc_device_register(client->name, &client->dev,
_
Quoted 21 lines · On Fri, 28 Aug 2009 04:30:10 +0400 Anton Vorontsov <[email protected]>…
On Fri, Aug 28, 2009 at 03:19:25AM +0400, Anton Vorontsov wrote:
[...]Quoted 13 lines
Quoted 2 lines
That is why platform code should
device_init_wakeup()and drivers should checkdevice_can_wakeup(dev)...They should (and do) check
may_wakeup()(i.e.should_wakeup) before suspending, notcan_wakeup().static int
ds1374_suspend(struct i2c_client *client, pm_message_t state){ if (client->irq >= 0 && device_may_wakeup(&client->dev)) enable_irq_wake(client->irq); return 0; }Quoted 5 lines
(quite funny, they issue
enable_irq_wake(), assuming that otherwise IRQ line won't trigger CPU wakeup. But in reality, there are interrupt controllers that you can't control in that regard: any IRQ activity will always resume CPU. And so 'echo disable >/sys/.../wakeup' won't guarantee anything. Unreliable, nasty? Could be.)
Quoted 6 lines
BTW, of course we can fix this by masking interrupts before suspending, but nobody actually do this (but should, I think).
And if RTC's IRQ is wired to power switch you're in trouble without any way to fix this.
afaik nothing got resolved here, so this patch is floating about wondering what its future will be.
From: Anton Vorontsov <[email protected]>
RTC core won't allow wakeup alarms to be set if RTC devices' parent (i.e. i2c_client or spi_device) isn't wakeup capable.
For I2C devices there is I2C_CLIENT_WAKE flag exists that we can pass via board info, and if set, I2C core will initialize wakeup capability. For SPI devices there is no such flag at all.
I believe that it's not platform code responsibility to allow or disallow wakeups, instead, drivers themselves should set the capability if a device can trigger wakeups.
That's what drivers/base/power/sysfs.c says:
* It is the responsibility of device drivers to enable (or disable)
* wakeup signaling as part of changing device power states, respecting
* the policy choices provided through the driver model.
I2C and SPI RTC devices send wakeup events via interrupt lines, so we should set the wakeup capability if IRQ is routed.
Ideally we should also check irq for wakeup capability before setting device's capability, i.e.
if (can_irq_wake(irq)) device_set_wakeup_capable(&client->dev, 1);
But there is no can_irq_wake() call exist, and it is not that trivial to implement it for all interrupts controllers and complex/cascaded setups.
drivers/base/power/sysfs.c also covers these cases:
* Devices may not be able to generate wakeup events from all power
* states. Also, the events may be ignored in some configurations;
* for example, they might need help from other devices that aren't
* active
So there is no guarantee that wakeup will actually work, and so I think there is no point in being pedantic wrt checking IRQ wakeup capability.
---
drivers/rtc/rtc-ds1305.c | 2 ++ drivers/rtc/rtc-ds1307.c | 2 ++ drivers/rtc/rtc-ds1374.c | 2 ++ 3 files changed, 6 insertions(+)
drivers/rtc/rtc-ds1305.c renamed from drivers/rtc/rtc-ds1305.c~rtc-set-wakeup-capability-for-i2c-and-spi-rtc-drivers +2 -0
@@ -780,6 +780,8 @@ static int __devinit ds1305_probe(struct spi->irq, status); goto fail1; } + +device_set_wakeup_capable (&spi->dev,1 ); }/* export NVRAM */
drivers/rtc/rtc-ds1307.c renamed from drivers/rtc/rtc-ds1307.c~rtc-set-wakeup-capability-for-i2c-and-spi-rtc-drivers +2 -0
@@ -881,6 +881,8 @@ read_rtc: "unable to request IRQ!\n"); goto exit_irq; } + +device_set_wakeup_capable (&client->dev,1 );set_bit (HAS_ALARM , &ds1307->flags);dev_dbg (&client->dev,"got IRQ %d\n" , client->irq); }
drivers/rtc/rtc-ds1374.c renamed from drivers/rtc/rtc-ds1374.c~rtc-set-wakeup-capability-for-i2c-and-spi-rtc-drivers +2 -0
@@ -383,6 +383,8 @@ static int ds1374_probe(struct i2c_cliendev_err (&client->dev,"unable to request IRQ\n" ); goto out_free; } + +device_set_wakeup_capable (&client->dev,1 ); } ds1374->rtc = rtc_device_register(client->name, &client->dev,
_