[PATCH 0/3] rtc: rzn1: support XTAL clk and SCMP method

[PATCH 0/3] rtc: rzn1: support XTAL clk and SCMP method

[Wolfram Sang]

So far, the code and the binding for the RZ/N1D RTC assumed an input
clock of 32768Hz, so it was not explicitly described. It makes sense to
do this, though. For one reason, clocks with other frequencies might be
used. This RTC supports that via the SCMP counting method. The other
reason is, the upcoming R-Car Gen5 has only the SCMP method described,
so we need to use it there later.

This series lets the driver handle the optional "xtal" clock and switch
to the SCMP method if suitable. It has been tested on a Renesas RZ/N1D
board with hacked devicetree values.

A branch with updated DTs can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/n1d/rtc

Looking forward to comments!


Changes since RFC:
* add binding tag from Krzysztof (Thanks!)
* new patch to disable controller before initially modifying CTL0
* cosmetic changes

Wolfram Sang (3):
  dt-bindings: rtc: rzn1: add optional second clock
  rtc: rzn1: Disable controller before initialization
  rtc: rzn1: support input frequencies other than 32768Hz

 .../bindings/rtc/renesas,rzn1-rtc.yaml        |  8 ++-
 drivers/rtc/rtc-rzn1.c                        | 59 +++++++++++++++----
 2 files changed, 53 insertions(+), 14 deletions(-)

-- 
2.47.2

[PATCH 1/3] dt-bindings: rtc: rzn1: add optional second clock

[Wolfram Sang]

The external crystal can be a second clock input. It is needed for the
SCMP counting method which allows using crystals different than 32768Hz.
It is also needed for an upcoming SoC which only supports the SCMP
method.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/rtc/renesas,rzn1-rtc.yaml         | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/renesas,rzn1-rtc.yaml b/Documentation/devicetree/bindings/rtc/renesas,rzn1-rtc.yaml
index f6e0c613af67..f6fdcc7090b6 100644
--- a/Documentation/devicetree/bindings/rtc/renesas,rzn1-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/renesas,rzn1-rtc.yaml
@@ -33,10 +33,14 @@ properties:
       - const: pps
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
 
   clock-names:
-    const: hclk
+    minItems: 1
+    items:
+      - const: hclk
+      - const: xtal
 
   power-domains:
     maxItems: 1
-- 
2.47.2

[PATCH 2/3] rtc: rzn1: Disable controller before initialization

[Wolfram Sang]

Datasheet says that the controller must be disabled before setting up
either SUBU or SCMP. This did not matter so far because the driver only
supported SUBU which was the default, too. It is good practice to follow
datasheet recommendations, though. It will also be needed because SCMP
mode will be added in a later patch.

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

diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
index 3c2861983ff1..7777df1e3426 100644
--- a/drivers/rtc/rtc-rzn1.c
+++ b/drivers/rtc/rtc-rzn1.c
@@ -25,6 +25,7 @@
 #define   RZN1_RTC_CTL0_SLSB_SUBU 0
 #define   RZN1_RTC_CTL0_SLSB_SCMP BIT(4)
 #define   RZN1_RTC_CTL0_AMPM BIT(5)
+#define   RZN1_RTC_CTL0_CEST BIT(6)
 #define   RZN1_RTC_CTL0_CE BIT(7)
 
 #define RZN1_RTC_CTL1 0x04
@@ -369,6 +370,7 @@ static const struct rtc_class_ops rzn1_rtc_ops = {
 static int rzn1_rtc_probe(struct platform_device *pdev)
 {
 	struct rzn1_rtc *rtc;
+	u32 val;
 	int irq;
 	int ret;
 
@@ -406,6 +408,14 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
 	 * Ensure the clock counter is enabled.
 	 * Set 24-hour mode and possible oscillator offset compensation in SUBU mode.
 	 */
+	val = readl(rtc->base + RZN1_RTC_CTL0) & ~RZN1_RTC_CTL0_CE;
+	writel(val, rtc->base + RZN1_RTC_CTL0);
+	/* Wait 2-4 32k clock cycles for the disabled controller */
+	ret = readl_poll_timeout(rtc->base + RZN1_RTC_CTL0, val,
+				 !(val & RZN1_RTC_CTL0_CEST), 62, 123);
+	if (ret)
+		goto dis_runtime_pm;
+
 	writel(RZN1_RTC_CTL0_CE | RZN1_RTC_CTL0_AMPM | RZN1_RTC_CTL0_SLSB_SUBU,
 	       rtc->base + RZN1_RTC_CTL0);
 
-- 
2.47.2

[PATCH 3/3] rtc: rzn1: support input frequencies other than 32768Hz

[Wolfram Sang]

When using the SCMP mode instead of SUBU, this RTC can also support
other input frequencies than 32768Hz. Also, upcoming SoCs will only
support SCMP.

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

diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
index 7777df1e3426..47ab62d5380e 100644
--- a/drivers/rtc/rtc-rzn1.c
+++ b/drivers/rtc/rtc-rzn1.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/bcd.h>
+#include <linux/clk.h>
 #include <linux/init.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
@@ -22,7 +23,6 @@
 #include <linux/spinlock.h>
 
 #define RZN1_RTC_CTL0 0x00
-#define   RZN1_RTC_CTL0_SLSB_SUBU 0
 #define   RZN1_RTC_CTL0_SLSB_SCMP BIT(4)
 #define   RZN1_RTC_CTL0_AMPM BIT(5)
 #define   RZN1_RTC_CTL0_CEST BIT(6)
@@ -50,6 +50,8 @@
 #define   RZN1_RTC_SUBU_DEV BIT(7)
 #define   RZN1_RTC_SUBU_DECR BIT(6)
 
+#define RZN1_RTC_SCMP 0x3c
+
 #define RZN1_RTC_ALM 0x40
 #define RZN1_RTC_ALH 0x44
 #define RZN1_RTC_ALW 0x48
@@ -357,22 +359,21 @@ static int rzn1_rtc_set_offset(struct device *dev, long offset)
 	return 0;
 }
 
-static const struct rtc_class_ops rzn1_rtc_ops = {
+static struct rtc_class_ops rzn1_rtc_ops = {
 	.read_time = rzn1_rtc_read_time,
 	.set_time = rzn1_rtc_set_time,
 	.read_alarm = rzn1_rtc_read_alarm,
 	.set_alarm = rzn1_rtc_set_alarm,
 	.alarm_irq_enable = rzn1_rtc_alarm_irq_enable,
-	.read_offset = rzn1_rtc_read_offset,
-	.set_offset = rzn1_rtc_set_offset,
 };
 
 static int rzn1_rtc_probe(struct platform_device *pdev)
 {
 	struct rzn1_rtc *rtc;
-	u32 val;
-	int irq;
-	int ret;
+	u32 val, use_scmp = 0;
+	struct clk *xtal;
+	unsigned long rate;
+	int irq, ret;
 
 	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
 	if (!rtc)
@@ -404,10 +405,24 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Ensure the clock counter is enabled.
-	 * Set 24-hour mode and possible oscillator offset compensation in SUBU mode.
-	 */
+	/* Only switch to scmp if we have an xtal clock with a valid rate and != 32768 */
+	xtal = devm_clk_get_optional(&pdev->dev, "xtal");
+	if (IS_ERR(xtal)) {
+		ret = PTR_ERR(xtal);
+		goto dis_runtime_pm;
+	} else if (xtal) {
+		rate = clk_get_rate(xtal);
+
+		if (rate < 32000 || rate > BIT(22)) {
+			ret = -EOPNOTSUPP;
+			goto dis_runtime_pm;
+		}
+
+		if (rate != 32768)
+			use_scmp = RZN1_RTC_CTL0_SLSB_SCMP;
+	}
+
+	/* Disable controller during SUBU/SCMP setup */
 	val = readl(rtc->base + RZN1_RTC_CTL0) & ~RZN1_RTC_CTL0_CE;
 	writel(val, rtc->base + RZN1_RTC_CTL0);
 	/* Wait 2-4 32k clock cycles for the disabled controller */
@@ -416,8 +431,18 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
 	if (ret)
 		goto dis_runtime_pm;
 
-	writel(RZN1_RTC_CTL0_CE | RZN1_RTC_CTL0_AMPM | RZN1_RTC_CTL0_SLSB_SUBU,
-	       rtc->base + RZN1_RTC_CTL0);
+	/* Set desired modes leaving the controller disabled */
+	writel(RZN1_RTC_CTL0_AMPM | use_scmp, rtc->base + RZN1_RTC_CTL0);
+
+	if (use_scmp) {
+		writel(rate - 1, rtc->base + RZN1_RTC_SCMP);
+	} else {
+		rzn1_rtc_ops.read_offset = rzn1_rtc_read_offset;
+		rzn1_rtc_ops.set_offset = rzn1_rtc_set_offset;
+	}
+
+	/* Enable controller finally */
+	writel(RZN1_RTC_CTL0_CE | RZN1_RTC_CTL0_AMPM | use_scmp, rtc->base + RZN1_RTC_CTL0);
 
 	/* Disable all interrupts */
 	writel(0, rtc->base + RZN1_RTC_CTL1);
-- 
2.47.2

Re: [PATCH 1/3] dt-bindings: rtc: rzn1: add optional second clock

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, 19 Mar 2025 at 12:03, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> The external crystal can be a second clock input. It is needed for the
> SCMP counting method which allows using crystals different than 32768Hz.
> It is also needed for an upcoming SoC which only supports the SCMP
> method.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/Documentation/devicetree/bindings/rtc/renesas,rzn1-rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/renesas,rzn1-rtc.yaml
> @@ -33,10 +33,14 @@ properties:
>        - const: pps
>
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
>
>    clock-names:
> -    const: hclk
> +    minItems: 1
> +    items:
> +      - const: hclk
> +      - const: xtal

Shouldn't the second clock become required? Or do you plan to make
that change after all upstream DTS files have been updated?

This is orthogonal to the driver considering it optional to maintain
compatibility with old DTBs.

>
>    power-domains:
>      maxItems: 1

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] rtc: rzn1: support input frequencies other than 32768Hz

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, 19 Mar 2025 at 12:04, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> When using the SCMP mode instead of SUBU, this RTC can also support
> other input frequencies than 32768Hz. Also, upcoming SoCs will only
> support SCMP.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Suggestions for improvement below...

> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -404,10 +405,24 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 return ret;
>
> -       /*
> -        * Ensure the clock counter is enabled.
> -        * Set 24-hour mode and possible oscillator offset compensation in SUBU mode.
> -        */
> +       /* Only switch to scmp if we have an xtal clock with a valid rate and != 32768 */
> +       xtal = devm_clk_get_optional(&pdev->dev, "xtal");
> +       if (IS_ERR(xtal)) {
> +               ret = PTR_ERR(xtal);
> +               goto dis_runtime_pm;
> +       } else if (xtal) {
> +               rate = clk_get_rate(xtal);
> +
> +               if (rate < 32000 || rate > BIT(22)) {

Perhaps

    #define RTCA0SCMP_MIN    32000
    #define RTCA0SCMP_MASK    GEN_MASK(21. 0)

and

    if (rate < RTCA0SCMP_MIN || rate > FIELD_MAX(RTCA0SCMP_MASK) + 1)

?

> +                       ret = -EOPNOTSUPP;
> +                       goto dis_runtime_pm;
> +               }
> +
> +               if (rate != 32768)
> +                       use_scmp = RZN1_RTC_CTL0_SLSB_SCMP;
> +       }
> +
> +       /* Disable controller during SUBU/SCMP setup */
>         val = readl(rtc->base + RZN1_RTC_CTL0) & ~RZN1_RTC_CTL0_CE;
>         writel(val, rtc->base + RZN1_RTC_CTL0);
>         /* Wait 2-4 32k clock cycles for the disabled controller */

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] dt-bindings: rtc: rzn1: add optional second clock

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, 10 Apr 2025 at 17:08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, 19 Mar 2025 at 12:03, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > The external crystal can be a second clock input. It is needed for the
> > SCMP counting method which allows using crystals different than 32768Hz.
> > It is also needed for an upcoming SoC which only supports the SCMP
> > method.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> > --- a/Documentation/devicetree/bindings/rtc/renesas,rzn1-rtc.yaml
> > +++ b/Documentation/devicetree/bindings/rtc/renesas,rzn1-rtc.yaml
> > @@ -33,10 +33,14 @@ properties:
> >        - const: pps
> >
> >    clocks:
> > -    maxItems: 1
> > +    minItems: 1
> > +    maxItems: 2
> >
> >    clock-names:
> > -    const: hclk
> > +    minItems: 1
> > +    items:
> > +      - const: hclk
> > +      - const: xtal
>
> Shouldn't the second clock become required? Or do you plan to make
> that change after all upstream DTS files have been updated?

Upon second thought: this xtal clock is documented to be the "rtc"
input to the RZ/N1 system controller[1], so it looks like the original
idea was to obtain it through the system controller.  Unfortunately
the clock driver[2] does not use the rtc input clock, nor provides it
to consumers.

So either we fix that, or we go with your solution...

[1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml#L32
[2] https://elixir.bootlin.com/linux/latest/source/drivers/clk/renesas/r9a06g032-clocks.c

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] rtc: rzn1: support input frequencies other than 32768Hz

[Wolfram Sang]

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


> > +               if (rate < 32000 || rate > BIT(22)) {
> 
> Perhaps
> 
>     #define RTCA0SCMP_MIN    32000
>     #define RTCA0SCMP_MASK    GEN_MASK(21. 0)
> 
> and
> 
>     if (rate < RTCA0SCMP_MIN || rate > FIELD_MAX(RTCA0SCMP_MASK) + 1)
> 
> ?

You really think this is more readable than the original code? I am
really tired of bike-shedding so I don't care much, but I do wonder...


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

Re: [PATCH 1/3] dt-bindings: rtc: rzn1: add optional second clock

[Wolfram Sang]

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


> > >    clock-names:
> > > -    const: hclk
> > > +    minItems: 1
> > > +    items:
> > > +      - const: hclk
> > > +      - const: xtal
> >
> > Shouldn't the second clock become required? Or do you plan to make
> > that change after all upstream DTS files have been updated?

True, we should make the second clock a requirement from now on.

> Upon second thought: this xtal clock is documented to be the "rtc"
> input to the RZ/N1 system controller[1], so it looks like the original
> idea was to obtain it through the system controller.  Unfortunately
> the clock driver[2] does not use the rtc input clock, nor provides it
> to consumers.

So, it would basically be a pass-through? I don't see any register in
SYSCTRL handling the external RTC clock.

> So either we fix that, or we go with your solution...

If it is a pass-through, I wonder what it would gain us, but I can do
that if there are reasons for it.


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

Re: [PATCH 3/3] rtc: rzn1: support input frequencies other than 32768Hz

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, 10 Apr 2025 at 22:09, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > +               if (rate < 32000 || rate > BIT(22)) {
> >
> > Perhaps
> >
> >     #define RTCA0SCMP_MIN    32000
> >     #define RTCA0SCMP_MASK    GEN_MASK(21. 0)
> >
> > and
> >
> >     if (rate < RTCA0SCMP_MIN || rate > FIELD_MAX(RTCA0SCMP_MASK) + 1)
> >
> > ?
>
> You really think this is more readable than the original code? I am
> really tired of bike-shedding so I don't care much, but I do wonder...

I don't mind the literal 32000, but the "> BIT(22)" looks rather
obscure, IMO.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] dt-bindings: rtc: rzn1: add optional second clock

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, 10 Apr 2025 at 22:23, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > >    clock-names:
> > > > -    const: hclk
> > > > +    minItems: 1
> > > > +    items:
> > > > +      - const: hclk
> > > > +      - const: xtal
> > >
> > > Shouldn't the second clock become required? Or do you plan to make
> > > that change after all upstream DTS files have been updated?
>
> True, we should make the second clock a requirement from now on.
>
> > Upon second thought: this xtal clock is documented to be the "rtc"
> > input to the RZ/N1 system controller[1], so it looks like the original
> > idea was to obtain it through the system controller.  Unfortunately
> > the clock driver[2] does not use the rtc input clock, nor provides it
> > to consumers.
>
> So, it would basically be a pass-through? I don't see any register in
> SYSCTRL handling the external RTC clock.

I assume you are right, I didn't study RZ/N1D in detail.

> > So either we fix that, or we go with your solution...
>
> If it is a pass-through, I wonder what it would gain us, but I can do
> that if there are reasons for it.

Let's go for your solution.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] dt-bindings: rtc: rzn1: add optional second clock

[Wolfram Sang]

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


> > If it is a pass-through, I wonder what it would gain us, but I can do
> > that if there are reasons for it.
> 
> Let's go for your solution.

Thanks!


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

Re: [PATCH 3/3] rtc: rzn1: support input frequencies other than 32768Hz

[Wolfram Sang]

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


> > >     if (rate < RTCA0SCMP_MIN || rate > FIELD_MAX(RTCA0SCMP_MASK) + 1)
> > >
> > > ?
> >
> > You really think this is more readable than the original code? I am
> > really tired of bike-shedding so I don't care much, but I do wonder...
> 
> I don't mind the literal 32000, but the "> BIT(22)" looks rather
> obscure, IMO.

Really? Ok, then it is up to what Alexandre prefers, I guess...


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

Re: [PATCH 3/3] rtc: rzn1: support input frequencies other than 32768Hz

[Wolfram Sang]

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

On Fri, Apr 11, 2025 at 08:39:49AM +0200, Wolfram Sang wrote:
> 
> > > >     if (rate < RTCA0SCMP_MIN || rate > FIELD_MAX(RTCA0SCMP_MASK) + 1)
> > > >
> > > > ?
> > >
> > > You really think this is more readable than the original code? I am
> > > really tired of bike-shedding so I don't care much, but I do wonder...
> > 
> > I don't mind the literal 32000, but the "> BIT(22)" looks rather
> > obscure, IMO.
> 
> Really? Ok, then it is up to what Alexandre prefers, I guess...

Alexandre, what is your preference? Everything else has been sorted out
by now AFAICS...


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