Re: [PATCH 06/12] rtc: renesas-rtca3: Add driver for RTCA-3 available on Renesas RZ/G3S SoC

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

On 14/06/2024 14:06:38+0300, claudiu beznea wrote:
> >> +	/*
> >> +	 * Stop the RTC and set to 12 hours mode and calendar count mode.
> >> +	 * RCR2.START initial value is undefined so we need to stop here
> >> +	 * all the time.
> >> +	 */
> > 
> > Certainly not, if you stop the RTC on probe, you lose the time
> > information, this must only be done when the RTC has never been
> > initialised. The whole goal of the RTC is the keep time across reboots,
> > its lifecycle is longer than the system.
> 
> This was also my first thought when I read the HW manual.
> 
> It has been done like this to follow the HW manual. According to HW manual
> [1], chapter 22.3.19 RTC Control Register 2 (RCR2), initial value of START
> bit is undefined.
> 
> If it's 1 while probing but it has never been initialized, we can falsely
> detect that RTC is started and skip the rest of the initialization steps.
> W/o initialization configuration, the RTC will not be able to work.
> 
> Even with this implementation we don't loose the time b/w reboots. Here is
> the output on my board [2]. The steps I did were the following:
> 1/ remove the power to the board (I don't have a battery for RTC installed
>    at the moment)
> 2/ boot the board and issue hwclock -w
> 3/ reboot
> 4/ check the systime and rtc time
> 5/ poweroff
> 6/ poweron
> 7/ boot and check systime and RTC time
> 
> As you can see the time is not lost but continue to increment. I presume
> the hardware takes into account that time needs to increment when initial
> configuration is executed.

I don't think so, I guess it stops ticking but the registers are not
reset so when ts starts ticking again, you are not too far from the time
that it should report.

> 
> [1]
> https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz-mpus/rzg3s-general-purpose-microprocessors-single-core-arm-cortex-a55-11-ghz-cpu-and-dual-core-cortex-m33-250
> [2] https://p.fr33tux.org/585cd6
> 
> > 
> > Also, why do you insist on 12H-mode? The proper thing to do is to support
> > 12H-mode on read but always use 24H-mode when setting the time.
> 
> OK, I wasn't aware of this. I think I followed this approach as it looked
> to me the number of operation to update the hardware registers was lower
> for 12h mode.

How come, using 12H-mode implies testing for the AM/PM bit and doing and
addition while 24H-mode would simply be a read?

> >> +	priv->rtc_dev->ops = &rtca3_ops;
> >> +	priv->rtc_dev->max_user_freq = 256;
> >> +	priv->rtc_dev->range_min = mktime64(1999, 1, 1, 0, 0, 0);
> >> +	priv->rtc_dev->range_max = mktime64(2098, 12, 31, 23, 59, 59);
> > 
> > This very much looks like the range should be 2000 to 2099, why do you
> > want to shift it?
> 
> 2000-2099 was my first option for this but then I saw one of your old
> commits on this topic and, since I'm not very familiar with RTC,
> I took it as example. I'll adjust as you proposed.
> 
> commit beee05dfbead
> Author: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Date:   Wed Mar 20 12:30:10 2019 +0100
> 
>     rtc: sh: set range
> 
>     The SH RTC is a BCD RTC with some version having 4 digits for the year.
> 
>     The range for the RTCs with only 2 digits for the year was unfortunately
>     shifted to handle 1999 to 2098.
> 
>     Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>     Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

This is because the sh driver predated the introduction of the range and
was already shifting it.


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