Re: [PATCH 2/2] rtc: zynqmp: fix invalid read_time before 1 second

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 28/11/2019 11:04:19-0500, Jean-Francois Dagenais wrote:
> Hi Michal, all,
> 
> > On Nov 28, 2019, at 03:19, Michal Simek <michal.simek@xilinx.com> wrote:
> > 
> >> in CURRENT_TIME and proceeds to use SET_TIME_READ (RTC_SET_TM_RD in the
> >> code). This register contains garbage at this moment and this is
> >> returned as the current time.
> > 
> > How did you test this?
> 
> Remember the linux trampoline code... jumping from FSBL to the kernel without
> u-boot? Well, got it working and it speeds our boot substantially. This means we
> now reach rtc-zynqmp.c's probe within one second of power on. When we boot the
> board without a psbatt, and no internet connection (which means
> systemd-timesyncd cannot fix the bunkers date), we were ending up with a date in
> the very distant future (>50 years into the future). Aside from the date in our
> UI, all our SSL connections were failing because of certificate dates. This got
> our attention.
> 
> We drilled down quite a lot to find the real root cause. We used JTAG with xsct
> with our board simply powered on but not booted (bootselect SD without an SD
> card). This means no FSBL/psu_init code has run.
> 
> Exhibit A:
> 
> xsct% rrd rtc
>      set_time_write                  set_time_read: 00000000
>         calib_write                     calib_read: 00000000
>        current_time: 00000000                alarm: 00000000
>      rtc_int_status:       00         rtc_int_mask:       03
>          rtc_int_en                    rtc_int_dis
>          addr_error:       00  addr_error_int_mask:       01
>   addr_error_int_en             addr_error_int_dis
>             control: 01000000           safety_chk: 00000000
> 
> 
> Then we enable the RTC:
> 
> xsct% rwr rtc control 0x81000000
> 

Doesn't that enable battery switchover instead of simply enabling the
rtc, I though you didn't have a battery.

Or does that mean that your previous read of control returning bit 24
set is also bogus?

I ask because the simpler solution would simply to return -EINVAL in
xlnx_rtc_read_time when you detect a power on condition.

> The counter now counts, set_time_read and calib_read are garbage:
> xsct% rrd rtc
>      set_time_write                  set_time_read: fffe6bff
>         calib_write                     calib_read: 000f7fff
>        current_time: 00000002                alarm: 00000000
>      rtc_int_status:       01         rtc_int_mask:       03
>          rtc_int_en                    rtc_int_dis
>          addr_error:       00  addr_error_int_mask:       01
>   addr_error_int_en             addr_error_int_dis
>             control: 81000000           safety_chk: 00000000
> xsct% rrd rtc
>      set_time_write                  set_time_read: fffe6bff
>         calib_write                     calib_read: 000f7fff
>        current_time: 00000005                alarm: 00000000
>      rtc_int_status:       01         rtc_int_mask:       03
>          rtc_int_en                    rtc_int_dis
>          addr_error:       00  addr_error_int_mask:       01
>   addr_error_int_en             addr_error_int_dis
>             control: 81000000           safety_chk: 00000000
> xsct% rrd rtc
>      set_time_write                  set_time_read: fffe6bff
>         calib_write                     calib_read: 000f7fff
>        current_time: 00000008                alarm: 00000000
>      rtc_int_status:       01         rtc_int_mask:       03
>          rtc_int_en                    rtc_int_dis
>          addr_error:       00  addr_error_int_mask:       01
>   addr_error_int_en             addr_error_int_dis
>             control: 81000000           safety_chk: 00000000
> 
> We tested further, inserted a psbatt, enabled the RTC, but never touched
> set_time_write. Powered our board on and off. The observations is that that
> set_time_read and calib_read registers are not initialized to 0x0 upon power on.
> Once set though, they keep their values as long as the system is powered on or
> psbatt keeps the RTC alive. This is a contradiction of the register POR values
> specification.
> 
> So for rtc-zynqmp.c, it means that using the set_time_read register is a bad
> idea unless the code can definitely say that the set_time_write has been written
> to. And this is exactly what our patch does, by use of the seconds interrupt
> being enabled which signals the read_time() function that there is a pending
> write, and therefore the set_time_read can reliably be read.
> 
> Hope this helps!
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com