[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [PATCH] arm/rtc: enable pl031 for arm64.
Hello Jianyong,
Please find the comments inline:
Thanks & Regards
Sharan
On 12/2/20 10:38 AM, Jianyong Wu wrote:
Hi Sharan,
Thanks for your comments.
-----Original Message-----
From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
Sent: Tuesday, December 1, 2020 12:55 AM
To: Jianyong Wu <Jianyong.Wu@xxxxxxx>; minios-
devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx
Cc: Justin He <Justin.He@xxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>
Subject: Re: [Minios-devel] [PATCH] arm/rtc: enable pl031 for arm64.
Hello Jianyong Wu,
Please find the comments inline:
Thanks & Regards
Sharan
On 3/31/20 11:57 AM, Jianyong Wu wrote:
Currently, rtc is not enabled in arm, so wall time can't be provided
currectly.
pl031 is chosen as the rtc device for arm in this patch, but we have
interface extension of capable of plugging other rtc device.
This patch use the new fdt API of
"fdt_node_offset_idx_by_compatible_list"
in Justin's patch in review.
Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
---
plat/common/arm/time.c | 2 +
plat/drivers/include/rtc/rtc.h | 77 ++++++++
plat/drivers/rtc/pl031.c | 315
+++++++++++++++++++++++++++++++++
plat/kvm/Config.uk | 5 +
plat/kvm/Makefile.uk | 11 +-
5 files changed, 409 insertions(+), 1 deletion(-)
create mode 100644 plat/drivers/include/rtc/rtc.h
create mode 100644 plat/drivers/rtc/pl031.c
diff --git a/plat/common/arm/time.c b/plat/common/arm/time.c index
bbb3c72..7560fff 100644
--- a/plat/common/arm/time.c
+++ b/plat/common/arm/time.c
@@ -150,4 +150,6 @@ void ukplat_time_init(void)
/* Enable timer */
generic_timer_enable();
+/* Initialize rtc */
+_dtb_init_rtc(_libkvmplat_cfg.dtb);
}
diff --git a/plat/drivers/include/rtc/rtc.h
b/plat/drivers/include/rtc/rtc.h new file mode 100644 index
0000000..4dafb87
--- /dev/null
+++ b/plat/drivers/include/rtc/rtc.h
@@ -0,0 +1,77 @@
+/* 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.
Remove the above line since it is incompatible with the BSD license.
OK
+ */
+
+#ifndef __PLAT_KVM_ARM_RTC_H__
+#define __PLAT_KVM_ARM_RTC_H__
+
+struct rtc_time {
+int year;
+int mon;
+int day;
+int hour;
+int min;
+int sec;
+};
+
+struct rtc_ops {
+void (*enable)(int);
+int (*status)(void);
+void (*read)(struct rtc_time *);
+uint32_t (*read_raw)(void);
+void (*write)(struct rtc_time *);
+void (*write_raw)(uint32_t);
We should rename it as read_time and the write_time. I am not sure why we
need to expose the *_raw as a part of the rtc_ops operation because this is
used only for a specific operation.
OK
Instead we could read the boot tick with an API function like:
uint32_t rtc_boot_tick_get(struct rtc_dev *dev);
OK
+void (*read_alarm)(struct rtc_time *);
+uint32_t (*read_alarm_raw)(void);
+void (*write_alarm)(struct rtc_time *);
+void (*write_alarm_raw)(uint32_t);
+void (*alarm_irq_enable)(int);
+};
Why don't these rtc_ops take the rtc_dev as its parameter. It would be
difficult to support multiple device.
OK
+
+struct rtc_dev {
+char *name;
+int id;
+const struct rtc_ops *ops;
+};
+
+#ifdef CONFIG_RTC_PL031
+
+#include <stdint.h>
+
+extern uint32_t rtc_boot_seconds;
+
+int _dtb_init_rtc(void *dtb);
+
+#endif
+#endif //__PLAT_KVM_ARM_GICV2_H__
s/__PLAT_KVM_ARM_GICV2_H__/__PLAT_KVM_ARM_RTC_H__
diff --git a/plat/drivers/rtc/pl031.c b/plat/drivers/rtc/pl031.c
new file mode 100644
index 0000000..ff24411
--- /dev/null
+++ b/plat/drivers/rtc/pl031.c
@@ -0,0 +1,315 @@
+/* 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.
Remove the above line since it is incompatible with the BSD license.
OK
+ */
+#include <string.h>
+#include <libfdt.h>
+#include <stdio.h>
+#include <uk/assert.h>
+#include <uk/essentials.h>
+#include <uk/print.h>
+#include <uk/list.h>
+#include <arm/cpu.h>
+#include <ofw/fdt.h>
+#include <rtc/rtc.h>
+#include <gic/gic-v2.h>
+#include <ofw/gic_fdt.h>
+#include <uk/plat/common/irq.h>
+
+static uint64_t rtc_base_addr;
+uint32_t rtc_boot_seconds;
+
+/* Define offset of RTC registers */
+#define RTC_DR0
+#define RTC_MR0x4
+#define RTC_LR0x8
+#define RTC_CR0xc
+#define RTC_IMSC0x10
+#define RTC_RIS0x14
+#define RTC_MIS0x18
+#define RTC_ICR0x1c
+
+#define RTC_REG(r)(void *)(rtc_base_addr + (r))
+
+#define RTC_DEV_NUM 1
+#define RTC_ENABLE 1
+#define RTC_DISABLE 0
+
+static int day_per_mon[12] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
+
+static const char * const rtc_device_list[] = {
+"arm,pl031",
+};
+
+static void pl031_raw_to_tm(uint32_t raw, struct rtc_time *rt)
Move these function into a header file `rtc.h` as a static inline
function as these are generic conversion functions.
There is a for loop in this function, so it may not be appropriate to be a
inline function. isn't?
Sure the compiler might not inline it. I wasn't completely sure, as it
was constant iteration.
Anyways it would not be a problem to provide it in `plat/driver/rtc/rtc.c`
+{
+int hour, days, years, dy4, dy100, dy400, normal_days,
+day_in_year, sum = 0, leap;
+
+rt->sec = raw % 60;
+rt->min = (raw % (60 * 60)) / 60;
+hour = raw / 60 / 60;
+days = hour / 24;
+rt->hour = hour % 24;
+/*
+ * total days for every continue 4-years, assuming there is a leap
+ * year among every 4 years.
+ */
+dy4 = 365 * 3 + 366;
+// total days for every continue 100-years.
+dy100 = 25 * dy4 - 1;
+// total days for every continue 400-years.
+dy400 = dy100 * 4 + 1;
+// normalize the days by get rid of the additional day in leap year
+normal_days = days - days / dy4 + days / dy100 + days / dy400;
+years = normal_days / 365;
+rt->year = 1970 + years;
+leap = ((!(rt->year % 4) && (rt->year % 100)) || !(rt->year % 400));
+day_in_year = normal_days - years * 365;
+/*
+ * if the residue days larger than the sum of the first two month
+ * we should consider Feb,29.
+ */
+sum += leap * (day_in_year >= (day_per_mon[0] +
day_per_mon[1]));
+for (int i = 0; i < 12; i++) {
+sum += day_per_mon[i];
+if (day_in_year < sum) {
+rt->mon = i + 1;
+rt->day = day_in_year - (sum - day_per_mon[i]) + 1;
+break;
+}
+}
+}
+
+static uint32_t pl031_tm_to_raw(struct rtc_time *rt)
Move these function into a header file `rtc.h` as a static inline function.
it's a little odd to put this device-specific function into rtc.h, as all of
the terms in rtc.h should be
generic.
Ps: also these functions in linux kernel are kept in its specific file.
I am not sure why this function is device specific as it convert a
generic `struct rtc_time` to an uint32 number.
+{
+int leaps, leap, days, sec;
+
+leaps = (rt->year - 1970) / 4 - (rt->year - 1970) / 100 +
+(rt->year - 1970) / 400;
+leap = ((!(rt->year % 4) && (rt->year % 100)) || !(rt->year % 400));
+days = (rt->year - 1970) * 365 + leaps;
+if (rt->mon == 1) {
+days += day_per_mon[0];
+} else {
+for (int i = 0; i < rt->mon - 1; i++)
+days += day_per_mon[i];
+}
+days += rt->day + (rt->mon > 2) * leap - 1;
+sec = days * 3600 * 24 + rt->hour * 3600 + rt->min * 60 + rt->sec;
+
+return sec;
+}
+
+static uint32_t pl031_read_raw(void)
+{
+return ioreg_read32(RTC_REG(RTC_DR));
+}
+
+static void pl031_read_time(struct rtc_time *rt)
+{
+uint32_t raw;
+
+raw = pl031_read_raw();
+pl031_raw_to_tm(raw, rt);
+}
+
+static void pl031_write_raw(uint32_t val)
+{
+ioreg_write32(RTC_REG(RTC_LR), val);
+}
+
+static void pl031_write_time(struct rtc_time *rt)
+{
+uint32_t raw;
+
+raw = pl031_tm_to_raw(rt);
+pl031_write_raw(raw);
+}
+
+/*
+ * set rtc match register comparing with counter
+ * value to generat a interrupt
+ */
+static void pl031_write_alarm_raw(uint32_t alarm)
+{
+ioreg_write32(RTC_REG(RTC_MR), alarm);
+}
+
+static void pl031_write_alarm(struct rtc_time *rt)
+{
+uint32_t raw;
+
+raw = pl031_tm_to_raw(rt);
+pl031_write_alarm_raw(raw);
+}
+
+static uint32_t pl031_read_alarm_raw(void)
+{
+return ioreg_read32(RTC_REG(RTC_MR));
+}
+
+static void pl031_read_alarm(struct rtc_time *rt)
+{
+pl031_raw_to_tm(pl031_read_alarm_raw(), rt);
+}
+
+/*
+ * If pl031 is not enabled, enable it by write 1 to RTC_CR, otherwise
+ * do nothing.
+ */
+static void pl031_enable(int enable)
+{
+ioreg_write32(RTC_REG(RTC_CR), enable);
+}
+
+/* return rtc status, 1 denotes enable and 0 denotes disable */
+static int pl031_get_status(void)
+{
+int val;
+
+val = ioreg_read32(RTC_REG(RTC_CR));
+val &= RTC_ENABLE;
+return val;
+}
+
+/* enable alarm irq, 1 denotes enable, 2 denotes disable */
+static void pl031_enable_intr(int enable)
+{
+ioreg_write32(RTC_REG(RTC_IMSC), enable);
+}
+
+static uint32_t pl031_get_raw_intr_state(void)
+{
+return ioreg_read32(RTC_REG(RTC_RIS));
+}
+
+static void pl031_clear_intr(void)
+{
+while (pl031_get_raw_intr_state())
+ioreg_write32(RTC_REG(RTC_ICR), 1);
+}
+
+/* wait for platform device framework to register this handler */
+int pl031_irq_handler(void *arg __unused)
+{
+pl031_clear_intr();
+// TODO: do something real
+
+return 1;
+}
+
+static const struct rtc_ops ops_pl031 = {
+.enable= pl031_enable,
+.status= pl031_get_status,
+.read= pl031_read_time,
+.read_raw= pl031_read_raw,
+.write= pl031_write_time,
+.write_raw= pl031_write_raw,
+.read_alarm= pl031_read_alarm,
+.read_alarm_raw= pl031_read_alarm_raw,
+.write_alarm= pl031_write_alarm,
+.write_alarm_raw= pl031_write_alarm_raw,
+.alarm_irq_enable= pl031_enable_intr,
+};
+
+const struct rtc_dev rtc_pl031 = {
+.name= "rtc_pl031",
+.id= 0,
+.ops= &ops_pl031,
+};
Do we need to combine the rtc_pl031 driver code with a generic rtc
library code. The code below this comment belongs to generic
`plat/driver/rtc/rtc.c`. While we do this change we should also split it
into 2 libraries namely,
librtc and the libpl031.
What about recovering the "rtc.c" and put these generic terms into it?
Sure we could put the generic function into `plat/driver/rtc/rtc.c`
+
+const struct rtc_dev *rtc_list[RTC_DEV_NUM];
Why should we define this as an array. We might define it using the list
implementation `include/uk/list.h`
+
+static void rtc_dev_register(void)
Register function with parameter struct rtc_dev.
OK
+{
+rtc_list[rtc_pl031.id] = &rtc_pl031;
+}
+
+void _dtb_init_rtc(void *dtb)
+{
+uint64_t size;
+uint32_t irq_type, hwirq, trigger_type;
+int fdt_rtc, ret, index, irq, rc;
+
+uk_pr_info("Probing RTC...\n");
+/*
+ * We choose the first available rtc device in device list as the
+ * system rtc.
+ */
+fdt_rtc = fdt_node_offset_idx_by_compatible_list(dtb, -1,
+rtc_device_list, &index);
Missing this function. I guess this should be
fdt_node_offset_by_compatible_list.
OK
+if (fdt_rtc < 0) {
+uk_pr_warn("Could not find rtc device!, fdt_rtc is %d\n",
+fdt_rtc);
+return;
+}
+
+ret = fdt_get_address(dtb, fdt_rtc, 0, &rtc_base_addr, &size);
+if (ret < 0) {
+uk_pr_warn("Could not get rtc address\n");
Print the error code.
OK
+return;
+}
+
+rc = gic_get_irq_from_dtb(dtb, fdt_rtc, 0, &irq_type, &hwirq,
+&trigger_type);
+if (rc < 0) {
+uk_pr_warn("Failed to find IRQ number from DTB\n");
Print the error code.
OK
+return;
+}
+
+irq = gic_irq_translate(irq_type, hwirq);
+if (irq < 0 || irq >= __MAX_IRQ) {
+uk_pr_warn("Failed to translate IRQ number, type=%u,
+ hwirq=%u\n", irq_type, hwirq);
+return;
+}
+
+rc = ukplat_irq_register(irq, pl031_irq_handler, NULL);
+if (rc < 0) {
+uk_pr_warn("Failed to register rtc interrupt handler\n");
Print the error code.
OK
+return;
+}
+
+rtc_dev_register();
+
+if (!rtc_list[index]->ops->status())
+rtc_list[index]->ops->enable(RTC_ENABLE);
+
+/* Record the boot seconds */
+rtc_boot_seconds = rtc_list[index]->ops->read_raw();
+/* Disable rtc alarm irq at its reset */
+rtc_list[index]->ops->alarm_irq_enable(RTC_DISABLE);
+
+uk_pr_info("Found RTC on: %lu\n", rtc_base_addr);
+}
diff --git a/plat/kvm/Config.uk b/plat/kvm/Config.uk
index 3372b6c..1bf72f3 100644
--- a/plat/kvm/Config.uk
+++ b/plat/kvm/Config.uk
@@ -133,6 +133,11 @@ config LIBGICV2
select LIBOFW
depends on ARCH_ARM_64
+config LIBPL031
+ bool "Arm platform rtc device driver"
+ default y if ARCH_ARM_64
+ depends on ARCH_ARM_64
+
config LIBOFW
bool "Open Firmware library support"
default n
diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
index a6d6f5e..d4f4cfd 100644
--- a/plat/kvm/Makefile.uk
+++ b/plat/kvm/Makefile.uk
@@ -14,7 +14,7 @@ $(eval $(call
addplatlib_s,kvm,libkvmvirtioblk,$(CONFIG_VIRTIO_BLK)))
$(eval $(call addplatlib_s,kvm,libkvmvirtio9p,$(CONFIG_VIRTIO_9P)))
$(eval $(call addplatlib_s,kvm,libkvmofw,$(CONFIG_LIBOFW)))
$(eval $(call addplatlib_s,kvm,libkvmgicv2,$(CONFIG_LIBGICV2)))
-
+$(eval $(call addplatlib_s,kvm,libkvmpl031,$(CONFIG_LIBPL031)))
##
## Platform library definitions
##
@@ -181,3 +181,12 @@ LIBKVMGICV2_CINCLUDES-y += -
I$(UK_PLAT_COMMON_BASE)/include
LIBKVMGICV2_CINCLUDES-y += -I$(UK_PLAT_DRIVERS_BASE)/include
LIBKVMGICV2_SRCS-y += $(UK_PLAT_DRIVERS_BASE)/gic/gic-v2.c
+
+##
+## RTC-PL031 library definitions
+##
+LIBKVMPL031_CINCLUDES-y+= -
I$(LIBKVMPLAT_BASE)/include
Does it need some header from the KVM platform?
I don't think so. This RTC driver is common on arm, it's better put this under
"plat/common/"
But there is no a "Makefile". So I have to put it here.
Thanks
Jianyong
+LIBKVMPL031_CINCLUDES-y+= -
I$(UK_PLAT_COMMON_BASE)/include
+LIBKVMPL031_CINCLUDES-y+= -
I$(UK_PLAT_DRIVERS_BASE)/include
+
+LIBKVMPL031_SRCS-y += $(UK_PLAT_DRIVERS_BASE)/rtc/pl031.c
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
|
