| Message ID | 20211019131724.3109-3-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: s3c: S3C driver improvements | expand |
On 19/10/2021 15:17, Sam Protsenko wrote: > This RTC driver only accepts dates from 2000 to 2099 year. It starts > counting from 2000 to avoid Y2K problem, 1. Where is the minimum (2000) year set in the RTC driver? > and S3C RTC only supports 100 On some of the devices 100, on some 1000, therefore, no. This does not look correct. > years range. Provide this info to RTC framework. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/rtc/rtc-s3c.c | 2 ++ > 1 file changed, 2 insertions(+) > Best regards, Krzysztof
On 19/10/2021 16:17:22+0300, Sam Protsenko wrote: > This RTC driver only accepts dates from 2000 to 2099 year. It starts > counting from 2000 to avoid Y2K problem, and S3C RTC only supports 100 > years range. Provide this info to RTC framework. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/rtc/rtc-s3c.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c > index 10e591794276..d9994efd70ef 100644 > --- a/drivers/rtc/rtc-s3c.c > +++ b/drivers/rtc/rtc-s3c.c > @@ -454,6 +454,8 @@ static int s3c_rtc_probe(struct platform_device *pdev) > } > > info->rtc->ops = &s3c_rtcops; > + info->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000; > + info->rtc->range_max = RTC_TIMESTAMP_END_2099; > This change is missing the if (year < 0 || year >= 100) removal in s3c_rtc_settime() > ret = devm_rtc_register_device(info->rtc); > if (ret) > -- > 2.30.2 >
On 19/10/2021 18:17, Krzysztof Kozlowski wrote: > On 19/10/2021 15:17, Sam Protsenko wrote: >> This RTC driver only accepts dates from 2000 to 2099 year. It starts >> counting from 2000 to avoid Y2K problem, > > 1. Where is the minimum (2000) year set in the RTC driver? Ah, indeed. I found it now in the driver. > >> and S3C RTC only supports 100 > > On some of the devices 100, on some 1000, therefore, no. This does not > look correct. That part of sentence is still incorrect, but change itself makes sense. Driver does not support <2000. Best regards, Krzysztof
On Tue, 19 Oct 2021 at 19:20, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 19/10/2021 16:17:22+0300, Sam Protsenko wrote: > > This RTC driver only accepts dates from 2000 to 2099 year. It starts > > counting from 2000 to avoid Y2K problem, and S3C RTC only supports 100 > > years range. Provide this info to RTC framework. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > drivers/rtc/rtc-s3c.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c > > index 10e591794276..d9994efd70ef 100644 > > --- a/drivers/rtc/rtc-s3c.c > > +++ b/drivers/rtc/rtc-s3c.c > > @@ -454,6 +454,8 @@ static int s3c_rtc_probe(struct platform_device *pdev) > > } > > > > info->rtc->ops = &s3c_rtcops; > > + info->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000; > > + info->rtc->range_max = RTC_TIMESTAMP_END_2099; > > > > This change is missing the if (year < 0 || year >= 100) removal in > s3c_rtc_settime() > It's not actually removed in [PATCH 3/4] (if I'm following you correctly), it was replaced with this code: <<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> if (rtc_tm.tm_year < 0 || rtc_tm.tm_year >= 100) { dev_err(dev, "rtc only supports 100 years\n"); return -EINVAL; } <<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> But [PATCH 3/4] is mostly needed for [PATCH 4/4], so you can drop it if you don't like it. Or it might be kept as a cleanup. + + if (rtc_tm.tm_year < 0 || rtc_tm.tm_year >= 100) { + dev_err(dev, "rtc only supports 100 years\n"); + return -EINVAL; + } > > ret = devm_rtc_register_device(info->rtc); > > if (ret) > > -- > > 2.30.2 > > > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Tue, 19 Oct 2021 at 19:22, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 19/10/2021 18:17, Krzysztof Kozlowski wrote: > > On 19/10/2021 15:17, Sam Protsenko wrote: > >> This RTC driver only accepts dates from 2000 to 2099 year. It starts > >> counting from 2000 to avoid Y2K problem, > > > > 1. Where is the minimum (2000) year set in the RTC driver? > > Ah, indeed. I found it now in the driver. > > > > >> and S3C RTC only supports 100 > > > > On some of the devices 100, on some 1000, therefore, no. This does not > > look correct. > > That part of sentence is still incorrect, but change itself makes sense. > Driver does not support <2000. > Driver itself does not allow setting year >= 2100: <<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> if (year < 0 || year >= 100) { dev_err(dev, "rtc only supports 100 years\n"); return -EINVAL; } <<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> Devices might allow it, so the commit message phrasing is incorrect and should be replaced, yes. But the code should be correct. Should I send v2 with fixed commit message? > Best regards, > Krzysztof
On 19/10/2021 19:31:55+0300, Sam Protsenko wrote: > On Tue, 19 Oct 2021 at 19:20, Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: > > > > On 19/10/2021 16:17:22+0300, Sam Protsenko wrote: > > > This RTC driver only accepts dates from 2000 to 2099 year. It starts > > > counting from 2000 to avoid Y2K problem, and S3C RTC only supports 100 > > > years range. Provide this info to RTC framework. > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > --- > > > drivers/rtc/rtc-s3c.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c > > > index 10e591794276..d9994efd70ef 100644 > > > --- a/drivers/rtc/rtc-s3c.c > > > +++ b/drivers/rtc/rtc-s3c.c > > > @@ -454,6 +454,8 @@ static int s3c_rtc_probe(struct platform_device *pdev) > > > } > > > > > > info->rtc->ops = &s3c_rtcops; > > > + info->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000; > > > + info->rtc->range_max = RTC_TIMESTAMP_END_2099; > > > > > > > This change is missing the if (year < 0 || year >= 100) removal in > > s3c_rtc_settime() > > > > It's not actually removed in [PATCH 3/4] (if I'm following you > correctly), it was replaced with this code: > > <<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> > if (rtc_tm.tm_year < 0 || rtc_tm.tm_year >= 100) { > dev_err(dev, "rtc only supports 100 years\n"); > return -EINVAL; > } > <<<<<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> > After setting the range, the core will never pass values outside of this range so it is not necessary to check in the driver anymore.
On 19/10/2021 19:35:26+0300, Sam Protsenko wrote: > On Tue, 19 Oct 2021 at 19:22, Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: > > > > On 19/10/2021 18:17, Krzysztof Kozlowski wrote: > > > On 19/10/2021 15:17, Sam Protsenko wrote: > > >> This RTC driver only accepts dates from 2000 to 2099 year. It starts > > >> counting from 2000 to avoid Y2K problem, > > > > > > 1. Where is the minimum (2000) year set in the RTC driver? > > > > Ah, indeed. I found it now in the driver. > > > > > > > >> and S3C RTC only supports 100 > > > > > > On some of the devices 100, on some 1000, therefore, no. This does not > > > look correct. > > > > That part of sentence is still incorrect, but change itself makes sense. > > Driver does not support <2000. > > > > Driver itself does not allow setting year >= 2100: > > <<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> > if (year < 0 || year >= 100) { > dev_err(dev, "rtc only supports 100 years\n"); > return -EINVAL; > } > <<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> > > Devices might allow it, so the commit message phrasing is incorrect > and should be replaced, yes. But the code should be correct. Should I > send v2 with fixed commit message? > It would be better to pass the proper values because else nobody will ever come back and fix it (hence why I didn't move that driver to devm_rtc_register_device yet).
On Tue, 19 Oct 2021 at 20:48, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 19/10/2021 19:35:26+0300, Sam Protsenko wrote: > > On Tue, 19 Oct 2021 at 19:22, Krzysztof Kozlowski > > <krzysztof.kozlowski@canonical.com> wrote: > > > > > > On 19/10/2021 18:17, Krzysztof Kozlowski wrote: > > > > On 19/10/2021 15:17, Sam Protsenko wrote: > > > >> This RTC driver only accepts dates from 2000 to 2099 year. It starts > > > >> counting from 2000 to avoid Y2K problem, > > > > > > > > 1. Where is the minimum (2000) year set in the RTC driver? > > > > > > Ah, indeed. I found it now in the driver. > > > > > > > > > > >> and S3C RTC only supports 100 > > > > > > > > On some of the devices 100, on some 1000, therefore, no. This does not > > > > look correct. > > > > > > That part of sentence is still incorrect, but change itself makes sense. > > > Driver does not support <2000. > > > > > > > Driver itself does not allow setting year >= 2100: > > > > <<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> > > if (year < 0 || year >= 100) { > > dev_err(dev, "rtc only supports 100 years\n"); > > return -EINVAL; > > } > > <<<<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>>>> > > > > Devices might allow it, so the commit message phrasing is incorrect > > and should be replaced, yes. But the code should be correct. Should I > > send v2 with fixed commit message? > > > > It would be better to pass the proper values because else nobody will > ever come back and fix it (hence why I didn't move that driver to > devm_rtc_register_device yet). > Krzysztof, do you have by chance the doc for different SoCs supported by S3C RTC driver? I can implement proper values for min/max range for each SoC, as Alexandre asked, by adding those to driver data. But I need max year register value (100, 1000, etc) for each of those chips: - "samsung,s3c2410-rtc" - "samsung,s3c2416-rtc" - "samsung,s3c2443-rtc" - "samsung,s3c6410-rtc" - "samsung,exynos3250-rtc" For example Exynos850 TRM states that BCDYEAR register has [11:0] bits for holding the year value in BCD format, so it's 10^(12/4)=1000 years max. > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On 19/10/2021 22:12:09+0300, Sam Protsenko wrote: > > It would be better to pass the proper values because else nobody will > > ever come back and fix it (hence why I didn't move that driver to > > devm_rtc_register_device yet). > > > > Krzysztof, do you have by chance the doc for different SoCs supported > by S3C RTC driver? I can implement proper values for min/max range for > each SoC, as Alexandre asked, by adding those to driver data. But I > need max year register value (100, 1000, etc) for each of those chips: > > - "samsung,s3c2410-rtc" > - "samsung,s3c2416-rtc" > - "samsung,s3c2443-rtc" > - "samsung,s3c6410-rtc" > - "samsung,exynos3250-rtc" > > For example Exynos850 TRM states that BCDYEAR register has [11:0] bits > for holding the year value in BCD format, so it's 10^(12/4)=1000 years > max. > And the question will be whether time is contiguous over this period. A very common thing is that the RTC will think that years divisible by 100 are not leap years, even if the register accepts higher values. This makes it work for 2000 but fails in 2100.
On 19/10/2021 21:12, Sam Protsenko wrote: > Krzysztof, do you have by chance the doc for different SoCs supported > by S3C RTC driver? I can implement proper values for min/max range for > each SoC, as Alexandre asked, by adding those to driver data. But I > need max year register value (100, 1000, etc) for each of those chips: > > - "samsung,s3c2410-rtc" > - "samsung,s3c2416-rtc" > - "samsung,s3c2443-rtc" > - "samsung,s3c6410-rtc" > - "samsung,exynos3250-rtc" > > For example Exynos850 TRM states that BCDYEAR register has [11:0] bits > for holding the year value in BCD format, so it's 10^(12/4)=1000 years > max. > I think all S3C chips have only 8-bit wide year, so 2000-2099, while S5Pv210 and Exynos has 12-bit (1000 years). However I doubt there is big benefit of supporting more than 2100. :) If you still want, you would need to create the patch carefully because not many people can test it... Best regards, Krzysztof
On Wed, 20 Oct 2021 at 09:29, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 19/10/2021 21:12, Sam Protsenko wrote: > > Krzysztof, do you have by chance the doc for different SoCs supported > > by S3C RTC driver? I can implement proper values for min/max range for > > each SoC, as Alexandre asked, by adding those to driver data. But I > > need max year register value (100, 1000, etc) for each of those chips: > > > > - "samsung,s3c2410-rtc" > > - "samsung,s3c2416-rtc" > > - "samsung,s3c2443-rtc" > > - "samsung,s3c6410-rtc" > > - "samsung,exynos3250-rtc" > > > > For example Exynos850 TRM states that BCDYEAR register has [11:0] bits > > for holding the year value in BCD format, so it's 10^(12/4)=1000 years > > max. > > > > I think all S3C chips have only 8-bit wide year, so 2000-2099, while > S5Pv210 and Exynos has 12-bit (1000 years). However I doubt there is big > benefit of supporting more than 2100. :) If you still want, you would > need to create the patch carefully because not many people can test it... > Guys, After testing thoroughly, I can confirm that Alexandre is right about leap years (Exynos850 RTC treats both 2000 and 2100 as leap years). And it also overflows internally on 2159 year, limiting the actual time range at 160 years. So I'll keep that range at 100 years for all RTCs. As Krzysztof said, there is no practical reasons in trying to increase it anyway. Will send v2 soon. What I'm curious about is RTC testing. I've found this test suite: tools/testing/selftests/rtc/rtctest.c But it doesn't seem to cover corner cases (like checking leap years, which was discussed here). Just a thought: maybe it should be added there, so everyone can benefit from that? For example, I know that in Linaro we are running LKFT tests for different boards, so that might theoretically reveal some bugs. Though I understand possible implications: we probably don't know which ranges are supported in driver that's being tested. Anyway, just saying. > > Best regards, > Krzysztof
On 21/10/2021 22:48:51+0300, Sam Protsenko wrote: > After testing thoroughly, I can confirm that Alexandre is right about > leap years (Exynos850 RTC treats both 2000 and 2100 as leap years). > And it also overflows internally on 2159 year, limiting the actual > time range at 160 years. So I'll keep that range at 100 years for all > RTCs. As Krzysztof said, there is no practical reasons in trying to > increase it anyway. Will send v2 soon. > > What I'm curious about is RTC testing. I've found this test suite: > > tools/testing/selftests/rtc/rtctest.c > > But it doesn't seem to cover corner cases (like checking leap years, > which was discussed here). Just a thought: maybe it should be added > there, so everyone can benefit from that? For example, I know that in > Linaro we are running LKFT tests for different boards, so that might > theoretically reveal some bugs. Though I understand possible > implications: we probably don't know which ranges are supported in > driver that's being tested. Anyway, just saying. > Sorry, I should have pointed to: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c This does check for the actual range of an RTC.
diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c index 10e591794276..d9994efd70ef 100644 --- a/drivers/rtc/rtc-s3c.c +++ b/drivers/rtc/rtc-s3c.c @@ -454,6 +454,8 @@ static int s3c_rtc_probe(struct platform_device *pdev) } info->rtc->ops = &s3c_rtcops; + info->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000; + info->rtc->range_max = RTC_TIMESTAMP_END_2099; ret = devm_rtc_register_device(info->rtc); if (ret)
This RTC driver only accepts dates from 2000 to 2099 year. It starts counting from 2000 to avoid Y2K problem, and S3C RTC only supports 100 years range. Provide this info to RTC framework. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/rtc/rtc-s3c.c | 2 ++ 1 file changed, 2 insertions(+)