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 <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?
> +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
