Messages in this thread | | | Date | Fri, 2 Dec 2022 12:36:53 +0800 | Subject | Re: [PATCH v11 1/3] clocksource: loongson2_hpet: add hpet driver support | From | Yinbo Zhu <> |
| |
在 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@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?
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.
| |