[U-Boot] [PATCH v2 0/2] arm: qemu-arm: enable PL031 (RTC)

[U-Boot] [PATCH v2 0/2] arm: qemu-arm: enable PL031 (RTC)

[AKASHI Takahiro]

This is a revised version of my proposal[1].

 [1] https://lists.denx.de/pipermail/u-boot/2018-July/333534.html

VMs provided by qemu for arm are equiped with PL031 device.
In patch#1, pl031 driver is converted to driver model, and
in patch#2, this driver is enabled in defconfig of qemu-arm.

This patch set is motivated by running UEFI SCT(Self-Certification Test)
and measuring time on u-boot on qemu-arm.
Heinrich has already posted UEFI's GetTime() implementation.

Changes in v2(July 4, 2018)
* based on Heinrich's comments,
* remove legacy mode interface
* enable the driver in defconfig

AKASHI Takahiro (2):
  rtc: pl031: convert the driver to driver model
  arm: qemu-arm: enable PL031 (RTC) in defconfig

 configs/qemu_arm64_defconfig         |   2 +
 configs/qemu_arm_defconfig           |   2 +
 drivers/rtc/Kconfig                  |   6 ++
 drivers/rtc/pl031.c                  | 109 +++++++++++++++++----------
 include/dm/platform_data/rtc_pl031.h |  12 +++
 5 files changed, 91 insertions(+), 40 deletions(-)
 create mode 100644 include/dm/platform_data/rtc_pl031.h

-- 
2.17.0

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

[AKASHI Takahiro]

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);
+
+	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
-- 
2.17.0

[U-Boot] [PATCH v2 2/2] arm: qemu-arm: enable PL031 (RTC) in defconfig

[AKASHI Takahiro]

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 configs/qemu_arm64_defconfig | 2 ++
 configs/qemu_arm_defconfig   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/configs/qemu_arm64_defconfig b/configs/qemu_arm64_defconfig
index cdf5072fe4..f3e3963860 100644
--- a/configs/qemu_arm64_defconfig
+++ b/configs/qemu_arm64_defconfig
@@ -28,3 +28,5 @@ CONFIG_USB=y
 CONFIG_DM_USB=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_EHCI_PCI=y
+CONFIG_DM_RTC=y
+CONFIG_RTC_PL031=y
diff --git a/configs/qemu_arm_defconfig b/configs/qemu_arm_defconfig
index bbce6cd719..28dfba0283 100644
--- a/configs/qemu_arm_defconfig
+++ b/configs/qemu_arm_defconfig
@@ -28,3 +28,5 @@ CONFIG_USB=y
 CONFIG_DM_USB=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_EHCI_PCI=y
+CONFIG_DM_RTC=y
+CONFIG_RTC_PL031=y
-- 
2.17.0

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

[Alexander Graf]

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

[U-Boot] [PATCH v2 2/2] arm: qemu-arm: enable PL031 (RTC) in defconfig

[Alexander Graf]

On 07/04/2018 09:36 AM, AKASHI Takahiro wrote:
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   configs/qemu_arm64_defconfig | 2 ++
>   configs/qemu_arm_defconfig   | 2 ++
>   2 files changed, 4 insertions(+)
>
> diff --git a/configs/qemu_arm64_defconfig b/configs/qemu_arm64_defconfig
> index cdf5072fe4..f3e3963860 100644
> --- a/configs/qemu_arm64_defconfig
> +++ b/configs/qemu_arm64_defconfig
> @@ -28,3 +28,5 @@ CONFIG_USB=y
>   CONFIG_DM_USB=y
>   CONFIG_USB_EHCI_HCD=y
>   CONFIG_USB_EHCI_PCI=y
> +CONFIG_DM_RTC=y
> +CONFIG_RTC_PL031=y

Is there any particular reason you don't just do select statements in 
the ARCH_QEMU definition? Or maybe imply?


Alex

> diff --git a/configs/qemu_arm_defconfig b/configs/qemu_arm_defconfig
> index bbce6cd719..28dfba0283 100644
> --- a/configs/qemu_arm_defconfig
> +++ b/configs/qemu_arm_defconfig
> @@ -28,3 +28,5 @@ CONFIG_USB=y
>   CONFIG_DM_USB=y
>   CONFIG_USB_EHCI_HCD=y
>   CONFIG_USB_EHCI_PCI=y
> +CONFIG_DM_RTC=y
> +CONFIG_RTC_PL031=y

[U-Boot] [PATCH v2 2/2] arm: qemu-arm: enable PL031 (RTC) in defconfig

[Heinrich Schuchardt]

On 07/04/2018 10:56 AM, Alexander Graf wrote:
> On 07/04/2018 09:36 AM, AKASHI Takahiro wrote:
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>   configs/qemu_arm64_defconfig | 2 ++
>>   configs/qemu_arm_defconfig   | 2 ++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/configs/qemu_arm64_defconfig b/configs/qemu_arm64_defconfig
>> index cdf5072fe4..f3e3963860 100644
>> --- a/configs/qemu_arm64_defconfig
>> +++ b/configs/qemu_arm64_defconfig
>> @@ -28,3 +28,5 @@ CONFIG_USB=y
>>   CONFIG_DM_USB=y
>>   CONFIG_USB_EHCI_HCD=y
>>   CONFIG_USB_EHCI_PCI=y
>> +CONFIG_DM_RTC=y
>> +CONFIG_RTC_PL031=y
> 
> Is there any particular reason you don't just do select statements in
> the ARCH_QEMU definition? Or maybe imply?

'select' makes it impossible to switch configuration options off. We
should only use it if really needed. 'imply' is fine here.

Configuration options should appear in *_defconfig files in the same
sequence as in the generated .config file. CONFIG_RTC_PL031 precedes
CONFIG_SCSI.

Please, add a commit message to the next version of the patch.

Best regards

Heinrich

> 
> 
> Alex
> 
>> diff --git a/configs/qemu_arm_defconfig b/configs/qemu_arm_defconfig
>> index bbce6cd719..28dfba0283 100644
>> --- a/configs/qemu_arm_defconfig
>> +++ b/configs/qemu_arm_defconfig
>> @@ -28,3 +28,5 @@ CONFIG_USB=y
>>   CONFIG_DM_USB=y
>>   CONFIG_USB_EHCI_HCD=y
>>   CONFIG_USB_EHCI_PCI=y
>> +CONFIG_DM_RTC=y
>> +CONFIG_RTC_PL031=y
> 
> 
> 

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

[Heinrich Schuchardt]

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
> 

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

[Tuomas Tynkkynen]

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.

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

[Tuomas Tynkkynen]

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.

[U-Boot] [PATCH v2 2/2] arm: qemu-arm: enable PL031 (RTC) in defconfig

[AKASHI Takahiro]

On Wed, Jul 04, 2018 at 12:25:34PM +0200, Heinrich Schuchardt wrote:
> On 07/04/2018 10:56 AM, Alexander Graf wrote:
> > On 07/04/2018 09:36 AM, AKASHI Takahiro wrote:
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> ---
> >>   configs/qemu_arm64_defconfig | 2 ++
> >>   configs/qemu_arm_defconfig   | 2 ++
> >>   2 files changed, 4 insertions(+)
> >>
> >> diff --git a/configs/qemu_arm64_defconfig b/configs/qemu_arm64_defconfig
> >> index cdf5072fe4..f3e3963860 100644
> >> --- a/configs/qemu_arm64_defconfig
> >> +++ b/configs/qemu_arm64_defconfig
> >> @@ -28,3 +28,5 @@ CONFIG_USB=y
> >>   CONFIG_DM_USB=y
> >>   CONFIG_USB_EHCI_HCD=y
> >>   CONFIG_USB_EHCI_PCI=y
> >> +CONFIG_DM_RTC=y
> >> +CONFIG_RTC_PL031=y
> > 
> > Is there any particular reason you don't just do select statements in
> > the ARCH_QEMU definition? Or maybe imply?
> 
> 'select' makes it impossible to switch configuration options off. We
> should only use it if really needed. 'imply' is fine here.

Basically I don't care whether those two go into defconfig or ARCH_QEMU,
but as far as RTC_PL031 is concerned, it always comes with qemu's VM and
"selecting" it at ARCH_QEMU is quite reasonable.
Turning off this option only saves hundreds of bytes for VM. Who cares?

That's said, we will go for 'imply.'

For CMD_DATE, it will be automatically turned on if DM_RTC.

To make RTC_PL031 independent from CMD_DATE, we also have to
modify rtc/date.c's dependency (DM_RTC).

> Configuration options should appear in *_defconfig files in the same
> sequence as in the generated .config file. CONFIG_RTC_PL031 precedes
> CONFIG_SCSI.

So they will go away from *_defconfig.

Thanks,
-Takahiro AKASHI

> Please, add a commit message to the next version of the patch.
> 
> Best regards
> 
> Heinrich
> 
> > 
> > 
> > Alex
> > 
> >> diff --git a/configs/qemu_arm_defconfig b/configs/qemu_arm_defconfig
> >> index bbce6cd719..28dfba0283 100644
> >> --- a/configs/qemu_arm_defconfig
> >> +++ b/configs/qemu_arm_defconfig
> >> @@ -28,3 +28,5 @@ CONFIG_USB=y
> >>   CONFIG_DM_USB=y
> >>   CONFIG_USB_EHCI_HCD=y
> >>   CONFIG_USB_EHCI_PCI=y
> >> +CONFIG_DM_RTC=y
> >> +CONFIG_RTC_PL031=y
> > 
> > 
> > 
> 

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

[AKASHI Takahiro]

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

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

[AKASHI Takahiro]

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
> 
> 

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

[AKASHI Takahiro]

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
> > 
> 

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

[AKASHI Takahiro]

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

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

[Simon Glass]

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