Re: [PATCH v11 1/3] clocksource: loongson2_hpet: add hpet driver support
From: Yinbo Zhu
Date: Thu Dec 01 2022 - 23:37:06 EST
在 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 <zhuyinbo@xxxxxxxxxxx>
+ * 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.