lkml.org 
[lkml]   [2018]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v4 2/4] rtc: OMAP: Add support for rtc-only mode
On Thu, Jul 12, 2018 at 10:37:38AM +0530, Keerthy wrote:
> Prepare rtc driver for rtc-only with DDR in self-refresh mode.
> omap_rtc_power_off now should cater to two features:
>
> 1) RTC plus DDR in self-refresh is power a saving mode where in the
> entire system including the different voltage rails from PMIC are
> shutdown except the ones feeding on to RTC and DDR. DDR is kept in
> self-refresh hence the contents are preserved. RTC ALARM2 is connected
> to PMIC_EN line once we the ALARM2 is triggered we enter the mode with
> DDR in self-refresh and RTC Ticking. After a predetermined time an RTC
> ALARM1 triggers waking up the system[1]. The control goes to bootloader.
> The bootloader then checks RTC scratchpad registers to confirm it was an
> rtc_only wakeup and follows a different path, configure bare minimal
> clocks for ddr and then jumps to the resume address in another RTC
> scratchpad registers and transfers the control to Kernel. Kernel then
> restores the saved context. omap_rtc_power_off_program does the ALARM2
> programming part.
>
> [1] http://www.ti.com/lit/ug/spruhl7h/spruhl7h.pdf Page 2884
>
> 2) Power-off: This is usual poweroff mode. omap_rtc_power_off calls the
> above omap_rtc_power_off_program function and in addition to that
> programs the OMAP_RTC_PMIC_REG for any external wake ups for PMIC like
> the pushbutton and shuts off the PMIC.
>
> Hence the split in omap_rtc_power_off.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
> drivers/rtc/rtc-omap.c | 53 ++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> index 88da927..cb19ece 100644
> --- a/drivers/rtc/rtc-omap.c
> +++ b/drivers/rtc/rtc-omap.c
> @@ -415,21 +415,12 @@ static int omap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>
> static struct omap_rtc *omap_rtc_power_off_rtc;
>
> -/*
> - * omap_rtc_poweroff: RTC-controlled power off
> - *
> - * The RTC can be used to control an external PMIC via the pmic_power_en pin,
> - * which can be configured to transition to OFF on ALARM2 events.
> - *
> - * Notes:
> - * The two-second alarm offset is the shortest offset possible as the alarm
> - * registers must be set before the next timer update and the offset
> - * calculation is too heavy for everything to be done within a single access
> - * period (~15 us).
> - *
> - * Called with local interrupts disabled.
> +/**
> + * omap_rtc_power_off_program: Set the pmic power off sequence. The RTC
> + * generates pmic_pwr_enable control, which can be used to control an external
> + * PMIC.

Since this is kerneldoc, you need an empty line after "sequence." above.

You should also add a comment that this function depends on local
interrupts being disabled when called (for the wait_not_busy handling);
and make sure you follow that in subsequent patches.

> */
> -static void omap_rtc_power_off(void)
> +static int omap_rtc_power_off_program(struct device *dev)

dev is never used in this function.

> {
> struct omap_rtc *rtc = omap_rtc_power_off_rtc;
> struct rtc_time tm;
> @@ -456,7 +447,7 @@ static void omap_rtc_power_off(void)
> if (tm2bcd(&tm) < 0) {
> dev_err(&rtc->rtc->dev, "power off failed\n");
> rtc->type->lock(rtc);
> - return;
> + return -EINVAL;
> }
>
> rtc_wait_not_busy(rtc);
> @@ -481,6 +472,38 @@ static void omap_rtc_power_off(void)
> goto again;
> rtc->type->lock(rtc);
>
> + return 0;
> +}
> +
> +/*
> + * omap_rtc_poweroff: RTC-controlled power off
> + *
> + * The RTC can be used to control an external PMIC via the pmic_power_en pin,
> + * which can be configured to transition to OFF on ALARM2 events.
> + *
> + * Notes:
> + * The one-second alarm offset is the shortest offset possible as the alarm
> + * registers must be set before the next timer update and the offset
> + * calculation is too heavy for everything to be done within a single access
> + * period (~15 us).

This note really doesn't make much sense anymore, and should have been
updated as part of the previous patch.

> + *
> + * Called with local interrupts disabled.
> + */
> +static void omap_rtc_power_off(void)
> +{
> + struct rtc_device *rtc = omap_rtc_power_off_rtc->rtc;

Use struct omap_rtc here as before.

> + u32 val;
> +
> + omap_rtc_power_off_program(rtc->dev.parent);
> +
> + /* Set PMIC power enable and EXT_WAKEUP in case PB power on is used */
> + omap_rtc_power_off_rtc->type->unlock(omap_rtc_power_off_rtc);
> + val = rtc_readl(omap_rtc_power_off_rtc, OMAP_RTC_PMIC_REG);
> + val |= OMAP_RTC_PMIC_POWER_EN_EN | OMAP_RTC_PMIC_EXT_WKUP_POL(0) |
> + OMAP_RTC_PMIC_EXT_WKUP_EN(0);
> + rtc_writel(omap_rtc_power_off_rtc, OMAP_RTC_PMIC_REG, val);
> + omap_rtc_power_off_rtc->type->lock(omap_rtc_power_off_rtc);

What is all this, and why is it here?

Surely fiddling with this register after you've set the alarm to trigger
isn't the right thing to do.
> +
> /*
> * Wait for alarm to trigger (within two seconds) and external PMIC to
> * power off the system. Add a 500 ms margin for external latencies

Johan

\
 
 \ /
  Last update: 2018-07-19 12:30    [W:0.084 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site