* [PATCH] rtc: at91rm9200: add correction support
@ 2020-11-08 23:20 Alexandre Belloni
2020-11-10 13:18 ` Nicolas Ferre
0 siblings, 1 reply; 3+ messages in thread
From: Alexandre Belloni @ 2020-11-08 23:20 UTC (permalink / raw)
To: Alessandro Zummo, Alexandre Belloni, Nicolas Ferre, Ludovic Desroches
Cc: linux-rtc, linux-arm-kernel, linux-kernel
The sama5d4 and sama5d2 RTCs are able to correct for imprecise crystals, up
to 1953 ppm.
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
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
+
#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*/
+ 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.
+ */
+ 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)
+ corr = 128;
+
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] rtc: at91rm9200: add correction support
2020-11-08 23:20 [PATCH] rtc: at91rm9200: add correction support Alexandre Belloni
@ 2020-11-10 13:18 ` Nicolas Ferre
2020-11-15 0:49 ` Alexandre Belloni
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Ferre @ 2020-11-10 13:18 UTC (permalink / raw)
To: Alexandre Belloni, Alessandro Zummo, Ludovic Desroches
Cc: linux-rtc, linux-arm-kernel, linux-kernel, Sandeep Sheriker
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@bootlin.com>
> ---
> 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@microchip.com>
Thanks, best regards,
Nicolas
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] rtc: at91rm9200: add correction support
2020-11-10 13:18 ` Nicolas Ferre
@ 2020-11-15 0:49 ` Alexandre Belloni
0 siblings, 0 replies; 3+ messages in thread
From: Alexandre Belloni @ 2020-11-15 0:49 UTC (permalink / raw)
To: Nicolas Ferre
Cc: Alessandro Zummo, Ludovic Desroches, linux-rtc, linux-arm-kernel,
linux-kernel, Sandeep Sheriker
On 10/11/2020 14:18:27+0100, Nicolas Ferre wrote:
> 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...
>
I did see that when I reviewed the sam9x60 dtsi and it has its own
compatible string upstream.
> > to 1953 ppm.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> > 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.
>
> > +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?)
>
Yes, 764ppb is closer to 1525ppb (the 1.5ppm from the datasheet) than 0
so at that point it starts to make sense to correct the offset.
> > + 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?
>
The values in the datasheet are not that well rounded, the comment is
really the answer, starting with 29208ppb, with highppm = 1 the values
are a superset of the ones with highppm = 0
> > + */
> > + 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.
>
Yes, this handles corrections between 764ppb and 1525ppb.
> > + 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.
>
Well, there isn't much more to explain. However, I think the IP could be
a bit more friendly because high correction values means very little
correction is happening. Also, NEGPPM is reversed versus other RTCs and
it was not 100% clear from the datasheet.
> 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@microchip.com>
>
The offset calculations are usually coming directly from the datasheet
so it is not unusual to have little explanation. It is obviously easier
when there is a more direct correlation between the register value and
the offset value in ppb.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-15 0:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-08 23:20 [PATCH] rtc: at91rm9200: add correction support Alexandre Belloni
2020-11-10 13:18 ` Nicolas Ferre
2020-11-15 0:49 ` Alexandre Belloni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).