From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Date: Wed, 4 Jul 2018 10:53:34 +0200 Subject: [U-Boot] [PATCH v2 1/2] rtc: pl031: convert the driver to driver model In-Reply-To: <20180704073628.23596-2-takahiro.akashi@linaro.org> References: <20180704073628.23596-1-takahiro.akashi@linaro.org> <20180704073628.23596-2-takahiro.akashi@linaro.org> Message-ID: <98c08ea3-47bd-0872-04b8-4177d7a78c31@suse.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > --- > 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 > #include > +#include > +#include > #include > > -#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 > + > +struct pl031_rtc_platdata { > + phys_addr_t base; > +}; > + > +#endif