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

[PATCH v6 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.

RTC tracks clock time during system suspend. It is used as 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 V6:
- Fixed a coding style issue regarding kernel doc reported by kernel test robot
- Refactored small sections from the S32G RTC driver without impacting
  functionality
- Fixed an error probe path issue on S32G RTC driver.
- Added 'rtc' schema in S32G-RTC bindings

Changes in V5:
- Removed rollover support.
- Removed clock switching support between Runtime and Suspend. A clock source
  which is always available has been used instead.
- Enabled 512 value RTC hardware frequency divisor to achieve higher rollover
  time
- Removed unneeded 'remove' callback.
- Decreased driver print verbosity on error paths.
- Provided 'clock-names' actual names in bindings documentation
- Remove __maybe_unused notations. Used the DEFINE_SIMPLE_DEV_PM_OPS() and
  pm_sleep_ptr() macros to handle the .suspend/.resume callbacks.
- Fixed some alignment issues.

Changes in V4:
- Dropped 'assigned-*' clock management approach. Simplified RTC Runtime
  and Suspend/Standby clock configuration.
- Simplified error paths on probe function
- Removed node label from bindings example
- Several cosmetic coding style fixes

Changes in V3:
- Removed 'nxp,s32g3-rtc' compatible string
- Change to 'remove' callback from 'remove_new'
- Used 'dev.parent' from 'struct rtc_device' instead of defining a
  specific 'struct device' in driver data
- Fixed several errors reported by kernel test robot
- Removed 'assigned-clocks', 'assigned-clock-parents' and
  'assigned-clock-rates' from required properties in the binding
  documentation.
- Refactored S32G RTC driver such that a default clock source and
  divisors configuration will be applied in case 'assigned-clocks' and
  'assigned-clock-parents' properties are missing.

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 |  72 +++
 MAINTAINERS                                   |   2 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/rtc/Kconfig                           |  11 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-s32g.c                        | 529 ++++++++++++++++++
 6 files changed, 616 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
 create mode 100644 drivers/rtc/rtc-s32g.c

-- 
2.45.2

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

[Ciprian Costea]

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

RTC tracks clock time during system suspend and it is used as a wakeup
source on S32G2/S32G3 architecture.

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

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 | 72 +++++++++++++++++++
 1 file changed, 72 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..40fd2fa298fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
@@ -0,0 +1,72 @@
+# 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>
+
+description:
+  RTC hardware module present on S32G2/S32G3 SoCs is used as a wakeup source.
+  It is not kept alive during system reset and it is not battery-powered.
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - nxp,s32g2-rtc
+      - items:
+          - const: nxp,s32g3-rtc
+          - const: nxp,s32g2-rtc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: ipg clock drives the access to the RTC iomapped registers
+      - description: Clock source for the RTC module. Can be selected between
+          4 different clock sources using an integrated hardware mux.
+          On S32G2/S32G3 SoCs, 'source0' is the SIRC clock (~32KHz) and it is
+          available during standby and runtime. 'source1' is reserved and cannot
+          be used. 'source2' is the FIRC clock and it is only available during
+          runtime providing a better resolution (~48MHz). 'source3' is an external
+          RTC clock source which can be additionally added in hardware.
+
+  clock-names:
+    items:
+      - const: ipg
+      - enum: [ source0, source1, source2, source3 ]
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    rtc@40060000 {
+        compatible = "nxp,s32g3-rtc",
+                     "nxp,s32g2-rtc";
+        reg = <0x40060000 0x1000>;
+        interrupts = <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clks 54>, <&clks 55>;
+        clock-names = "ipg", "source0";
+    };
-- 
2.45.2

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

[Rob Herring (Arm)]

On Fri, 06 Dec 2024 09:09:52 +0200, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> RTC tracks clock time during system suspend and it is used as a wakeup
> source on S32G2/S32G3 architecture.
> 
> RTC from S32G2/S32G3 is not battery-powered and it is not kept alive
> during system reset.
> 
> 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 | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/nxp,s32g-rtc.yaml
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH v6 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.

RTC tracks clock time during system suspend. It can be a wakeup source
for the S32G2/S32G3 SoC based boards.

The RTC module 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 | 529 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 541 insertions(+)
 create mode 100644 drivers/rtc/rtc-s32g.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index a60bcc791a48..25ee7c6d8748 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -2103,4 +2103,15 @@ config RTC_DRV_AMLOGIC_A4
 	  This driver can also be built as a module. If so, the module
 	  will be called "rtc-amlogic-a4".
 
+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 489b4ab07068..e4b616ecd5ce 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -161,6 +161,7 @@ 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_RENESAS_RTCA3)	+= rtc-renesas-rtca3.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..0989b6f2a613
--- /dev/null
+++ b/drivers/rtc/rtc-s32g.c
@@ -0,0 +1,529 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/iopoll.h>
+#include <linux/math64.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.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				BIT(30)
+#define RTCC_APIEN				BIT(15)
+#define RTCC_APIIE				BIT(14)
+#define RTCC_CLKSEL_MASK		GENMASK(13, 12)
+#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 RTCCNT_MAX_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 source1 reserved and
+ * should not be used.
+ */
+#define RTC_CLK_SRC1_RESERVED		BIT(1)
+
+enum {
+	DIV1 = 1,
+	DIV32 = 32,
+	DIV512 = 512,
+	DIV512_32 = 16384
+};
+
+static const char *rtc_clk_src[RTC_CLK_MUX_SIZE] = {
+	"source0",
+	"source1",
+	"source2",
+	"source3"
+};
+
+struct rtc_time_base {
+	s64 sec;
+	u64 cycles;
+	struct rtc_time tm;
+};
+
+struct rtc_priv {
+	struct rtc_device *rdev;
+	void __iomem *rtc_base;
+	struct clk *ipg;
+	struct clk *clk_src;
+	const struct rtc_soc_data *rtc_data;
+	struct rtc_time_base base;
+	u64 rtc_hz;
+	int irq;
+	int clk_src_idx;
+};
+
+struct rtc_soc_data {
+	u32 clk_div;
+	u32 reserved_clk_mask;
+};
+
+static const struct rtc_soc_data rtc_s32g2_data = {
+	.clk_div = DIV512,
+	.reserved_clk_mask = RTC_CLK_SRC1_RESERVED,
+};
+
+static u64 cycles_to_sec(u64 hz, u64 cycles)
+{
+	return div_u64(cycles, hz);
+}
+
+/**
+ * sec_to_rtcval - Convert a number of seconds to a value suitable for
+ * RTCVAL in our clock's
+ * current configuration.
+ * @priv: Pointer to the 'rtc_priv' structure
+ * @seconds: Number of seconds to convert
+ * @rtcval: The value to go into RTCVAL[RTCVAL]
+ *
+ * Return: 0 for success, -EINVAL if @seconds push the counter past the
+ *          32bit register range
+ */
+static int sec_to_rtcval(const struct rtc_priv *priv,
+			 unsigned long seconds, u32 *rtcval)
+{
+	u32 delta_cnt;
+
+	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_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;
+	*rtcval = delta_cnt + ioread32(priv->rtc_base + RTCCNT_OFFSET);
+
+	return 0;
+}
+
+static irqreturn_t s32g_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_RTCF) {
+		iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
+		iowrite32(status | RTCS_RTCF, priv->rtc_base + RTCS_OFFSET);
+		rtc_update_irq(priv->rdev, 1, RTC_AF);
+	}
+
+	if (status & RTCS_APIF) {
+		iowrite32(status | RTCS_APIF, priv->rtc_base + RTCS_OFFSET);
+		rtc_update_irq(priv->rdev, 1, RTC_PF);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
+				     u32 offset)
+{
+	u32 counter;
+
+	counter = ioread32(priv->rtc_base + offset);
+
+	if (counter < priv->base.cycles)
+		return -EINVAL;
+
+	counter -= priv->base.cycles;
+
+	return priv->base.sec + cycles_to_sec(priv->rtc_hz, counter);
+}
+
+static int s32g_rtc_read_time(struct device *dev,
+			      struct rtc_time *tm)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	s64 sec;
+
+	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, rtccnt, rtcval;
+	s64 sec;
+
+	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) {
+		rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
+		rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET);
+
+		if (rtccnt < rtcval)
+			alrm->pending = 1;
+	}
+
+	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->irq)
+		return -EIO;
+
+	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;
+	u32 rtcval, rtcs;
+	int ret = 0;
+
+	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);
+	ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval);
+	if (ret) {
+		dev_warn(dev, "Alarm is set too far in the future\n");
+		return -ERANGE;
+	}
+
+	ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC),
+				0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET);
+	if (ret)
+		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);
+
+	priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET);
+	priv->base.sec = rtc_tm_to_time64(time);
+
+	return 0;
+}
+
+/*
+ * Disable the 32-bit free running counter.
+ * This allows Clock Source and Divisors selection
+ * to be performed without causing synchronization issues.
+ */
+static void s32g_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 s32g_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 rtc_clk_src_setup(struct rtc_priv *priv)
+{
+	u32 rtcc = 0;
+
+	if (priv->rtc_data->reserved_clk_mask & (1 << priv->clk_src_idx))
+		return -EOPNOTSUPP;
+
+	rtcc = FIELD_PREP(RTCC_CLKSEL_MASK, priv->clk_src_idx);
+
+	switch (priv->rtc_data->clk_div) {
+	case DIV512_32:
+		rtcc |= RTCC_DIV512EN;
+		rtcc |= RTCC_DIV32EN;
+		break;
+	case DIV512:
+		rtcc |= RTCC_DIV512EN;
+		break;
+	case DIV32:
+		rtcc |= RTCC_DIV32EN;
+		break;
+	case DIV1:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	rtcc |= RTCC_RTCIE;
+	/*
+	 * Make sure the CNTEN is 0 before we configure
+	 * the clock source and dividers.
+	 */
+	s32g_rtc_disable(priv);
+	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
+	s32g_rtc_enable(priv);
+
+	return 0;
+}
+
+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 int rtc_clk_dts_setup(struct rtc_priv *priv,
+			     struct device *dev)
+{
+	int i;
+
+	priv->ipg = devm_clk_get_enabled(dev, "ipg");
+	if (IS_ERR(priv->ipg))
+		return dev_err_probe(dev, PTR_ERR(priv->ipg),
+				"Failed to get 'ipg' clock\n");
+
+	for (i = 0; i < RTC_CLK_MUX_SIZE; i++) {
+		priv->clk_src = devm_clk_get_enabled(dev, rtc_clk_src[i]);
+		if (!IS_ERR(priv->clk_src)) {
+			priv->clk_src_idx = i;
+			break;
+		}
+	}
+
+	if (IS_ERR(priv->clk_src))
+		return dev_err_probe(dev, PTR_ERR(priv->clk_src),
+				"Failed to get rtc module clock source\n");
+
+	return 0;
+}
+
+static int s32g_rtc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rtc_priv *priv;
+	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 PTR_ERR(priv->rtc_base);
+
+	device_init_wakeup(dev, true);
+
+	ret = rtc_clk_dts_setup(priv, dev);
+	if (ret)
+		return ret;
+
+	priv->rdev = devm_rtc_allocate_device(dev);
+	if (IS_ERR(priv->rdev))
+		return PTR_ERR(priv->rdev);
+
+	ret = rtc_clk_src_setup(priv);
+	if (ret)
+		return ret;
+
+	priv->rtc_hz = clk_get_rate(priv->clk_src);
+	if (!priv->rtc_hz) {
+		dev_err(dev, "Failed to get RTC frequency\n");
+		ret = -EINVAL;
+		goto disable_rtc;
+	}
+
+	priv->rtc_hz /= priv->rtc_data->clk_div;
+
+	platform_set_drvdata(pdev, priv);
+	priv->rdev->ops = &rtc_ops;
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0) {
+		ret = priv->irq;
+		goto disable_rtc;
+	}
+
+	ret = devm_request_irq(dev, priv->irq,
+			       s32g_rtc_handler, 0, dev_name(dev), pdev);
+	if (ret) {
+		dev_err(dev, "Request interrupt %d failed, error: %d\n",
+			priv->irq, ret);
+		goto disable_rtc;
+	}
+
+	ret = devm_rtc_register_device(priv->rdev);
+	if (ret)
+		goto disable_rtc;
+
+	return 0;
+
+disable_rtc:
+	s32g_rtc_disable(priv);
+	return ret;
+}
+
+static void s32g_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 s32g_rtc_suspend(struct device *dev)
+{
+	struct rtc_priv *init_priv = dev_get_drvdata(dev);
+	struct rtc_priv priv;
+	long long base_sec;
+	u32 rtcval, rtccnt, offset;
+	int ret = 0;
+	u32 sec;
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	/* Save last known timestamp */
+	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));
+
+	rtccnt = ioread32(init_priv->rtc_base + RTCCNT_OFFSET);
+	rtcval = ioread32(init_priv->rtc_base + RTCVAL_OFFSET);
+	offset = rtcval - rtccnt;
+	sec = cycles_to_sec(init_priv->rtc_hz, offset);
+
+	/* 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 = sec_to_rtcval(&priv, sec, &rtcval);
+	if (ret) {
+		dev_warn(dev, "Alarm is too far in the future\n");
+		return -ERANGE;
+	}
+
+	s32g_enable_api_irq(dev, 1);
+	iowrite32(offset, priv.rtc_base + APIVAL_OFFSET);
+
+	return ret;
+}
+
+static int s32g_rtc_resume(struct device *dev)
+{
+	struct rtc_priv *priv = dev_get_drvdata(dev);
+	int ret;
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	/* Disable wake-up interrupts */
+	s32g_enable_api_irq(dev, 0);
+
+	ret = rtc_clk_src_setup(priv);
+	if (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},
+	{ /* sentinel */ },
+};
+
+static DEFINE_SIMPLE_DEV_PM_OPS(s32g_rtc_pm_ops,
+			 s32g_rtc_suspend, s32g_rtc_resume);
+
+static struct platform_driver s32g_rtc_driver = {
+	.driver		= {
+		.name			= "s32g-rtc",
+		.pm				= pm_sleep_ptr(&s32g_rtc_pm_ops),
+		.of_match_table = rtc_dt_ids,
+	},
+	.probe		= s32g_rtc_probe,
+};
+module_platform_driver(s32g_rtc_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
+MODULE_LICENSE("GPL");
-- 
2.45.2

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

[Arnd Bergmann]

On Fri, Dec 6, 2024, at 08:09, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>
> Add a RTC driver for NXP S32G2/S32G3 SoCs.
>
> RTC tracks clock time during system suspend. It can be a wakeup source
> for the S32G2/S32G3 SoC based boards.
>
> The RTC module 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>

I read through the driver and this looks all good to me, but there
are two fairly minor things I noticed:

> +	u64 rtc_hz;

I see the clock rate is a 64-bit value, which is clearly what
comes from the clk interface in the kernel

> +static u64 cycles_to_sec(u64 hz, u64 cycles)
> +{
> +	return div_u64(cycles, hz);
> +}

and you divide by the clk rate to convert the register value
to seconds (as expected)

> +	u32 delta_cnt;
> +
> +	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_VAL))
> +		return -EINVAL;

However, the range of the register value is only 32 bits,
which means there is no need to ever divide it by a 64-bit
number, and with the 32kHz clock in the binding example,
you only have about 37 hours worth of range here.

It would appear that this makes the rtc unsuitable for
storing absolute time across reboots, and only serve during
runtime, which is a limitation you should probably document.

> +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
> +				     u32 offset)
> +{
> +	u32 counter;
> +
> +	counter = ioread32(priv->rtc_base + offset);
> +
> +	if (counter < priv->base.cycles)
> +		return -EINVAL;

If 'counter' wraps every 37 hours, this will inevitably fail,
right? E.g. if priv->base.cycles was already at a large
32-bit number, even reading it shortly later will produce
a small value after the wraparound.

Using something like time_before() should address this,
but I think you may need a custom version that works on
32-bit numbers.

> +static int s32g_rtc_resume(struct device *dev)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Disable wake-up interrupts */
> +	s32g_enable_api_irq(dev, 0);
> +
> +	ret = rtc_clk_src_setup(priv);
> +	if (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);
> +}

This also fails if the system has been suspended for more than
37 hours, right?

One more minor comment: you are using ioread32()/iowrite32()
to access the MMIO registers, which is a bit unusual. I would
suggest changing those to the more common readl()/writel()
that do the exact same thing on arm64.

        Arnd

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

[Ciprian Marian Costea]

On 12/6/2024 10:04 AM, Arnd Bergmann wrote:
> [You don't often get email from arnd@arndb.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Fri, Dec 6, 2024, at 08:09, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> Add a RTC driver for NXP S32G2/S32G3 SoCs.
>>
>> RTC tracks clock time during system suspend. It can be a wakeup source
>> for the S32G2/S32G3 SoC based boards.
>>
>> The RTC module 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>

Hello Arnd,

Thanks you for your review on this patchset.

> 
> I read through the driver and this looks all good to me, but there
> are two fairly minor things I noticed:
> 
>> +     u64 rtc_hz;
> 
> I see the clock rate is a 64-bit value, which is clearly what
> comes from the clk interface in the kernel
> 
>> +static u64 cycles_to_sec(u64 hz, u64 cycles)
>> +{
>> +     return div_u64(cycles, hz);
>> +}
> 
> and you divide by the clk rate to convert the register value
> to seconds (as expected)
> 
>> +     u32 delta_cnt;
>> +
>> +     if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_VAL))
>> +             return -EINVAL;
> 
> However, the range of the register value is only 32 bits,
> which means there is no need to ever divide it by a 64-bit
> number, and with the 32kHz clock in the binding example,
> you only have about 37 hours worth of range here.
> 

I am not sure what is the suggestion here. To cast 'cycles' variable to 
32-bit ?
If yes, indeed 'div_u64' converts 'cycles' (the divisor) to 32-bit so I 
agree it should be u32 instead of u64.
If not, I would prefer to keep using a 64-by-32 division and avoid 
casting 'hz' to 32-bit.

> It would appear that this makes the rtc unsuitable for
> storing absolute time across reboots, and only serve during
> runtime, which is a limitation you should probably document.
> 

Actually there is the option to use DIV512 and/or DIV32 hardware 
divisors for the RTC clock. The driver uses a DIV512 divisor by default 
in order to achieve higher RTC count ranges (by achieving a smaller RTC 
freq). Therefore, the 37 hours become 37 * 512 => ~ 2 years.

However, the rtc limitation of not being persistent during reboot 
remains, due to hardware RTC module registers present of NXP S32G2/S32G3 
SoCs being reset during system reboot. On the other hand, during system 
suspend, the RTC module will keep counting if a clock source is available.

Currently, this limittion is documented as follows:
"RTC tracks clock time during system suspend. It can be a wakeup source 
for the S32G2/S32G3 SoC based boards.

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

>> +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
>> +                                  u32 offset)
>> +{
>> +     u32 counter;
>> +
>> +     counter = ioread32(priv->rtc_base + offset);
>> +
>> +     if (counter < priv->base.cycles)
>> +             return -EINVAL;
> 
> If 'counter' wraps every 37 hours, this will inevitably fail,
> right? E.g. if priv->base.cycles was already at a large
> 32-bit number, even reading it shortly later will produce
> a small value after the wraparound.
> 
> Using something like time_before() should address this,
> but I think you may need a custom version that works on
> 32-bit numbers.
> 

This is correct. Would the following change be acceptable ?

-       if (counter < priv->base.cycles)
-               return -EINVAL;
-
-       counter -= priv->base.cycles;
+       if (counter < priv->base.cycles) {
+               /* A rollover on RTCCTN has occurred */
+               counter += RTCCNT_MAX_VAL - priv->base.cycles;
+               priv->base.cycles = 0;
+       } else {
+               counter -= priv->base.cycles;
+       }


>> +static int s32g_rtc_resume(struct device *dev)
>> +{
>> +     struct rtc_priv *priv = dev_get_drvdata(dev);
>> +     int ret;
>> +
>> +     if (!device_may_wakeup(dev))
>> +             return 0;
>> +
>> +     /* Disable wake-up interrupts */
>> +     s32g_enable_api_irq(dev, 0);
>> +
>> +     ret = rtc_clk_src_setup(priv);
>> +     if (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);
>> +}
> 
> This also fails if the system has been suspended for more than
> 37 hours, right?

Actually, the system would not go into suspend (returning with error) if 
the alarm setting passes the 32-bit / clk_freq range.
The check is added in 'sec_to_rtcval' which is called from the suspend 
routine.

> 
> One more minor comment: you are using ioread32()/iowrite32()
> to access the MMIO registers, which is a bit unusual. I would
> suggest changing those to the more common readl()/writel()
> that do the exact same thing on arm64.
> 
>          Arnd

Makes sense. I will change to readl/writel in V7.

Best Regards,
Ciprian

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

[Arnd Bergmann]

On Fri, Dec 6, 2024, at 13:05, Ciprian Marian Costea wrote:
> On 12/6/2024 10:04 AM, Arnd Bergmann wrote:
>> 
>> However, the range of the register value is only 32 bits,
>> which means there is no need to ever divide it by a 64-bit
>> number, and with the 32kHz clock in the binding example,
>> you only have about 37 hours worth of range here.
>> 
>
> I am not sure what is the suggestion here. To cast 'cycles' variable to 
> 32-bit ?
> If yes, indeed 'div_u64' converts 'cycles' (the divisor) to 32-bit so I 
> agree it should be u32 instead of u64.
> If not, I would prefer to keep using a 64-by-32 division and avoid 
> casting 'hz' to 32-bit.

The confusing bit here is that the extra function just serves to
the dividend 'cycles' from 32-bit to 64-bit, and then calling
div_u64() implicitly casts the dividend 'hz' from 64-bit to
32-bit, so you definitely get a 32-by-32 division already
if the function is inlined properly.

I think storing 'rtc_hz' as a u32 variable and adding a range
check when filling it would help, mainly to save the next reader
from having to understand what is going on.

>> It would appear that this makes the rtc unsuitable for
>> storing absolute time across reboots, and only serve during
>> runtime, which is a limitation you should probably document.
>> 
>
> Actually there is the option to use DIV512 and/or DIV32 hardware 
> divisors for the RTC clock. The driver uses a DIV512 divisor by default 
> in order to achieve higher RTC count ranges (by achieving a smaller RTC 
> freq). Therefore, the 37 hours become 37 * 512 => ~ 2 years.

Ah, makes sense. Can you add comments in an appropriate place in
the code about this?

> However, the rtc limitation of not being persistent during reboot 
> remains, due to hardware RTC module registers present of NXP S32G2/S32G3 
> SoCs being reset during system reboot. On the other hand, during system 
> suspend, the RTC module will keep counting if a clock source is available.
>
> Currently, this limittion is documented as follows:
> "RTC tracks clock time during system suspend. It can be a wakeup source 
> for the S32G2/S32G3 SoC based boards.
>
> The RTC module from S32G2/S32G3 is not battery-powered and it is not 
> kept alive during system reset."

My bad, I really should not have skipped the patch
description ;-)

>> If 'counter' wraps every 37 hours, this will inevitably fail,
>> right? E.g. if priv->base.cycles was already at a large
>> 32-bit number, even reading it shortly later will produce
>> a small value after the wraparound.
>> 
>> Using something like time_before() should address this,
>> but I think you may need a custom version that works on
>> 32-bit numbers.
>> 
>
> This is correct. Would the following change be acceptable ?
>
> -       if (counter < priv->base.cycles)
> -               return -EINVAL;
> -
> -       counter -= priv->base.cycles;
> +       if (counter < priv->base.cycles) {
> +               /* A rollover on RTCCTN has occurred */
> +               counter += RTCCNT_MAX_VAL - priv->base.cycles;
> +               priv->base.cycles = 0;
> +       } else {
> +               counter -= priv->base.cycles;
> +       }

This is the same as just removing the error handling and
relying on unsigned integer overflow semantics.

The usual check we do in time_before()/time_after instead
checks if the elapsed time is less than half the available
range:

#define time_after(a,b)         \
        (typecheck(unsigned long, a) && \
         typecheck(unsigned long, b) && \
         ((long)((b) - (a)) < 0))

>>> +static int s32g_rtc_resume(struct device *dev)
>>> +{
>>> +     struct rtc_priv *priv = dev_get_drvdata(dev);
>>> +     int ret;
>>> +
>>> +     if (!device_may_wakeup(dev))
>>> +             return 0;
>>> +
>>> +     /* Disable wake-up interrupts */
>>> +     s32g_enable_api_irq(dev, 0);
>>> +
>>> +     ret = rtc_clk_src_setup(priv);
>>> +     if (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);
>>> +}
>> 
>> This also fails if the system has been suspended for more than
>> 37 hours, right?
>
> Actually, the system would not go into suspend (returning with error) if 
> the alarm setting passes the 32-bit / clk_freq range.
> The check is added in 'sec_to_rtcval' which is called from the suspend 
> routine.

Who sets that alarm though? Are you relying on custom userspace
for this, or is that something that the kernel already does
that I'm missing?

       Arnd

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

[Ciprian Marian Costea]

On 12/6/2024 2:41 PM, Arnd Bergmann wrote:
> [You don't often get email from arnd@arndb.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Fri, Dec 6, 2024, at 13:05, Ciprian Marian Costea wrote:
>> On 12/6/2024 10:04 AM, Arnd Bergmann wrote:
>>>
>>> However, the range of the register value is only 32 bits,
>>> which means there is no need to ever divide it by a 64-bit
>>> number, and with the 32kHz clock in the binding example,
>>> you only have about 37 hours worth of range here.
>>>
>>
>> I am not sure what is the suggestion here. To cast 'cycles' variable to
>> 32-bit ?
>> If yes, indeed 'div_u64' converts 'cycles' (the divisor) to 32-bit so I
>> agree it should be u32 instead of u64.
>> If not, I would prefer to keep using a 64-by-32 division and avoid
>> casting 'hz' to 32-bit.
> 
> The confusing bit here is that the extra function just serves to
> the dividend 'cycles' from 32-bit to 64-bit, and then calling
> div_u64() implicitly casts the dividend 'hz' from 64-bit to
> 32-bit, so you definitely get a 32-by-32 division already
> if the function is inlined properly.
> 
> I think storing 'rtc_hz' as a u32 variable and adding a range
> check when filling it would help, mainly to save the next reader
> from having to understand what is going on.
> 

The confusion on my end is that I cannot see where 'div_u64() implicitly 
casts the dividend 'hz' from 64-bit to 32-bit' by following the method's 
implementation [1]

But I agree that 'rtc_hz' can be stored into a 32-bit variable with a 
range check added when it is taken from the Linux clock API to avoid any
unneeded abstractions.

[1] 
https://elixir.bootlin.com/linux/v6.12.4/source/include/linux/math64.h#L127

>>> It would appear that this makes the rtc unsuitable for
>>> storing absolute time across reboots, and only serve during
>>> runtime, which is a limitation you should probably document.
>>>
>>
>> Actually there is the option to use DIV512 and/or DIV32 hardware
>> divisors for the RTC clock. The driver uses a DIV512 divisor by default
>> in order to achieve higher RTC count ranges (by achieving a smaller RTC
>> freq). Therefore, the 37 hours become 37 * 512 => ~ 2 years.
> 
> Ah, makes sense. Can you add comments in an appropriate place in
> the code about this?
> 

Sure. I will add such comment in the S32G RTC driver in V7.

>> However, the rtc limitation of not being persistent during reboot
>> remains, due to hardware RTC module registers present of NXP S32G2/S32G3
>> SoCs being reset during system reboot. On the other hand, during system
>> suspend, the RTC module will keep counting if a clock source is available.
>>
>> Currently, this limittion is documented as follows:
>> "RTC tracks clock time during system suspend. It can be a wakeup source
>> for the S32G2/S32G3 SoC based boards.
>>
>> The RTC module from S32G2/S32G3 is not battery-powered and it is not
>> kept alive during system reset."
> 
> My bad, I really should not have skipped the patch
> description ;-)
> 
>>> If 'counter' wraps every 37 hours, this will inevitably fail,
>>> right? E.g. if priv->base.cycles was already at a large
>>> 32-bit number, even reading it shortly later will produce
>>> a small value after the wraparound.
>>>
>>> Using something like time_before() should address this,
>>> but I think you may need a custom version that works on
>>> 32-bit numbers.
>>>
>>
>> This is correct. Would the following change be acceptable ?
>>
>> -       if (counter < priv->base.cycles)
>> -               return -EINVAL;
>> -
>> -       counter -= priv->base.cycles;
>> +       if (counter < priv->base.cycles) {
>> +               /* A rollover on RTCCTN has occurred */
>> +               counter += RTCCNT_MAX_VAL - priv->base.cycles;
>> +               priv->base.cycles = 0;
>> +       } else {
>> +               counter -= priv->base.cycles;
>> +       }
> 
> This is the same as just removing the error handling and
> relying on unsigned integer overflow semantics.
> 
> The usual check we do in time_before()/time_after instead
> checks if the elapsed time is less than half the available
> range:
> 
> #define time_after(a,b)         \
>          (typecheck(unsigned long, a) && \
>           typecheck(unsigned long, b) && \
>           ((long)((b) - (a)) < 0))
> 

Ok. Thanks for the suggestion. I will look into using 
'time_before()/time_after()' API instead of directly checking via 
comparison operators.

>>>> +static int s32g_rtc_resume(struct device *dev)
>>>> +{
>>>> +     struct rtc_priv *priv = dev_get_drvdata(dev);
>>>> +     int ret;
>>>> +
>>>> +     if (!device_may_wakeup(dev))
>>>> +             return 0;
>>>> +
>>>> +     /* Disable wake-up interrupts */
>>>> +     s32g_enable_api_irq(dev, 0);
>>>> +
>>>> +     ret = rtc_clk_src_setup(priv);
>>>> +     if (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);
>>>> +}
>>>
>>> This also fails if the system has been suspended for more than
>>> 37 hours, right?
>>
>> Actually, the system would not go into suspend (returning with error) if
>> the alarm setting passes the 32-bit / clk_freq range.
>> The check is added in 'sec_to_rtcval' which is called from the suspend
>> routine.
> 
> Who sets that alarm though? Are you relying on custom userspace
> for this, or is that something that the kernel already does
> that I'm missing?
> 
>         Arnd

The test usage is via 'rtcwake' [2] userspace tool.
I've detailed a bit the testing scenario in the cover letter for this 
patchset [3]:

"
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
#
"

[2] https://man7.org/linux/man-pages/man8/rtcwake.8.html
[3] 
https://lore.kernel.org/all/20241206070955.1503412-1-ciprianmarian.costea@oss.nxp.com/

Best Regards,
Ciprian

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

[Arnd Bergmann]

On Mon, Dec 9, 2024, at 18:17, Ciprian Marian Costea wrote:
> On 12/6/2024 2:41 PM, Arnd Bergmann wrote:

>> I think storing 'rtc_hz' as a u32 variable and adding a range
>> check when filling it would help, mainly to save the next reader
>> from having to understand what is going on.
>> 
>
> The confusion on my end is that I cannot see where 'div_u64() implicitly 
> casts the dividend 'hz' from 64-bit to 32-bit' by following the method's 
> implementation [1]

I mean passing a 64-bit variable into a function that takes a
32-bit argument truncates the range.

> But I agree that 'rtc_hz' can be stored into a 32-bit variable with a 
> range check added when it is taken from the Linux clock API to avoid any
> unneeded abstractions.

ok

>> 
>> This is the same as just removing the error handling and
>> relying on unsigned integer overflow semantics.
>> 
>> The usual check we do in time_before()/time_after instead
>> checks if the elapsed time is less than half the available
>> range:
>> 
>> #define time_after(a,b)         \
>>          (typecheck(unsigned long, a) && \
>>           typecheck(unsigned long, b) && \
>>           ((long)((b) - (a)) < 0))
>> 
>
> Ok. Thanks for the suggestion. I will look into using 
> 'time_before()/time_after()' API instead of directly checking via 
> comparison operators.

To be clear: you can't directly use time_before() here because
that takes an 'unsigned long' argument, so you want the
same logic, but for u32 values. I have not found an existing
helper for that, but it's possible I missed it.

>> Who sets that alarm though? Are you relying on custom userspace
>> for this, or is that something that the kernel already does
>> that I'm missing?
>
> The test usage is via 'rtcwake' [2] userspace tool.
> I've detailed a bit the testing scenario in the cover letter for this 
> patchset [3]:
>
> "
> 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

Got it. I feel this also needs either some documentation in
the source code, or some infrastructure in the rtc layer if
this is a common problem in other drivers as well. If there
is a maximum time that the system can be suspended for without
a wakeup, why not just set an earlier wakeup in the kernel
when you have all the information for it?

Or maybe this should not actually be an 'rtc' driver at all?
In the old days, we used drivers like
arch/arm/mach-omap1/timer32k.c to register a handler
for read_persistent_clock64(), which completely bypasses
the RTC layer and provides both automatic wakeup and more
accurate accounting of sleep time. 

Another example was the tegra clocksource driver, which used
to use read_persistent_clock64() but changed to being
a CLOCK_SOURCE_SUSPEND_NONSTOP source in 95170f0708f2
("clocksource/drivers/tegra: Rework for compensation of
suspend time"). The same seems true for timer-ti-32k.c and
timer-sprd.c.

Alexandre, Daniel, any recommendations here?

     Arnd

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

[Alexandre Belloni]

On 10/12/2024 09:22:51+0100, Arnd Bergmann wrote:
> On Mon, Dec 9, 2024, at 18:17, Ciprian Marian Costea wrote:
> > On 12/6/2024 2:41 PM, Arnd Bergmann wrote:
> 
> >> I think storing 'rtc_hz' as a u32 variable and adding a range
> >> check when filling it would help, mainly to save the next reader
> >> from having to understand what is going on.
> >> 
> >
> > The confusion on my end is that I cannot see where 'div_u64() implicitly 
> > casts the dividend 'hz' from 64-bit to 32-bit' by following the method's 
> > implementation [1]
> 
> I mean passing a 64-bit variable into a function that takes a
> 32-bit argument truncates the range.
> 
> > But I agree that 'rtc_hz' can be stored into a 32-bit variable with a 
> > range check added when it is taken from the Linux clock API to avoid any
> > unneeded abstractions.
> 
> ok
> 
> >> 
> >> This is the same as just removing the error handling and
> >> relying on unsigned integer overflow semantics.
> >> 
> >> The usual check we do in time_before()/time_after instead
> >> checks if the elapsed time is less than half the available
> >> range:
> >> 
> >> #define time_after(a,b)         \
> >>          (typecheck(unsigned long, a) && \
> >>           typecheck(unsigned long, b) && \
> >>           ((long)((b) - (a)) < 0))
> >> 
> >
> > Ok. Thanks for the suggestion. I will look into using 
> > 'time_before()/time_after()' API instead of directly checking via 
> > comparison operators.
> 
> To be clear: you can't directly use time_before() here because
> that takes an 'unsigned long' argument, so you want the
> same logic, but for u32 values. I have not found an existing
> helper for that, but it's possible I missed it.
> 
> >> Who sets that alarm though? Are you relying on custom userspace
> >> for this, or is that something that the kernel already does
> >> that I'm missing?
> >
> > The test usage is via 'rtcwake' [2] userspace tool.
> > I've detailed a bit the testing scenario in the cover letter for this 
> > patchset [3]:
> >
> > "
> > 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
> 
> Got it. I feel this also needs either some documentation in
> the source code, or some infrastructure in the rtc layer if
> this is a common problem in other drivers as well. If there
> is a maximum time that the system can be suspended for without
> a wakeup, why not just set an earlier wakeup in the kernel
> when you have all the information for it?
> 
> Or maybe this should not actually be an 'rtc' driver at all?
> In the old days, we used drivers like
> arch/arm/mach-omap1/timer32k.c to register a handler
> for read_persistent_clock64(), which completely bypasses
> the RTC layer and provides both automatic wakeup and more
> accurate accounting of sleep time. 
> 
> Another example was the tegra clocksource driver, which used
> to use read_persistent_clock64() but changed to being
> a CLOCK_SOURCE_SUSPEND_NONSTOP source in 95170f0708f2
> ("clocksource/drivers/tegra: Rework for compensation of
> suspend time"). The same seems true for timer-ti-32k.c and
> timer-sprd.c.
> 
> Alexandre, Daniel, any recommendations here?
>

We have a few driver for timers that are not actual RTCs but need the
common alarm and wakeup API so they can use alarmtimers for example.
The limitation is not super common as RTCs will generally have the same
range support for alarm as what they have for time and date.

However, I'm still super confused by how complex this driver is. There
are only 3 interesting registers in this IP, one of those being read
only...

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

Re: [PATCH v6 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 linus/master v6.13-rc2 next-20241209]
[cannot apply to arm64/for-next/core]
[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/20241206-151308
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link:    https://lore.kernel.org/r/20241206070955.1503412-3-ciprianmarian.costea%40oss.nxp.com
patch subject: [PATCH v6 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20241210/202412101248.CBSpbBCZ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241210/202412101248.CBSpbBCZ-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/202412101248.CBSpbBCZ-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: "__udivdi3" [drivers/rtc/rtc-s32g.ko] undefined!
>> ERROR: modpost: "__divdi3" [drivers/rtc/rtc-s32g.ko] undefined!

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MODVERSIONS
   Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
   Selected by [y]:
   - RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=n] || GCC_PLUGINS [=y]) && MODULES [=y]
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]

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

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

[Alexandre Belloni]

On 06/12/2024 09:09:53+0200, Ciprian Costea wrote:
> +/*
> + * S32G2 and S32G3 SoCs have RTC clock source1 reserved and
> + * should not be used.
> + */
> +#define RTC_CLK_SRC1_RESERVED		BIT(1)
> +
> +enum {
> +	DIV1 = 1,
> +	DIV32 = 32,
> +	DIV512 = 512,
> +	DIV512_32 = 16384
> +};
> +
> +static const char *rtc_clk_src[RTC_CLK_MUX_SIZE] = {
> +	"source0",
> +	"source1",
> +	"source2",
> +	"source3"
> +};
> +
> +struct rtc_time_base {
> +	s64 sec;
> +	u64 cycles;
> +	struct rtc_time tm;

I don't think storing an rtc_time is necessary. I don't even think
cycles is necessary.

> +};
> +
> +struct rtc_priv {
> +	struct rtc_device *rdev;
> +	void __iomem *rtc_base;
> +	struct clk *ipg;
> +	struct clk *clk_src;
> +	const struct rtc_soc_data *rtc_data;
> +	struct rtc_time_base base;
> +	u64 rtc_hz;
> +	int irq;
> +	int clk_src_idx;
> +};
> +
> +struct rtc_soc_data {
> +	u32 clk_div;
> +	u32 reserved_clk_mask;
> +};
> +
> +static const struct rtc_soc_data rtc_s32g2_data = {
> +	.clk_div = DIV512,

If you input clock rate is higher that 16kHz, why don't you divide by
16384?

> +	.reserved_clk_mask = RTC_CLK_SRC1_RESERVED,
> +};
> +
> +static u64 cycles_to_sec(u64 hz, u64 cycles)
> +{
> +	return div_u64(cycles, hz);
> +}
> +
> +/**
> + * sec_to_rtcval - Convert a number of seconds to a value suitable for
> + * RTCVAL in our clock's
> + * current configuration.
> + * @priv: Pointer to the 'rtc_priv' structure
> + * @seconds: Number of seconds to convert
> + * @rtcval: The value to go into RTCVAL[RTCVAL]
> + *
> + * Return: 0 for success, -EINVAL if @seconds push the counter past the
> + *          32bit register range
> + */
> +static int sec_to_rtcval(const struct rtc_priv *priv,
> +			 unsigned long seconds, u32 *rtcval)
> +{
> +	u32 delta_cnt;
> +
> +	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_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;
> +	*rtcval = delta_cnt + ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t s32g_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_RTCF) {
> +		iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
> +		iowrite32(status | RTCS_RTCF, priv->rtc_base + RTCS_OFFSET);
> +		rtc_update_irq(priv->rdev, 1, RTC_AF);
> +	}
> +
> +	if (status & RTCS_APIF) {
> +		iowrite32(status | RTCS_APIF, priv->rtc_base + RTCS_OFFSET);
> +		rtc_update_irq(priv->rdev, 1, RTC_PF);

I don't think you use APIF as a periodic interrupt so it doesn't really
make sense to use RTC_PF instead of RTC_AF.

> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
> +				     u32 offset)
> +{
> +	u32 counter;
> +
> +	counter = ioread32(priv->rtc_base + offset);
> +
> +	if (counter < priv->base.cycles)
> +		return -EINVAL;
> +
> +	counter -= priv->base.cycles;
> +
> +	return priv->base.sec + cycles_to_sec(priv->rtc_hz, counter);
> +}
> +
> +static int s32g_rtc_read_time(struct device *dev,
> +			      struct rtc_time *tm)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	s64 sec;
> +
> +	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, rtccnt, rtcval;
> +	s64 sec;
> +
> +	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) {
> +		rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +		rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET);
> +
> +		if (rtccnt < rtcval)
> +			alrm->pending = 1;

This limits the range of your alarm, why don't you simply check whether
RTCF is set?

> +	}
> +
> +	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->irq)
> +		return -EIO;


This will never happen as you are not letting probe finish when you
can't request the irq.


> +
> +	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;
> +	u32 rtcval, rtcs;
> +	int ret = 0;
> +
> +	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);
> +	ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval);
> +	if (ret) {
> +		dev_warn(dev, "Alarm is set too far in the future\n");
> +		return -ERANGE;
> +	}
> +
> +	ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC),
> +				0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET);
> +	if (ret)
> +		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);
> +
> +	priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +	priv->base.sec = rtc_tm_to_time64(time);
> +

To simplify all the calculations you are doing, I suggest you reset
RTCCNT here and store the epoch of the rtc as a number of seconds.

This wll allow you to avoid having to read the counter in set_alarm
also, you then get a direct conversion for RTCVAL as this will simply be
rtc_tm_to_time64(&alrm->time) - epoch that you have to convert in cycles

You will also then know right away whether this is too large to fit in a
32bit register.


> +	return 0;
> +}
> +
> +/*
> + * Disable the 32-bit free running counter.
> + * This allows Clock Source and Divisors selection
> + * to be performed without causing synchronization issues.
> + */
> +static void s32g_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 s32g_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 rtc_clk_src_setup(struct rtc_priv *priv)
> +{
> +	u32 rtcc = 0;
> +
> +	if (priv->rtc_data->reserved_clk_mask & (1 << priv->clk_src_idx))
> +		return -EOPNOTSUPP;
> +
> +	rtcc = FIELD_PREP(RTCC_CLKSEL_MASK, priv->clk_src_idx);
> +
> +	switch (priv->rtc_data->clk_div) {
> +	case DIV512_32:
> +		rtcc |= RTCC_DIV512EN;
> +		rtcc |= RTCC_DIV32EN;
> +		break;
> +	case DIV512:
> +		rtcc |= RTCC_DIV512EN;
> +		break;
> +	case DIV32:
> +		rtcc |= RTCC_DIV32EN;
> +		break;
> +	case DIV1:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	rtcc |= RTCC_RTCIE;
> +	/*
> +	 * Make sure the CNTEN is 0 before we configure
> +	 * the clock source and dividers.
> +	 */
> +	s32g_rtc_disable(priv);
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +	s32g_rtc_enable(priv);
> +
> +	return 0;
> +}
> +
> +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 int rtc_clk_dts_setup(struct rtc_priv *priv,
> +			     struct device *dev)
> +{
> +	int i;
> +
> +	priv->ipg = devm_clk_get_enabled(dev, "ipg");
> +	if (IS_ERR(priv->ipg))
> +		return dev_err_probe(dev, PTR_ERR(priv->ipg),
> +				"Failed to get 'ipg' clock\n");
> +
> +	for (i = 0; i < RTC_CLK_MUX_SIZE; i++) {
> +		priv->clk_src = devm_clk_get_enabled(dev, rtc_clk_src[i]);
> +		if (!IS_ERR(priv->clk_src)) {
> +			priv->clk_src_idx = i;
> +			break;
> +		}
> +	}
> +
> +	if (IS_ERR(priv->clk_src))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk_src),
> +				"Failed to get rtc module clock source\n");
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rtc_priv *priv;
> +	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 PTR_ERR(priv->rtc_base);
> +
> +	device_init_wakeup(dev, true);
> +
> +	ret = rtc_clk_dts_setup(priv, dev);
> +	if (ret)
> +		return ret;
> +
> +	priv->rdev = devm_rtc_allocate_device(dev);
> +	if (IS_ERR(priv->rdev))
> +		return PTR_ERR(priv->rdev);
> +
> +	ret = rtc_clk_src_setup(priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->rtc_hz = clk_get_rate(priv->clk_src);
> +	if (!priv->rtc_hz) {
> +		dev_err(dev, "Failed to get RTC frequency\n");
> +		ret = -EINVAL;
> +		goto disable_rtc;
> +	}
> +
> +	priv->rtc_hz /= priv->rtc_data->clk_div;
> +
> +	platform_set_drvdata(pdev, priv);
> +	priv->rdev->ops = &rtc_ops;
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0) {
> +		ret = priv->irq;
> +		goto disable_rtc;
> +	}
> +
> +	ret = devm_request_irq(dev, priv->irq,
> +			       s32g_rtc_handler, 0, dev_name(dev), pdev);
> +	if (ret) {
> +		dev_err(dev, "Request interrupt %d failed, error: %d\n",
> +			priv->irq, ret);
> +		goto disable_rtc;
> +	}
> +
> +	ret = devm_rtc_register_device(priv->rdev);
> +	if (ret)
> +		goto disable_rtc;
> +
> +	return 0;
> +
> +disable_rtc:
> +	s32g_rtc_disable(priv);
> +	return ret;
> +}
> +
> +static void s32g_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 s32g_rtc_suspend(struct device *dev)
> +{
> +	struct rtc_priv *init_priv = dev_get_drvdata(dev);
> +	struct rtc_priv priv;
> +	long long base_sec;
> +	u32 rtcval, rtccnt, offset;
> +	int ret = 0;
> +	u32 sec;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Save last known timestamp */
> +	ret = s32g_rtc_read_time(dev, &init_priv->base.tm);
> +	if (ret)
> +		return ret;

I don't think that whole calculation is necessary as you are never
actually resetting RTCCNT in suspend

> +
> +	/*
> +	 * Use a local copy of the RTC control block to
> +	 * avoid restoring it on resume path.
> +	 */
> +	memcpy(&priv, init_priv, sizeof(priv));
> +
> +	rtccnt = ioread32(init_priv->rtc_base + RTCCNT_OFFSET);
> +	rtcval = ioread32(init_priv->rtc_base + RTCVAL_OFFSET);
> +	offset = rtcval - rtccnt;
> +	sec = cycles_to_sec(init_priv->rtc_hz, offset);
> +
> +	/* 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 = sec_to_rtcval(&priv, sec, &rtcval);
> +	if (ret) {
> +		dev_warn(dev, "Alarm is too far in the future\n");
> +		return -ERANGE;
> +	}
> +
> +	s32g_enable_api_irq(dev, 1);
> +	iowrite32(offset, priv.rtc_base + APIVAL_OFFSET);

What about always using APIVAL instead of RTCVAL so you don't have
anything to do in s32g_rtc_suspend.


> +
> +	return ret;
> +}
> +
> +static int s32g_rtc_resume(struct device *dev)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Disable wake-up interrupts */
> +	s32g_enable_api_irq(dev, 0);
> +
> +	ret = rtc_clk_src_setup(priv);
> +	if (ret)
> +		return ret;

I don't think this is necessary.
> +
> +	/*
> +	 * 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);

And so this is useless too so yo udon't actually have anything to do in
s32g_rtc_resume.

> +}
> +
> +static const struct of_device_id rtc_dt_ids[] = {
> +	{ .compatible = "nxp,s32g2-rtc", .data = &rtc_s32g2_data},
> +	{ /* sentinel */ },
> +};
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(s32g_rtc_pm_ops,
> +			 s32g_rtc_suspend, s32g_rtc_resume);
> +
> +static struct platform_driver s32g_rtc_driver = {
> +	.driver		= {
> +		.name			= "s32g-rtc",
> +		.pm				= pm_sleep_ptr(&s32g_rtc_pm_ops),
> +		.of_match_table = rtc_dt_ids,
> +	},
> +	.probe		= s32g_rtc_probe,
> +};
> +module_platform_driver(s32g_rtc_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
> +MODULE_LICENSE("GPL");
> -- 
> 2.45.2
> 

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

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

[Ciprian Marian Costea]

On 12/11/2024 1:25 AM, Alexandre Belloni wrote:

Hello Alexandre,

Thank you for your detailed review on this driver.
Sorry for responding to this review so late, I've just now found some 
time to continue work on this RTC driver.

> On 06/12/2024 09:09:53+0200, Ciprian Costea wrote:
>> +/*
>> + * S32G2 and S32G3 SoCs have RTC clock source1 reserved and
>> + * should not be used.
>> + */
>> +#define RTC_CLK_SRC1_RESERVED		BIT(1)
>> +
>> +enum {
>> +	DIV1 = 1,
>> +	DIV32 = 32,
>> +	DIV512 = 512,
>> +	DIV512_32 = 16384
>> +};
>> +
>> +static const char *rtc_clk_src[RTC_CLK_MUX_SIZE] = {
>> +	"source0",
>> +	"source1",
>> +	"source2",
>> +	"source3"
>> +};
>> +
>> +struct rtc_time_base {
>> +	s64 sec;
>> +	u64 cycles;
>> +	struct rtc_time tm;
>  > I don't think storing an rtc_time is necessary. I don't even think
> cycles is necessary.
> 

Indeed, switching to usage of just APIVAL (as you've suggested) instead 
of relying also on RTCVAL would make 'rtc_time' redundant. Will remove.

>> +};
>> +
>> +struct rtc_priv {
>> +	struct rtc_device *rdev;
>> +	void __iomem *rtc_base;
>> +	struct clk *ipg;
>> +	struct clk *clk_src;
>> +	const struct rtc_soc_data *rtc_data;
>> +	struct rtc_time_base base;
>> +	u64 rtc_hz;
>> +	int irq;
>> +	int clk_src_idx;
>> +};
>> +
>> +struct rtc_soc_data {
>> +	u32 clk_div;
>> +	u32 reserved_clk_mask;
>> +};
>> +
>> +static const struct rtc_soc_data rtc_s32g2_data = {
>> +	.clk_div = DIV512,
> 
> If you input clock rate is higher that 16kHz, why don't you divide by
> 16384?
> 

Yes, the default input clock rate is ~32KHz. I will enable both divisors 
to increase the RTC counter resolution.

>> +	.reserved_clk_mask = RTC_CLK_SRC1_RESERVED,
>> +};
>> +
>> +static u64 cycles_to_sec(u64 hz, u64 cycles)
>> +{
>> +	return div_u64(cycles, hz);
>> +}
>> +
>> +/**
>> + * sec_to_rtcval - Convert a number of seconds to a value suitable for
>> + * RTCVAL in our clock's
>> + * current configuration.
>> + * @priv: Pointer to the 'rtc_priv' structure
>> + * @seconds: Number of seconds to convert
>> + * @rtcval: The value to go into RTCVAL[RTCVAL]
>> + *
>> + * Return: 0 for success, -EINVAL if @seconds push the counter past the
>> + *          32bit register range
>> + */
>> +static int sec_to_rtcval(const struct rtc_priv *priv,
>> +			 unsigned long seconds, u32 *rtcval)
>> +{
>> +	u32 delta_cnt;
>> +
>> +	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_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;
>> +	*rtcval = delta_cnt + ioread32(priv->rtc_base + RTCCNT_OFFSET);
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t s32g_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_RTCF) {
>> +		iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
>> +		iowrite32(status | RTCS_RTCF, priv->rtc_base + RTCS_OFFSET);
>> +		rtc_update_irq(priv->rdev, 1, RTC_AF);
>> +	}
>> +
>> +	if (status & RTCS_APIF) {
>> +		iowrite32(status | RTCS_APIF, priv->rtc_base + RTCS_OFFSET);
>> +		rtc_update_irq(priv->rdev, 1, RTC_PF);
> 
> I don't think you use APIF as a periodic interrupt so it doesn't really
> make sense to use RTC_PF instead of RTC_AF.
> 

Correct. I will change to `rtc_update_irq(priv->rdev, 1, RTC_IRQF | 
RTC_AF);`

>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
>> +				     u32 offset)
>> +{
>> +	u32 counter;
>> +
>> +	counter = ioread32(priv->rtc_base + offset);
>> +
>> +	if (counter < priv->base.cycles)
>> +		return -EINVAL;
>> +
>> +	counter -= priv->base.cycles;
>> +
>> +	return priv->base.sec + cycles_to_sec(priv->rtc_hz, counter);
>> +}
>> +
>> +static int s32g_rtc_read_time(struct device *dev,
>> +			      struct rtc_time *tm)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +	s64 sec;
>> +
>> +	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, rtccnt, rtcval;
>> +	s64 sec;
>> +
>> +	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) {
>> +		rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
>> +		rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET);
>> +
>> +		if (rtccnt < rtcval)
>> +			alrm->pending = 1;
> 
> This limits the range of your alarm, why don't you simply check whether
> RTCF is set?
> 

Nice idea. I will refactor in V7.

>> +	}
>> +
>> +	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->irq)
>> +		return -EIO;
> 
> 
> This will never happen as you are not letting probe finish when you
> can't request the irq.
> >

Correct. I will remove this redundant check.

>> +
>> +	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;
>> +	u32 rtcval, rtcs;
>> +	int ret = 0;
>> +
>> +	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);
>> +	ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval);
>> +	if (ret) {
>> +		dev_warn(dev, "Alarm is set too far in the future\n");
>> +		return -ERANGE;
>> +	}
>> +
>> +	ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC),
>> +				0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET);
>> +	if (ret)
>> +		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);
>> +
>> +	priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET);
>> +	priv->base.sec = rtc_tm_to_time64(time);
>> +
> 
> To simplify all the calculations you are doing, I suggest you reset
> RTCCNT here and store the epoch of the rtc as a number of seconds.
> 
> This wll allow you to avoid having to read the counter in set_alarm
> also, you then get a direct conversion for RTCVAL as this will simply be
> rtc_tm_to_time64(&alrm->time) - epoch that you have to convert in cycles
> 
> You will also then know right away whether this is too large to fit in a
> 32bit register.
> 
> 

Unfortunatelly, the RTCCNT register is not writable. Hence it cannot be 
reset. The only way to reset it would be to disable & enable the RTC 
module via 'RTCC_CNTEN' which would not be acceptable in this callback 
and in the end it is something to be avoided.

Nevertheless, I believe by using just APIVAL (as you've suggested) 
instead of relying also on RTCVAL would greatly reduce the complexity of 
this driver. I will refactor in V7.

>> +	return 0;
>> +}
>> +
>> +/*
>> + * Disable the 32-bit free running counter.
>> + * This allows Clock Source and Divisors selection
>> + * to be performed without causing synchronization issues.
>> + */
>> +static void s32g_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 s32g_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 rtc_clk_src_setup(struct rtc_priv *priv)
>> +{
>> +	u32 rtcc = 0;
>> +
>> +	if (priv->rtc_data->reserved_clk_mask & (1 << priv->clk_src_idx))
>> +		return -EOPNOTSUPP;
>> +
>> +	rtcc = FIELD_PREP(RTCC_CLKSEL_MASK, priv->clk_src_idx);
>> +
>> +	switch (priv->rtc_data->clk_div) {
>> +	case DIV512_32:
>> +		rtcc |= RTCC_DIV512EN;
>> +		rtcc |= RTCC_DIV32EN;
>> +		break;
>> +	case DIV512:
>> +		rtcc |= RTCC_DIV512EN;
>> +		break;
>> +	case DIV32:
>> +		rtcc |= RTCC_DIV32EN;
>> +		break;
>> +	case DIV1:
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	rtcc |= RTCC_RTCIE;
>> +	/*
>> +	 * Make sure the CNTEN is 0 before we configure
>> +	 * the clock source and dividers.
>> +	 */
>> +	s32g_rtc_disable(priv);
>> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
>> +	s32g_rtc_enable(priv);
>> +
>> +	return 0;
>> +}
>> +
>> +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 int rtc_clk_dts_setup(struct rtc_priv *priv,
>> +			     struct device *dev)
>> +{
>> +	int i;
>> +
>> +	priv->ipg = devm_clk_get_enabled(dev, "ipg");
>> +	if (IS_ERR(priv->ipg))
>> +		return dev_err_probe(dev, PTR_ERR(priv->ipg),
>> +				"Failed to get 'ipg' clock\n");
>> +
>> +	for (i = 0; i < RTC_CLK_MUX_SIZE; i++) {
>> +		priv->clk_src = devm_clk_get_enabled(dev, rtc_clk_src[i]);
>> +		if (!IS_ERR(priv->clk_src)) {
>> +			priv->clk_src_idx = i;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (IS_ERR(priv->clk_src))
>> +		return dev_err_probe(dev, PTR_ERR(priv->clk_src),
>> +				"Failed to get rtc module clock source\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int s32g_rtc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rtc_priv *priv;
>> +	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 PTR_ERR(priv->rtc_base);
>> +
>> +	device_init_wakeup(dev, true);
>> +
>> +	ret = rtc_clk_dts_setup(priv, dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	priv->rdev = devm_rtc_allocate_device(dev);
>> +	if (IS_ERR(priv->rdev))
>> +		return PTR_ERR(priv->rdev);
>> +
>> +	ret = rtc_clk_src_setup(priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	priv->rtc_hz = clk_get_rate(priv->clk_src);
>> +	if (!priv->rtc_hz) {
>> +		dev_err(dev, "Failed to get RTC frequency\n");
>> +		ret = -EINVAL;
>> +		goto disable_rtc;
>> +	}
>> +
>> +	priv->rtc_hz /= priv->rtc_data->clk_div;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +	priv->rdev->ops = &rtc_ops;
>> +
>> +	priv->irq = platform_get_irq(pdev, 0);
>> +	if (priv->irq < 0) {
>> +		ret = priv->irq;
>> +		goto disable_rtc;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, priv->irq,
>> +			       s32g_rtc_handler, 0, dev_name(dev), pdev);
>> +	if (ret) {
>> +		dev_err(dev, "Request interrupt %d failed, error: %d\n",
>> +			priv->irq, ret);
>> +		goto disable_rtc;
>> +	}
>> +
>> +	ret = devm_rtc_register_device(priv->rdev);
>> +	if (ret)
>> +		goto disable_rtc;
>> +
>> +	return 0;
>> +
>> +disable_rtc:
>> +	s32g_rtc_disable(priv);
>> +	return ret;
>> +}
>> +
>> +static void s32g_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 s32g_rtc_suspend(struct device *dev)
>> +{
>> +	struct rtc_priv *init_priv = dev_get_drvdata(dev);
>> +	struct rtc_priv priv;
>> +	long long base_sec;
>> +	u32 rtcval, rtccnt, offset;
>> +	int ret = 0;
>> +	u32 sec;
>> +
>> +	if (!device_may_wakeup(dev))
>> +		return 0;
>> +
>> +	/* Save last known timestamp */
>> +	ret = s32g_rtc_read_time(dev, &init_priv->base.tm);
>> +	if (ret)
>> +		return ret;
> 
> I don't think that whole calculation is necessary as you are never
> actually resetting RTCCNT in suspend
> 

The RTC module needs to be reinitialized during resume, because the RTC 
registers are being reset during Standby/Suspend to RAM operations.

Nevertheless, I will greatly reduce the complexity of these calculations 
in V7.

>> +
>> +	/*
>> +	 * Use a local copy of the RTC control block to
>> +	 * avoid restoring it on resume path.
>> +	 */
>> +	memcpy(&priv, init_priv, sizeof(priv));
>> +
>> +	rtccnt = ioread32(init_priv->rtc_base + RTCCNT_OFFSET);
>> +	rtcval = ioread32(init_priv->rtc_base + RTCVAL_OFFSET);
>> +	offset = rtcval - rtccnt;
>> +	sec = cycles_to_sec(init_priv->rtc_hz, offset);
>> +
>> +	/* 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 = sec_to_rtcval(&priv, sec, &rtcval);
>> +	if (ret) {
>> +		dev_warn(dev, "Alarm is too far in the future\n");
>> +		return -ERANGE;
>> +	}
>> +
>> +	s32g_enable_api_irq(dev, 1);
>> +	iowrite32(offset, priv.rtc_base + APIVAL_OFFSET);
> 
> What about always using APIVAL instead of RTCVAL so you don't have
> anything to do in s32g_rtc_suspend.
> 

This is a great idea. I will update in V7.

> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int s32g_rtc_resume(struct device *dev)
>> +{
>> +	struct rtc_priv *priv = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	if (!device_may_wakeup(dev))
>> +		return 0;
>> +
>> +	/* Disable wake-up interrupts */
>> +	s32g_enable_api_irq(dev, 0);
>> +
>> +	ret = rtc_clk_src_setup(priv);
>> +	if (ret)
>> +		return ret;
> 
> I don't think this is necessary.

It is, because RTC registers are being reset during Standby/Suspend to 
RAM operations.

>> +
>> +	/*
>> +	 * 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);
> 
> And so this is useless too so yo udon't actually have anything to do in
> s32g_rtc_resume.
> 

I will refactor (simplify) the entire store of time logic from suspend & 
resume in V7.

Regards,
Ciprian

>> +}
>> +
>> +static const struct of_device_id rtc_dt_ids[] = {
>> +	{ .compatible = "nxp,s32g2-rtc", .data = &rtc_s32g2_data},
>> +	{ /* sentinel */ },
>> +};
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(s32g_rtc_pm_ops,
>> +			 s32g_rtc_suspend, s32g_rtc_resume);
>> +
>> +static struct platform_driver s32g_rtc_driver = {
>> +	.driver		= {
>> +		.name			= "s32g-rtc",
>> +		.pm				= pm_sleep_ptr(&s32g_rtc_pm_ops),
>> +		.of_match_table = rtc_dt_ids,
>> +	},
>> +	.probe		= s32g_rtc_probe,
>> +};
>> +module_platform_driver(s32g_rtc_driver);
>> +
>> +MODULE_AUTHOR("NXP");
>> +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.45.2
>>
> 

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

[Ciprian Costea]

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

The RTC hardware module present on S32G based SoCs tracks clock time
during system suspend and it is used as a wakeup source on S32G2/S32G3
architecture.

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 c62831e61586..d9d0ddd0d31e 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1210,6 +1210,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 v6 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 1e930c7a58b1..cf1f5b0a31f9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2833,8 +2833,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