Re: [PATCH] rtc: at91rm9200: add correction support
From: Nicolas Ferre
Date: Tue Nov 10 2020 - 08:18:34 EST
Hi Alexandre,
Thanks you for adding this feature to newest at91 RTC IPs.
On 09/11/2020 at 00:20, Alexandre Belloni wrote:
The sama5d4 and sama5d2 RTCs are able to correct for imprecise crystals, up
FYI, sam9x60 RTC has the same correction capability.
... and I now realize that sam9x60 using sam9x5-rtc compatibility sting
is maybe not the right choice...
to 1953 ppm.
Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
---
drivers/rtc/rtc-at91rm9200.c | 103 +++++++++++++++++++++++++++++++++--
1 file changed, 99 insertions(+), 4 deletions(-)
diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 5e811e04cb21..1eea187d9850 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -36,6 +36,10 @@
#define AT91_RTC_UPDCAL BIT(1) /* Update Request Calendar Register */
#define AT91_RTC_MR 0x04 /* Mode Register */
+#define AT91_RTC_HRMOD BIT(0) /* 12/24 hour mode */
+#define AT91_RTC_NEGPPM BIT(4) /* Negative PPM correction */
+#define AT91_RTC_CORRECTION GENMASK(14, 8) /* Slow clock correction */
+#define AT91_RTC_HIGHPPM BIT(15) /* High PPM correction */
#define AT91_RTC_TIMR 0x08 /* Time Register */
#define AT91_RTC_SEC GENMASK(6, 0) /* Current Second */
@@ -77,6 +81,9 @@
#define AT91_RTC_NVTIMALR BIT(2) /* Non valid Time Alarm */
#define AT91_RTC_NVCALALR BIT(3) /* Non valid Calendar Alarm */
+#define AT91_RTC_CORR_DIVIDEND 3906000
+#define AT91_RTC_CORR_LOW_RATIO 20
IMHO, it's worth telling here that these values are from the product
datasheet in formula coming from explanation of HIGHPPM bit of register
RTC_MR.
+
#define at91_rtc_read(field) \
readl_relaxed(at91_rtc_regs + field)
#define at91_rtc_write(field, val) \
@@ -84,6 +91,7 @@
struct at91_rtc_config {
bool use_shadow_imr;
+ bool has_correction;
};
static const struct at91_rtc_config *at91_rtc_config;
@@ -293,6 +301,75 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
return 0;
}
+static int at91_rtc_readoffset(struct device *dev, long *offset)
+{
+ u32 mr = at91_rtc_read(AT91_RTC_MR);
+ long val = FIELD_GET(AT91_RTC_CORRECTION, mr);
+
+ if (!val) {
+ *offset = 0;
+ return 0;
+ }
+
+ val++;
+
+ if (!(mr & AT91_RTC_NEGPPM))
+ val = -val;
+
+ if (!(mr & AT91_RTC_HIGHPPM))
+ val *= AT91_RTC_CORR_LOW_RATIO;
+
+ *offset = DIV_ROUND_CLOSEST(AT91_RTC_CORR_DIVIDEND, val);
+
+ return 0;
+}
+
+static int at91_rtc_setoffset(struct device *dev, long offset)
+{
+ long corr;
+ u32 mr;
+
+ if (offset > AT91_RTC_CORR_DIVIDEND / 2)
+ return -ERANGE;
+ if (offset < -AT91_RTC_CORR_DIVIDEND / 2)
+ return -ERANGE;
+
+ mr = at91_rtc_read(AT91_RTC_MR);
+ mr &= ~(AT91_RTC_NEGPPM | AT91_RTC_CORRECTION | AT91_RTC_HIGHPPM);
+
+ if (offset > 0)
+ mr |= AT91_RTC_NEGPPM;
+ else
+ offset = -offset;
+
+ /* offset less than 764 ppb, disable correction*/
Does it correspond to the 1.5 ppm value of the datasheet?
(sorry I'm not so used to these computations?)
+ if (offset < 764) {
+ at91_rtc_write(AT91_RTC_MR, mr & ~AT91_RTC_NEGPPM);
+
+ return 0;
+ }
+
+ /*
+ * 29208 ppb is the perfect cutoff between low range and high range
+ * low range values are never better than high range value after that.
And here, I'm lost. Does it correspond to the sentence:
"HIGHPPM set to 1 is recommended for 30 ppm correction and above." ? And
rounding using register values, am I right?
+ */
+ if (offset < 29208) {
+ corr = DIV_ROUND_CLOSEST(AT91_RTC_CORR_DIVIDEND, offset * AT91_RTC_CORR_LOW_RATIO);
+ } else {
+ corr = DIV_ROUND_CLOSEST(AT91_RTC_CORR_DIVIDEND, offset);
+ mr |= AT91_RTC_HIGHPPM;
+ }
+
+ if (corr > 128)
Okay, it's maximized to the width of register field, got it.
+ corr = 128;
I'm kind of following and don't know what other RTC drivers are doing...
but would prefer more explanation on numerical values.
+
+ mr |= FIELD_PREP(AT91_RTC_CORRECTION, corr - 1);
+
+ at91_rtc_write(AT91_RTC_MR, mr);
+
+ return 0;
+}
+
/*
* IRQ handler for the RTC
*/
@@ -343,6 +420,10 @@ static const struct at91_rtc_config at91sam9x5_config = {
.use_shadow_imr = true,
};
+static const struct at91_rtc_config sama5d4_config = {
+ .has_correction = true,
+};
+
static const struct of_device_id at91_rtc_dt_ids[] = {
{
.compatible = "atmel,at91rm9200-rtc",
@@ -352,10 +433,10 @@ static const struct of_device_id at91_rtc_dt_ids[] = {
.data = &at91sam9x5_config,
}, {
.compatible = "atmel,sama5d4-rtc",
- .data = &at91rm9200_config,
+ .data = &sama5d4_config,
}, {
.compatible = "atmel,sama5d2-rtc",
- .data = &at91rm9200_config,
+ .data = &sama5d4_config,
}, {
/* sentinel */
}
@@ -370,6 +451,16 @@ static const struct rtc_class_ops at91_rtc_ops = {
.alarm_irq_enable = at91_rtc_alarm_irq_enable,
};
+static const struct rtc_class_ops sama5d4_rtc_ops = {
+ .read_time = at91_rtc_readtime,
+ .set_time = at91_rtc_settime,
+ .read_alarm = at91_rtc_readalarm,
+ .set_alarm = at91_rtc_setalarm,
+ .alarm_irq_enable = at91_rtc_alarm_irq_enable,
+ .set_offset = at91_rtc_setoffset,
+ .read_offset = at91_rtc_readoffset,
+};
+
/*
* Initialize and install RTC driver
*/
@@ -416,7 +507,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
}
at91_rtc_write(AT91_RTC_CR, 0);
- at91_rtc_write(AT91_RTC_MR, 0); /* 24 hour mode */
+ at91_rtc_write(AT91_RTC_MR, at91_rtc_read(AT91_RTC_MR) & ~AT91_RTC_HRMOD);
/* Disable all interrupts */
at91_rtc_write_idr(AT91_RTC_ACKUPD | AT91_RTC_ALARM |
@@ -437,7 +528,11 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
if (!device_can_wakeup(&pdev->dev))
device_init_wakeup(&pdev->dev, 1);
- rtc->ops = &at91_rtc_ops;
+ if (at91_rtc_config->has_correction)
+ rtc->ops = &sama5d4_rtc_ops;
+ else
+ rtc->ops = &at91_rtc_ops;
+
rtc->range_min = RTC_TIMESTAMP_BEGIN_1900;
rtc->range_max = RTC_TIMESTAMP_END_2099;
ret = rtc_register_device(rtc);
--
2.28.0
Alexandre, you know much more than me about the habits of RTC drivers
writers. Even if I would like a little more documentation on values
used, I absolutely won't hold this feature adoption, so here is my:
Reviewed-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
Thanks, best regards,
Nicolas
--
Nicolas Ferre