[RESEND PATCH v2 0/3] Add support for Nuvoton ma35d1 rtc controller

[RESEND PATCH v2 0/3] Add support for Nuvoton ma35d1 rtc controller

[Jacky Huang]

From: Jacky Huang <ychuang3@nuvoton.com>

This patch series adds the rtc driver for the nuvoton ma35d1 ARMv8 SoC.
It includes DT binding documentation, the ma35d1 rtc driver, and device
tree updates.

The ma35d1 rtc controller provides real-time and calendar messaging
capabilities. It supports programmable time tick and alarm match
interrupts. The time and calendar messages are expressed in BCD format.

This rtc driver has been tested on the ma35d1 som board with Linux 6.5-rc2.

v2:
  - Updated nuvoton,ma35d1-rtc.yaml
    - Modified patch title and fixed typo
    - Added reference to rtc.yaml
    - Used unevaluatedProperties instead of additionalProperties
  - Modified rtc driver
    - Used dev_err_probe()
    - Removed ma35d1_rtc_remove()
    - Made other minor fixes

Jacky Huang (3):
  dt-bindings: rtc: Add Nuvoton ma35d1 rtc
  arm64: dts: nuvoton: Add rtc for ma35d1
  rtc: Add driver for Nuvoton ma35d1 rtc controller

 .../bindings/rtc/nuvoton,ma35d1-rtc.yaml      |  48 +++
 .../boot/dts/nuvoton/ma35d1-iot-512m.dts      |   4 +
 .../boot/dts/nuvoton/ma35d1-som-256m.dts      |   4 +
 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi       |   8 +
 drivers/rtc/Kconfig                           |  11 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-ma35d1.c                      | 355 ++++++++++++++++++
 7 files changed, 431 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
 create mode 100644 drivers/rtc/rtc-ma35d1.c

-- 
2.34.1

[RESEND PATCH v2 1/3] dt-bindings: rtc: Add Nuvoton ma35d1 rtc

[Jacky Huang]

From: Jacky Huang <ychuang3@nuvoton.com>

Add documentation describing the Nuvoton ma35d1 rtc controller.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/rtc/nuvoton,ma35d1-rtc.yaml      | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml

diff --git a/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml b/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
new file mode 100644
index 000000000000..5e4ade803eed
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/nuvoton,ma35d1-rtc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton MA35D1 Real Time Clock
+
+maintainers:
+  - Min-Jen Chen <mjchen@nuvoton.com>
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,ma35d1-rtc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+    rtc@40410000 {
+        compatible = "nuvoton,ma35d1-rtc";
+        reg = <0x40410000 0x200>;
+        interrupts = <GIC_SPI 5 IRQ_TYPE_EDGE_RISING>;
+        clocks = <&clk RTC_GATE>;
+    };
+
+...
-- 
2.34.1

[RESEND PATCH v2 2/3] arm64: dts: nuvoton: Add rtc for ma35d1

[Jacky Huang]

From: Jacky Huang <ychuang3@nuvoton.com>

Add rtc controller support to the dtsi of ma35d1 SoC and
enable rtc on SOM and IoT boards.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
 arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts | 4 ++++
 arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts | 4 ++++
 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi         | 8 ++++++++
 3 files changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
index b89e2be6abae..b3be4331abcf 100644
--- a/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
+++ b/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
@@ -54,3 +54,7 @@ &clk {
 			   "integer",
 			   "integer";
 };
+
+&rtc {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
index a1ebddecb7f8..9858788a589c 100644
--- a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
+++ b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
@@ -54,3 +54,7 @@ &clk {
 			   "integer",
 			   "integer";
 };
+
+&rtc {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
index 781cdae566a0..394395bfd3ae 100644
--- a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
+++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
@@ -95,6 +95,14 @@ clk: clock-controller@40460200 {
 			clocks = <&clk_hxt>;
 		};
 
+		rtc: rtc@40410000 {
+			compatible = "nuvoton,ma35d1-rtc";
+			reg = <0x0 0x40410000 0x0 0x200>;
+			interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clk RTC_GATE>;
+			status = "disabled";
+		};
+
 		uart0: serial@40700000 {
 			compatible = "nuvoton,ma35d1-uart";
 			reg = <0x0 0x40700000 0x0 0x100>;
-- 
2.34.1

[RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

[Jacky Huang]

From: Jacky Huang <ychuang3@nuvoton.com>

The ma35d1 rtc controller provides real-time and calendar messaging
capabilities. It supports programmable time tick and alarm match
interrupts. The time and calendar messages are expressed in BCD format.
This driver supports the built-in rtc controller of the ma35d1. It
enables setting and reading the rtc time and configuring and reading
the rtc alarm.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
 drivers/rtc/Kconfig      |  11 ++
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-ma35d1.c | 355 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 367 insertions(+)
 create mode 100644 drivers/rtc/rtc-ma35d1.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 05f4b2d66290..95ddd8f616f4 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1929,6 +1929,17 @@ config RTC_DRV_TI_K3
 	  This driver can also be built as a module, if so, the module
 	  will be called "rtc-ti-k3".
 
+config RTC_DRV_MA35D1
+	tristate "Nuvoton MA35D1 RTC"
+	depends on ARCH_MA35 || COMPILE_TEST
+	select REGMAP_MMIO
+	help
+	   If you say yes here you get support for the Nuvoton MA35D1
+	   On-Chip Real Time Clock.
+
+	   This driver can also be built as a module, if so, the module
+	   will be called "rtc-ma35d1".
+
 comment "HID Sensor RTC drivers"
 
 config RTC_DRV_HID_SENSOR_TIME
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index fd209883ee2e..763c9cb5dde1 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_RTC_DRV_M41T94)	+= rtc-m41t94.o
 obj-$(CONFIG_RTC_DRV_M48T35)	+= rtc-m48t35.o
 obj-$(CONFIG_RTC_DRV_M48T59)	+= rtc-m48t59.o
 obj-$(CONFIG_RTC_DRV_M48T86)	+= rtc-m48t86.o
+obj-$(CONFIG_RTC_DRV_MA35D1)	+= rtc-ma35d1.o
 obj-$(CONFIG_RTC_DRV_MAX6900)	+= rtc-max6900.o
 obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max6902.o
 obj-$(CONFIG_RTC_DRV_MAX6916)	+= rtc-max6916.o
diff --git a/drivers/rtc/rtc-ma35d1.c b/drivers/rtc/rtc-ma35d1.c
new file mode 100644
index 000000000000..f63b484ee37f
--- /dev/null
+++ b/drivers/rtc/rtc-ma35d1.c
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RTC driver for Nuvoton MA35D1
+ *
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ */
+
+#include <linux/bcd.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+/* MA35D1 RTC Control Registers */
+#define MA35_REG_RTC_INIT	0x00
+#define MA35_REG_RTC_SINFASTS	0x04
+#define MA35_REG_RTC_FREQADJ	0x08
+#define MA35_REG_RTC_TIME	0x0c
+#define MA35_REG_RTC_CAL	0x10
+#define MA35_REG_RTC_CLKFMT	0x14
+#define MA35_REG_RTC_WEEKDAY	0x18
+#define MA35_REG_RTC_TALM	0x1c
+#define MA35_REG_RTC_CALM	0x20
+#define MA35_REG_RTC_LEAPYEAR	0x24
+#define MA35_REG_RTC_INTEN	0x28
+#define MA35_REG_RTC_INTSTS	0x2c
+#define MA35_REG_RTC_TICK	0x30
+
+/* register MA35_REG_RTC_INIT */
+#define RTC_INIT_ACTIVE		BIT(0)
+#define RTC_INIT_MAGIC_CODE	0xa5eb1357
+
+/* register MA35_REG_RTC_CLKFMT */
+#define RTC_CLKFMT_24HEN	BIT(0)
+#define RTC_CLKFMT_DCOMPEN	BIT(16)
+
+/* register MA35_REG_RTC_INTEN */
+#define RTC_INTEN_ALMIEN	BIT(0)
+#define RTC_INTEN_TICKIEN	BIT(1)
+#define RTC_INTEN_CLKFIEN	BIT(24)
+#define RTC_INTEN_CLKSTIEN	BIT(25)
+
+/* register MA35_REG_RTC_INTSTS */
+#define RTC_INTSTS_ALMIF	BIT(0)
+#define RTC_INTSTS_TICKIF	BIT(1)
+#define RTC_INTSTS_CLKFIF	BIT(24)
+#define RTC_INTSTS_CLKSTIF	BIT(25)
+
+#define RTC_INIT_TIMEOUT	250
+
+struct ma35_rtc {
+	int irq_num;
+	void __iomem *rtc_reg;
+	struct rtc_device *rtcdev;
+};
+
+struct ma35_bcd_time {
+	int bcd_sec;
+	int bcd_min;
+	int bcd_hour;
+	int bcd_mday;
+	int bcd_mon;
+	int bcd_year;
+};
+
+static u32 rtc_reg_read(struct ma35_rtc *p, u32 offset)
+{
+	return __raw_readl(p->rtc_reg + offset);
+}
+
+static inline void rtc_reg_write(struct ma35_rtc *p, u32 offset, u32 value)
+{
+	__raw_writel(value, p->rtc_reg + offset);
+}
+
+static irqreturn_t ma35d1_rtc_interrupt(int irq, void *data)
+{
+	struct ma35_rtc *rtc = (struct ma35_rtc *)data;
+	unsigned long events = 0, rtc_irq;
+
+	rtc_irq = rtc_reg_read(rtc, MA35_REG_RTC_INTSTS);
+
+	if (rtc_irq & RTC_INTSTS_ALMIF) {
+		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_ALMIF);
+		events |= RTC_AF | RTC_IRQF;
+	}
+
+	if (rtc_irq & RTC_INTSTS_TICKIF) {
+		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_TICKIF);
+		events |= RTC_UF | RTC_IRQF;
+	}
+
+	rtc_update_irq(rtc->rtcdev, 1, events);
+
+	return IRQ_HANDLED;
+}
+
+static int ma35d1_rtc_init(struct ma35_rtc *rtc, u32 ms_timeout)
+{
+	const unsigned long timeout = jiffies + msecs_to_jiffies(ms_timeout);
+
+	do {
+		if (rtc_reg_read(rtc, MA35_REG_RTC_INIT) & RTC_INIT_ACTIVE)
+			return 0;
+
+		rtc_reg_write(rtc, MA35_REG_RTC_INIT, RTC_INIT_MAGIC_CODE);
+
+		mdelay(1);
+
+	} while (time_before(jiffies, timeout));
+
+	return -ETIMEDOUT;
+}
+
+static int ma35d1_rtc_bcd2bin(u32 time, u32 cal, u32 wday, struct rtc_time *tm)
+{
+	tm->tm_mday	= bcd2bin(cal >> 0);
+	tm->tm_mon	= bcd2bin(cal >> 8);
+	tm->tm_mon	= tm->tm_mon - 1;
+	tm->tm_year	= bcd2bin(cal >> 16) + 100;
+
+	tm->tm_sec	= bcd2bin(time >> 0);
+	tm->tm_min	= bcd2bin(time >> 8);
+	tm->tm_hour	= bcd2bin(time >> 16);
+
+	tm->tm_wday = wday;
+
+	return rtc_valid_tm(tm);
+}
+
+static int ma35d1_rtc_alarm_bcd2bin(u32 talm, u32 calm, struct rtc_time *tm)
+{
+	tm->tm_mday	= bcd2bin(calm >> 0);
+	tm->tm_mon	= bcd2bin(calm >> 8);
+	tm->tm_mon	= tm->tm_mon - 1;
+	tm->tm_year	= bcd2bin(calm >> 16) + 100;
+
+	tm->tm_sec	= bcd2bin(talm >> 0);
+	tm->tm_min	= bcd2bin(talm >> 8);
+	tm->tm_hour	= bcd2bin(talm >> 16);
+
+	return rtc_valid_tm(tm);
+}
+
+static void ma35d1_rtc_bin2bcd(struct device *dev, struct rtc_time *settm,
+			       struct ma35_bcd_time *gettm)
+{
+	gettm->bcd_mday = bin2bcd(settm->tm_mday) << 0;
+	gettm->bcd_mon  = bin2bcd((settm->tm_mon + 1)) << 8;
+
+	if (settm->tm_year < 100) {
+		dev_warn(dev, "The year will be between 1970-1999, right?\n");
+		gettm->bcd_year = bin2bcd(settm->tm_year) << 16;
+	} else {
+		gettm->bcd_year = bin2bcd(settm->tm_year - 100) << 16;
+	}
+
+	gettm->bcd_sec  = bin2bcd(settm->tm_sec) << 0;
+	gettm->bcd_min  = bin2bcd(settm->tm_min) << 8;
+	gettm->bcd_hour = bin2bcd(settm->tm_hour) << 16;
+}
+
+static int ma35d1_alarm_irq_enable(struct device *dev, u32 enabled)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	u32 reg_ien;
+
+	reg_ien = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
+
+	if (enabled)
+		rtc_reg_write(rtc, MA35_REG_RTC_INTEN, reg_ien | RTC_INTEN_ALMIEN);
+	else
+		rtc_reg_write(rtc, MA35_REG_RTC_INTEN, reg_ien & ~RTC_INTEN_ALMIEN);
+
+	return 0;
+}
+
+static int ma35d1_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	u32 time, cal, wday;
+
+	time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
+	cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
+	wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
+
+	return ma35d1_rtc_bcd2bin(time, cal, wday, tm);
+}
+
+static int ma35d1_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	struct ma35_bcd_time gettm;
+	u32 val;
+
+	ma35d1_rtc_bin2bcd(dev, tm, &gettm);
+
+	val = gettm.bcd_mday | gettm.bcd_mon | gettm.bcd_year;
+	rtc_reg_write(rtc, MA35_REG_RTC_CAL, val);
+
+	val = gettm.bcd_sec | gettm.bcd_min | gettm.bcd_hour;
+	rtc_reg_write(rtc, MA35_REG_RTC_TIME, val);
+
+	val = tm->tm_wday;
+	rtc_reg_write(rtc, MA35_REG_RTC_WEEKDAY, val);
+
+	return 0;
+}
+
+static int ma35d1_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	u32 talm, calm;
+
+	talm = rtc_reg_read(rtc, MA35_REG_RTC_TALM);
+	calm  = rtc_reg_read(rtc, MA35_REG_RTC_CALM);
+
+	return ma35d1_rtc_alarm_bcd2bin(talm, calm, &alrm->time);
+}
+
+static int ma35d1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	struct ma35_bcd_time tm;
+	unsigned long val;
+
+	ma35d1_rtc_bin2bcd(dev, &alrm->time, &tm);
+
+	val = tm.bcd_mday | tm.bcd_mon | tm.bcd_year;
+	rtc_reg_write(rtc, MA35_REG_RTC_CALM, val);
+
+	val = tm.bcd_sec | tm.bcd_min | tm.bcd_hour;
+	rtc_reg_write(rtc, MA35_REG_RTC_TALM, val);
+
+	return 0;
+}
+
+static const struct rtc_class_ops ma35d1_rtc_ops = {
+	.read_time = ma35d1_rtc_read_time,
+	.set_time = ma35d1_rtc_set_time,
+	.read_alarm = ma35d1_rtc_read_alarm,
+	.set_alarm = ma35d1_rtc_set_alarm,
+	.alarm_irq_enable = ma35d1_alarm_irq_enable,
+};
+
+static int ma35d1_rtc_probe(struct platform_device *pdev)
+{
+	struct ma35_rtc *rtc;
+	struct clk *clk;
+	u32 regval;
+	int err;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(rtc->rtc_reg))
+		return PTR_ERR(rtc->rtc_reg);
+
+	clk = of_clk_get(pdev->dev.of_node, 0);
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc clock\n");
+
+	err = clk_prepare_enable(clk);
+	if (err)
+		return -ENOENT;
+
+	platform_set_drvdata(pdev, rtc);
+
+	rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
+					       &ma35d1_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc->rtcdev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtcdev),
+				     "failed to register rtc device\n");
+
+	err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
+	if (err)
+		return err;
+
+	regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
+	regval |= RTC_CLKFMT_24HEN;
+	rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);
+
+	rtc->irq_num = platform_get_irq(pdev, 0);
+
+	err = devm_request_irq(&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt,
+			       IRQF_NO_SUSPEND, "ma35d1rtc", rtc);
+	if (err)
+		return dev_err_probe(&pdev->dev, err, "Failed to request rtc irq\n");
+
+	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
+	regval |= RTC_INTEN_TICKIEN;
+	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
+
+	device_init_wakeup(&pdev->dev, true);
+
+	return 0;
+}
+
+static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
+	u32 regval;
+
+	if (device_may_wakeup(&pdev->dev))
+		enable_irq_wake(rtc->irq_num);
+
+	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
+	regval &= ~RTC_INTEN_TICKIEN;
+	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
+
+	return 0;
+}
+
+static int ma35d1_rtc_resume(struct platform_device *pdev)
+{
+	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
+	u32 regval;
+
+	if (device_may_wakeup(&pdev->dev))
+		disable_irq_wake(rtc->irq_num);
+
+	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
+	regval |= RTC_INTEN_TICKIEN;
+	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
+
+	return 0;
+}
+
+static const struct of_device_id ma35d1_rtc_of_match[] = {
+	{ .compatible = "nuvoton,ma35d1-rtc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
+
+static struct platform_driver ma35d1_rtc_driver = {
+	.suspend    = ma35d1_rtc_suspend,
+	.resume     = ma35d1_rtc_resume,
+	.probe      = ma35d1_rtc_probe,
+	.driver		= {
+		.name	= "rtc-ma35d1",
+		.of_match_table = ma35d1_rtc_of_match,
+	},
+};
+
+module_platform_driver(ma35d1_rtc_driver);
+
+MODULE_AUTHOR("Min-Jen Chen <mjchen@nuvoton.com>");
+MODULE_DESCRIPTION("MA35D1 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

[Alexandre Belloni]

Hello,

On 09/08/2023 01:15:42+0000, Jacky Huang wrote:
> +
> +struct ma35_bcd_time {
> +	int bcd_sec;
> +	int bcd_min;
> +	int bcd_hour;
> +	int bcd_mday;
> +	int bcd_mon;
> +	int bcd_year;
> +};

I don't get why this struct is useful.

> +
> +static irqreturn_t ma35d1_rtc_interrupt(int irq, void *data)
> +{
> +	struct ma35_rtc *rtc = (struct ma35_rtc *)data;
> +	unsigned long events = 0, rtc_irq;
> +
> +	rtc_irq = rtc_reg_read(rtc, MA35_REG_RTC_INTSTS);
> +
> +	if (rtc_irq & RTC_INTSTS_ALMIF) {
> +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_ALMIF);
> +		events |= RTC_AF | RTC_IRQF;
> +	}
> +
> +	if (rtc_irq & RTC_INTSTS_TICKIF) {
> +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_TICKIF);
> +		events |= RTC_UF | RTC_IRQF;

How did you test this path?

> +	}
> +
> +	rtc_update_irq(rtc->rtcdev, 1, events);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ma35d1_rtc_init(struct ma35_rtc *rtc, u32 ms_timeout)
> +{
> +	const unsigned long timeout = jiffies + msecs_to_jiffies(ms_timeout);
> +
> +	do {
> +		if (rtc_reg_read(rtc, MA35_REG_RTC_INIT) & RTC_INIT_ACTIVE)
> +			return 0;
> +
> +		rtc_reg_write(rtc, MA35_REG_RTC_INIT, RTC_INIT_MAGIC_CODE);
> +
> +		mdelay(1);
> +
> +	} while (time_before(jiffies, timeout));
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int ma35d1_rtc_bcd2bin(u32 time, u32 cal, u32 wday, struct rtc_time *tm)
> +{
> +	tm->tm_mday	= bcd2bin(cal >> 0);
> +	tm->tm_mon	= bcd2bin(cal >> 8);
> +	tm->tm_mon	= tm->tm_mon - 1;
> +	tm->tm_year	= bcd2bin(cal >> 16) + 100;
> +
> +	tm->tm_sec	= bcd2bin(time >> 0);
> +	tm->tm_min	= bcd2bin(time >> 8);
> +	tm->tm_hour	= bcd2bin(time >> 16);
> +
> +	tm->tm_wday = wday;
> +
> +	return rtc_valid_tm(tm);
> +}
> +
> +static int ma35d1_rtc_alarm_bcd2bin(u32 talm, u32 calm, struct rtc_time *tm)
> +{
> +	tm->tm_mday	= bcd2bin(calm >> 0);
> +	tm->tm_mon	= bcd2bin(calm >> 8);
> +	tm->tm_mon	= tm->tm_mon - 1;
> +	tm->tm_year	= bcd2bin(calm >> 16) + 100;
> +
> +	tm->tm_sec	= bcd2bin(talm >> 0);
> +	tm->tm_min	= bcd2bin(talm >> 8);
> +	tm->tm_hour	= bcd2bin(talm >> 16);
> +
> +	return rtc_valid_tm(tm);
> +}
> +
> +static void ma35d1_rtc_bin2bcd(struct device *dev, struct rtc_time *settm,
> +			       struct ma35_bcd_time *gettm)
> +{
> +	gettm->bcd_mday = bin2bcd(settm->tm_mday) << 0;
> +	gettm->bcd_mon  = bin2bcd((settm->tm_mon + 1)) << 8;
> +
> +	if (settm->tm_year < 100) {
> +		dev_warn(dev, "The year will be between 1970-1999, right?\n");

No, don't do that, properly set the rtc hardware range and let the user
choose their time offset/window.

> +		gettm->bcd_year = bin2bcd(settm->tm_year) << 16;
> +	} else {
> +		gettm->bcd_year = bin2bcd(settm->tm_year - 100) << 16;
> +	}
> +
> +	gettm->bcd_sec  = bin2bcd(settm->tm_sec) << 0;
> +	gettm->bcd_min  = bin2bcd(settm->tm_min) << 8;
> +	gettm->bcd_hour = bin2bcd(settm->tm_hour) << 16;
> +}

Those functions are only used once, simply put them in their call site.

> +
> +static int ma35d1_alarm_irq_enable(struct device *dev, u32 enabled)
> +{
> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
> +	u32 reg_ien;
> +
> +	reg_ien = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> +
> +	if (enabled)
> +		rtc_reg_write(rtc, MA35_REG_RTC_INTEN, reg_ien | RTC_INTEN_ALMIEN);
> +	else
> +		rtc_reg_write(rtc, MA35_REG_RTC_INTEN, reg_ien & ~RTC_INTEN_ALMIEN);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
> +	u32 time, cal, wday;
> +
> +	time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
> +	cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
> +	wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);

Are the registers properly latched when reading? How do you ensure that
MA35_REG_RTC_TIME didn't change before reading MA35_REG_RTC_CAL ?

> +
> +	return ma35d1_rtc_bcd2bin(time, cal, wday, tm);
> +}
> +
> +static int ma35d1_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
> +	struct ma35_bcd_time gettm;
> +	u32 val;
> +
> +	ma35d1_rtc_bin2bcd(dev, tm, &gettm);
> +
> +	val = gettm.bcd_mday | gettm.bcd_mon | gettm.bcd_year;
> +	rtc_reg_write(rtc, MA35_REG_RTC_CAL, val);
> +
> +	val = gettm.bcd_sec | gettm.bcd_min | gettm.bcd_hour;
> +	rtc_reg_write(rtc, MA35_REG_RTC_TIME, val);
> +

Same question about latching, shouldn't you stop the rtc while doing
this?

> +	val = tm->tm_wday;
> +	rtc_reg_write(rtc, MA35_REG_RTC_WEEKDAY, val);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
> +	struct ma35_bcd_time tm;
> +	unsigned long val;
> +
> +	ma35d1_rtc_bin2bcd(dev, &alrm->time, &tm);
> +
> +	val = tm.bcd_mday | tm.bcd_mon | tm.bcd_year;
> +	rtc_reg_write(rtc, MA35_REG_RTC_CALM, val);
> +
> +	val = tm.bcd_sec | tm.bcd_min | tm.bcd_hour;
> +	rtc_reg_write(rtc, MA35_REG_RTC_TALM, val);
> +

What about handling alrm.enabled here?

> +	return 0;
> +}
> +
> +static const struct rtc_class_ops ma35d1_rtc_ops = {
> +	.read_time = ma35d1_rtc_read_time,
> +	.set_time = ma35d1_rtc_set_time,
> +	.read_alarm = ma35d1_rtc_read_alarm,
> +	.set_alarm = ma35d1_rtc_set_alarm,
> +	.alarm_irq_enable = ma35d1_alarm_irq_enable,
> +};
> +
> +static int ma35d1_rtc_probe(struct platform_device *pdev)
> +{
> +	struct ma35_rtc *rtc;
> +	struct clk *clk;
> +	u32 regval;
> +	int err;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rtc->rtc_reg))
> +		return PTR_ERR(rtc->rtc_reg);
> +
> +	clk = of_clk_get(pdev->dev.of_node, 0);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc clock\n");
> +
> +	err = clk_prepare_enable(clk);
> +	if (err)
> +		return -ENOENT;
> +
> +	platform_set_drvdata(pdev, rtc);
> +
> +	rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
> +					       &ma35d1_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc->rtcdev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtcdev),
> +				     "failed to register rtc device\n");

This MUST be done last in probe, else you open a race with userspace.


> +
> +	err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
> +	if (err)
> +		return err;
> +

I don't believe you should do this on every probe but only when this
hasn't been done yet.

> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
> +	regval |= RTC_CLKFMT_24HEN;
> +	rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);
> +

ditto

> +	rtc->irq_num = platform_get_irq(pdev, 0);
> +
> +	err = devm_request_irq(&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt,
> +			       IRQF_NO_SUSPEND, "ma35d1rtc", rtc);
> +	if (err)
> +		return dev_err_probe(&pdev->dev, err, "Failed to request rtc irq\n");
> +
> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> +	regval |= RTC_INTEN_TICKIEN;
> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> +
> +	device_init_wakeup(&pdev->dev, true);
> +

You must set the rtc range here.

> +	return 0;
> +}
> +
> +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
> +	u32 regval;
> +
> +	if (device_may_wakeup(&pdev->dev))
> +		enable_irq_wake(rtc->irq_num);
> +
> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> +	regval &= ~RTC_INTEN_TICKIEN;
> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);

This is not what the user is asking, don't do this. Also, how was this
tested?

> +
> +	return 0;
> +}
> +
> +static int ma35d1_rtc_resume(struct platform_device *pdev)
> +{
> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
> +	u32 regval;
> +
> +	if (device_may_wakeup(&pdev->dev))
> +		disable_irq_wake(rtc->irq_num);
> +
> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> +	regval |= RTC_INTEN_TICKIEN;
> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ma35d1_rtc_of_match[] = {
> +	{ .compatible = "nuvoton,ma35d1-rtc", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
> +
> +static struct platform_driver ma35d1_rtc_driver = {
> +	.suspend    = ma35d1_rtc_suspend,
> +	.resume     = ma35d1_rtc_resume,
> +	.probe      = ma35d1_rtc_probe,
> +	.driver		= {
> +		.name	= "rtc-ma35d1",
> +		.of_match_table = ma35d1_rtc_of_match,
> +	},
> +};
> +
> +module_platform_driver(ma35d1_rtc_driver);
> +
> +MODULE_AUTHOR("Min-Jen Chen <mjchen@nuvoton.com>");
> +MODULE_DESCRIPTION("MA35D1 RTC driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

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

Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

[Alexandre Belloni]

On 09/08/2023 04:10:27+0200, Alexandre Belloni wrote:
> > +static int ma35d1_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct ma35_rtc *rtc;
> > +	struct clk *clk;
> > +	u32 regval;
> > +	int err;
> > +
> > +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> > +	if (!rtc)
> > +		return -ENOMEM;
> > +
> > +	rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(rtc->rtc_reg))
> > +		return PTR_ERR(rtc->rtc_reg);
> > +
> > +	clk = of_clk_get(pdev->dev.of_node, 0);
> > +	if (IS_ERR(clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc clock\n");
> > +
> > +	err = clk_prepare_enable(clk);
> > +	if (err)
> > +		return -ENOENT;
> > +
> > +	platform_set_drvdata(pdev, rtc);
> > +
> > +	rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
> > +					       &ma35d1_rtc_ops, THIS_MODULE);
> > +	if (IS_ERR(rtc->rtcdev))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtcdev),
> > +				     "failed to register rtc device\n");
> 
> This MUST be done last in probe, else you open a race with userspace.
> 
> 
> > +
> > +	err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
> > +	if (err)
> > +		return err;
> > +
> 
> I don't believe you should do this on every probe but only when this
> hasn't been done yet.
> 

Also, if you can know that the time has never been set, don't discard
this information, this is crucial information and it needs to be
reported to userspace.


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

Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

[Jacky Huang]

On 2023/8/9 上午 10:10, Alexandre Belloni wrote:
> Hello,
>
> On 09/08/2023 01:15:42+0000, Jacky Huang wrote:
>> +
>> +struct ma35_bcd_time {
>> +	int bcd_sec;
>> +	int bcd_min;
>> +	int bcd_hour;
>> +	int bcd_mday;
>> +	int bcd_mon;
>> +	int bcd_year;
>> +};
> I don't get why this struct is useful.

I will remove this and modify code.


>> +
>> +static irqreturn_t ma35d1_rtc_interrupt(int irq, void *data)
>> +{
>> +	struct ma35_rtc *rtc = (struct ma35_rtc *)data;
>> +	unsigned long events = 0, rtc_irq;
>> +
>> +	rtc_irq = rtc_reg_read(rtc, MA35_REG_RTC_INTSTS);
>> +
>> +	if (rtc_irq & RTC_INTSTS_ALMIF) {
>> +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_ALMIF);
>> +		events |= RTC_AF | RTC_IRQF;
>> +	}
>> +
>> +	if (rtc_irq & RTC_INTSTS_TICKIF) {
>> +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_TICKIF);
>> +		events |= RTC_UF | RTC_IRQF;
> How did you test this path?

We use BusyBox 'date' command to observe the time changed.
In addition, we wrote a simple code to test it.
(https://github.com/OpenNuvoton/MA35D1_Linux_Applications/tree/master/examples/rtc)

>> +	}
>> +
>> +	rtc_update_irq(rtc->rtcdev, 1, events);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int ma35d1_rtc_init(struct ma35_rtc *rtc, u32 ms_timeout)
>> +{
>> +	const unsigned long timeout = jiffies + msecs_to_jiffies(ms_timeout);
>> +
>> +	do {
>> +		if (rtc_reg_read(rtc, MA35_REG_RTC_INIT) & RTC_INIT_ACTIVE)
>> +			return 0;
>> +
>> +		rtc_reg_write(rtc, MA35_REG_RTC_INIT, RTC_INIT_MAGIC_CODE);
>> +
>> +		mdelay(1);
>> +
>> +	} while (time_before(jiffies, timeout));
>> +
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +static int ma35d1_rtc_bcd2bin(u32 time, u32 cal, u32 wday, struct rtc_time *tm)
>> +{
>> +	tm->tm_mday	= bcd2bin(cal >> 0);
>> +	tm->tm_mon	= bcd2bin(cal >> 8);
>> +	tm->tm_mon	= tm->tm_mon - 1;
>> +	tm->tm_year	= bcd2bin(cal >> 16) + 100;
>> +
>> +	tm->tm_sec	= bcd2bin(time >> 0);
>> +	tm->tm_min	= bcd2bin(time >> 8);
>> +	tm->tm_hour	= bcd2bin(time >> 16);
>> +
>> +	tm->tm_wday = wday;
>> +
>> +	return rtc_valid_tm(tm);
>> +}
>> +
>> +static int ma35d1_rtc_alarm_bcd2bin(u32 talm, u32 calm, struct rtc_time *tm)
>> +{
>> +	tm->tm_mday	= bcd2bin(calm >> 0);
>> +	tm->tm_mon	= bcd2bin(calm >> 8);
>> +	tm->tm_mon	= tm->tm_mon - 1;
>> +	tm->tm_year	= bcd2bin(calm >> 16) + 100;
>> +
>> +	tm->tm_sec	= bcd2bin(talm >> 0);
>> +	tm->tm_min	= bcd2bin(talm >> 8);
>> +	tm->tm_hour	= bcd2bin(talm >> 16);
>> +
>> +	return rtc_valid_tm(tm);
>> +}
>> +
>> +static void ma35d1_rtc_bin2bcd(struct device *dev, struct rtc_time *settm,
>> +			       struct ma35_bcd_time *gettm)
>> +{
>> +	gettm->bcd_mday = bin2bcd(settm->tm_mday) << 0;
>> +	gettm->bcd_mon  = bin2bcd((settm->tm_mon + 1)) << 8;
>> +
>> +	if (settm->tm_year < 100) {
>> +		dev_warn(dev, "The year will be between 1970-1999, right?\n");
> No, don't do that, properly set the rtc hardware range and let the user
> choose their time offset/window.

We will modify the code as:

#define MA35_BASE_YEAR 2000 /* assume 20YY not 19YY */

int year;

year = tm->tm_year + 1900 – MA35_BASE_YEAR;
if (year < 0) {
     dev_err(dev, "invalid year: %d\n", year);
     return -EINVAL;
}


>> +		gettm->bcd_year = bin2bcd(settm->tm_year) << 16;
>> +	} else {
>> +		gettm->bcd_year = bin2bcd(settm->tm_year - 100) << 16;
>> +	}
>> +
>> +	gettm->bcd_sec  = bin2bcd(settm->tm_sec) << 0;
>> +	gettm->bcd_min  = bin2bcd(settm->tm_min) << 8;
>> +	gettm->bcd_hour = bin2bcd(settm->tm_hour) << 16;
>> +}
> Those functions are only used once, simply put them in their call site.

Sure, I will remove this function and put the code in their call site


>> +
>> +static int ma35d1_alarm_irq_enable(struct device *dev, u32 enabled)
>> +{
>> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
>> +	u32 reg_ien;
>> +
>> +	reg_ien = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>> +
>> +	if (enabled)
>> +		rtc_reg_write(rtc, MA35_REG_RTC_INTEN, reg_ien | RTC_INTEN_ALMIEN);
>> +	else
>> +		rtc_reg_write(rtc, MA35_REG_RTC_INTEN, reg_ien & ~RTC_INTEN_ALMIEN);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> +{
>> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
>> +	u32 time, cal, wday;
>> +
>> +	time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
>> +	cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
>> +	wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
> Are the registers properly latched when reading? How do you ensure that
> MA35_REG_RTC_TIME didn't change before reading MA35_REG_RTC_CAL ?

We will update the code as

do {
     time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
     cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
     wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
} while ((time != rtc_reg_read(rtc, MA35_REG_RTC_TIME)) ||
          (cal != rtc_reg_read(rtc, MA35_REG_RTC_CAL)) ||
          (wday != = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY)) );


>> +
>> +	return ma35d1_rtc_bcd2bin(time, cal, wday, tm);
>> +}
>> +
>> +static int ma35d1_rtc_set_time(struct device *dev, struct rtc_time *tm)
>> +{
>> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
>> +	struct ma35_bcd_time gettm;
>> +	u32 val;
>> +
>> +	ma35d1_rtc_bin2bcd(dev, tm, &gettm);
>> +
>> +	val = gettm.bcd_mday | gettm.bcd_mon | gettm.bcd_year;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_CAL, val);
>> +
>> +	val = gettm.bcd_sec | gettm.bcd_min | gettm.bcd_hour;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_TIME, val);
>> +
> Same question about latching, shouldn't you stop the rtc while doing
> this?

In fact, once enabled, the RTC hardware of MA35D1 cannot be stopped; 
this is how the hardware is designed.
Inside the MA35D1 RTC, there's an internal counter that advances by 128 
counts per second.
When the time or date register is updated, the internal counter of the 
RTC hardware will automatically reset to 0,
so there is no need to worry about memory latch issues.


>> +	val = tm->tm_wday;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_WEEKDAY, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>> +{
>> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
>> +	struct ma35_bcd_time tm;
>> +	unsigned long val;
>> +
>> +	ma35d1_rtc_bin2bcd(dev, &alrm->time, &tm);
>> +
>> +	val = tm.bcd_mday | tm.bcd_mon | tm.bcd_year;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_CALM, val);
>> +
>> +	val = tm.bcd_sec | tm.bcd_min | tm.bcd_hour;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_TALM, val);
>> +
> What about handling alrm.enabled here?

The MA35D1 RTC hardware design does not have an alarm enable/disable bit.
The decision to utilize the alarm can only be made through enabling or 
disabling the alarm interrupt.

>> +	return 0;
>> +}
>> +
>> +static const struct rtc_class_ops ma35d1_rtc_ops = {
>> +	.read_time = ma35d1_rtc_read_time,
>> +	.set_time = ma35d1_rtc_set_time,
>> +	.read_alarm = ma35d1_rtc_read_alarm,
>> +	.set_alarm = ma35d1_rtc_set_alarm,
>> +	.alarm_irq_enable = ma35d1_alarm_irq_enable,
>> +};
>> +
>> +static int ma35d1_rtc_probe(struct platform_device *pdev)
>> +{
>> +	struct ma35_rtc *rtc;
>> +	struct clk *clk;
>> +	u32 regval;
>> +	int err;
>> +
>> +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
>> +	if (!rtc)
>> +		return -ENOMEM;
>> +
>> +	rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(rtc->rtc_reg))
>> +		return PTR_ERR(rtc->rtc_reg);
>> +
>> +	clk = of_clk_get(pdev->dev.of_node, 0);
>> +	if (IS_ERR(clk))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc clock\n");
>> +
>> +	err = clk_prepare_enable(clk);
>> +	if (err)
>> +		return -ENOENT;
>> +
>> +	platform_set_drvdata(pdev, rtc);
>> +
>> +	rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
>> +					       &ma35d1_rtc_ops, THIS_MODULE);
>> +	if (IS_ERR(rtc->rtcdev))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtcdev),
>> +				     "failed to register rtc device\n");
> This MUST be done last in probe, else you open a race with userspace.
>

Yes, I will moved it to last in probe.


>> +
>> +	err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
>> +	if (err)
>> +		return err;
>> +
> I don't believe you should do this on every probe but only when this
> hasn't been done yet.
>
>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
>> +	regval |= RTC_CLKFMT_24HEN;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);
>> +
> ditto

I will modify them as:

If (!(rtc_reg_read(rtc, MA35_REG_RTC_INIT) & RTC_INIT_ACK)) {
     err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
     if (err)
         return err;
     regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
     regval |= RTC_CLKFMT_24HEN;
     rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);}
}


>> +	rtc->irq_num = platform_get_irq(pdev, 0);
>> +
>> +	err = devm_request_irq(&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt,
>> +			       IRQF_NO_SUSPEND, "ma35d1rtc", rtc);
>> +	if (err)
>> +		return dev_err_probe(&pdev->dev, err, "Failed to request rtc irq\n");
>> +
>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>> +	regval |= RTC_INTEN_TICKIEN;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>> +
>> +	device_init_wakeup(&pdev->dev, true);
>> +
> You must set the rtc range here.

I will add:
ma35d1_rtc->rtcdev->range_min = RTC_TIMESTAMP_BEGIN_2000;
ma35d1_rtc->rtcdev->range_max = RTC_TIMESTAMP_END_2099;


>> +	return 0;
>> +}
>> +
>> +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
>> +{
>> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
>> +	u32 regval;
>> +
>> +	if (device_may_wakeup(&pdev->dev))
>> +		enable_irq_wake(rtc->irq_num);
>> +
>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>> +	regval &= ~RTC_INTEN_TICKIEN;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> This is not what the user is asking, don't do this. Also, how was this
> tested?

Sure, I will remove these three lines of code.

We test it with "echo mem > /sys/power/state".

>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_rtc_resume(struct platform_device *pdev)
>> +{
>> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
>> +	u32 regval;
>> +
>> +	if (device_may_wakeup(&pdev->dev))
>> +		disable_irq_wake(rtc->irq_num);
>> +
>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>> +	regval |= RTC_INTEN_TICKIEN;
>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id ma35d1_rtc_of_match[] = {
>> +	{ .compatible = "nuvoton,ma35d1-rtc", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
>> +
>> +static struct platform_driver ma35d1_rtc_driver = {
>> +	.suspend    = ma35d1_rtc_suspend,
>> +	.resume     = ma35d1_rtc_resume,
>> +	.probe      = ma35d1_rtc_probe,
>> +	.driver		= {
>> +		.name	= "rtc-ma35d1",
>> +		.of_match_table = ma35d1_rtc_of_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(ma35d1_rtc_driver);
>> +
>> +MODULE_AUTHOR("Min-Jen Chen <mjchen@nuvoton.com>");
>> +MODULE_DESCRIPTION("MA35D1 RTC driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.34.1
>>


Best Regards,
Jacky Huang

Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

[Alexandre Belloni]

On 09/08/2023 16:12:54+0800, Jacky Huang wrote:
> 
> 
> On 2023/8/9 上午 10:10, Alexandre Belloni wrote:
> > Hello,
> > 
> > On 09/08/2023 01:15:42+0000, Jacky Huang wrote:
> > > +
> > > +struct ma35_bcd_time {
> > > +	int bcd_sec;
> > > +	int bcd_min;
> > > +	int bcd_hour;
> > > +	int bcd_mday;
> > > +	int bcd_mon;
> > > +	int bcd_year;
> > > +};
> > I don't get why this struct is useful.
> 
> I will remove this and modify code.
> 
> 
> > > +
> > > +static irqreturn_t ma35d1_rtc_interrupt(int irq, void *data)
> > > +{
> > > +	struct ma35_rtc *rtc = (struct ma35_rtc *)data;
> > > +	unsigned long events = 0, rtc_irq;
> > > +
> > > +	rtc_irq = rtc_reg_read(rtc, MA35_REG_RTC_INTSTS);
> > > +
> > > +	if (rtc_irq & RTC_INTSTS_ALMIF) {
> > > +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_ALMIF);
> > > +		events |= RTC_AF | RTC_IRQF;
> > > +	}
> > > +
> > > +	if (rtc_irq & RTC_INTSTS_TICKIF) {
> > > +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_TICKIF);
> > > +		events |= RTC_UF | RTC_IRQF;
> > How did you test this path?
> 
> We use BusyBox 'date' command to observe the time changed.
> In addition, we wrote a simple code to test it.
> (https://github.com/OpenNuvoton/MA35D1_Linux_Applications/tree/master/examples/rtc)
> 

You should probably run rtctest:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/rtc/rtctest.c


> > > +	if (settm->tm_year < 100) {
> > > +		dev_warn(dev, "The year will be between 1970-1999, right?\n");
> > No, don't do that, properly set the rtc hardware range and let the user
> > choose their time offset/window.
> 
> We will modify the code as:
> 
> #define MA35_BASE_YEAR 2000 /* assume 20YY not 19YY */
> 
> int year;
> 
> year = tm->tm_year + 1900 – MA35_BASE_YEAR;
> if (year < 0) {
>     dev_err(dev, "invalid year: %d\n", year);
>     return -EINVAL;
> }

This is not needed as the rtc core is going to check the value is in the
range once you set it.

> > > +	time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
> > > +	cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
> > > +	wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
> > Are the registers properly latched when reading? How do you ensure that
> > MA35_REG_RTC_TIME didn't change before reading MA35_REG_RTC_CAL ?
> 
> We will update the code as
> 
> do {
>     time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
>     cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
>     wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
> } while ((time != rtc_reg_read(rtc, MA35_REG_RTC_TIME)) ||
>          (cal != rtc_reg_read(rtc, MA35_REG_RTC_CAL)) ||
>          (wday != = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY)) );

Ok, so this mean your hardware doesn't do latching. I don't think it is
necessary to check MA35_REG_RTC_WEEKDAY.

> 
> 
> > > +
> > > +	return ma35d1_rtc_bcd2bin(time, cal, wday, tm);
> > > +}
> > > +
> > > +static int ma35d1_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
> > > +	struct ma35_bcd_time gettm;
> > > +	u32 val;
> > > +
> > > +	ma35d1_rtc_bin2bcd(dev, tm, &gettm);
> > > +
> > > +	val = gettm.bcd_mday | gettm.bcd_mon | gettm.bcd_year;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_CAL, val);
> > > +
> > > +	val = gettm.bcd_sec | gettm.bcd_min | gettm.bcd_hour;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_TIME, val);
> > > +
> > Same question about latching, shouldn't you stop the rtc while doing
> > this?
> 
> In fact, once enabled, the RTC hardware of MA35D1 cannot be stopped; this is
> how the hardware is designed.
> Inside the MA35D1 RTC, there's an internal counter that advances by 128
> counts per second.
> When the time or date register is updated, the internal counter of the RTC
> hardware will automatically reset to 0,
> so there is no need to worry about memory latch issues.

Ok, great

> 
> 
> > > +	val = tm->tm_wday;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_WEEKDAY, val);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ma35d1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > > +{
> > > +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
> > > +	struct ma35_bcd_time tm;
> > > +	unsigned long val;
> > > +
> > > +	ma35d1_rtc_bin2bcd(dev, &alrm->time, &tm);
> > > +
> > > +	val = tm.bcd_mday | tm.bcd_mon | tm.bcd_year;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_CALM, val);
> > > +
> > > +	val = tm.bcd_sec | tm.bcd_min | tm.bcd_hour;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_TALM, val);
> > > +
> > What about handling alrm.enabled here?
> 
> The MA35D1 RTC hardware design does not have an alarm enable/disable bit.
> The decision to utilize the alarm can only be made through enabling or
> disabling the alarm interrupt.

Exactly, you should use alrm.enabled to enable or disable the alarm
interrupt. You can simply call ma35d1_alarm_irq_enable, many drivers are
doing this.

> 
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct rtc_class_ops ma35d1_rtc_ops = {
> > > +	.read_time = ma35d1_rtc_read_time,
> > > +	.set_time = ma35d1_rtc_set_time,
> > > +	.read_alarm = ma35d1_rtc_read_alarm,
> > > +	.set_alarm = ma35d1_rtc_set_alarm,
> > > +	.alarm_irq_enable = ma35d1_alarm_irq_enable,
> > > +};
> > > +
> > > +static int ma35d1_rtc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct ma35_rtc *rtc;
> > > +	struct clk *clk;
> > > +	u32 regval;
> > > +	int err;
> > > +
> > > +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> > > +	if (!rtc)
> > > +		return -ENOMEM;
> > > +
> > > +	rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
> > > +	if (IS_ERR(rtc->rtc_reg))
> > > +		return PTR_ERR(rtc->rtc_reg);
> > > +
> > > +	clk = of_clk_get(pdev->dev.of_node, 0);
> > > +	if (IS_ERR(clk))
> > > +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc clock\n");
> > > +
> > > +	err = clk_prepare_enable(clk);
> > > +	if (err)
> > > +		return -ENOENT;
> > > +
> > > +	platform_set_drvdata(pdev, rtc);
> > > +
> > > +	rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
> > > +					       &ma35d1_rtc_ops, THIS_MODULE);
> > > +	if (IS_ERR(rtc->rtcdev))
> > > +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtcdev),
> > > +				     "failed to register rtc device\n");
> > This MUST be done last in probe, else you open a race with userspace.
> > 
> 
> Yes, I will moved it to last in probe.
> 
> 
> > > +
> > > +	err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
> > > +	if (err)
> > > +		return err;
> > > +
> > I don't believe you should do this on every probe but only when this
> > hasn't been done yet.
> > 
> > > +	regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
> > > +	regval |= RTC_CLKFMT_24HEN;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);
> > > +
> > ditto
> 
> I will modify them as:
> 
> If (!(rtc_reg_read(rtc, MA35_REG_RTC_INIT) & RTC_INIT_ACK)) {
>     err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
>     if (err)
>         return err;
>     regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
>     regval |= RTC_CLKFMT_24HEN;
>     rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);}
> }
> 
> 
> > > +	rtc->irq_num = platform_get_irq(pdev, 0);
> > > +
> > > +	err = devm_request_irq(&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt,
> > > +			       IRQF_NO_SUSPEND, "ma35d1rtc", rtc);
> > > +	if (err)
> > > +		return dev_err_probe(&pdev->dev, err, "Failed to request rtc irq\n");
> > > +
> > > +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> > > +	regval |= RTC_INTEN_TICKIEN;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> > > +
> > > +	device_init_wakeup(&pdev->dev, true);
> > > +
> > You must set the rtc range here.
> 
> I will add:
> ma35d1_rtc->rtcdev->range_min = RTC_TIMESTAMP_BEGIN_2000;
> ma35d1_rtc->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
> 

Perfect. Maybe you should run rtc-range to check for proper operation in
the range:
https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c

> 
> > > +	return 0;
> > > +}
> > > +
> > > +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> > > +{
> > > +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
> > > +	u32 regval;
> > > +
> > > +	if (device_may_wakeup(&pdev->dev))
> > > +		enable_irq_wake(rtc->irq_num);
> > > +
> > > +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> > > +	regval &= ~RTC_INTEN_TICKIEN;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> > This is not what the user is asking, don't do this. Also, how was this
> > tested?
> 
> Sure, I will remove these three lines of code.
> 
> We test it with "echo mem > /sys/power/state".
> 

Yes, my point is that if UIE is enabled, then the user wants to be woken
up every second. If this is not what is wanted, then UIE has to be
disabled before going to suspend.

My question is why are you enabling RTC_INTEN_TICKIEN in probe? I don't
expect anyone to use an actual hardware tick interrupt, unless the alarm
is broken and can't be set every second. This is why I questioned the
RTC_UF path because I don't expect it to be taken at all.

> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ma35d1_rtc_resume(struct platform_device *pdev)
> > > +{
> > > +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
> > > +	u32 regval;
> > > +
> > > +	if (device_may_wakeup(&pdev->dev))
> > > +		disable_irq_wake(rtc->irq_num);
> > > +
> > > +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> > > +	regval |= RTC_INTEN_TICKIEN;
> > > +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id ma35d1_rtc_of_match[] = {
> > > +	{ .compatible = "nuvoton,ma35d1-rtc", },
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
> > > +
> > > +static struct platform_driver ma35d1_rtc_driver = {
> > > +	.suspend    = ma35d1_rtc_suspend,
> > > +	.resume     = ma35d1_rtc_resume,
> > > +	.probe      = ma35d1_rtc_probe,
> > > +	.driver		= {
> > > +		.name	= "rtc-ma35d1",
> > > +		.of_match_table = ma35d1_rtc_of_match,
> > > +	},
> > > +};
> > > +
> > > +module_platform_driver(ma35d1_rtc_driver);
> > > +
> > > +MODULE_AUTHOR("Min-Jen Chen <mjchen@nuvoton.com>");
> > > +MODULE_DESCRIPTION("MA35D1 RTC driver");
> > > +MODULE_LICENSE("GPL");
> > > -- 
> > > 2.34.1
> > > 
> 
> 
> Best Regards,
> Jacky Huang
> 

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

Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

[Jacky Huang]

On 2023/8/10 上午 06:51, Alexandre Belloni wrote:
> On 09/08/2023 16:12:54+0800, Jacky Huang wrote:
>>
>> On 2023/8/9 上午 10:10, Alexandre Belloni wrote:
>>> Hello,
>>>
>>> On 09/08/2023 01:15:42+0000, Jacky Huang wrote:
>>>> +
>>>> +struct ma35_bcd_time {
>>>> +	int bcd_sec;
>>>> +	int bcd_min;
>>>> +	int bcd_hour;
>>>> +	int bcd_mday;
>>>> +	int bcd_mon;
>>>> +	int bcd_year;
>>>> +};
>>> I don't get why this struct is useful.
>> I will remove this and modify code.
>>
>>
>>>> +
>>>> +static irqreturn_t ma35d1_rtc_interrupt(int irq, void *data)
>>>> +{
>>>> +	struct ma35_rtc *rtc = (struct ma35_rtc *)data;
>>>> +	unsigned long events = 0, rtc_irq;
>>>> +
>>>> +	rtc_irq = rtc_reg_read(rtc, MA35_REG_RTC_INTSTS);
>>>> +
>>>> +	if (rtc_irq & RTC_INTSTS_ALMIF) {
>>>> +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_ALMIF);
>>>> +		events |= RTC_AF | RTC_IRQF;
>>>> +	}
>>>> +
>>>> +	if (rtc_irq & RTC_INTSTS_TICKIF) {
>>>> +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_TICKIF);
>>>> +		events |= RTC_UF | RTC_IRQF;
>>> How did you test this path?
>> We use BusyBox 'date' command to observe the time changed.
>> In addition, we wrote a simple code to test it.
>> (https://github.com/OpenNuvoton/MA35D1_Linux_Applications/tree/master/examples/rtc)
>>
> You should probably run rtctest:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/rtc/rtctest.c

Thank you for providing this information.
We will run this test before submitting the next version.

>
>>>> +	if (settm->tm_year < 100) {
>>>> +		dev_warn(dev, "The year will be between 1970-1999, right?\n");
>>> No, don't do that, properly set the rtc hardware range and let the user
>>> choose their time offset/window.
>> We will modify the code as:
>>
>> #define MA35_BASE_YEAR 2000 /* assume 20YY not 19YY */
>>
>> int year;
>>
>> year = tm->tm_year + 1900 – MA35_BASE_YEAR;
>> if (year < 0) {
>>      dev_err(dev, "invalid year: %d\n", year);
>>      return -EINVAL;
>> }
> This is not needed as the rtc core is going to check the value is in the
> range once you set it.

OK, I will drop the "if (year < 0)" check.

>>>> +	time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
>>>> +	cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
>>>> +	wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
>>> Are the registers properly latched when reading? How do you ensure that
>>> MA35_REG_RTC_TIME didn't change before reading MA35_REG_RTC_CAL ?
>> We will update the code as
>>
>> do {
>>      time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
>>      cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
>>      wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
>> } while ((time != rtc_reg_read(rtc, MA35_REG_RTC_TIME)) ||
>>           (cal != rtc_reg_read(rtc, MA35_REG_RTC_CAL)) ||
>>           (wday != = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY)) );
> Ok, so this mean your hardware doesn't do latching. I don't think it is
> necessary to check MA35_REG_RTC_WEEKDAY.

OK, I will remove the MA35_REG_RTC_WEEKDAY check.
>>
>>>> +
>>>> +	return ma35d1_rtc_bcd2bin(time, cal, wday, tm);
>>>> +}
>>>> +
>>>> +static int ma35d1_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>>> +{
>>>> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
>>>> +	struct ma35_bcd_time gettm;
>>>> +	u32 val;
>>>> +
>>>> +	ma35d1_rtc_bin2bcd(dev, tm, &gettm);
>>>> +
>>>> +	val = gettm.bcd_mday | gettm.bcd_mon | gettm.bcd_year;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_CAL, val);
>>>> +
>>>> +	val = gettm.bcd_sec | gettm.bcd_min | gettm.bcd_hour;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_TIME, val);
>>>> +
>>> Same question about latching, shouldn't you stop the rtc while doing
>>> this?
>> In fact, once enabled, the RTC hardware of MA35D1 cannot be stopped; this is
>> how the hardware is designed.
>> Inside the MA35D1 RTC, there's an internal counter that advances by 128
>> counts per second.
>> When the time or date register is updated, the internal counter of the RTC
>> hardware will automatically reset to 0,
>> so there is no need to worry about memory latch issues.
> Ok, great
>
>>
>>>> +	val = tm->tm_wday;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_WEEKDAY, val);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ma35d1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>>>> +{
>>>> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
>>>> +	struct ma35_bcd_time tm;
>>>> +	unsigned long val;
>>>> +
>>>> +	ma35d1_rtc_bin2bcd(dev, &alrm->time, &tm);
>>>> +
>>>> +	val = tm.bcd_mday | tm.bcd_mon | tm.bcd_year;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_CALM, val);
>>>> +
>>>> +	val = tm.bcd_sec | tm.bcd_min | tm.bcd_hour;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_TALM, val);
>>>> +
>>> What about handling alrm.enabled here?
>> The MA35D1 RTC hardware design does not have an alarm enable/disable bit.
>> The decision to utilize the alarm can only be made through enabling or
>> disabling the alarm interrupt.
> Exactly, you should use alrm.enabled to enable or disable the alarm
> interrupt. You can simply call ma35d1_alarm_irq_enable, many drivers are
> doing this.

OK, we will use irq enable/disable to support alrm.enabled.

>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct rtc_class_ops ma35d1_rtc_ops = {
>>>> +	.read_time = ma35d1_rtc_read_time,
>>>> +	.set_time = ma35d1_rtc_set_time,
>>>> +	.read_alarm = ma35d1_rtc_read_alarm,
>>>> +	.set_alarm = ma35d1_rtc_set_alarm,
>>>> +	.alarm_irq_enable = ma35d1_alarm_irq_enable,
>>>> +};
>>>> +
>>>> +static int ma35d1_rtc_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct ma35_rtc *rtc;
>>>> +	struct clk *clk;
>>>> +	u32 regval;
>>>> +	int err;
>>>> +
>>>> +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
>>>> +	if (!rtc)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
>>>> +	if (IS_ERR(rtc->rtc_reg))
>>>> +		return PTR_ERR(rtc->rtc_reg);
>>>> +
>>>> +	clk = of_clk_get(pdev->dev.of_node, 0);
>>>> +	if (IS_ERR(clk))
>>>> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc clock\n");
>>>> +
>>>> +	err = clk_prepare_enable(clk);
>>>> +	if (err)
>>>> +		return -ENOENT;
>>>> +
>>>> +	platform_set_drvdata(pdev, rtc);
>>>> +
>>>> +	rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
>>>> +					       &ma35d1_rtc_ops, THIS_MODULE);
>>>> +	if (IS_ERR(rtc->rtcdev))
>>>> +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtcdev),
>>>> +				     "failed to register rtc device\n");
>>> This MUST be done last in probe, else you open a race with userspace.
>>>
>> Yes, I will moved it to last in probe.
>>
>>
>>>> +
>>>> +	err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
>>>> +	if (err)
>>>> +		return err;
>>>> +
>>> I don't believe you should do this on every probe but only when this
>>> hasn't been done yet.
>>>
>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
>>>> +	regval |= RTC_CLKFMT_24HEN;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);
>>>> +
>>> ditto
>> I will modify them as:
>>
>> If (!(rtc_reg_read(rtc, MA35_REG_RTC_INIT) & RTC_INIT_ACK)) {
>>      err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
>>      if (err)
>>          return err;
>>      regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
>>      regval |= RTC_CLKFMT_24HEN;
>>      rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);}
>> }
>>
>>
>>>> +	rtc->irq_num = platform_get_irq(pdev, 0);
>>>> +
>>>> +	err = devm_request_irq(&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt,
>>>> +			       IRQF_NO_SUSPEND, "ma35d1rtc", rtc);
>>>> +	if (err)
>>>> +		return dev_err_probe(&pdev->dev, err, "Failed to request rtc irq\n");
>>>> +
>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>>>> +	regval |= RTC_INTEN_TICKIEN;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>>>> +
>>>> +	device_init_wakeup(&pdev->dev, true);
>>>> +
>>> You must set the rtc range here.
>> I will add:
>> ma35d1_rtc->rtcdev->range_min = RTC_TIMESTAMP_BEGIN_2000;
>> ma35d1_rtc->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
>>
> Perfect. Maybe you should run rtc-range to check for proper operation in
> the range:
> https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c

We will run this test.
Thanks for providing this information.

>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
>>>> +{
>>>> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
>>>> +	u32 regval;
>>>> +
>>>> +	if (device_may_wakeup(&pdev->dev))
>>>> +		enable_irq_wake(rtc->irq_num);
>>>> +
>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>>>> +	regval &= ~RTC_INTEN_TICKIEN;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>>> This is not what the user is asking, don't do this. Also, how was this
>>> tested?
>> Sure, I will remove these three lines of code.
>>
>> We test it with "echo mem > /sys/power/state".
>>
> Yes, my point is that if UIE is enabled, then the user wants to be woken
> up every second. If this is not what is wanted, then UIE has to be
> disabled before going to suspend.
>
> My question is why are you enabling RTC_INTEN_TICKIEN in probe? I don't
> expect anyone to use an actual hardware tick interrupt, unless the alarm
> is broken and can't be set every second. This is why I questioned the
> RTC_UF path because I don't expect it to be taken at all.

Yes, we will remove TICKIEN from probe and modify ma35d1_alarm_irq_enable().
TICKIEN will be enabled only if UIE is enabled.

static int ma35d1_alarm_irq_enable(struct device *dev, unsigned int enabled)
{
     struct ma35d1_rtc *rtc = dev_get_drvdata(dev);

     if (enabled) {
         if (rtc->rtc->uie_rtctimer.enabled)
             rtc_reg_write(rtc, NVT_RTC_INTEN,
                       (rtc_reg_read(rtc, 
NVT_RTC_INTEN)|(RTC_INTEN_TICKIEN)));
         if (rtc->rtc->aie_timer.enabled)
             rtc_reg_write(rtc, NVT_RTC_INTEN,
                       (rtc_reg_read(rtc, 
NVT_RTC_INTEN)|(RTC_INTEN_ALMIEN)));
     } else {
         if (rtc->rtc->uie_rtctimer.enabled)
             rtc_reg_write(rtc, NVT_RTC_INTEN,
                       (rtc_reg_read(rtc, NVT_RTC_INTEN) & 
(~RTC_INTEN_TICKIEN)));
         if (rtc->rtc->aie_timer.enabled)
             rtc_reg_write(rtc, NVT_RTC_INTEN,
                       (rtc_reg_read(rtc, NVT_RTC_INTEN) & 
(~RTC_INTEN_ALMIEN)));
     }
     return 0;
}



>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ma35d1_rtc_resume(struct platform_device *pdev)
>>>> +{
>>>> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
>>>> +	u32 regval;
>>>> +
>>>> +	if (device_may_wakeup(&pdev->dev))
>>>> +		disable_irq_wake(rtc->irq_num);
>>>> +
>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>>>> +	regval |= RTC_INTEN_TICKIEN;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id ma35d1_rtc_of_match[] = {
>>>> +	{ .compatible = "nuvoton,ma35d1-rtc", },
>>>> +	{},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
>>>> +
>>>> +static struct platform_driver ma35d1_rtc_driver = {
>>>> +	.suspend    = ma35d1_rtc_suspend,
>>>> +	.resume     = ma35d1_rtc_resume,
>>>> +	.probe      = ma35d1_rtc_probe,
>>>> +	.driver		= {
>>>> +		.name	= "rtc-ma35d1",
>>>> +		.of_match_table = ma35d1_rtc_of_match,
>>>> +	},
>>>> +};
>>>> +
>>>> +module_platform_driver(ma35d1_rtc_driver);
>>>> +
>>>> +MODULE_AUTHOR("Min-Jen Chen <mjchen@nuvoton.com>");
>>>> +MODULE_DESCRIPTION("MA35D1 RTC driver");
>>>> +MODULE_LICENSE("GPL");
>>>> -- 
>>>> 2.34.1
>>>>
>>
>> Best Regards,
>> Jacky Huang
>>

Best Regards,
Jacky Huang

Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

[Alexandre Belloni]

On 10/08/2023 15:21:47+0800, Jacky Huang wrote:
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> > > > > +{
> > > > > +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
> > > > > +	u32 regval;
> > > > > +
> > > > > +	if (device_may_wakeup(&pdev->dev))
> > > > > +		enable_irq_wake(rtc->irq_num);
> > > > > +
> > > > > +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> > > > > +	regval &= ~RTC_INTEN_TICKIEN;
> > > > > +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> > > > This is not what the user is asking, don't do this. Also, how was this
> > > > tested?
> > > Sure, I will remove these three lines of code.
> > > 
> > > We test it with "echo mem > /sys/power/state".
> > > 
> > Yes, my point is that if UIE is enabled, then the user wants to be woken
> > up every second. If this is not what is wanted, then UIE has to be
> > disabled before going to suspend.
> > 
> > My question is why are you enabling RTC_INTEN_TICKIEN in probe? I don't
> > expect anyone to use an actual hardware tick interrupt, unless the alarm
> > is broken and can't be set every second. This is why I questioned the
> > RTC_UF path because I don't expect it to be taken at all.
> 
> Yes, we will remove TICKIEN from probe and modify ma35d1_alarm_irq_enable().
> TICKIEN will be enabled only if UIE is enabled.
> 
> static int ma35d1_alarm_irq_enable(struct device *dev, unsigned int enabled)
> {
>     struct ma35d1_rtc *rtc = dev_get_drvdata(dev);
> 
>     if (enabled) {
>         if (rtc->rtc->uie_rtctimer.enabled)
>             rtc_reg_write(rtc, NVT_RTC_INTEN,
>                       (rtc_reg_read(rtc,
> NVT_RTC_INTEN)|(RTC_INTEN_TICKIEN)));


Don't do that unless the regular alarm can't be set every second. Simply
always use ALMIEN, then check rtctest is passing properly.

>         if (rtc->rtc->aie_timer.enabled)
>             rtc_reg_write(rtc, NVT_RTC_INTEN,
>                       (rtc_reg_read(rtc,
> NVT_RTC_INTEN)|(RTC_INTEN_ALMIEN)));
>     } else {
>         if (rtc->rtc->uie_rtctimer.enabled)
>             rtc_reg_write(rtc, NVT_RTC_INTEN,
>                       (rtc_reg_read(rtc, NVT_RTC_INTEN) &
> (~RTC_INTEN_TICKIEN)));
>         if (rtc->rtc->aie_timer.enabled)
>             rtc_reg_write(rtc, NVT_RTC_INTEN,
>                       (rtc_reg_read(rtc, NVT_RTC_INTEN) &
> (~RTC_INTEN_ALMIEN)));
>     }
>     return 0;
> }
> 

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

Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

[Jacky Huang]

On 2023/8/10 下午 03:30, Alexandre Belloni wrote:
> On 10/08/2023 15:21:47+0800, Jacky Huang wrote:
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
>>>>>> +{
>>>>>> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
>>>>>> +	u32 regval;
>>>>>> +
>>>>>> +	if (device_may_wakeup(&pdev->dev))
>>>>>> +		enable_irq_wake(rtc->irq_num);
>>>>>> +
>>>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>>>>>> +	regval &= ~RTC_INTEN_TICKIEN;
>>>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>>>>> This is not what the user is asking, don't do this. Also, how was this
>>>>> tested?
>>>> Sure, I will remove these three lines of code.
>>>>
>>>> We test it with "echo mem > /sys/power/state".
>>>>
>>> Yes, my point is that if UIE is enabled, then the user wants to be woken
>>> up every second. If this is not what is wanted, then UIE has to be
>>> disabled before going to suspend.
>>>
>>> My question is why are you enabling RTC_INTEN_TICKIEN in probe? I don't
>>> expect anyone to use an actual hardware tick interrupt, unless the alarm
>>> is broken and can't be set every second. This is why I questioned the
>>> RTC_UF path because I don't expect it to be taken at all.
>> Yes, we will remove TICKIEN from probe and modify ma35d1_alarm_irq_enable().
>> TICKIEN will be enabled only if UIE is enabled.
>>
>> static int ma35d1_alarm_irq_enable(struct device *dev, unsigned int enabled)
>> {
>>      struct ma35d1_rtc *rtc = dev_get_drvdata(dev);
>>
>>      if (enabled) {
>>          if (rtc->rtc->uie_rtctimer.enabled)
>>              rtc_reg_write(rtc, NVT_RTC_INTEN,
>>                        (rtc_reg_read(rtc,
>> NVT_RTC_INTEN)|(RTC_INTEN_TICKIEN)));
>
> Don't do that unless the regular alarm can't be set every second. Simply
> always use ALMIEN, then check rtctest is passing properly.

OK, I got it. I will drop the TICKINT and use ALMIEN only.

MA35D1 RTC has an alarm mask register which supports alarm mask for 
seconds, minutes, and hours.
We will use the alarm mask to have RTC generate an alarm interrupt per 
second, and make sure
the driver can pass rtctest.

>>          if (rtc->rtc->aie_timer.enabled)
>>              rtc_reg_write(rtc, NVT_RTC_INTEN,
>>                        (rtc_reg_read(rtc,
>> NVT_RTC_INTEN)|(RTC_INTEN_ALMIEN)));
>>      } else {
>>          if (rtc->rtc->uie_rtctimer.enabled)
>>              rtc_reg_write(rtc, NVT_RTC_INTEN,
>>                        (rtc_reg_read(rtc, NVT_RTC_INTEN) &
>> (~RTC_INTEN_TICKIEN)));
>>          if (rtc->rtc->aie_timer.enabled)
>>              rtc_reg_write(rtc, NVT_RTC_INTEN,
>>                        (rtc_reg_read(rtc, NVT_RTC_INTEN) &
>> (~RTC_INTEN_ALMIEN)));
>>      }
>>      return 0;
>> }
>>


Best Regards,
Jacky Huang

Re: [RESEND PATCH v2 0/3] Add support for Nuvoton ma35d1 rtc controller

[Arnd Bergmann]

On Wed, Aug 9, 2023, at 03:15, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
>
> This patch series adds the rtc driver for the nuvoton ma35d1 ARMv8 SoC.
> It includes DT binding documentation, the ma35d1 rtc driver, and device
> tree updates.
>
> The ma35d1 rtc controller provides real-time and calendar messaging
> capabilities. It supports programmable time tick and alarm match
> interrupts. The time and calendar messages are expressed in BCD format.
>
> This rtc driver has been tested on the ma35d1 som board with Linux 6.5-rc2.

Hi Jacky,

I see you added soc@kernel.org to Cc for all three patches, which
has put them into the patchwork tracker.

Now that the platoform support is merged, I do not pick up patches
for other subsystems through the soc tree, so please drop the Cc
here.

You can post the the dts change along with the driver, but the
correct process is that the subsystem maintainer(s) pick up the
DT binding and the driver once the review is complete, and then
you send the dts changes to soc@kernel.org. Depending on the
platform, there may be a lot of conflicting dts changes, so this
way you can aggregate any patches for these files before sending
them to the soc tree for inclusion, while I then merge them
with all the dts changes for other platforms and any global
cleanup. 

    Arnd

Re: [RESEND PATCH v2 0/3] Add support for Nuvoton ma35d1 rtc controller

[Jacky Huang]

On 2023/8/12 下午 04:53, Arnd Bergmann wrote:
> On Wed, Aug 9, 2023, at 03:15, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@nuvoton.com>
>>
>> This patch series adds the rtc driver for the nuvoton ma35d1 ARMv8 SoC.
>> It includes DT binding documentation, the ma35d1 rtc driver, and device
>> tree updates.
>>
>> The ma35d1 rtc controller provides real-time and calendar messaging
>> capabilities. It supports programmable time tick and alarm match
>> interrupts. The time and calendar messages are expressed in BCD format.
>>
>> This rtc driver has been tested on the ma35d1 som board with Linux 6.5-rc2.
> Hi Jacky,
>
> I see you added soc@kernel.org to Cc for all three patches, which
> has put them into the patchwork tracker.
>
> Now that the platoform support is merged, I do not pick up patches
> for other subsystems through the soc tree, so please drop the Cc
> here.
>
> You can post the the dts change along with the driver, but the
> correct process is that the subsystem maintainer(s) pick up the
> DT binding and the driver once the review is complete, and then
> you send the dts changes to soc@kernel.org. Depending on the
> platform, there may be a lot of conflicting dts changes, so this
> way you can aggregate any patches for these files before sending
> them to the soc tree for inclusion, while I then merge them
> with all the dts changes for other platforms and any global
> cleanup.
>
>      Arnd


Dear Arnd,


Thank you for the detailed explanation. I now understand how to proceed.

I will remove Cc: soc@kernel.org from here. I apologize for any 
inconvenience

this patch series may have caused you and appreciate your assistance.


Best Regards,

Jacky Huang