Re: [Minios-devel] [RFC PATCH v2 1/2] enable rtc pl031 for arm

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Jiayong Wu,

There is a checkpatch error.

Please find the comments inline:

Thanks & Regards
Sharan

On 11/28/19 8:40 AM, Jianyong Wu wrote:
> Currently, as rtc is not enabled in arm, wall time can't be
> offered currectly.
> pl031 is chosen as the rtc device for arm.
> Nearly all of the function the device offers has been implemented.
> ---
>   plat/Config.uk            |   7 ++
>   plat/common/arm/rtc.c     | 144 ++++++++++++++++++++++++++++++++++++++
>   plat/common/include/rtc.h |  48 +++++++++++++
>   plat/kvm/Makefile.uk      |   1 +
>   plat/kvm/arm/setup.c      |   6 ++
>   5 files changed, 206 insertions(+)
>   create mode 100644 plat/common/arm/rtc.c
>   create mode 100644 plat/common/include/rtc.h
It would be better to create it as a driver plat/driver/rtc instead of 
plat/common. We should also add it as separate library as we did for gic 
and ofw.

> diff --git a/plat/Config.uk b/plat/Config.uk
> index 0eb5a10..ee78ce1 100644
> --- a/plat/Config.uk
> +++ b/plat/Config.uk
> @@ -21,3 +21,10 @@ config HZ
>         help
>                 Configure the timer interrupt frequency.
>                 Only change this if you know what you're doing.
> +
> +config RTC_PL031
> +       bool "enable rtc pl031"
> +       default n
> +       depends on ARCH_ARM_64
> +       help
> +               pl031 is rtc device for arm. Equipt rtc will let you get wall 
> time.
This configuration should move into the plat/kvm.
> diff --git a/plat/common/arm/rtc.c b/plat/common/arm/rtc.c
> new file mode 100644
> index 0000000..64695c9
> --- /dev/null
> +++ b/plat/common/arm/rtc.c
> @@ -0,0 +1,144 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> + *          Jianyong Wu <Jianyong.Wu@xxxxxxx>
> + *
> + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
Can you consider removing the "THIS HEADER MAY NOT BE EXTRACTED OR 
MODIFIED IN ANY WAY." from the license. Seems incompatible with the BSD 
license.
> + */
> +#include <string.h>
> +#include <libfdt.h>
> +#include <uk/assert.h>
> +#include <uk/essentials.h>
> +#include <uk/print.h>
> +#include <arm/cpu.h>
> +#include <ofw/fdt.h>
> +#include <stdio.h>
> +
> +static uint64_t rtc_base_addr;
> +uint32_t rtc_boot_seconds;
> +
> +/* Define offset of RTC registers */
> +#define RTC_REG_DR     0
> +#define RTC_REG_MR     0x4
> +#define RTC_REG_LR     0x8
> +#define RTC_REG_CR     0xc
> +#define RTC_REG_IMSC   0x10
> +#define RTC_REG_RIS    0x14
> +#define RTC_REG_MIS    0x18
> +#define RTC_REG_ICR    0x1c
  For these register map the document[1] describe these register as 
RTCDR. It
  is better to user similar naming scheme. Maybe we also put it into a 
separate
  header for the register map.
> +
> +#define RTC_REG(r)     (void *)(rtc_base_addr + (r))
> +
> +static const char *rtc_device_list[] = {
> +       "arm,pl031",
> +};
> +
> +static uint32_t rtc_read(void)
1.   why not define the read register based on `struct rtc_time` based 
[2] or even use the struct timespec from time.h [3]. I prefer using the 
second option.
2.  It might be wise to rtc device reference, to the public reference 
like read, write and alarm. From the user application perspective we 
might select the type of the rtc interface and the underlying driver 
would be called.
> +{
> +       return ioreg_read32(RTC_REG(RTC_REG_DR));
> +}
> +
> +/*
> + * set rtc match register comparing with counter
> + * value to generat a interrupt
> + */
> +void rtc_set_match(uint32_t alam)
s/alam/alarm
> +{
> +       ioreg_write32(RTC_REG(RTC_REG_MR), alam);
> +}
> +
> +void rtc_update(uint32_t val)
> +{
> +       ioreg_write32(RTC_REG(RTC_REG_LR), val);
> +}
> +
> +void rtc_enable(void)
> +{
> +       ioreg_write32(RTC_REG(RTC_REG_CR), 1);
Instead of 1 use a macro `RTC_START`
> +}
> +
> +/* return rtc status, 1 denotes enable and 0 denotes disable */
> +uint32_t rtc_get_status(void)
> +{
> +       uint32_t val;
> +
> +       val = ioreg_read32(RTC_REG(RTC_REG_CR));
> +       val &= 0x1;
Instead of 0x1, it is better to use macro like RTC_ENABLE_MASK
> +       return val;
> +}
> +
> +/* mask alam */
s/alam/alarm
> +void rtc_mask_intr(void)
  If I enable the interrupt which handler handles this interrupt? 
Shouldn't handler also be part of this  implementation. If the interrupt 
is need to setup the alarm we might also make it module local function 
instead of exposing it.
> +{
> +       ioreg_write32(RTC_REG(RTC_REG_IMSC), 1);
Do we expect the user to make two call to match the time and enable
the interrupt separately or can we combine it into a single API of
setting the alarm?
> +}
> +
> +/* clear alam mask */
 s/alam/alarm
> +void rtc_unmask_intr(void)
> +{
> +       ioreg_write32(RTC_REG(RTC_REG_IMSC), 0);
> +}
> +
> +/* return the raw state of rtc interrupt before masking*/
> +uint32_t rtc_get_intr_raw_state(void)
> +{
> +       return ioreg_read32(RTC_REG(RTC_REG_RIS));
> +}
> +
> +/* return interrupt state after interrupt masking */
> +uint32_t rtc_get_intr_state(void)
> +{
> +       return ioreg_read32(RTC_REG(RTC_REG_MIS));
> +}
> +
> +void rtc_clear_intr(void)
> +{
> +       ioreg_write32(RTC_REG(RTC_REG_ICR), 1);
> +}
> +
> +void _dtb_init_rtc(void *dtb)
> +{
> +       uint64_t size;
> +       int fdt_rtc, ret;
> +
> +       uk_pr_info("Probing RTC...\n");
> +       fdt_rtc = fdt_node_offset_by_compatible_list(dtb, -1, rtc_device_list);
> +       if (fdt_rtc < 0)
> +               UK_CRASH("Could not find rtc device!\n");
Do we have to crash here?
> +
> +       ret = fdt_get_address(dtb, fdt_rtc, 0, &rtc_base_addr, &size);
> +       if (ret < 0)
> +               UK_CRASH("Could not get rtc address\n");
Do we have to crash here?


> +
> +       /* Record the boot seconds */
> +       rtc_boot_seconds = rtc_read();
 Should we not enable the device before reading from the dev
> +
> +       uk_pr_info("Found RTC on: %lu\n", rtc_base_addr);
> +}
> diff --git a/plat/common/include/rtc.h b/plat/common/include/rtc.h
> new file mode 100644
> index 0000000..20a1b22
> --- /dev/null
> +++ b/plat/common/include/rtc.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> + *          Jianyong Wu <Jianyong.Wu@xxxxxxx>
> + *
> + * Copyright (c) 2019, Arm Ltd. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +
> +#ifndef __PLAT_CMN_ARM_RTC_H__
> +#define __PLAT_CMN_ARM_RTC_H__
> +
> +#ifdef CONFIG_RTC_PL031
> +
> +#include <stdint.h>
> +
> +extern uint32_t rtc_boot_seconds;
> +
> +int _dtb_init_rtc(void *dtb);
> +
> +#endif
> +#endif //__PLAT_CMN_ARM_GICV2_H__
> diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
> index c900d45..8c7ec37 100644
> --- a/plat/kvm/Makefile.uk
> +++ b/plat/kvm/Makefile.uk
> @@ -74,6 +74,7 @@ endif
>   ifeq ($(findstring y,$(CONFIG_KVM_KERNEL_SERIAL_CONSOLE) 
> $(CONFIG_KVM_DEBUG_SERIAL_CONSOLE)),y)
>   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
> $(UK_PLAT_COMMON_BASE)/arm/pl011.c|common
>   endif
> +LIBKVMPLAT_SRCS-$(CONFIG_RTC_PL031) += $(UK_PLAT_COMMON_BASE)/arm/rtc.c|common
>   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
> $(UK_PLAT_COMMON_BASE)/arm/cpu_native.c|common
>   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
> $(UK_PLAT_COMMON_BASE)/arm/cache64.S|common
>   LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) += 
> $(UK_PLAT_COMMON_BASE)/arm/psci_arm64.S|common
> diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
> index 3046646..725a74e 100644
> --- a/plat/kvm/arm/setup.c
> +++ b/plat/kvm/arm/setup.c
> @@ -20,6 +20,7 @@
>    */
>   #include <uk/config.h>
>   #include <libfdt.h>
> +#include <rtc.h>
>   #include <sections.h>
>   #include <kvm/console.h>
>   #include <kvm/config.h>
> @@ -221,6 +222,11 @@ void _libkvmplat_start(void *dtb_pointer)
>         /* Initialize memory from DTB */
>         _init_dtb_mem();
>   
> +#ifdef CONFIG_RTC_PL031
> +       /* Initialize rtc */
> +       _dtb_init_rtc(dtb_pointer);
> +#endif
> +
>         /* Initialize interrupt controller */
>         intctrl_init();
>   

[1] 
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0224c/real_time_clock_pl031_r1p3_technical_reference_manual_DDI0224C.pdf
[2]https://linux.die.net/man/4/rtc

[3] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/time.h.html

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel