Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a5ed729a-55f8-f148-03b8-7952f94572dd@foss.st.com>
Date:   Wed, 5 Jul 2023 17:57:51 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 3/7] rtc: stm32: improve rtc precision
To:     Alexandre Belloni <alexandre.belloni@bootlin.com>
CC:     Alessandro Zummo <a.zummo@towertech.it>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Antonio Borneo <antonio.borneo@foss.st.com>,
        Christophe Guibout <christophe.guibout@foss.st.com>,
        Gabriel Fernandez <gabriel.fernandez@foss.st.com>,
        <linux-rtc@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>
References: <20230615092753.323844-1-valentin.caron@foss.st.com>
 <20230615092753.323844-4-valentin.caron@foss.st.com>
 <20230625231454d53ad0e0@mail.local>
Content-Language: en-US
From:   Valentin CARON <valentin.caron@foss.st.com>
In-Reply-To: <20230625231454d53ad0e0@mail.local>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Alexandre,

Sorry for the delay

On 6/26/23 01:14, Alexandre Belloni wrote:
> On 15/06/2023 11:27:49+0200, Valentin Caron wrote:
>> From: Christophe Guibout <christophe.guibout@foss.st.com>
>>
>> The rtc is used to update the stgen counter on wake up from
>> low power modes, so it needs to be as much accurate as possible.
>>
>> The maximization of asynchronous divider leads to a 4ms rtc
>> precision clock.
>> By decreasing pred_a to 0, it will have pred_s=32767 (when
>> need_accuracy is true), so stgen clock becomes more accurate
>> with 30us precision.
>> Nevertheless this will leads to an increase of power consumption.
>>
>> Signed-off-by: Christophe Guibout <christophe.guibout@foss.st.com>
>> Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
>> ---
>>   drivers/rtc/rtc-stm32.c | 26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
>> index bd7a59a07537..cad88668bcfb 100644
>> --- a/drivers/rtc/rtc-stm32.c
>> +++ b/drivers/rtc/rtc-stm32.c
>> @@ -114,6 +114,7 @@ struct stm32_rtc_data {
>>   	void (*clear_events)(struct stm32_rtc *rtc, unsigned int flags);
>>   	bool has_pclk;
>>   	bool need_dbp;
>> +	bool need_accuracy;
>>   };
>>   
>>   struct stm32_rtc {
>> @@ -545,6 +546,7 @@ static void stm32_rtc_clear_events(struct stm32_rtc *rtc,
>>   static const struct stm32_rtc_data stm32_rtc_data = {
>>   	.has_pclk = false,
>>   	.need_dbp = true,
>> +	.need_accuracy = false,
>>   	.regs = {
>>   		.tr = 0x00,
>>   		.dr = 0x04,
>> @@ -566,6 +568,7 @@ static const struct stm32_rtc_data stm32_rtc_data = {
>>   static const struct stm32_rtc_data stm32h7_rtc_data = {
>>   	.has_pclk = true,
>>   	.need_dbp = true,
>> +	.need_accuracy = false,
>>   	.regs = {
>>   		.tr = 0x00,
>>   		.dr = 0x04,
>> @@ -596,6 +599,7 @@ static void stm32mp1_rtc_clear_events(struct stm32_rtc *rtc,
>>   static const struct stm32_rtc_data stm32mp1_data = {
>>   	.has_pclk = true,
>>   	.need_dbp = false,
>> +	.need_accuracy = true,
>>   	.regs = {
>>   		.tr = 0x00,
>>   		.dr = 0x04,
>> @@ -636,11 +640,25 @@ static int stm32_rtc_init(struct platform_device *pdev,
>>   	pred_a_max = STM32_RTC_PRER_PRED_A >> STM32_RTC_PRER_PRED_A_SHIFT;
>>   	pred_s_max = STM32_RTC_PRER_PRED_S >> STM32_RTC_PRER_PRED_S_SHIFT;
>>   
>> -	for (pred_a = pred_a_max; pred_a + 1 > 0; pred_a--) {
>> -		pred_s = (rate / (pred_a + 1)) - 1;
>> +	if (rate > (pred_a_max + 1) * (pred_s_max + 1)) {
>> +		dev_err(&pdev->dev, "rtc_ck rate is too high: %dHz\n", rate);
> What is the expect user action after seeing this message?

User could change of source clock by a smaller one (in terms of 
frequency) or adjust the clock divider in amount of RTC.
e.g. on STM32MP1, RTC source clock could be:
- LSE clock
- LSI clock
- HSE clock * an adjustable divider.

>
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (rtc->data->need_accuracy) {
>> +		for (pred_a = 0; pred_a <= pred_a_max; pred_a++) {
>> +			pred_s = (rate / (pred_a + 1)) - 1;
>>   
>> -		if (((pred_s + 1) * (pred_a + 1)) == rate)
>> -			break;
>> +			if (pred_s <= pred_s_max && ((pred_s + 1) * (pred_a + 1)) == rate)
>> +				break;
>> +		}
>> +	} else {
>> +		for (pred_a = pred_a_max; pred_a + 1 > 0; pred_a--) {
>> +			pred_s = (rate / (pred_a + 1)) - 1;
>> +
>> +			if (((pred_s + 1) * (pred_a + 1)) == rate)
>> +				break;
>> +		}
>>   	}
>>   
>>   	/*
>> -- 
>> 2.25.1
>>

Regards,
Valentin