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.aka...@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
>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
