Patchwork rtc: make rtc-omap wakeup capable

login
register
mail settings
Submitter Sekhar Nori
Date Nov. 24, 2009, 1:18 p.m.
Message ID <1259068720-26113-1-git-send-email-nsekhar@ti.com>
Download mbox | patch
Permalink /patch/39207/
State Deferred
Headers show

Comments

Sekhar Nori - Nov. 24, 2009, 1:18 p.m.
The rtc-omap driver currently hardcodes the RTC to be
not capable of wakeup events. On the DA850/OMAP-L138
SoC, the RTC is a wake up source from its "deep sleep"
mode.

Let platform data set the wakeup capability flag instead.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
Applies to latest of Linus's tree.

 drivers/rtc/rtc-omap.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)
Sekhar Nori - Jan. 5, 2010, 8:16 a.m.
Hello,

On Tue, Nov 24, 2009 at 18:48:40, Nori, Sekhar wrote:
> The rtc-omap driver currently hardcodes the RTC to be
> not capable of wakeup events. On the DA850/OMAP-L138
> SoC, the RTC is a wake up source from its "deep sleep"
> mode.
>
> Let platform data set the wakeup capability flag instead.
>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

Any comments on this patch?

Thanks,
Sekhar
Alessandro Zummo - Jan. 20, 2010, 11:37 p.m.
On Tue, 5 Jan 2010 13:46:17 +0530
"Nori, Sekhar" <nsekhar@ti.com> wrote:

> > Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>  
> 
> Any comments on this patch?

 please let me know if you get the Acked-by from
 the author.
Sekhar Nori - Jan. 23, 2010, 12:14 p.m.
Hi David, George,

On Thu, Jan 21, 2010 at 05:07:13, Alessandro Zummo wrote:
> On Tue, 5 Jan 2010 13:46:17 +0530
> "Nori, Sekhar" <nsekhar@ti.com> wrote:
>
> > > Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> > > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> >
> > Any comments on this patch?
>
>  please let me know if you get the Acked-by from
>  the author.

Would you please Ack this patch? The patch basically
lets the platform data set the device wake-up capability
flag for OMAP RTC instead of always hard-coding it to zero
in RTC driver.

This should not break any existing platforms since
device_register() initializes this flag to zero anyway.

Here is the link to the patch:

http://patchwork.ozlabs.org/patch/39207/

Thanks,
Sekhar
David Brownell - Jan. 23, 2010, 5:07 p.m.
On Saturday 23 January 2010, Nori, Sekhar wrote:
> Hi David, George,
> 
> Would you please Ack this patch? The patch basically
> lets the platform data set the device wake-up capability
> flag for OMAP RTC instead of always hard-coding it to zero
> in RTC driver.
> 
> This should not break any existing platforms since
> device_register() initializes this flag to zero anyway.
> 
> Here is the link to the patch:
> 
> http://patchwork.ozlabs.org/patch/39207/

This just deletes the device_init_wakeup() call and the
comment describing how/why to use another setting.

Where is the code to replace it and *set* those flags
to a nonzero default, so the device can be woken up?
That is, to achieve what $SUBJECT says this achieves??

The $SUBJECT is inaccurate.  The patch description
also has it wrong ... what I think you're trying to
do is change the model for how the wakeup capability
is configured.  That's fine, and probably overdue.

So I think maybe this needs changing.  Minimally te
code comment; maybe add a block noting chip-specific
issues, where the OMAP1 chips can initialize their
platform devices approppriately IFF their board is
wired right, but OMAP-L138 doesn't have that issue
(and so can unilaterally device_init_wakeup to true
in its chip init logic, for all boards).

And to be paired with a patch which makes the L138
boards behave right... maybe that chip init code
already does that, if so mention that in the patch
comments so we're not left guessing.

I'll be glad to see this driver finally serving as
a wakeup event source.  I don't recall seeing OMAP1
boards wired so that it could do that; else this
would have issued those events a *long* time ago!

- Dave


> 
> Thanks,
> Sekhar
> 
>
David Brownell - Jan. 23, 2010, 5:29 p.m.
On Saturday 23 January 2010, David Brownell wrote:
> So I think maybe this needs changing.  Minimally te
> code comment; maybe add a block noting chip-specific
> issues, where the OMAP1 chips can initialize their
> platform devices approppriately IFF their board is
> wired right, but OMAP-L138 doesn't have that issue
> (and so can unilaterally device_init_wakeup to true
> in its chip init logic, for all boards).

And likewise the header comment at top of file,
which talks about how wiring RTC_WAKE_INT is a
board-specific issue.  Which is true for OMAP1,
but not for L138 ...
Sekhar Nori - Jan. 29, 2010, 6:02 p.m.
Hi Dave,

Thanks for the review and apologize for the late response.
I was out for a while.

On Sat, Jan 23, 2010 at 22:37:25, David Brownell wrote:
> On Saturday 23 January 2010, Nori, Sekhar wrote:
> > Hi David, George,
> >
> > Would you please Ack this patch? The patch basically
> > lets the platform data set the device wake-up capability
> > flag for OMAP RTC instead of always hard-coding it to zero
> > in RTC driver.
> >
> > This should not break any existing platforms since
> > device_register() initializes this flag to zero anyway.
> >
> > Here is the link to the patch:
> >
> > http://patchwork.ozlabs.org/patch/39207/
>
> This just deletes the device_init_wakeup() call and the
> comment describing how/why to use another setting.
>
> Where is the code to replace it and *set* those flags
> to a nonzero default, so the device can be woken up?
> That is, to achieve what $SUBJECT says this achieves??

That code is present in board code after the platform
device is registered. For OMAP-L1 it is done in
da8xx_register_rtc() in arch/arm/mach-davinci/devices-da8xx.c

http://git.kernel.org/?p=linux/kernel/git/khilman/linux-davinci.git;a=blob;f=arch/arm/mach-davinci/devices-da8xx.c;h=0a96791d3b0f6dd8238c4e0826f3f452b3905477;hb=887f8d18de5efbeafc76e5c509fd53f9018992fa#l584

>
> The $SUBJECT is inaccurate.  The patch description
> also has it wrong ... what I think you're trying to
> do is change the model for how the wakeup capability
> is configured.  That's fine, and probably overdue.

Right, I see that now.

>
> So I think maybe this needs changing.  Minimally te
> code comment; maybe add a block noting chip-specific
> issues, where the OMAP1 chips can initialize their
> platform devices approppriately IFF their board is
> wired right, but OMAP-L138 doesn't have that issue
> (and so can unilaterally device_init_wakeup to true
> in its chip init logic, for all boards).
> And likewise the header comment at top of file,
> which talks about how wiring RTC_WAKE_INT is a
> board-specific issue.  Which is true for OMAP1,
> but not for L138 ...

Okay.

>
> And to be paired with a patch which makes the L138
> boards behave right... maybe that chip init code
> already does that, if so mention that in the patch
> comments so we're not left guessing.

Okay. That code is already in Kevin's tree. I will refer
to that in patch description.

>
> I'll be glad to see this driver finally serving as
> a wakeup event source.  I don't recall seeing OMAP1
> boards wired so that it could do that; else this
> would have issued those events a *long* time ago!

Yes, and it was fun seeing the rtc wake-up the OMAP-L138
from suspend.

Thanks,
Sekhar
David Brownell - Jan. 29, 2010, 6:43 p.m.
On Friday 29 January 2010, Nori, Sekhar wrote:
> 
> Hi Dave,
> 
> Thanks for the review and apologize for the late response.
> I was out for a while.

Heh, my mailboxes are way overloaded ... I'm starting to
catch up, be glad your note wasn't too deeply buried!


> > So I think maybe this needs changing.  Minimally ...
> 
> Okay.
> 
> ...
> > I'll be glad to see this driver finally serving as
> > a wakeup event source.  I don't recall seeing OMAP1
> > boards wired so that it could do that; else this
> > would have issued those events a *long* time ago!
> 
> Yes, and it was fun seeing the rtc wake-up the OMAP-L138
> from suspend.

Yeah, that's always a good milestone.  RTC wakeup *SHOULD* be
something almost every system can do, from suspend states.

But it can get messy.  Fortunately OMAP doesn't have to cope
with buggy ACPI BIOS code, or broken-by-design hardware.  ;)

- Dave

Patch

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 0587d53..ac9d55b 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -400,16 +400,11 @@  static int __init omap_rtc_probe(struct platform_device *pdev)
 
 	/* BOARD-SPECIFIC CUSTOMIZATION CAN GO HERE:
 	 *
-	 *  - Boards wired so that RTC_WAKE_INT does something, and muxed
-	 *    right (W13_1610_RTC_WAKE_INT is the default after chip reset),
-	 *    should initialize the device wakeup flag appropriately.
-	 *
 	 *  - Boards wired so RTC_ON_nOFF is used as the reset signal,
 	 *    rather than nPWRON_RESET, should forcibly enable split
 	 *    power mode.  (Some chip errata report that RTC_CTRL_SPLIT
 	 *    is write-only, and always reads as zero...)
 	 */
-	device_init_wakeup(&pdev->dev, 0);
 
 	if (new_ctrl & (u8) OMAP_RTC_CTRL_SPLIT)
 		pr_info("%s: split power mode\n", pdev->name);