| Message ID | 20180704073628.23596-2-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm: qemu-arm: enable PL031 (RTC) | expand |
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. > +#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? > { > - 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? 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. > + 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? 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
On 07/04/2018 09:36 AM, AKASHI Takahiro wrote: > 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)) > +#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) > { > - 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) > + pl031_rtc_init(pdata); > + > + tim = RTC_READ_REG(pdata->base, RTC_DR); > + > + rtc_to_tm(tim, tm); Please, check the return code. The RTC may contain invalid data. > + > + 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; Please, add a debug statement here, too. > > - 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; No error has occurred. Please, return 0. > } > > /* > - * 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) nits: There is no English word initted. I would prefer pl031_initialized. Best regards Heinrich > + pl031_rtc_init(pdata); > > - 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 >
On 07/04/2018 11:53 AM, 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> >> --- ...snip... >> /* >> - * 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? 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. > I think the initialization code in rtc_init() should be move to the .probe callback instead, so there's no need to keep the bool aroun.
Hi Akashi, Thank you for the DM conversion. On 07/04/2018 10:36 AM, AKASHI Takahiro wrote: <..snip..> > 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 > I think this file won't be necessary, the structure can stay private to pl031.c. PL031 is an ARM IP block and U-Boot on ARM uses the device tree to locate devices.
On Wed, Jul 04, 2018 at 02:35:19PM +0300, Tuomas Tynkkynen wrote: > Hi Akashi, > > Thank you for the DM conversion. > > On 07/04/2018 10:36 AM, AKASHI Takahiro wrote: > <..snip..> > >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 > > > > I think this file won't be necessary, the structure can stay private to pl031.c. > PL031 is an ARM IP block and U-Boot on ARM uses the device tree to locate devices. I think that you are suggesting we would use udevice.priv instead of udevice.platdata, right? That will be fine with device-tree based devices. But don't we have to take care of no-device-tree (probably legacy) devices here? Thanks, -Takhairo AKASHI
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 > >
On Wed, Jul 04, 2018 at 12:50:52PM +0200, Heinrich Schuchardt wrote: > On 07/04/2018 09:36 AM, AKASHI Takahiro wrote: > > 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)) > > +#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) > > { > > - 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) > > + pl031_rtc_init(pdata); > > + > > + tim = RTC_READ_REG(pdata->base, RTC_DR); > > + > > + rtc_to_tm(tim, tm); > > Please, check the return code. The RTC may contain invalid data. But how do we know if the value is bogus or not? > > + > > + 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; > > Please, add a debug statement here, too. OK. > > > > - 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; > > No error has occurred. Please, return 0. Ouch. > > } > > > > /* > > - * 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) > > nits: > > There is no English word initted. I would prefer pl031_initialized. This variable will go away. Thanks, -Takahiro AKASHI > Best regards > > Heinrich > > > + pl031_rtc_init(pdata); > > > > - 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 > > >
On Thu, Jul 05, 2018 at 04:12:59PM +0900, AKASHI Takahiro wrote: > On Wed, Jul 04, 2018 at 02:35:19PM +0300, Tuomas Tynkkynen wrote: > > Hi Akashi, > > > > Thank you for the DM conversion. > > > > On 07/04/2018 10:36 AM, AKASHI Takahiro wrote: > > <..snip..> > > >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 > > > > > > > I think this file won't be necessary, the structure can stay private to pl031.c. > > PL031 is an ARM IP block and U-Boot on ARM uses the device tree to locate devices. > > I think that you are suggesting we would use udevice.priv instead of > udevice.platdata, right? That will be fine with device-tree based devices. > But don't we have to take care of no-device-tree (probably legacy) > devices here? Does anybody have a comment on this? I will be able to go either with priv or platdata once agreed. > Thanks, > -Takhairo AKASHI
Hi, On 8 July 2018 at 18:50, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > On Thu, Jul 05, 2018 at 04:12:59PM +0900, AKASHI Takahiro wrote: >> On Wed, Jul 04, 2018 at 02:35:19PM +0300, Tuomas Tynkkynen wrote: >> > Hi Akashi, >> > >> > Thank you for the DM conversion. >> > >> > On 07/04/2018 10:36 AM, AKASHI Takahiro wrote: >> > <..snip..> >> > >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 >> > > >> > >> > I think this file won't be necessary, the structure can stay private to pl031.c. >> > PL031 is an ARM IP block and U-Boot on ARM uses the device tree to locate devices. >> >> I think that you are suggesting we would use udevice.priv instead of >> udevice.platdata, right? That will be fine with device-tree based devices. >> But don't we have to take care of no-device-tree (probably legacy) >> devices here? > > Does anybody have a comment on this? > I will be able to go either with priv or platdata once agreed. There is no need to support legacy (non-DM) options unless there is a board that doesn't build without it. Even then, the hour is late and DM conversions should be well underway, so just removing that driver from the board is probably acceptable until the maintainer gets around to it. You should really use platdata to hold info read from the device tree, so to me what you have looks correct, except that I think the struct can be private to your driver. Instead of devfdt_get_addr(), please use dev_read_addr() which is the livetree version. Regards, Simon
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)) +#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) { - 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) + 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); - 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
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