[v2] add NXP RTC driver support for S32G2/S32G3 SoCs

[PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

Posted by Ciprian Costea 1 month, 1 week ago

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.

Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 ++++++++++++++++++
 1 file changed, 102 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml

diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
new file mode 100644
index 000000000000..3a77d4dd8f3d
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
@@ -0,0 +1,102 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP S32G2/S32G3 Real Time Clock (RTC)
+
+maintainers:
+  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
+  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - nxp,s32g2-rtc
+      - items:
+          - const: nxp,s32g3-rtc
+          - const: nxp,s32g2-rtc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#clock-cells":
+    const: 1
+
+  clocks:
+    items:
+      - description: ipg clock drives the access to the
+          RTC iomapped registers
+
+  clock-names:
+    items:
+      - const: ipg
+
+  assigned-clocks:
+    minItems: 1
+    items:
+      - description: Runtime clock source. It must be a clock
+            source for the RTC module. It will be disabled by hardware
+            during Standby/Suspend.
+      - description: Standby/Suspend clock source. It is optional
+            and can be used in case the RTC will continue ticking during
+            platform/system suspend. RTC hardware module contains a
+            hardware mux for clock source selection.
+
+  assigned-clock-parents:
+    description: List of phandles to each parent clock.
+
+  assigned-clock-rates:
+    description: List of frequencies for RTC clock sources.
+            RTC module contains 2 hardware divisors which can be
+            enabled or not. Hence, available frequencies are the following
+            parent_freq, parent_freq / 512, parent_freq / 32 or
+            parent_freq / (512 * 32)
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#clock-cells"
+  - clocks
+  - clock-names
+  - assigned-clocks
+  - assigned-clock-parents
+  - assigned-clock-rates
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    rtc0: rtc@40060000 {
+        compatible = "nxp,s32g3-rtc",
+                   "nxp,s32g2-rtc";
+        reg = <0x40060000 0x1000>;
+        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
+        #clock-cells = <1>;
+        clocks = <&clks 54>;
+        clock-names = "ipg";
+        /*
+         * Configuration of default parent clocks.
+         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
+         * 4-7 IDs are Suspend/Standby clock sources.
+         */
+        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
+        assigned-clock-parents = <&clks 56>, <&clks 55>;
+        /*
+         * Clock frequency can be divided by value
+         * 512 or 32 (or both) via hardware divisors.
+         * Below configuration:
+         * Runtime clock source: FIRC (51 MHz) / 512 (DIV512)
+         * Suspend/Standby clock source: SIRC (32 KHz)
+         */
+        assigned-clock-rates = <99609>, <32000>;
+    };
-- 
2.45.2

Re: [PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

Posted by Rob Herring 1 month, 1 week ago

On Tue, Oct 15, 2024 at 01:51:30PM +0300, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
> 
> Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> ---
>  .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 ++++++++++++++++++
>  1 file changed, 102 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> new file mode 100644
> index 000000000000..3a77d4dd8f3d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> @@ -0,0 +1,102 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32G2/S32G3 Real Time Clock (RTC)
> +
> +maintainers:
> +  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
> +  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - nxp,s32g2-rtc
> +      - items:
> +          - const: nxp,s32g3-rtc
> +          - const: nxp,s32g2-rtc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  clocks:
> +    items:
> +      - description: ipg clock drives the access to the
> +          RTC iomapped registers
> +
> +  clock-names:
> +    items:
> +      - const: ipg
> +
> +  assigned-clocks:
> +    minItems: 1
> +    items:
> +      - description: Runtime clock source. It must be a clock
> +            source for the RTC module. It will be disabled by hardware
> +            during Standby/Suspend.
> +      - description: Standby/Suspend clock source. It is optional
> +            and can be used in case the RTC will continue ticking during
> +            platform/system suspend. RTC hardware module contains a
> +            hardware mux for clock source selection.

If the RTC h/w contains a mux, then your mux inputs should be listed in 
'clocks', not here.

> +
> +  assigned-clock-parents:
> +    description: List of phandles to each parent clock.
> +
> +  assigned-clock-rates:
> +    description: List of frequencies for RTC clock sources.
> +            RTC module contains 2 hardware divisors which can be
> +            enabled or not. Hence, available frequencies are the following
> +            parent_freq, parent_freq / 512, parent_freq / 32 or
> +            parent_freq / (512 * 32)

In general, assigned-clocks* do not need to be documented and should 
never be required.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#clock-cells"
> +  - clocks
> +  - clock-names
> +  - assigned-clocks
> +  - assigned-clock-parents
> +  - assigned-clock-rates
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    rtc0: rtc@40060000 {
> +        compatible = "nxp,s32g3-rtc",
> +                   "nxp,s32g2-rtc";
> +        reg = <0x40060000 0x1000>;
> +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
> +        #clock-cells = <1>;
> +        clocks = <&clks 54>;
> +        clock-names = "ipg";
> +        /*
> +         * Configuration of default parent clocks.
> +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
> +         * 4-7 IDs are Suspend/Standby clock sources.
> +         */
> +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;

That's weird...

> +        assigned-clock-parents = <&clks 56>, <&clks 55>;

I'd expect these should be in 'clocks'. I don't think this node should 
be a clock provider unless it provides a clock to something outside the 
RTC.

Looks like you are just using assigned-clocks to configure the clock mux 
in the RTC. That's way over complicated. Just define a vendor specific 
property with the mux settings. 

> +        /*
> +         * Clock frequency can be divided by value
> +         * 512 or 32 (or both) via hardware divisors.
> +         * Below configuration:
> +         * Runtime clock source: FIRC (51 MHz) / 512 (DIV512)
> +         * Suspend/Standby clock source: SIRC (32 KHz)
> +         */
> +        assigned-clock-rates = <99609>, <32000>;
> +    };
> -- 
> 2.45.2
>

Re: [PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

Posted by Rob Herring 1 month, 1 week ago

On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote:
> On Tue, Oct 15, 2024 at 01:51:30PM +0300, Ciprian Costea wrote:
> > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > 
> > This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
> > 
> > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > ---
> >  .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 ++++++++++++++++++
> >  1 file changed, 102 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > new file mode 100644
> > index 000000000000..3a77d4dd8f3d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > @@ -0,0 +1,102 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP S32G2/S32G3 Real Time Clock (RTC)
> > +
> > +maintainers:
> > +  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
> > +  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - enum:
> > +          - nxp,s32g2-rtc
> > +      - items:
> > +          - const: nxp,s32g3-rtc
> > +          - const: nxp,s32g2-rtc
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: ipg clock drives the access to the
> > +          RTC iomapped registers
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ipg
> > +
> > +  assigned-clocks:
> > +    minItems: 1
> > +    items:
> > +      - description: Runtime clock source. It must be a clock
> > +            source for the RTC module. It will be disabled by hardware
> > +            during Standby/Suspend.
> > +      - description: Standby/Suspend clock source. It is optional
> > +            and can be used in case the RTC will continue ticking during
> > +            platform/system suspend. RTC hardware module contains a
> > +            hardware mux for clock source selection.
> 
> If the RTC h/w contains a mux, then your mux inputs should be listed in 
> 'clocks', not here.
> 
> > +
> > +  assigned-clock-parents:
> > +    description: List of phandles to each parent clock.
> > +
> > +  assigned-clock-rates:
> > +    description: List of frequencies for RTC clock sources.
> > +            RTC module contains 2 hardware divisors which can be
> > +            enabled or not. Hence, available frequencies are the following
> > +            parent_freq, parent_freq / 512, parent_freq / 32 or
> > +            parent_freq / (512 * 32)
> 
> In general, assigned-clocks* do not need to be documented and should 
> never be required.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - "#clock-cells"
> > +  - clocks
> > +  - clock-names
> > +  - assigned-clocks
> > +  - assigned-clock-parents
> > +  - assigned-clock-rates
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    rtc0: rtc@40060000 {
> > +        compatible = "nxp,s32g3-rtc",
> > +                   "nxp,s32g2-rtc";
> > +        reg = <0x40060000 0x1000>;
> > +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
> > +        #clock-cells = <1>;
> > +        clocks = <&clks 54>;
> > +        clock-names = "ipg";
> > +        /*
> > +         * Configuration of default parent clocks.
> > +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
> > +         * 4-7 IDs are Suspend/Standby clock sources.
> > +         */
> > +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
> 
> That's weird...
> 
> > +        assigned-clock-parents = <&clks 56>, <&clks 55>;
> 
> I'd expect these should be in 'clocks'. I don't think this node should 
> be a clock provider unless it provides a clock to something outside the 
> RTC.
> 
> Looks like you are just using assigned-clocks to configure the clock mux 
> in the RTC. That's way over complicated. Just define a vendor specific 
> property with the mux settings. 

I just read v1 and got told use the clock framework...

I disagree completely. Tons of h/w blocks have the ability to select 
(internal to the block) from multiple clock sources. Making the block a 
clock provider to itself is completely pointless and an overkill, and 
we *never* do that. Any display controller or audio interface has 
mutiple clock sources as just 2 examples.

However, I don't see why you need the divider config in DT. Can't you 
figure out what divider you need based on input frequency? The output 
frequency should be fixed, right?

Rob

Re: [PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

Posted by Alexandre Belloni 1 month, 1 week ago

On 15/10/2024 16:27:17-0500, Rob Herring wrote:
> On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote:
> > On Tue, Oct 15, 2024 at 01:51:30PM +0300, Ciprian Costea wrote:
> > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > 
> > > This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
> > > 
> > > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> > > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> > > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > > Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > ---
> > >  .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 ++++++++++++++++++
> > >  1 file changed, 102 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > new file mode 100644
> > > index 000000000000..3a77d4dd8f3d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > @@ -0,0 +1,102 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: NXP S32G2/S32G3 Real Time Clock (RTC)
> > > +
> > > +maintainers:
> > > +  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
> > > +  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - enum:
> > > +          - nxp,s32g2-rtc
> > > +      - items:
> > > +          - const: nxp,s32g3-rtc
> > > +          - const: nxp,s32g2-rtc
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  "#clock-cells":
> > > +    const: 1
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: ipg clock drives the access to the
> > > +          RTC iomapped registers
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: ipg
> > > +
> > > +  assigned-clocks:
> > > +    minItems: 1
> > > +    items:
> > > +      - description: Runtime clock source. It must be a clock
> > > +            source for the RTC module. It will be disabled by hardware
> > > +            during Standby/Suspend.
> > > +      - description: Standby/Suspend clock source. It is optional
> > > +            and can be used in case the RTC will continue ticking during
> > > +            platform/system suspend. RTC hardware module contains a
> > > +            hardware mux for clock source selection.
> > 
> > If the RTC h/w contains a mux, then your mux inputs should be listed in 
> > 'clocks', not here.
> > 
> > > +
> > > +  assigned-clock-parents:
> > > +    description: List of phandles to each parent clock.
> > > +
> > > +  assigned-clock-rates:
> > > +    description: List of frequencies for RTC clock sources.
> > > +            RTC module contains 2 hardware divisors which can be
> > > +            enabled or not. Hence, available frequencies are the following
> > > +            parent_freq, parent_freq / 512, parent_freq / 32 or
> > > +            parent_freq / (512 * 32)
> > 
> > In general, assigned-clocks* do not need to be documented and should 
> > never be required.
> > 
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - "#clock-cells"
> > > +  - clocks
> > > +  - clock-names
> > > +  - assigned-clocks
> > > +  - assigned-clock-parents
> > > +  - assigned-clock-rates
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    rtc0: rtc@40060000 {
> > > +        compatible = "nxp,s32g3-rtc",
> > > +                   "nxp,s32g2-rtc";
> > > +        reg = <0x40060000 0x1000>;
> > > +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
> > > +        #clock-cells = <1>;
> > > +        clocks = <&clks 54>;
> > > +        clock-names = "ipg";
> > > +        /*
> > > +         * Configuration of default parent clocks.
> > > +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
> > > +         * 4-7 IDs are Suspend/Standby clock sources.
> > > +         */
> > > +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
> > 
> > That's weird...
> > 
> > > +        assigned-clock-parents = <&clks 56>, <&clks 55>;
> > 
> > I'd expect these should be in 'clocks'. I don't think this node should 
> > be a clock provider unless it provides a clock to something outside the 
> > RTC.
> > 
> > Looks like you are just using assigned-clocks to configure the clock mux 
> > in the RTC. That's way over complicated. Just define a vendor specific 
> > property with the mux settings. 
> 
> I just read v1 and got told use the clock framework...
> 
> I disagree completely. Tons of h/w blocks have the ability to select 
> (internal to the block) from multiple clock sources. Making the block a 
> clock provider to itself is completely pointless and an overkill, and 
> we *never* do that. Any display controller or audio interface has 
> mutiple clock sources as just 2 examples.

And in 6 months, we are going to learn that the rtc is used to clock the
wifi chip or whatever and we are going to need to add everything in the
CCF and we will have an unused property that we are going to have to
support forever to avoid breaking the ABI. This already happened...

> 
> However, I don't see why you need the divider config in DT. Can't you 
> figure out what divider you need based on input frequency? The output 
> frequency should be fixed, right?
> 
> Rob

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

Re: [PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

Posted by Rob Herring 3 weeks, 1 day ago

On Wed, Oct 16, 2024 at 06:08:23PM +0200, Alexandre Belloni wrote:
> On 15/10/2024 16:27:17-0500, Rob Herring wrote:
> > On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote:
> > > On Tue, Oct 15, 2024 at 01:51:30PM +0300, Ciprian Costea wrote:
> > > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > > 
> > > > This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
> > > > 
> > > > Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> > > > Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
> > > > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > > > Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > > ---
> > > >  .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 ++++++++++++++++++
> > > >  1 file changed, 102 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > > new file mode 100644
> > > > index 000000000000..3a77d4dd8f3d
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> > > > @@ -0,0 +1,102 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: NXP S32G2/S32G3 Real Time Clock (RTC)
> > > > +
> > > > +maintainers:
> > > > +  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
> > > > +  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - enum:
> > > > +          - nxp,s32g2-rtc
> > > > +      - items:
> > > > +          - const: nxp,s32g3-rtc
> > > > +          - const: nxp,s32g2-rtc
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  "#clock-cells":
> > > > +    const: 1
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: ipg clock drives the access to the
> > > > +          RTC iomapped registers
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: ipg
> > > > +
> > > > +  assigned-clocks:
> > > > +    minItems: 1
> > > > +    items:
> > > > +      - description: Runtime clock source. It must be a clock
> > > > +            source for the RTC module. It will be disabled by hardware
> > > > +            during Standby/Suspend.
> > > > +      - description: Standby/Suspend clock source. It is optional
> > > > +            and can be used in case the RTC will continue ticking during
> > > > +            platform/system suspend. RTC hardware module contains a
> > > > +            hardware mux for clock source selection.
> > > 
> > > If the RTC h/w contains a mux, then your mux inputs should be listed in 
> > > 'clocks', not here.
> > > 
> > > > +
> > > > +  assigned-clock-parents:
> > > > +    description: List of phandles to each parent clock.
> > > > +
> > > > +  assigned-clock-rates:
> > > > +    description: List of frequencies for RTC clock sources.
> > > > +            RTC module contains 2 hardware divisors which can be
> > > > +            enabled or not. Hence, available frequencies are the following
> > > > +            parent_freq, parent_freq / 512, parent_freq / 32 or
> > > > +            parent_freq / (512 * 32)
> > > 
> > > In general, assigned-clocks* do not need to be documented and should 
> > > never be required.
> > > 
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - interrupts
> > > > +  - "#clock-cells"
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - assigned-clocks
> > > > +  - assigned-clock-parents
> > > > +  - assigned-clock-rates
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > > +
> > > > +    rtc0: rtc@40060000 {
> > > > +        compatible = "nxp,s32g3-rtc",
> > > > +                   "nxp,s32g2-rtc";
> > > > +        reg = <0x40060000 0x1000>;
> > > > +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
> > > > +        #clock-cells = <1>;
> > > > +        clocks = <&clks 54>;
> > > > +        clock-names = "ipg";
> > > > +        /*
> > > > +         * Configuration of default parent clocks.
> > > > +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
> > > > +         * 4-7 IDs are Suspend/Standby clock sources.
> > > > +         */
> > > > +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
> > > 
> > > That's weird...
> > > 
> > > > +        assigned-clock-parents = <&clks 56>, <&clks 55>;
> > > 
> > > I'd expect these should be in 'clocks'. I don't think this node should 
> > > be a clock provider unless it provides a clock to something outside the 
> > > RTC.
> > > 
> > > Looks like you are just using assigned-clocks to configure the clock mux 
> > > in the RTC. That's way over complicated. Just define a vendor specific 
> > > property with the mux settings. 
> > 
> > I just read v1 and got told use the clock framework...
> > 
> > I disagree completely. Tons of h/w blocks have the ability to select 
> > (internal to the block) from multiple clock sources. Making the block a 
> > clock provider to itself is completely pointless and an overkill, and 
> > we *never* do that. Any display controller or audio interface has 
> > mutiple clock sources as just 2 examples.
> 
> And in 6 months, we are going to learn that the rtc is used to clock the
> wifi chip or whatever and we are going to need to add everything in the
> CCF and we will have an unused property that we are going to have to
> support forever to avoid breaking the ABI. This already happened...

For that to happen, the RTC needs to have a clock output. AFAICT, from 
the series it doesn't have any clock output. If it does have an output 
clock, then yes, I would agree with you. But I only know as much as what 
is put here about this h/w.

Rob

Re: [PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

Posted by Ciprian Marian Costea 3 weeks, 1 day ago

On 11/4/2024 5:29 PM, Rob Herring wrote:
> On Wed, Oct 16, 2024 at 06:08:23PM +0200, Alexandre Belloni wrote:
>> On 15/10/2024 16:27:17-0500, Rob Herring wrote:
>>> On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote:
>>>> On Tue, Oct 15, 2024 at 01:51:30PM +0300, Ciprian Costea wrote:
>>>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>>>
>>>>> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
>>>>>
>>>>> Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
>>>>> Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
>>>>> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>>>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>>>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>>> ---
>>>>>   .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 ++++++++++++++++++
>>>>>   1 file changed, 102 insertions(+)
>>>>>   create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..3a77d4dd8f3d
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>>> @@ -0,0 +1,102 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: NXP S32G2/S32G3 Real Time Clock (RTC)
>>>>> +
>>>>> +maintainers:
>>>>> +  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
>>>>> +  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    oneOf:
>>>>> +      - enum:
>>>>> +          - nxp,s32g2-rtc
>>>>> +      - items:
>>>>> +          - const: nxp,s32g3-rtc
>>>>> +          - const: nxp,s32g2-rtc
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  "#clock-cells":
>>>>> +    const: 1
>>>>> +
>>>>> +  clocks:
>>>>> +    items:
>>>>> +      - description: ipg clock drives the access to the
>>>>> +          RTC iomapped registers
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: ipg
>>>>> +
>>>>> +  assigned-clocks:
>>>>> +    minItems: 1
>>>>> +    items:
>>>>> +      - description: Runtime clock source. It must be a clock
>>>>> +            source for the RTC module. It will be disabled by hardware
>>>>> +            during Standby/Suspend.
>>>>> +      - description: Standby/Suspend clock source. It is optional
>>>>> +            and can be used in case the RTC will continue ticking during
>>>>> +            platform/system suspend. RTC hardware module contains a
>>>>> +            hardware mux for clock source selection.
>>>>
>>>> If the RTC h/w contains a mux, then your mux inputs should be listed in
>>>> 'clocks', not here.
>>>>
>>>>> +
>>>>> +  assigned-clock-parents:
>>>>> +    description: List of phandles to each parent clock.
>>>>> +
>>>>> +  assigned-clock-rates:
>>>>> +    description: List of frequencies for RTC clock sources.
>>>>> +            RTC module contains 2 hardware divisors which can be
>>>>> +            enabled or not. Hence, available frequencies are the following
>>>>> +            parent_freq, parent_freq / 512, parent_freq / 32 or
>>>>> +            parent_freq / (512 * 32)
>>>>
>>>> In general, assigned-clocks* do not need to be documented and should
>>>> never be required.
>>>>
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - reg
>>>>> +  - interrupts
>>>>> +  - "#clock-cells"
>>>>> +  - clocks
>>>>> +  - clock-names
>>>>> +  - assigned-clocks
>>>>> +  - assigned-clock-parents
>>>>> +  - assigned-clock-rates
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>> +
>>>>> +    rtc0: rtc@40060000 {
>>>>> +        compatible = "nxp,s32g3-rtc",
>>>>> +                   "nxp,s32g2-rtc";
>>>>> +        reg = <0x40060000 0x1000>;
>>>>> +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +        #clock-cells = <1>;
>>>>> +        clocks = <&clks 54>;
>>>>> +        clock-names = "ipg";
>>>>> +        /*
>>>>> +         * Configuration of default parent clocks.
>>>>> +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
>>>>> +         * 4-7 IDs are Suspend/Standby clock sources.
>>>>> +         */
>>>>> +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
>>>>
>>>> That's weird...
>>>>
>>>>> +        assigned-clock-parents = <&clks 56>, <&clks 55>;
>>>>
>>>> I'd expect these should be in 'clocks'. I don't think this node should
>>>> be a clock provider unless it provides a clock to something outside the
>>>> RTC.
>>>>
>>>> Looks like you are just using assigned-clocks to configure the clock mux
>>>> in the RTC. That's way over complicated. Just define a vendor specific
>>>> property with the mux settings.
>>>
>>> I just read v1 and got told use the clock framework...
>>>
>>> I disagree completely. Tons of h/w blocks have the ability to select
>>> (internal to the block) from multiple clock sources. Making the block a
>>> clock provider to itself is completely pointless and an overkill, and
>>> we *never* do that. Any display controller or audio interface has
>>> mutiple clock sources as just 2 examples.
>>
>> And in 6 months, we are going to learn that the rtc is used to clock the
>> wifi chip or whatever and we are going to need to add everything in the
>> CCF and we will have an unused property that we are going to have to
>> support forever to avoid breaking the ABI. This already happened...
> 
> For that to happen, the RTC needs to have a clock output. AFAICT, from
> the series it doesn't have any clock output. If it does have an output
> clock, then yes, I would agree with you. But I only know as much as what
> is put here about this h/w.
> 
> Rob

Hello Rob,

Thank you for resuming discussion on this patchset.
Indeed, the RTC does not have a clock output.

In the meantime I've sent a V3 where 'assigned-*' entries are no longer 
required.

Please let me know if V3 is headed into the right direction since I 
prefer not to throw away current 'assigned-*' CCF approach since IMHO 
its scalable with the clock source selection for runtime and suspend.

Ciprian

Re: [PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs

Posted by Ciprian Marian Costea 1 month, 1 week ago

On 10/16/2024 7:08 PM, Alexandre Belloni wrote:

Hello Rob and Alexandre,

Thank you for your careful review!

> On 15/10/2024 16:27:17-0500, Rob Herring wrote:
>> On Tue, Oct 15, 2024 at 04:15:40PM -0500, Rob Herring wrote:
>>> On Tue, Oct 15, 2024 at 01:51:30PM +0300, Ciprian Costea wrote:
>>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>>
>>>> This patch adds the dt-bindings for NXP S32G2/S32G3 SoCs RTC driver.
>>>>
>>>> Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
>>>> Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@nxp.com>
>>>> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>> ---
>>>>   .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 ++++++++++++++++++
>>>>   1 file changed, 102 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>> new file mode 100644
>>>> index 000000000000..3a77d4dd8f3d
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
>>>> @@ -0,0 +1,102 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/rtc/nxp,s32g-rtc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: NXP S32G2/S32G3 Real Time Clock (RTC)
>>>> +
>>>> +maintainers:
>>>> +  - Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
>>>> +  - Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - enum:
>>>> +          - nxp,s32g2-rtc
>>>> +      - items:
>>>> +          - const: nxp,s32g3-rtc
>>>> +          - const: nxp,s32g2-rtc
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  "#clock-cells":
>>>> +    const: 1
>>>> +
>>>> +  clocks:
>>>> +    items:
>>>> +      - description: ipg clock drives the access to the
>>>> +          RTC iomapped registers
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: ipg
>>>> +
>>>> +  assigned-clocks:
>>>> +    minItems: 1
>>>> +    items:
>>>> +      - description: Runtime clock source. It must be a clock
>>>> +            source for the RTC module. It will be disabled by hardware
>>>> +            during Standby/Suspend.
>>>> +      - description: Standby/Suspend clock source. It is optional
>>>> +            and can be used in case the RTC will continue ticking during
>>>> +            platform/system suspend. RTC hardware module contains a
>>>> +            hardware mux for clock source selection.
>>>
>>> If the RTC h/w contains a mux, then your mux inputs should be listed in
>>> 'clocks', not here.
>>>
>>>> +
>>>> +  assigned-clock-parents:
>>>> +    description: List of phandles to each parent clock.
>>>> +
>>>> +  assigned-clock-rates:
>>>> +    description: List of frequencies for RTC clock sources.
>>>> +            RTC module contains 2 hardware divisors which can be
>>>> +            enabled or not. Hence, available frequencies are the following
>>>> +            parent_freq, parent_freq / 512, parent_freq / 32 or
>>>> +            parent_freq / (512 * 32)
>>>
>>> In general, assigned-clocks* do not need to be documented and should
>>> never be required.
>>>
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +  - "#clock-cells"
>>>> +  - clocks
>>>> +  - clock-names
>>>> +  - assigned-clocks
>>>> +  - assigned-clock-parents
>>>> +  - assigned-clock-rates
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>> +
>>>> +    rtc0: rtc@40060000 {
>>>> +        compatible = "nxp,s32g3-rtc",
>>>> +                   "nxp,s32g2-rtc";
>>>> +        reg = <0x40060000 0x1000>;
>>>> +        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
>>>> +        #clock-cells = <1>;
>>>> +        clocks = <&clks 54>;
>>>> +        clock-names = "ipg";
>>>> +        /*
>>>> +         * Configuration of default parent clocks.
>>>> +         * 'assigned-clocks' 0-3 IDs are Runtime clock sources
>>>> +         * 4-7 IDs are Suspend/Standby clock sources.
>>>> +         */
>>>> +        assigned-clocks = <&rtc0 2>, <&rtc0 4>;
>>>
>>> That's weird...
>>>
>>>> +        assigned-clock-parents = <&clks 56>, <&clks 55>;
>>>
>>> I'd expect these should be in 'clocks'. I don't think this node should
>>> be a clock provider unless it provides a clock to something outside the
>>> RTC.
>>>
>>> Looks like you are just using assigned-clocks to configure the clock mux
>>> in the RTC. That's way over complicated. Just define a vendor specific
>>> property with the mux settings.
>>
>> I just read v1 and got told use the clock framework...
>>
>> I disagree completely. Tons of h/w blocks have the ability to select
>> (internal to the block) from multiple clock sources. Making the block a
>> clock provider to itself is completely pointless and an overkill, and
>> we *never* do that. Any display controller or audio interface has
>> mutiple clock sources as just 2 examples.
> 
> And in 6 months, we are going to learn that the rtc is used to clock the
> wifi chip or whatever and we are going to need to add everything in the
> CCF and we will have an unused property that we are going to have to
> support forever to avoid breaking the ABI. This already happened...
> 
>>
>> However, I don't see why you need the divider config in DT. Can't you
>> figure out what divider you need based on input frequency? The output
>> frequency should be fixed, right?
>>
>> Rob
> 

Would the following approach be acceptable ?

1. Keep 'assigned-clocks' based implementation as optional (not 
required), in order to take leverage of its scalability for allowing 
different clock source and divisor settings.
2. Implement a default clock muxing setting in the driver for both 
Runtime and Suspend.


Best Regards,
Ciprian

[PATCH v2 1/4] dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs
[PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support
[PATCH v2 3/4] arm64: defconfig: add S32G RTC module support
[PATCH v2 4/4] MAINTAINERS: add NXP S32G RTC driver