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.
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.
> + 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?
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Alexandre Belloni <[email protected]>
Thank you for your review,
Claudiu Beznea
>
>