diff mbox series

[1/2] rtc: Add driver for Sunplus SP7021

Message ID 1635834123-24668-2-git-send-email-vincent.shih@sunplus.com
State Superseded
Headers show
Series Add RTC driver for Sunplus SP7021 SoC | expand

Commit Message

Vincent Shih Nov. 2, 2021, 6:22 a.m. UTC
Add driver for Sunplus SP7021

Signed-off-by: Vincent Shih <vincent.shih@sunplus.com>
---
 MAINTAINERS               |   6 +
 drivers/rtc/Kconfig       |  10 ++
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-sunplus.c | 389 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 406 insertions(+)
 create mode 100644 drivers/rtc/rtc-sunplus.c

Comments

Philipp Zabel Nov. 2, 2021, 2:22 p.m. UTC | #1
On Tue, 2021-11-02 at 14:22 +0800, Vincent Shih wrote:
[...]
> +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();

Drop these.

> +	memset(&sp_rtc, 0, sizeof(sp_rtc));
> +
> +	// find and map our resources
> +	res = platform_get_resource_byname(plat_dev, IORESOURCE_MEM, RTC_REG_NAME);
> +	RTC_DEBUG("res = 0x%x\n", res->start);

Drop, this will crash if res == NULL.

> +
> +	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);
> +	}
> +	RTC_DEBUG("reg_base = 0x%lx\n", (unsigned long)reg_base);

Drop or use dev_dbg() instead.

> +
> +	// 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);

Only enable the clock after all resources are requested. That will
simplify the error path.

> +
> +	// reset
> +	sp_rtc.rstc = devm_reset_control_get(&plat_dev->dev, NULL);

Use devm_reset_control_get_exclusive() instead.
This should be done before clk_prepare_enable().

> +	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;

Then you could use return dev_err_probe() here.

> +	}
> +
> +	ret = reset_control_deassert(sp_rtc.rstc);

Same as for the clock, only deassert the reset after all resources are
requested.

> +	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);

This should be done before clk_prepare_enable().

> +	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);

This could be done before clk_prepare_enable().

> +	if (ret) {
> +		RTC_ERR("Failed to retrieve \'charging-mode\'!\n");
> +		goto free_reset_assert;
> +	}
> +	sp_rtc_set_batt_charge_ctrl(sp_rtc.charging_mode);
> +
> +	device_init_wakeup(&plat_dev->dev, 1);
> +
> +	rtc = devm_rtc_device_register(&plat_dev->dev, "sp7021-rtc", &sp_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc)) {
> +		ret = PTR_ERR(rtc);
> +		goto free_reset_assert;
> +	}
> +
> +	platform_set_drvdata(plat_dev, rtc);
> +
> +	RTC_INFO("sp7021-rtc loaded\n");

Use dev_info() instead.

> +
> +	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);

	clk_disable_unprepare(sp_rtc.rtcclk);


regards
Philipp
Randy Dunlap Nov. 2, 2021, 7:21 p.m. UTC | #2
Hi--

On 11/1/21 11:22 PM, Vincent Shih wrote:
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e1bc521..0c205d2 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1028,6 +1028,16 @@ config RTC_DRV_DS1685_FAMILY
>   	  This driver can also be built as a module. If so, the module
>   	  will be called rtc-ds1685.
>   
> +config RTC_DRV_SUNPLUS
> +	bool "Sunplus SP7021 RTC"
> +	depends on SOC_SP7021
> +	help
> +		Say 'yse' to get support for Sunplus SP7021 real-time clock

		     yes

> +		(RTC) for industrial applications.
> +		It provides RTC status check, timer/alarm functionalities,
> +		user data reservation only with battery with voltage over 2.5V,
> +		RTC power status check and battery charge.


Also please follow coding-style for Kconfig files:

(from Documentation/process/coding-style.rst, section 10):

For all of the Kconfig* configuration files throughout the source tree,
the indentation is somewhat different.  Lines under a ``config`` definition
are indented with one tab, while help text is indented an additional two
spaces.


thanks.
Alexandre Belloni Nov. 3, 2021, 12:36 a.m. UTC | #3
Hello,

On 02/11/2021 14:22:02+0800, Vincent Shih wrote:
> +/**************************************************************************************************/
> +/* How to test RTC:										  */
> +/*												  */
> +/* 1. use kernel commands									  */
> +/* hwclock - query and set the hardware clock (RTC)						  */
> +/*												  */
> +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/* date 121209002014 # Set system to 2014/Dec/12 09:00						  */
> +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/* hwclock -s # Set the System Time from the Hardware Clock					  */
> +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/* date 121213002014 # Set system to 2014/Dec/12 13:00						  */
> +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/* hwclock -w # Set the Hardware Clock to the current System Time				  */
> +/* (for i in `seq 10000`; do (echo ------ && echo -n 'date  : ' && date && echo -n 'hwclock -r: ' */
> +/*								&& hwclock -r; sleep 1); done)	  */
> +/*												  */
> +/* How to setup alarm (e.g., 10 sec later):							  */
> +/*     echo 0 > /sys/class/rtc/rtc0/wakealarm && nnn=`date '+%s'` && echo $nnn && \		  */
> +/*     nnn=`expr $nnn + 10` && echo $nnn > /sys/class/rtc/rtc0/wakealarm			  */
> +/*												  */
> +/* 2. use RTC Driver Test Program (\linux\application\module_test\rtc\rtc-test.c)		  */
> +/*												  */
> +/**************************************************************************************************/

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

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

> +
> +/* ---------------------------------------------------------------------------------------------- */
> +#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.

> +
> +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?

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

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

> +
> +	if ((rtc->aie_timer.enabled) && (rtc->aie_timer.node.expires == ktime_set(alarm_time, 0))) {
> +		if (rtc->uie_rtctimer.enabled)
> +			sp_rtc.set_alarm_again = 1;
> +	}

You have to explain that.

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

doesn't writel come with a barrier?

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

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

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

> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* ---------------------------------------------------------------------------------------------- */
> +/* mode   bat_charge_rsel   bat_charge_dsel   bat_charge_en     Remarks				  */
> +/* 0xE            x              x                 0            Disable				  */
> +/* 0x1            0              0                 1            0.86mA (2K Ohm with diode)	  */
> +/* 0x5            1              0                 1            1.81mA (250 Ohm with diode)	  */
> +/* 0x9            2              0                 1            2.07mA (50 Ohm with diode)	  */
> +/* 0xD            3              0                 1            16.0mA (0 Ohm with diode)	  */
> +/* 0x3            0              1                 1            1.36mA (2K Ohm without diode)	  */
> +/* 0x7            1              1                 1            3.99mA (250 Ohm without diode)	  */
> +/* 0xB            2              1                 1            4.41mA (50 Ohm without diode)	  */
> +/* 0xF            3              1                 1            16.0mA (0 Ohm without diode)	  */
> +/* ---------------------------------------------------------------------------------------------- */
> +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);
> +	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);
> +	}
> +	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);
> +	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.

> +
> +	device_init_wakeup(&plat_dev->dev, 1);
> +
> +	rtc = devm_rtc_device_register(&plat_dev->dev, "sp7021-rtc", &sp_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc)) {
> +		ret = PTR_ERR(rtc);
> +		goto free_reset_assert;
> +	}

Use devm_rtc_allocate_device/devm_rtc_register_device instead.

> +
> +	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 <vincent.shih@sunplus.com>");
> +MODULE_DESCRIPTION("Sunplus RTC driver");
> +MODULE_LICENSE("GPL v2");
> +
> -- 
> 2.7.4
>
Vincent Shih 施錕鴻 Nov. 5, 2021, 12:05 p.m. UTC | #4
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 施錕鴻
> <vincent.shih@sunplus.com>
> Subject: Re: [PATCH 1/2] rtc: Add driver for Sunplus SP7021
> 
> Hello,
> 
> On 02/11/2021 14:22:02+0800, Vincent Shih wrote:
> >
> +/**************************************************************
> ************************************/
> > +/* How to test RTC:										  */
> > +/*												  */
> > +/* 1. use kernel commands									  */
> > +/* hwclock - query and set the hardware clock (RTC)
> 	  */
> > +/*												  */
> > +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date &&
> echo -n 'hwclock -r: ' */
> > +/*								&& hwclock -r; sleep 1); done)
> */
> > +/* date 121209002014 # Set system to 2014/Dec/12 09:00
> 		  */
> > +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date &&
> echo -n 'hwclock -r: ' */
> > +/*								&& hwclock -r; sleep 1); done)
> */
> > +/* hwclock -s # Set the System Time from the Hardware Clock
> 		  */
> > +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date &&
> echo -n 'hwclock -r: ' */
> > +/*								&& hwclock -r; sleep 1); done)
> */
> > +/* date 121213002014 # Set system to 2014/Dec/12 13:00
> 		  */
> > +/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date &&
> echo -n 'hwclock -r: ' */
> > +/*								&& hwclock -r; sleep 1); done)
> */
> > +/* hwclock -w # Set the Hardware Clock to the current System Time
> 		  */
> > +/* (for i in `seq 10000`; do (echo ------ && echo -n 'date  : ' && date &&
> echo -n 'hwclock -r: ' */
> > +/*								&& hwclock -r; sleep 1); done)
> */
> > +/*												  */
> > +/* How to setup alarm (e.g., 10 sec later):
> */
> > +/*     echo 0 > /sys/class/rtc/rtc0/wakealarm && nnn=`date '+%s'` &&
> echo $nnn && \		  */
> > +/*     nnn=`expr $nnn + 10` && echo $nnn >
> /sys/class/rtc/rtc0/wakealarm			  */
> > +/*												  */
> > +/* 2. use RTC Driver Test Program
> (\linux\application\module_test\rtc\rtc-test.c)		  */
> > +/*												  */
> >
> +/**************************************************************
> ******
> > +******************************/
> 
> 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.

> 
> > +#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.

> 
> > +
> > +/*
> > +---------------------------------------------------------------------
> > +------------------------- */ #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.

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

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

> 
> > +	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??

> 
> > +
> > +	if ((rtc->aie_timer.enabled) && (rtc->aie_timer.node.expires ==
> ktime_set(alarm_time, 0))) {
> > +		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.

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

> 
> > +
> > +	// 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??

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

> 
> > +	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

> 
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/* ----------------------------------------------------------------------------------------------
> */
> > +/* mode   bat_charge_rsel   bat_charge_dsel   bat_charge_en
> Remarks				  */
> > +/* 0xE            x              x                 0
> Disable				  */
> > +/* 0x1            0              0                 1
> 0.86mA (2K Ohm with diode)	  */
> > +/* 0x5            1              0                 1
> 1.81mA (250 Ohm with diode)	  */
> > +/* 0x9            2              0                 1
> 2.07mA (50 Ohm with diode)	  */
> > +/* 0xD            3              0                 1
> 16.0mA (0 Ohm with diode)	  */
> > +/* 0x3            0              1                 1
> 1.36mA (2K Ohm without diode)	  */
> > +/* 0x7            1              1                 1
> 3.99mA (250 Ohm without diode)	  */
> > +/* 0xB            2              1                 1
> 4.41mA (50 Ohm without diode)	  */
> > +/* 0xF            3              1                 1
> 16.0mA (0 Ohm without diode)	  */
> > +/*
> > +---------------------------------------------------------------------
> > +------------------------- */ 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);
> > +	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);
> > +	}
> > +	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);
> > +	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.

> 
> > +
> > +	device_init_wakeup(&plat_dev->dev, 1);
> > +
> > +	rtc = devm_rtc_device_register(&plat_dev->dev, "sp7021-rtc",
> &sp_rtc_ops, THIS_MODULE);
> > +	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

> > +
> > +	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 <vincent.shih@sunplus.com>");
> > +MODULE_DESCRIPTION("Sunplus RTC driver"); MODULE_LICENSE("GPL
> v2");
> > +
> > --
> > 2.7.4
> >
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Alexandre Belloni Nov. 5, 2021, 12:52 p.m. UTC | #5
On 05/11/2021 12:05:22+0000, Vincent Shih 施錕鴻 wrote:
> > > +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??
> 

No, please set the rtc .range_min and .range_max and the core will do
the check for you.

> > 
> > > +
> > > +	if ((rtc->aie_timer.enabled) && (rtc->aie_timer.node.expires ==
> > ktime_set(alarm_time, 0))) {
> > > +		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.
> 
> > 
> > > +
> > > +	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.
> 
> > 
> > > +
> > > +	// 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??
> 

From the .set_alarm point of view, there is no difference between a
regular alarm and the uie alarm set by the core. alrm.enabled will
always be enabled.

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

No, stop emulating UIE in your driver and let the core handle that. If
UIE is enabled for the RTC, it will set up an alarm every second. Your
driver doesn't have to handle the difference.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 3b79fd4..6c1a535 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17945,6 +17945,12 @@  L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS RTC DRIVER
+M:	Vincent Shih <vincent.shih@sunplus.com>
+L:	linux-rtc@vger.kernel.org
+S:	Maintained
+F:	drivers/rtc/rtc-sunplus.c
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index e1bc521..0c205d2 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1028,6 +1028,16 @@  config RTC_DRV_DS1685_FAMILY
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-ds1685.
 
+config RTC_DRV_SUNPLUS
+	bool "Sunplus SP7021 RTC"
+	depends on SOC_SP7021
+	help
+		Say 'yse' to get support for Sunplus SP7021 real-time clock
+		(RTC) for industrial applications.
+		It provides RTC status check, timer/alarm functionalities,
+		user data reservation only with battery with voltage over 2.5V,
+		RTC power status check and battery charge.
+
 choice
 	prompt "Subtype"
 	depends on RTC_DRV_DS1685_FAMILY
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 5ceeafe..92039b3 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -165,6 +165,7 @@  obj-$(CONFIG_RTC_DRV_STMP)	+= rtc-stmp3xxx.o
 obj-$(CONFIG_RTC_DRV_SUN4V)	+= rtc-sun4v.o
 obj-$(CONFIG_RTC_DRV_SUN6I)	+= rtc-sun6i.o
 obj-$(CONFIG_RTC_DRV_SUNXI)	+= rtc-sunxi.o
+obj-$(CONFIG_RTC_DRV_SUNPLUS)	+= rtc-sunplus.o
 obj-$(CONFIG_RTC_DRV_TEGRA)	+= rtc-tegra.o
 obj-$(CONFIG_RTC_DRV_TEST)	+= rtc-test.o
 obj-$(CONFIG_RTC_DRV_TPS6586X)	+= rtc-tps6586x.o
diff --git a/drivers/rtc/rtc-sunplus.c b/drivers/rtc/rtc-sunplus.c
new file mode 100644
index 0000000..6d87ab6
--- /dev/null
+++ b/drivers/rtc/rtc-sunplus.c
@@ -0,0 +1,389 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/**************************************************************************************************/
+/* How to test RTC:										  */
+/*												  */
+/* 1. use kernel commands									  */
+/* hwclock - query and set the hardware clock (RTC)						  */
+/*												  */
+/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
+/*								&& hwclock -r; sleep 1); done)	  */
+/* date 121209002014 # Set system to 2014/Dec/12 09:00						  */
+/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
+/*								&& hwclock -r; sleep 1); done)	  */
+/* hwclock -s # Set the System Time from the Hardware Clock					  */
+/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
+/*								&& hwclock -r; sleep 1); done)	  */
+/* date 121213002014 # Set system to 2014/Dec/12 13:00						  */
+/* (for i in `seq 5`; do (echo ------ && echo -n 'date      : ' && date && echo -n 'hwclock -r: ' */
+/*								&& hwclock -r; sleep 1); done)	  */
+/* hwclock -w # Set the Hardware Clock to the current System Time				  */
+/* (for i in `seq 10000`; do (echo ------ && echo -n 'date  : ' && date && echo -n 'hwclock -r: ' */
+/*								&& hwclock -r; sleep 1); done)	  */
+/*												  */
+/* How to setup alarm (e.g., 10 sec later):							  */
+/*     echo 0 > /sys/class/rtc/rtc0/wakealarm && nnn=`date '+%s'` && echo $nnn && \		  */
+/*     nnn=`expr $nnn + 10` && echo $nnn > /sys/class/rtc/rtc0/wakealarm			  */
+/*												  */
+/* 2. use RTC Driver Test Program (\linux\application\module_test\rtc\rtc-test.c)		  */
+/*												  */
+/**************************************************************************************************/
+#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>
+
+/* ---------------------------------------------------------------------------------------------- */
+#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;
+};
+
+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);
+
+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);
+
+	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;
+
+	if ((rtc->aie_timer.enabled) && (rtc->aie_timer.node.expires == ktime_set(alarm_time, 0))) {
+		if (rtc->uie_rtctimer.enabled)
+			sp_rtc.set_alarm_again = 1;
+	}
+
+	writel((u32)alarm_time, &rtc_reg_ptr->rtc_alarm_set);
+	wmb();			// make sure settings are effective.
+
+	// 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);
+
+	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);
+
+	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");
+	}
+
+	return IRQ_HANDLED;
+}
+
+/* ---------------------------------------------------------------------------------------------- */
+/* mode   bat_charge_rsel   bat_charge_dsel   bat_charge_en     Remarks				  */
+/* 0xE            x              x                 0            Disable				  */
+/* 0x1            0              0                 1            0.86mA (2K Ohm with diode)	  */
+/* 0x5            1              0                 1            1.81mA (250 Ohm with diode)	  */
+/* 0x9            2              0                 1            2.07mA (50 Ohm with diode)	  */
+/* 0xD            3              0                 1            16.0mA (0 Ohm with diode)	  */
+/* 0x3            0              1                 1            1.36mA (2K Ohm without diode)	  */
+/* 0x7            1              1                 1            3.99mA (250 Ohm without diode)	  */
+/* 0xB            2              1                 1            4.41mA (50 Ohm without diode)	  */
+/* 0xF            3              1                 1            16.0mA (0 Ohm without diode)	  */
+/* ---------------------------------------------------------------------------------------------- */
+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);
+	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);
+	}
+	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);
+	if (ret) {
+		RTC_ERR("Failed to retrieve \'charging-mode\'!\n");
+		goto free_reset_assert;
+	}
+	sp_rtc_set_batt_charge_ctrl(sp_rtc.charging_mode);
+
+	device_init_wakeup(&plat_dev->dev, 1);
+
+	rtc = devm_rtc_device_register(&plat_dev->dev, "sp7021-rtc", &sp_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc)) {
+		ret = PTR_ERR(rtc);
+		goto free_reset_assert;
+	}
+
+	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 <vincent.shih@sunplus.com>");
+MODULE_DESCRIPTION("Sunplus RTC driver");
+MODULE_LICENSE("GPL v2");
+