RTC: RK808: Work around hardware bug on November 31st

In Fuzhou, China, the month of November seems to be having 31 days.
That's nice and all (I'm sure you can get a lot more done in a year that
way), but back here in other parts of the world we are not so lucky.
Therefore, if we read that date from the RTC we should correct it to
December 1st.

This is not a full workaround. Since the RTC actually ticks all the way
through that imaginary day, there's no easy way to detect and correct
this issue if the device was offline the whole time and allowed it to
tick through to December 1st on the Rockchip calendar (which would be
December 2nd on the Gregorian one). Those edge cases can only really be
solved by regularly syncing to an external time source (e.g. NTP).

Signed-off-by: Julius Werner <jwerner@chromium.org>
Reviewed-by: Chris Zhong <zyw@rock-chips.com>
---
 drivers/rtc/rtc-rk808.c | 93 ++++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 43 deletions(-)

Hi,

I would have liked to be in copy of that mail. Maybe you used
get_maintainers on an old tree?

On 02/12/2015 at 17:53:04 -0800, Julius Werner wrote :
> In Fuzhou, China, the month of November seems to be having 31 days.
> That's nice and all (I'm sure you can get a lot more done in a year that
> way), but back here in other parts of the world we are not so lucky.
> Therefore, if we read that date from the RTC we should correct it to
> December 1st.
> 

Wow, nice one...

I hope reading the time properly fails thanks to the rtc_valid_tm(tm) in
__rtc_read_time().

> This is not a full workaround. Since the RTC actually ticks all the way
> through that imaginary day, there's no easy way to detect and correct
> this issue if the device was offline the whole time and allowed it to
> tick through to December 1st on the Rockchip calendar (which would be
> December 2nd on the Gregorian one). Those edge cases can only really be
> solved by regularly syncing to an external time source (e.g. NTP).
> 

It will also happen if nobody reads the RTC time for that day (highly
improbable in a default configuration). Do you need that patch for 4.4
or can it wait 4.5?


> Signed-off-by: Julius Werner <jwerner@chromium.org>
> Reviewed-by: Chris Zhong <zyw@rock-chips.com>
> ---
>  drivers/rtc/rtc-rk808.c | 93 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 50 insertions(+), 43 deletions(-)
>

> I would have liked to be in copy of that mail. Maybe you used
> get_maintainers on an old tree?

Oops, yes, sorry, I ran it on a 3.14 backport and then just added
people I found in older patches for that file.

> I hope reading the time properly fails thanks to the rtc_valid_tm(tm) in
> __rtc_read_time().

It should, yeah. I haven't tested it (beyond compiling) on master, and
our version doesn't have that code yet, so I can't say for sure.

> It will also happen if nobody reads the RTC time for that day (highly
> improbable in a default configuration). Do you need that patch for 4.4
> or can it wait 4.5?

True. I don't really need the patch to land in a particular release
since we maintain our own tree with backports. It would probably still
be nice if you can get it into the 4.4 LTS to reduce the chance this
will hit anyone again next year.

Julius,

On Wed, Dec 2, 2015 at 5:53 PM, Julius Werner <jwerner@chromium.org> wrote:
> In Fuzhou, China, the month of November seems to be having 31 days.
> That's nice and all (I'm sure you can get a lot more done in a year that
> way), but back here in other parts of the world we are not so lucky.
> Therefore, if we read that date from the RTC we should correct it to
> December 1st.
>
> This is not a full workaround. Since the RTC actually ticks all the way
> through that imaginary day, there's no easy way to detect and correct
> this issue if the device was offline the whole time and allowed it to
> tick through to December 1st on the Rockchip calendar (which would be
> December 2nd on the Gregorian one). Those edge cases can only really be
> solved by regularly syncing to an external time source (e.g. NTP).
>
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> Reviewed-by: Chris Zhong <zyw@rock-chips.com>
> ---
>  drivers/rtc/rtc-rk808.c | 93 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 50 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/rtc/rtc-rk808.c b/drivers/rtc/rtc-rk808.c
> index 91ca0bc..b605b35 100644
> --- a/drivers/rtc/rtc-rk808.c
> +++ b/drivers/rtc/rtc-rk808.c
> @@ -56,6 +56,50 @@ struct rk808_rtc {
>         int irq;
>  };
>
> +/* Set current time and date in RTC */
> +static int rk808_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +       struct rk808_rtc *rk808_rtc = dev_get_drvdata(dev);
> +       struct rk808 *rk808 = rk808_rtc->rk808;
> +       u8 rtc_data[NUM_TIME_REGS];
> +       int ret;
> +
> +       rtc_data[0] = bin2bcd(tm->tm_sec);
> +       rtc_data[1] = bin2bcd(tm->tm_min);
> +       rtc_data[2] = bin2bcd(tm->tm_hour);
> +       rtc_data[3] = bin2bcd(tm->tm_mday);
> +       rtc_data[4] = bin2bcd(tm->tm_mon + 1);
> +       rtc_data[5] = bin2bcd(tm->tm_year - 100);
> +       rtc_data[6] = bin2bcd(tm->tm_wday);
> +       dev_dbg(dev, "set RTC date/time %4d-%02d-%02d(%d) %02d:%02d:%02d\n",
> +               1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
> +               tm->tm_wday, tm->tm_hour , tm->tm_min, tm->tm_sec);
> +
> +       /* Stop RTC while updating the RTC registers */
> +       ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG,
> +                                BIT_RTC_CTRL_REG_STOP_RTC_M,
> +                                BIT_RTC_CTRL_REG_STOP_RTC_M);
> +       if (ret) {
> +               dev_err(dev, "Failed to update RTC control: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = regmap_bulk_write(rk808->regmap, RK808_SECONDS_REG,
> +                               rtc_data, NUM_TIME_REGS);
> +       if (ret) {
> +               dev_err(dev, "Failed to bull write rtc_data: %d\n", ret);
> +               return ret;
> +       }
> +       /* Start RTC again */
> +       ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG,
> +                                BIT_RTC_CTRL_REG_STOP_RTC_M, 0);
> +       if (ret) {
> +               dev_err(dev, "Failed to update RTC control: %d\n", ret);
> +               return ret;
> +       }
> +       return 0;
> +}
> +
>  /* Read current time and date in RTC */
>  static int rk808_rtc_readtime(struct device *dev, struct rtc_time *tm)
>  {
> @@ -105,51 +149,14 @@ static int rk808_rtc_readtime(struct device *dev, struct rtc_time *tm)
>                 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
>                 tm->tm_wday, tm->tm_hour , tm->tm_min, tm->tm_sec);
>
> -       return ret;
> -}
> -
> -/* Set current time and date in RTC */
> -static int rk808_rtc_set_time(struct device *dev, struct rtc_time *tm)
> -{
> -       struct rk808_rtc *rk808_rtc = dev_get_drvdata(dev);
> -       struct rk808 *rk808 = rk808_rtc->rk808;
> -       u8 rtc_data[NUM_TIME_REGS];
> -       int ret;
> -
> -       rtc_data[0] = bin2bcd(tm->tm_sec);
> -       rtc_data[1] = bin2bcd(tm->tm_min);
> -       rtc_data[2] = bin2bcd(tm->tm_hour);
> -       rtc_data[3] = bin2bcd(tm->tm_mday);
> -       rtc_data[4] = bin2bcd(tm->tm_mon + 1);
> -       rtc_data[5] = bin2bcd(tm->tm_year - 100);
> -       rtc_data[6] = bin2bcd(tm->tm_wday);
> -       dev_dbg(dev, "set RTC date/time %4d-%02d-%02d(%d) %02d:%02d:%02d\n",
> -               1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
> -               tm->tm_wday, tm->tm_hour , tm->tm_min, tm->tm_sec);
> -
> -       /* Stop RTC while updating the RTC registers */
> -       ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG,
> -                                BIT_RTC_CTRL_REG_STOP_RTC_M,
> -                                BIT_RTC_CTRL_REG_STOP_RTC_M);
> -       if (ret) {
> -               dev_err(dev, "Failed to update RTC control: %d\n", ret);
> -               return ret;
> +       if (tm->tm_mon == 10 && tm->tm_mday == 31) {
> +               dev_warn(dev, "correcting Nov 31st to Dec 1st (HW bug)\n");
> +               tm->tm_mon = 11;
> +               tm->tm_mday = 1;
> +               rk808_rtc_set_time(dev, tm);

If a device is in S3 for the whole day that the glitch occurs and then
we wake up then we'll end up thinking it's Dec 1st instead of Dec 2nd,
right?  That case _could_ be handled by knowing that the last time we
read the clock it was before 12/1 and that this time it is after
11/30.  Then we add the extra day.  In order to do this, we'd have to
know that we're on hardware with the glitch, which I guess could
either be done with a device tree property or by spending 1 second
probing the device at bootup (that would be a bit of a pain...).

Obviously the trick above wouldn't handle if the clock ticked when the
device was in S5, but I'd imagine that most systems treat the RTC as
slightly questionable on an initial bootup anyway (though I'd imagine
that they rely on it working across S3).

What do you think?  Is that a worthwhile thing to handle too?  It's
pretty common for me to not turn all my devices on every day.

-Doug

> If a device is in S3 for the whole day that the glitch occurs and then
> we wake up then we'll end up thinking it's Dec 1st instead of Dec 2nd,
> right?  That case _could_ be handled by knowing that the last time we
> read the clock it was before 12/1 and that this time it is after
> 11/30.  Then we add the extra day.  In order to do this, we'd have to
> know that we're on hardware with the glitch, which I guess could
> either be done with a device tree property or by spending 1 second
> probing the device at bootup (that would be a bit of a pain...).
>
> Obviously the trick above wouldn't handle if the clock ticked when the
> device was in S5, but I'd imagine that most systems treat the RTC as
> slightly questionable on an initial bootup anyway (though I'd imagine
> that they rely on it working across S3).

True, we could do that. I don't think it makes much sense to
differentiate between S3 and S5 like that, though... the problem can
happen just the same after both, and I don't think there's a practical
difference in how systems treat that (if userspace has ways to
double-check the system time, such as syncing to a network time
source, it should really be doing that after both resume and reboot).
Of course, building a work-around like that for S5 will become more
complicated and requires persistent storage.

For Chromium OS, we're already planning to improve tlsdated such that
I don't think this will be an issue anymore (making it schedule a
resync after resume, not just after reboot, which is a probably a good
idea in general). For other systems that don't have any kind of
network time sync, I think the best solution would be to handle this
completely with a small userspace hook on boot and resume (because you
probably need to access the file system to keep track of the last seen
time anyway, you can do the device identification through
/proc/device-tree just as well, and this avoids putting too much hacky
workaround logic into the kernel).

The other thing that would worry me about this approach is that it
requires perfect identification of the problem, and Rockchip will
hopefully eventually be able to fix this either in RK808 or a
successor chip that might use the same RTC interface (and thus
driver). Detecting it at boot is probably a bad idea because a
crash/brownout at the wrong moment will permanently leave you with a
bad time. I really think fixing this as best as we easily can and
leaving the hard edge-cases to userspace is the best approach here.

Julius,

On Fri, Dec 4, 2015 at 4:25 PM, Julius Werner <jwerner@chromium.org> wrote:
>> If a device is in S3 for the whole day that the glitch occurs and then
>> we wake up then we'll end up thinking it's Dec 1st instead of Dec 2nd,
>> right?  That case _could_ be handled by knowing that the last time we
>> read the clock it was before 12/1 and that this time it is after
>> 11/30.  Then we add the extra day.  In order to do this, we'd have to
>> know that we're on hardware with the glitch, which I guess could
>> either be done with a device tree property or by spending 1 second
>> probing the device at bootup (that would be a bit of a pain...).
>>
>> Obviously the trick above wouldn't handle if the clock ticked when the
>> device was in S5, but I'd imagine that most systems treat the RTC as
>> slightly questionable on an initial bootup anyway (though I'd imagine
>> that they rely on it working across S3).
>
> True, we could do that. I don't think it makes much sense to
> differentiate between S3 and S5 like that, though... the problem can
> happen just the same after both, and I don't think there's a practical
> difference in how systems treat that (if userspace has ways to
> double-check the system time, such as syncing to a network time
> source, it should really be doing that after both resume and reboot).
> Of course, building a work-around like that for S5 will become more
> complicated and requires persistent storage.

Right, the need for persistent storage is what makes S5 hard...


> For Chromium OS, we're already planning to improve tlsdated such that
> I don't think this will be an issue anymore (making it schedule a
> resync after resume, not just after reboot, which is a probably a good
> idea in general). For other systems that don't have any kind of
> network time sync, I think the best solution would be to handle this
> completely with a small userspace hook on boot and resume (because you
> probably need to access the file system to keep track of the last seen
> time anyway, you can do the device identification through
> /proc/device-tree just as well, and this avoids putting too much hacky
> workaround logic into the kernel).

How would such a hook work?  If userspace sees the system suspend on
Nov 30th and sees the system wake up on Dec 1st, how does it know
whether it should adjust?  If it's truly Dec 1st then the kernel will
have adjusted the date from Nov 31st to Dec 1st.  If it's truly Dec
2nd then the kernel will not have adjusted the date and the RTC will
have ticked past Nov 31 and onto Dec 1st.  Userspace can't tell.
Userspace could try to parse "dmesg" and look to see if the kernel
adjusted, but that's ugly.  We could add a sysfs entry, but it seems
pretty hard to imagine that all Linux distros using rk808 will add
this type of hook...


> The other thing that would worry me about this approach is that it
> requires perfect identification of the problem, and Rockchip will
> hopefully eventually be able to fix this either in RK808 or a
> successor chip that might use the same RTC interface (and thus
> driver). Detecting it at boot is probably a bad idea because a
> crash/brownout at the wrong moment will permanently leave you with a
> bad time. I really think fixing this as best as we easily can and
> leaving the hard edge-cases to userspace is the best approach here.

Yes, you're right.  Detecting is a bit scary.

Chris: any chance there's an RK808 revision hiding somewhere in the
i2c register banks that we could rely on?

Adding a device tree hook doesn't seem insane, but you're right that
Rockchip could start producing new revisions of rk808 with this fixed
and all of sudden we'd be adjusting the wrong way.  ...so you're
probably right that this is a bad idea...


So I guess my #1 choice would be to find a revision somewhere in the
rk808 i2c register space.  If that's not there, then I guess you're
patch is probably better than trying to adjust and maybe being wrong
when newer rk808 revisions fix this...

-Doug

> How would such a hook work?  If userspace sees the system suspend on
> Nov 30th and sees the system wake up on Dec 1st, how does it know
> whether it should adjust?  If it's truly Dec 1st then the kernel will
> have adjusted the date from Nov 31st to Dec 1st.  If it's truly Dec
> 2nd then the kernel will not have adjusted the date and the RTC will
> have ticked past Nov 31 and onto Dec 1st.  Userspace can't tell.
> Userspace could try to parse "dmesg" and look to see if the kernel
> adjusted, but that's ugly.

Good point, I didn't think that through far enough. I guess parsing
dmesg would be an option, but a pretty ugly one and it wouldn't be
guaranteed to work if you got an early boot kernel crash after the
correction. So, really, it seems like there's no reliable way to fix
this for S5 (unless we start doing crazy things like writing to disk
from kernel code).

Hi,

On Fri, Dec 4, 2015 at 5:54 PM, Julius Werner <jwerner@chromium.org> wrote:
>> How would such a hook work?  If userspace sees the system suspend on
>> Nov 30th and sees the system wake up on Dec 1st, how does it know
>> whether it should adjust?  If it's truly Dec 1st then the kernel will
>> have adjusted the date from Nov 31st to Dec 1st.  If it's truly Dec
>> 2nd then the kernel will not have adjusted the date and the RTC will
>> have ticked past Nov 31 and onto Dec 1st.  Userspace can't tell.
>> Userspace could try to parse "dmesg" and look to see if the kernel
>> adjusted, but that's ugly.
>
> Good point, I didn't think that through far enough. I guess parsing
> dmesg would be an option, but a pretty ugly one and it wouldn't be
> guaranteed to work if you got an early boot kernel crash after the
> correction. So, really, it seems like there's no reliable way to fix
> this for S5 (unless we start doing crazy things like writing to disk
> from kernel code).

Hmmm, this made me think.  We _do_ have some storage we could use,
depending on how hacky^H^H^H^H^H^H clever we wanted to be.  We've got
the alarm registers in the RTC.  If we set the alarm to something but
then turn the alarm off then we can use that to store information that
will persist in S5 (as long as the RTC is ticking).  What do you
think?  I'd have to think of a scheme, but we could certainly use
alarms that are several years in the future (or the past) as a
sentinel, then use the day/month of the last time the kernel saw the
time....

...and speaking of the alarm, we also need to handle the RTC bug for
setting the alarm.  If you set an alarm for 10 seconds after Nov 30,
you need to set the alarm for Nov 31st or it will actually fire 10
seconds + 1 day later.

Hi,

On Fri, Dec 4, 2015 at 8:02 PM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Fri, Dec 4, 2015 at 5:54 PM, Julius Werner <jwerner@chromium.org> wrote:
>>> How would such a hook work?  If userspace sees the system suspend on
>>> Nov 30th and sees the system wake up on Dec 1st, how does it know
>>> whether it should adjust?  If it's truly Dec 1st then the kernel will
>>> have adjusted the date from Nov 31st to Dec 1st.  If it's truly Dec
>>> 2nd then the kernel will not have adjusted the date and the RTC will
>>> have ticked past Nov 31 and onto Dec 1st.  Userspace can't tell.
>>> Userspace could try to parse "dmesg" and look to see if the kernel
>>> adjusted, but that's ugly.
>>
>> Good point, I didn't think that through far enough. I guess parsing
>> dmesg would be an option, but a pretty ugly one and it wouldn't be
>> guaranteed to work if you got an early boot kernel crash after the
>> correction. So, really, it seems like there's no reliable way to fix
>> this for S5 (unless we start doing crazy things like writing to disk
>> from kernel code).
>
> Hmmm, this made me think.  We _do_ have some storage we could use,
> depending on how hacky^H^H^H^H^H^H clever we wanted to be.  We've got
> the alarm registers in the RTC.  If we set the alarm to something but
> then turn the alarm off then we can use that to store information that
> will persist in S5 (as long as the RTC is ticking).  What do you
> think?  I'd have to think of a scheme, but we could certainly use
> alarms that are several years in the future (or the past) as a
> sentinel, then use the day/month of the last time the kernel saw the
> time....

Actually, it wouldn't even be that terribly hacky  Whenever the alarm
isn't in use then set it to the next Nov 31st.  At boot time, if the
alarm is set to Nov 31st and the current date is >= the alarm time
then you adjust.  At shutdown time always disable the alarm (and set
to Nov 31st).  To handle resume time you might as well just keep the
state somewhere in RAM (see below for why).

Whenever you set the alarm for real then presumably you'll need to
adjust for Nov 31st and presumably you'll wake up and deal with the
alarm, then turn it off.  So if you set the alarm for July 4th then
(presumably) you'll wake up way before Nov 31st.  If you set the alarm
for Dec 25th and it's currently Oct 31st then you'll have to adjust in
the alarm code and you'll really set it for Dec 24th.  As per above,
we're in S3 (presumably) or have some persistent kernel state so we
know to adjust everything when we wake up (even if we wake up for a
non-alarm reason).

You'll still get a failure if you set the alarm and then forcibly go
into S5 without software knowledge, then stay in S5 long enough to
cross over Nov 31st without seeing it (but somehow keep the RTC
state).  ...but come on, that seems so incredibly rare!  :-P


Of course, all this hinges on being able to tell whether we've got the
bug or not so we know whether to adjust.  Assuming that there is no ID
register, we could get someone from Rockchip to agree to change
_something_ in a way that it's visible to us if they ever fix the
bug...


-Doug

> If you set the alarm
> for Dec 25th and it's currently Oct 31st then you'll have to adjust in
> the alarm code and you'll really set it for Dec 24th.  As per above,
> we're in S3 (presumably) or have some persistent kernel state so we
> know to adjust everything when we wake up (even if we wake up for a
> non-alarm reason).

How do you deal with the case where you set an alarm in late December,
but you then power the system on earlier in December by other means? I
think then you couldn't tell if the fix had already been applied?

> You'll still get a failure if you set the alarm and then forcibly go
> into S5 without software knowledge, then stay in S5 long enough to
> cross over Nov 31st without seeing it (but somehow keep the RTC
> state).  ...but come on, that seems so incredibly rare!  :-P

I think you could just set it to "November 31st, disabled" at probe
time (if it isn't already) and keep it like that indefinitely? I think
as long as you don't need to actually use the alarm, that would work
fine.

Still, with the vast majority of practically existing devices with an
RK808 almost constantly connected to some network, I'm not sure if a
huge hack-around is really worth it here. (I guess we could still just
do the S3 thing which is much less complicated, assuming we can
guarantee correct identification.)

Julius,

On Fri, Dec 4, 2015 at 11:17 PM, Julius Werner <jwerner@chromium.org> wrote:
>> If you set the alarm
>> for Dec 25th and it's currently Oct 31st then you'll have to adjust in
>> the alarm code and you'll really set it for Dec 24th.  As per above,
>> we're in S3 (presumably) or have some persistent kernel state so we
>> know to adjust everything when we wake up (even if we wake up for a
>> non-alarm reason).
>
> How do you deal with the case where you set an alarm in late December,
> but you then power the system on earlier in December by other means? I
> think then you couldn't tell if the fix had already been applied?

I was presuming that alarms were never set at power off time unless
power off happened due to an exceptional case.  AKA: normal Linux
shutdown disables all alarms.  If you happened to have an exceptional
shutdown (out of battery?) while an alarm is set then, yes, my
solution fails.  Presumably if the RTC keeps state but the system ran
out of battery, you've got two batteries in your system (something
none of the rk808-based Chromebooks have--we back rk808 state up with
the main battery, not a coin cell).

In Chromebooks you could possibly get a shutdown that Linux didn't
know anything about if you got a forcible reboot (watchdog, crash, or
Refresh-Power) and then you managed to convince the BIOS to shut you
down (you have the dev screen and press power off there).  Again, this
seems pretty darn rare.


>> You'll still get a failure if you set the alarm and then forcibly go
>> into S5 without software knowledge, then stay in S5 long enough to
>> cross over Nov 31st without seeing it (but somehow keep the RTC
>> state).  ...but come on, that seems so incredibly rare!  :-P
>
> I think you could just set it to "November 31st, disabled" at probe
> time (if it isn't already) and keep it like that indefinitely? I think
> as long as you don't need to actually use the alarm, that would work
> fine.

Yup.  In ChromeOS we only use the alarm in suspend stress testing, but
I'd believe that anyone using Android on one of these systems would
use the RTC much more.

We might (?) use the RTC alarm in ChromeOS if we ever support dark
resume etc on rk3288-based devices, right?


> Still, with the vast majority of practically existing devices with an
> RK808 almost constantly connected to some network, I'm not sure if a
> huge hack-around is really worth it here. (I guess we could still just
> do the S3 thing which is much less complicated, assuming we can
> guarantee correct identification.)

I'm pretty certain that we need to handle correcting the alarm.
Setting an alarm for 10 seconds from now and getting the alarm firing
1 day and 10 seconds from now seems like a pretty huge problem, at
least for any system that relies on the RTC alarm a lot.  That pretty
much means that we need some way to identify problematic devices.
...so I think if we have no revision register then we should either
assume all rk808 devices have this problem (and hope and pray that
Rockchip gives us a way to ID things if they ever add a fix) or come
up with some other type of solution (probe one time when the clock is
presumably wrong and store something somewhere in rk808 to indicate
that we've already probed)

Once we have to fix the alarm, handling S3 seems like it will be not
much more work.

I'm OK with not handling S5, but I think with my proposed scheme we
could also handle it if we wanted.


All hacks should be contained in the rk808 driver, which should make
them much less objectionable in general.

-Doug

Hi Doug

RK808 has a shadowed register for saving a "frozen" RTC time.
When we setting "GET_TIME" to 1, the time will save in this shadowed
register. So if we do not set the "GET_TIME", we always get the last time.

read the old time before "get_time", and then read the time again for new
time. If the old time earlier than 12.1 && new time later than 12.1, we 
should
+1 day for the correct rtc time.

On the other hand, we should set the "GET_TIME" after rk808_rtc_set_time,
for restore the time before suspend/shut_down.

regmap_bulk_read(rk808->regmap, RK808_SECONDS_REG,
                               rtc_data, NUM_TIME_REGS);

<....>

   /* Force an update of the shadowed registers right now */
     ret = regmap_update_bits(rk808->regmap, RK808_RTC_CTRL_REG,
                  BIT_RTC_CTRL_REG_RTC_GET_TIME,
                  BIT_RTC_CTRL_REG_RTC_GET_TIME);

regmap_bulk_read(rk808->regmap, RK808_SECONDS_REG,
                               rtc_data, NUM_TIME_REGS);


On 12/06/2015 08:36 AM, Doug Anderson wrote:
> Julius,
>
> On Fri, Dec 4, 2015 at 11:17 PM, Julius Werner <jwerner@chromium.org> wrote:
>>> If you set the alarm
>>> for Dec 25th and it's currently Oct 31st then you'll have to adjust in
>>> the alarm code and you'll really set it for Dec 24th.  As per above,
>>> we're in S3 (presumably) or have some persistent kernel state so we
>>> know to adjust everything when we wake up (even if we wake up for a
>>> non-alarm reason).
>> How do you deal with the case where you set an alarm in late December,
>> but you then power the system on earlier in December by other means? I
>> think then you couldn't tell if the fix had already been applied?
> I was presuming that alarms were never set at power off time unless
> power off happened due to an exceptional case.  AKA: normal Linux
> shutdown disables all alarms.  If you happened to have an exceptional
> shutdown (out of battery?) while an alarm is set then, yes, my
> solution fails.  Presumably if the RTC keeps state but the system ran
> out of battery, you've got two batteries in your system (something
> none of the rk808-based Chromebooks have--we back rk808 state up with
> the main battery, not a coin cell).
>
> In Chromebooks you could possibly get a shutdown that Linux didn't
> know anything about if you got a forcible reboot (watchdog, crash, or
> Refresh-Power) and then you managed to convince the BIOS to shut you
> down (you have the dev screen and press power off there).  Again, this
> seems pretty darn rare.
>
>
>>> You'll still get a failure if you set the alarm and then forcibly go
>>> into S5 without software knowledge, then stay in S5 long enough to
>>> cross over Nov 31st without seeing it (but somehow keep the RTC
>>> state).  ...but come on, that seems so incredibly rare!  :-P
>> I think you could just set it to "November 31st, disabled" at probe
>> time (if it isn't already) and keep it like that indefinitely? I think
>> as long as you don't need to actually use the alarm, that would work
>> fine.
> Yup.  In ChromeOS we only use the alarm in suspend stress testing, but
> I'd believe that anyone using Android on one of these systems would
> use the RTC much more.
>
> We might (?) use the RTC alarm in ChromeOS if we ever support dark
> resume etc on rk3288-based devices, right?
>
>
>> Still, with the vast majority of practically existing devices with an
>> RK808 almost constantly connected to some network, I'm not sure if a
>> huge hack-around is really worth it here. (I guess we could still just
>> do the S3 thing which is much less complicated, assuming we can
>> guarantee correct identification.)
> I'm pretty certain that we need to handle correcting the alarm.
> Setting an alarm for 10 seconds from now and getting the alarm firing
> 1 day and 10 seconds from now seems like a pretty huge problem, at
> least for any system that relies on the RTC alarm a lot.  That pretty
> much means that we need some way to identify problematic devices.
> ...so I think if we have no revision register then we should either
> assume all rk808 devices have this problem (and hope and pray that
> Rockchip gives us a way to ID things if they ever add a fix) or come
> up with some other type of solution (probe one time when the clock is
> presumably wrong and store something somewhere in rk808 to indicate
> that we've already probed)
>
> Once we have to fix the alarm, handling S3 seems like it will be not
> much more work.
>
> I'm OK with not handling S5, but I think with my proposed scheme we
> could also handle it if we wanted.
>
>
> All hacks should be contained in the rk808 driver, which should make
> them much less objectionable in general.
>
> -Doug
>
>
>

Chris,

On Sun, Dec 6, 2015 at 5:33 PM, Chris Zhong <zyw@rock-chips.com> wrote:
> Hi Doug
>
> RK808 has a shadowed register for saving a "frozen" RTC time.
> When we setting "GET_TIME" to 1, the time will save in this shadowed
> register. So if we do not set the "GET_TIME", we always get the last time.
>
> read the old time before "get_time", and then read the time again for new
> time. If the old time earlier than 12.1 && new time later than 12.1, we
> should
> +1 day for the correct rtc time.
>
> On the other hand, we should set the "GET_TIME" after rk808_rtc_set_time,
> for restore the time before suspend/shut_down.

Ah, good idea using the shadow registers.  The whole point of the
shadow registers is to enable atomic read of time, right?  So if the
clock ticks as you are reading 23:59:59 you don't end up reading
23:59:00 or 24:59:59 (you'll get either 23:59:59 or 24:00:00).  So
right, time read will now be:

1. Read GET_TIME.  Confirm it's 1.
2. Read the time.
3. Set GET_TIME to 0.
4. Set GET_TIME to 1.
5. Read the time.

If time from #2 < 11/31 and time from #5 >= 11/31 then we do the
adjust.  If GET_TIME wasn't 1 in step #1 then we won't do any
adjusting unless the time is actually 11/31.

Between steps #4 and #5 we'll need to add a small delay since old code
used to use the setting to 0 as a delay (as commented).

We should presumably always leave GET_TIME as 1 unless we're actively
reading the time for the most reliability.  Also, if we've already
read the time this bootup, we can certainly optimize the above by
skipping #1 and #2.

-Doug

Hi,

On Sun, Dec 6, 2015 at 6:50 PM, Doug Anderson <dianders@chromium.org> wrote:
> Chris,
>
> On Sun, Dec 6, 2015 at 5:33 PM, Chris Zhong <zyw@rock-chips.com> wrote:
>> Hi Doug
>>
>> RK808 has a shadowed register for saving a "frozen" RTC time.
>> When we setting "GET_TIME" to 1, the time will save in this shadowed
>> register. So if we do not set the "GET_TIME", we always get the last time.
>>
>> read the old time before "get_time", and then read the time again for new
>> time. If the old time earlier than 12.1 && new time later than 12.1, we
>> should
>> +1 day for the correct rtc time.
>>
>> On the other hand, we should set the "GET_TIME" after rk808_rtc_set_time,
>> for restore the time before suspend/shut_down.
>
> Ah, good idea using the shadow registers.  The whole point of the
> shadow registers is to enable atomic read of time, right?  So if the
> clock ticks as you are reading 23:59:59 you don't end up reading
> 23:59:00 or 24:59:59 (you'll get either 23:59:59 or 24:00:00).  So
> right, time read will now be:
>
> 1. Read GET_TIME.  Confirm it's 1.
> 2. Read the time.
> 3. Set GET_TIME to 0.
> 4. Set GET_TIME to 1.
> 5. Read the time.
>
> If time from #2 < 11/31 and time from #5 >= 11/31 then we do the
> adjust.  If GET_TIME wasn't 1 in step #1 then we won't do any
> adjusting unless the time is actually 11/31.
>
> Between steps #4 and #5 we'll need to add a small delay since old code
> used to use the setting to 0 as a delay (as commented).
>
> We should presumably always leave GET_TIME as 1 unless we're actively
> reading the time for the most reliability.  Also, if we've already
> read the time this bootup, we can certainly optimize the above by
> skipping #1 and #2.

Oh, but also we still need to know whether to adjust the alarm.  I
think you said that all existing rk808 chips have this bug and that
you'll set a bit (to be determined later) if/when this bug is fixed.
So we still need to assume that all rk808 chips have this bug...

-Doug

On 12/07/2015 10:52 AM, Doug Anderson wrote:
> Hi,
>
> On Sun, Dec 6, 2015 at 6:50 PM, Doug Anderson <dianders@chromium.org> wrote:
>> Chris,
>>
>> On Sun, Dec 6, 2015 at 5:33 PM, Chris Zhong <zyw@rock-chips.com> wrote:
>>> Hi Doug
>>>
>>> RK808 has a shadowed register for saving a "frozen" RTC time.
>>> When we setting "GET_TIME" to 1, the time will save in this shadowed
>>> register. So if we do not set the "GET_TIME", we always get the last time.
>>>
>>> read the old time before "get_time", and then read the time again for new
>>> time. If the old time earlier than 12.1 && new time later than 12.1, we
>>> should
>>> +1 day for the correct rtc time.
>>>
>>> On the other hand, we should set the "GET_TIME" after rk808_rtc_set_time,
>>> for restore the time before suspend/shut_down.
>> Ah, good idea using the shadow registers.  The whole point of the
>> shadow registers is to enable atomic read of time, right?  So if the
>> clock ticks as you are reading 23:59:59 you don't end up reading
>> 23:59:00 or 24:59:59 (you'll get either 23:59:59 or 24:00:00).  So
>> right, time read will now be:
>>
>> 1. Read GET_TIME.  Confirm it's 1.
>> 2. Read the time.
>> 3. Set GET_TIME to 0.
>> 4. Set GET_TIME to 1.
>> 5. Read the time.
>>
>> If time from #2 < 11/31 and time from #5 >= 11/31 then we do the
>> adjust.  If GET_TIME wasn't 1 in step #1 then we won't do any
>> adjusting unless the time is actually 11/31.
>>
>> Between steps #4 and #5 we'll need to add a small delay since old code
>> used to use the setting to 0 as a delay (as commented).
>>
>> We should presumably always leave GET_TIME as 1 unless we're actively
>> reading the time for the most reliability.  Also, if we've already
>> read the time this bootup, we can certainly optimize the above by
>> skipping #1 and #2.
GET_TIME: Rising transition of this register transfers dynamic registers 
into
static shadowed registers.
So only the rising of GET_TIME would update the "static shadowed registers".
We only need ensure that the rising occurs on condition that we want to the
really time.
> Oh, but also we still need to know whether to adjust the alarm.  I
> think you said that all existing rk808 chips have this bug and that
> you'll set a bit (to be determined later) if/when this bug is fixed.
> So we still need to assume that all rk808 chips have this bug...
I think so, all rk808 chips have this bug. And we can read the version 
register
to differentiate the PMICs, once this bug is fixed.
>
> -Doug
>
>
>

> I was presuming that alarms were never set at power off time unless
> power off happened due to an exceptional case.  AKA: normal Linux
> shutdown disables all alarms.

Hmm... maybe I misunderstand how this works. Are alarms never used to
wake from S5? (It doesn't seem to work on my HiSense Chromebook right
now, but maybe that's just an artifact of our board design? I'm pretty
sure I've seen S5-wakes work on other platforms... I doubt that RK808
is intrinsically incapable of that, it just depends on how you hook it
up.)

> RK808 has a shadowed register for saving a "frozen" RTC time.
> When we setting "GET_TIME" to 1, the time will save in this shadowed
> register. So if we do not set the "GET_TIME", we always get the last
> time.
>
> read the old time before "get_time", and then read the time again for new
> time. If the old time earlier than 12.1 && new time later than 12.1, we should
> +1 day for the correct rtc time.

Unfortunately, this doesn't work through S5 if system firmware also
accesses the RTC and transitions GET_TIME on boot (which it does on
Chromebooks). We could fix this for coreboot, but it's probably still
a bad idea to rely on it since Linux should be as firmware-agnostic as
possible.

> I'm pretty certain that we need to handle correcting the alarm.
> Setting an alarm for 10 seconds from now and getting the alarm firing
> 1 day and 10 seconds from now seems like a pretty huge problem, at
> least for any system that relies on the RTC alarm a lot.  That pretty
> much means that we need some way to identify problematic devices.
> ...so I think if we have no revision register then we should either
> assume all rk808 devices have this problem (and hope and pray that
> Rockchip gives us a way to ID things if they ever add a fix) or come
> up with some other type of solution (probe one time when the clock is
> presumably wrong and store something somewhere in rk808 to indicate
> that we've already probed)

> Once we have to fix the alarm, handling S3 seems like it will be not
> much more work.

Agreed, scheduling alarms in the future is also an important point. So
I guess I'll update the patch to fix alarm scheduling and S3 as well,
and we'll just ignore S5 as infeasible?

[+Alexandre... sorry, didn't notice that you somehow fell off the
thread again, but I presume you get rtc-linux anyway]

> Agreed, scheduling alarms in the future is also an important point. So
> I guess I'll update the patch to fix alarm scheduling and S3 as well,
> and we'll just ignore S5 as infeasible?

Found another edge case trying to implement this: if you set an alarm
on Nov 25th for Dec 5th, the code will have to adjust it to Dec 4th to
account for the extra day. But if you then power off (or brownout) to
S5 and don't power it on again until December, you have no way of
knowing whether the already set alarm timestamp is on the Rockchip or
the Gregorian calendar. You may or may not sync your current time back
through an external source, but you have no way of knowing what you
should do with the alarm.

Other than that my current idea is roughly:

- if the current time gets written, always assume the new date is correct
- store a "last known timestamp" in memory on boot and on every write
- if the current time gets read, adjust it forward by the amount of
Nov 31sts since the stored timestamp (this adjustment itself
constitutes a write)
- if an alarm is written, adjust the timestamp written to hardware
backward by the amount of Nov 31sts between the stored timestamp and
the alarm time
- if an alarm is read, adjust the returned result forward by the
amount of Nov 31sts between the stored timestamp and the alarm time
- if the current time gets written, read the alarm time before the
write and write it back afterwards (the last known timestamp will
change as a result of writing the current time, meaning the newly
written hardware alarm time may be different from the old one)

Julius,

On Mon, Dec 7, 2015 at 12:28 PM, Julius Werner <jwerner@chromium.org> wrote:
>> I was presuming that alarms were never set at power off time unless
>> power off happened due to an exceptional case.  AKA: normal Linux
>> shutdown disables all alarms.
>
> Hmm... maybe I misunderstand how this works. Are alarms never used to
> wake from S5? (It doesn't seem to work on my HiSense Chromebook right
> now, but maybe that's just an artifact of our board design? I'm pretty
> sure I've seen S5-wakes work on other platforms... I doubt that RK808
> is intrinsically incapable of that, it just depends on how you hook it
> up.)

Good point.  The hardware out to be capable of it.


>> RK808 has a shadowed register for saving a "frozen" RTC time.
>> When we setting "GET_TIME" to 1, the time will save in this shadowed
>> register. So if we do not set the "GET_TIME", we always get the last
>> time.
>>
>> read the old time before "get_time", and then read the time again for new
>> time. If the old time earlier than 12.1 && new time later than 12.1, we should
>> +1 day for the correct rtc time.
>
> Unfortunately, this doesn't work through S5 if system firmware also
> accesses the RTC and transitions GET_TIME on boot (which it does on
> Chromebooks). We could fix this for coreboot, but it's probably still
> a bad idea to rely on it since Linux should be as firmware-agnostic as
> possible.

Dang.  Who wrote that firmware, anyway?  :-P

Technically you could still implement this and if firmware happens to
read the RTC (and doesn't correct things) then we won't be able to
correct things.  ...but certainly if you read the old time and then
the new time and the old time was < 11/31 and the new time was >=
11/31 you know you need to correct.

I'd say that there's a good chance that other firmware (UBoot) doesn't
actually read the RTC anyway.  Why would it?  We only do it for even
log, right?


>> I'm pretty certain that we need to handle correcting the alarm.
>> Setting an alarm for 10 seconds from now and getting the alarm firing
>> 1 day and 10 seconds from now seems like a pretty huge problem, at
>> least for any system that relies on the RTC alarm a lot.  That pretty
>> much means that we need some way to identify problematic devices.
>> ...so I think if we have no revision register then we should either
>> assume all rk808 devices have this problem (and hope and pray that
>> Rockchip gives us a way to ID things if they ever add a fix) or come
>> up with some other type of solution (probe one time when the clock is
>> presumably wrong and store something somewhere in rk808 to indicate
>> that we've already probed)
>
>> Once we have to fix the alarm, handling S3 seems like it will be not
>> much more work.
>
> Agreed, scheduling alarms in the future is also an important point. So
> I guess I'll update the patch to fix alarm scheduling and S3 as well,
> and we'll just ignore S5 as infeasible?

> Technically you could still implement this and if firmware happens to
> read the RTC (and doesn't correct things) then we won't be able to
> correct things.  ...but certainly if you read the old time and then
> the new time and the old time was < 11/31 and the new time was >=
> 11/31 you know you need to correct.
>
> I'd say that there's a good chance that other firmware (UBoot) doesn't
> actually read the RTC anyway.  Why would it?  We only do it for even
> log, right?

Hah! Sounds like you assume U-Boot was doing the things it does with
comprehensible reasoning. From my experience, that's a dangerous wager
(not that I'm biased or anything... ;) ). FWIW, they seem to be having
a huge (175kloc, of only the finest copied kernel code from 10+ years
ago, I presume) repository of RTC drivers, a separate command line
command to read/write to it, and an SNTP client.

Still, you're right that adding the GET_TIME check wouldn't hurt... at
worst it just does nothing. I'll try it out.

> Still, you're right that adding the GET_TIME check wouldn't hurt... at
> worst it just does nothing. I'll try it out.

Hmm... so it basically works (when you hack the RTC read out of the
firmware), but only on reboot for some reason (which makes it way less
useful). After a full power off I'm reading only zeroes out until I
transition GET_TIME again. My guess would be that these shadow
registers are not in the RTC power well... I've talked to Alex and he
said that we disable the RK808's main power source in S5, but that
devices without an EC (e.g. phones) would likely leave it on. So I
guess it's still a good extra workaround to keep in the code, but it
will never help on Chromebooks (and there's no point in hacking up our
firmware for it).