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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/31/2024 6:39 PM, Frank Li wrote:

Hello Frank,

Thank you for your review!

On Thu, Oct 31, 2024 at 10:35:54AM +0200, Ciprian Costea wrote:
From: Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>

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

Co-developed-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@xxxxxxx>
Signed-off-by: Bogdan-Gabriel Roman <bogdan-gabriel.roman@xxxxxxx>
Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@xxxxxxx>
Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@xxxxxxx>
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>
---

next time you can cc imx@xxxxxxxxxxxxxxx

Thanks for your suggestion, I will start adding this list.


  .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 99 +++++++++++++++++++
  1 file changed, 99 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..3694af883dc7
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
@@ -0,0 +1,99 @@
+# 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@xxxxxxx>
+  - Ciprian Marian Costea <ciprianmarian.costea@xxxxxxx>
+
+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

Does your RTC is clock provider? why need #clock-cells


RTC clocking on S32G2/S32G3 has a clock mux for selecting between up to 4 different clock sources (parents). Now, as suggested in previous reviews, I've switched to using the CCF in order to implement this hardware particularity. In the end I've resorted to implementing 'assigned-*' approach which while not mandatory as per proposed bindings I find it quite scalable in selecting clock sources for the RTC module compared to the first iteration (V1) of this patchset.

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

Needn't assigned-*


'assigned-*' entries are not required, but based on my previous answer I would prefer to document them in order to instruct further S32 SoC based boards which may use this driver.

+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#clock-cells"
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    rtc0: rtc@40060000 {

needn't label

Thanks. I will remove the label in V4.


+        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






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux