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.g...@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.tynkky...@iki.fi>
>> Tested-by: Tuomas Tynkkynen <tuomas.tynkky...@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@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.aka...@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.aka...@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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
