From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takahiro AKASHI Date: Tue, 3 Jul 2018 14:29:46 +0900 Subject: [U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC In-Reply-To: <4fbf7098-b7ed-bc43-8dbe-7fa7f95cdd68@gmx.de> References: <20180628223416.29533-1-xypron.glpk@gmx.de> <3036110f-231f-0bc9-8b26-9ece81443cb0@iki.fi> <20180702235131.GR23681@linaro.org> <4fbf7098-b7ed-bc43-8dbe-7fa7f95cdd68@gmx.de> Message-ID: <20180703052945.GT23681@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > >>> --- > >>> 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 > >> Tested-by: Tuomas Tynkkynen > >> > >> - 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 > > Date: Tue, 3 Jul 2018 08:32:16 +0900 > > Subject: [PATCH] rtc: pl031: convert the driver to driver model > > > > Signed-off-by: AKASHI Takahiro > > --- > > 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 > > +#include > > +#include > > #include > > > > +#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 > > > > 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 > > #include > > +#include > > +#include > > #include > > > > #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 > > >