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

[Takahiro AKASHI <takahiro.akashi@linaro.org>]

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