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

[claudiu beznea <claudiu.beznea@tuxon.dev>]

On 17.06.2024 10:25, Alexandre Belloni wrote:
> 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.

I'll double check with hardware team on this and return with an answer.

Thank you for your review,
Claudiu Beznea

> 
>>
>> [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.
> 
>