Re: [PATCH v11 1/3] clocksource: loongson2_hpet: add hpet driver support

[Thomas Gleixner <tglx@linutronix.de>]

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@loongson.cn>
> + * 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