Patchwork [v2] rtc: omap: let device wakeup capability be configured from chip init logic

login
register
mail settings
Submitter Sekhar Nori
Date March 7, 2010, 12:35 p.m.
Message ID <1267965354-16805-1-git-send-email-nsekhar@ti.com>
Download mbox | patch
Permalink /patch/47089/
State New
Headers show

Comments

Sekhar Nori - March 7, 2010, 12:35 p.m.
The rtc-omap driver currently hardcodes the RTC wakeup capability
to be "not capable". While this seems to be true for existing OMAP1
boards which are not wired for this, the DA850/OMAP-L138 SoC, the
RTC can always be wake up source from its "deep sleep" mode.

This patch lets the wakeup capability to be set from platform data and
does not override the setting from the driver. For DA850/OMAP-L138, this
is done from arch/arm/mach-davinci/devices-da8xx.c:da8xx_register_rtc()

Note that this patch does not change the behavior on any existing OMAP1
board since the platform device registration sets the wakeup capability
to 0 by default.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---

Dave, would you please ack this patch if you are satisfied?

 drivers/rtc/rtc-omap.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
Sekhar Nori - May 20, 2010, 8:43 a.m.
Hi Dave,

On Sun, Mar 07, 2010 at 18:05:54, Nori, Sekhar wrote:
> The rtc-omap driver currently hardcodes the RTC wakeup capability
> to be "not capable". While this seems to be true for existing OMAP1
> boards which are not wired for this, the DA850/OMAP-L138 SoC, the
> RTC can always be wake up source from its "deep sleep" mode.
>
> This patch lets the wakeup capability to be set from platform data and
> does not override the setting from the driver. For DA850/OMAP-L138, this
> is done from arch/arm/mach-davinci/devices-da8xx.c:da8xx_register_rtc()
>
> Note that this patch does not change the behavior on any existing OMAP1
> board since the platform device registration sets the wakeup capability
> to 0 by default.
>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>
> Dave, would you please ack this patch if you are satisfied?

Can you please ack this patch if it looks good (or suggest
any other changes you would like to see)?

Thanks,
Sekhar

>
>  drivers/rtc/rtc-omap.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> index 64d9727..73377b0 100644
> --- a/drivers/rtc/rtc-omap.c
> +++ b/drivers/rtc/rtc-omap.c
> @@ -34,7 +34,8 @@
>   * Board-specific wiring options include using split power mode with
>   * RTC_OFF_NOFF used as the reset signal (so the RTC won't be reset),
>   * and wiring RTC_WAKE_INT (so the RTC alarm can wake the system from
> - * low power modes).  See the BOARD-SPECIFIC CUSTOMIZATION comment.
> + * low power modes) for OMAP1 boards (OMAP-L138 has this built into
> + * the SoC). See the BOARD-SPECIFIC CUSTOMIZATION comment.
>   */
>
>  #define OMAP_RTC_BASE                        0xfffb4800
> @@ -401,16 +402,17 @@ 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.
> +      *  - Device wake-up capability setting should come through chip
> +      *    init logic. OMAP1 boards should initialize the "wakeup capable"
> +      *    flag in the platform device if the board is wired right for
> +      *    being woken up by RTC alarm. For OMAP-L138, this capability
> +      *    is built into the SoC by the "Deep Sleep" capability.
>        *
>        *  - 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);
> --
> 1.6.2.4
>
>
David Brownell - May 20, 2010, 4:51 p.m.
> On Sun, Mar 07, 2010 at 18:05:54, Nori, Sekhar wrote:
> > The rtc-omap driver currently hardcodes the RTC wakeup
> capability
> > to be "not capable". While this seems to be true for
> existing OMAP1
> > boards which are not wired for this, the
> DA850/OMAP-L138 SoC, the
> > RTC can always be wake up source from its "deep sleep"
> mode.

Good.  Linux has work to do yet getting wake events to behave,
and I keep thinking that RTC support is the best place for such
work to start ... (since most RTCs seem to issue wake events,
even on ACPI/PC hardware).

So having RTCs do this right is forward motion, and will help
(I'd hope!) get other wake event sources working too.


That driver, as you know, started with OMAP1 where few boards were
actually wired so the RTC would wake.  Which is why it ignored the
wake events ... :(



> >
> > This patch lets the wakeup capability to be set from
> platform data and
> > does not override the setting from the driver. For
> DA850/OMAP-L138, this
> > is done from
> arch/arm/mach-davinci/devices-da8xx.c:da8xx_register_rtc()
> >
> > Note that this patch does not change the behavior on
> any existing OMAP1
> > board since the platform device registration sets the
> wakeup capability
> > to 0 by default.
> > Dave, would you please ack this patch if you are
> satisfied?

I don't see how it could work, since it always sets the
wake capability to the default of "can't wake"...  Even if
the board *can* wake...

The model I've worked with is that for devices which can issue
wake events, the board init code sets that flag, and the driver
responds appropriately.  Presumably that could work here too...


> Can you please ack this patch if it looks good (or suggest
> any other changes you would like to see)?
> 
>    device_init_wakeup(&pdev->dev, 0);

Having that line seems to defeat the purpose...
Sekhar Nori - May 24, 2010, 6:06 a.m.
Hi Dave,

On Thu, May 20, 2010 at 22:21:59, David Brownell wrote:
>
> > > This patch lets the wakeup capability to be set from
> > platform data and
> > > does not override the setting from the driver. For
> > DA850/OMAP-L138, this
> > > is done from
> > arch/arm/mach-davinci/devices-da8xx.c:da8xx_register_rtc()
> > >
> > > Note that this patch does not change the behavior on
> > any existing OMAP1
> > > board since the platform device registration sets the
> > wakeup capability
> > > to 0 by default.

Just to be clear, I was actually referring to the fact
that the device_initialize() in drivers/base/core.c is
already clearing the 'can wake-up' flag to 0, so there
should be no need to do so again in the driver.

> > > Dave, would you please ack this patch if you are
> > satisfied?
>
> I don't see how it could work, since it always sets the
> wake capability to the default of "can't wake"...  Even if
> the board *can* wake...

Are you referring to da8xx_register_rtc() here? It sets the
'can wake' flag to true.

        ret = platform_device_register(&da8xx_rtc_device);
        if (!ret)
                /* Atleast on DA850, RTC is a wakeup source */
                device_init_wakeup(&da8xx_rtc_device.dev, true);

>
> The model I've worked with is that for devices which can issue
> wake events, the board init code sets that flag, and the driver
> responds appropriately.  Presumably that could work here too...

Yes, except that the response to the wakeup event is really specific
to DaVinci/DA850 and is handled in arch/arm/mach-davinci/sleep.S, not
in the RTC driver.

>
>
> > Can you please ack this patch if it looks good (or suggest
> > any other changes you would like to see)?
> >
> >    device_init_wakeup(&pdev->dev, 0);
>
> Having that line seems to defeat the purpose...

Agreed, and the patch removes it for that reason.

Thanks,
Sekhar
Sekhar Nori - June 17, 2010, 4:46 a.m.
Hi Dave,

Any thoughts on my responses below? If you are
satisfied, would you please Ack my patch?

Thanks,
Sekhar

On Mon, May 24, 2010 at 11:36:45, Nori, Sekhar wrote:
> Hi Dave,
>
> On Thu, May 20, 2010 at 22:21:59, David Brownell wrote:
> >
> > > > This patch lets the wakeup capability to be set from
> > > platform data and
> > > > does not override the setting from the driver. For
> > > DA850/OMAP-L138, this
> > > > is done from
> > > arch/arm/mach-davinci/devices-da8xx.c:da8xx_register_rtc()
> > > >
> > > > Note that this patch does not change the behavior on
> > > any existing OMAP1
> > > > board since the platform device registration sets the
> > > wakeup capability
> > > > to 0 by default.
>
> Just to be clear, I was actually referring to the fact
> that the device_initialize() in drivers/base/core.c is
> already clearing the 'can wake-up' flag to 0, so there
> should be no need to do so again in the driver.
>
> > > > Dave, would you please ack this patch if you are
> > > satisfied?
> >
> > I don't see how it could work, since it always sets the
> > wake capability to the default of "can't wake"...  Even if
> > the board *can* wake...
>
> Are you referring to da8xx_register_rtc() here? It sets the
> 'can wake' flag to true.
>
>         ret = platform_device_register(&da8xx_rtc_device);
>         if (!ret)
>                 /* Atleast on DA850, RTC is a wakeup source */
>                 device_init_wakeup(&da8xx_rtc_device.dev, true);
>
> >
> > The model I've worked with is that for devices which can issue
> > wake events, the board init code sets that flag, and the driver
> > responds appropriately.  Presumably that could work here too...
>
> Yes, except that the response to the wakeup event is really specific
> to DaVinci/DA850 and is handled in arch/arm/mach-davinci/sleep.S, not
> in the RTC driver.
>
> >
> >
> > > Can you please ack this patch if it looks good (or suggest
> > > any other changes you would like to see)?
> > >
> > >    device_init_wakeup(&pdev->dev, 0);
> >
> > Having that line seems to defeat the purpose...
>
> Agreed, and the patch removes it for that reason.
>
> Thanks,
> Sekhar
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
>
David Brownell - July 18, 2010, 8:52 p.m.
--- On Wed, 6/16/10, Nori, Sekhar <nsekhar@ti.com> wrote:
<rtc-linux@googlegroups.com>
> Date: Wednesday, June 16, 2010, 9:46 PM
> Hi Dave,
> 
> Any thoughts on my responses below? If you are
> satisfied, would you please Ack my patch?

I don't have time to properly review it.  If it
works, go for it ... bugs can be fixed later, and
the principle behind the patch is fine.
Sekhar Nori - July 22, 2010, 3:40 p.m.
Hi Alessandro,

On Mon, Jul 19, 2010 at 02:22:13, David Brownell wrote:
>
>
> --- On Wed, 6/16/10, Nori, Sekhar <nsekhar@ti.com> wrote:
> <rtc-linux@googlegroups.com>
> > Date: Wednesday, June 16, 2010, 9:46 PM
> > Hi Dave,
> >
> > Any thoughts on my responses below? If you are
> > satisfied, would you please Ack my patch?
>
> I don't have time to properly review it.  If it
> works, go for it ... bugs can be fixed later, and
> the principle behind the patch is fine.
>

Since Dave is OK with merging this, would you please
queue this for 2.6.36?

Thanks,
Sekhar
Wan ZongShun - July 23, 2010, 9:46 a.m.
2010/7/22 Nori, Sekhar <nsekhar@ti.com>:
> Hi Alessandro,
>
> On Mon, Jul 19, 2010 at 02:22:13, David Brownell wrote:
>>
>>
>> --- On Wed, 6/16/10, Nori, Sekhar <nsekhar@ti.com> wrote:
>> <rtc-linux@googlegroups.com>
>> > Date: Wednesday, June 16, 2010, 9:46 PM
>> > Hi Dave,
>> >
>> > Any thoughts on my responses below? If you are
>> > satisfied, would you please Ack my patch?
>>
>> I don't have time to properly review it.  If it
>> works, go for it ... bugs can be fixed later, and
>> the principle behind the patch is fine.
>>
>
> Since Dave is OK with merging this, would you please
> queue this for 2.6.36?

Alessandro is so busy too, you can get merging help from Andrew.

>
> Thanks,
> Sekhar
>
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.
Sekhar Nori - Aug. 6, 2010, 5:05 a.m.
Hi Andrew,

On Fri, Jul 23, 2010 at 15:16:19, Wan ZongShun wrote:
> 2010/7/22 Nori, Sekhar <nsekhar@ti.com>:
> > Hi Alessandro,
> >
> > On Mon, Jul 19, 2010 at 02:22:13, David Brownell wrote:
> >>
> >>
> >> --- On Wed, 6/16/10, Nori, Sekhar <nsekhar@ti.com> wrote:
> >> <rtc-linux@googlegroups.com>
> >> > Date: Wednesday, June 16, 2010, 9:46 PM
> >> > Hi Dave,
> >> >
> >> > Any thoughts on my responses below? If you are
> >> > satisfied, would you please Ack my patch?
> >>
> >> I don't have time to properly review it.  If it
> >> works, go for it ... bugs can be fixed later, and
> >> the principle behind the patch is fine.
> >>
> >
> > Since Dave is OK with merging this, would you please
> > queue this for 2.6.36?
>
> Alessandro is so busy too, you can get merging help from Andrew.

Can you please help merge this patch? Here is the patch
in question:

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

I can resend it to the list if that’s more convenient.

Thanks,
Sekhar

Patch

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index 64d9727..73377b0 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -34,7 +34,8 @@ 
  * Board-specific wiring options include using split power mode with
  * RTC_OFF_NOFF used as the reset signal (so the RTC won't be reset),
  * and wiring RTC_WAKE_INT (so the RTC alarm can wake the system from
- * low power modes).  See the BOARD-SPECIFIC CUSTOMIZATION comment.
+ * low power modes) for OMAP1 boards (OMAP-L138 has this built into
+ * the SoC). See the BOARD-SPECIFIC CUSTOMIZATION comment.
  */
 
 #define OMAP_RTC_BASE			0xfffb4800
@@ -401,16 +402,17 @@  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.
+	 *  - Device wake-up capability setting should come through chip
+	 *    init logic. OMAP1 boards should initialize the "wakeup capable"
+	 *    flag in the platform device if the board is wired right for
+	 *    being woken up by RTC alarm. For OMAP-L138, this capability
+	 *    is built into the SoC by the "Deep Sleep" capability.
 	 *
 	 *  - 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);