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

[Nicolas Ferre <nicolas.ferre@microchip.com>]

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