Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c46e5ebf-5293-5123-52d3-b3594c6e9244@loongson.cn>
Date:   Fri, 2 Dec 2022 12:36:53 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH v11 1/3] clocksource: loongson2_hpet: add hpet driver
 support
To:     Thomas Gleixner <tglx@linutronix.de>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Huacai Chen <chenhuacai@kernel.org>,
        WANG Xuerui <kernel@xen0n.name>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Jianmin Lv <lvjianmin@loongson.cn>,
        Yun Liu <liuyun@loongson.cn>,
        Yang Li <yang.lee@linux.alibaba.com>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        loongarch@lists.linux.dev, zhuyinbo@loongson.cn
References: <20221129030925.14074-1-zhuyinbo@loongson.cn>
 <87k03bs6pj.ffs@tglx>
From:   Yinbo Zhu <zhuyinbo@loongson.cn>
In-Reply-To: <87k03bs6pj.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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