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@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>
---
.../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@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
+
+ 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
