Hello,

-----Original Message-----
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
Sent: Wednesday, November 3, 2021 8:36 AM
To: Vincent Shih <vincent.sunplus@gmail.com>
Cc: a.zummo@towertech.it; p.zabel@pengutronix.de;
linux-kernel@vger.kernel.org; linux-rtc@vger.kernel.org; robh+dt@kernel.org;
devicetree@vger.kernel.org; Vincent Shih 施錕鴻
[off-list ref]
Subject: Re: [PATCH 1/2] rtc: Add driver for Sunplus SP7021

Hello,

On 02/11/2021 14:22:02+0800, Vincent Shih wrote:

quoted

+/**************************************************************
************************************/

quoted

+/* How to test RTC:										  */
+/*												  */
+/* 1. use kernel commands									  */
+/* hwclock - query and set the hardware clock (RTC)

	  */

quoted

+/*												  */
+/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date &&

echo -n 'hwclock -r: ' */

quoted

+/*								&& hwclock -r; sleep 1); done)

*/

quoted

+/* date 121209002014 # Set system to 2014/Dec/12 09:00

		  */

quoted

+/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date &&

echo -n 'hwclock -r: ' */

quoted

+/*								&& hwclock -r; sleep 1); done)

*/

quoted

+/* hwclock -s # Set the System Time from the Hardware Clock

		  */

quoted

+/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date &&

echo -n 'hwclock -r: ' */

quoted

+/*								&& hwclock -r; sleep 1); done)

*/

quoted

+/* date 121213002014 # Set system to 2014/Dec/12 13:00

		  */

quoted

+/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date &&

echo -n 'hwclock -r: ' */

quoted

+/*								&& hwclock -r; sleep 1); done)

*/

quoted

+/* hwclock -w # Set the Hardware Clock to the current System Time

		  */

quoted

+/* (for i in `seq 10000`; do (echo ------ && echo -n 'date  : ' && date &&

echo -n 'hwclock -r: ' */

quoted

+/*								&& hwclock -r; sleep 1); done)

*/

quoted

+/*												  */
+/* How to setup alarm (e.g., 10 sec later):

*/

quoted

+/*     echo 0 > /sys/class/rtc/rtc0/wakealarm && nnn=`date '+%s'` &&

echo $nnn && \		  */

quoted

+/*     nnn=`expr $nnn + 10` && echo $nnn >

/sys/class/rtc/rtc0/wakealarm			  */

quoted

+/*												  */
+/* 2. use RTC Driver Test Program

(\linux\application\module_test\rtc\rtc-test.c)		  */

quoted

+/*												  */

+/**************************************************************
******

quoted

+******************************/

I don't feel this is a super useful comment, especially since it is buried in a
driver and this basically says to use rtctest.c

I will remove them.

quoted

+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/rtc.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/of.h>
+#include <linux/ktime.h>
+#include <linux/io.h>

This list has to be sorted

I will modify it.

quoted

+
+/*
+---------------------------------------------------------------------
+------------------------- */ #define FUNC_DEBUG() pr_debug("[RTC]
+Debug: %s(%d)\n", __func__, __LINE__)
+
+#define RTC_DEBUG(fmt, args ...) pr_debug("[RTC] Debug: " fmt, ##
+args) #define RTC_INFO(fmt, args ...) pr_info("[RTC] Info: " fmt, ##
+args) #define RTC_WARN(fmt, args ...) pr_warn("[RTC] Warning: " fmt,
+## args) #define RTC_ERR(fmt, args ...) pr_err("[RTC] Error: " fmt,
+## args)
+/*
+---------------------------------------------------------------------
+------------------------- */
+
+struct sunplus_rtc {
+	struct clk *rtcclk;
+	struct reset_control *rstc;
+	unsigned long set_alarm_again;
+	u32 charging_mode;
+};
+
+struct sunplus_rtc sp_rtc;
+
+#define RTC_REG_NAME		"rtc_reg"
+
+struct sp_rtc_reg {
+	unsigned int rsv00;
+	unsigned int rsv01;
+	unsigned int rsv02;
+	unsigned int rsv03;
+	unsigned int rsv04;
+	unsigned int rsv05;
+	unsigned int rsv06;
+	unsigned int rsv07;
+	unsigned int rsv08;
+	unsigned int rsv09;
+	unsigned int rsv10;
+	unsigned int rsv11;
+	unsigned int rsv12;
+	unsigned int rsv13;
+	unsigned int rsv14;
+	unsigned int rsv15;
+	unsigned int rtc_ctrl;
+	unsigned int rtc_timer_out;
+	unsigned int rtc_divider;
+	unsigned int rtc_timer_set;
+	unsigned int rtc_alarm_set;
+	unsigned int rtc_user_data;
+	unsigned int rtc_reset_record;
+	unsigned int rtc_battery_ctrl;
+	unsigned int rtc_trim_ctrl;
+	unsigned int rsv25;
+	unsigned int rsv26;
+	unsigned int rsv27;
+	unsigned int rsv28;
+	unsigned int rsv29;
+	unsigned int rsv30;
+	unsigned int rsv31;
+};

We don't use that kind of structs, please use defines and offsets.

I will modify it.

quoted

+
+static struct sp_rtc_reg *rtc_reg_ptr;
+
+static void sp_get_seconds(unsigned long *secs) {
+	*secs = (unsigned long)readl(&rtc_reg_ptr->rtc_timer_out);
+}
+
+static void sp_set_seconds(unsigned long secs) {
+	writel((u32)secs, &rtc_reg_ptr->rtc_timer_set); }
+
+static int sp_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	unsigned long secs;
+
+	sp_get_seconds(&secs);
+	rtc_time64_to_tm(secs, tm);
+	RTC_DEBUG("%s:  RTC date/time to %d-%d-%d, %02d:%02d:%02d.\r\n",
+		__func__, tm->tm_mday, tm->tm_mon + 1, tm->tm_year,
+					tm->tm_hour, tm->tm_min, tm->tm_sec);
+
+	return rtc_valid_tm(tm);
+}
+
+int sp_rtc_get_time(struct rtc_time *tm) {
+	unsigned long secs;
+
+	sp_get_seconds(&secs);
+	rtc_time64_to_tm(secs, tm);
+	return 0;
+}
+EXPORT_SYMBOL(sp_rtc_get_time);
+

Why is this exported?

It is useless. I will remove it. 

quoted

+static int sp_rtc_suspend(struct platform_device *pdev, pm_message_t
+state) {
+	FUNC_DEBUG();
+
+	// Keep RTC from system reset
+	writel((1 << (16+4)) | (1 << 4), &rtc_reg_ptr->rtc_ctrl);
+

Plenty of magic values here, please explain.

I will give definitions for them.

quoted

+	return 0;
+}
+
+static int sp_rtc_resume(struct platform_device *pdev) {
+	/*						*/
+	/* Because RTC is still powered during suspend,	*/
+	/* there is nothing to do here.			*/
+	/*						*/
+	FUNC_DEBUG();
+
+	// Keep RTC from system reset
+	writel((1 << (16+4)) | (1 << 4), &rtc_reg_ptr->rtc_ctrl);
+
+	return 0;
+}
+
+static int sp_rtc_set_time(struct device *dev, struct rtc_time *tm) {
+	unsigned long secs;
+
+	secs = rtc_tm_to_time64(tm);
+	RTC_DEBUG("%s, secs = %lu\n", __func__, secs);
+	sp_set_seconds(secs);
+
+	return 0;
+}
+
+static int sp_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
+*alrm) {
+	struct rtc_device *rtc = dev_get_drvdata(dev);
+	unsigned long alarm_time;
+
+	alarm_time = rtc_tm_to_time64(&alrm->time);
+	RTC_DEBUG("%s, alarm_time: %u\n", __func__, (u32)(alarm_time));
+
+	if (alarm_time > 0xFFFFFFFF)
+		return -EINVAL;

Please set the range of the rtc properly and the core will do this check for you.

I modified it as the following statements

#define RTC_ALARM_SET 0x50		//register offset
#define ALARM_SET 0xFFFFFFFF		//field in the register
If (alarm_time > ALARM_SET)
      return -EINVAL;

Is it applicable??

quoted

+
+	if ((rtc->aie_timer.enabled) && (rtc->aie_timer.node.expires ==

ktime_set(alarm_time, 0))) {

quoted

+		if (rtc->uie_rtctimer.enabled)
+			sp_rtc.set_alarm_again = 1;
+	}

You have to explain that.

Since the alarm and update interrupts use the same HW one, rtc->aie_timer.enabled,
rtc->uie_rtctimer.enabled and sp_rtc.set_alarm_again are used to distinguish between
alarm interrupt and update one in rtc_irq_handler() (RTC_UF or RTC_AF). There is
only alarm interrupt supported in out HW. I found the update interrupt is implemented
by the alarm one in kernel.

quoted

+
+	writel((u32)alarm_time, &rtc_reg_ptr->rtc_alarm_set);
+	wmb();			// make sure settings are effective.

doesn't writel come with a barrier?

It is useless. I will remove it.

quoted

+
+	// enable alarm for update irq
+	if (rtc->uie_rtctimer.enabled)
+		writel((0x003F << 16) | 0x17, &rtc_reg_ptr->rtc_ctrl);
+	else if (!rtc->aie_timer.enabled)
+		writel((0x0007 << 16) | 0x0, &rtc_reg_ptr->rtc_ctrl);

Magic values, please explain also, I'm not sure why you need to look at
uie_rtctimer and aie_timer as your RTC seems capable of having an alarm
every seconds.

1. I will give the definitions for that magic values.
2. It is for update interrupt. rtc_alarm_irq_enable() will call alarm_irq_enable()
  to enable HW alarm interrupt , but rtc_update_irq_enable() will not. Therefore
  the HW interrupt for update one is enabled here. Otherwise how can I enable
  HW alarm interrupt for update one??

quoted

+
+	return 0;
+}
+
+static int sp_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
+*alrm) {
+	unsigned int alarm_time;
+
+	alarm_time = readl(&rtc_reg_ptr->rtc_alarm_set);
+	RTC_DEBUG("%s, alarm_time: %u\n", __func__, alarm_time);
+	rtc_time64_to_tm((unsigned long)(alarm_time), &alrm->time);
+

You have to also set whether the alarm is enabled or not, else, simply don't
bother returning anything.

I will modify it.

quoted

+	return 0;
+}
+
+static int sp_rtc_alarm_irq_enable(struct device *dev, unsigned int
+enabled) {
+	struct rtc_device *rtc = dev_get_drvdata(dev);
+
+	if (enabled)
+		writel((0x003F << 16) | 0x17, &rtc_reg_ptr->rtc_ctrl);
+	else if (!rtc->uie_rtctimer.enabled)
+		writel((0x0007 << 16) | 0x0, &rtc_reg_ptr->rtc_ctrl);
+
+	return 0;
+}
+
+static const struct rtc_class_ops sp_rtc_ops = {
+	.read_time =		sp_rtc_read_time,
+	.set_time =		sp_rtc_set_time,
+	.set_alarm =		sp_rtc_set_alarm,
+	.read_alarm =		sp_rtc_read_alarm,
+	.alarm_irq_enable =	sp_rtc_alarm_irq_enable,
+};
+
+static irqreturn_t rtc_irq_handler(int irq, void *dev_id) {
+	struct platform_device *plat_dev = dev_id;
+	struct rtc_device *rtc = platform_get_drvdata(plat_dev);
+
+	if (rtc->uie_rtctimer.enabled) {
+		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_UF);
+		RTC_DEBUG("[RTC] update irq\n");
+
+		if (sp_rtc.set_alarm_again == 1) {
+			sp_rtc.set_alarm_again = 0;
+			rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
+			RTC_DEBUG("[RTC] alarm irq\n");
+		}
+	} else {
+		rtc_update_irq(rtc, 1, RTC_IRQF | RTC_AF);
+		RTC_DEBUG("[RTC] alarm irq\n");
+	}

I'm pretty sure you can get rid of most of that and stop looking at uie_rtctimer.

rtc->aie_timer.enabled, rtc->uie_rtctimer.enabled and sp_rtc.set_alarm_again are
used to distinguish between alarm interrupt and update one. There 3 conditions for
triggering the alarm interrupt :
1. only alarm
2. only update
3. alarm and update together

quoted

+
+	return IRQ_HANDLED;
+}
+
+/* ----------------------------------------------------------------------------------------------

*/

quoted

+/* mode   bat_charge_rsel   bat_charge_dsel   bat_charge_en

Remarks				  */

quoted

+/* 0xE            x              x                 0

Disable				  */

quoted

+/* 0x1            0              0                 1

0.86mA (2K Ohm with diode)	  */

quoted

+/* 0x5            1              0                 1

1.81mA (250 Ohm with diode)	  */

quoted

+/* 0x9            2              0                 1

2.07mA (50 Ohm with diode)	  */

quoted

+/* 0xD            3              0                 1

16.0mA (0 Ohm with diode)	  */

quoted

+/* 0x3            0              1                 1

1.36mA (2K Ohm without diode)	  */

quoted

+/* 0x7            1              1                 1

3.99mA (250 Ohm without diode)	  */

quoted

+/* 0xB            2              1                 1

4.41mA (50 Ohm without diode)	  */

quoted

+/* 0xF            3              1                 1

16.0mA (0 Ohm without diode)	  */

quoted

+/*
+---------------------------------------------------------------------
+------------------------- */ static void sp_rtc_set_batt_charge_ctrl(u32 _mode) {
+	u8 m = _mode & 0x000F;
+
+	RTC_DEBUG("battery charge mode: 0x%X\n", m);
+	writel((0x000F << 16) | m, &rtc_reg_ptr->rtc_battery_ctrl); }
+
+static int sp_rtc_probe(struct platform_device *plat_dev) {
+	int ret;
+	int err, irq;
+	struct rtc_device *rtc = NULL;
+	struct resource *res;
+	void __iomem *reg_base = NULL;
+
+	FUNC_DEBUG();
+
+	memset(&sp_rtc, 0, sizeof(sp_rtc));
+
+	// find and map our resources
+	res = platform_get_resource_byname(plat_dev, IORESOURCE_MEM,

RTC_REG_NAME);

quoted

+	RTC_DEBUG("res = 0x%x\n", res->start);
+
+	if (res) {
+		reg_base = devm_ioremap_resource(&plat_dev->dev, res);
+		if (IS_ERR(reg_base))
+			RTC_ERR("%s devm_ioremap_resource fail\n",

RTC_REG_NAME);

quoted

+	}
+	RTC_DEBUG("reg_base = 0x%lx\n", (unsigned long)reg_base);
+
+	// clk
+	sp_rtc.rtcclk = devm_clk_get(&plat_dev->dev, NULL);
+	RTC_DEBUG("sp_rtc->clk = 0x%lx\n", (unsigned long)sp_rtc.rtcclk);
+	if (IS_ERR(sp_rtc.rtcclk))
+		RTC_DEBUG("devm_clk_get fail\n");
+
+	ret = clk_prepare_enable(sp_rtc.rtcclk);
+
+	// reset
+	sp_rtc.rstc = devm_reset_control_get(&plat_dev->dev, NULL);
+	RTC_DEBUG("sp_rtc->rstc = 0x%lx\n", (unsigned long)sp_rtc.rstc);
+	if (IS_ERR(sp_rtc.rstc)) {
+		ret = PTR_ERR(sp_rtc.rstc);
+		RTC_ERR("SPI failed to retrieve reset controller: %d\n", ret);
+		goto free_clk;
+	}
+
+	ret = reset_control_deassert(sp_rtc.rstc);
+	if (ret)
+		goto free_clk;
+
+	rtc_reg_ptr = (struct sp_rtc_reg *)(reg_base);
+
+	// Keep RTC from system reset
+	writel((1 << (16+4)) | (1 << 4), &rtc_reg_ptr->rtc_ctrl);
+
+	// request irq
+	irq = platform_get_irq(plat_dev, 0);
+	if (irq < 0) {
+		RTC_ERR("platform_get_irq failed\n");
+		goto free_reset_assert;
+	}
+
+	err = devm_request_irq(&plat_dev->dev, irq, rtc_irq_handler,
+					IRQF_TRIGGER_RISING, "rtc irq", plat_dev);
+	if (err) {
+		RTC_ERR("devm_request_irq failed: %d\n", err);
+		goto free_reset_assert;
+	}
+
+	// Get charging-mode.
+	ret = of_property_read_u32(plat_dev->dev.of_node, "charging-mode",

&sp_rtc.charging_mode);

quoted

+	if (ret) {
+		RTC_ERR("Failed to retrieve \'charging-mode\'!\n");
+		goto free_reset_assert;
+	}
+	sp_rtc_set_batt_charge_ctrl(sp_rtc.charging_mode);

There are generic trickle charger property, please use those.

I will modify it.

quoted

+
+	device_init_wakeup(&plat_dev->dev, 1);
+
+	rtc = devm_rtc_device_register(&plat_dev->dev, "sp7021-rtc",

&sp_rtc_ops, THIS_MODULE);

quoted

+	if (IS_ERR(rtc)) {
+		ret = PTR_ERR(rtc);
+		goto free_reset_assert;
+	}

Use devm_rtc_allocate_device/devm_rtc_register_device instead.

I will modify it

quoted

+
+	platform_set_drvdata(plat_dev, rtc);
+
+	RTC_INFO("sp7021-rtc loaded\n");
+
+	return 0;
+
+free_reset_assert:
+	reset_control_assert(sp_rtc.rstc);
+free_clk:
+	clk_disable_unprepare(sp_rtc.rtcclk);
+
+	return ret;
+}
+
+static int sp_rtc_remove(struct platform_device *plat_dev) {
+	reset_control_assert(sp_rtc.rstc);
+
+	return 0;
+}
+
+static const struct of_device_id sp_rtc_of_match[] = {
+	{ .compatible = "sunplus,sp7021-rtc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sp_rtc_of_match);
+
+static struct platform_driver sp_rtc_driver = {
+	.probe   = sp_rtc_probe,
+	.remove  = sp_rtc_remove,
+	.suspend = sp_rtc_suspend,
+	.resume  = sp_rtc_resume,
+	.driver  = {
+		.name = "sp7021-rtc",
+		.owner = THIS_MODULE,
+		.of_match_table = sp_rtc_of_match,
+	},
+};
+module_platform_driver(sp_rtc_driver);
+
+MODULE_AUTHOR("Vincent Shih [off-list ref]");
+MODULE_DESCRIPTION("Sunplus RTC driver"); MODULE_LICENSE("GPL

v2");

quoted

+
--
2.7.4

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