[PATCH] rtc: at91rm9200: add correction support

Subsystems: real time clock (rtc) subsystem, the rest

STALE2088d LANDED

Landed in mainline as f6a46f8b302d on 2020-11-17.

3 messages, 2 authors, 2020-11-15 · open the first message on its own page

[PATCH] rtc: at91rm9200: add correction support

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date: 2020-11-08 23:20:16 

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] rtc: at91rm9200: add correction support

From: Nicolas Ferre <nicolas.ferre@microchip.com>
Date: 2020-11-10 13:18:34 

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...
quoted hunk
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.
quoted hunk
+
  #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.
quoted hunk
+
+       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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] rtc: at91rm9200: add correction support

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date: 2020-11-15 00:49:40 

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:
quoted
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.
quoted
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.
quoted
+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.
quoted
+       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
quoted
+        */
+       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.
quoted
+               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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help