Date: Fri, 14 Jun 2024 14:06:38 +0300
From: claudiu beznea <claudiu.beznea@...on.dev>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
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

Hi, Alexandre,

On 14.06.2024 12:21, Alexandre Belloni wrote:
> Hello Claudiu,
> 
> On 14/06/2024 10:19:26+0300, Claudiu wrote:
>> +static int rtca3_initial_setup(struct rtca3_priv *priv)
>> +{
>> +	unsigned long osc32k_rate;
>> +	u8 pes, tmp, mask;
>> +	u32 sleep_us;
>> +	int ret;
>> +
>> +	osc32k_rate = clk_get_rate(priv->clk);
>> +	if (!osc32k_rate)
>> +		return -EINVAL;
>> +
>> +	sleep_us = DIV_ROUND_UP_ULL(1000000ULL, osc32k_rate) * 6;
>> +
>> +	priv->ppb.ten_sec = DIV_ROUND_CLOSEST_ULL(1000000000ULL, (osc32k_rate * 10));
>> +	priv->ppb.sixty_sec = DIV_ROUND_CLOSEST_ULL(1000000000ULL, (osc32k_rate * 60));
>> +
>> +	/*
>> +	 * According to HW manual (section 22.4.2. Clock and count mode setting procedure)
>> +	 * we need to wait at least 6 cycles of the 32KHz clock after clock was enabled.
>> +	 */
>> +	usleep_range(sleep_us, sleep_us + 10);
>> +
>> +	/* Disable alarm and carry interrupts. */
>> +	mask = RTCA3_RCR1_AIE | RTCA3_RCR1_CIE;
>> +	rtca3_byte_update_bits(priv, RTCA3_RCR1, mask, 0);
>> +	ret = readb_poll_timeout(priv->base + RTCA3_RCR1, tmp, !(tmp & mask),
>> +				 10, RTCA3_DEFAULT_TIMEOUT_US);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * 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.

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

I'll adjust as proposed.

> 
>> +	mask = RTCA3_RCR2_START | RTCA3_RCR2_HR24 | RTCA3_RCR2_CNTMD;
>> +	writeb(0, priv->base + RTCA3_RCR2);
>> +	ret = readb_poll_timeout(priv->base + RTCA3_RCR2, tmp, !(tmp & mask),
>> +				 10, RTCA3_DEFAULT_TIMEOUT_US);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Execute reset and wait for reset and calendar count mode to be applied. */
>> +	mask = RTCA3_RCR2_RESET | RTCA3_RCR2_CNTMD;
>> +	writeb(RTCA3_RCR2_RESET, priv->base + RTCA3_RCR2);
>> +	ret = readb_poll_timeout(priv->base + RTCA3_RCR2, tmp, !(tmp & mask),
>> +				 10, RTCA3_RESET_TIMEOUT_US);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * According to HW manual (section 22.6.3. Notes on writing to and reading
>> +	 * from registers) after reset we need to wait 6 clock cycles before
>> +	 * writing to RTC registers.
>> +	 */
>> +	usleep_range(sleep_us, sleep_us + 10);
>> +
>> +	/* Set no adjustment. */
>> +	writeb(0, priv->base + RTCA3_RADJ);
>> +	ret = readb_poll_timeout(priv->base + RTCA3_RADJ, tmp, !tmp, 10,
>> +				 RTCA3_DEFAULT_TIMEOUT_US);
>> +
>> +	/* Start the RTC and enable automatic time error adjustment. */
>> +	mask = RTCA3_RCR2_START | RTCA3_RCR2_AADJE;
>> +	writeb(RTCA3_RCR2_START | RTCA3_RCR2_AADJE, priv->base + RTCA3_RCR2);
>> +	ret = readb_poll_timeout(priv->base + RTCA3_RCR2, tmp, ((tmp & mask) == mask),
>> +				 10, RTCA3_START_TIMEOUT_US);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * According to HW manual (section 22.6.4. Notes on writing to and reading
>> +	 * from registers) we need to wait 1/128 seconds while the clock is operating
>> +	 * (RCR2.START bit = 1) to be able to read the counters after a return from
>> +	 * reset.
>> +	 */
>> +	usleep_range(8000, 9000);
>> +
>> +	/* Set period interrupt to 1/64 seconds. It is necessary for alarm setup. */
>> +	pes = FIELD_PREP(RTCA3_RCR1_PES, RTCA3_RCR1_PES_1_64_SEC);
>> +	rtca3_byte_update_bits(priv, RTCA3_RCR1, RTCA3_RCR1_PES, pes);
>> +	return readb_poll_timeout(priv->base + RTCA3_RCR1, tmp, ((tmp & RTCA3_RCR1_PES) == pes),
>> +				  10, RTCA3_DEFAULT_TIMEOUT_US);
>> +}
>> +
>> +static int rtca3_request_irqs(struct platform_device *pdev, struct rtca3_priv *priv)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int ret, irq;
>> +
>> +	irq = platform_get_irq_byname(pdev, "alarm");
>> +	if (irq < 0)
>> +		return dev_err_probe(dev, irq, "Failed to get alarm IRQ!\n");
>> +
>> +	ret = devm_request_irq(dev, irq, rtca3_alarm_handler, 0, "rtca3-alarm", priv);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to request alarm IRQ!\n");
>> +	priv->wakeup_irq = irq;
>> +
>> +	irq = platform_get_irq_byname(pdev, "period");
>> +	if (irq < 0)
>> +		return dev_err_probe(dev, irq, "Failed to get period IRQ!\n");
>> +
>> +	ret = devm_request_irq(dev, irq, rtca3_periodic_handler, 0, "rtca3-period", priv);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to request period IRQ!\n");
>> +
>> +	/*
>> +	 * Driver doesn't implement carry handler. Just get the IRQ here
>> +	 * for backward compatibility, in case carry support will be added later.
>> +	 */
>> +	irq = platform_get_irq_byname(pdev, "carry");
>> +	if (irq < 0)
>> +		return dev_err_probe(dev, irq, "Failed to get carry IRQ!\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int rtca3_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rtca3_priv *priv;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(priv->base))
>> +		return PTR_ERR(priv->base);
>> +
>> +	priv->clk = devm_clk_get_enabled(dev, "counter");
>> +	if (IS_ERR(priv->clk))
>> +		return PTR_ERR(priv->clk);
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	spin_lock_init(&priv->lock);
>> +	atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_DONE);
>> +	init_completion(&priv->set_alarm_completion);
>> +
>> +	ret = rtca3_initial_setup(priv);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to setup the RTC!\n");
>> +
>> +	ret = rtca3_request_irqs(pdev, priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	device_init_wakeup(&pdev->dev, 1);
>> +
>> +	priv->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
>> +	if (IS_ERR(priv->rtc_dev))
>> +		return PTR_ERR(priv->rtc_dev);
>> +
>> +	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>


Thank you for your review,
Claudiu Beznea

> 
> 

Powered by blists - more mailing lists