Re: [PATCH 2/2] rtc: mt6359: fix year issue

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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@baylibre.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
>>
>