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@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.
>
> > +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@xxxxxxxxxxxxx>
>
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
