Re: [PATCH v4 2/7] clocksource/drivers: Add EcoNet Timer HPT driver

[Caleb James DeLisle <cjd@cjdns.fr>]

On 05/05/2025 18:36, Daniel Lezcano wrote:
> On Wed, Apr 30, 2025 at 01:34:28PM +0000, Caleb James DeLisle wrote:
>> Introduce a clocksource driver for the so-called high-precision timer (HPT)
>> in the EcoNet EN751221 MIPS SoC.
> As a new driver, please document the timer (up - down ?, SPI/PPI, etc
> ...) that will help to understand the code more easily, especially the
> reg_* functions (purposes?).


Sure thing, I can elaborate the comment in the header of
timer-econet-en751221.c. Let me know if you'd like it described
somewhere else as well, such as the help of config ECONET_EN751221_TIMER.


>
>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
>> ---
>>   drivers/clocksource/Kconfig                 |   8 +
>>   drivers/clocksource/Makefile                |   1 +
>>   drivers/clocksource/timer-econet-en751221.c | 216 ++++++++++++++++++++
>>   3 files changed, 225 insertions(+)
>>   create mode 100644 drivers/clocksource/timer-econet-en751221.c
>>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 487c85259967..976afb0b2312 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -73,6 +73,14 @@ config DW_APB_TIMER_OF
>>   	select DW_APB_TIMER
>>   	select TIMER_OF
>>   
>> +config ECONET_EN751221_TIMER
>> +	bool "EcoNet EN751221 High Precision Timer" if COMPILE_TEST
>> +	depends on HAS_IOMEM
>> +	select CLKSRC_MMIO
>> +	select TIMER_OF
>> +	help
>> +	  Support for CPU timer found on EcoNet MIPS based SoCs.
>> +
>>   config FTTMR010_TIMER
>>   	bool "Faraday Technology timer driver" if COMPILE_TEST
>>   	depends on HAS_IOMEM
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index 43ef16a4efa6..d2998601eda5 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -17,6 +17,7 @@ obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
>>   obj-$(CONFIG_CLKSRC_MMIO)	+= mmio.o
>>   obj-$(CONFIG_DAVINCI_TIMER)	+= timer-davinci.o
>>   obj-$(CONFIG_DIGICOLOR_TIMER)	+= timer-digicolor.o
>> +obj-$(CONFIG_ECONET_EN751221_TIMER)	+= timer-econet-en751221.o
>>   obj-$(CONFIG_OMAP_DM_TIMER)	+= timer-ti-dm.o
>>   obj-$(CONFIG_OMAP_DM_SYSTIMER)	+= timer-ti-dm-systimer.o
>>   obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
>> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
>> new file mode 100644
>> index 000000000000..9cfeead09377
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-econet-en751221.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Timer present on EcoNet EN75xx MIPS based SoCs.
>> + *
>> + * Copyright (C) 2025 by Caleb James DeLisle <cjd@cjdns.fr>
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/sched_clock.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/clk.h>
>> +
>> +#define ECONET_BITS			32
>> +#define ECONET_MIN_DELTA		0x00001000
>> +#define ECONET_MAX_DELTA		GENMASK(ECONET_BITS - 2, 0)
>> +/* 34Kc hardware has 1 block and 1004Kc has 2. */
>> +#define ECONET_NUM_BLOCKS		DIV_ROUND_UP(NR_CPUS, 2)
>> +
>> +static struct {
>> +	void __iomem	*membase[ECONET_NUM_BLOCKS];
>> +	u32		freq_hz;
>> +} econet_timer __ro_after_init;
>> +
>> +static DEFINE_PER_CPU(struct clock_event_device, econet_timer_pcpu);
>> +
>> +/* Each memory block has 2 timers, the order of registers is:
>> + * CTL, CMR0, CNT0, CMR1, CNT1
>> + */
>> +static inline void __iomem *reg_ctl(u32 timer_n)
>> +{
>> +	return econet_timer.membase[timer_n >> 1];
>> +}
>> +
>> +static inline void __iomem *reg_compare(u32 timer_n)
>> +{
>> +	return econet_timer.membase[timer_n >> 1] + (timer_n & 1) * 0x08 + 0x04;
>> +}
>> +
>> +static inline void __iomem *reg_count(u32 timer_n)
>> +{
>> +	return econet_timer.membase[timer_n >> 1] + (timer_n & 1) * 0x08 + 0x08;
>> +}
>> +
>> +static inline u32 ctl_bit_enabled(u32 timer_n)
>> +{
>> +	return 1U << (timer_n & 1);
>> +}
>> +
>> +static inline u32 ctl_bit_pending(u32 timer_n)
>> +{
>> +	return 1U << ((timer_n & 1) + 16);
>> +}
>> +
>> +static bool cevt_is_pending(int cpu_id)
>> +{
>> +	return ioread32(reg_ctl(cpu_id)) & ctl_bit_pending(cpu_id);
>> +}
>> +
>> +static irqreturn_t cevt_interrupt(int irq, void *dev_id)
>> +{
>> +	struct clock_event_device *dev = this_cpu_ptr(&econet_timer_pcpu);
>> +	int cpu = cpumask_first(dev->cpumask);
>> +
>> +	if (!cevt_is_pending(cpu)) {
>> +		pr_debug("%s IRQ %d on CPU %d is not pending\n", __func__, irq, cpu);
>> +		return IRQ_NONE;
>> +	}
> How this can happen ? Can this interrupt be shared ?
To my knowledge it can't, but vendor code does it and I wasn't feeling
bold enough to gamble.
> The system will account this as a spurious interrupt, the debug
> message is not needed.
Good point, will remove.
>
>> +	iowrite32(ioread32(reg_count(cpu)), reg_compare(cpu));
>> +	dev->event_handler(dev);
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int cevt_set_next_event(ulong delta, struct clock_event_device *dev)
>> +{
>> +	u32 next;
>> +	int cpu;
>> +
>> +	cpu = cpumask_first(dev->cpumask);
>> +	next = ioread32(reg_count(cpu)) + delta;
>> +	iowrite32(next, reg_compare(cpu));
>> +
>> +	if ((s32)(next - ioread32(reg_count(cpu))) < ECONET_MIN_DELTA / 2)
>> +		return -ETIME;
>> +
>> +	return 0;
>> +}
>> +
>> +static int cevt_init_cpu(uint cpu)
>> +{
>> +	struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, cpu);
>> +	u32 reg;
>> +
>> +	pr_info("%s: Setting up clockevent for CPU %d\n", cd->name, cpu);
> That is more debug information
Fair enough, will change.
>> +	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
>> +	iowrite32(reg, reg_ctl(cpu));
>> +
>> +	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
>> +
>> +	/* Do this last because it synchronously configures the timer */
>> +	clockevents_config_and_register(
>> +		cd, econet_timer.freq_hz,
>> +		ECONET_MIN_DELTA, ECONET_MAX_DELTA);
>> +
>> +	return 0;
>> +}
>> +
>> +static u64 notrace sched_clock_read(void)
>> +{
>> +	/* Always read from clock zero no matter the CPU */
>> +	return (u64)ioread32(reg_count(0));
>> +}
>> +
>> +/* Init */
>> +
>> +static void __init cevt_dev_init(uint cpu)
>> +{
>> +	iowrite32(0, reg_count(cpu));
>> +	iowrite32(U32_MAX, reg_compare(cpu));
>> +}
>> +
>> +static int __init cevt_init(struct device_node *np)
>> +{
>> +	int i, irq, ret;
>> +
>> +	irq = irq_of_parse_and_map(np, 0);
>> +	if (irq <= 0) {
>> +		pr_err("%pOFn: irq_of_parse_and_map failed", np);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = request_percpu_irq(irq, cevt_interrupt, np->name, &econet_timer_pcpu);
>> +
>> +	if (ret < 0) {
>> +		pr_err("%pOFn: IRQ %d setup failed (%d)\n", np, irq, ret);
>> +		goto err_unmap_irq;
>> +	}
>> +
>> +	for_each_possible_cpu(i) {
>> +		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);
>> +
>> +		cd->rating		= 310,
>> +		cd->features		= CLOCK_EVT_FEAT_ONESHOT |
>> +					  CLOCK_EVT_FEAT_C3STOP |
>> +					  CLOCK_EVT_FEAT_PERCPU;
>> +		cd->set_next_event	= cevt_set_next_event;
>> +		cd->irq			= irq;
>> +		cd->cpumask		= cpumask_of(i);
>> +		cd->name		= np->name;
>> +
>> +		cevt_dev_init(i);
>> +	}
>> +
>> +	cpuhp_setup_state(CPUHP_AP_MIPS_GIC_TIMER_STARTING,
>> +			  "clockevents/en75/timer:starting",
>> +			  cevt_init_cpu, NULL);
> cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, ... ) ?

I see that Ingenic does this. This is the only timer so until it's up,
sleeping causes a hang. If sleeping is prior to CPUHP_AP_ONLINE_DYN is
considered a bug then this should be okay, but I'm not informed enough
to say whether that is the case so I'll follow your guidance here.


Thanks,

Caleb


>
>> +	return 0;
>> +
>> +err_unmap_irq:
>> +	irq_dispose_mapping(irq);
>> +	return ret;
>> +}
>> +
>> +static int __init timer_init(struct device_node *np)
>> +{
>> +	int num_blocks = DIV_ROUND_UP(num_possible_cpus(), 2);
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	clk = of_clk_get(np, 0);
>> +	if (IS_ERR(clk)) {
>> +		pr_err("%pOFn: Failed to get CPU clock from DT %ld\n", np, PTR_ERR(clk));
>> +		return PTR_ERR(clk);
>> +	}
>> +
>> +	econet_timer.freq_hz = clk_get_rate(clk);
>> +
>> +	for (int i = 0; i < num_blocks; i++) {
>> +		econet_timer.membase[i] = of_iomap(np, i);
>> +		if (!econet_timer.membase[i]) {
>> +			pr_err("%pOFn: failed to map register [%d]\n", np, i);
>> +			return -ENXIO;
>> +		}
>> +	}
>> +
>> +	/* For clocksource purposes always read clock zero, whatever the CPU */
>> +	ret = clocksource_mmio_init(reg_count(0), np->name,
>> +				    econet_timer.freq_hz, 301, ECONET_BITS,
>> +				    clocksource_mmio_readl_up);
>> +	if (ret) {
>> +		pr_err("%pOFn: clocksource_mmio_init failed: %d", np, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = cevt_init(np);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	sched_clock_register(sched_clock_read, ECONET_BITS,
>> +			     econet_timer.freq_hz);
>> +
>> +	pr_info("%pOFn: using %u.%03u MHz high precision timer\n", np,
>> +		econet_timer.freq_hz / 1000000,
>> +		(econet_timer.freq_hz / 1000) % 1000);
>> +
>> +	return 0;
>> +}
>> +
>> +TIMER_OF_DECLARE(econet_timer_hpt, "econet,en751221-timer", timer_init);
>> -- 
>> 2.39.5
>>