HPET (High Precision Event Timer) defines a new set of timers, which
are used by the operating system to schedule threads, interrupt the
kernel and interrupt the multimedia timer server. The operating
system can assign different timers to different applications. By
configuration, each timer can generate interrupt independently.
The Loongson-2 HPET module includes a main count and three comparators,
all of which are 32 bits wide. Among the three comparators, only
one comparator supports periodic interrupt, all three comparators
support non periodic interrupts.
Signed-off-by: Yinbo Zhu <[email protected]>
---
Change in v11:
1. Detach timer_probe to another patch.
2. Remove the #ifdef in timer_probe context.
Change in v10:
1. Replace "goto err" with "return -ENOMEM" if of_iomap fail.
2. This patch need rely on clock patch, which patchwork
link was "https://patchwork.kernel.org/project/linux-clk/list/?series=691497".
Change in v9:
1. Replace string "register" with "request" in hpet_request_irq.
2. Move the varible "ret" to the begining position in
loongson2_hpet_init and initialized it.
3. Adjust if judgement in clk_get_rate context, there was less
less indentation in the normal path.
4. This patch need rely on clock patch, which patchwork
link was "https://patchwork.kernel.org/project/linux-clk/list/?series=691497".
Change in v8:
1. Add all history change log information.
Change in v7:
1. Replace setup_irq with request_irq.
Change in v6:
1. Move comma to the end of the previous line if that comma at
the beginning of the line.
Change in v5:
1. Replace string loongson2 with Loongson-2 in commit message
and Kconfig file.
2. Replace string LOONGSON2 with LOONGSON-2 in MAINTAINERS.
3. Make include asm headers after all linux headers.
4. Add blank place before comma if comma when the comma is at
the beginning of the line.
Change in v4:
1. Use common clock framework ops to gain apb clock.
2. This patch need rely on clock patch, which patchwork
link was "https://patchwork.kernel.org/project/linux-clk/list/?series=688892".
Change in v3:
1. NO change, but other patch in this series of patches set
has changes
Change in v2:
1. NO change, but other patch in this series of patches set
has changes
MAINTAINERS | 6 +
drivers/clocksource/Kconfig | 9 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/loongson2_hpet.c | 334 +++++++++++++++++++++++++++
4 files changed, 350 insertions(+)
create mode 100644 drivers/clocksource/loongson2_hpet.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 0cdd1437c093..5d5967f55c36 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12029,6 +12029,12 @@ F: Documentation/devicetree/bindings/clock/loongson,ls2k-clk.yaml
F: drivers/clk/clk-loongson2.c
F: include/dt-bindings/clock/loongson,ls2k-clk.h
+LOONGSON-2 SOC SERIES HPET DRIVER
+M: Yinbo Zhu <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/clocksource/loongson2_hpet.c
+
LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
M: Sathya Prakash <[email protected]>
M: Sreekanth Reddy <[email protected]>
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4469e7f555e9..f114ee47e6f7 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -721,4 +721,13 @@ config GOLDFISH_TIMER
help
Support for the timer/counter of goldfish-rtc
+config LOONGSON2_HPET
+ bool "Loongson-2 High Precision Event Timer (HPET)"
+ select TIMER_PROBE
+ select TIMER_OF
+ help
+ This option enables Loongson-2 High Precision Event Timer
+ (HPET) module driver. It supports the oneshot, the periodic
+ modes and high resolution. It is used as a clocksource and
+ a clockevent.
endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 64ab547de97b..1a3abb770f11 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -88,3 +88,4 @@ obj-$(CONFIG_MICROCHIP_PIT64B) += timer-microchip-pit64b.o
obj-$(CONFIG_MSC313E_TIMER) += timer-msc313e.o
obj-$(CONFIG_GOLDFISH_TIMER) += timer-goldfish.o
obj-$(CONFIG_GXP_TIMER) += timer-gxp.o
+obj-$(CONFIG_LOONGSON2_HPET) += loongson2_hpet.o
diff --git a/drivers/clocksource/loongson2_hpet.c b/drivers/clocksource/loongson2_hpet.c
new file mode 100644
index 000000000000..9b828f9728ca
--- /dev/null
+++ b/drivers/clocksource/loongson2_hpet.c
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Author: Yinbo Zhu <[email protected]>
+ * Copyright (C) 2022-2023 Loongson Technology Corporation Limited
+ */
+
+#include <linux/init.h>
+#include <linux/percpu.h>
+#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/clk.h>
+#include <asm/time.h>
+
+/* HPET regs */
+#define HPET_CFG 0x010
+#define HPET_STATUS 0x020
+#define HPET_COUNTER 0x0f0
+#define HPET_T0_IRS 0x001
+#define HPET_T0_CFG 0x100
+#define HPET_T0_CMP 0x108
+#define HPET_CFG_ENABLE 0x001
+#define HPET_TN_LEVEL 0x0002
+#define HPET_TN_ENABLE 0x0004
+#define HPET_TN_PERIODIC 0x0008
+#define HPET_TN_SETVAL 0x0040
+#define HPET_TN_32BIT 0x0100
+
+#define HPET_MIN_CYCLES 16
+#define HPET_MIN_PROG_DELTA (HPET_MIN_CYCLES * 12)
+#define HPET_COMPARE_VAL ((hpet_freq + HZ / 2) / HZ)
+
+void __iomem *hpet_mmio_base;
+unsigned int hpet_freq;
+unsigned int hpet_t0_irq;
+unsigned int hpet_irq_flags;
+unsigned int hpet_t0_cfg;
+
+static DEFINE_SPINLOCK(hpet_lock);
+DEFINE_PER_CPU(struct clock_event_device, hpet_clockevent_device);
+
+static int hpet_read(int offset)
+{
+ return readl(hpet_mmio_base + offset);
+}
+
+static void hpet_write(int offset, int data)
+{
+ writel(data, hpet_mmio_base + offset);
+}
+
+static void hpet_start_counter(void)
+{
+ unsigned int cfg = hpet_read(HPET_CFG);
+
+ cfg |= HPET_CFG_ENABLE;
+ hpet_write(HPET_CFG, cfg);
+}
+
+static void hpet_stop_counter(void)
+{
+ unsigned int cfg = hpet_read(HPET_CFG);
+
+ cfg &= ~HPET_CFG_ENABLE;
+ hpet_write(HPET_CFG, cfg);
+}
+
+static void hpet_reset_counter(void)
+{
+ hpet_write(HPET_COUNTER, 0);
+ hpet_write(HPET_COUNTER + 4, 0);
+}
+
+static void hpet_restart_counter(void)
+{
+ hpet_stop_counter();
+ hpet_reset_counter();
+ hpet_start_counter();
+}
+
+static void hpet_enable_legacy_int(void)
+{
+ /* Do nothing on Loongson2 */
+}
+
+static int hpet_set_state_periodic(struct clock_event_device *evt)
+{
+ int cfg;
+
+ spin_lock(&hpet_lock);
+
+ pr_info("set clock event to periodic mode!\n");
+ /* stop counter */
+ hpet_stop_counter();
+ hpet_reset_counter();
+ hpet_write(HPET_T0_CMP, 0);
+
+ /* enables the timer0 to generate a periodic interrupt */
+ cfg = hpet_read(HPET_T0_CFG);
+ cfg &= ~HPET_TN_LEVEL;
+ cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
+ HPET_TN_32BIT | hpet_irq_flags;
+ hpet_write(HPET_T0_CFG, cfg);
+
+ /* set the comparator */
+ hpet_write(HPET_T0_CMP, HPET_COMPARE_VAL);
+ udelay(1);
+ hpet_write(HPET_T0_CMP, HPET_COMPARE_VAL);
+
+ /* start counter */
+ hpet_start_counter();
+
+ spin_unlock(&hpet_lock);
+ return 0;
+}
+
+static int hpet_set_state_shutdown(struct clock_event_device *evt)
+{
+ int cfg;
+
+ spin_lock(&hpet_lock);
+
+ cfg = hpet_read(HPET_T0_CFG);
+ cfg &= ~HPET_TN_ENABLE;
+ hpet_write(HPET_T0_CFG, cfg);
+
+ spin_unlock(&hpet_lock);
+ return 0;
+}
+
+static int hpet_set_state_oneshot(struct clock_event_device *evt)
+{
+ int cfg;
+
+ spin_lock(&hpet_lock);
+
+ pr_info("set clock event to one shot mode!\n");
+ cfg = hpet_read(HPET_T0_CFG);
+ /*
+ * set timer0 type
+ * 1 : periodic interrupt
+ * 0 : non-periodic(oneshot) interrupt
+ */
+ cfg &= ~HPET_TN_PERIODIC;
+ cfg |= HPET_TN_ENABLE | HPET_TN_32BIT |
+ hpet_irq_flags;
+ hpet_write(HPET_T0_CFG, cfg);
+
+ /* start counter */
+ hpet_start_counter();
+
+ spin_unlock(&hpet_lock);
+ return 0;
+}
+
+static int hpet_tick_resume(struct clock_event_device *evt)
+{
+ spin_lock(&hpet_lock);
+ hpet_enable_legacy_int();
+ spin_unlock(&hpet_lock);
+
+ return 0;
+}
+
+static int hpet_next_event(unsigned long delta,
+ struct clock_event_device *evt)
+{
+ u32 cnt;
+ s32 res;
+
+ cnt = hpet_read(HPET_COUNTER);
+ cnt += (u32) delta;
+ hpet_write(HPET_T0_CMP, cnt);
+
+ res = (s32)(cnt - hpet_read(HPET_COUNTER));
+
+ return res < HPET_MIN_CYCLES ? -ETIME : 0;
+}
+
+static irqreturn_t hpet_irq_handler(int irq, void *data)
+{
+ int is_irq;
+ struct clock_event_device *cd;
+ unsigned int cpu = smp_processor_id();
+
+ is_irq = hpet_read(HPET_STATUS);
+ if (is_irq & HPET_T0_IRS) {
+ /* clear the TIMER0 irq status register */
+ hpet_write(HPET_STATUS, HPET_T0_IRS);
+ cd = &per_cpu(hpet_clockevent_device, cpu);
+ cd->event_handler(cd);
+ return IRQ_HANDLED;
+ }
+ return IRQ_NONE;
+}
+
+/*
+ * HPET address assignation and irq setting should be done in bios.
+ * But, sometimes bios don't do this, we just setup here directly.
+ */
+static void hpet_setup(void)
+{
+ hpet_enable_legacy_int();
+}
+
+static int hpet_request_irq(struct clock_event_device *cd)
+{
+ unsigned long flags = IRQD_NO_BALANCING | IRQF_TIMER;
+
+ if (request_irq(cd->irq, hpet_irq_handler, flags, "hpet", NULL)) {
+ pr_err("Failed to request hpet interrupt\n");
+ return -1;
+ }
+
+ disable_irq(cd->irq);
+ irq_set_affinity(cd->irq, cd->cpumask);
+ enable_irq(cd->irq);
+
+ return 0;
+}
+
+static int __init loongson2_hpet_clockevent_init(void)
+{
+ unsigned int cpu = smp_processor_id();
+ struct clock_event_device *cd;
+
+ hpet_setup();
+
+ cd = &per_cpu(hpet_clockevent_device, cpu);
+ cd->name = "hpet";
+ cd->rating = 300;
+ cd->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
+ cd->set_state_shutdown = hpet_set_state_shutdown;
+ cd->set_state_periodic = hpet_set_state_periodic;
+ cd->set_state_oneshot = hpet_set_state_oneshot;
+ cd->tick_resume = hpet_tick_resume;
+ cd->set_next_event = hpet_next_event;
+ cd->irq = hpet_t0_irq;
+ cd->cpumask = cpumask_of(cpu);
+ clockevent_set_clock(cd, hpet_freq);
+ cd->max_delta_ns = clockevent_delta2ns(0x7fffffff, cd);
+ cd->max_delta_ticks = 0x7fffffff;
+ cd->min_delta_ns = clockevent_delta2ns(HPET_MIN_PROG_DELTA, cd);
+ cd->min_delta_ticks = HPET_MIN_PROG_DELTA;
+
+ clockevents_register_device(cd);
+ if (hpet_request_irq(cd))
+ return -1;
+
+ pr_info("hpet clock event device register\n");
+
+ return 0;
+}
+
+static u64 hpet_read_counter(struct clocksource *cs)
+{
+ return (u64)hpet_read(HPET_COUNTER);
+}
+
+static void hpet_suspend(struct clocksource *cs)
+{
+ hpet_t0_cfg = hpet_read(HPET_T0_CFG);
+}
+
+static void hpet_resume(struct clocksource *cs)
+{
+ hpet_write(HPET_T0_CFG, hpet_t0_cfg);
+ hpet_setup();
+ hpet_restart_counter();
+}
+
+struct clocksource csrc_hpet = {
+ .name = "hpet",
+ .rating = 300,
+ .read = hpet_read_counter,
+ .mask = CLOCKSOURCE_MASK(32),
+ /* oneshot mode work normal with this flag */
+ .flags = CLOCK_SOURCE_IS_CONTINUOUS,
+ .suspend = hpet_suspend,
+ .resume = hpet_resume,
+ .mult = 0,
+ .shift = 10,
+};
+
+static int __init loongson2_hpet_clocksource_init(void)
+{
+ csrc_hpet.mult = clocksource_hz2mult(hpet_freq, csrc_hpet.shift);
+
+ /* start counter */
+ hpet_start_counter();
+
+ return clocksource_register_hz(&csrc_hpet, hpet_freq);
+}
+
+static int __init loongson2_hpet_init(struct device_node *np)
+{
+ struct clk *clk;
+ int ret = -EINVAL;
+
+ hpet_mmio_base = of_iomap(np, 0);
+ if (!hpet_mmio_base) {
+ pr_err("hpet: unable to map loongson2 hpet registers\n");
+ return -ENOMEM;
+ }
+
+ hpet_t0_irq = irq_of_parse_and_map(np, 0);
+ if (hpet_t0_irq <= 0) {
+ pr_err("hpet: unable to get IRQ from DT, %d\n", hpet_t0_irq);
+ goto err;
+ }
+
+ clk = of_clk_get(np, 0);
+ if (IS_ERR(clk))
+ goto err;
+
+ hpet_freq = clk_get_rate(clk);
+ clk_put(clk);
+
+ hpet_irq_flags = HPET_TN_LEVEL;
+
+ loongson2_hpet_clocksource_init();
+
+ loongson2_hpet_clockevent_init();
+
+ return 0;
+
+err:
+ iounmap(hpet_mmio_base);
+ return ret;
+}
+
+TIMER_OF_DECLARE(loongson2_hpet, "loongson,ls2k-hpet", loongson2_hpet_init);
--
2.31.1
On Tue, Nov 29 2022 at 11:09, Yinbo Zhu wrote:
> HPET (High Precision Event Timer) defines a new set of timers, which
It's not really new. The HPET specification is 20 years old :)
> +++ b/drivers/clocksource/loongson2_hpet.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Author: Yinbo Zhu <[email protected]>
> + * Copyright (C) 2022-2023 Loongson Technology Corporation Limited
> + */
> +
> +#include <linux/init.h>
> +#include <linux/percpu.h>
> +#include <linux/delay.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/clk.h>
> +#include <asm/time.h>
> +
> +/* HPET regs */
> +#define HPET_CFG 0x010
> +#define HPET_STATUS 0x020
> +#define HPET_COUNTER 0x0f0
> +#define HPET_T0_IRS 0x001
> +#define HPET_T0_CFG 0x100
> +#define HPET_T0_CMP 0x108
> +#define HPET_CFG_ENABLE 0x001
> +#define HPET_TN_LEVEL 0x0002
> +#define HPET_TN_ENABLE 0x0004
> +#define HPET_TN_PERIODIC 0x0008
> +#define HPET_TN_SETVAL 0x0040
> +#define HPET_TN_32BIT 0x0100
So this is another copy of the defines which are already available in
x86 and mips. Seriously?
> +static DEFINE_SPINLOCK(hpet_lock);
This wants to be a raw spinlock if at all. But first you have to explain
the purpose of this lock.
> +DEFINE_PER_CPU(struct clock_event_device, hpet_clockevent_device);
Why needs this to be global and why is it needed at all?
This code does support exactly _ONE_ clock event device.
> +static int hpet_read(int offset)
> +{
> + return readl(hpet_mmio_base + offset);
> +}
> +
> +static void hpet_write(int offset, int data)
> +{
> + writel(data, hpet_mmio_base + offset);
> +}
> +
> +static void hpet_start_counter(void)
> +{
> + unsigned int cfg = hpet_read(HPET_CFG);
> +
> + cfg |= HPET_CFG_ENABLE;
> + hpet_write(HPET_CFG, cfg);
> +}
> +
> +static void hpet_stop_counter(void)
> +{
> + unsigned int cfg = hpet_read(HPET_CFG);
> +
> + cfg &= ~HPET_CFG_ENABLE;
> + hpet_write(HPET_CFG, cfg);
> +}
> +
> +static void hpet_reset_counter(void)
> +{
> + hpet_write(HPET_COUNTER, 0);
> + hpet_write(HPET_COUNTER + 4, 0);
> +}
> +
> +static void hpet_restart_counter(void)
> +{
> + hpet_stop_counter();
> + hpet_reset_counter();
> + hpet_start_counter();
> +}
This is also a copy of the x86 HPET code....
> +static void hpet_enable_legacy_int(void)
> +{
> + /* Do nothing on Loongson2 */
> +}
> +
> +static int hpet_set_state_periodic(struct clock_event_device *evt)
> +{
> + int cfg;
> +
> + spin_lock(&hpet_lock);
What's the purpose of this lock ?
> + pr_info("set clock event to periodic mode!\n");
> +
> + /* stop counter */
> + hpet_stop_counter();
> + hpet_reset_counter();
> + hpet_write(HPET_T0_CMP, 0);
> +
> + /* enables the timer0 to generate a periodic interrupt */
> + cfg = hpet_read(HPET_T0_CFG);
> + cfg &= ~HPET_TN_LEVEL;
> + cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
> + HPET_TN_32BIT | hpet_irq_flags;
> + hpet_write(HPET_T0_CFG, cfg);
> +
> + /* set the comparator */
> + hpet_write(HPET_T0_CMP, HPET_COMPARE_VAL);
> + udelay(1);
> + hpet_write(HPET_T0_CMP, HPET_COMPARE_VAL);
> +
> + /* start counter */
> + hpet_start_counter();
Pretty much the same code as hpet_clkevt_set_state_periodic()
> + spin_unlock(&hpet_lock);
> + return 0;
> +}
> +
> +static int hpet_set_state_shutdown(struct clock_event_device *evt)
> +{
> + int cfg;
> +
> + spin_lock(&hpet_lock);
> +
> + cfg = hpet_read(HPET_T0_CFG);
> + cfg &= ~HPET_TN_ENABLE;
> + hpet_write(HPET_T0_CFG, cfg);
> +
> + spin_unlock(&hpet_lock);
Another slightly different copy of the x86 code
> + return 0;
> +}
> +
> +static int hpet_set_state_oneshot(struct clock_event_device *evt)
> +{
> + int cfg;
> +
> + spin_lock(&hpet_lock);
> +
> + pr_info("set clock event to one shot mode!\n");
> + cfg = hpet_read(HPET_T0_CFG);
> + /*
> + * set timer0 type
> + * 1 : periodic interrupt
> + * 0 : non-periodic(oneshot) interrupt
> + */
> + cfg &= ~HPET_TN_PERIODIC;
> + cfg |= HPET_TN_ENABLE | HPET_TN_32BIT |
> + hpet_irq_flags;
> + hpet_write(HPET_T0_CFG, cfg);
Yet another copy.
> + /* start counter */
> + hpet_start_counter();
Why doe you need an explicit start here?
> + spin_unlock(&hpet_lock);
> + return 0;
> +}
> +
> +static int hpet_tick_resume(struct clock_event_device *evt)
> +{
> + spin_lock(&hpet_lock);
> + hpet_enable_legacy_int();
> + spin_unlock(&hpet_lock);
More copy and paste just to slap a spinlock on to it which has zero
value AFAICT.
> + return 0;
> +}
> +
> +static int hpet_next_event(unsigned long delta,
> + struct clock_event_device *evt)
> +{
> + u32 cnt;
> + s32 res;
> +
> + cnt = hpet_read(HPET_COUNTER);
> + cnt += (u32) delta;
> + hpet_write(HPET_T0_CMP, cnt);
> +
> + res = (s32)(cnt - hpet_read(HPET_COUNTER));
> +
> + return res < HPET_MIN_CYCLES ? -ETIME : 0;
Another copy of the x86 code except for omitting the big comment which
explains the logic.
Seriously, this is not how it works. Instead of copy & paste, we create
shared infrastructure and just keep the real architecture specific
pieces separate.
Thanks,
tglx
在 2022/12/1 19:29, Thomas Gleixner 写道:
> On Tue, Nov 29 2022 at 11:09, Yinbo Zhu wrote:
>> HPET (High Precision Event Timer) defines a new set of timers, which
> It's not really new. The HPET specification is 20 years old :)
I will change it.
thanks.
>
>> +++ b/drivers/clocksource/loongson2_hpet.c
>> @@ -0,0 +1,334 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Author: Yinbo Zhu <[email protected]>
>> + * Copyright (C) 2022-2023 Loongson Technology Corporation Limited
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/percpu.h>
>> +#include <linux/delay.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/clk.h>
>> +#include <asm/time.h>
>> +
>> +/* HPET regs */
>> +#define HPET_CFG 0x010
>> +#define HPET_STATUS 0x020
>> +#define HPET_COUNTER 0x0f0
>> +#define HPET_T0_IRS 0x001
>> +#define HPET_T0_CFG 0x100
>> +#define HPET_T0_CMP 0x108
>> +#define HPET_CFG_ENABLE 0x001
>> +#define HPET_TN_LEVEL 0x0002
>> +#define HPET_TN_ENABLE 0x0004
>> +#define HPET_TN_PERIODIC 0x0008
>> +#define HPET_TN_SETVAL 0x0040
>> +#define HPET_TN_32BIT 0x0100
> So this is another copy of the defines which are already available in
> x86 and mips. Seriously?
in fact, these definition was also record in LoongArch Loongson-2 SoC
datasheet.
>
>> +static DEFINE_SPINLOCK(hpet_lock);
> This wants to be a raw spinlock if at all. But first you have to explain
> the purpose of this lock.
>
>> +DEFINE_PER_CPU(struct clock_event_device, hpet_clockevent_device);
> Why needs this to be global and why is it needed at all?
>
> This code does support exactly _ONE_ clock event device.
This is consider that the one hardware clock_event_device is used for
multiple cpu cores,
and earch cpu cores has a device from its perspective, so add
DEFINE_SPINLOCK(hpet_lock)
and DEFINE_PER_CPU(struct clock_event_device, hpet_clockevent_device),
the use of locks described below is also this reason .
and I will use raw spinlock to replace spin lock.
>
>> +static int hpet_read(int offset)
>> +{
>> + return readl(hpet_mmio_base + offset);
>> +}
>> +
>> +static void hpet_write(int offset, int data)
>> +{
>> + writel(data, hpet_mmio_base + offset);
>> +}
>> +
>> +static void hpet_start_counter(void)
>> +{
>> + unsigned int cfg = hpet_read(HPET_CFG);
>> +
>> + cfg |= HPET_CFG_ENABLE;
>> + hpet_write(HPET_CFG, cfg);
>> +}
>> +
>> +static void hpet_stop_counter(void)
>> +{
>> + unsigned int cfg = hpet_read(HPET_CFG);
>> +
>> + cfg &= ~HPET_CFG_ENABLE;
>> + hpet_write(HPET_CFG, cfg);
>> +}
>> +
>> +static void hpet_reset_counter(void)
>> +{
>> + hpet_write(HPET_COUNTER, 0);
>> + hpet_write(HPET_COUNTER + 4, 0);
>> +}
>> +
>> +static void hpet_restart_counter(void)
>> +{
>> + hpet_stop_counter();
>> + hpet_reset_counter();
>> + hpet_start_counter();
>> +}
> This is also a copy of the x86 HPET code....
>
>> +static void hpet_enable_legacy_int(void)
>> +{
>> + /* Do nothing on Loongson2 */
>> +}
>> +
>> +static int hpet_set_state_periodic(struct clock_event_device *evt)
>> +{
>> + int cfg;
>> +
>> + spin_lock(&hpet_lock);
> What's the purpose of this lock ?
>
>> + pr_info("set clock event to periodic mode!\n");
>> +
>> + /* stop counter */
>> + hpet_stop_counter();
>> + hpet_reset_counter();
>> + hpet_write(HPET_T0_CMP, 0);
>> +
>> + /* enables the timer0 to generate a periodic interrupt */
>> + cfg = hpet_read(HPET_T0_CFG);
>> + cfg &= ~HPET_TN_LEVEL;
>> + cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
>> + HPET_TN_32BIT | hpet_irq_flags;
>> + hpet_write(HPET_T0_CFG, cfg);
>> +
>> + /* set the comparator */
>> + hpet_write(HPET_T0_CMP, HPET_COMPARE_VAL);
>> + udelay(1);
>> + hpet_write(HPET_T0_CMP, HPET_COMPARE_VAL);
>> +
>> + /* start counter */
>> + hpet_start_counter();
> Pretty much the same code as hpet_clkevt_set_state_periodic()
>
>> + spin_unlock(&hpet_lock);
>> + return 0;
>> +}
>> +
>> +static int hpet_set_state_shutdown(struct clock_event_device *evt)
>> +{
>> + int cfg;
>> +
>> + spin_lock(&hpet_lock);
>> +
>> + cfg = hpet_read(HPET_T0_CFG);
>> + cfg &= ~HPET_TN_ENABLE;
>> + hpet_write(HPET_T0_CFG, cfg);
>> +
>> + spin_unlock(&hpet_lock);
> Another slightly different copy of the x86 code
>
>> + return 0;
>> +}
>> +
>> +static int hpet_set_state_oneshot(struct clock_event_device *evt)
>> +{
>> + int cfg;
>> +
>> + spin_lock(&hpet_lock);
>> +
>> + pr_info("set clock event to one shot mode!\n");
>> + cfg = hpet_read(HPET_T0_CFG);
>> + /*
>> + * set timer0 type
>> + * 1 : periodic interrupt
>> + * 0 : non-periodic(oneshot) interrupt
>> + */
>> + cfg &= ~HPET_TN_PERIODIC;
>> + cfg |= HPET_TN_ENABLE | HPET_TN_32BIT |
>> + hpet_irq_flags;
>> + hpet_write(HPET_T0_CFG, cfg);
> Yet another copy.
>
>> + /* start counter */
>> + hpet_start_counter();
> Why doe you need an explicit start here?
if the hpet doesn't support period mode, the the hpet irq doesn't enable in
oneshot mode, so add it in here.
>
>> + spin_unlock(&hpet_lock);
>> + return 0;
>> +}
>> +
>> +static int hpet_tick_resume(struct clock_event_device *evt)
>> +{
>> + spin_lock(&hpet_lock);
>> + hpet_enable_legacy_int();
>> + spin_unlock(&hpet_lock);
> More copy and paste just to slap a spinlock on to it which has zero
> value AFAICT.
thank you for reminding me, I will remove it.
>> + return 0;
>> +}
>> +
>> +static int hpet_next_event(unsigned long delta,
>> + struct clock_event_device *evt)
>> +{
>> + u32 cnt;
>> + s32 res;
>> +
>> + cnt = hpet_read(HPET_COUNTER);
>> + cnt += (u32) delta;
>> + hpet_write(HPET_T0_CMP, cnt);
>> +
>> + res = (s32)(cnt - hpet_read(HPET_COUNTER));
>> +
>> + return res < HPET_MIN_CYCLES ? -ETIME : 0;
> Another copy of the x86 code except for omitting the big comment which
> explains the logic.
>
> Seriously, this is not how it works. Instead of copy & paste, we create
> shared infrastructure and just keep the real architecture specific
> pieces separate.
>
> Thanks,
>
> tglx
I don't find the shared infrastructure in LoongArch, I want to support
hpet for LoongArch
architecture Loongson-2 SoC series. the peripherals on the SoC are
generally
descriped by dts.
In addition, I havent' found any architecture releated differences for
hpet, at least Mips (loongson)
and LoongArch should be like this, in addtion to the hpet control base
address.
Loongson-2 SoC need to support dts, so I refer to the hpet driver of
Mips and add
hpet dts support for LoongArch architecture Loongson-2 SoC.
Thanks,
Yinbo.
On Fri, Dec 02 2022 at 12:36, Yinbo Zhu wrote:
> 在 2022/12/1 19:29, Thomas Gleixner 写道:
>>
>>> +static DEFINE_SPINLOCK(hpet_lock);
>> This wants to be a raw spinlock if at all. But first you have to explain
>> the purpose of this lock.
>>
>>> +DEFINE_PER_CPU(struct clock_event_device, hpet_clockevent_device);
>> Why needs this to be global and why is it needed at all?
>>
>> This code does support exactly _ONE_ clock event device.
>
> This is consider that the one hardware clock_event_device is used for
> multiple cpu cores,
>
> and earch cpu cores has a device from its perspective, so add
> DEFINE_SPINLOCK(hpet_lock)
>
> and DEFINE_PER_CPU(struct clock_event_device, hpet_clockevent_device),
>
> the use of locks described below is also this reason .
You cannot use _ONE_ clock event device as per CPU clock event device
for multiple CPUs. That simply cannot work ever.
There are two types of clockevent devices:
1) strictly per CPU, i.e. one distinct device per CPU
2) global broadcast device
The global broadcast device is used if there are less physical devices
than CPUs or to handle the cases where the per CPU device stops in
deep idle states.
For the case that there are less physical devices than CPUs, you have to
install dummy per CPU devices. Grep for CLOCK_EVT_FEAT_DUMMY.
The core code will use the broadcast device to provide timer interrupts
and it propagates them to the CPUs which are backed by a dummy per CPU
device via IPIs.
None of this needs a lock in the driver code unless the hardware is
really dumb designed and has a register shared with something else.
The serialization for all clockevent devices is completely provided by
the core code.
>> Seriously, this is not how it works. Instead of copy & paste, we create
>> shared infrastructure and just keep the real architecture specific
>> pieces separate.
>
> I don't find the shared infrastructure in LoongArch, I want to support
> hpet for LoongArch
Of course you can't find shared infrastructure because there is none.
That's the whole point. Instead of creating copies of code, you rework
the code so that the common parts can be shared between x86, longson and
loongarch. Then you have the architecture/platform specific pieces which
deal with the specific enumeration/initialization and use the shared
infrastructure.
Thanks,
tglx