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

[Heinrich Schuchardt]

On 07/03/2018 07:29 AM, Takahiro AKASHI wrote:
> 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.

There is no device-tree provided by U-Boot for qemu-arm because it is
already provided by QEMU itself. You can verify with U-Boot command 'fdt
print' that this device tree provides a reg property for the clock.

	fdt addr ${fdtcontroladdr}
	fdt print /

Cf. function board_fdt_blob_setup() in board/emulation/qemu-arm/qemu-arm.c.

Looking at the device trees in the Linux kernel for boards that we do
not currently support like
linux/arch/arm/boot/dts/arm-realview-eb.dtsi
you will find that all boards but one provide a reg property for the
PL031 with the address of the clock registers. And this address depends
on the board.

So if you rely on the QEMU delivered device tree to provide the address
of the RTC registers this will enable us to support the PL031 RTC on
other boards like the HiKey 960 in the future.

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

You are right. In the qemu-arm device tree that string is also used for
other items.

Regards

Heinrich