[PATCH 0/4] rtc: s3c: S3C driver improvements

[PATCH 0/4] rtc: s3c: S3C driver improvements

[Sam Protsenko]

While working on Exynos850 support (where this driver works fine in its
current state), I've stumbled upon some minor issue. This is the effort
to fix those. [PATCH 1/4] was already sent as a separate submission, but
Alexandre Belloni asked me to set time range while at it. This is done
in this series, in [PATCH 2/4]. So first two patches are basically
moving S3C RTC driver to newer API usage. And last two patches fixing
some error message which I noticed when doing the very first boot of my
board (when RTC registers are not initialized with S3C driver yet).
Patches 1/4 and 3/4 don't introduce any functional changes, only doing
some refactoring and cleaning up.

Sam Protsenko (4):
  rtc: s3c: Remove usage of devm_rtc_device_register()
  rtc: s3c: Add time range
  rtc: s3c: Extract read/write IO into separate functions
  rtc: s3c: Fix RTC read on first boot

 drivers/rtc/rtc-s3c.c | 137 +++++++++++++++++++++++++++++-------------
 1 file changed, 96 insertions(+), 41 deletions(-)

-- 
2.30.2

[PATCH 1/4] rtc: s3c: Remove usage of devm_rtc_device_register()

[Sam Protsenko]

devm_rtc_device_register() is deprecated. Use devm_rtc_allocate_device()
and devm_rtc_register_device() API instead. This change doesn't change
the behavior, but allows for further improvements.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/rtc/rtc-s3c.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index e57d3ca70a78..10e591794276 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -447,15 +447,18 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 1);
 
-	/* register RTC and exit */
-	info->rtc = devm_rtc_device_register(&pdev->dev, "s3c", &s3c_rtcops,
-					     THIS_MODULE);
+	info->rtc = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(info->rtc)) {
-		dev_err(&pdev->dev, "cannot attach rtc\n");
 		ret = PTR_ERR(info->rtc);
 		goto err_nortc;
 	}
 
+	info->rtc->ops = &s3c_rtcops;
+
+	ret = devm_rtc_register_device(info->rtc);
+	if (ret)
+		goto err_nortc;
+
 	ret = devm_request_irq(&pdev->dev, info->irq_alarm, s3c_rtc_alarmirq,
 			       0, "s3c2410-rtc alarm", info);
 	if (ret) {
-- 
2.30.2

[PATCH 2/4] rtc: s3c: Add time range

[Sam Protsenko]

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;
 
 	ret = devm_rtc_register_device(info->rtc);
 	if (ret)
-- 
2.30.2

[PATCH 3/4] rtc: s3c: Extract read/write IO into separate functions

[Sam Protsenko]

Create dedicated functions for I/O operations and BCD conversion. It can
be useful to separete those from representation conversion and other
stuff found in RTC callbacks, e.g. for initializing RTC registers.

This patch does not introduce any functional changes, it's merely
refactoring change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/rtc/rtc-s3c.c | 98 +++++++++++++++++++++++++++----------------
 1 file changed, 61 insertions(+), 37 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index d9994efd70ef..238928e29fbc 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -127,10 +127,9 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 	return ret;
 }
 
-/* Time read/write */
-static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
+/* Read time from RTC and convert it from BCD */
+static int s3c_rtc_read_time(struct s3c_rtc *info, struct rtc_time *tm)
 {
-	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int have_retried = 0;
 	int ret;
 
@@ -139,54 +138,40 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 		return ret;
 
 retry_get_time:
-	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
-	rtc_tm->tm_hour = readb(info->base + S3C2410_RTCHOUR);
-	rtc_tm->tm_mday = readb(info->base + S3C2410_RTCDATE);
-	rtc_tm->tm_mon  = readb(info->base + S3C2410_RTCMON);
-	rtc_tm->tm_year = readb(info->base + S3C2410_RTCYEAR);
-	rtc_tm->tm_sec  = readb(info->base + S3C2410_RTCSEC);
-
-	/* the only way to work out whether the system was mid-update
+	tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
+	tm->tm_hour = readb(info->base + S3C2410_RTCHOUR);
+	tm->tm_mday = readb(info->base + S3C2410_RTCDATE);
+	tm->tm_mon  = readb(info->base + S3C2410_RTCMON);
+	tm->tm_year = readb(info->base + S3C2410_RTCYEAR);
+	tm->tm_sec  = readb(info->base + S3C2410_RTCSEC);
+
+	/*
+	 * The only way to work out whether the system was mid-update
 	 * when we read it is to check the second counter, and if it
 	 * is zero, then we re-try the entire read
 	 */
-
-	if (rtc_tm->tm_sec == 0 && !have_retried) {
+	if (tm->tm_sec == 0 && !have_retried) {
 		have_retried = 1;
 		goto retry_get_time;
 	}
 
-	rtc_tm->tm_sec = bcd2bin(rtc_tm->tm_sec);
-	rtc_tm->tm_min = bcd2bin(rtc_tm->tm_min);
-	rtc_tm->tm_hour = bcd2bin(rtc_tm->tm_hour);
-	rtc_tm->tm_mday = bcd2bin(rtc_tm->tm_mday);
-	rtc_tm->tm_mon = bcd2bin(rtc_tm->tm_mon);
-	rtc_tm->tm_year = bcd2bin(rtc_tm->tm_year);
-
 	s3c_rtc_disable_clk(info);
 
-	rtc_tm->tm_year += 100;
-	rtc_tm->tm_mon -= 1;
+	tm->tm_sec  = bcd2bin(tm->tm_sec);
+	tm->tm_min  = bcd2bin(tm->tm_min);
+	tm->tm_hour = bcd2bin(tm->tm_hour);
+	tm->tm_mday = bcd2bin(tm->tm_mday);
+	tm->tm_mon  = bcd2bin(tm->tm_mon);
+	tm->tm_year = bcd2bin(tm->tm_year);
 
-	dev_dbg(dev, "read time %ptR\n", rtc_tm);
 	return 0;
 }
 
-static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
+/* Convert time to BCD and write it to RTC */
+static int s3c_rtc_write_time(struct s3c_rtc *info, const struct rtc_time *tm)
 {
-	struct s3c_rtc *info = dev_get_drvdata(dev);
-	int year = tm->tm_year - 100;
 	int ret;
 
-	dev_dbg(dev, "set time %ptR\n", tm);
-
-	/* we get around y2k by simply not supporting it */
-
-	if (year < 0 || year >= 100) {
-		dev_err(dev, "rtc only supports 100 years\n");
-		return -EINVAL;
-	}
-
 	ret = s3c_rtc_enable_clk(info);
 	if (ret)
 		return ret;
@@ -195,14 +180,53 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 	writeb(bin2bcd(tm->tm_min),  info->base + S3C2410_RTCMIN);
 	writeb(bin2bcd(tm->tm_hour), info->base + S3C2410_RTCHOUR);
 	writeb(bin2bcd(tm->tm_mday), info->base + S3C2410_RTCDATE);
-	writeb(bin2bcd(tm->tm_mon + 1), info->base + S3C2410_RTCMON);
-	writeb(bin2bcd(year), info->base + S3C2410_RTCYEAR);
+	writeb(bin2bcd(tm->tm_mon),  info->base + S3C2410_RTCMON);
+	writeb(bin2bcd(tm->tm_year), info->base + S3C2410_RTCYEAR);
 
 	s3c_rtc_disable_clk(info);
 
 	return 0;
 }
 
+static int s3c_rtc_gettime(struct device *dev, struct rtc_time *tm)
+{
+	struct s3c_rtc *info = dev_get_drvdata(dev);
+	int ret;
+
+	ret = s3c_rtc_read_time(info, tm);
+	if (ret)
+		return ret;
+
+	/* Convert internal representation to actual date/time */
+	tm->tm_year += 100;
+	tm->tm_mon -= 1;
+
+	dev_dbg(dev, "read time %ptR\n", tm);
+	return 0;
+}
+
+static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
+{
+	struct s3c_rtc *info = dev_get_drvdata(dev);
+	struct rtc_time rtc_tm = *tm;
+
+	dev_dbg(dev, "set time %ptR\n", tm);
+
+	/*
+	 * Convert actual date/time to internal representation.
+	 * We get around Y2K by simply not supporting it.
+	 */
+	rtc_tm.tm_year -= 100;
+	rtc_tm.tm_mon += 1;
+
+	if (rtc_tm.tm_year < 0 || rtc_tm.tm_year >= 100) {
+		dev_err(dev, "rtc only supports 100 years\n");
+		return -EINVAL;
+	}
+
+	return s3c_rtc_write_time(info, &rtc_tm);
+}
+
 static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
-- 
2.30.2

[PATCH 4/4] rtc: s3c: Fix RTC read on first boot

[Sam Protsenko]

On first RTC boot it has the month register value set to 0.
Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads
to the next error message in kernel log:

    hctosys: unable to read the hardware clock

That happens in s3c_rtc_probe() when trying to register the RTC, which
in turn tries to read and validate the time. Initialize RTC date/time
registers to valid values in probe function on the first boot to prevent
such errors.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/rtc/rtc-s3c.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 238928e29fbc..c7e763bcf61f 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -403,6 +403,28 @@ static int s3c_rtc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/* Set RTC with valid date/time values on first boot */
+static int s3c_rtc_init_time(struct s3c_rtc *info)
+{
+	struct rtc_time tm;
+	int ret;
+
+	ret = s3c_rtc_read_time(info, &tm);
+	if (ret)
+		return ret;
+
+	/* Only init RTC date/time on first boot */
+	if (tm.tm_mday > 0)
+		return 0;
+
+	/* Init date/time: 1 Jan 2000 00:00:00 */
+	memset(&tm, 0, sizeof(struct rtc_time));
+	tm.tm_mday = 1;	/* tm_mday min valid value is 1 */
+	tm.tm_mon = 1;	/* January in internal representation */
+
+	return s3c_rtc_write_time(info, &tm);
+}
+
 static int s3c_rtc_probe(struct platform_device *pdev)
 {
 	struct s3c_rtc *info = NULL;
@@ -471,6 +493,10 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 1);
 
+	ret = s3c_rtc_init_time(info);
+	if (ret)
+		goto err_nortc;
+
 	info->rtc = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(info->rtc)) {
 		ret = PTR_ERR(info->rtc);
-- 
2.30.2

Re: [PATCH 4/4] rtc: s3c: Fix RTC read on first boot

[Alexandre Belloni]

On 19/10/2021 16:17:24+0300, Sam Protsenko wrote:
> On first RTC boot it has the month register value set to 0.
> Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads
> to the next error message in kernel log:
> 
>     hctosys: unable to read the hardware clock
> 
> That happens in s3c_rtc_probe() when trying to register the RTC, which
> in turn tries to read and validate the time. Initialize RTC date/time
> registers to valid values in probe function on the first boot to prevent
> such errors.
> 

No, never ever do that, the time is bogus and it has to stay this way,
else userspace can't know whether the time on the RTC is the actual wall
time or just some random value that you have set from the driver.

> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/rtc/rtc-s3c.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index 238928e29fbc..c7e763bcf61f 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -403,6 +403,28 @@ static int s3c_rtc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +/* Set RTC with valid date/time values on first boot */
> +static int s3c_rtc_init_time(struct s3c_rtc *info)
> +{
> +	struct rtc_time tm;
> +	int ret;
> +
> +	ret = s3c_rtc_read_time(info, &tm);
> +	if (ret)
> +		return ret;
> +
> +	/* Only init RTC date/time on first boot */
> +	if (tm.tm_mday > 0)
> +		return 0;
> +
> +	/* Init date/time: 1 Jan 2000 00:00:00 */
> +	memset(&tm, 0, sizeof(struct rtc_time));
> +	tm.tm_mday = 1;	/* tm_mday min valid value is 1 */
> +	tm.tm_mon = 1;	/* January in internal representation */
> +
> +	return s3c_rtc_write_time(info, &tm);
> +}
> +
>  static int s3c_rtc_probe(struct platform_device *pdev)
>  {
>  	struct s3c_rtc *info = NULL;
> @@ -471,6 +493,10 @@ static int s3c_rtc_probe(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, 1);
>  
> +	ret = s3c_rtc_init_time(info);
> +	if (ret)
> +		goto err_nortc;
> +
>  	info->rtc = devm_rtc_allocate_device(&pdev->dev);
>  	if (IS_ERR(info->rtc)) {
>  		ret = PTR_ERR(info->rtc);
> -- 
> 2.30.2
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/4] rtc: s3c: Fix RTC read on first boot

[Sam Protsenko]

On Tue, 19 Oct 2021 at 18:48, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 19/10/2021 16:17:24+0300, Sam Protsenko wrote:
> > On first RTC boot it has the month register value set to 0.
> > Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads
> > to the next error message in kernel log:
> >
> >     hctosys: unable to read the hardware clock
> >
> > That happens in s3c_rtc_probe() when trying to register the RTC, which
> > in turn tries to read and validate the time. Initialize RTC date/time
> > registers to valid values in probe function on the first boot to prevent
> > such errors.
> >
>
> No, never ever do that, the time is bogus and it has to stay this way,
> else userspace can't know whether the time on the RTC is the actual wall
> time or just some random value that you have set from the driver.
>

Thought about that, but that error message looked distracting and not
very helpful in understanding what's actually going on. Anyway, can
you please drop this patch from series (and maybe [PATCH 3/4] too) and
apply the rest?

> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/rtc/rtc-s3c.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> > index 238928e29fbc..c7e763bcf61f 100644
> > --- a/drivers/rtc/rtc-s3c.c
> > +++ b/drivers/rtc/rtc-s3c.c
> > @@ -403,6 +403,28 @@ static int s3c_rtc_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +/* Set RTC with valid date/time values on first boot */
> > +static int s3c_rtc_init_time(struct s3c_rtc *info)
> > +{
> > +     struct rtc_time tm;
> > +     int ret;
> > +
> > +     ret = s3c_rtc_read_time(info, &tm);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Only init RTC date/time on first boot */
> > +     if (tm.tm_mday > 0)
> > +             return 0;
> > +
> > +     /* Init date/time: 1 Jan 2000 00:00:00 */
> > +     memset(&tm, 0, sizeof(struct rtc_time));
> > +     tm.tm_mday = 1; /* tm_mday min valid value is 1 */
> > +     tm.tm_mon = 1;  /* January in internal representation */
> > +
> > +     return s3c_rtc_write_time(info, &tm);
> > +}
> > +
> >  static int s3c_rtc_probe(struct platform_device *pdev)
> >  {
> >       struct s3c_rtc *info = NULL;
> > @@ -471,6 +493,10 @@ static int s3c_rtc_probe(struct platform_device *pdev)
> >
> >       device_init_wakeup(&pdev->dev, 1);
> >
> > +     ret = s3c_rtc_init_time(info);
> > +     if (ret)
> > +             goto err_nortc;
> > +
> >       info->rtc = devm_rtc_allocate_device(&pdev->dev);
> >       if (IS_ERR(info->rtc)) {
> >               ret = PTR_ERR(info->rtc);
> > --
> > 2.30.2
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH 4/4] rtc: s3c: Fix RTC read on first boot

[Krzysztof Kozlowski]

On 19/10/2021 17:48, Alexandre Belloni wrote:
> On 19/10/2021 16:17:24+0300, Sam Protsenko wrote:
>> On first RTC boot it has the month register value set to 0.
>> Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads
>> to the next error message in kernel log:
>>
>>     hctosys: unable to read the hardware clock
>>
>> That happens in s3c_rtc_probe() when trying to register the RTC, which
>> in turn tries to read and validate the time. Initialize RTC date/time
>> registers to valid values in probe function on the first boot to prevent
>> such errors.
>>
> 
> No, never ever do that, the time is bogus and it has to stay this way,
> else userspace can't know whether the time on the RTC is the actual wall
> time or just some random value that you have set from the driver.
> 

Indeed. This looks basically like a revert of your commit 5c78cceeb2d8
("rtc: s3c: stop setting bogus time"). For the Samsung PMIC RTC, we
dropped time initialization in commit fe787a5b2297 ("rtc: s5m: remove
undocumented time init on first boot").

Best regards,
Krzysztof

Re: [PATCH 1/4] rtc: s3c: Remove usage of devm_rtc_device_register()

[Krzysztof Kozlowski]

On 19/10/2021 15:17, Sam Protsenko wrote:
> devm_rtc_device_register() is deprecated. Use devm_rtc_allocate_device()
> and devm_rtc_register_device() API instead. This change doesn't change
> the behavior, but allows for further improvements.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/rtc/rtc-s3c.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH 2/4] rtc: s3c: Add time range

[Krzysztof Kozlowski]

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

Re: [PATCH 4/4] rtc: s3c: Fix RTC read on first boot

[Krzysztof Kozlowski]

On 19/10/2021 18:04, Sam Protsenko wrote:
> On Tue, 19 Oct 2021 at 18:48, Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
>>
>> On 19/10/2021 16:17:24+0300, Sam Protsenko wrote:
>>> On first RTC boot it has the month register value set to 0.
>>> Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads
>>> to the next error message in kernel log:
>>>
>>>     hctosys: unable to read the hardware clock
>>>
>>> That happens in s3c_rtc_probe() when trying to register the RTC, which
>>> in turn tries to read and validate the time. Initialize RTC date/time
>>> registers to valid values in probe function on the first boot to prevent
>>> such errors.
>>>
>>
>> No, never ever do that, the time is bogus and it has to stay this way,
>> else userspace can't know whether the time on the RTC is the actual wall
>> time or just some random value that you have set from the driver.
>>
> 
> Thought about that, but that error message looked distracting and not
> very helpful in understanding what's actually going on. Anyway, can
> you please drop this patch from series (and maybe [PATCH 3/4] too) and
> apply the rest?
> 

Please give it some time for review. Pinging after few hours is too fast.

Best regards,
Krzysztof

Re: [PATCH 2/4] rtc: s3c: Add time range

[Alexandre Belloni]

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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/4] rtc: s3c: Add time range

[Krzysztof Kozlowski]

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

Re: [PATCH 3/4] rtc: s3c: Extract read/write IO into separate functions

[Krzysztof Kozlowski]

On 19/10/2021 15:17, Sam Protsenko wrote:
> Create dedicated functions for I/O operations and BCD conversion. It can
> be useful to separete those from representation conversion and other
> stuff found in RTC callbacks, e.g. for initializing RTC registers.
> This patch does not introduce any functional changes, it's merely
> refactoring change.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/rtc/rtc-s3c.c | 98 +++++++++++++++++++++++++++----------------
>  1 file changed, 61 insertions(+), 37 deletions(-)
> 

Looks correct:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH 2/4] rtc: s3c: Add time range

[Sam Protsenko]

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

Re: [PATCH 2/4] rtc: s3c: Add time range

[Sam Protsenko]

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

Re: [PATCH 2/4] rtc: s3c: Add time range

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/4] rtc: s3c: Add time range

[Alexandre Belloni]

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).

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/4] rtc: s3c: Add time range

[Sam Protsenko]

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

Re: [PATCH 2/4] rtc: s3c: Add time range

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/4] rtc: s3c: Add time range

[Krzysztof Kozlowski]

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

Re: [PATCH 2/4] rtc: s3c: Add time range

[Sam Protsenko]

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

Re: [PATCH 2/4] rtc: s3c: Add time range

[Alexandre Belloni]

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.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com