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

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

[Ciprian Costea]

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

This patch series add support for the NXP
RTC hardware module present on S32G2/S32G3 SoCs.

The RTC module is used to enable Suspend to RAM (STR) support.
RTC tracks clock time during system suspend. It represents a
time-based wakeup source for the S32G2/S32G3 SoCs.

RTC is not battery-powered and it is not kept alive during
system reset.

Following is an example of Suspend to RAM trigger on
S32G2/S32G3 SoCs, using userspace tools such as rtcwake:
# rtcwake -s 2 -m mem
# rtcwake: assuming RTC uses UTC ...
# rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Feb  6 06:28:36 2036
#

Changes in V2:
- Removed 'clksel' support from dts bindings.
  Used clock parents support from CCF to better illustrate the
  RTC hardware IP from S32G2/S32G3.
- Removed frequency dividers support from dts bindings.
  Used assigned clock frequencies support from CCF instead.
- Reduced the interval while RTC is voluntarily disabled to a simple
  register write in order to avoid any race condition between a possbile
  rollover and 'suspend' callback execution flow.
- Updated bindings documentation with respect to clocking support.
- Fixed a potential unused variable warning reported
  by kernel test robot.
- Updated to usage of 'devm_rtc_allocate_device' and 'devm_rtc_register_device'
  instead of deprecated 'devm_rtc_device_register'.

Ciprian Marian Costea (4):
  dt-bindings: rtc: add schema for NXP S32G2/S32G3 SoCs
  rtc: s32g: add NXP S32G2/S32G3 SoC support
  arm64: defconfig: add S32G RTC module support
  MAINTAINERS: add NXP S32G RTC driver

 .../devicetree/bindings/rtc/nxp,s32g-rtc.yaml | 102 +++
 MAINTAINERS                                   |   2 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/rtc/Kconfig                           |  11 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-s32g.c                        | 778 ++++++++++++++++++
 6 files changed, 895 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
 create mode 100644 drivers/rtc/rtc-s32g.c

-- 
2.45.2

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

[Ciprian Costea]

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

[PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Ciprian Costea]

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

Add a RTC driver for NXP S32G2/S32G3 SoCs.

The RTC module is used to enable Suspend to RAM (STR) support
on NXP S32G2/S32G3 SoC based boards.
RTC tracks clock time during system suspend.

RTC from S32G2/S32G3 is not battery-powered and it is not
kept alive during system reset.

Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@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>
---
 drivers/rtc/Kconfig    |  11 +
 drivers/rtc/Makefile   |   1 +
 drivers/rtc/rtc-s32g.c | 778 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 790 insertions(+)
 create mode 100644 drivers/rtc/rtc-s32g.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index e87c3d74565c..18fc3577f6cd 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -2054,4 +2054,15 @@ config RTC_DRV_SSD202D
 	  This driver can also be built as a module, if so, the module
 	  will be called "rtc-ssd20xd".
 
+config RTC_DRV_S32G
+	tristate "RTC driver for S32G2/S32G3 SoCs"
+	depends on ARCH_S32 || COMPILE_TEST
+	depends on COMMON_CLK
+	help
+	  Say yes to enable RTC driver for platforms based on the
+	  S32G2/S32G3 SoC family.
+
+	  This RTC module can be used as a wakeup source.
+	  Please note that it is not battery-powered.
+
 endif # RTC_CLASS
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 8ee79cb18322..a63d010a753c 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -158,6 +158,7 @@ obj-$(CONFIG_RTC_DRV_RX8025)	+= rtc-rx8025.o
 obj-$(CONFIG_RTC_DRV_RX8111)	+= rtc-rx8111.o
 obj-$(CONFIG_RTC_DRV_RX8581)	+= rtc-rx8581.o
 obj-$(CONFIG_RTC_DRV_RZN1)	+= rtc-rzn1.o
+obj-$(CONFIG_RTC_DRV_S32G)	+= rtc-s32g.o
 obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
 obj-$(CONFIG_RTC_DRV_S3C)	+= rtc-s3c.o
 obj-$(CONFIG_RTC_DRV_S5M)	+= rtc-s5m.o
diff --git a/drivers/rtc/rtc-s32g.c b/drivers/rtc/rtc-s32g.c
new file mode 100644
index 000000000000..d6502d8bf616
--- /dev/null
+++ b/drivers/rtc/rtc-s32g.c
@@ -0,0 +1,778 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/of_irq.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+#include <linux/iopoll.h>
+
+#define RTCC_OFFSET	0x4ul
+#define RTCS_OFFSET	0x8ul
+#define RTCCNT_OFFSET	0xCul
+#define APIVAL_OFFSET	0x10ul
+#define RTCVAL_OFFSET	0x14ul
+
+/* RTCC fields */
+#define RTCC_CNTEN				BIT(31)
+#define RTCC_RTCIE_SHIFT		30
+#define RTCC_RTCIE				BIT(RTCC_RTCIE_SHIFT)
+#define RTCC_ROVREN				BIT(28)
+#define RTCC_APIEN				BIT(15)
+#define RTCC_APIIE				BIT(14)
+#define RTCC_CLKSEL_OFFSET		12
+#define RTCC_CLKSEL_MASK		GENMASK(13, 12)
+#define RTCC_CLKSEL(n)			(((n) << 12) & RTCC_CLKSEL_MASK)
+#define RTCC_DIV512EN			BIT(11)
+#define RTCC_DIV32EN			BIT(10)
+
+/* RTCS fields */
+#define RTCS_RTCF		BIT(29)
+#define RTCS_INV_RTC		BIT(18)
+#define RTCS_APIF		BIT(13)
+#define RTCS_ROVRF		BIT(10)
+
+#define ROLLOVER_VAL		GENMASK(31, 0)
+#define RTC_SYNCH_TIMEOUT	(100 * USEC_PER_MSEC)
+
+#define RTC_CLK_MUX_SIZE	4
+
+/*
+ * S32G2 and S32G3 SoCs have RTC clock source 1 reserved and
+ * should not be used.
+ */
+#define RTC_QUIRK_SRC1_RESERVED			BIT(2)
+
+#define to_rtcpriv(_hw)	container_of(_hw, struct rtc_priv, clk)
+
+enum {
+	RTC_CLK_SRC0 = 0,
+	RTC_CLK_SRC1,
+	RTC_CLK_SRC2,
+	RTC_CLK_SRC3
+};
+
+enum {
+	DIV1 = 1,
+	DIV32 = 32,
+	DIV512 = 512,
+	DIV512_32 = 16384
+};
+
+struct rtc_time_base {
+	s64 sec;
+	u64 cycles;
+	u64 rollovers;
+	struct rtc_time tm;
+};
+
+struct rtc_priv {
+	struct rtc_device *rdev;
+	struct device *dev;
+	u8 __iomem *rtc_base;
+	struct clk_hw clk;
+	struct clk *ipg;
+	const struct rtc_soc_data *rtc_data;
+	struct rtc_time_base base;
+	u64 rtc_hz;
+	u64 rollovers;
+	int dt_irq_id;
+	int runtime_src_idx;
+	int suspend_src_idx;
+	u32 runtime_div;
+	u32 suspend_div;
+};
+
+struct rtc_soc_data {
+	u32 quirks;
+};
+
+static const struct rtc_soc_data rtc_s32g2_data = {
+	.quirks = RTC_QUIRK_SRC1_RESERVED,
+};
+
+static int is_src1_reserved(struct rtc_priv *priv)
+{
+	return priv->rtc_data->quirks & RTC_QUIRK_SRC1_RESERVED;
+}
+
+static u64 cycles_to_sec(u64 hz, u64 cycles)
+{
+	return cycles / hz;
+}
+
+/*
+ * Convert a number of seconds to a value suitable for RTCVAL in our clock's
+ * current configuration.
+ * @rtcval: The value to go into RTCVAL[RTCVAL]
+ * Returns: 0 for success, -EINVAL if @seconds push the counter at least
+ *          twice the rollover interval
+ */
+static int sec_to_rtcval(const struct rtc_priv *priv,
+			 unsigned long seconds, u32 *rtcval)
+{
+	u32 rtccnt, delta_cnt;
+	u32 target_cnt = 0;
+
+	/* For now, support at most one rollover of the counter */
+	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, ROLLOVER_VAL))
+		return -EINVAL;
+
+	/*
+	 * RTCCNT is read-only; we must return a value relative to the
+	 * current value of the counter (and hope we don't linger around
+	 * too much before we get to enable the interrupt)
+	 */
+	delta_cnt = seconds * priv->rtc_hz;
+	rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
+
+	if (~rtccnt < delta_cnt)
+		target_cnt = (delta_cnt - ~rtccnt);
+	else
+		target_cnt = rtccnt + delta_cnt;
+
+	/*
+	 * According to RTCVAL register description,
+	 * its minimum value should be 4.
+	 */
+	if (unlikely(target_cnt < 4))
+		target_cnt = 4;
+
+	*rtcval = target_cnt;
+
+	return 0;
+}
+
+static irqreturn_t rtc_handler(int irq, void *dev)
+{
+	struct rtc_priv *priv = platform_get_drvdata(dev);
+	u32 status;
+
+	status = ioread32(priv->rtc_base + RTCS_OFFSET);
+	if (status & RTCS_ROVRF) {
+		if (priv->rollovers == ULONG_MAX)
+			priv->rollovers = 0;
+		else
+			priv->rollovers++;
+	}
+
+	if (status & RTCS_RTCF) {
+		iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
+		rtc_update_irq(priv->rdev, 1, RTC_AF);
+	}
+
+	if (status & RTCS_APIF)
+		rtc_update_irq(priv->rdev, 1, RTC_PF);
+
+	iowrite32(status, priv->rtc_base + RTCS_OFFSET);
+
+	return IRQ_HANDLED;
+}
+
+static int get_time_left(struct device *dev, struct rtc_priv *priv,
+			 u32 *sec)
+{
+	u32 rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
+	u32 rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET);
+
+	if (rtcval < rtccnt) {
+		dev_err(dev, "RTC timer expired before entering suspend\n");
+		return -EIO;
+	}
+
+	*sec = cycles_to_sec(priv->rtc_hz, rtcval - rtccnt);
+
+	return 0;
+}
+
+static int s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
+				     u32 offset)
+{
+	u64 cycles, sec, base_cycles;
+	u32 counter;
+
+	counter = ioread32(priv->rtc_base + offset);
+	cycles = priv->rollovers * ROLLOVER_VAL + counter;
+	base_cycles = priv->base.cycles + priv->base.rollovers * ROLLOVER_VAL;
+
+	if (cycles < base_cycles)
+		return -EINVAL;
+
+	cycles -= base_cycles;
+	sec = priv->base.sec + cycles_to_sec(priv->rtc_hz, cycles);
+
+	return sec;
+}
+
+static int s32g_rtc_read_time(struct device *dev,
+			      struct rtc_time *tm)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	u64 sec;
+
+	if (!tm)
+		return -EINVAL;
+
+	sec = s32g_rtc_get_time_or_alrm(priv, RTCCNT_OFFSET);
+	if (sec < 0)
+		return -EINVAL;
+
+	rtc_time64_to_tm(sec, tm);
+
+	return 0;
+}
+
+static int s32g_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	u32 rtcc, sec_left;
+	u64 sec;
+
+	if (!alrm)
+		return -EINVAL;
+
+	sec = s32g_rtc_get_time_or_alrm(priv, RTCVAL_OFFSET);
+	if (sec < 0)
+		return -EINVAL;
+
+	rtc_time64_to_tm(sec, &alrm->time);
+
+	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+	alrm->enabled = sec && (rtcc & RTCC_RTCIE);
+
+	alrm->pending = 0;
+	if (alrm->enabled && !get_time_left(dev, priv, &sec_left))
+		alrm->pending = !!sec_left;
+
+	return 0;
+}
+
+static int s32g_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	u32 rtcc;
+
+	if (!priv->dt_irq_id)
+		return -EIO;
+
+	/*
+	 * RTCIE cannot be deasserted because it will also disable the
+	 * rollover interrupt.
+	 */
+	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+	if (enabled)
+		rtcc |= RTCC_RTCIE;
+
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+
+	return 0;
+}
+
+static int s32g_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	struct rtc_time time_crt;
+	long long t_crt, t_alrm;
+	int ret = 0;
+	u32 rtcval, rtcs;
+
+	iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
+
+	t_alrm = rtc_tm_to_time64(&alrm->time);
+
+	/*
+	 * Assuming the alarm is being set relative to the same time
+	 * returned by our s32g_rtc_read_time callback
+	 */
+	ret = s32g_rtc_read_time(dev, &time_crt);
+	if (ret)
+		return ret;
+
+	t_crt = rtc_tm_to_time64(&time_crt);
+	if (t_alrm <= t_crt) {
+		dev_warn(dev, "Alarm is set in the past\n");
+		return -EINVAL;
+	}
+
+	ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval);
+	if (ret) {
+		/*
+		 * Rollover support enables RTC alarm
+		 * for a maximum timespan of ~3 months.
+		 */
+		dev_warn(dev, "Alarm is set too far in the future\n");
+		return ret;
+	}
+
+	ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC),
+				0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET);
+	if (ret) {
+		dev_err(dev, "Synchronization failed\n");
+		return ret;
+	}
+
+	iowrite32(rtcval, priv->rtc_base + RTCVAL_OFFSET);
+
+	return 0;
+}
+
+static int s32g_rtc_set_time(struct device *dev,
+			     struct rtc_time *time)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+
+	if (!time)
+		return -EINVAL;
+
+	priv->base.rollovers = priv->rollovers;
+	priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET);
+	priv->base.sec = rtc_tm_to_time64(time);
+
+	return 0;
+}
+
+static void rtc_disable(struct rtc_priv *priv)
+{
+	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+
+	rtcc &= ~RTCC_CNTEN;
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+}
+
+static void rtc_enable(struct rtc_priv *priv)
+{
+	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+
+	rtcc |= RTCC_CNTEN;
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+}
+
+static int get_div_config(unsigned long req_rate,
+			  unsigned long prate)
+{
+	if (req_rate == prate)
+		return DIV1;
+	else if (req_rate == prate / (DIV512 * DIV32))
+		return DIV512_32;
+	else if (req_rate == prate / DIV512)
+		return DIV512;
+	else if (req_rate == prate / DIV32)
+		return DIV32;
+
+	return 0;
+}
+
+static void adjust_dividers(struct rtc_priv *priv,
+			    u32 div_val, u32 *reg)
+{
+	switch (div_val) {
+	case DIV512_32:
+		*reg |= RTCC_DIV512EN;
+		*reg |= RTCC_DIV32EN;
+		break;
+	case DIV512:
+		*reg |= RTCC_DIV512EN;
+		break;
+	case DIV32:
+		*reg |= RTCC_DIV32EN;
+		break;
+	default:
+		return;
+	}
+
+	priv->rtc_hz /= div_val;
+}
+
+static unsigned long get_prate_by_index(struct clk_hw *hw,
+					u8 index)
+{
+	struct clk_hw *parent;
+
+	parent = clk_hw_get_parent_by_index(hw, index);
+	if (!parent)
+		return -EINVAL;
+
+	return clk_hw_get_rate(parent);
+}
+
+static int rtc_clk_determine_rate(struct clk_hw *hw,
+				  struct clk_rate_request *req)
+{
+	struct rtc_priv *priv = to_rtcpriv(hw);
+	int i, num_parents = clk_hw_get_num_parents(hw);
+	u32 config;
+
+	for (i = 0; i < num_parents; i++) {
+		config = get_div_config(req->rate, get_prate_by_index(hw, i));
+		if (config) {
+			if (i < RTC_CLK_MUX_SIZE)
+				/* Runtime clk source divisors */
+				priv->runtime_div = config;
+			else
+				/* Suspend clk source divisors */
+				priv->suspend_div = config;
+
+			return 0;
+		}
+	}
+
+	dev_err(priv->dev, "Failed to determine RTC clock rate\n");
+	return -EINVAL;
+}
+
+static u8 rtc_clk_get_parent(struct clk_hw *hw)
+{
+	struct rtc_priv *priv = to_rtcpriv(hw);
+
+	return (ioread32(priv->rtc_base + RTCC_OFFSET) &
+		RTCC_CLKSEL_MASK) >> RTCC_CLKSEL_OFFSET;
+}
+
+static int rtc_clk_src_switch(struct clk_hw *hw, u8 src)
+{
+	struct rtc_priv *priv = to_rtcpriv(hw);
+	u32 rtcc = 0;
+
+	switch (src % RTC_CLK_MUX_SIZE) {
+	case RTC_CLK_SRC0:
+		rtcc |= RTCC_CLKSEL(RTC_CLK_SRC0);
+		break;
+	case RTC_CLK_SRC1:
+		if (is_src1_reserved(priv))
+			return -EOPNOTSUPP;
+		rtcc |= RTCC_CLKSEL(RTC_CLK_SRC1);
+		break;
+	case RTC_CLK_SRC2:
+		rtcc |= RTCC_CLKSEL(RTC_CLK_SRC2);
+		break;
+	case RTC_CLK_SRC3:
+		rtcc |= RTCC_CLKSEL(RTC_CLK_SRC3);
+		break;
+	default:
+		dev_err(priv->dev, "Invalid clock mux parent: %d\n", src);
+		return -EINVAL;
+	}
+
+	priv->rtc_hz = get_prate_by_index(hw, src);
+	if (!priv->rtc_hz) {
+		dev_err(priv->dev, "Failed to get RTC frequency\n");
+		return -EINVAL;
+	}
+
+	if (src < RTC_CLK_MUX_SIZE)
+		adjust_dividers(priv, priv->runtime_div, &rtcc);
+	else
+		adjust_dividers(priv, priv->suspend_div, &rtcc);
+
+	rtcc |= RTCC_RTCIE | RTCC_ROVREN;
+	/*
+	 * Make sure the CNTEN is 0 before we configure
+	 * the clock source and dividers.
+	 */
+	rtc_disable(priv);
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+	rtc_enable(priv);
+
+	return 0;
+}
+
+static int rtc_clk_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct rtc_priv *priv = to_rtcpriv(hw);
+
+	/*
+	 * 0-3 IDs are Runtime clk sources
+	 * 4-7 IDs are Suspend clk sources
+	 */
+	if (index < RTC_CLK_MUX_SIZE) {
+		/* Runtime clk source */
+		priv->runtime_src_idx = index;
+		return 0;
+	} else if (index < RTC_CLK_MUX_SIZE * 2) {
+		/* Suspend clk source */
+		priv->suspend_src_idx = index;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static const struct rtc_class_ops rtc_ops = {
+	.read_time = s32g_rtc_read_time,
+	.set_time = s32g_rtc_set_time,
+	.read_alarm = s32g_rtc_read_alarm,
+	.set_alarm = s32g_rtc_set_alarm,
+	.alarm_irq_enable = s32g_rtc_alarm_irq_enable,
+};
+
+static const struct clk_ops rtc_clk_ops = {
+	.determine_rate = rtc_clk_determine_rate,
+	.get_parent = rtc_clk_get_parent,
+	.set_parent = rtc_clk_set_parent,
+};
+
+static int priv_dts_init(struct rtc_priv *priv)
+{
+	struct device *dev = priv->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+
+	priv->ipg = devm_clk_get(dev, "ipg");
+	if (IS_ERR(priv->ipg)) {
+		dev_err(dev, "Failed to get 'ipg' clock\n");
+		return PTR_ERR(priv->ipg);
+	}
+
+	priv->dt_irq_id = platform_get_irq(pdev, 0);
+	if (priv->dt_irq_id < 0) {
+		dev_err(dev, "Error reading interrupt # from dts\n");
+		return priv->dt_irq_id;
+	}
+
+	return 0;
+}
+
+static int rtc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rtc_priv *priv;
+	static const char *parents[RTC_CLK_MUX_SIZE * 2] = {
+		"rtc_runtime_s0",
+		"rtc_runtime_s1",
+		"rtc_runtime_s2",
+		"rtc_runtime_s3",
+		"rtc_standby_s0",
+		"rtc_standby_s1",
+		"rtc_standby_s2",
+		"rtc_standby_s3"
+	};
+	struct clk_init_data clk_init = {
+		.name = "rtc_clk",
+		.ops = &rtc_clk_ops,
+		.flags = 0,
+		.parent_names = parents,
+		.num_parents = ARRAY_SIZE(parents),
+	};
+	int ret = 0;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->rtc_data = of_device_get_match_data(dev);
+	if (!priv->rtc_data)
+		return -ENODEV;
+
+	priv->rtc_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->rtc_base))
+		return dev_err_probe(dev, PTR_ERR(priv->rtc_base),
+				"Failed to map registers\n");
+
+	device_init_wakeup(dev, true);
+	priv->dev = dev;
+
+	ret = priv_dts_init(priv);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(priv->ipg);
+	if (ret) {
+		dev_err(dev, "Cannot enable 'ipg' clock, error: %d\n", ret);
+		return ret;
+	}
+
+	priv->runtime_src_idx = -EINVAL;
+	priv->suspend_src_idx = -EINVAL;
+
+	priv->clk.init = &clk_init;
+	ret = devm_clk_hw_register(dev, &priv->clk);
+	if (ret) {
+		dev_err(dev, "Failed to register rtc_clk clk\n");
+		goto disable_ipg_clk;
+	}
+
+	ret = of_clk_add_hw_provider(dev->of_node,
+				     of_clk_hw_simple_get, priv->clk.clk);
+	if (ret) {
+		dev_err(dev, "Failed to add rtc_clk clk provider\n");
+		goto disable_ipg_clk;
+	}
+
+	if (priv->runtime_src_idx < 0) {
+		ret = priv->runtime_src_idx;
+		dev_err(dev, "RTC runtime clock source is not specified\n");
+		goto disable_ipg_clk;
+	}
+
+	ret = rtc_clk_src_switch(&priv->clk, priv->runtime_src_idx);
+	if (ret) {
+		dev_err(dev, "Failed clk source switch, err: %d\n", ret);
+		goto disable_ipg_clk;
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->rdev = devm_rtc_allocate_device(dev);
+	if (IS_ERR(priv->rdev)) {
+		dev_err(dev, "Failed to allocate RTC device\n");
+		ret = PTR_ERR(priv->rdev);
+		goto disable_rtc;
+	}
+
+	priv->rdev->ops = &rtc_ops;
+
+	ret = devm_rtc_register_device(priv->rdev);
+	if (ret) {
+		dev_err(dev, "Failed to register RTC device\n");
+		goto disable_rtc;
+	}
+
+	ret = devm_request_irq(dev, priv->dt_irq_id,
+			       rtc_handler, 0, dev_name(dev), pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Request interrupt %d failed, error: %d\n",
+			priv->dt_irq_id, ret);
+		goto disable_rtc;
+	}
+
+	return 0;
+
+disable_ipg_clk:
+	clk_disable_unprepare(priv->ipg);
+disable_rtc:
+	rtc_disable(priv);
+	return ret;
+}
+
+static void rtc_remove(struct platform_device *pdev)
+{
+	struct rtc_priv *priv = platform_get_drvdata(pdev);
+
+	rtc_disable(priv);
+}
+
+static void  __maybe_unused enable_api_irq(struct device *dev, unsigned int enabled)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	u32 api_irq = RTCC_APIEN | RTCC_APIIE;
+	u32 rtcc;
+
+	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
+	if (enabled)
+		rtcc |= api_irq;
+	else
+		rtcc &= ~api_irq;
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+}
+
+static int __maybe_unused rtc_suspend(struct device *dev)
+{
+	struct rtc_priv *init_priv = dev_get_drvdata(dev);
+	struct rtc_priv priv;
+	long long base_sec;
+	int ret = 0;
+	u32 rtcval;
+	u32 sec;
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	if (init_priv->suspend_src_idx < 0)
+		return 0;
+
+	if (rtc_clk_get_parent(&init_priv->clk) == init_priv->suspend_src_idx)
+		return 0;
+
+	/* Save last known timestamp before we switch clocks and reinit RTC */
+	ret = s32g_rtc_read_time(dev, &init_priv->base.tm);
+	if (ret)
+		return ret;
+
+	/*
+	 * Use a local copy of the RTC control block to
+	 * avoid restoring it on resume path.
+	 */
+	memcpy(&priv, init_priv, sizeof(priv));
+
+	ret = get_time_left(dev, init_priv, &sec);
+	if (ret)
+		return ret;
+
+	/* Adjust for the number of seconds we'll be asleep */
+	base_sec = rtc_tm_to_time64(&init_priv->base.tm);
+	base_sec += sec;
+	rtc_time64_to_tm(base_sec, &init_priv->base.tm);
+
+	ret = rtc_clk_src_switch(&priv.clk, priv.suspend_src_idx);
+	if (ret) {
+		dev_err(dev, "Failed clk source switch, err: %d\n", ret);
+		return ret;
+	}
+
+	ret = sec_to_rtcval(&priv, sec, &rtcval);
+	if (ret) {
+		dev_warn(dev, "Alarm is too far in the future\n");
+		return ret;
+	}
+
+	s32g_rtc_alarm_irq_enable(dev, 0);
+	enable_api_irq(dev, 1);
+	iowrite32(rtcval, priv.rtc_base + APIVAL_OFFSET);
+	iowrite32(0, priv.rtc_base + RTCVAL_OFFSET);
+
+	return ret;
+}
+
+static int __maybe_unused rtc_resume(struct device *dev)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	int ret;
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	if (rtc_clk_get_parent(&priv->clk) == priv->runtime_src_idx)
+		return 0;
+
+	/* Disable wake-up interrupts */
+	enable_api_irq(dev, 0);
+
+	ret = rtc_clk_src_switch(&priv->clk, priv->runtime_src_idx);
+	if (ret) {
+		dev_err(dev, "Failed clk source switch, err: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Now RTCCNT has just been reset, and is out of sync with priv->base;
+	 * reapply the saved time settings
+	 */
+	return s32g_rtc_set_time(dev, &priv->base.tm);
+}
+
+static const struct of_device_id rtc_dt_ids[] = {
+	{ .compatible = "nxp,s32g2-rtc", .data = &rtc_s32g2_data},
+	{ .compatible = "nxp,s32g3-rtc", .data = &rtc_s32g2_data},
+	{ /* sentinel */ },
+};
+
+static SIMPLE_DEV_PM_OPS(rtc_pm_ops,
+			 rtc_suspend, rtc_resume);
+
+static struct platform_driver rtc_driver = {
+	.driver		= {
+		.name			= "s32g-rtc",
+		.pm				= &rtc_pm_ops,
+		.of_match_table = rtc_dt_ids,
+	},
+	.probe		= rtc_probe,
+	.remove_new	= rtc_remove,
+};
+module_platform_driver(rtc_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
+MODULE_LICENSE("GPL");
-- 
2.45.2

[PATCH v2 3/4] arm64: defconfig: add S32G RTC module support

[Ciprian Costea]

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

The RTC module enables Suspend to RAM (STR) support
on NXP S32G2/S32G3 SoC based boards.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 5fdbfea7a5b2..584cef78c984 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1209,6 +1209,7 @@ CONFIG_RTC_DRV_DA9063=m
 CONFIG_RTC_DRV_EFI=y
 CONFIG_RTC_DRV_CROS_EC=y
 CONFIG_RTC_DRV_FSL_FTM_ALARM=m
+CONFIG_RTC_DRV_S32G=m
 CONFIG_RTC_DRV_S3C=y
 CONFIG_RTC_DRV_PL031=y
 CONFIG_RTC_DRV_SUN6I=y
-- 
2.45.2

[PATCH v2 4/4] MAINTAINERS: add NXP S32G RTC driver

[Ciprian Costea]

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

Add the NXP S32G RTC driver as maintained so further patches
on this driver can be reviewed under this architecture.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7bfef98226d9..991a9df6819d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2763,8 +2763,10 @@ R:	Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>
 L:	NXP S32 Linux Team <s32@nxp.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
+F:	Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
 F:	arch/arm64/boot/dts/freescale/s32g*.dts*
 F:	drivers/pinctrl/nxp/
+F:	drivers/rtc/rtc-s32g.c
 
 ARM/Orion SoC/Technologic Systems TS-78xx platform support
 M:	Alexander Clouter <alex@digriz.org.uk>
-- 
2.45.2

Re: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Rob Herring]

On Tue, Oct 15, 2024 at 01:51:31PM +0300, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> Add a RTC driver for NXP S32G2/S32G3 SoCs.
> 
> The RTC module is used to enable Suspend to RAM (STR) support
> on NXP S32G2/S32G3 SoC based boards.
> RTC tracks clock time during system suspend.
> 
> RTC from S32G2/S32G3 is not battery-powered and it is not
> kept alive during system reset.
> 
> Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
> Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@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>
> ---
>  drivers/rtc/Kconfig    |  11 +
>  drivers/rtc/Makefile   |   1 +
>  drivers/rtc/rtc-s32g.c | 778 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 790 insertions(+)
>  create mode 100644 drivers/rtc/rtc-s32g.c


> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 8ee79cb18322..a63d010a753c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -158,6 +158,7 @@ obj-$(CONFIG_RTC_DRV_RX8025)	+= rtc-rx8025.o
>  obj-$(CONFIG_RTC_DRV_RX8111)	+= rtc-rx8111.o
>  obj-$(CONFIG_RTC_DRV_RX8581)	+= rtc-rx8581.o
>  obj-$(CONFIG_RTC_DRV_RZN1)	+= rtc-rzn1.o
> +obj-$(CONFIG_RTC_DRV_S32G)	+= rtc-s32g.o
>  obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
>  obj-$(CONFIG_RTC_DRV_S3C)	+= rtc-s3c.o
>  obj-$(CONFIG_RTC_DRV_S5M)	+= rtc-s5m.o
> diff --git a/drivers/rtc/rtc-s32g.c b/drivers/rtc/rtc-s32g.c
> new file mode 100644
> index 000000000000..d6502d8bf616
> --- /dev/null
> +++ b/drivers/rtc/rtc-s32g.c


> +static const struct of_device_id rtc_dt_ids[] = {
> +	{ .compatible = "nxp,s32g2-rtc", .data = &rtc_s32g2_data},
> +	{ .compatible = "nxp,s32g3-rtc", .data = &rtc_s32g2_data},

Considering the data is the same and g3 has a fallback to g2 in the 
binding, you can drop this line.

And really you don't need rtc_s32g2_data because there is only 1 
possible value. But maybe you know some differences which are coming 
soon? Then it's probably okay. Up to the RTC maintainer though.

Rob

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

[Rob Herring]

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

[Rob Herring]

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 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Uwe Kleine-König]

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

Hello,

On Tue, Oct 15, 2024 at 01:51:31PM +0300, Ciprian Costea wrote:
> +static void rtc_disable(struct rtc_priv *priv)

A very generic name for a driver specific function. I'm a big fan of
driver specific prefixes and I wonder why this isn't called
s34g_rtc_disable().

Also a comment about what is actually disabled here would be nice (or
maybe a better name). I hope this doesn't stop the RTC ticking??

> +static struct platform_driver rtc_driver = {
> +	.driver		= {
> +		.name			= "s32g-rtc",
> +		.pm				= &rtc_pm_ops,
> +		.of_match_table = rtc_dt_ids,
> +	},
> +	.probe		= rtc_probe,
> +	.remove_new	= rtc_remove,
> +};

After commit 0edb555a65d1 ("platform: Make platform_driver::remove()
return void") .remove() is (again) the right callback to implement for
platform drivers. Please just drop "_new".

Best regards
Uwe

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

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

[Alexandre Belloni]

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 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Nobuhiro Iwamatsu]

Hello,

2024年10月15日(火) 19:52 Ciprian Costea <ciprianmarian.costea@oss.nxp.com>:
>
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>
> Add a RTC driver for NXP S32G2/S32G3 SoCs.
>
> The RTC module is used to enable Suspend to RAM (STR) support
> on NXP S32G2/S32G3 SoC based boards.
> RTC tracks clock time during system suspend.
>
> RTC from S32G2/S32G3 is not battery-powered and it is not
> kept alive during system reset.
>
> Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
> Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@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>
> ---
>  drivers/rtc/Kconfig    |  11 +
>  drivers/rtc/Makefile   |   1 +
>  drivers/rtc/rtc-s32g.c | 778 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 790 insertions(+)
>  create mode 100644 drivers/rtc/rtc-s32g.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e87c3d74565c..18fc3577f6cd 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -2054,4 +2054,15 @@ config RTC_DRV_SSD202D
>           This driver can also be built as a module, if so, the module
>           will be called "rtc-ssd20xd".
>
> +config RTC_DRV_S32G
> +       tristate "RTC driver for S32G2/S32G3 SoCs"
> +       depends on ARCH_S32 || COMPILE_TEST
> +       depends on COMMON_CLK
> +       help
> +         Say yes to enable RTC driver for platforms based on the
> +         S32G2/S32G3 SoC family.
> +
> +         This RTC module can be used as a wakeup source.
> +         Please note that it is not battery-powered.
> +
>  endif # RTC_CLASS
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 8ee79cb18322..a63d010a753c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -158,6 +158,7 @@ obj-$(CONFIG_RTC_DRV_RX8025)        += rtc-rx8025.o
>  obj-$(CONFIG_RTC_DRV_RX8111)   += rtc-rx8111.o
>  obj-$(CONFIG_RTC_DRV_RX8581)   += rtc-rx8581.o
>  obj-$(CONFIG_RTC_DRV_RZN1)     += rtc-rzn1.o
> +obj-$(CONFIG_RTC_DRV_S32G)     += rtc-s32g.o
>  obj-$(CONFIG_RTC_DRV_S35390A)  += rtc-s35390a.o
>  obj-$(CONFIG_RTC_DRV_S3C)      += rtc-s3c.o
>  obj-$(CONFIG_RTC_DRV_S5M)      += rtc-s5m.o
> diff --git a/drivers/rtc/rtc-s32g.c b/drivers/rtc/rtc-s32g.c
> new file mode 100644
> index 000000000000..d6502d8bf616
> --- /dev/null
> +++ b/drivers/rtc/rtc-s32g.c

[...]

> +enum {
> +       DIV1 = 1,
> +       DIV32 = 32,
> +       DIV512 = 512,
> +       DIV512_32 = 16384
> +};
> +
> +struct rtc_time_base {
> +       s64 sec;
> +       u64 cycles;
> +       u64 rollovers;
> +       struct rtc_time tm;
> +};
> +
> +struct rtc_priv {
> +       struct rtc_device *rdev;
> +       struct device *dev;

You don't have to have 'struct device' here. You can refer to 'struct
device' from
member variable dev.parent of 'struct rtc_device'.

Best regards,
  Nobuhiro

-- 
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org / kernel.org}
   GPG ID: 32247FBB40AD1FA6

Re: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 10/16/2024 12:04 AM, Rob Herring wrote:

Hello Rob,

Thanks for your careful review!

> On Tue, Oct 15, 2024 at 01:51:31PM +0300, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> Add a RTC driver for NXP S32G2/S32G3 SoCs.
>>
>> The RTC module is used to enable Suspend to RAM (STR) support
>> on NXP S32G2/S32G3 SoC based boards.
>> RTC tracks clock time during system suspend.
>>
>> RTC from S32G2/S32G3 is not battery-powered and it is not
>> kept alive during system reset.
>>
>> Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
>> Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@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>
>> ---
>>   drivers/rtc/Kconfig    |  11 +
>>   drivers/rtc/Makefile   |   1 +
>>   drivers/rtc/rtc-s32g.c | 778 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 790 insertions(+)
>>   create mode 100644 drivers/rtc/rtc-s32g.c
> 
> 
>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>> index 8ee79cb18322..a63d010a753c 100644
>> --- a/drivers/rtc/Makefile
>> +++ b/drivers/rtc/Makefile
>> @@ -158,6 +158,7 @@ obj-$(CONFIG_RTC_DRV_RX8025)	+= rtc-rx8025.o
>>   obj-$(CONFIG_RTC_DRV_RX8111)	+= rtc-rx8111.o
>>   obj-$(CONFIG_RTC_DRV_RX8581)	+= rtc-rx8581.o
>>   obj-$(CONFIG_RTC_DRV_RZN1)	+= rtc-rzn1.o
>> +obj-$(CONFIG_RTC_DRV_S32G)	+= rtc-s32g.o
>>   obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
>>   obj-$(CONFIG_RTC_DRV_S3C)	+= rtc-s3c.o
>>   obj-$(CONFIG_RTC_DRV_S5M)	+= rtc-s5m.o
>> diff --git a/drivers/rtc/rtc-s32g.c b/drivers/rtc/rtc-s32g.c
>> new file mode 100644
>> index 000000000000..d6502d8bf616
>> --- /dev/null
>> +++ b/drivers/rtc/rtc-s32g.c
> 
> 
>> +static const struct of_device_id rtc_dt_ids[] = {
>> +	{ .compatible = "nxp,s32g2-rtc", .data = &rtc_s32g2_data},
>> +	{ .compatible = "nxp,s32g3-rtc", .data = &rtc_s32g2_data},
> 
> Considering the data is the same and g3 has a fallback to g2 in the
> binding, you can drop this line.

Correct, I will update in V2.

> 
> And really you don't need rtc_s32g2_data because there is only 1
> possible value. But maybe you know some differences which are coming
> soon? Then it's probably okay. Up to the RTC maintainer though.
> 

I understand your point. I've considered adding 'rtc_s32g2_data' because 
for both S32G2 and S32G3 SoCs, one entry in the clock source mux is 
reserved.
Since this is IMHO a SoC limiation, and future SoCs which may use this 
driver may not have this limitation I've considered adding this 
limitation as a quirk.

> Rob



Best Regards,
Ciprian

Re: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 10/17/2024 11:34 AM, Nobuhiro Iwamatsu wrote:

Hello Nobuhiro,

Thanks for your review!

> Hello,
> 
> 2024年10月15日(火) 19:52 Ciprian Costea <ciprianmarian.costea@oss.nxp.com>:
>>
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> Add a RTC driver for NXP S32G2/S32G3 SoCs.
>>
>> The RTC module is used to enable Suspend to RAM (STR) support
>> on NXP S32G2/S32G3 SoC based boards.
>> RTC tracks clock time during system suspend.
>>
>> RTC from S32G2/S32G3 is not battery-powered and it is not
>> kept alive during system reset.
>>
>> Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com>
>> Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@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>
>> ---
>>   drivers/rtc/Kconfig    |  11 +
>>   drivers/rtc/Makefile   |   1 +
>>   drivers/rtc/rtc-s32g.c | 778 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 790 insertions(+)
>>   create mode 100644 drivers/rtc/rtc-s32g.c
>>
>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> index e87c3d74565c..18fc3577f6cd 100644
>> --- a/drivers/rtc/Kconfig
>> +++ b/drivers/rtc/Kconfig
>> @@ -2054,4 +2054,15 @@ config RTC_DRV_SSD202D
>>            This driver can also be built as a module, if so, the module
>>            will be called "rtc-ssd20xd".
>>
>> +config RTC_DRV_S32G
>> +       tristate "RTC driver for S32G2/S32G3 SoCs"
>> +       depends on ARCH_S32 || COMPILE_TEST
>> +       depends on COMMON_CLK
>> +       help
>> +         Say yes to enable RTC driver for platforms based on the
>> +         S32G2/S32G3 SoC family.
>> +
>> +         This RTC module can be used as a wakeup source.
>> +         Please note that it is not battery-powered.
>> +
>>   endif # RTC_CLASS
>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>> index 8ee79cb18322..a63d010a753c 100644
>> --- a/drivers/rtc/Makefile
>> +++ b/drivers/rtc/Makefile
>> @@ -158,6 +158,7 @@ obj-$(CONFIG_RTC_DRV_RX8025)        += rtc-rx8025.o
>>   obj-$(CONFIG_RTC_DRV_RX8111)   += rtc-rx8111.o
>>   obj-$(CONFIG_RTC_DRV_RX8581)   += rtc-rx8581.o
>>   obj-$(CONFIG_RTC_DRV_RZN1)     += rtc-rzn1.o
>> +obj-$(CONFIG_RTC_DRV_S32G)     += rtc-s32g.o
>>   obj-$(CONFIG_RTC_DRV_S35390A)  += rtc-s35390a.o
>>   obj-$(CONFIG_RTC_DRV_S3C)      += rtc-s3c.o
>>   obj-$(CONFIG_RTC_DRV_S5M)      += rtc-s5m.o
>> diff --git a/drivers/rtc/rtc-s32g.c b/drivers/rtc/rtc-s32g.c
>> new file mode 100644
>> index 000000000000..d6502d8bf616
>> --- /dev/null
>> +++ b/drivers/rtc/rtc-s32g.c
> 
> [...]
> 
>> +enum {
>> +       DIV1 = 1,
>> +       DIV32 = 32,
>> +       DIV512 = 512,
>> +       DIV512_32 = 16384
>> +};
>> +
>> +struct rtc_time_base {
>> +       s64 sec;
>> +       u64 cycles;
>> +       u64 rollovers;
>> +       struct rtc_time tm;
>> +};
>> +
>> +struct rtc_priv {
>> +       struct rtc_device *rdev;
>> +       struct device *dev;
> 
> You don't have to have 'struct device' here. You can refer to 'struct
> device' from
> member variable dev.parent of 'struct rtc_device'.
> 

Thanks for your suggestion. I will update accordingly in V3.

> Best regards,
>    Nobuhiro
> 

Best Regards,
Ciprian

Re: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 10/16/2024 12:42 PM, Uwe Kleine-König wrote:

Hello Uwe,

Thanks for your review!

> Hello,
> 
> On Tue, Oct 15, 2024 at 01:51:31PM +0300, Ciprian Costea wrote:
>> +static void rtc_disable(struct rtc_priv *priv)
> 
> A very generic name for a driver specific function. I'm a big fan of
> driver specific prefixes and I wonder why this isn't called
> s34g_rtc_disable().
> 
> Also a comment about what is actually disabled here would be nice (or
> maybe a better name). I hope this doesn't stop the RTC ticking??
> 

Indeed, 's32g_rtc_disable' would be a better name. I will update in V3 
and add a comment before the function definition.

It does stop the RTC running counter because as per RTC module 
documentation from S32G2/S32G3 SoC Reference Manual, the clock source 
and divisors selection should be performed with RTC counter disabled in 
order to not cause any synchronization issues.

So following this limitation, when the clock source is switched (as it 
is the case for S32G2 and S32G3 and Suspend to RAM) the RTC counter 
should be disabled.

>> +static struct platform_driver rtc_driver = {
>> +	.driver		= {
>> +		.name			= "s32g-rtc",
>> +		.pm				= &rtc_pm_ops,
>> +		.of_match_table = rtc_dt_ids,
>> +	},
>> +	.probe		= rtc_probe,
>> +	.remove_new	= rtc_remove,
>> +};
> 
> After commit 0edb555a65d1 ("platform: Make platform_driver::remove()
> return void") .remove() is (again) the right callback to implement for
> platform drivers. Please just drop "_new".
> 

Thanks for pointing this out. I was not aware of this change. I will 
update in V3.

> Best regards
> Uwe

Best Regards,
Ciprian

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

[Ciprian Marian Costea]

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

Re: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[kernel test robot]

Hi Ciprian,

kernel test robot noticed the following build warnings:

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on robh/for-next arm64/for-next/core linus/master v6.12-rc3 next-20241018]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ciprian-Costea/dt-bindings-rtc-add-schema-for-NXP-S32G2-S32G3-SoCs/20241015-185302
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20241015105133.656360-3-ciprianmarian.costea%40oss.nxp.com
patch subject: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support
config: alpha-randconfig-r071-20241019 (https://download.01.org/0day-ci/archive/20241019/202410191711.oc5s2ZYc-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410191711.oc5s2ZYc-lkp@intel.com/

smatch warnings:
drivers/rtc/rtc-s32g.c:221 s32g_rtc_read_time() warn: unsigned 'sec' is never less than zero.
drivers/rtc/rtc-s32g.c:221 s32g_rtc_read_time() warn: error code type promoted to positive: 'sec'
drivers/rtc/rtc-s32g.c:239 s32g_rtc_read_alarm() warn: unsigned 'sec' is never less than zero.
drivers/rtc/rtc-s32g.c:239 s32g_rtc_read_alarm() warn: error code type promoted to positive: 'sec'

vim +/sec +221 drivers/rtc/rtc-s32g.c

   210	
   211	static int s32g_rtc_read_time(struct device *dev,
   212				      struct rtc_time *tm)
   213	{
   214		struct rtc_priv *priv = dev_get_drvdata(dev);
   215		u64 sec;
   216	
   217		if (!tm)
   218			return -EINVAL;
   219	
   220		sec = s32g_rtc_get_time_or_alrm(priv, RTCCNT_OFFSET);
 > 221		if (sec < 0)
   222			return -EINVAL;
   223	
   224		rtc_time64_to_tm(sec, tm);
   225	
   226		return 0;
   227	}
   228	
   229	static int s32g_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
   230	{
   231		struct rtc_priv *priv = dev_get_drvdata(dev);
   232		u32 rtcc, sec_left;
   233		u64 sec;
   234	
   235		if (!alrm)
   236			return -EINVAL;
   237	
   238		sec = s32g_rtc_get_time_or_alrm(priv, RTCVAL_OFFSET);
 > 239		if (sec < 0)
   240			return -EINVAL;
   241	
   242		rtc_time64_to_tm(sec, &alrm->time);
   243	
   244		rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
   245		alrm->enabled = sec && (rtcc & RTCC_RTCIE);
   246	
   247		alrm->pending = 0;
   248		if (alrm->enabled && !get_time_left(dev, priv, &sec_left))
   249			alrm->pending = !!sec_left;
   250	
   251		return 0;
   252	}
   253	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[kernel test robot]

Hi Ciprian,

kernel test robot noticed the following build errors:

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on robh/for-next arm64/for-next/core linus/master v6.12-rc3 next-20241018]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ciprian-Costea/dt-bindings-rtc-add-schema-for-NXP-S32G2-S32G3-SoCs/20241015-185302
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20241015105133.656360-3-ciprianmarian.costea%40oss.nxp.com
patch subject: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support
config: arm-randconfig-r123-20241018 (https://download.01.org/0day-ci/archive/20241019/202410191710.kHHObl1i-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20241019/202410191710.kHHObl1i-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410191710.kHHObl1i-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/rtc/rtc-s32g.o: in function `cycles_to_sec':
>> rtc-s32g.c:(.text.cycles_to_sec+0x60): undefined reference to `__aeabi_uldivmod'
   arm-linux-gnueabi-ld: drivers/rtc/rtc-s32g.o: in function `rtc_clk_src_switch':
   rtc-s32g.c:(.text.rtc_clk_src_switch+0x110): undefined reference to `__aeabi_uldivmod'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

[kernel test robot]

Hi Ciprian,

kernel test robot noticed the following build errors:

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on robh/for-next arm64/for-next/core linus/master v6.12-rc3 next-20241018]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ciprian-Costea/dt-bindings-rtc-add-schema-for-NXP-S32G2-S32G3-SoCs/20241015-185302
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20241015105133.656360-3-ciprianmarian.costea%40oss.nxp.com
patch subject: [PATCH v2 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support
config: powerpc-randconfig-001-20241019 (https://download.01.org/0day-ci/archive/20241019/202410192150.qZi3WkG1-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241019/202410192150.qZi3WkG1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410192150.qZi3WkG1-lkp@intel.com/

All errors (new ones prefixed by >>):

   powerpc-linux-ld: drivers/rtc/rtc-s32g.o: in function `s32g_rtc_get_time_or_alrm':
>> drivers/rtc/rtc-s32g.c:105:(.text+0x1a0): undefined reference to `__udivdi3'
   powerpc-linux-ld: drivers/rtc/rtc-s32g.o: in function `get_time_left':
   drivers/rtc/rtc-s32g.c:105:(.text+0x6a8): undefined reference to `__udivdi3'
   powerpc-linux-ld: drivers/rtc/rtc-s32g.o: in function `sec_to_rtcval':
   drivers/rtc/rtc-s32g.c:105:(.text+0xbfc): undefined reference to `__udivdi3'
   powerpc-linux-ld: drivers/rtc/rtc-s32g.o: in function `rtc_clk_src_switch':
   drivers/rtc/rtc-s32g.c:387:(.text+0x1630): undefined reference to `__udivdi3'

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [y]:
   - RESOURCE_KUNIT_TEST [=y] && RUNTIME_TESTING_MENU [=y] && KUNIT [=y]


vim +105 drivers/rtc/rtc-s32g.c

   102	
   103	static u64 cycles_to_sec(u64 hz, u64 cycles)
   104	{
 > 105		return cycles / hz;
   106	}
   107	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Rob Herring]

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

[Ciprian Marian Costea]

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