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