[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC

[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC

[Heinrich Schuchardt]

QEMU provides an emulated ARM AMBA PrimeCell PL031 RTC.

The patch sets the base address in the board include file according to the
definition in hw/arm/virt.c of the QEMU source. It defines the Kconfig
option for the existing driver, and enables the RTC driver in
qemu_arm64_defconfig and qemu_arm_defconfig as well as the date command.

We need an RTC to provide the GetTime() runtime service in the UEFI
subsystem.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 configs/qemu_arm64_defconfig | 2 ++
 configs/qemu_arm_defconfig   | 2 ++
 drivers/rtc/Kconfig          | 7 +++++++
 include/configs/qemu-arm.h   | 3 +++
 4 files changed, 14 insertions(+)

diff --git a/configs/qemu_arm64_defconfig b/configs/qemu_arm64_defconfig
index cdf5072fe4..e9a35d010f 100644
--- a/configs/qemu_arm64_defconfig
+++ b/configs/qemu_arm64_defconfig
@@ -9,6 +9,7 @@ CONFIG_DISTRO_DEFAULTS=y
 # CONFIG_DISPLAY_BOARDINFO is not set
 CONFIG_CMD_PCI=y
 CONFIG_CMD_USB=y
+CONFIG_CMD_DATE=y
 CONFIG_OF_BOARD=y
 CONFIG_SCSI_AHCI=y
 CONFIG_AHCI_PCI=y
@@ -20,6 +21,7 @@ CONFIG_NVME=y
 CONFIG_PCI=y
 CONFIG_DM_PCI=y
 CONFIG_PCIE_ECAM_GENERIC=y
+CONFIG_RTC_PL031=y
 CONFIG_SCSI=y
 CONFIG_DM_SCSI=y
 CONFIG_SYSRESET=y
diff --git a/configs/qemu_arm_defconfig b/configs/qemu_arm_defconfig
index bbce6cd719..9d30dc31a5 100644
--- a/configs/qemu_arm_defconfig
+++ b/configs/qemu_arm_defconfig
@@ -9,6 +9,7 @@ CONFIG_DISTRO_DEFAULTS=y
 # CONFIG_DISPLAY_BOARDINFO is not set
 CONFIG_CMD_PCI=y
 CONFIG_CMD_USB=y
+CONFIG_CMD_DATE=y
 CONFIG_OF_BOARD=y
 CONFIG_SCSI_AHCI=y
 CONFIG_AHCI_PCI=y
@@ -20,6 +21,7 @@ CONFIG_NVME=y
 CONFIG_PCI=y
 CONFIG_DM_PCI=y
 CONFIG_PCIE_ECAM_GENERIC=y
+CONFIG_RTC_PL031=y
 CONFIG_SCSI=y
 CONFIG_DM_SCSI=y
 CONFIG_SYSRESET=y
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index a3f8c8aecc..54365092ec 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -48,6 +48,13 @@ config RTC_RX8010SJ
 	help
 	  Support for Epson RX8010SJ Real Time Clock devices.
 
+config RTC_PL031
+	bool "Enable ARM AMBA PL031 RTC driver"
+	help
+	  The ARM PrimeCell Real Time Clock (PL031) is an optional SoC
+	  peripheral based on the Advanced Microcontroller Bus Architecture
+	  (AMBA). It is emulated in QEMU virtual ARM machines.
+
 config RTC_MV
 	bool "Enable Marvell RTC driver"
 	depends on DM_RTC
diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h
index 095aec28b1..be516bac0c 100644
--- a/include/configs/qemu-arm.h
+++ b/include/configs/qemu-arm.h
@@ -24,6 +24,9 @@
 /* For block devices, QEMU emulates an ICH9 AHCI controller over PCI */
 #define CONFIG_SYS_SCSI_MAX_SCSI_ID 6
 
+/* QEMU emulates the ARM AMBA PL031 RTC */
+#define CONFIG_SYS_RTC_PL031_BASE	0x09010000
+
 /* Environment options */
 #define CONFIG_ENV_SIZE				SZ_64K
 
-- 
2.17.1

[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC

[Tuomas Tynkkynen]

Hi Heinrich,

On 06/29/2018 01:34 AM, Heinrich Schuchardt wrote:
> QEMU provides an emulated ARM AMBA PrimeCell PL031 RTC.
> 
> The patch sets the base address in the board include file according to the
> definition in hw/arm/virt.c of the QEMU source. It defines the Kconfig
> option for the existing driver, and enables the RTC driver in
> qemu_arm64_defconfig and qemu_arm_defconfig as well as the date command.
> 
> We need an RTC to provide the GetTime() runtime service in the UEFI
> subsystem.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   configs/qemu_arm64_defconfig | 2 ++
>   configs/qemu_arm_defconfig   | 2 ++
>   drivers/rtc/Kconfig          | 7 +++++++
>   include/configs/qemu-arm.h   | 3 +++
>   4 files changed, 14 insertions(+)
> 

Reviewed-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
Tested-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>

- Tuomas

[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC

[Takahiro AKASHI]

On Mon, Jul 02, 2018 at 07:07:55PM +0300, Tuomas Tynkkynen wrote:
> Hi Heinrich,
> 
> On 06/29/2018 01:34 AM, Heinrich Schuchardt wrote:
> >QEMU provides an emulated ARM AMBA PrimeCell PL031 RTC.
> >
> >The patch sets the base address in the board include file according to the
> >definition in hw/arm/virt.c of the QEMU source. It defines the Kconfig
> >option for the existing driver, and enables the RTC driver in
> >qemu_arm64_defconfig and qemu_arm_defconfig as well as the date command.
> >
> >We need an RTC to provide the GetTime() runtime service in the UEFI
> >subsystem.
> >
> >Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >---
> >  configs/qemu_arm64_defconfig | 2 ++
> >  configs/qemu_arm_defconfig   | 2 ++
> >  drivers/rtc/Kconfig          | 7 +++++++
> >  include/configs/qemu-arm.h   | 3 +++
> >  4 files changed, 14 insertions(+)
> >
> 
> Reviewed-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
> Tested-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
> 
> - Tuomas

Well, it is a good time. Why not change the driver to driver model
like below:
 * I intentionally leave CONFIG_DM_RTC not mandatory here
 * The patch may be split into two parts; one for rtc, the other for qemu-arm

---8<---

[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC

[Heinrich Schuchardt]

On 07/03/2018 01:51 AM, Takahiro AKASHI wrote:
> On Mon, Jul 02, 2018 at 07:07:55PM +0300, Tuomas Tynkkynen wrote:
>> Hi Heinrich,
>>
>> On 06/29/2018 01:34 AM, Heinrich Schuchardt wrote:
>>> QEMU provides an emulated ARM AMBA PrimeCell PL031 RTC.
>>>
>>> The patch sets the base address in the board include file according to the
>>> definition in hw/arm/virt.c of the QEMU source. It defines the Kconfig
>>> option for the existing driver, and enables the RTC driver in
>>> qemu_arm64_defconfig and qemu_arm_defconfig as well as the date command.
>>>
>>> We need an RTC to provide the GetTime() runtime service in the UEFI
>>> subsystem.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>  configs/qemu_arm64_defconfig | 2 ++
>>>  configs/qemu_arm_defconfig   | 2 ++
>>>  drivers/rtc/Kconfig          | 7 +++++++
>>>  include/configs/qemu-arm.h   | 3 +++
>>>  4 files changed, 14 insertions(+)
>>>
>>
>> Reviewed-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
>> Tested-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
>>
>> - Tuomas
> 
> Well, it is a good time. Why not change the driver to driver model
> like below:
>  * I intentionally leave CONFIG_DM_RTC not mandatory here
>  * The patch may be split into two parts; one for rtc, the other for qemu-arm

Hello Takahiro,

thank you for your suggestion. Yes we should convert drivers to the
driver model. Unfortunately the patch that you appended below is not
applicable to u-boot/master.

Error: patch failed: include/configs/qemu-arm.h:36
error: include/configs/qemu-arm.h: patch does not apply
Patch failed at 0001 ARM: qemu-arm: enable RTC

So I copied the changes to qemu-arm.h manually. Instead of defining the
base address here it would be preferable to read the address from the
device tree. This will become important if we get

Compiling with CONFIG_DM_RTC and CONFIG_RTC_PL031 resulted in warnings:

  CC      drivers/rtc/pl031.o
drivers/rtc/pl031.c: In function ‘pl031_rtc_set’:
drivers/rtc/pl031.c:141:17: warning: passing argument 1 of ‘rtc_set’
discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  return rtc_set(tm);
                 ^~
drivers/rtc/pl031.c:72:5: note: expected ‘struct rtc_time *’ but
argument is of type ‘const struct rtc_time *’
 int rtc_set(struct rtc_time *tmp)
     ^~~~~~~

I tested both with qemu-arm64_defconfig and qemu-arm_defconfig. The date
command is running fine in both cases.

The driver with your patch cannot be compiled without DM_RTC (due to
missing symbol CONFIG_SYS_RTC_PL031_BASE) so in Kconfig it should depend
on DM_RTC.

There is no need to keep any old style stuff. I suggest to drop legacy
functions and #ifdef CONFIG_DM_RTC. Also the following line can be removed:

scripts/config_whitelist.txt:4118:CONFIG_SYS_RTC_PL031_BASE

In qemu/hw/arm/virt.c, function create_rtc() a device tree node is
created which describes the pl031 RTC including the base address. From
what I read in the code the node should look like this:

/{

	pl011 at 09010000 {
		compatible = "arm,pl011", "arm,primecell";
		#address-cells = <2>;
		#size-cells = <2>;
		reg = reg = <0x09010000 0x00001000>;
		...
	};

};

So there is no need to define SYS_RTC_BASE and using the U_BOOT_DEVICE
macro. We can set up the platform data in the probe function by reading
the reg property of the device node.

Should we also add .compatible="arm,primecell" to the list of IDs?

I would prefer enabling the RTC and the date command by default for
qemu_arm64_defconfig and qemu_arm_defconfig as in my original patch.

If you could, please, send a rebased patch-set, I would be fine with it
replacing my patch.

Best regards

Heinrich

> 
> ---8<---
>>From c2e701dfb8ca48025e8266035cd455287178f85b Mon Sep 17 00:00:00 2001
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Date: Tue, 3 Jul 2018 08:32:16 +0900
> Subject: [PATCH] rtc: pl031: convert the driver to driver model
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  board/emulation/qemu-arm/qemu-arm.c  | 13 ++++
>  drivers/rtc/Kconfig                  |  7 +++
>  drivers/rtc/pl031.c                  | 91 +++++++++++++++++++++++++---
>  include/configs/qemu-arm.h           |  4 ++
>  include/dm/platform_data/rtc_pl031.h | 10 +++
>  5 files changed, 118 insertions(+), 7 deletions(-)
>  create mode 100644 include/dm/platform_data/rtc_pl031.h
> 
> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> index 085cbbef99..3084f2aa71 100644
> --- a/board/emulation/qemu-arm/qemu-arm.c
> +++ b/board/emulation/qemu-arm/qemu-arm.c
> @@ -3,8 +3,21 @@
>   * Copyright (c) 2017 Tuomas Tynkkynen
>   */
>  #include <common.h>
> +#include <dm.h>
> +#include <dm/platform_data/rtc_pl031.h>
>  #include <fdtdec.h>
>  
> +#ifdef CONFIG_DM_RTC
> +static const struct pl031_rtc_platdata pl031_pdata = {
> +	.base = SYS_RTC_BASE,
> +};
> +
> +U_BOOT_DEVICE(qemu_arm_rtc) = {
> +	.name = "rtc-pl031",
> +	.platdata = &pl031_pdata,
> +};
> +#endif
> +
>  #ifdef CONFIG_ARM64
>  #include <asm/armv8/mmu.h>
>  
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index a3f8c8aecc..50d9236601 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -55,6 +55,13 @@ 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 RTC driver"
> +	imply DM_RTC
> +	imply CMD_DATE
> +	help
> +	  Enable ARM PL031 RTC driver.
> +
>  config RTC_S35392A
>  	bool "Enable S35392A driver"
>  	select BITREVERSE
> diff --git a/drivers/rtc/pl031.c b/drivers/rtc/pl031.c
> index 8955805e3b..884e3a13fe 100644
> --- a/drivers/rtc/pl031.c
> +++ b/drivers/rtc/pl031.c
> @@ -8,12 +8,25 @@
>  
>  #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!
> +#define	__RTC_WRITE_REG(base, addr, val) \
> +			(*(volatile unsigned int *)((base) + (addr)) = (val))
> +#define	__RTC_READ_REG(base, addr)	\
> +			(*(volatile unsigned int *)((base) + (addr)))
> +
> +#ifdef CONFIG_DM_RTC
> +phys_addr_t pl031_base;
> +#else
> +# ifndef CONFIG_SYS_RTC_PL031_BASE
> +# error CONFIG_SYS_RTC_PL031_BASE is not defined!
> +# endif
> +
> +phys_addr_t pl031_base = CONFIG_SYS_RTC_PL031_BASE;
>  #endif
>  
>  /*
> @@ -30,10 +43,8 @@
>  
>  #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(addr, val)	__RTC_WRITE_REG(pl031_base, addr, val)
> +#define	RTC_READ_REG(addr)		__RTC_READ_REG(pl031_base, addr)
>  
>  static int pl031_initted = 0;
>  
> @@ -104,4 +115,70 @@ int rtc_get(struct rtc_time *tmp)
>  	return 0;
>  }
>  
> -#endif
> +#ifdef CONFIG_DM_RTC
> +void pl031_rtc_init(struct pl031_rtc_platdata *pdata)
> +{
> +	pl031_base = pdata->base;
> +}
> +
> +static int pl031_rtc_get(struct udevice *dev, struct rtc_time *tm)
> +{
> +	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> +
> +	if (!pl031_initted)
> +		pl031_rtc_init(pdata);
> +
> +	return rtc_get(tm);
> +}
> +
> +static int pl031_rtc_set(struct udevice *dev, const struct rtc_time *tm)
> +{
> +	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> +
> +	if (!pl031_initted)
> +		pl031_rtc_init(pdata);
> +
> +	return rtc_set(tm);
> +}
> +
> +static int pl031_rtc_reset(struct udevice *dev)
> +{
> +	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> +
> +	if (!pl031_initted)
> +		pl031_rtc_init(pdata);
> +
> +	rtc_reset();
> +
> +	return 0;
> +}
> +
> +static const struct rtc_ops pl031_rtc_ops = {
> +	.get = pl031_rtc_get,
> +	.set = pl031_rtc_set,
> +	.reset = pl031_rtc_reset,
> +};
> +
> +static const struct udevice_id pl031_rtc_ids[] = {
> +	{ .compatible = "arm,pl031" },
> +	{ }
> +};
> +
> +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;
> +}
> +
> +U_BOOT_DRIVER(rtc_pl031) = {
> +	.name	= "rtc-pl031",
> +	.id	= UCLASS_RTC,
> +	.ofdata_to_platdata = pl031_rtc_ofdata_to_platdata,
> +	.of_match = pl031_rtc_ids,
> +	.ops	= &pl031_rtc_ops,
> +};
> +#endif /* CONFIG_DM_RTC */
> +
> +#endif /* CONFIG_CMD_DATE */
> diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h
> index b7debb9cc7..569fa729a9 100644
> --- a/include/configs/qemu-arm.h
> +++ b/include/configs/qemu-arm.h
> @@ -36,6 +36,10 @@
>  #define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_FLASH_BASE0
>  #define CONFIG_SYS_FLASH_BASE		CONFIG_SYS_FLASH_BASE0
>  
> +#define SYS_PERI_BASE			0x9000000
> +#define SYS_UART_BASE			SYS_PERI_BASE
> +#define SYS_RTC_BASE			(SYS_PERI_BASE + 0x10000)
> +
>  /* PCI */
>  /*
>   * #define CONFIG_SYS_PCI_64BIT		1
> diff --git a/include/dm/platform_data/rtc_pl031.h b/include/dm/platform_data/rtc_pl031.h
> new file mode 100644
> index 0000000000..b35642b15d
> --- /dev/null
> +++ b/include/dm/platform_data/rtc_pl031.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef __rtc_pl031_h
> +#define __rtc_pl031_h
> +
> +struct pl031_rtc_platdata {
> +	phys_addr_t base;
> +};
> +
> +#endif
> 

[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC

[Heinrich Schuchardt]

On 07/03/2018 04:07 AM, Heinrich Schuchardt wrote:
> On 07/03/2018 01:51 AM, Takahiro AKASHI wrote:
>> On Mon, Jul 02, 2018 at 07:07:55PM +0300, Tuomas Tynkkynen wrote:
>>> Hi Heinrich,
>>>
>>> On 06/29/2018 01:34 AM, Heinrich Schuchardt wrote:
>>>> QEMU provides an emulated ARM AMBA PrimeCell PL031 RTC.
>>>>
>>>> The patch sets the base address in the board include file according to the
>>>> definition in hw/arm/virt.c of the QEMU source. It defines the Kconfig
>>>> option for the existing driver, and enables the RTC driver in
>>>> qemu_arm64_defconfig and qemu_arm_defconfig as well as the date command.
>>>>
>>>> We need an RTC to provide the GetTime() runtime service in the UEFI
>>>> subsystem.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>  configs/qemu_arm64_defconfig | 2 ++
>>>>  configs/qemu_arm_defconfig   | 2 ++
>>>>  drivers/rtc/Kconfig          | 7 +++++++
>>>>  include/configs/qemu-arm.h   | 3 +++
>>>>  4 files changed, 14 insertions(+)
>>>>
>>>
>>> Reviewed-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
>>> Tested-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
>>>
>>> - Tuomas
>>
>> Well, it is a good time. Why not change the driver to driver model
>> like below:
>>  * I intentionally leave CONFIG_DM_RTC not mandatory here
>>  * The patch may be split into two parts; one for rtc, the other for qemu-arm
> 
> Hello Takahiro,
> 
> thank you for your suggestion. Yes we should convert drivers to the
> driver model. Unfortunately the patch that you appended below is not
> applicable to u-boot/master.
> 
> Error: patch failed: include/configs/qemu-arm.h:36
> error: include/configs/qemu-arm.h: patch does not apply
> Patch failed at 0001 ARM: qemu-arm: enable RTC
> 
> So I copied the changes to qemu-arm.h manually. Instead of defining the
> base address here it would be preferable to read the address from the
> device tree. This will become important if we get
> 
> Compiling with CONFIG_DM_RTC and CONFIG_RTC_PL031 resulted in warnings:
> 
>   CC      drivers/rtc/pl031.o
> drivers/rtc/pl031.c: In function ‘pl031_rtc_set’:
> drivers/rtc/pl031.c:141:17: warning: passing argument 1 of ‘rtc_set’
> discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>   return rtc_set(tm);
>                  ^~
> drivers/rtc/pl031.c:72:5: note: expected ‘struct rtc_time *’ but
> argument is of type ‘const struct rtc_time *’
>  int rtc_set(struct rtc_time *tmp)
>      ^~~~~~~
> 
> I tested both with qemu-arm64_defconfig and qemu-arm_defconfig. The date
> command is running fine in both cases.
> 
> The driver with your patch cannot be compiled without DM_RTC (due to
> missing symbol CONFIG_SYS_RTC_PL031_BASE) so in Kconfig it should depend
> on DM_RTC.
> 
> There is no need to keep any old style stuff. I suggest to drop legacy
> functions and #ifdef CONFIG_DM_RTC. Also the following line can be removed:
> 
> scripts/config_whitelist.txt:4118:CONFIG_SYS_RTC_PL031_BASE
> 
> In qemu/hw/arm/virt.c, function create_rtc() a device tree node is
> created which describes the pl031 RTC including the base address. From
> what I read in the code the node should look like this:
> 
> /{
> 
> 	pl011 at 09010000 {
> 		compatible = "arm,pl011", "arm,primecell";
> 		#address-cells = <2>;
> 		#size-cells = <2>;
> 		reg = reg = <0x09010000 0x00001000>;
> 		...
> 	};
> 
> };

You can print the actual device tree within U-Boot using:

	fdt addr ${fdtcontroladdr}
	fdt print /

This is the relevant part of the output:

/ {
        interrupt-parent = <0x00008001>;
        #size-cells = <0x00000002>;
        #address-cells = <0x00000002>;
        compatible = "linux,dummy-virt";

        pl031 at 9010000 {
                clock-names = "apb_pclk";
                clocks = <0x00008000>;
                interrupts = <0x00000000 0x00000002 0x00000004>;
                reg = <0x00000000 0x09010000 0x00000000 0x00001000>;
                compatible = "arm,pl031", "arm,primecell";
        };

};

Regards

Heinrich

[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC

[Takahiro AKASHI]

On Tue, Jul 03, 2018 at 04:07:31AM +0200, Heinrich Schuchardt wrote:
> On 07/03/2018 01:51 AM, Takahiro AKASHI wrote:
> > On Mon, Jul 02, 2018 at 07:07:55PM +0300, Tuomas Tynkkynen wrote:
> >> Hi Heinrich,
> >>
> >> On 06/29/2018 01:34 AM, Heinrich Schuchardt wrote:
> >>> QEMU provides an emulated ARM AMBA PrimeCell PL031 RTC.
> >>>
> >>> The patch sets the base address in the board include file according to the
> >>> definition in hw/arm/virt.c of the QEMU source. It defines the Kconfig
> >>> option for the existing driver, and enables the RTC driver in
> >>> qemu_arm64_defconfig and qemu_arm_defconfig as well as the date command.
> >>>
> >>> We need an RTC to provide the GetTime() runtime service in the UEFI
> >>> subsystem.
> >>>
> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> ---
> >>>  configs/qemu_arm64_defconfig | 2 ++
> >>>  configs/qemu_arm_defconfig   | 2 ++
> >>>  drivers/rtc/Kconfig          | 7 +++++++
> >>>  include/configs/qemu-arm.h   | 3 +++
> >>>  4 files changed, 14 insertions(+)
> >>>
> >>
> >> Reviewed-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
> >> Tested-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
> >>
> >> - Tuomas
> > 
> > Well, it is a good time. Why not change the driver to driver model
> > like below:
> >  * I intentionally leave CONFIG_DM_RTC not mandatory here
> >  * The patch may be split into two parts; one for rtc, the other for qemu-arm
> 
> Hello Takahiro,
> 
> thank you for your suggestion. Yes we should convert drivers to the
> driver model. Unfortunately the patch that you appended below is not
> applicable to u-boot/master.

Thank you for your review. It is very helpful as I have not fully
tested my change.

> Error: patch failed: include/configs/qemu-arm.h:36
> error: include/configs/qemu-arm.h: patch does not apply
> Patch failed at 0001 ARM: qemu-arm: enable RTC
> 
> So I copied the changes to qemu-arm.h manually. Instead of defining the
> base address here it would be preferable to read the address from the
> device tree. This will become important if we get
> 
> Compiling with CONFIG_DM_RTC and CONFIG_RTC_PL031 resulted in warnings:
> 
>   CC      drivers/rtc/pl031.o
> drivers/rtc/pl031.c: In function ‘pl031_rtc_set’:
> drivers/rtc/pl031.c:141:17: warning: passing argument 1 of ‘rtc_set’
> discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>   return rtc_set(tm);
>                  ^~
> drivers/rtc/pl031.c:72:5: note: expected ‘struct rtc_time *’ but
> argument is of type ‘const struct rtc_time *’
>  int rtc_set(struct rtc_time *tmp)
>      ^~~~~~~
> 
> I tested both with qemu-arm64_defconfig and qemu-arm_defconfig. The date
> command is running fine in both cases.
> 
> The driver with your patch cannot be compiled without DM_RTC (due to
> missing symbol CONFIG_SYS_RTC_PL031_BASE) so in Kconfig it should depend
> on DM_RTC.

Ouch.

> There is no need to keep any old style stuff. I suggest to drop legacy
> functions and #ifdef CONFIG_DM_RTC. Also the following line can be removed:

My concern was that it would break any downstream code.


> scripts/config_whitelist.txt:4118:CONFIG_SYS_RTC_PL031_BASE
> 
> In qemu/hw/arm/virt.c, function create_rtc() a device tree node is
> created which describes the pl031 RTC including the base address. From
> what I read in the code the node should look like this:
> 
> /{
> 
> 	pl011 at 09010000 {
> 		compatible = "arm,pl011", "arm,primecell";
> 		#address-cells = <2>;
> 		#size-cells = <2>;
> 		reg = reg = <0x09010000 0x00001000>;
> 		...
> 	};
> 
> };
> 
> So there is no need to define SYS_RTC_BASE and using the U_BOOT_DEVICE
> macro. We can set up the platform data in the probe function by reading
> the reg property of the device node.

Since no dts for qemu-arm is provided in u-boot tree, I'm not sure
that this change makes sense.

> Should we also add .compatible="arm,primecell" to the list of IDs?

Yeah, I know "arm,primecell" is also added for other arm IPs, but
think that it is too vague in contrast to pl031, isn't it?

> I would prefer enabling the RTC and the date command by default for
> qemu_arm64_defconfig and qemu_arm_defconfig as in my original patch.

OK.
At least, CMD_DATE will be enabled automatically by "imply."

> If you could, please, send a rebased patch-set, I would be fine with it
> replacing my patch.

OK.

Thanks,
-Takahiro AKASHI


> Best regards
> 
> Heinrich
> 
> > 
> > ---8<---
> >>From c2e701dfb8ca48025e8266035cd455287178f85b Mon Sep 17 00:00:00 2001
> > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Date: Tue, 3 Jul 2018 08:32:16 +0900
> > Subject: [PATCH] rtc: pl031: convert the driver to driver model
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  board/emulation/qemu-arm/qemu-arm.c  | 13 ++++
> >  drivers/rtc/Kconfig                  |  7 +++
> >  drivers/rtc/pl031.c                  | 91 +++++++++++++++++++++++++---
> >  include/configs/qemu-arm.h           |  4 ++
> >  include/dm/platform_data/rtc_pl031.h | 10 +++
> >  5 files changed, 118 insertions(+), 7 deletions(-)
> >  create mode 100644 include/dm/platform_data/rtc_pl031.h
> > 
> > diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> > index 085cbbef99..3084f2aa71 100644
> > --- a/board/emulation/qemu-arm/qemu-arm.c
> > +++ b/board/emulation/qemu-arm/qemu-arm.c
> > @@ -3,8 +3,21 @@
> >   * Copyright (c) 2017 Tuomas Tynkkynen
> >   */
> >  #include <common.h>
> > +#include <dm.h>
> > +#include <dm/platform_data/rtc_pl031.h>
> >  #include <fdtdec.h>
> >  
> > +#ifdef CONFIG_DM_RTC
> > +static const struct pl031_rtc_platdata pl031_pdata = {
> > +	.base = SYS_RTC_BASE,
> > +};
> > +
> > +U_BOOT_DEVICE(qemu_arm_rtc) = {
> > +	.name = "rtc-pl031",
> > +	.platdata = &pl031_pdata,
> > +};
> > +#endif
> > +
> >  #ifdef CONFIG_ARM64
> >  #include <asm/armv8/mmu.h>
> >  
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index a3f8c8aecc..50d9236601 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -55,6 +55,13 @@ 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 RTC driver"
> > +	imply DM_RTC
> > +	imply CMD_DATE
> > +	help
> > +	  Enable ARM PL031 RTC driver.
> > +
> >  config RTC_S35392A
> >  	bool "Enable S35392A driver"
> >  	select BITREVERSE
> > diff --git a/drivers/rtc/pl031.c b/drivers/rtc/pl031.c
> > index 8955805e3b..884e3a13fe 100644
> > --- a/drivers/rtc/pl031.c
> > +++ b/drivers/rtc/pl031.c
> > @@ -8,12 +8,25 @@
> >  
> >  #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!
> > +#define	__RTC_WRITE_REG(base, addr, val) \
> > +			(*(volatile unsigned int *)((base) + (addr)) = (val))
> > +#define	__RTC_READ_REG(base, addr)	\
> > +			(*(volatile unsigned int *)((base) + (addr)))
> > +
> > +#ifdef CONFIG_DM_RTC
> > +phys_addr_t pl031_base;
> > +#else
> > +# ifndef CONFIG_SYS_RTC_PL031_BASE
> > +# error CONFIG_SYS_RTC_PL031_BASE is not defined!
> > +# endif
> > +
> > +phys_addr_t pl031_base = CONFIG_SYS_RTC_PL031_BASE;
> >  #endif
> >  
> >  /*
> > @@ -30,10 +43,8 @@
> >  
> >  #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(addr, val)	__RTC_WRITE_REG(pl031_base, addr, val)
> > +#define	RTC_READ_REG(addr)		__RTC_READ_REG(pl031_base, addr)
> >  
> >  static int pl031_initted = 0;
> >  
> > @@ -104,4 +115,70 @@ int rtc_get(struct rtc_time *tmp)
> >  	return 0;
> >  }
> >  
> > -#endif
> > +#ifdef CONFIG_DM_RTC
> > +void pl031_rtc_init(struct pl031_rtc_platdata *pdata)
> > +{
> > +	pl031_base = pdata->base;
> > +}
> > +
> > +static int pl031_rtc_get(struct udevice *dev, struct rtc_time *tm)
> > +{
> > +	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> > +
> > +	if (!pl031_initted)
> > +		pl031_rtc_init(pdata);
> > +
> > +	return rtc_get(tm);
> > +}
> > +
> > +static int pl031_rtc_set(struct udevice *dev, const struct rtc_time *tm)
> > +{
> > +	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> > +
> > +	if (!pl031_initted)
> > +		pl031_rtc_init(pdata);
> > +
> > +	return rtc_set(tm);
> > +}
> > +
> > +static int pl031_rtc_reset(struct udevice *dev)
> > +{
> > +	struct pl031_rtc_platdata *pdata = dev_get_platdata(dev);
> > +
> > +	if (!pl031_initted)
> > +		pl031_rtc_init(pdata);
> > +
> > +	rtc_reset();
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct rtc_ops pl031_rtc_ops = {
> > +	.get = pl031_rtc_get,
> > +	.set = pl031_rtc_set,
> > +	.reset = pl031_rtc_reset,
> > +};
> > +
> > +static const struct udevice_id pl031_rtc_ids[] = {
> > +	{ .compatible = "arm,pl031" },
> > +	{ }
> > +};
> > +
> > +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;
> > +}
> > +
> > +U_BOOT_DRIVER(rtc_pl031) = {
> > +	.name	= "rtc-pl031",
> > +	.id	= UCLASS_RTC,
> > +	.ofdata_to_platdata = pl031_rtc_ofdata_to_platdata,
> > +	.of_match = pl031_rtc_ids,
> > +	.ops	= &pl031_rtc_ops,
> > +};
> > +#endif /* CONFIG_DM_RTC */
> > +
> > +#endif /* CONFIG_CMD_DATE */
> > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h
> > index b7debb9cc7..569fa729a9 100644
> > --- a/include/configs/qemu-arm.h
> > +++ b/include/configs/qemu-arm.h
> > @@ -36,6 +36,10 @@
> >  #define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_FLASH_BASE0
> >  #define CONFIG_SYS_FLASH_BASE		CONFIG_SYS_FLASH_BASE0
> >  
> > +#define SYS_PERI_BASE			0x9000000
> > +#define SYS_UART_BASE			SYS_PERI_BASE
> > +#define SYS_RTC_BASE			(SYS_PERI_BASE + 0x10000)
> > +
> >  /* PCI */
> >  /*
> >   * #define CONFIG_SYS_PCI_64BIT		1
> > diff --git a/include/dm/platform_data/rtc_pl031.h b/include/dm/platform_data/rtc_pl031.h
> > new file mode 100644
> > index 0000000000..b35642b15d
> > --- /dev/null
> > +++ b/include/dm/platform_data/rtc_pl031.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +
> > +#ifndef __rtc_pl031_h
> > +#define __rtc_pl031_h
> > +
> > +struct pl031_rtc_platdata {
> > +	phys_addr_t base;
> > +};
> > +
> > +#endif
> > 
> 

[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC

[Heinrich Schuchardt]

On 07/03/2018 07:29 AM, Takahiro AKASHI wrote:
> On Tue, Jul 03, 2018 at 04:07:31AM +0200, Heinrich Schuchardt wrote:
>> On 07/03/2018 01:51 AM, Takahiro AKASHI wrote:
>>> On Mon, Jul 02, 2018 at 07:07:55PM +0300, Tuomas Tynkkynen wrote:
>>>> Hi Heinrich,
>>>>
>>>> On 06/29/2018 01:34 AM, Heinrich Schuchardt wrote:
>>>>> QEMU provides an emulated ARM AMBA PrimeCell PL031 RTC.
>>>>>
>>>>> The patch sets the base address in the board include file according to the
>>>>> definition in hw/arm/virt.c of the QEMU source. It defines the Kconfig
>>>>> option for the existing driver, and enables the RTC driver in
>>>>> qemu_arm64_defconfig and qemu_arm_defconfig as well as the date command.
>>>>>
>>>>> We need an RTC to provide the GetTime() runtime service in the UEFI
>>>>> subsystem.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>>  configs/qemu_arm64_defconfig | 2 ++
>>>>>  configs/qemu_arm_defconfig   | 2 ++
>>>>>  drivers/rtc/Kconfig          | 7 +++++++
>>>>>  include/configs/qemu-arm.h   | 3 +++
>>>>>  4 files changed, 14 insertions(+)
>>>>>
>>>>
>>>> Reviewed-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
>>>> Tested-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
>>>>
>>>> - Tuomas
>>>
>>> Well, it is a good time. Why not change the driver to driver model
>>> like below:
>>>  * I intentionally leave CONFIG_DM_RTC not mandatory here
>>>  * The patch may be split into two parts; one for rtc, the other for qemu-arm
>>
>> Hello Takahiro,
>>
>> thank you for your suggestion. Yes we should convert drivers to the
>> driver model. Unfortunately the patch that you appended below is not
>> applicable to u-boot/master.
> 
> Thank you for your review. It is very helpful as I have not fully
> tested my change.
> 
>> Error: patch failed: include/configs/qemu-arm.h:36
>> error: include/configs/qemu-arm.h: patch does not apply
>> Patch failed at 0001 ARM: qemu-arm: enable RTC
>>
>> So I copied the changes to qemu-arm.h manually. Instead of defining the
>> base address here it would be preferable to read the address from the
>> device tree. This will become important if we get
>>
>> Compiling with CONFIG_DM_RTC and CONFIG_RTC_PL031 resulted in warnings:
>>
>>   CC      drivers/rtc/pl031.o
>> drivers/rtc/pl031.c: In function ‘pl031_rtc_set’:
>> drivers/rtc/pl031.c:141:17: warning: passing argument 1 of ‘rtc_set’
>> discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>>   return rtc_set(tm);
>>                  ^~
>> drivers/rtc/pl031.c:72:5: note: expected ‘struct rtc_time *’ but
>> argument is of type ‘const struct rtc_time *’
>>  int rtc_set(struct rtc_time *tmp)
>>      ^~~~~~~
>>
>> I tested both with qemu-arm64_defconfig and qemu-arm_defconfig. The date
>> command is running fine in both cases.
>>
>> The driver with your patch cannot be compiled without DM_RTC (due to
>> missing symbol CONFIG_SYS_RTC_PL031_BASE) so in Kconfig it should depend
>> on DM_RTC.
> 
> Ouch.
> 
>> There is no need to keep any old style stuff. I suggest to drop legacy
>> functions and #ifdef CONFIG_DM_RTC. Also the following line can be removed:
> 
> My concern was that it would break any downstream code.
> 
> 
>> scripts/config_whitelist.txt:4118:CONFIG_SYS_RTC_PL031_BASE
>>
>> In qemu/hw/arm/virt.c, function create_rtc() a device tree node is
>> created which describes the pl031 RTC including the base address. From
>> what I read in the code the node should look like this:
>>
>> /{
>>
>> 	pl011 at 09010000 {
>> 		compatible = "arm,pl011", "arm,primecell";
>> 		#address-cells = <2>;
>> 		#size-cells = <2>;
>> 		reg = reg = <0x09010000 0x00001000>;
>> 		...
>> 	};
>>
>> };
>>
>> So there is no need to define SYS_RTC_BASE and using the U_BOOT_DEVICE
>> macro. We can set up the platform data in the probe function by reading
>> the reg property of the device node.
> 
> Since no dts for qemu-arm is provided in u-boot tree, I'm not sure
> that this change makes sense.

There is no device-tree provided by U-Boot for qemu-arm because it is
already provided by QEMU itself. You can verify with U-Boot command 'fdt
print' that this device tree provides a reg property for the clock.

	fdt addr ${fdtcontroladdr}
	fdt print /

Cf. function board_fdt_blob_setup() in board/emulation/qemu-arm/qemu-arm.c.

Looking at the device trees in the Linux kernel for boards that we do
not currently support like
linux/arch/arm/boot/dts/arm-realview-eb.dtsi
you will find that all boards but one provide a reg property for the
PL031 with the address of the clock registers. And this address depends
on the board.

So if you rely on the QEMU delivered device tree to provide the address
of the RTC registers this will enable us to support the PL031 RTC on
other boards like the HiKey 960 in the future.

> 
>> Should we also add .compatible="arm,primecell" to the list of IDs?
> 
> Yeah, I know "arm,primecell" is also added for other arm IPs, but
> think that it is too vague in contrast to pl031, isn't it?

You are right. In the qemu-arm device tree that string is also used for
other items.

Regards

Heinrich

[U-Boot] [U-Boot,1/1] ARM: qemu-arm: enable RTC

[Tom Rini]

On Fri, Jun 29, 2018 at 12:34:16AM +0200, Heinrich Schuchardt wrote:

> QEMU provides an emulated ARM AMBA PrimeCell PL031 RTC.
> 
> The patch sets the base address in the board include file according to the
> definition in hw/arm/virt.c of the QEMU source. It defines the Kconfig
> option for the existing driver, and enables the RTC driver in
> qemu_arm64_defconfig and qemu_arm_defconfig as well as the date command.
> 
> We need an RTC to provide the GetTime() runtime service in the UEFI
> subsystem.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
> Tested-by: Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180720/045e9750/attachment.sig>