Message-ID: <82b5b5d6-2009-45d6-bb4e-517b8978b88c@baylibre.com>
Date: Fri, 7 Feb 2025 10:30:42 +0100
From: Alexandre Mergnat <amergnat@...libre.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: Eddie Huang <eddie.huang@...iatek.com>, Sean Wang
 <sean.wang@...iatek.com>, Matthias Brugger <matthias.bgg@...il.com>,
 Macpaul Lin <macpaul.lin@...iatek.com>,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 linux-rtc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] rtc: mt6359: fix year issue

Hello Alex and Angelo

Thanks for your comments

Angelo, IMHO, "regmap_read_poll_timeout", mutex, RTC_PROT, register read/write,... aren't related to 
this issue (but I've verified anyway to be sure and everything works as expected ;) )

After digging a bit more, RTC_MIN_YEAR_OFFSET (68) hide another problems.
Please, keep in mind that I probably misunderstand how range_min, range_max, start_secs have to be use.

1)
Default driver values:
	rtc->rtc_dev->range_min = mktime64(1900, 1, 1, 0, 0, 0);
	rtc->rtc_dev->range_max = mktime64(2027, 1, 1, 23, 59, 59);
	rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);

First issue is HW alarm doesn't fire.
When the PMIC is powered-on, the year register value is 42 (is it a bug or usefull feature ?)
When "hwclock -r" (read) is sent, the RTC framework steps are:
- read time from the HW registers
- set alarm
- HW alarm triggered
- read time from the HW registers
...

But the year value written during the "set alarm" is different from 42 due to an offset issue. Then 
the alarm will trigger some years later and the timeout fire instead.

I understand that offset_secs value come from different formulas depending of the 3 variable value.
In this case, the formula is 'offset_secs = start_secs - range_min'.

Then change start_secs to mktime64(1942, 1, 1, 0, 0, 0) to have, in theory, the good offset.
But now, I don't reach the set_alarm step and have:

     Waiting for clock tick...
     ioctl(3, RTC_UIE_ON, 0): Invalid argument
     Waiting in loop for time from /dev/rtc0 to change
     hwclock: ioctl(RTC_RD_NAME) to /dev/rtc0 to read the time failed: Invalid argument
     ...synchronization failed


This issue above doesn't appear if I remove this piece of code
	/*
	 * Since the reading time values from RTC device are always in the RTC
	 * original valid range, but we need to skip the overlapped region
	 * between expanded range and original range, which is no need to add
	 * the offset.
	 */
	if (rtc->start_secs > rtc->range_min && secs >= rtc->start_secs)
		return;


Better but not over.
My current local time is 7:31:4 1-0-42 (hour:min:sec day-month-year)
Now the alarm setup value is 7:31:5 1-1-84. Compared to working setup, year value isn't consistent, 
it should be 42 instead of 84.
The removed piece of code isn't the cause of that because before removing it I've workarounded it by 
adding 8 hours (or one day) to the start_secs (mktime64(1942, 1, 1, 8, 0, 0)) and I observe the same 
issue: alarm value = 15:31:5 1-1-84 which is 8 hours and 42 years more than expected. Come back to 
the first issue, alarm doesn't fire, that make sense.

So why the alarm value is 1-1-84 instead of 1-1-42 for "start_secs = mktime64(1942, 1, 1, 0, 0, 0)" ?

2)
Playing with range allow me to find these 2 setup which have a working alarm setup and return me 
clock time : 2070/01/01, and I can't change the 2070 event if I shift the range.
In this case, the formula is 'offset_secs = range_max - range_min + 1', so change start_secs is 
useless unless I go out of the range and then, I have another issue...

	rtc->rtc_dev->range_min = mktime64(1960, 1, 1, 0, 0, 0);
	rtc->rtc_dev->range_max = mktime64(2087, 12, 31, 23, 59, 59);
	rtc->rtc_dev->start_secs = mktime64(2010, 1, 1, 0, 0, 0);
or
	rtc->rtc_dev->range_min = mktime64(2000, 1, 1, 0, 0, 0);
	rtc->rtc_dev->range_max = mktime64(2128, 1, 1, 23, 59, 59);
	rtc->rtc_dev->start_secs = mktime64(2010, 1, 1, 0, 0, 0);
or
	rtc->rtc_dev->range_min = mktime64(2004, 1, 1, 0, 0, 0);
	rtc->rtc_dev->range_max = mktime64(2132, 1, 1, 23, 59, 59);
	rtc->rtc_dev->start_secs = mktime64(2010, 1, 1, 0, 0, 0);

Note that I must add 1 day to the range_max in the 2nd & 3rd example to make it work, probably due 
to leap years ?


My first though was min and max range give the supported hardware year range, and start_secs a 
wished "software" value applying offset to the HW value. But after some try, I see a lot of 
limitations and requierements to make it "working", so I probably misunderstand how range_min, 
range_max, start_secs have to be use.


Regards
Alexandre Mergnat

On 14/01/2025 11:53, AngeloGioacchino Del Regno wrote:
> Il 09/01/25 16:46, Alexandre Belloni ha scritto:
>> On 09/01/2025 16:29:52+0100, Alexandre Mergnat wrote:
>>> Removing the RTC_MIN_YEAR_OFFSET addition and subtraction has
>>> introduced a regression.
>>>
>>> ~# hwclock -r --verbose
>>> hwclock from util-linux 2.37.4
>>> System Time: 1262312013.143552
>>> Trying to open: /dev/rtc0
>>> Using the rtc interface to the clock.
>>> Assuming hardware clock is kept in UTC time.
>>> Waiting for clock tick...
>>> hwclock: select() to /dev/rtc0 to wait for clock tick timed out
>>> ...synchronization failed
>>>
>>> Bring back the RTC_MIN_YEAR_OFFSET to fix the RTC.
>>>
>>
>> NAK, you'd have to investigate a bit more, I want to get rid of the
>> RTC_MIN_YEAR_OFFSET insanity.
>>
> 
> If literally all currently supported MediaTek PMICs RTC are working fine (and
> it's not just one), but the one that is introduced here has issues, clearly
> the problem is *not* about the min_year_offset not being there, I'd say!
> 
> Btw:
> "hwclock: select() to /dev/rtc0 to wait for clock tick timed out"
> ...is the WRTGR write failing? :-)
> 
> And no, Alexandre M, don't trust the regmap_read_poll_timeout() to return an
> error, I'm not sure that the CBUSY gets set to zero for *literally all* failures...
> 
> In particular.... some RTCs are *locked* and need to be *unlocked*, and in that
> case I'm not sure if the write just goes through but gets ignored or if CBUSY
> stays set.
> 
> Anyway, check the RTC_PROT register for the unlocking mechanism :-)
> 
> Cheers,
> Angelo
> 
>>> Fixes: 34bbdc12d04e ("rtc: mt6359: Add RTC hardware range and add support for start-year")
>>> Signed-off-by: Alexandre Mergnat <amergnat@...libre.com>
>>> ---
>>>   drivers/rtc/rtc-mt6397.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
>>> index 55e75712edd4..9930b6bdb6ca 100644
>>> --- a/drivers/rtc/rtc-mt6397.c
>>> +++ b/drivers/rtc/rtc-mt6397.c
>>> @@ -96,6 +96,12 @@ static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>>               goto exit;
>>>       } while (sec < tm->tm_sec);
>>> +    /* HW register use 7 bits to store year data, minus
>>> +     * RTC_MIN_YEAR_OFFSET before write year data to register, and plus
>>> +     * RTC_MIN_YEAR_OFFSET back after read year from register
>>> +     */
>>> +    tm->tm_year += RTC_MIN_YEAR_OFFSET;
>>> +
>>>       /* HW register start mon/wday from one, but tm_mon/tm_wday start from zero. */
>>>       tm->tm_mon--;
>>>       tm->tm_wday--;
>>> @@ -110,6 +116,7 @@ static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>>       int ret;
>>>       u16 data[RTC_OFFSET_COUNT];
>>> +    tm->tm_year -= RTC_MIN_YEAR_OFFSET;
>>>       tm->tm_mon++;
>>>       tm->tm_wday++;
>>> @@ -167,6 +174,7 @@ static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>>>       tm->tm_mon = data[RTC_OFFSET_MTH] & RTC_AL_MTH_MASK;
>>>       tm->tm_year = data[RTC_OFFSET_YEAR] & RTC_AL_YEA_MASK;
>>> +    tm->tm_year += RTC_MIN_YEAR_OFFSET;
>>>       tm->tm_mon--;
>>>       return 0;
>>> @@ -182,6 +190,7 @@ static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>>>       int ret;
>>>       u16 data[RTC_OFFSET_COUNT];
>>> +    tm->tm_year -= RTC_MIN_YEAR_OFFSET;
>>>       tm->tm_mon++;
>>>       mutex_lock(&rtc->lock);
>>>
>>> -- 
>>> 2.25.1
>>>
>>
> 
> 

-- 
Regards,
Alexandre

Powered by blists - more mailing lists