[U-Boot] [PATCH v2 1/2] rtc: pl031: convert the driver to driver model

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Jul 04, 2018 at 10:53:34AM +0200, Alexander Graf wrote:
> On 07/04/2018 09:36 AM, AKASHI Takahiro wrote:
> 
> This patch is missing a patch description. I'm not the maintainer of the rtc
> code base so it's not my call, but I personally just reject all patches with
> empty patch descriptions ;).
> 
> And thanks a lot for doing the conversion! I think it's a very good step
> forward.
> 
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  drivers/rtc/Kconfig                  |   6 ++
> >  drivers/rtc/pl031.c                  | 109 +++++++++++++++++----------
> >  include/dm/platform_data/rtc_pl031.h |  12 +++
> >  3 files changed, 87 insertions(+), 40 deletions(-)
> >  create mode 100644 include/dm/platform_data/rtc_pl031.h
> >
> >diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> >index a3f8c8aecc..96c4cce410 100644
> >--- a/drivers/rtc/Kconfig
> >+++ b/drivers/rtc/Kconfig
> >@@ -55,6 +55,12 @@ config RTC_MV
> >  	  Enable Marvell RTC driver. This driver supports the rtc that is present
> >  	  on some Marvell SoCs.
> >+config RTC_PL031
> >+	bool "Enable ARM PL031 driver"
> >+	depends on DM_RTC
> >+	help
> >+	  Enable ARM PL031 driver.
> >+
> >  config RTC_S35392A
> >  	bool "Enable S35392A driver"
> >  	select BITREVERSE
> >diff --git a/drivers/rtc/pl031.c b/drivers/rtc/pl031.c
> >index 8955805e3b..eecade8374 100644
> >--- a/drivers/rtc/pl031.c
> >+++ b/drivers/rtc/pl031.c
> >@@ -8,14 +8,10 @@
> >  #include <common.h>
> >  #include <command.h>
> >+#include <dm.h>
> >+#include <dm/platform_data/rtc_pl031.h>
> >  #include <rtc.h>
> >-#if defined(CONFIG_CMD_DATE)
> >-
> >-#ifndef CONFIG_SYS_RTC_PL031_BASE
> >-#error CONFIG_SYS_RTC_PL031_BASE is not defined!
> >-#endif
> >-
> >  /*
> >   * Register definitions
> >   */
> >@@ -30,78 +26,111 @@
> >  #define RTC_CR_START	(1 << 0)
> >-#define	RTC_WRITE_REG(addr, val) \
> >-			(*(volatile unsigned int *)(CONFIG_SYS_RTC_PL031_BASE + (addr)) = (val))
> >-#define	RTC_READ_REG(addr)	\
> >-			(*(volatile unsigned int *)(CONFIG_SYS_RTC_PL031_BASE + (addr)))
> >+#define	RTC_WRITE_REG(base, addr, val) \
> >+			(*(volatile unsigned int *)((base) + (addr)) = (val))
> 
> Any particular reason you don't pass in pdata? While at it you could then
> also convert the macros into inline functions. That would make the code
> slightly more readable and result in pretty much the same binary output.
> 
> Also, you don't want to have the explicit casts here as they may get
> compiled into something that is not explicitly valid as MMIO access opcode.
> Instead, please convert it to readl()/writel() while you're at it.

OK.

> >+#define	RTC_READ_REG(base, addr)	\
> >+			(*(volatile unsigned int *)((base) + (addr)))
> >  static int pl031_initted = 0;
> >  /* Enable RTC Start in Control register*/
> >-void rtc_init(void)
> >+void pl031_rtc_init(struct pl031_rtc_platdata *pdata)
> 
> Does this have to be global still? I guess we can now make this function
> static, no?

See the comment below.

> >  {
> >-	RTC_WRITE_REG(RTC_CR, RTC_CR_START);
> >+	RTC_WRITE_REG(pdata->base, RTC_CR, RTC_CR_START);
> >  	pl031_initted = 1;
> >  }
> >  /*
> >- * Reset the RTC. We set the date back to 1970-01-01.
> >+ * Get the current time from the RTC
> >   */
> >-void rtc_reset(void)
> >+static int pl031_rtc_get(struct udevice *dev, struct rtc_time *tm)
> >  {
> >-	RTC_WRITE_REG(RTC_LR, 0x00);
> >-	if(!pl031_initted)
> >-		rtc_init();
> >+	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> >+	ulong tim;
> >+
> >+	if (!tm) {
> >+		puts("Error getting the date/time\n");
> >+		return -1;
> >+	}
> >+
> >+	if (!pl031_initted)
> 
> In theory with dm you can now have multiple instances of the device, right?

('date' command assumes that there is only one RTC device on system.)

> So we can no longer have a global variable that indicates if a device is
> initialized. Instead, this needs to move into device private data.

As Tuomas suggested, init routine should go into probe function,
so this global will be no longer needed.

> 
> >+		pl031_rtc_init(pdata);
> >+
> >+	tim = RTC_READ_REG(pdata->base, RTC_DR);
> >+
> >+	rtc_to_tm(tim, tm);
> >+
> >+	debug("Get DATE: %4d-%02d-%02d (wday=%d)  TIME: %2d:%02d:%02d\n",
> >+		tm->tm_year, tm->tm_mon, tm->tm_mday, tm->tm_wday,
> >+		tm->tm_hour, tm->tm_min, tm->tm_sec);
> >+
> >+	return 0;
> >  }
> >  /*
> >   * Set the RTC
> >-*/
> >-int rtc_set(struct rtc_time *tmp)
> >+ */
> >+static int pl031_rtc_set(struct udevice *dev, const struct rtc_time *tm)
> >  {
> >+	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> >  	unsigned long tim;
> >-	if(!pl031_initted)
> >-		rtc_init();
> >-
> >-	if (tmp == NULL) {
> >+	if (!tm) {
> >  		puts("Error setting the date/time\n");
> >  		return -1;
> >  	}
> >+	if (!pl031_initted)
> >+		pl031_rtc_init(pdata);
> >+
> >  	/* Calculate number of seconds this incoming time represents */
> >-	tim = rtc_mktime(tmp);
> >+	tim = rtc_mktime(tm);
> >-	RTC_WRITE_REG(RTC_LR, tim);
> >+	RTC_WRITE_REG(pdata->base, RTC_LR, tim);
> >  	return -1;
> >  }
> >  /*
> >- * Get the current time from the RTC
> >+ * Reset the RTC. We set the date back to 1970-01-01.
> >   */
> >-int rtc_get(struct rtc_time *tmp)
> >+static int pl031_rtc_reset(struct udevice *dev)
> >  {
> >-	ulong tim;
> >+	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> >-	if(!pl031_initted)
> >-		rtc_init();
> >+	RTC_WRITE_REG(pdata->base, RTC_LR, 0x00);
> >-	if (tmp == NULL) {
> >-		puts("Error getting the date/time\n");
> >-		return -1;
> >-	}
> >+	if (!pl031_initted)
> >+		pl031_rtc_init(pdata);
> 
> After reset, don't you have to reset the initted bool as well?

Anyhow, this code (rtc_init) will be gone,
and RTC will stay activated after reset.

Writing 0 to CR(control) will also reset current RTC value to 0,
but I don't want to change the logic.

# "date reset" shows the current date:
#   Reset RTC...
#   Date: 2000-01-01 (Saturday)    Time:  0:00:00
# Is this correct behavior?

Thanks,
-Takahiro AKASHI

> 
> Alex
> 
> >-	tim = RTC_READ_REG(RTC_DR);
> >+	return 0;
> >+}
> >+
> >+static const struct rtc_ops pl031_rtc_ops = {
> >+	.get = pl031_rtc_get,
> >+	.set = pl031_rtc_set,
> >+	.reset = pl031_rtc_reset,
> >+};
> >-	rtc_to_tm(tim, tmp);
> >+static const struct udevice_id pl031_rtc_ids[] = {
> >+	{ .compatible = "arm,pl031" },
> >+	{ }
> >+};
> >-	debug ( "Get DATE: %4d-%02d-%02d (wday=%d)  TIME: %2d:%02d:%02d\n",
> >-		tmp->tm_year, tmp->tm_mon, tmp->tm_mday, tmp->tm_wday,
> >-		tmp->tm_hour, tmp->tm_min, tmp->tm_sec);
> >+static int pl031_rtc_ofdata_to_platdata(struct udevice *dev)
> >+{
> >+	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> >+	pdata->base = devfdt_get_addr(dev);
> >  	return 0;
> >  }
> >-#endif
> >+U_BOOT_DRIVER(rtc_pl031) = {
> >+	.name	= "rtc-pl031",
> >+	.id	= UCLASS_RTC,
> >+	.of_match = pl031_rtc_ids,
> >+	.ofdata_to_platdata = pl031_rtc_ofdata_to_platdata,
> >+	.platdata_auto_alloc_size = sizeof(struct pl031_rtc_platdata),
> >+	.ops	= &pl031_rtc_ops,
> >+};
> >diff --git a/include/dm/platform_data/rtc_pl031.h b/include/dm/platform_data/rtc_pl031.h
> >new file mode 100644
> >index 0000000000..8e4ba1ce69
> >--- /dev/null
> >+++ b/include/dm/platform_data/rtc_pl031.h
> >@@ -0,0 +1,12 @@
> >+/* SPDX-License-Identifier: GPL-2.0+ */
> >+
> >+#ifndef __rtc_pl031_h
> >+#define __rtc_pl031_h
> >+
> >+#include <asm/types.h>
> >+
> >+struct pl031_rtc_platdata {
> >+	phys_addr_t base;
> >+};
> >+
> >+#endif
> 
>