[U-Boot] [PATCH v2 1/2] rtc: pl031: convert the driver to driver model
Alexander Graf
agraf at suse.de
Wed Jul 4 08:53:34 UTC 2018
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 at 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
More information about the U-Boot
mailing list