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@xxxxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
Thank you for your review,
Claudiu Beznea
>
>
