[PATCH v2 0/3] rtc: s3c: S3C driver improvements

[PATCH v2 0/3] 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/3]: moves S3C RTC driver to newer API usage
    (no functional changes)
  * [PATCH 2/3]: refactoring/cleanup (no functional changes)
  * [PATCH 3/3]: adds time range, as [PATCH 1/3] made it possible

Changes in v2:
  - Dropped "rtc: s3c: Fix RTC read on first boot" patch
  - Switched the order for [PATCH 2/3] and [PATCH 3/3], so that the
    refactoring patch can stay w/o functional changes w.r.t. previous
    state
  - See also changelist for each particular patch

Sam Protsenko (3):
  rtc: s3c: Remove usage of devm_rtc_device_register()
  rtc: s3c: Extract read/write IO into separate functions
  rtc: s3c: Add time range

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

-- 
2.30.2

[PATCH v2 1/3] 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
Changes in v2:
 - Added R-b tag by Krzysztof Kozlowski

 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 v2 2/3] 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 separate those from representation conversion and other
stuff found in RTC callbacks.

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

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
Changes in v2:
  - Added R-b tag by Krzysztof Kozlowski
  - Changed the commit message wording a bit

 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 10e591794276..d1baf655c008 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 v2 3/3] rtc: s3c: Add time range

[Sam Protsenko]

This RTC driver starts counting from 2000 to avoid Y2K problem. Also it
only supports 100 years range for all RTCs.  Provide that info to RTC
framework. Also remove check for 100 years range in s3c_rtc_settime(),
as RTC core won't pass any invalid values to the driver, now that
correct range is set.

Here is the rationale on 100 years range limitation. Info on different
Samsung RTCs (credit goes to Krzysztof Kozlowski):
  - All S3C chips have only 8-bit wide year register (can store 100
    years range in BCD format)
  - S5Pv210 and Exynos chips have 12-bit year register (can store 1000
    years range in BCD format)

But in reality we usually can't make use of those 12 bits either:
  - RTCs might think that both 2000 and 2100 years are leap years. So
    when the YEAR register is 0, RTC goes from 28 Feb to 29 Feb, and
    when the YEAR register is 100, RTC also goes from 28 Feb to 29 Feb.
    This is of course incorrect: RTC breaks leap year criteria, which
    breaks the time contiguity, which leads to inability to use the RTC
    after year of 2099. It was found for example on Exynos850 SoC.
  - Despite having 12 bits for holding the year value, RTC might
    overflow the year value internally much earlier. For example, on
    Exynos850 the RTC overflows when YEAR=159, making the next YEAR=0.
    This way RTC actually has range of 160 years, not 1000 as one may
    think.

All that said, there is no sense in trying to increase the time range
for more than 100 years on RTCs that seem capable of that. It also
doesn't have too much practical value -- current hardware will be
probably obsolete by 2100.

Tested manually on Exynos850 RTC:

    $ date -s "1999-12-31 23:59:50"
    $ hwclock -w -f /dev/rtc0
    $ date -s "2100-01-01 00:00:00"
    $ hwclock -w -f /dev/rtc0
    $ date -s "2000-01-01 00:00:00"
    $ hwclock -w -f /dev/rtc0
    $ hwclock -r -f /dev/rtc0
    $ date -s "2099-12-31 23:59:50"
    $ hwclock -w -f /dev/rtc0
    $ hwclock -r -f /dev/rtc0

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Removed check for 100 years range in s3c_rtc_settime()
  - Improved the commit message

 drivers/rtc/rtc-s3c.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index d1baf655c008..db529733c9c4 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -219,11 +219,6 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 	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);
 }
 
@@ -478,6 +473,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

Re: [PATCH v2 3/3] rtc: s3c: Add time range

[Krzysztof Kozlowski]

On 21/10/2021 22:22, Sam Protsenko wrote:
> This RTC driver starts counting from 2000 to avoid Y2K problem. Also it
> only supports 100 years range for all RTCs.  Provide that info to RTC
> framework. Also remove check for 100 years range in s3c_rtc_settime(),
> as RTC core won't pass any invalid values to the driver, now that
> correct range is set.
> 
> Here is the rationale on 100 years range limitation. Info on different
> Samsung RTCs (credit goes to Krzysztof Kozlowski):
>   - All S3C chips have only 8-bit wide year register (can store 100
>     years range in BCD format)
>   - S5Pv210 and Exynos chips have 12-bit year register (can store 1000
>     years range in BCD format)
> 
> But in reality we usually can't make use of those 12 bits either:
>   - RTCs might think that both 2000 and 2100 years are leap years. So
>     when the YEAR register is 0, RTC goes from 28 Feb to 29 Feb, and
>     when the YEAR register is 100, RTC also goes from 28 Feb to 29 Feb.
>     This is of course incorrect: RTC breaks leap year criteria, which
>     breaks the time contiguity, which leads to inability to use the RTC
>     after year of 2099. It was found for example on Exynos850 SoC.
>   - Despite having 12 bits for holding the year value, RTC might
>     overflow the year value internally much earlier. For example, on
>     Exynos850 the RTC overflows when YEAR=159, making the next YEAR=0.
>     This way RTC actually has range of 160 years, not 1000 as one may
>     think.
> 
> All that said, there is no sense in trying to increase the time range
> for more than 100 years on RTCs that seem capable of that. It also
> doesn't have too much practical value -- current hardware will be
> probably obsolete by 2100.
> 
> Tested manually on Exynos850 RTC:
> 
>     $ date -s "1999-12-31 23:59:50"
>     $ hwclock -w -f /dev/rtc0
>     $ date -s "2100-01-01 00:00:00"
>     $ hwclock -w -f /dev/rtc0
>     $ date -s "2000-01-01 00:00:00"
>     $ hwclock -w -f /dev/rtc0
>     $ hwclock -r -f /dev/rtc0
>     $ date -s "2099-12-31 23:59:50"
>     $ hwclock -w -f /dev/rtc0
>     $ hwclock -r -f /dev/rtc0
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Removed check for 100 years range in s3c_rtc_settime()
>   - Improved the commit message
> 


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


Best regards,
Krzysztof

Re: [PATCH v2 0/3] rtc: s3c: S3C driver improvements

[Alexandre Belloni]

On Thu, 21 Oct 2021 23:22:53 +0300, Sam Protsenko wrote:
> 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/3]: moves S3C RTC driver to newer API usage
>     (no functional changes)
>   * [PATCH 2/3]: refactoring/cleanup (no functional changes)
>   * [PATCH 3/3]: adds time range, as [PATCH 1/3] made it possible
> 
> [...]

Applied, thanks!

[1/3] rtc: s3c: Remove usage of devm_rtc_device_register()
      commit: dba28c37f23a09fc32dbc37463ddb2feb3886f98
[2/3] rtc: s3c: Extract read/write IO into separate functions
      commit: e4a1444e10cbda2892a4ea7325ef5efa47c75cfb
[3/3] rtc: s3c: Add time range
      commit: a5feda3b361e11b291786d5c4ff86d4b9a55498f

Best regards,
-- 
Alexandre Belloni <alexandre.belloni@bootlin.com>

Re: [PATCH v2 0/3] rtc: s3c: S3C driver improvements

[Sam Protsenko]

On Sun, 24 Oct 2021 at 00:58, Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On Thu, 21 Oct 2021 23:22:53 +0300, Sam Protsenko wrote:
> > 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/3]: moves S3C RTC driver to newer API usage
> >     (no functional changes)
> >   * [PATCH 2/3]: refactoring/cleanup (no functional changes)
> >   * [PATCH 3/3]: adds time range, as [PATCH 1/3] made it possible
> >
> > [...]
>
> Applied, thanks!
>

Hi Alexandre,

Just want to check if this series is going to be merged during current
merge window, or is it scheduled for the next one?

Thanks!

> [1/3] rtc: s3c: Remove usage of devm_rtc_device_register()
>       commit: dba28c37f23a09fc32dbc37463ddb2feb3886f98
> [2/3] rtc: s3c: Extract read/write IO into separate functions
>       commit: e4a1444e10cbda2892a4ea7325ef5efa47c75cfb
> [3/3] rtc: s3c: Add time range
>       commit: a5feda3b361e11b291786d5c4ff86d4b9a55498f
>
> Best regards,
> --
> Alexandre Belloni <alexandre.belloni@bootlin.com>

Re: [PATCH v2 0/3] rtc: s3c: S3C driver improvements

[Alexandre Belloni]

On 11/11/2021 19:05:06+0200, Sam Protsenko wrote:
> On Sun, 24 Oct 2021 at 00:58, Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > On Thu, 21 Oct 2021 23:22:53 +0300, Sam Protsenko wrote:
> > > 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/3]: moves S3C RTC driver to newer API usage
> > >     (no functional changes)
> > >   * [PATCH 2/3]: refactoring/cleanup (no functional changes)
> > >   * [PATCH 3/3]: adds time range, as [PATCH 1/3] made it possible
> > >
> > > [...]
> >
> > Applied, thanks!
> >
> 
> Hi Alexandre,
> 
> Just want to check if this series is going to be merged during current
> merge window, or is it scheduled for the next one?
> 

This is now pulled by Linus.

> Thanks!
> 
> > [1/3] rtc: s3c: Remove usage of devm_rtc_device_register()
> >       commit: dba28c37f23a09fc32dbc37463ddb2feb3886f98
> > [2/3] rtc: s3c: Extract read/write IO into separate functions
> >       commit: e4a1444e10cbda2892a4ea7325ef5efa47c75cfb
> > [3/3] rtc: s3c: Add time range
> >       commit: a5feda3b361e11b291786d5c4ff86d4b9a55498f
> >
> > Best regards,
> > --
> > Alexandre Belloni <alexandre.belloni@bootlin.com>

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