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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 17 Jun 2024 09:25:51 +0200
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: claudiu beznea <claudiu.beznea@...on.dev>
Cc: geert+renesas@...der.be, mturquette@...libre.com, sboyd@...nel.org,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	lee@...nel.org, magnus.damm@...il.com,
	linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-rtc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 06/12] rtc: renesas-rtca3: Add driver for RTCA-3
 available on Renesas RZ/G3S SoC

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@...tlin.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@...der.be>
>     Signed-off-by: Alexandre Belloni <alexandre.belloni@...tlin.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