Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v11 1/3] clocksource: loongson2_hpet: add hpet driver support | Date | Thu, 01 Dec 2022 12:29:12 +0100 |
| |
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
| |