[U-Boot] [PATCH 1/1] ARM: qemu-arm: enable RTC
Takahiro AKASHI
takahiro.akashi at linaro.org
Tue Jul 3 05:29:46 UTC 2018
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 at 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 at iki.fi>
> >> Tested-by: Tuomas Tynkkynen <tuomas.tynkkynen at 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 at 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 at 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
> >
>
More information about the U-Boot
mailing list