[RFC PATCH 0/3] Add HiSilicon system timer driver

[RFC PATCH 0/3] Add HiSilicon system timer driver

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

HiSilicon system timer is a memory mapped platform timer compatible with
the arm's generic timer specification. The timer supports both SPI and
LPI interrupt and can be enumerated through ACPI DSDT table. Since the
timer is fully compatible with the spec, it can reuse most codes of the
arm_arch_timer driver. However since the arm_arch_timer driver only
supports GTDT and SPI interrupt, this series support the HiSilicon system
timer by:

- refactor some of the arm_arch_timer codes and export the function to
  register a arch memory timer by other drivers
- retrieve the IO memory and interrupt resource through DSDT in a separate
  driver, then setup and register the clockevent device reuse the arm_arch_timer
  function

Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).

Yicong Yang (3):
  clocksource/drivers/arm_arch_timer: Split the function of
    __arch_timer_setup()
  clocksource/drivers/arm_arch_timer: Extend and export
    arch_timer_mem_register()
  clocksource/drivers: Add HiSilicon system timer driver

 drivers/clocksource/Kconfig          |  10 +++
 drivers/clocksource/Makefile         |   1 +
 drivers/clocksource/arm_arch_timer.c | 123 +++++++++++++++------------
 drivers/clocksource/timer-hisi-sys.c |  68 +++++++++++++++
 include/clocksource/arm_arch_timer.h |   2 +
 5 files changed, 148 insertions(+), 56 deletions(-)
 create mode 100644 drivers/clocksource/timer-hisi-sys.c

-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 1/3] clocksource/drivers/arm_arch_timer: Split the function of __arch_timer_setup()

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Currently we use __arch_timer_setup() to setup and register clockevents
device for both cp15 and memory-mapped timer. However there's not too
much in common of the setups for cp15 and memory-mapped timer. So split
the setup function for cp15 and memory-mapped timer into separate
functions. This will also allows future extension for platform timers.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/clocksource/arm_arch_timer.c | 105 ++++++++++++++-------------
 1 file changed, 54 insertions(+), 51 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 7dd2c615bce2..2e20a8ec50ca 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -849,65 +849,68 @@ static u64 __arch_timer_check_delta(void)
 	return CLOCKSOURCE_MASK(arch_counter_get_width());
 }
 
-static void __arch_timer_setup(unsigned type,
-			       struct clock_event_device *clk)
+static void __arch_timer_setup_cp15(struct clock_event_device *clk)
 {
+	typeof(clk->set_next_event) sne;
 	u64 max_delta;
 
 	clk->features = CLOCK_EVT_FEAT_ONESHOT;
 
-	if (type == ARCH_TIMER_TYPE_CP15) {
-		typeof(clk->set_next_event) sne;
-
-		arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
-
-		if (arch_timer_c3stop)
-			clk->features |= CLOCK_EVT_FEAT_C3STOP;
-		clk->name = "arch_sys_timer";
-		clk->rating = 450;
-		clk->cpumask = cpumask_of(smp_processor_id());
-		clk->irq = arch_timer_ppi[arch_timer_uses_ppi];
-		switch (arch_timer_uses_ppi) {
-		case ARCH_TIMER_VIRT_PPI:
-			clk->set_state_shutdown = arch_timer_shutdown_virt;
-			clk->set_state_oneshot_stopped = arch_timer_shutdown_virt;
-			sne = erratum_handler(set_next_event_virt);
-			break;
-		case ARCH_TIMER_PHYS_SECURE_PPI:
-		case ARCH_TIMER_PHYS_NONSECURE_PPI:
-		case ARCH_TIMER_HYP_PPI:
-			clk->set_state_shutdown = arch_timer_shutdown_phys;
-			clk->set_state_oneshot_stopped = arch_timer_shutdown_phys;
-			sne = erratum_handler(set_next_event_phys);
-			break;
-		default:
-			BUG();
-		}
-
-		clk->set_next_event = sne;
-		max_delta = __arch_timer_check_delta();
-	} else {
-		clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
-		clk->name = "arch_mem_timer";
-		clk->rating = 400;
-		clk->cpumask = cpu_possible_mask;
-		if (arch_timer_mem_use_virtual) {
-			clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
-			clk->set_state_oneshot_stopped = arch_timer_shutdown_virt_mem;
-			clk->set_next_event =
-				arch_timer_set_next_event_virt_mem;
-		} else {
-			clk->set_state_shutdown = arch_timer_shutdown_phys_mem;
-			clk->set_state_oneshot_stopped = arch_timer_shutdown_phys_mem;
-			clk->set_next_event =
-				arch_timer_set_next_event_phys_mem;
-		}
+	arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
 
-		max_delta = CLOCKSOURCE_MASK(56);
+	if (arch_timer_c3stop)
+		clk->features |= CLOCK_EVT_FEAT_C3STOP;
+	clk->name = "arch_sys_timer";
+	clk->rating = 450;
+	clk->cpumask = cpumask_of(smp_processor_id());
+	clk->irq = arch_timer_ppi[arch_timer_uses_ppi];
+	switch (arch_timer_uses_ppi) {
+	case ARCH_TIMER_VIRT_PPI:
+		clk->set_state_shutdown = arch_timer_shutdown_virt;
+		clk->set_state_oneshot_stopped = arch_timer_shutdown_virt;
+		sne = erratum_handler(set_next_event_virt);
+		break;
+	case ARCH_TIMER_PHYS_SECURE_PPI:
+	case ARCH_TIMER_PHYS_NONSECURE_PPI:
+	case ARCH_TIMER_HYP_PPI:
+		clk->set_state_shutdown = arch_timer_shutdown_phys;
+		clk->set_state_oneshot_stopped = arch_timer_shutdown_phys;
+		sne = erratum_handler(set_next_event_phys);
+		break;
+	default:
+		BUG();
 	}
 
+	clk->set_next_event = sne;
+	max_delta = __arch_timer_check_delta();
+
 	clk->set_state_shutdown(clk);
+	clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
+}
+
+static void __arch_timer_setup_mem(struct clock_event_device *clk)
+{
+	u64 max_delta;
 
+	clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ;
+	clk->name = "arch_mem_timer";
+	clk->rating = 400;
+	clk->cpumask = cpu_possible_mask;
+	if (arch_timer_mem_use_virtual) {
+		clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
+		clk->set_state_oneshot_stopped = arch_timer_shutdown_virt_mem;
+		clk->set_next_event =
+			arch_timer_set_next_event_virt_mem;
+	} else {
+		clk->set_state_shutdown = arch_timer_shutdown_phys_mem;
+		clk->set_state_oneshot_stopped = arch_timer_shutdown_phys_mem;
+		clk->set_next_event =
+			arch_timer_set_next_event_phys_mem;
+	}
+
+	max_delta = CLOCKSOURCE_MASK(56);
+
+	clk->set_state_shutdown(clk);
 	clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
 }
 
@@ -1004,7 +1007,7 @@ static int arch_timer_starting_cpu(unsigned int cpu)
 	struct clock_event_device *clk = this_cpu_ptr(arch_timer_evt);
 	u32 flags;
 
-	__arch_timer_setup(ARCH_TIMER_TYPE_CP15, clk);
+	__arch_timer_setup_cp15(clk);
 
 	flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
 	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
@@ -1294,7 +1297,7 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
 
 	arch_timer_mem->base = base;
 	arch_timer_mem->evt.irq = irq;
-	__arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt);
+	__arch_timer_setup_mem(&arch_timer_mem->evt);
 
 	if (arch_timer_mem_use_virtual)
 		func = arch_timer_handler_virt_mem;
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 2/3] clocksource/drivers/arm_arch_timer: Extend and export arch_timer_mem_register()

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Currently the memory-mapped timer will be probed by the GTDT table on
ACPI based systems, and the timer interrupt can only be the GSI (SPI
interrupt for arm64) per ACPI spec. However the generic timer
specification doesn't restrict the interrupt type and per BSA Spec
section 3.8.1 (DEN0094C 1.0C) the timer interrupt can also be a LPI
interrupt.

So this patch extends and exports the arch_timer_mem_register() function
to allow other drivers registers a generic timer using LPI interrupt
and probed by other means rather than GTDT. Note that the GTDT timer
still has a higher priority, if a GTDT timer is registered, we'll block
later registration of other timers.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/clocksource/arm_arch_timer.c | 26 +++++++++++++++++---------
 include/clocksource/arm_arch_timer.h |  2 ++
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 2e20a8ec50ca..6e9445192ca4 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -66,7 +66,7 @@ struct arch_timer {
 	struct clock_event_device evt;
 };
 
-static struct arch_timer *arch_timer_mem __ro_after_init;
+static struct arch_timer *arch_timer_mem;
 
 #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
 
@@ -888,15 +888,16 @@ static void __arch_timer_setup_cp15(struct clock_event_device *clk)
 	clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
 }
 
-static void __arch_timer_setup_mem(struct clock_event_device *clk)
+static void __arch_timer_setup_mem(struct clock_event_device *clk,
+				   bool irq_virtual, const char *name)
 {
 	u64 max_delta;
 
 	clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ;
-	clk->name = "arch_mem_timer";
+	clk->name = name;
 	clk->rating = 400;
 	clk->cpumask = cpu_possible_mask;
-	if (arch_timer_mem_use_virtual) {
+	if (irq_virtual) {
 		clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
 		clk->set_state_oneshot_stopped = arch_timer_shutdown_virt_mem;
 		clk->set_next_event =
@@ -1286,25 +1287,30 @@ static int __init arch_timer_register(void)
 	return err;
 }
 
-static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
+int arch_timer_mem_register(void __iomem *base, unsigned int irq,
+			    bool irq_virtual, const char *name)
 {
 	int ret;
 	irq_handler_t func;
 
+	/* If we've already register a memory timer, fail the registration */
+	if (arch_timer_mem)
+		return -EEXIST;
+
 	arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
 	if (!arch_timer_mem)
 		return -ENOMEM;
 
 	arch_timer_mem->base = base;
 	arch_timer_mem->evt.irq = irq;
-	__arch_timer_setup_mem(&arch_timer_mem->evt);
+	__arch_timer_setup_mem(&arch_timer_mem->evt, irq_virtual, name);
 
-	if (arch_timer_mem_use_virtual)
+	if (irq_virtual)
 		func = arch_timer_handler_virt_mem;
 	else
 		func = arch_timer_handler_phys_mem;
 
-	ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
+	ret = request_irq(irq, func, IRQF_TIMER, name, &arch_timer_mem->evt);
 	if (ret) {
 		pr_err("Failed to request mem timer irq\n");
 		kfree(arch_timer_mem);
@@ -1313,6 +1319,7 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(arch_timer_mem_register);
 
 static const struct of_device_id arch_timer_of_match[] __initconst = {
 	{ .compatible   = "arm,armv7-timer",    },
@@ -1560,7 +1567,8 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
 		return -ENXIO;
 	}
 
-	ret = arch_timer_mem_register(base, irq);
+	ret = arch_timer_mem_register(base, irq, arch_timer_mem_use_virtual,
+				      "arch_mem_timer");
 	if (ret) {
 		iounmap(base);
 		return ret;
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index cbbc9a6dc571..d0fa2065586c 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -89,6 +89,8 @@ extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
 extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void);
 extern bool arch_timer_evtstrm_available(void);
+extern int arch_timer_mem_register(void __iomem *base, unsigned int irq,
+				   bool irq_virtual, const char *name);
 
 #else
 
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH 3/3] clocksource/drivers: Add HiSilicon system timer driver

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

HiSilicon system timer is compatible with arm's generic timer
specification but is enumerated through DSDT and can use SPI/LPI
interrupt as timer interrupt. This patch adds the support
for the timer. The driver probes the device IO memory and
interrupt resources through DSDT and then reuse the codes
of the arm_arch_timer for setup and register the clockevent
device.

Example DSDT node will be like:
	Device (TIM0)
        {
            Name (_HID, "HISI03F2")  // _HID: Hardware ID
            Name (_UID, Zero)  // _UID: Unique ID
            Name (RBUF, ResourceTemplate ()
            {
                QWordMemory (ResourceConsumer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
                    0x0000000000000000, // Granularity
                    0x0000000401170000, // Range Minimum
                    0x0000000401170fff, // Range Maximum
                    0x0000000000000000, // Translation Offset
                    0x0000000000001000, // Length
                    ,, , AddressRangeMemory, TypeStatic)
                Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
                {
                    0x000003C6,
                }
            })
	    Method (_CRS, 0, NotSerialized)
	    {
		Return (RBUF)
	    }
	}

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/clocksource/Kconfig          | 10 ++++
 drivers/clocksource/Makefile         |  1 +
 drivers/clocksource/timer-hisi-sys.c | 68 ++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 drivers/clocksource/timer-hisi-sys.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 0ba0dc4ecf06..2e43cd6e2add 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -732,4 +732,14 @@ config GOLDFISH_TIMER
 	help
 	  Support for the timer/counter of goldfish-rtc
 
+config HISI_SYS_TIMER
+	tristate "HiSilicon system timer driver"
+	depends on ARM_ARCH_TIMER && ARM64 && ACPI
+	help
+	  Support for HiSilicon system timer which used as a clockevent
+	  device.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called timer_hisi_sys.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 368c3461dab8..39ababd0d4dd 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -89,3 +89,4 @@ obj-$(CONFIG_MSC313E_TIMER)		+= timer-msc313e.o
 obj-$(CONFIG_GOLDFISH_TIMER)		+= timer-goldfish.o
 obj-$(CONFIG_GXP_TIMER)			+= timer-gxp.o
 obj-$(CONFIG_CLKSRC_LOONGSON1_PWM)	+= timer-loongson1-pwm.o
+obj-$(CONFIG_HISI_SYS_TIMER)		+= timer-hisi-sys.o
diff --git a/drivers/clocksource/timer-hisi-sys.c b/drivers/clocksource/timer-hisi-sys.c
new file mode 100644
index 000000000000..1ef39d97e83d
--- /dev/null
+++ b/drivers/clocksource/timer-hisi-sys.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for HiSilicon system timer driver.
+ *
+ * The device is fully compatible with ARM's Generic Timer specification.
+ * The device is enumerated through DSDT rather than GTDT and can use
+ * LPI interrupt besides SPI.
+ *
+ * Copyright (c) 2023 HiSilicon Technologies Co., Ltd.
+ * Author: Yicong Yang <yangyicong@hisilicon.com>
+ */
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/units.h>
+
+#include <clocksource/arm_arch_timer.h>
+
+#define DRV_NAME	"hisi_sys_timer"
+
+#define CNTFRQ		0x10
+
+static const struct acpi_device_id hisi_sys_timer_acpi_ids[] = {
+	{ "HISI03F2", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, hisi_sys_timer_acpi_ids);
+
+static int hisi_sys_timer_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	void __iomem *iobase;
+	int ret, irq;
+	u32 freq;
+
+	iobase = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(iobase))
+		return PTR_ERR(iobase);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return dev_err_probe(dev, ret, "failed to get interrupt\n");
+
+	ret = arch_timer_mem_register(iobase, irq, false, DRV_NAME);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register timer\n");
+
+	freq = readl_relaxed(iobase + CNTFRQ);
+	dev_info(dev, "%s works at %ldMHz\n", DRV_NAME, freq / HZ_PER_MHZ);
+	return 0;
+}
+
+static struct platform_driver hisi_sys_timer_driver = {
+	.probe = hisi_sys_timer_probe,
+	.driver = {
+		.name = DRV_NAME,
+		.acpi_match_table = hisi_sys_timer_acpi_ids,
+	},
+};
+module_platform_driver(hisi_sys_timer_driver);
+
+MODULE_AUTHOR("Yicong Yang <yangyicong@hisilicon.com>");
+MODULE_DESCRIPTION("HiSilicon system timer driver");
+MODULE_LICENSE("GPL");
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Mark Rutland]

Hi,

On Tue, Oct 10, 2023 at 08:30:30PM +0800, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> HiSilicon system timer is a memory mapped platform timer compatible with
> the arm's generic timer specification. The timer supports both SPI and
> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
> timer is fully compatible with the spec, it can reuse most codes of the
> arm_arch_timer driver. However since the arm_arch_timer driver only
> supports GTDT and SPI interrupt, this series support the HiSilicon system
> timer by:
> 
> - refactor some of the arm_arch_timer codes and export the function to
>   register a arch memory timer by other drivers
> - retrieve the IO memory and interrupt resource through DSDT in a separate
>   driver, then setup and register the clockevent device reuse the arm_arch_timer
>   function
> 
> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).

This seems like an oversight; there *should* be a generic way of describing
this, and I've poked our BSA and ACPI architects to figure out how this is
supposed to work. The lack of a way to do that seems like a major oversight and
something that needs to be solved.

I'll try to get back to you shortly on that.

Regardless of that, I do not think this should be a separate driver, and I'm
very much not keen on having vendor-specific companion drivers like this. Using
LPIs isn't specific to HiSilicon, and this should be entirely common (and if we
need a DSDT device, should use a common HID too).

Thanks,
Mark.

> 
> Yicong Yang (3):
>   clocksource/drivers/arm_arch_timer: Split the function of
>     __arch_timer_setup()
>   clocksource/drivers/arm_arch_timer: Extend and export
>     arch_timer_mem_register()
>   clocksource/drivers: Add HiSilicon system timer driver
> 
>  drivers/clocksource/Kconfig          |  10 +++
>  drivers/clocksource/Makefile         |   1 +
>  drivers/clocksource/arm_arch_timer.c | 123 +++++++++++++++------------
>  drivers/clocksource/timer-hisi-sys.c |  68 +++++++++++++++
>  include/clocksource/arm_arch_timer.h |   2 +
>  5 files changed, 148 insertions(+), 56 deletions(-)
>  create mode 100644 drivers/clocksource/timer-hisi-sys.c
> 
> -- 
> 2.24.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Marc Zyngier]

On Tue, 10 Oct 2023 13:30:30 +0100,
Yicong Yang <yangyicong@huawei.com> wrote:
> 
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> HiSilicon system timer is a memory mapped platform timer compatible with
> the arm's generic timer specification. The timer supports both SPI and
> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
> timer is fully compatible with the spec, it can reuse most codes of the
> arm_arch_timer driver. However since the arm_arch_timer driver only
> supports GTDT and SPI interrupt, this series support the HiSilicon system
> timer by:
> 
> - refactor some of the arm_arch_timer codes and export the function to
>   register a arch memory timer by other drivers
> - retrieve the IO memory and interrupt resource through DSDT in a separate
>   driver, then setup and register the clockevent device reuse the arm_arch_timer
>   function
> 
> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).

This strikes me as pretty odd. LPIs are, by definition, *edge*
triggered. The timer interrupt must be *level* triggered. So there
must be some bridge in the middle that is going to regenerate edges on
EOI, and that cannot be architectural.

What am I missing?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Yicong Yang]

On 2023/10/10 23:43, Mark Rutland wrote:
> Hi,
> 
> On Tue, Oct 10, 2023 at 08:30:30PM +0800, Yicong Yang wrote:
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> HiSilicon system timer is a memory mapped platform timer compatible with
>> the arm's generic timer specification. The timer supports both SPI and
>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>> timer is fully compatible with the spec, it can reuse most codes of the
>> arm_arch_timer driver. However since the arm_arch_timer driver only
>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>> timer by:
>>
>> - refactor some of the arm_arch_timer codes and export the function to
>>   register a arch memory timer by other drivers
>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>>   driver, then setup and register the clockevent device reuse the arm_arch_timer
>>   function
>>
>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
> 
> This seems like an oversight; there *should* be a generic way of describing
> this, and I've poked our BSA and ACPI architects to figure out how this is
> supposed to work. The lack of a way to do that seems like a major oversight and
> something that needs to be solved.
> 
> I'll try to get back to you shortly on that.
> 

Looking forward to the conclusion/solution.

> Regardless of that, I do not think this should be a separate driver, and I'm
> very much not keen on having vendor-specific companion drivers like this. Using
> LPIs isn't specific to HiSilicon, and this should be entirely common (and if we
> need a DSDT device, should use a common HID too).
> 

This is reasonable. Actually we're using most funtions of the arch timer driver, only
do the resource enumeration in the separate driver which is lack in the arch timer driver.
So if there's a common solution in the arch timer driver as well as a common HID we're
willing to use it.

Thanks.

> Thanks,
> Mark.
> 
>>
>> Yicong Yang (3):
>>   clocksource/drivers/arm_arch_timer: Split the function of
>>     __arch_timer_setup()
>>   clocksource/drivers/arm_arch_timer: Extend and export
>>     arch_timer_mem_register()
>>   clocksource/drivers: Add HiSilicon system timer driver
>>
>>  drivers/clocksource/Kconfig          |  10 +++
>>  drivers/clocksource/Makefile         |   1 +
>>  drivers/clocksource/arm_arch_timer.c | 123 +++++++++++++++------------
>>  drivers/clocksource/timer-hisi-sys.c |  68 +++++++++++++++
>>  include/clocksource/arm_arch_timer.h |   2 +
>>  5 files changed, 148 insertions(+), 56 deletions(-)
>>  create mode 100644 drivers/clocksource/timer-hisi-sys.c
>>
>> -- 
>> 2.24.0
>>
> 
> .
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Yicong Yang]

On 2023/10/11 0:36, Marc Zyngier wrote:
> On Tue, 10 Oct 2023 13:30:30 +0100,
> Yicong Yang <yangyicong@huawei.com> wrote:
>>
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> HiSilicon system timer is a memory mapped platform timer compatible with
>> the arm's generic timer specification. The timer supports both SPI and
>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>> timer is fully compatible with the spec, it can reuse most codes of the
>> arm_arch_timer driver. However since the arm_arch_timer driver only
>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>> timer by:
>>
>> - refactor some of the arm_arch_timer codes and export the function to
>>   register a arch memory timer by other drivers
>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>>   driver, then setup and register the clockevent device reuse the arm_arch_timer
>>   function
>>
>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
> 
> This strikes me as pretty odd. LPIs are, by definition, *edge*
> triggered. The timer interrupt must be *level* triggered. So there
> must be some bridge in the middle that is going to regenerate edges on
> EOI, and that cannot be architectural.
> 
> What am I missing?

In our case, if the timer is working on LPI mode, it's not directly connected
to the GIC. It'll be wired to hisi-mbigen irqchip which will send LPIs to the
GIC.

Thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Mark Rutland]

On Wed, Oct 11, 2023 at 10:10:11AM +0800, Yicong Yang wrote:
> On 2023/10/11 0:36, Marc Zyngier wrote:
> > On Tue, 10 Oct 2023 13:30:30 +0100,
> > Yicong Yang <yangyicong@huawei.com> wrote:
> >>
> >> From: Yicong Yang <yangyicong@hisilicon.com>
> >>
> >> HiSilicon system timer is a memory mapped platform timer compatible with
> >> the arm's generic timer specification. The timer supports both SPI and
> >> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
> >> timer is fully compatible with the spec, it can reuse most codes of the
> >> arm_arch_timer driver. However since the arm_arch_timer driver only
> >> supports GTDT and SPI interrupt, this series support the HiSilicon system
> >> timer by:
> >>
> >> - refactor some of the arm_arch_timer codes and export the function to
> >>   register a arch memory timer by other drivers
> >> - retrieve the IO memory and interrupt resource through DSDT in a separate
> >>   driver, then setup and register the clockevent device reuse the arm_arch_timer
> >>   function
> >>
> >> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
> > 
> > This strikes me as pretty odd. LPIs are, by definition, *edge*
> > triggered. The timer interrupt must be *level* triggered. So there
> > must be some bridge in the middle that is going to regenerate edges on
> > EOI, and that cannot be architectural.
> > 
> > What am I missing?
> 
> In our case, if the timer is working on LPI mode, it's not directly connected
> to the GIC. It'll be wired to hisi-mbigen irqchip which will send LPIs to the
> GIC.

In that case, the timerr itself isn't using an LPI: it's wired to a secondary
interrupt controller, and the secondary interrupt controller is using an LPI.

The BSA doesn't describe that as a permitted configuration.

I think there are two problems here:

(1) The BSA spec is wrong, and shouldn't say "or LPI" here as it simply doesn't
    make sense.

    I think this should be fixed by removing the "or LPI" wording form the BSA
    spec for this interrupt.

(2) This platform is not compatible with the BSA, and is not compatible with
    the existing ACPI bindings in the GTDT.

Do you actually need this wakeup timer?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Yicong Yang]

Hi Mark,

On 2023/10/11 18:38, Mark Rutland wrote:
> On Wed, Oct 11, 2023 at 10:10:11AM +0800, Yicong Yang wrote:
>> On 2023/10/11 0:36, Marc Zyngier wrote:
>>> On Tue, 10 Oct 2023 13:30:30 +0100,
>>> Yicong Yang <yangyicong@huawei.com> wrote:
>>>>
>>>> From: Yicong Yang <yangyicong@hisilicon.com>
>>>>
>>>> HiSilicon system timer is a memory mapped platform timer compatible with
>>>> the arm's generic timer specification. The timer supports both SPI and
>>>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>>>> timer is fully compatible with the spec, it can reuse most codes of the
>>>> arm_arch_timer driver. However since the arm_arch_timer driver only
>>>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>>>> timer by:
>>>>
>>>> - refactor some of the arm_arch_timer codes and export the function to
>>>>   register a arch memory timer by other drivers
>>>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>>>>   driver, then setup and register the clockevent device reuse the arm_arch_timer
>>>>   function
>>>>
>>>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
>>>
>>> This strikes me as pretty odd. LPIs are, by definition, *edge*
>>> triggered. The timer interrupt must be *level* triggered. So there
>>> must be some bridge in the middle that is going to regenerate edges on
>>> EOI, and that cannot be architectural.
>>>
>>> What am I missing?
>>
>> In our case, if the timer is working on LPI mode, it's not directly connected
>> to the GIC. It'll be wired to hisi-mbigen irqchip which will send LPIs to the
>> GIC.
> 
> In that case, the timerr itself isn't using an LPI: it's wired to a secondary
> interrupt controller, and the secondary interrupt controller is using an LPI.
> 
> The BSA doesn't describe that as a permitted configuration.
> 
> I think there are two problems here:
> 
> (1) The BSA spec is wrong, and shouldn't say "or LPI" here as it simply doesn't
>     make sense.
> 
>     I think this should be fixed by removing the "or LPI" wording form the BSA
>     spec for this interrupt.
> 
> (2) This platform is not compatible with the BSA, and is not compatible with
>     the existing ACPI bindings in the GTDT.
> 
> Do you actually need this wakeup timer?
> 

Thanks for the quick feedback!

So the LPI timer mentioned in the BSA spec is probably a mistake and our LPI
mode is not compatible to the BSA spec. Then I need to discuss with my team
and re-evaluate the solution for the LPI mode of the timer.

Thanks,
Yicong

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Yicong Yang]

Hi Mark,

On 2023/10/11 21:10, Yicong Yang wrote:
> Hi Mark,
> 
> On 2023/10/11 18:38, Mark Rutland wrote:
>> On Wed, Oct 11, 2023 at 10:10:11AM +0800, Yicong Yang wrote:
>>> On 2023/10/11 0:36, Marc Zyngier wrote:
>>>> On Tue, 10 Oct 2023 13:30:30 +0100,
>>>> Yicong Yang <yangyicong@huawei.com> wrote:
>>>>>
>>>>> From: Yicong Yang <yangyicong@hisilicon.com>
>>>>>
>>>>> HiSilicon system timer is a memory mapped platform timer compatible with
>>>>> the arm's generic timer specification. The timer supports both SPI and
>>>>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>>>>> timer is fully compatible with the spec, it can reuse most codes of the
>>>>> arm_arch_timer driver. However since the arm_arch_timer driver only
>>>>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>>>>> timer by:
>>>>>
>>>>> - refactor some of the arm_arch_timer codes and export the function to
>>>>>   register a arch memory timer by other drivers
>>>>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>>>>>   driver, then setup and register the clockevent device reuse the arm_arch_timer
>>>>>   function
>>>>>
>>>>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
>>>>
>>>> This strikes me as pretty odd. LPIs are, by definition, *edge*
>>>> triggered. The timer interrupt must be *level* triggered. So there
>>>> must be some bridge in the middle that is going to regenerate edges on
>>>> EOI, and that cannot be architectural.
>>>>
>>>> What am I missing?
>>>
>>> In our case, if the timer is working on LPI mode, it's not directly connected
>>> to the GIC. It'll be wired to hisi-mbigen irqchip which will send LPIs to the
>>> GIC.
>>
>> In that case, the timerr itself isn't using an LPI: it's wired to a secondary
>> interrupt controller, and the secondary interrupt controller is using an LPI.
>>
>> The BSA doesn't describe that as a permitted configuration.
>>
>> I think there are two problems here:
>>
>> (1) The BSA spec is wrong, and shouldn't say "or LPI" here as it simply doesn't
>>     make sense.
>>
>>     I think this should be fixed by removing the "or LPI" wording form the BSA
>>     spec for this interrupt.
>>
>> (2) This platform is not compatible with the BSA, and is not compatible with
>>     the existing ACPI bindings in the GTDT.
>>
>> Do you actually need this wakeup timer?
>>
> 
> Thanks for the quick feedback!
> 
> So the LPI timer mentioned in the BSA spec is probably a mistake and our LPI
> mode is not compatible to the BSA spec. Then I need to discuss with my team
> and re-evaluate the solution for the LPI mode of the timer.
> 

Since our system timer interrupt supports both SPI and non-SPI mode. For some cases
we want to leave SPI interrupt for secure system and the timer for non-secure
system (linux) will be wired to the hisi-mbigen. Just want to confirm if the
solution to support the non-SPI mode in this patchset is acceptable for our use case,
though it's not permitted by BSA spec?

Thanks,
Yicong


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel