lkml.org 
[lkml]   [2022]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v11 1/3] clocksource: loongson2_hpet: add hpet driver support
From

在 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.

\
 
 \ /
  Last update: 2022-12-02 05:37    [W:0.503 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site