[RFC PATCH 0/2] rtc: rzn1: simplify driver

[RFC PATCH 0/2] rtc: rzn1: simplify driver

[Wolfram Sang]

Here are two patches to simplify the driver for RZ/N1D. They seem to
work well on my test board. I am still new to this subsystem, so I mark
the patches as RFC because my tests might have missed something.
Feedback from the maintainers would be much appreciated.

Thanks and happy hacking!


Wolfram Sang (2):
  rtc: rzn1: drop superfluous wday calculation
  rtc: rzn1: reduce register access

 drivers/rtc/rtc-rzn1.c | 86 +++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 55 deletions(-)

-- 
2.39.2

[RFC PATCH 1/2] rtc: rzn1: drop superfluous wday calculation

[Wolfram Sang]

The week register simply counts from 0 to 6 where the numbers do not
even represent a specific weekday. So we can adopt 'tm_wday' numbering
of the RTC core without converting it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/rtc/rtc-rzn1.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
index b0ea2847e982..4ae6e349faa0 100644
--- a/drivers/rtc/rtc-rzn1.c
+++ b/drivers/rtc/rtc-rzn1.c
@@ -75,19 +75,6 @@ static void rzn1_rtc_get_time_snapshot(struct rzn1_rtc *rtc, struct rtc_time *tm
 	tm->tm_year = readl(rtc->base + RZN1_RTC_YEARC);
 }
 
-static unsigned int rzn1_rtc_tm_to_wday(struct rtc_time *tm)
-{
-	time64_t time;
-	unsigned int days;
-	u32 secs;
-
-	time = rtc_tm_to_time64(tm);
-	days = div_s64_rem(time, 86400, &secs);
-
-	/* day of the week, 1970-01-01 was a Thursday */
-	return (days + 4) % 7;
-}
-
 static int rzn1_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
@@ -109,7 +96,6 @@ static int rzn1_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	tm->tm_sec = bcd2bin(tm->tm_sec);
 	tm->tm_min = bcd2bin(tm->tm_min);
 	tm->tm_hour = bcd2bin(tm->tm_hour);
-	tm->tm_wday = bcd2bin(tm->tm_wday);
 	tm->tm_mday = bcd2bin(tm->tm_mday);
 	tm->tm_mon = bcd2bin(tm->tm_mon) - 1;
 	tm->tm_year = bcd2bin(tm->tm_year) + 100;
@@ -126,7 +112,6 @@ static int rzn1_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	tm->tm_sec = bin2bcd(tm->tm_sec);
 	tm->tm_min = bin2bcd(tm->tm_min);
 	tm->tm_hour = bin2bcd(tm->tm_hour);
-	tm->tm_wday = bin2bcd(rzn1_rtc_tm_to_wday(tm));
 	tm->tm_mday = bin2bcd(tm->tm_mday);
 	tm->tm_mon = bin2bcd(tm->tm_mon + 1);
 	tm->tm_year = bin2bcd(tm->tm_year - 100);
-- 
2.39.2

[RFC PATCH 2/2] rtc: rzn1: reduce register access

[Wolfram Sang]

This RTC has special 32bit registers which return multiple of the same
8bit registers at once. Use these to minimize register access. Also, do
the to/from BCD conversions right away, so 'tm' always contains values
as described in time.h.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/rtc/rtc-rzn1.c | 75 +++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 42 deletions(-)

diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
index 4ae6e349faa0..cb220807d925 100644
--- a/drivers/rtc/rtc-rzn1.c
+++ b/drivers/rtc/rtc-rzn1.c
@@ -35,13 +35,13 @@
 #define   RZN1_RTC_CTL2_WUST BIT(5)
 #define   RZN1_RTC_CTL2_STOPPED (RZN1_RTC_CTL2_WAIT | RZN1_RTC_CTL2_WST)
 
-#define RZN1_RTC_SEC 0x14
-#define RZN1_RTC_MIN 0x18
-#define RZN1_RTC_HOUR 0x1c
-#define RZN1_RTC_WEEK 0x20
-#define RZN1_RTC_DAY 0x24
-#define RZN1_RTC_MONTH 0x28
-#define RZN1_RTC_YEAR 0x2c
+#define RZN1_RTC_TIME 0x30
+#define RZN1_RTC_TIME_MIN_SHIFT 8
+#define RZN1_RTC_TIME_HOUR_SHIFT 16
+#define RZN1_RTC_CAL 0x34
+#define RZN1_RTC_CAL_DAY_SHIFT 8
+#define RZN1_RTC_CAL_MON_SHIFT 16
+#define RZN1_RTC_CAL_YEAR_SHIFT 24
 
 #define RZN1_RTC_SUBU 0x38
 #define   RZN1_RTC_SUBU_DEV BIT(7)
@@ -52,12 +52,8 @@
 #define RZN1_RTC_ALW 0x48
 
 #define RZN1_RTC_SECC 0x4c
-#define RZN1_RTC_MINC 0x50
-#define RZN1_RTC_HOURC 0x54
-#define RZN1_RTC_WEEKC 0x58
-#define RZN1_RTC_DAYC 0x5c
-#define RZN1_RTC_MONTHC 0x60
-#define RZN1_RTC_YEARC 0x64
+#define RZN1_RTC_TIMEC 0x68
+#define RZN1_RTC_CALC 0x6c
 
 struct rzn1_rtc {
 	struct rtc_device *rtcdev;
@@ -66,13 +62,18 @@ struct rzn1_rtc {
 
 static void rzn1_rtc_get_time_snapshot(struct rzn1_rtc *rtc, struct rtc_time *tm)
 {
-	tm->tm_sec = readl(rtc->base + RZN1_RTC_SECC);
-	tm->tm_min = readl(rtc->base + RZN1_RTC_MINC);
-	tm->tm_hour = readl(rtc->base + RZN1_RTC_HOURC);
-	tm->tm_wday = readl(rtc->base + RZN1_RTC_WEEKC);
-	tm->tm_mday = readl(rtc->base + RZN1_RTC_DAYC);
-	tm->tm_mon = readl(rtc->base + RZN1_RTC_MONTHC);
-	tm->tm_year = readl(rtc->base + RZN1_RTC_YEARC);
+	u32 val;
+
+	val = readl(rtc->base + RZN1_RTC_TIMEC);
+	tm->tm_sec = bcd2bin(val);
+	tm->tm_min = bcd2bin(val >> RZN1_RTC_TIME_MIN_SHIFT);
+	tm->tm_hour = bcd2bin(val >> RZN1_RTC_TIME_HOUR_SHIFT);
+
+	val = readl(rtc->base + RZN1_RTC_CALC);
+	tm->tm_wday = val & 0x0f;
+	tm->tm_mday = bcd2bin(val >> RZN1_RTC_CAL_DAY_SHIFT);
+	tm->tm_mon = bcd2bin(val >> RZN1_RTC_CAL_MON_SHIFT) - 1;
+	tm->tm_year = bcd2bin(val >> RZN1_RTC_CAL_YEAR_SHIFT) + 100;
 }
 
 static int rzn1_rtc_read_time(struct device *dev, struct rtc_time *tm)
@@ -90,16 +91,9 @@ static int rzn1_rtc_read_time(struct device *dev, struct rtc_time *tm)
 
 	rzn1_rtc_get_time_snapshot(rtc, tm);
 	secs = readl(rtc->base + RZN1_RTC_SECC);
-	if (tm->tm_sec != secs)
+	if (tm->tm_sec != bcd2bin(secs))
 		rzn1_rtc_get_time_snapshot(rtc, tm);
 
-	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) - 1;
-	tm->tm_year = bcd2bin(tm->tm_year) + 100;
-
 	return 0;
 }
 
@@ -109,13 +103,6 @@ static int rzn1_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	u32 val;
 	int ret;
 
-	tm->tm_sec = bin2bcd(tm->tm_sec);
-	tm->tm_min = bin2bcd(tm->tm_min);
-	tm->tm_hour = bin2bcd(tm->tm_hour);
-	tm->tm_mday = bin2bcd(tm->tm_mday);
-	tm->tm_mon = bin2bcd(tm->tm_mon + 1);
-	tm->tm_year = bin2bcd(tm->tm_year - 100);
-
 	val = readl(rtc->base + RZN1_RTC_CTL2);
 	if (!(val & RZN1_RTC_CTL2_STOPPED)) {
 		/* Hold the counter if it was counting up */
@@ -129,13 +116,17 @@ static int rzn1_rtc_set_time(struct device *dev, struct rtc_time *tm)
 			return ret;
 	}
 
-	writel(tm->tm_sec, rtc->base + RZN1_RTC_SEC);
-	writel(tm->tm_min, rtc->base + RZN1_RTC_MIN);
-	writel(tm->tm_hour, rtc->base + RZN1_RTC_HOUR);
-	writel(tm->tm_wday, rtc->base + RZN1_RTC_WEEK);
-	writel(tm->tm_mday, rtc->base + RZN1_RTC_DAY);
-	writel(tm->tm_mon, rtc->base + RZN1_RTC_MONTH);
-	writel(tm->tm_year, rtc->base + RZN1_RTC_YEAR);
+	val = bin2bcd(tm->tm_sec);
+	val |= bin2bcd(tm->tm_min) << RZN1_RTC_TIME_MIN_SHIFT;
+	val |= bin2bcd(tm->tm_hour) << RZN1_RTC_TIME_HOUR_SHIFT;
+	writel(val, rtc->base + RZN1_RTC_TIME);
+
+	val = tm->tm_wday;
+	val |= bin2bcd(tm->tm_mday) << RZN1_RTC_CAL_DAY_SHIFT;
+	val |= bin2bcd(tm->tm_mon + 1) << RZN1_RTC_CAL_MON_SHIFT;
+	val |= bin2bcd(tm->tm_year - 100) << RZN1_RTC_CAL_YEAR_SHIFT;
+	writel(val, rtc->base + RZN1_RTC_CAL);
+
 	writel(0, rtc->base + RZN1_RTC_CTL2);
 
 	return 0;
-- 
2.39.2

Re: [RFC PATCH 2/2] rtc: rzn1: reduce register access

[Miquel Raynal]

On 22/11/2024 at 11:14:48 +01, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> This RTC has special 32bit registers which return multiple of the same
> 8bit registers at once. Use these to minimize register access. Also, do
> the to/from BCD conversions right away, so 'tm' always contains values
> as described in time.h.

IIRC it was easier to write the other way, but it is probably more optimized
like that, so

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

However I don't feel comfortable commenting on patch 1, I'll let Alex check.

>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---

Thanks,
Miquèl

Re: [RFC PATCH 1/2] rtc: rzn1: drop superfluous wday calculation

[Alexandre Belloni]

On 22/11/2024 11:14:47+0100, Wolfram Sang wrote:
> The week register simply counts from 0 to 6 where the numbers do not
> even represent a specific weekday. So we can adopt 'tm_wday' numbering
> of the RTC core without converting it.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/rtc/rtc-rzn1.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index b0ea2847e982..4ae6e349faa0 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -75,19 +75,6 @@ static void rzn1_rtc_get_time_snapshot(struct rzn1_rtc *rtc, struct rtc_time *tm
>  	tm->tm_year = readl(rtc->base + RZN1_RTC_YEARC);
>  }
>  
> -static unsigned int rzn1_rtc_tm_to_wday(struct rtc_time *tm)
> -{
> -	time64_t time;
> -	unsigned int days;
> -	u32 secs;
> -
> -	time = rtc_tm_to_time64(tm);
> -	days = div_s64_rem(time, 86400, &secs);
> -
> -	/* day of the week, 1970-01-01 was a Thursday */
> -	return (days + 4) % 7;
> -}
> -
>  static int rzn1_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
> @@ -109,7 +96,6 @@ static int rzn1_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	tm->tm_sec = bcd2bin(tm->tm_sec);
>  	tm->tm_min = bcd2bin(tm->tm_min);
>  	tm->tm_hour = bcd2bin(tm->tm_hour);
> -	tm->tm_wday = bcd2bin(tm->tm_wday);

With this, you're not even using wday anymore. This is fine as there are
probably no userspace users of the value but the commit message claims
it is now using it without conversion.

>  	tm->tm_mday = bcd2bin(tm->tm_mday);
>  	tm->tm_mon = bcd2bin(tm->tm_mon) - 1;
>  	tm->tm_year = bcd2bin(tm->tm_year) + 100;
> @@ -126,7 +112,6 @@ static int rzn1_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	tm->tm_sec = bin2bcd(tm->tm_sec);
>  	tm->tm_min = bin2bcd(tm->tm_min);
>  	tm->tm_hour = bin2bcd(tm->tm_hour);
> -	tm->tm_wday = bin2bcd(rzn1_rtc_tm_to_wday(tm));
>  	tm->tm_mday = bin2bcd(tm->tm_mday);
>  	tm->tm_mon = bin2bcd(tm->tm_mon + 1);
>  	tm->tm_year = bin2bcd(tm->tm_year - 100);
> -- 
> 2.39.2
> 

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

Re: [RFC PATCH 1/2] rtc: rzn1: drop superfluous wday calculation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]


> >  	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
> > @@ -109,7 +96,6 @@ static int rzn1_rtc_read_time(struct device *dev, struct rtc_time *tm)
> >  	tm->tm_sec = bcd2bin(tm->tm_sec);
> >  	tm->tm_min = bcd2bin(tm->tm_min);
> >  	tm->tm_hour = bcd2bin(tm->tm_hour);
> > -	tm->tm_wday = bcd2bin(tm->tm_wday);
> 
> With this, you're not even using wday anymore. This is fine as there are
> probably no userspace users of the value but the commit message claims
> it is now using it without conversion.

But it is still read form and written to the register. And the values of
the register go from 0 to 6, same as tm_wday. So not even BCD conversion
is necessary. So, I think it is still used. Am I missing something?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 1/2] rtc: rzn1: drop superfluous wday calculation

[Alexandre Belloni]

On 23/11/2024 09:01:19+0100, Wolfram Sang wrote:
> 
> > >  	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
> > > @@ -109,7 +96,6 @@ static int rzn1_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > >  	tm->tm_sec = bcd2bin(tm->tm_sec);
> > >  	tm->tm_min = bcd2bin(tm->tm_min);
> > >  	tm->tm_hour = bcd2bin(tm->tm_hour);
> > > -	tm->tm_wday = bcd2bin(tm->tm_wday);
> > 
> > With this, you're not even using wday anymore. This is fine as there are
> > probably no userspace users of the value but the commit message claims
> > it is now using it without conversion.
> 
> But it is still read form and written to the register. And the values of
> the register go from 0 to 6, same as tm_wday. So not even BCD conversion
> is necessary. So, I think it is still used. Am I missing something?
> 

You didn't, I misread the diff.



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

Re: [RFC PATCH 1/2] rtc: rzn1: drop superfluous wday calculation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]


> > is necessary. So, I think it is still used. Am I missing something?
> 
> You didn't, I misread the diff.

Ah, good. This kind of justifies my change in patch 2 to put
BCD-conversion and register access in one place, it seems :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] rtc: rzn1: simplify driver

[Alexandre Belloni]

On Fri, 22 Nov 2024 11:14:46 +0100, Wolfram Sang wrote:
> Here are two patches to simplify the driver for RZ/N1D. They seem to
> work well on my test board. I am still new to this subsystem, so I mark
> the patches as RFC because my tests might have missed something.
> Feedback from the maintainers would be much appreciated.
> 
> Thanks and happy hacking!
> 
> [...]

Applied, thanks!

[1/2] rtc: rzn1: drop superfluous wday calculation
      https://git.kernel.org/abelloni/c/692f983b2dc9
[2/2] rtc: rzn1: reduce register access
      https://git.kernel.org/abelloni/c/3ed345c948ef

Best regards,

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