| Message ID | 20250109-enable-rtc-v3-4-f003e8144419@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable RTC for the MT6357 | expand |
On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote: > The start time parameters is currently hardcoded to the driver, but > it may not fit with all equivalent RTC that driver is able to support. > > Remove the start_secs and set_start_time value setup because it > will be handled by the rtc_device_get_offset function using the > start-year DTS property. > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > drivers/rtc/rtc-mt6397.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c > index 692c00ff544b2..d47626d47602f 100644 > --- a/drivers/rtc/rtc-mt6397.c > +++ b/drivers/rtc/rtc-mt6397.c > @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev) > rtc->rtc_dev->ops = &mtk_rtc_ops; > rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900; > rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59); > - rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0); > - rtc->rtc_dev->set_start_time = true; > This is going to break the time for people upgrading their kernel, you are unfortunately stuck with this. > return devm_rtc_register_device(rtc->rtc_dev); > } > > -- > 2.25.1 >
On 11/04/2025 15:36:12+0200, Alexandre Belloni wrote: > On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote: > > The start time parameters is currently hardcoded to the driver, but > > it may not fit with all equivalent RTC that driver is able to support. > > > > Remove the start_secs and set_start_time value setup because it > > will be handled by the rtc_device_get_offset function using the > > start-year DTS property. > > > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > > --- > > drivers/rtc/rtc-mt6397.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c > > index 692c00ff544b2..d47626d47602f 100644 > > --- a/drivers/rtc/rtc-mt6397.c > > +++ b/drivers/rtc/rtc-mt6397.c > > @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev) > > rtc->rtc_dev->ops = &mtk_rtc_ops; > > rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900; > > rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59); > > - rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0); > > - rtc->rtc_dev->set_start_time = true; > > > > This is going to break the time for people upgrading their kernel, you > are unfortunately stuck with this. > To be clear, the breakage will happen when upgrading the kernel but not the device tree with 5/5 > > return devm_rtc_register_device(rtc->rtc_dev); > > } > > > > -- > > 2.25.1 > > > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Il 11/04/25 15:39, Alexandre Belloni ha scritto: > On 11/04/2025 15:36:12+0200, Alexandre Belloni wrote: >> On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote: >>> The start time parameters is currently hardcoded to the driver, but >>> it may not fit with all equivalent RTC that driver is able to support. >>> >>> Remove the start_secs and set_start_time value setup because it >>> will be handled by the rtc_device_get_offset function using the >>> start-year DTS property. >>> >>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> >>> --- >>> drivers/rtc/rtc-mt6397.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c >>> index 692c00ff544b2..d47626d47602f 100644 >>> --- a/drivers/rtc/rtc-mt6397.c >>> +++ b/drivers/rtc/rtc-mt6397.c >>> @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev) >>> rtc->rtc_dev->ops = &mtk_rtc_ops; >>> rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900; >>> rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59); >>> - rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0); >>> - rtc->rtc_dev->set_start_time = true; >>> >> >> This is going to break the time for people upgrading their kernel, you >> are unfortunately stuck with this. >> > > To be clear, the breakage will happen when upgrading the kernel but not > the device tree with 5/5 > Yes, you're stuck with this. Devicetree has to be retrocompatible. Besides, this start_secs is what gets used by default, and the start-year devicetree property should take precedence and effectively override the start_secs default. Just keep it there.... :-) Cheers, Angelo >>> return devm_rtc_register_device(rtc->rtc_dev); >>> } >>> >>> -- >>> 2.25.1 >>> >> >> -- >> Alexandre Belloni, co-owner and COO, Bootlin >> Embedded Linux and Kernel engineering >> https://bootlin.com >
On 14/04/2025 13:09, AngeloGioacchino Del Regno wrote: > Il 11/04/25 15:39, Alexandre Belloni ha scritto: >> On 11/04/2025 15:36:12+0200, Alexandre Belloni wrote: >>> On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote: >>>> The start time parameters is currently hardcoded to the driver, but >>>> it may not fit with all equivalent RTC that driver is able to support. >>>> >>>> Remove the start_secs and set_start_time value setup because it >>>> will be handled by the rtc_device_get_offset function using the >>>> start-year DTS property. >>>> >>>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> >>>> --- >>>> drivers/rtc/rtc-mt6397.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c >>>> index 692c00ff544b2..d47626d47602f 100644 >>>> --- a/drivers/rtc/rtc-mt6397.c >>>> +++ b/drivers/rtc/rtc-mt6397.c >>>> @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev) >>>> rtc->rtc_dev->ops = &mtk_rtc_ops; >>>> rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900; >>>> rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59); >>>> - rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0); >>>> - rtc->rtc_dev->set_start_time = true; >>> >>> This is going to break the time for people upgrading their kernel, you >>> are unfortunately stuck with this. >>> >> >> To be clear, the breakage will happen when upgrading the kernel but not >> the device tree with 5/5 >> > > Yes, you're stuck with this. Devicetree has to be retrocompatible. > > Besides, this start_secs is what gets used by default, and the start-year > devicetree property should take precedence and effectively override the > start_secs default. > > Just keep it there.... :-) When you boot your board for the first time, is the date January 2nd 1968 ? If not, that mean it is used as a finetune offset year. IMHO, mktime64(1968, 1, 2, 0, 0, 0) is a workaround for the rtc framework issue we try to solve in this serie because start_secs is negative (1968 < 1970). Now framework handle the negative value properly, even if you keep mktime64(1968, 1, 2, 0, 0, 0) , the device time will change. I prefer to notify you. :) TBH, it's hard to follow the logic, so I've a question: If I push in my V4 a framework fix that drivers using year < 1970 will need to have a new start_secs or start-year value to stay aligned with there previous value, do you will accept it ? Because drivers implementation are based on a bugged framework, so it's impossible, IMHO, to fix the framework without impacting date values. If you don't want to touch framework, then consider offset year in the drivers to reach above 1970 :) If you have a solution to keep "rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);" without having the date changed, don't hesitate to tell me, I'm not forcing for a specific one, just tell me what you prefer for the V4. Regards, Alex
Hello Alex, On Mon, Apr 14, 2025 at 03:56:11PM +0200, Alexandre Mergnat wrote: > On 14/04/2025 13:09, AngeloGioacchino Del Regno wrote: > > Il 11/04/25 15:39, Alexandre Belloni ha scritto: > > > On 11/04/2025 15:36:12+0200, Alexandre Belloni wrote: > > > > On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote: > > > > > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c > > > > > index 692c00ff544b2..d47626d47602f 100644 > > > > > --- a/drivers/rtc/rtc-mt6397.c > > > > > +++ b/drivers/rtc/rtc-mt6397.c > > > > > @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev) > > > > > rtc->rtc_dev->ops = &mtk_rtc_ops; > > > > > rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900; > > > > > rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59); > > > > > - rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0); > > > > > - rtc->rtc_dev->set_start_time = true; > > > > > > > > This is going to break the time for people upgrading their kernel, you > > > > are unfortunately stuck with this. > > > > > > To be clear, the breakage will happen when upgrading the kernel but not > > > the device tree with 5/5 > > > > Yes, you're stuck with this. Devicetree has to be retrocompatible. > > > > Besides, this start_secs is what gets used by default, and the start-year > > devicetree property should take precedence and effectively override the > > start_secs default. > > > > Just keep it there.... :-) It would work to keep setting start_secs but allow overwriting that value in the device tree. But see below. > When you boot your board for the first time, is the date January 2nd 1968 ? > If not, that mean it is used as a finetune offset year. > IMHO, mktime64(1968, 1, 2, 0, 0, 0) is a workaround for the rtc framework > issue we try to solve in this serie because start_secs is negative (1968 < > 1970). Now framework handle the negative value properly, even if you keep > mktime64(1968, 1, 2, 0, 0, 0) , the device time will change. I prefer to > notify you. :) I don't understand everything you wrote here, but as far as I see it, rtc_time64_to_tm() not being able to handle dates before 1970 is the main issue here. This is of course only relevant, because your hardware occasionally contains such a date. The technically right fix is to extend rtc_time64_to_tm() to work for dates >= 1900-01-01. (An alternative would be to assume that a hardware read returning a date before 1970 is invalid. If you refuse to write dates before 1970 that should give a consistent behaviour. But the original approach is the nicer one.) > TBH, it's hard to follow the logic, so I've a question: > If I push in my V4 a framework fix that drivers using year < 1970 will need > to have a new start_secs or start-year value to stay aligned with there > previous value, do you will accept it ? Doesn't the need to shift the start year simply goes away once rtc_time64_to_tm() is fixed for negative time values? So I would expect that going forward with just patches #1 and #2 should result in a fixed driver regarding the breakage you're seeing. (I'm unsure about patch #3, I'll address that in a reply to the respective mail.) Best regards Uwe
On 14/04/2025 23:34:48+0200, Uwe Kleine-König wrote: > > > Yes, you're stuck with this. Devicetree has to be retrocompatible. > > > > > > Besides, this start_secs is what gets used by default, and the start-year > > > devicetree property should take precedence and effectively override the > > > start_secs default. > > > > > > Just keep it there.... :-) > > It would work to keep setting start_secs but allow overwriting that > value in the device tree. But see below. > This is already the case. > > When you boot your board for the first time, is the date January 2nd 1968 ? > > If not, that mean it is used as a finetune offset year. > > IMHO, mktime64(1968, 1, 2, 0, 0, 0) is a workaround for the rtc framework > > issue we try to solve in this serie because start_secs is negative (1968 < > > 1970). Now framework handle the negative value properly, even if you keep > > mktime64(1968, 1, 2, 0, 0, 0) , the device time will change. I prefer to > > notify you. :) > > I don't understand everything you wrote here, but as far as I see it, > rtc_time64_to_tm() not being able to handle dates before 1970 is the > main issue here. This is of course only relevant, because your hardware > occasionally contains such a date. The technically right fix is to > extend rtc_time64_to_tm() to work for dates >= 1900-01-01. (An > alternative would be to assume that a hardware read returning a date > before 1970 is invalid. If you refuse to write dates before 1970 that > should give a consistent behaviour. But the original approach is the > nicer one.) > Yes, the assumption is that dates before 1970 are definitively invalid. I still believe we live in a world were the time doesn't go back ;) Android *was* the only OS requiring to be able to set 01/01/1970. This changed after they realized that some hardware is not able to do that. > > TBH, it's hard to follow the logic, so I've a question: > > If I push in my V4 a framework fix that drivers using year < 1970 will need > > to have a new start_secs or start-year value to stay aligned with there > > previous value, do you will accept it ? > > Doesn't the need to shift the start year simply goes away once > rtc_time64_to_tm() is fixed for negative time values? > > So I would expect that going forward with just patches #1 and #2 should > result in a fixed driver regarding the breakage you're seeing. (I'm > unsure about patch #3, I'll address that in a reply to the respective > mail.) > This is also what I think but I don't think I'm going to allow the rtc_valid_tm() change. It shouldn't matter as the check should always happen after offsetting/windowing.
diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c index 692c00ff544b2..d47626d47602f 100644 --- a/drivers/rtc/rtc-mt6397.c +++ b/drivers/rtc/rtc-mt6397.c @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev) rtc->rtc_dev->ops = &mtk_rtc_ops; rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900; rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59); - rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0); - rtc->rtc_dev->set_start_time = true; return devm_rtc_register_device(rtc->rtc_dev); }
The start time parameters is currently hardcoded to the driver, but it may not fit with all equivalent RTC that driver is able to support. Remove the start_secs and set_start_time value setup because it will be handled by the rtc_device_get_offset function using the start-year DTS property. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> --- drivers/rtc/rtc-mt6397.c | 2 -- 1 file changed, 2 deletions(-)