[v7,0/3] Add timer driver for StarFive JH7110 RISC-V SoC

This patch serises are to add timer driver for the StarFive JH7110
RISC-V SoC. The first patch adds documentation to describe device
tree bindings. The subsequent patch adds timer driver and support
JH7110 SoC. The last patch adds device node about timer in JH7110
dts.

This timer has four free-running 32 bit counters and runs in 24MHz
clock on StarFive JH7110 SoC. And each channel(counter) triggers
an interrupt when timeout. They support one-shot mode and 
continuous-run mode.

Changes since v6: 
- Rebased on 6.6-rc6.
- Used sizeof() instead of the numbers of characters about names.
- Added devm_add_action_or_reset() to release the resets and 
  clocksources in the case of remove or error in the probe.
- Added flags to check each clocksource is suceessfully registered and 
  used in the release function.
- Dropped the variable of irq in the jh7110_clkevt struct.
- Dropped the wrappers and used enum definitions and writel() calls
  directly.

v6: https://lore.kernel.org/all/20231012081015.33121-1-xingyu.wu@starfivetech.com/

Changes since v5: 
- Rebased on 6.6-rc5.
- Changed the number about characters of name.
- Made the clkevt->periodic to a local variable.
- Dropped the variables of device and base.
- Used clkevt->evt.irq directly and dropped the extra copy of irq.

V5: https://lore.kernel.org/all/20230907053742.250444-1-xingyu.wu@starfivetech.com/

Changes since v4: 
- Rebased on 6.5.
- Dropped the useless enum and used value directly when writing
  registers.
- Modified the description in Kconfig.
- Add the reviewed tag in patch 3.

v4: https://lore.kernel.org/all/20230814101603.166951-1-xingyu.wu@starfivetech.com/

Changes since v3: 
- Rebased on 6.5-rc6
- Dropped the useless enum names like 'JH7110_TIMER_CH_0'.
- Dropped the platform data about JH7110 and used the register offsets
  directly.
- Drroped the useless functions of clk_disable_unprepare().

v3: https://lore.kernel.org/all/20230627055313.252519-1-xingyu.wu@starfivetech.com/

Changes since v2: 
- Rebased on 6.4-rc7.
- Merged the header file into the c file.
- Renamed the functions from 'starfive_' to 'jh7110_'
- Used function 'clocksource_register_hz' instead of
  'clocksource_mmio_init'.

v2: https://lore.kernel.org/all/20230320135433.144832-1-xingyu.wu@starfivetech.com/

Changes since v1:
- Added description about timer and modified properties' description
  in dt-bindings.
- Dropped the 'interrupt-names' and 'clock-frequency' in dt-bindings.
- Renamed the functions and added 'starfive_'
- Modified that the driver probe by platform bus.

v1: https://lore.kernel.org/all/20221223094801.181315-1-xingyu.wu@starfivetech.com/

Xingyu Wu (3):
  dt-bindings: timer: Add timer for StarFive JH7110 SoC
  clocksource: Add JH7110 timer driver
  riscv: dts: jh7110: starfive: Add timer node

 .../bindings/timer/starfive,jh7110-timer.yaml |  96 +++++
 MAINTAINERS                                   |   7 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  20 +
 drivers/clocksource/Kconfig                   |  11 +
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/timer-jh7110.c            | 380 ++++++++++++++++++
 6 files changed, 515 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
 create mode 100644 drivers/clocksource/timer-jh7110.c

Xingyu Wu wrote:
> Add timer driver for the StarFive JH7110 SoC.
>
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  MAINTAINERS                        |   7 +
>  drivers/clocksource/Kconfig        |  11 +
>  drivers/clocksource/Makefile       |   1 +
>  drivers/clocksource/timer-jh7110.c | 380 +++++++++++++++++++++++++++++
>  4 files changed, 399 insertions(+)
>  create mode 100644 drivers/clocksource/timer-jh7110.c
>
 diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a7bd8bd80e9..91c09b399131 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20473,6 +20473,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>  F:	sound/soc/starfive/jh7110_tdm.c
>
> +STARFIVE JH7110 TIMER DRIVER
> +M:	Samin Guo <samin.guo@starfivetech.com>
> +M:	Xingyu Wu <xingyu.wu@starfivetech.com>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
> +F:	drivers/clocksource/timer-jh7110.c
> +
>  STARFIVE JH71X0 CLOCK DRIVERS
>  M:	Emil Renner Berthing <kernel@esmil.dk>
>  M:	Hal Feng <hal.feng@starfivetech.com>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 0ba0dc4ecf06..821abcc1e517 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -641,6 +641,17 @@ config RISCV_TIMER
>  	  is accessed via both the SBI and the rdcycle instruction.  This is
>  	  required for all RISC-V systems.
>
> +config STARFIVE_JH7110_TIMER
> +	bool "Timer for the STARFIVE JH7110 SoC"
> +	depends on ARCH_STARFIVE || COMPILE_TEST
> +	select TIMER_OF
> +	select CLKSRC_MMIO
> +	default ARCH_STARFIVE
> +	help
> +	  This enables the timer for StarFive JH7110 SoC. On RISC-V platform,
> +	  the system has started RISCV_TIMER, but you can also use this timer
> +	  which can provide four channels to do a lot more things on JH7110 SoC.
> +
>  config CLINT_TIMER
>  	bool "CLINT Timer for the RISC-V platform" if COMPILE_TEST
>  	depends on GENERIC_SCHED_CLOCK && RISCV
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 368c3461dab8..b66ac05ec086 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_INGENIC_TIMER)		+= ingenic-timer.o
>  obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
>  obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
> +obj-$(CONFIG_STARFIVE_JH7110_TIMER)	+= timer-jh7110.o
>  obj-$(CONFIG_CLINT_TIMER)		+= timer-clint.o
>  obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
>  obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
> diff --git a/drivers/clocksource/timer-jh7110.c b/drivers/clocksource/timer-jh7110.c
> new file mode 100644
> index 000000000000..71de29a3ec91
> --- /dev/null
> +++ b/drivers/clocksource/timer-jh7110.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Starfive JH7110 Timer driver
> + *
> + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
> + *
> + * Author:
> + * Xingyu Wu <xingyu.wu@starfivetech.com>
> + * Samin Guo <samin.guo@starfivetech.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/sched_clock.h>
> +
> +/* Bias: Ch0-0x0, Ch1-0x40, Ch2-0x80, and so on. */
> +#define JH7110_TIMER_CH_LEN		0x40
> +#define JH7110_TIMER_CH_BASE(x)		((x) * JH7110_TIMER_CH_LEN)
> +#define JH7110_TIMER_CH_MAX		4
> +
> +#define JH7110_CLOCK_SOURCE_RATING	200
> +#define JH7110_VALID_BITS		32
> +#define JH7110_DELAY_US			0
> +#define JH7110_TIMEOUT_US		10000
> +#define JH7110_CLOCKEVENT_RATING	300
> +#define JH7110_TIMER_MAX_TICKS		0xffffffff
> +#define JH7110_TIMER_MIN_TICKS		0xf
> +#define JH7110_TIMER_RELOAD_VALUE	0
> +
> +#define JH7110_TIMER_INT_STATUS		0x00 /* RO[0:4]: Interrupt Status for channel0~4 */
> +#define JH7110_TIMER_CTL		0x04 /* RW[0]: 0-continuous run, 1-single run */
> +#define JH7110_TIMER_LOAD		0x08 /* RW: load value to counter */
> +#define JH7110_TIMER_ENABLE		0x10 /* RW[0]: timer enable register */
> +#define JH7110_TIMER_RELOAD		0x14 /* RW: write 1 or 0 both reload counter */
> +#define JH7110_TIMER_VALUE		0x18 /* RO: timer value register */
> +#define JH7110_TIMER_INT_CLR		0x20 /* RW: timer interrupt clear register */
> +#define JH7110_TIMER_INT_MASK		0x24 /* RW[0]: timer interrupt mask register */
> +
> +#define JH7110_TIMER_INT_CLR_ENA	BIT(0)
> +#define JH7110_TIMER_INT_CLR_AVA_MASK	BIT(1)
> +
> +struct jh7110_clkevt {
> +	struct clock_event_device evt;
> +	struct clocksource cs;
> +	bool cs_is_valid;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	u32 rate;
> +	u32 reload_val;
> +	void __iomem *base;
> +	char name[sizeof("jh7110-timer.chX")];
> +};

Maybe sort this by alignment so you save a few bytes. Eg. something like

struct jh7110_clkevt {
	struct clock_event_device evt;
	struct clocksource cs;
	struct clk *clk;
	struct reset_control *rst;
	void __iomem *base;
	u32 rate;
	u32 reload_val;
	bool cs_is_valid;
	char name[sizeof("jh7110-timer.chX")];
};

> +struct jh7110_timer_priv {
> +	struct clk *pclk;
> +	struct reset_control *prst;
> +	struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX];
> +};
> +
> +/* 0:continuous-run mode, 1:single-run mode */
> +enum jh7110_timer_mode {
> +	JH7110_TIMER_MODE_CONTIN,
> +	JH7110_TIMER_MODE_SINGLE,
> +};
> +
> +/* Interrupt Mask, 0:Unmask, 1:Mask */
> +enum jh7110_timer_int_mask {
> +	JH7110_TIMER_INT_ENA,
> +	JH7110_TIMER_INT_DIS,
> +};
> +
> +enum jh7110_timer_enable {
> +	JH7110_TIMER_DIS,
> +	JH7110_TIMER_ENA,
> +};
> +
> +static inline struct jh7110_clkevt *to_jh7110_clkevt(struct clock_event_device *evt)
> +{
> +	return container_of(evt, struct jh7110_clkevt, evt);
> +}
> +
> +/*
> + * BIT(0): Read value represent channel int status.
> + * Write 1 to this bit to clear interrupt. Write 0 has no effects.
> + * BIT(1): "1" means that it is clearing interrupt. BIT(0) can not be written.
> + */
> +static inline int jh7110_timer_int_clear(struct jh7110_clkevt *clkevt)
> +{
> +	u32 value;
> +	int ret;
> +
> +	/* Waiting interrupt can be cleared */
> +	ret = readl_poll_timeout_atomic(clkevt->base + JH7110_TIMER_INT_CLR, value,
> +					!(value & JH7110_TIMER_INT_CLR_AVA_MASK),
> +					JH7110_DELAY_US, JH7110_TIMEOUT_US);
> +	if (!ret)
> +		writel(JH7110_TIMER_INT_CLR_ENA, clkevt->base + JH7110_TIMER_INT_CLR);

Below you're calling this function in the interrupt handler and the shutdown
callback, so is it really safe to always enable the interrupt on timeouts?

I think you just want the version below and then let the caller handle
re-enabling the interrupts on errors if needed. (While you're at it you can
remove the inline and let the compiler decide, which is does anyway.)

static int jh7110_timer_int_clear(struct jh7110_clkevt *clkevt)
{
	u32 value;

	/* Waiting interrupt can be cleared */
	return readl_poll_timeout_atomic(clkevt->base + JH7110_TIMER_INT_CLR, value,
					 !(value & JH7110_TIMER_INT_CLR_AVA_MASK),
					 JH7110_DELAY_US, JH7110_TIMEOUT_US);
}

> +
> +	return ret;
> +}
> +
> +static int jh7110_timer_start(struct jh7110_clkevt *clkevt)
> +{
> +	int ret;
> +
> +	/* Disable and clear interrupt first */
> +	writel(JH7110_TIMER_INT_DIS, clkevt->base + JH7110_TIMER_INT_MASK);
> +	ret = jh7110_timer_int_clear(clkevt);
> +	if (ret)
> +		return ret;
> +
> +	writel(JH7110_TIMER_INT_ENA, clkevt->base + JH7110_TIMER_INT_MASK);
> +	writel(JH7110_TIMER_ENA, clkevt->base + JH7110_TIMER_ENABLE);
> +
> +	return 0;
> +}
> +
> +static int jh7110_timer_shutdown(struct clock_event_device *evt)
> +{
> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
> +
> +	writel(JH7110_TIMER_DIS, clkevt->base + JH7110_TIMER_ENABLE);
> +	return jh7110_timer_int_clear(clkevt);
> +}
> +
> +static void jh7110_timer_suspend(struct clock_event_device *evt)
> +{
> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
> +
> +	clkevt->reload_val = readl(clkevt->base + JH7110_TIMER_LOAD);
> +	jh7110_timer_shutdown(evt);
> +}
> +
> +static void jh7110_timer_resume(struct clock_event_device *evt)
> +{
> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
> +
> +	writel(clkevt->reload_val, clkevt->base + JH7110_TIMER_LOAD);
> +	writel(JH7110_TIMER_RELOAD_VALUE, clkevt->base + JH7110_TIMER_RELOAD);
> +	jh7110_timer_start(clkevt);
> +}
> +
> +static int jh7110_timer_tick_resume(struct clock_event_device *evt)
> +{
> +	jh7110_timer_resume(evt);
> +
> +	return 0;
> +}

Here you probably want it the other way around, so you handle possible errors
from jh7110_timer_start() properly. Eg.

static int jh7110_timer_tick_resume(struct clock_event_device *evt)
{
	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);

	writel(clkevt->reload_val, clkevt->base + JH7110_TIMER_LOAD);
	writel(JH7110_TIMER_RELOAD_VALUE, clkevt->base + JH7110_TIMER_RELOAD);
	return jh7110_timer_start(clkevt);
}

static void jh7110_timer_resume(struct clock_event_device *evt)
{
	jh7110_timer_tick_resume(evt);
}

> +/* IRQ handler for the timer */
> +static irqreturn_t jh7110_timer_interrupt(int irq, void *priv)
> +{
> +	struct clock_event_device *evt = (struct clock_event_device *)priv;

This cast is unnecessary and also calling the variable priv makes you think
it's a struct jh7110_timer_priv, but the interrupt is registered with a
struct clock_event_device pointer.

> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);

But here you're immediately casting it to a struct jh7110_clkevt pointer. So
why not just register the interrupt with the struct jh7110_clkevt pointer, do

	struct jh7110_clkevt *clkevt = data;

> +
> +	if (jh7110_timer_int_clear(clkevt))
> +		return IRQ_NONE;
> +
> +	if (evt->event_handler)
> +		evt->event_handler(evt);

..and just use clkevt->evt.event_handler and &clkevt->evt here?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int jh7110_timer_set_periodic(struct clock_event_device *evt)
> +{
> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
> +	u32 periodic = DIV_ROUND_CLOSEST(clkevt->rate, HZ);
> +
> +	writel(JH7110_TIMER_MODE_CONTIN, clkevt->base + JH7110_TIMER_CTL);
> +	writel(periodic, clkevt->base + JH7110_TIMER_LOAD);
> +
> +	return jh7110_timer_start(clkevt);
> +}
> +
> +static int jh7110_timer_set_oneshot(struct clock_event_device *evt)
> +{
> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
> +
> +	writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL);
> +	writel(JH7110_TIMER_MAX_TICKS, clkevt->base + JH7110_TIMER_LOAD);
> +
> +	return jh7110_timer_start(clkevt);
> +}
> +
> +static int jh7110_timer_set_next_event(unsigned long next,
> +				       struct clock_event_device *evt)
> +{
> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
> +
> +	writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL);
> +	writel(next, clkevt->base + JH7110_TIMER_LOAD);
> +
> +	return jh7110_timer_start(clkevt);
> +}
> +
> +static void jh7110_set_clockevent(struct clock_event_device *evt)
> +{
> +	evt->features = CLOCK_EVT_FEAT_PERIODIC |
> +			CLOCK_EVT_FEAT_ONESHOT |
> +			CLOCK_EVT_FEAT_DYNIRQ;
> +	evt->set_state_shutdown = jh7110_timer_shutdown;
> +	evt->set_state_periodic = jh7110_timer_set_periodic;
> +	evt->set_state_oneshot = jh7110_timer_set_oneshot;
> +	evt->set_state_oneshot_stopped = jh7110_timer_shutdown;
> +	evt->tick_resume = jh7110_timer_tick_resume;
> +	evt->set_next_event = jh7110_timer_set_next_event;
> +	evt->suspend = jh7110_timer_suspend;
> +	evt->resume = jh7110_timer_resume;
> +	evt->rating = JH7110_CLOCKEVENT_RATING;
> +}
> +
> +static u64 jh7110_timer_clocksource_read(struct clocksource *cs)
> +{
> +	struct jh7110_clkevt *clkevt = container_of(cs, struct jh7110_clkevt, cs);
> +
> +	return (u64)readl(clkevt->base + JH7110_TIMER_VALUE);
> +}
> +
> +static int jh7110_clocksource_init(struct jh7110_clkevt *clkevt)
> +{
> +	int ret;
> +
> +	clkevt->cs.name = clkevt->name;
> +	clkevt->cs.rating = JH7110_CLOCK_SOURCE_RATING;
> +	clkevt->cs.read = jh7110_timer_clocksource_read;
> +	clkevt->cs.mask = CLOCKSOURCE_MASK(JH7110_VALID_BITS);
> +	clkevt->cs.flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +
> +	ret = clocksource_register_hz(&clkevt->cs, clkevt->rate);
> +	if (ret)
> +		return ret;
> +
> +	clkevt->cs_is_valid = true; /* clocksource register done */
> +	writel(JH7110_TIMER_MODE_CONTIN, clkevt->base + JH7110_TIMER_CTL);
> +	writel(JH7110_TIMER_MAX_TICKS, clkevt->base + JH7110_TIMER_LOAD);
> +
> +	return jh7110_timer_start(clkevt);
> +}
> +
> +static void jh7110_clockevents_register(struct jh7110_clkevt *clkevt)
> +{
> +	clkevt->rate = clk_get_rate(clkevt->clk);
> +
> +	jh7110_set_clockevent(&clkevt->evt);
> +	clkevt->evt.name = clkevt->name;
> +	clkevt->evt.cpumask = cpu_possible_mask;
> +
> +	clockevents_config_and_register(&clkevt->evt, clkevt->rate,
> +					JH7110_TIMER_MIN_TICKS, JH7110_TIMER_MAX_TICKS);
> +}
> +
> +static void jh7110_timer_release(void *data)
> +{
> +	struct jh7110_timer_priv *priv = data;
> +	int i;
> +
> +	for (i = 0; i < JH7110_TIMER_CH_MAX; i++) {
> +		/* Disable each channel of timer */
> +		if (priv->clkevt[i].base)
> +			writel(JH7110_TIMER_DIS, priv->clkevt[i].base + JH7110_TIMER_ENABLE);
> +
> +		/* Avoid no initialization in the loop of the probe */
> +		if (!IS_ERR_OR_NULL(priv->clkevt[i].rst))
> +			reset_control_assert(priv->clkevt[i].rst);
> +
> +		if (priv->clkevt[i].cs_is_valid)
> +			clocksource_unregister(&priv->clkevt[i].cs);
> +	}
> +
> +	reset_control_assert(priv->prst);
> +}
> +
> +static int jh7110_timer_probe(struct platform_device *pdev)
> +{
> +	struct jh7110_timer_priv *priv;
> +	struct jh7110_clkevt *clkevt;
> +	char name[sizeof("chX")];
> +	int ch;
> +	int ret;
> +	void __iomem *base;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(base),
> +				     "failed to map registers\n");
> +
> +	priv->prst = devm_reset_control_get_exclusive(&pdev->dev, "apb");
> +	if (IS_ERR(priv->prst))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->prst),
> +				     "failed to get apb reset\n");
> +
> +	priv->pclk = devm_clk_get_enabled(&pdev->dev, "apb");
> +	if (IS_ERR(priv->pclk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->pclk),
> +				     "failed to get & enable apb clock\n");

priv->pclk is set here, but never read anywhere. Just remove it from the struct
and use a local variable here.

> +
> +	ret = reset_control_deassert(priv->prst);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to deassert apb reset\n");
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, jh7110_timer_release, priv);
> +	if (ret)
> +		return ret;
> +
> +	for (ch = 0; ch < JH7110_TIMER_CH_MAX; ch++) {
> +		clkevt = &priv->clkevt[ch];
> +		snprintf(name, sizeof(name), "ch%d", ch);
> +
> +		clkevt->base = base + JH7110_TIMER_CH_BASE(ch);
> +		/* Ensure timer is disabled */
> +		writel(JH7110_TIMER_DIS, clkevt->base + JH7110_TIMER_ENABLE);
> +
> +		clkevt->rst = devm_reset_control_get_exclusive(&pdev->dev, name);
> +		if (IS_ERR(clkevt->rst))
> +			return PTR_ERR(clkevt->rst);
> +
> +		clkevt->clk = devm_clk_get_enabled(&pdev->dev, name);
> +		if (IS_ERR(clkevt->clk))
> +			return PTR_ERR(clkevt->clk);
> +
> +		ret = reset_control_deassert(clkevt->rst);
> +		if (ret)
> +			return ret;
> +
> +		clkevt->evt.irq = platform_get_irq(pdev, ch);
> +		if (clkevt->evt.irq < 0)
> +			return clkevt->evt.irq;
> +
> +		snprintf(clkevt->name, sizeof(clkevt->name), "jh7110-timer.ch%d", ch);
> +		jh7110_clockevents_register(clkevt);
> +
> +		ret = devm_request_irq(&pdev->dev, clkevt->evt.irq, jh7110_timer_interrupt,
> +				       IRQF_TIMER | IRQF_IRQPOLL,
> +				       clkevt->name, &clkevt->evt);
> +		if (ret)
> +			return ret;
> +
> +		ret = jh7110_clocksource_init(clkevt);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id jh7110_timer_match[] = {
> +	{ .compatible = "starfive,jh7110-timer", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, jh7110_timer_match);
> +
> +static struct platform_driver jh7110_timer_driver = {
> +	.probe = jh7110_timer_probe,
> +	.driver = {
> +		.name = "jh7110-timer",
> +		.of_match_table = jh7110_timer_match,
> +	},
> +};
> +module_platform_driver(jh7110_timer_driver);
> +
> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
> +MODULE_DESCRIPTION("StarFive JH7110 timer driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1

Hi Xingyu,


On 19/10/2023 07:35, Xingyu Wu wrote:
> Add timer driver for the StarFive JH7110 SoC.

As it is a new timer, please add a proper nice description explaining 
the timer hardware, thanks.

> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>   MAINTAINERS                        |   7 +
>   drivers/clocksource/Kconfig        |  11 +
>   drivers/clocksource/Makefile       |   1 +
>   drivers/clocksource/timer-jh7110.c | 380 +++++++++++++++++++++++++++++
>   4 files changed, 399 insertions(+)
>   create mode 100644 drivers/clocksource/timer-jh7110.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a7bd8bd80e9..91c09b399131 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20473,6 +20473,13 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>   F:	sound/soc/starfive/jh7110_tdm.c
>   
> +STARFIVE JH7110 TIMER DRIVER
> +M:	Samin Guo <samin.guo@starfivetech.com>
> +M:	Xingyu Wu <xingyu.wu@starfivetech.com>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
> +F:	drivers/clocksource/timer-jh7110.c
> +
>   STARFIVE JH71X0 CLOCK DRIVERS
>   M:	Emil Renner Berthing <kernel@esmil.dk>
>   M:	Hal Feng <hal.feng@starfivetech.com>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 0ba0dc4ecf06..821abcc1e517 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -641,6 +641,17 @@ config RISCV_TIMER
>   	  is accessed via both the SBI and the rdcycle instruction.  This is
>   	  required for all RISC-V systems.
>   
> +config STARFIVE_JH7110_TIMER
> +	bool "Timer for the STARFIVE JH7110 SoC"
> +	depends on ARCH_STARFIVE || COMPILE_TEST

You may want to use ARCH_STARFIVE only if the platform can make this 
timer optional. Otherwise, set the option from the platform Kconfig and 
put the bool "bla bla" if COMPILE_TEST

> +	select TIMER_OF
> +	select CLKSRC_MMIO
> +	default ARCH_STARFIVE

no "default"

> +	help
> +	  This enables the timer for StarFive JH7110 SoC. On RISC-V platform,
> +	  the system has started RISCV_TIMER, but you can also use this timer
> +	  which can provide four channels to do a lot more things on JH7110 SoC.
> +
>   config CLINT_TIMER
>   	bool "CLINT Timer for the RISC-V platform" if COMPILE_TEST
>   	depends on GENERIC_SCHED_CLOCK && RISCV
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 368c3461dab8..b66ac05ec086 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_INGENIC_TIMER)		+= ingenic-timer.o
>   obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>   obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
>   obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
> +obj-$(CONFIG_STARFIVE_JH7110_TIMER)	+= timer-jh7110.o
>   obj-$(CONFIG_CLINT_TIMER)		+= timer-clint.o
>   obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
>   obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
> diff --git a/drivers/clocksource/timer-jh7110.c b/drivers/clocksource/timer-jh7110.c
> new file mode 100644
> index 000000000000..71de29a3ec91
> --- /dev/null
> +++ b/drivers/clocksource/timer-jh7110.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Starfive JH7110 Timer driver
> + *
> + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
> + *
> + * Author:
> + * Xingyu Wu <xingyu.wu@starfivetech.com>
> + * Samin Guo <samin.guo@starfivetech.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/sched_clock.h>

Please double check the headers and remove the pointless ones.


> +/* Bias: Ch0-0x0, Ch1-0x40, Ch2-0x80, and so on. */
> +#define JH7110_TIMER_CH_LEN		0x40
> +#define JH7110_TIMER_CH_BASE(x)		((x) * JH7110_TIMER_CH_LEN)
> +#define JH7110_TIMER_CH_MAX		4
> +
> +#define JH7110_CLOCK_SOURCE_RATING	200
> +#define JH7110_VALID_BITS		32
> +#define JH7110_DELAY_US			0
> +#define JH7110_TIMEOUT_US		10000
> +#define JH7110_CLOCKEVENT_RATING	300
> +#define JH7110_TIMER_MAX_TICKS		0xffffffff
> +#define JH7110_TIMER_MIN_TICKS		0xf
> +#define JH7110_TIMER_RELOAD_VALUE	0
> +
> +#define JH7110_TIMER_INT_STATUS		0x00 /* RO[0:4]: Interrupt Status for channel0~4 */
> +#define JH7110_TIMER_CTL		0x04 /* RW[0]: 0-continuous run, 1-single run */
> +#define JH7110_TIMER_LOAD		0x08 /* RW: load value to counter */
> +#define JH7110_TIMER_ENABLE		0x10 /* RW[0]: timer enable register */
> +#define JH7110_TIMER_RELOAD		0x14 /* RW: write 1 or 0 both reload counter */
> +#define JH7110_TIMER_VALUE		0x18 /* RO: timer value register */
> +#define JH7110_TIMER_INT_CLR		0x20 /* RW: timer interrupt clear register */
> +#define JH7110_TIMER_INT_MASK		0x24 /* RW[0]: timer interrupt mask register */
> +
> +#define JH7110_TIMER_INT_CLR_ENA	BIT(0)
> +#define JH7110_TIMER_INT_CLR_AVA_MASK	BIT(1)
> +
> +struct jh7110_clkevt {
> +	struct clock_event_device evt;
> +	struct clocksource cs;
> +	bool cs_is_valid;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	u32 rate;
> +	u32 reload_val;
> +	void __iomem *base;
> +	char name[sizeof("jh7110-timer.chX")];
> +};
> +
> +struct jh7110_timer_priv {
> +	struct clk *pclk;
> +	struct reset_control *prst;
> +	struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX];

Why do you need several clock events and clock sources ?

[ ... ]

On 2023/10/24 21:13, Emil Renner Berthing wrote:
> Xingyu Wu wrote:
>> Add timer driver for the StarFive JH7110 SoC.
>>
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>  MAINTAINERS                        |   7 +
>>  drivers/clocksource/Kconfig        |  11 +
>>  drivers/clocksource/Makefile       |   1 +
>>  drivers/clocksource/timer-jh7110.c | 380 +++++++++++++++++++++++++++++
>>  4 files changed, 399 insertions(+)
>>  create mode 100644 drivers/clocksource/timer-jh7110.c
>>
>  diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7a7bd8bd80e9..91c09b399131 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20473,6 +20473,13 @@ S:	Maintained
>>  F:	Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>>  F:	sound/soc/starfive/jh7110_tdm.c
>>
>> +STARFIVE JH7110 TIMER DRIVER
>> +M:	Samin Guo <samin.guo@starfivetech.com>
>> +M:	Xingyu Wu <xingyu.wu@starfivetech.com>
>> +S:	Supported
>> +F:	Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
>> +F:	drivers/clocksource/timer-jh7110.c
>> +
>>  STARFIVE JH71X0 CLOCK DRIVERS
>>  M:	Emil Renner Berthing <kernel@esmil.dk>
>>  M:	Hal Feng <hal.feng@starfivetech.com>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 0ba0dc4ecf06..821abcc1e517 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -641,6 +641,17 @@ config RISCV_TIMER
>>  	  is accessed via both the SBI and the rdcycle instruction.  This is
>>  	  required for all RISC-V systems.
>>
>> +config STARFIVE_JH7110_TIMER
>> +	bool "Timer for the STARFIVE JH7110 SoC"
>> +	depends on ARCH_STARFIVE || COMPILE_TEST
>> +	select TIMER_OF
>> +	select CLKSRC_MMIO
>> +	default ARCH_STARFIVE
>> +	help
>> +	  This enables the timer for StarFive JH7110 SoC. On RISC-V platform,
>> +	  the system has started RISCV_TIMER, but you can also use this timer
>> +	  which can provide four channels to do a lot more things on JH7110 SoC.
>> +
>>  config CLINT_TIMER
>>  	bool "CLINT Timer for the RISC-V platform" if COMPILE_TEST
>>  	depends on GENERIC_SCHED_CLOCK && RISCV
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index 368c3461dab8..b66ac05ec086 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -80,6 +80,7 @@ obj-$(CONFIG_INGENIC_TIMER)		+= ingenic-timer.o
>>  obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>>  obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
>>  obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
>> +obj-$(CONFIG_STARFIVE_JH7110_TIMER)	+= timer-jh7110.o
>>  obj-$(CONFIG_CLINT_TIMER)		+= timer-clint.o
>>  obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
>>  obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
>> diff --git a/drivers/clocksource/timer-jh7110.c b/drivers/clocksource/timer-jh7110.c
>> new file mode 100644
>> index 000000000000..71de29a3ec91
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-jh7110.c
>> @@ -0,0 +1,380 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Starfive JH7110 Timer driver
>> + *
>> + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
>> + *
>> + * Author:
>> + * Xingyu Wu <xingyu.wu@starfivetech.com>
>> + * Samin Guo <samin.guo@starfivetech.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/irq.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/sched_clock.h>
>> +
>> +/* Bias: Ch0-0x0, Ch1-0x40, Ch2-0x80, and so on. */
>> +#define JH7110_TIMER_CH_LEN		0x40
>> +#define JH7110_TIMER_CH_BASE(x)		((x) * JH7110_TIMER_CH_LEN)
>> +#define JH7110_TIMER_CH_MAX		4
>> +
>> +#define JH7110_CLOCK_SOURCE_RATING	200
>> +#define JH7110_VALID_BITS		32
>> +#define JH7110_DELAY_US			0
>> +#define JH7110_TIMEOUT_US		10000
>> +#define JH7110_CLOCKEVENT_RATING	300
>> +#define JH7110_TIMER_MAX_TICKS		0xffffffff
>> +#define JH7110_TIMER_MIN_TICKS		0xf
>> +#define JH7110_TIMER_RELOAD_VALUE	0
>> +
>> +#define JH7110_TIMER_INT_STATUS		0x00 /* RO[0:4]: Interrupt Status for channel0~4 */
>> +#define JH7110_TIMER_CTL		0x04 /* RW[0]: 0-continuous run, 1-single run */
>> +#define JH7110_TIMER_LOAD		0x08 /* RW: load value to counter */
>> +#define JH7110_TIMER_ENABLE		0x10 /* RW[0]: timer enable register */
>> +#define JH7110_TIMER_RELOAD		0x14 /* RW: write 1 or 0 both reload counter */
>> +#define JH7110_TIMER_VALUE		0x18 /* RO: timer value register */
>> +#define JH7110_TIMER_INT_CLR		0x20 /* RW: timer interrupt clear register */
>> +#define JH7110_TIMER_INT_MASK		0x24 /* RW[0]: timer interrupt mask register */
>> +
>> +#define JH7110_TIMER_INT_CLR_ENA	BIT(0)
>> +#define JH7110_TIMER_INT_CLR_AVA_MASK	BIT(1)
>> +
>> +struct jh7110_clkevt {
>> +	struct clock_event_device evt;
>> +	struct clocksource cs;
>> +	bool cs_is_valid;
>> +	struct clk *clk;
>> +	struct reset_control *rst;
>> +	u32 rate;
>> +	u32 reload_val;
>> +	void __iomem *base;
>> +	char name[sizeof("jh7110-timer.chX")];
>> +};
> 
> Maybe sort this by alignment so you save a few bytes. Eg. something like
> 
> struct jh7110_clkevt {
> 	struct clock_event_device evt;
> 	struct clocksource cs;
> 	struct clk *clk;
> 	struct reset_control *rst;
> 	void __iomem *base;
> 	u32 rate;
> 	u32 reload_val;
> 	bool cs_is_valid;
> 	char name[sizeof("jh7110-timer.chX")];
> };

Will fix.

> 
>> +struct jh7110_timer_priv {
>> +	struct clk *pclk;
>> +	struct reset_control *prst;
>> +	struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX];
>> +};
>> +
>> +/* 0:continuous-run mode, 1:single-run mode */
>> +enum jh7110_timer_mode {
>> +	JH7110_TIMER_MODE_CONTIN,
>> +	JH7110_TIMER_MODE_SINGLE,
>> +};
>> +
>> +/* Interrupt Mask, 0:Unmask, 1:Mask */
>> +enum jh7110_timer_int_mask {
>> +	JH7110_TIMER_INT_ENA,
>> +	JH7110_TIMER_INT_DIS,
>> +};
>> +
>> +enum jh7110_timer_enable {
>> +	JH7110_TIMER_DIS,
>> +	JH7110_TIMER_ENA,
>> +};
>> +
>> +static inline struct jh7110_clkevt *to_jh7110_clkevt(struct clock_event_device *evt)
>> +{
>> +	return container_of(evt, struct jh7110_clkevt, evt);
>> +}
>> +
>> +/*
>> + * BIT(0): Read value represent channel int status.
>> + * Write 1 to this bit to clear interrupt. Write 0 has no effects.
>> + * BIT(1): "1" means that it is clearing interrupt. BIT(0) can not be written.
>> + */
>> +static inline int jh7110_timer_int_clear(struct jh7110_clkevt *clkevt)
>> +{
>> +	u32 value;
>> +	int ret;
>> +
>> +	/* Waiting interrupt can be cleared */
>> +	ret = readl_poll_timeout_atomic(clkevt->base + JH7110_TIMER_INT_CLR, value,
>> +					!(value & JH7110_TIMER_INT_CLR_AVA_MASK),
>> +					JH7110_DELAY_US, JH7110_TIMEOUT_US);
>> +	if (!ret)
>> +		writel(JH7110_TIMER_INT_CLR_ENA, clkevt->base + JH7110_TIMER_INT_CLR);
> 
> Below you're calling this function in the interrupt handler and the shutdown
> callback, so is it really safe to always enable the interrupt on timeouts?
> 
> I think you just want the version below and then let the caller handle
> re-enabling the interrupts on errors if needed. (While you're at it you can
> remove the inline and let the compiler decide, which is does anyway.)
> 
> static int jh7110_timer_int_clear(struct jh7110_clkevt *clkevt)
> {
> 	u32 value;
> 
> 	/* Waiting interrupt can be cleared */
> 	return readl_poll_timeout_atomic(clkevt->base + JH7110_TIMER_INT_CLR, value,
> 					 !(value & JH7110_TIMER_INT_CLR_AVA_MASK),
> 					 JH7110_DELAY_US, JH7110_TIMEOUT_US);
> }
> 

So I drop this function and use the writel() of cleaning the interrupt directly in the interrupt handler.
And drop the inline.

>> +
>> +	return ret;
>> +}
>> +
>> +static int jh7110_timer_start(struct jh7110_clkevt *clkevt)
>> +{
>> +	int ret;
>> +
>> +	/* Disable and clear interrupt first */
>> +	writel(JH7110_TIMER_INT_DIS, clkevt->base + JH7110_TIMER_INT_MASK);
>> +	ret = jh7110_timer_int_clear(clkevt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	writel(JH7110_TIMER_INT_ENA, clkevt->base + JH7110_TIMER_INT_MASK);
>> +	writel(JH7110_TIMER_ENA, clkevt->base + JH7110_TIMER_ENABLE);
>> +
>> +	return 0;
>> +}
>> +
>> +static int jh7110_timer_shutdown(struct clock_event_device *evt)
>> +{
>> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
>> +
>> +	writel(JH7110_TIMER_DIS, clkevt->base + JH7110_TIMER_ENABLE);
>> +	return jh7110_timer_int_clear(clkevt);
>> +}
>> +
>> +static void jh7110_timer_suspend(struct clock_event_device *evt)
>> +{
>> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
>> +
>> +	clkevt->reload_val = readl(clkevt->base + JH7110_TIMER_LOAD);
>> +	jh7110_timer_shutdown(evt);
>> +}
>> +
>> +static void jh7110_timer_resume(struct clock_event_device *evt)
>> +{
>> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
>> +
>> +	writel(clkevt->reload_val, clkevt->base + JH7110_TIMER_LOAD);
>> +	writel(JH7110_TIMER_RELOAD_VALUE, clkevt->base + JH7110_TIMER_RELOAD);
>> +	jh7110_timer_start(clkevt);
>> +}
>> +
>> +static int jh7110_timer_tick_resume(struct clock_event_device *evt)
>> +{
>> +	jh7110_timer_resume(evt);
>> +
>> +	return 0;
>> +}
> 
> Here you probably want it the other way around, so you handle possible errors
> from jh7110_timer_start() properly. Eg.
> 
> static int jh7110_timer_tick_resume(struct clock_event_device *evt)
> {
> 	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
> 
> 	writel(clkevt->reload_val, clkevt->base + JH7110_TIMER_LOAD);
> 	writel(JH7110_TIMER_RELOAD_VALUE, clkevt->base + JH7110_TIMER_RELOAD);
> 	return jh7110_timer_start(clkevt);
> }
> 
> static void jh7110_timer_resume(struct clock_event_device *evt)
> {
> 	jh7110_timer_tick_resume(evt);
> }
> 

OK, so this makes the best use of the return value of jh7110_timer_start().
Will fix.

>> +/* IRQ handler for the timer */
>> +static irqreturn_t jh7110_timer_interrupt(int irq, void *priv)
>> +{
>> +	struct clock_event_device *evt = (struct clock_event_device *)priv;
> 
> This cast is unnecessary and also calling the variable priv makes you think
> it's a struct jh7110_timer_priv, but the interrupt is registered with a
> struct clock_event_device pointer.
> 
>> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
> 
> But here you're immediately casting it to a struct jh7110_clkevt pointer. So
> why not just register the interrupt with the struct jh7110_clkevt pointer, do
> 
> 	struct jh7110_clkevt *clkevt = data;
> 

You are right. Will fix.

>> +
>> +	if (jh7110_timer_int_clear(clkevt))
>> +		return IRQ_NONE;
>> +
>> +	if (evt->event_handler)
>> +		evt->event_handler(evt);
> 
> ..and just use clkevt->evt.event_handler and &clkevt->evt here?

Will fix.

> 
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int jh7110_timer_set_periodic(struct clock_event_device *evt)
>> +{
>> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
>> +	u32 periodic = DIV_ROUND_CLOSEST(clkevt->rate, HZ);
>> +
>> +	writel(JH7110_TIMER_MODE_CONTIN, clkevt->base + JH7110_TIMER_CTL);
>> +	writel(periodic, clkevt->base + JH7110_TIMER_LOAD);
>> +
>> +	return jh7110_timer_start(clkevt);
>> +}
>> +
>> +static int jh7110_timer_set_oneshot(struct clock_event_device *evt)
>> +{
>> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
>> +
>> +	writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL);
>> +	writel(JH7110_TIMER_MAX_TICKS, clkevt->base + JH7110_TIMER_LOAD);
>> +
>> +	return jh7110_timer_start(clkevt);
>> +}
>> +
>> +static int jh7110_timer_set_next_event(unsigned long next,
>> +				       struct clock_event_device *evt)
>> +{
>> +	struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
>> +
>> +	writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL);
>> +	writel(next, clkevt->base + JH7110_TIMER_LOAD);
>> +
>> +	return jh7110_timer_start(clkevt);
>> +}
>> +
>> +static void jh7110_set_clockevent(struct clock_event_device *evt)
>> +{
>> +	evt->features = CLOCK_EVT_FEAT_PERIODIC |
>> +			CLOCK_EVT_FEAT_ONESHOT |
>> +			CLOCK_EVT_FEAT_DYNIRQ;
>> +	evt->set_state_shutdown = jh7110_timer_shutdown;
>> +	evt->set_state_periodic = jh7110_timer_set_periodic;
>> +	evt->set_state_oneshot = jh7110_timer_set_oneshot;
>> +	evt->set_state_oneshot_stopped = jh7110_timer_shutdown;
>> +	evt->tick_resume = jh7110_timer_tick_resume;
>> +	evt->set_next_event = jh7110_timer_set_next_event;
>> +	evt->suspend = jh7110_timer_suspend;
>> +	evt->resume = jh7110_timer_resume;
>> +	evt->rating = JH7110_CLOCKEVENT_RATING;
>> +}
>> +
>> +static u64 jh7110_timer_clocksource_read(struct clocksource *cs)
>> +{
>> +	struct jh7110_clkevt *clkevt = container_of(cs, struct jh7110_clkevt, cs);
>> +
>> +	return (u64)readl(clkevt->base + JH7110_TIMER_VALUE);
>> +}
>> +
>> +static int jh7110_clocksource_init(struct jh7110_clkevt *clkevt)
>> +{
>> +	int ret;
>> +
>> +	clkevt->cs.name = clkevt->name;
>> +	clkevt->cs.rating = JH7110_CLOCK_SOURCE_RATING;
>> +	clkevt->cs.read = jh7110_timer_clocksource_read;
>> +	clkevt->cs.mask = CLOCKSOURCE_MASK(JH7110_VALID_BITS);
>> +	clkevt->cs.flags = CLOCK_SOURCE_IS_CONTINUOUS;
>> +
>> +	ret = clocksource_register_hz(&clkevt->cs, clkevt->rate);
>> +	if (ret)
>> +		return ret;
>> +
>> +	clkevt->cs_is_valid = true; /* clocksource register done */
>> +	writel(JH7110_TIMER_MODE_CONTIN, clkevt->base + JH7110_TIMER_CTL);
>> +	writel(JH7110_TIMER_MAX_TICKS, clkevt->base + JH7110_TIMER_LOAD);
>> +
>> +	return jh7110_timer_start(clkevt);
>> +}
>> +
>> +static void jh7110_clockevents_register(struct jh7110_clkevt *clkevt)
>> +{
>> +	clkevt->rate = clk_get_rate(clkevt->clk);
>> +
>> +	jh7110_set_clockevent(&clkevt->evt);
>> +	clkevt->evt.name = clkevt->name;
>> +	clkevt->evt.cpumask = cpu_possible_mask;
>> +
>> +	clockevents_config_and_register(&clkevt->evt, clkevt->rate,
>> +					JH7110_TIMER_MIN_TICKS, JH7110_TIMER_MAX_TICKS);
>> +}
>> +
>> +static void jh7110_timer_release(void *data)
>> +{
>> +	struct jh7110_timer_priv *priv = data;
>> +	int i;
>> +
>> +	for (i = 0; i < JH7110_TIMER_CH_MAX; i++) {
>> +		/* Disable each channel of timer */
>> +		if (priv->clkevt[i].base)
>> +			writel(JH7110_TIMER_DIS, priv->clkevt[i].base + JH7110_TIMER_ENABLE);
>> +
>> +		/* Avoid no initialization in the loop of the probe */
>> +		if (!IS_ERR_OR_NULL(priv->clkevt[i].rst))
>> +			reset_control_assert(priv->clkevt[i].rst);
>> +
>> +		if (priv->clkevt[i].cs_is_valid)
>> +			clocksource_unregister(&priv->clkevt[i].cs);
>> +	}
>> +
>> +	reset_control_assert(priv->prst);
>> +}
>> +
>> +static int jh7110_timer_probe(struct platform_device *pdev)
>> +{
>> +	struct jh7110_timer_priv *priv;
>> +	struct jh7110_clkevt *clkevt;
>> +	char name[sizeof("chX")];
>> +	int ch;
>> +	int ret;
>> +	void __iomem *base;
>> +
>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(base))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(base),
>> +				     "failed to map registers\n");
>> +
>> +	priv->prst = devm_reset_control_get_exclusive(&pdev->dev, "apb");
>> +	if (IS_ERR(priv->prst))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->prst),
>> +				     "failed to get apb reset\n");
>> +
>> +	priv->pclk = devm_clk_get_enabled(&pdev->dev, "apb");
>> +	if (IS_ERR(priv->pclk))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->pclk),
>> +				     "failed to get & enable apb clock\n");
> 
> priv->pclk is set here, but never read anywhere. Just remove it from the struct
> and use a local variable here.

Sorry, I will drop it and use a local variable.

> 
>> +
>> +	ret = reset_control_deassert(priv->prst);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "failed to deassert apb reset\n");
>> +
>> +	ret = devm_add_action_or_reset(&pdev->dev, jh7110_timer_release, priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (ch = 0; ch < JH7110_TIMER_CH_MAX; ch++) {
>> +		clkevt = &priv->clkevt[ch];
>> +		snprintf(name, sizeof(name), "ch%d", ch);
>> +
>> +		clkevt->base = base + JH7110_TIMER_CH_BASE(ch);
>> +		/* Ensure timer is disabled */
>> +		writel(JH7110_TIMER_DIS, clkevt->base + JH7110_TIMER_ENABLE);
>> +
>> +		clkevt->rst = devm_reset_control_get_exclusive(&pdev->dev, name);
>> +		if (IS_ERR(clkevt->rst))
>> +			return PTR_ERR(clkevt->rst);
>> +
>> +		clkevt->clk = devm_clk_get_enabled(&pdev->dev, name);
>> +		if (IS_ERR(clkevt->clk))
>> +			return PTR_ERR(clkevt->clk);
>> +
>> +		ret = reset_control_deassert(clkevt->rst);
>> +		if (ret)
>> +			return ret;
>> +
>> +		clkevt->evt.irq = platform_get_irq(pdev, ch);
>> +		if (clkevt->evt.irq < 0)
>> +			return clkevt->evt.irq;
>> +
>> +		snprintf(clkevt->name, sizeof(clkevt->name), "jh7110-timer.ch%d", ch);
>> +		jh7110_clockevents_register(clkevt);
>> +
>> +		ret = devm_request_irq(&pdev->dev, clkevt->evt.irq, jh7110_timer_interrupt,
>> +				       IRQF_TIMER | IRQF_IRQPOLL,
>> +				       clkevt->name, &clkevt->evt);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = jh7110_clocksource_init(clkevt);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id jh7110_timer_match[] = {
>> +	{ .compatible = "starfive,jh7110-timer", },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, jh7110_timer_match);
>> +
>> +static struct platform_driver jh7110_timer_driver = {
>> +	.probe = jh7110_timer_probe,
>> +	.driver = {
>> +		.name = "jh7110-timer",
>> +		.of_match_table = jh7110_timer_match,
>> +	},
>> +};
>> +module_platform_driver(jh7110_timer_driver);
>> +
>> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
>> +MODULE_DESCRIPTION("StarFive JH7110 timer driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.25.1

Best Regards,
Xingyu Wu

On 2023/10/24 22:56, Daniel Lezcano wrote:
> 
> Hi Xingyu,
> 
> 
> On 19/10/2023 07:35, Xingyu Wu wrote:
>> Add timer driver for the StarFive JH7110 SoC.
> 
> As it is a new timer, please add a proper nice description explaining the timer hardware, thanks.

OK. Will add the description in next version.

> 
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>   MAINTAINERS                        |   7 +
>>   drivers/clocksource/Kconfig        |  11 +
>>   drivers/clocksource/Makefile       |   1 +
>>   drivers/clocksource/timer-jh7110.c | 380 +++++++++++++++++++++++++++++
>>   4 files changed, 399 insertions(+)
>>   create mode 100644 drivers/clocksource/timer-jh7110.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7a7bd8bd80e9..91c09b399131 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20473,6 +20473,13 @@ S:    Maintained
>>   F:    Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>>   F:    sound/soc/starfive/jh7110_tdm.c
>>   +STARFIVE JH7110 TIMER DRIVER
>> +M:    Samin Guo <samin.guo@starfivetech.com>
>> +M:    Xingyu Wu <xingyu.wu@starfivetech.com>
>> +S:    Supported
>> +F:    Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
>> +F:    drivers/clocksource/timer-jh7110.c
>> +
>>   STARFIVE JH71X0 CLOCK DRIVERS
>>   M:    Emil Renner Berthing <kernel@esmil.dk>
>>   M:    Hal Feng <hal.feng@starfivetech.com>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 0ba0dc4ecf06..821abcc1e517 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -641,6 +641,17 @@ config RISCV_TIMER
>>         is accessed via both the SBI and the rdcycle instruction.  This is
>>         required for all RISC-V systems.
>>   +config STARFIVE_JH7110_TIMER
>> +    bool "Timer for the STARFIVE JH7110 SoC"
>> +    depends on ARCH_STARFIVE || COMPILE_TEST
> 
> You may want to use ARCH_STARFIVE only if the platform can make this timer optional. Otherwise, set the option from the platform Kconfig and put the bool "bla bla" if COMPILE_TEST

Yes, this timer only be used on the StarFive SoC. So I intend to modify to this:

bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST
depends on ARCH_STARFIVE

> 
>> +    select TIMER_OF
>> +    select CLKSRC_MMIO
>> +    default ARCH_STARFIVE
> 
> no "default"

Will drop it.

> 
>> +    help
>> +      This enables the timer for StarFive JH7110 SoC. On RISC-V platform,
>> +      the system has started RISCV_TIMER, but you can also use this timer
>> +      which can provide four channels to do a lot more things on JH7110 SoC.
>> +
>>   config CLINT_TIMER
>>       bool "CLINT Timer for the RISC-V platform" if COMPILE_TEST
>>       depends on GENERIC_SCHED_CLOCK && RISCV
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index 368c3461dab8..b66ac05ec086 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -80,6 +80,7 @@ obj-$(CONFIG_INGENIC_TIMER)        += ingenic-timer.o
>>   obj-$(CONFIG_CLKSRC_ST_LPC)        += clksrc_st_lpc.o
>>   obj-$(CONFIG_X86_NUMACHIP)        += numachip.o
>>   obj-$(CONFIG_RISCV_TIMER)        += timer-riscv.o
>> +obj-$(CONFIG_STARFIVE_JH7110_TIMER)    += timer-jh7110.o
>>   obj-$(CONFIG_CLINT_TIMER)        += timer-clint.o
>>   obj-$(CONFIG_CSKY_MP_TIMER)        += timer-mp-csky.o
>>   obj-$(CONFIG_GX6605S_TIMER)        += timer-gx6605s.o
>> diff --git a/drivers/clocksource/timer-jh7110.c b/drivers/clocksource/timer-jh7110.c
>> new file mode 100644
>> index 000000000000..71de29a3ec91
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-jh7110.c
>> @@ -0,0 +1,380 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Starfive JH7110 Timer driver
>> + *
>> + * Copyright (C) 2022-2023 StarFive Technology Co., Ltd.
>> + *
>> + * Author:
>> + * Xingyu Wu <xingyu.wu@starfivetech.com>
>> + * Samin Guo <samin.guo@starfivetech.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/irq.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/sched_clock.h>
> 
> Please double check the headers and remove the pointless ones.

Will fix.

> 
> 
>> +/* Bias: Ch0-0x0, Ch1-0x40, Ch2-0x80, and so on. */
>> +#define JH7110_TIMER_CH_LEN        0x40
>> +#define JH7110_TIMER_CH_BASE(x)        ((x) * JH7110_TIMER_CH_LEN)
>> +#define JH7110_TIMER_CH_MAX        4
>> +
>> +#define JH7110_CLOCK_SOURCE_RATING    200
>> +#define JH7110_VALID_BITS        32
>> +#define JH7110_DELAY_US            0
>> +#define JH7110_TIMEOUT_US        10000
>> +#define JH7110_CLOCKEVENT_RATING    300
>> +#define JH7110_TIMER_MAX_TICKS        0xffffffff
>> +#define JH7110_TIMER_MIN_TICKS        0xf
>> +#define JH7110_TIMER_RELOAD_VALUE    0
>> +
>> +#define JH7110_TIMER_INT_STATUS        0x00 /* RO[0:4]: Interrupt Status for channel0~4 */
>> +#define JH7110_TIMER_CTL        0x04 /* RW[0]: 0-continuous run, 1-single run */
>> +#define JH7110_TIMER_LOAD        0x08 /* RW: load value to counter */
>> +#define JH7110_TIMER_ENABLE        0x10 /* RW[0]: timer enable register */
>> +#define JH7110_TIMER_RELOAD        0x14 /* RW: write 1 or 0 both reload counter */
>> +#define JH7110_TIMER_VALUE        0x18 /* RO: timer value register */
>> +#define JH7110_TIMER_INT_CLR        0x20 /* RW: timer interrupt clear register */
>> +#define JH7110_TIMER_INT_MASK        0x24 /* RW[0]: timer interrupt mask register */
>> +
>> +#define JH7110_TIMER_INT_CLR_ENA    BIT(0)
>> +#define JH7110_TIMER_INT_CLR_AVA_MASK    BIT(1)
>> +
>> +struct jh7110_clkevt {
>> +    struct clock_event_device evt;
>> +    struct clocksource cs;
>> +    bool cs_is_valid;
>> +    struct clk *clk;
>> +    struct reset_control *rst;
>> +    u32 rate;
>> +    u32 reload_val;
>> +    void __iomem *base;
>> +    char name[sizeof("jh7110-timer.chX")];
>> +};
>> +
>> +struct jh7110_timer_priv {
>> +    struct clk *pclk;
>> +    struct reset_control *prst;
>> +    struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX];
> 
> Why do you need several clock events and clock sources ?

This timer has four counters (channels) which run independently. So each counter can have its own clock event and clock source to configure different settings.

> 
> [ ... ]
> 
> 

Thanks,
Xingyu Wu

Hi Xingyu,


On 25/10/2023 11:04, Xingyu Wu wrote:
> On 2023/10/24 22:56, Daniel Lezcano wrote:
>>
>> Hi Xingyu,
>>
>>
>> On 19/10/2023 07:35, Xingyu Wu wrote:
>>> Add timer driver for the StarFive JH7110 SoC.
>>
>> As it is a new timer, please add a proper nice description explaining the timer hardware, thanks.
> 
> OK. Will add the description in next version.
> 
>>
>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>> ---
>>>    MAINTAINERS                        |   7 +
>>>    drivers/clocksource/Kconfig        |  11 +
>>>    drivers/clocksource/Makefile       |   1 +
>>>    drivers/clocksource/timer-jh7110.c | 380 +++++++++++++++++++++++++++++
>>>    4 files changed, 399 insertions(+)
>>>    create mode 100644 drivers/clocksource/timer-jh7110.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 7a7bd8bd80e9..91c09b399131 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -20473,6 +20473,13 @@ S:    Maintained
>>>    F:    Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>>>    F:    sound/soc/starfive/jh7110_tdm.c
>>>    +STARFIVE JH7110 TIMER DRIVER
>>> +M:    Samin Guo <samin.guo@starfivetech.com>
>>> +M:    Xingyu Wu <xingyu.wu@starfivetech.com>
>>> +S:    Supported
>>> +F:    Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
>>> +F:    drivers/clocksource/timer-jh7110.c
>>> +
>>>    STARFIVE JH71X0 CLOCK DRIVERS
>>>    M:    Emil Renner Berthing <kernel@esmil.dk>
>>>    M:    Hal Feng <hal.feng@starfivetech.com>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index 0ba0dc4ecf06..821abcc1e517 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -641,6 +641,17 @@ config RISCV_TIMER
>>>          is accessed via both the SBI and the rdcycle instruction.  This is
>>>          required for all RISC-V systems.
>>>    +config STARFIVE_JH7110_TIMER
>>> +    bool "Timer for the STARFIVE JH7110 SoC"
>>> +    depends on ARCH_STARFIVE || COMPILE_TEST
>>
>> You may want to use ARCH_STARFIVE only if the platform can make this timer optional. Otherwise, set the option from the platform Kconfig and put the bool "bla bla" if COMPILE_TEST
> 
> Yes, this timer only be used on the StarFive SoC. So I intend to modify to this:
> 
> bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST
> depends on ARCH_STARFIVE

In this case, you should change the platform config and select the timer 
from there. Remove the depends on ARCH_STARFIVE so it is possible enable 
cross test compilation. Otherwise COMPILE_TEST will not work on other 
platforms.

[ ... ]

>>> +struct jh7110_clkevt {
>>> +    struct clock_event_device evt;
>>> +    struct clocksource cs;
>>> +    bool cs_is_valid;
>>> +    struct clk *clk;
>>> +    struct reset_control *rst;
>>> +    u32 rate;
>>> +    u32 reload_val;
>>> +    void __iomem *base;
>>> +    char name[sizeof("jh7110-timer.chX")];
>>> +};
>>> +
>>> +struct jh7110_timer_priv {
>>> +    struct clk *pclk;
>>> +    struct reset_control *prst;
>>> +    struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX];
>>
>> Why do you need several clock events and clock sources ?
> 
> This timer has four counters (channels) which run independently. So each counter can have its own clock event and clock source to configure different settings.

The kernel only needs one clocksource. Usually multiple clockevents are 
per-cpu based system.

The driver does not seem to have a per cpu timer but just initializing 
multiple clockevents which will end up unused, wasting energy.

On 2023/10/25 22:39, Daniel Lezcano wrote:
> 
> Hi Xingyu,
> 
> 
> On 25/10/2023 11:04, Xingyu Wu wrote:
>> On 2023/10/24 22:56, Daniel Lezcano wrote:
>>>
>>> Hi Xingyu,
>>>
>>>
>>> On 19/10/2023 07:35, Xingyu Wu wrote:
>>>> Add timer driver for the StarFive JH7110 SoC.
>>>
>>> As it is a new timer, please add a proper nice description explaining the timer hardware, thanks.
>>
>> OK. Will add the description in next version.
>>
>>>
>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>>> ---
>>>>    MAINTAINERS                        |   7 +
>>>>    drivers/clocksource/Kconfig        |  11 +
>>>>    drivers/clocksource/Makefile       |   1 +
>>>>    drivers/clocksource/timer-jh7110.c | 380 +++++++++++++++++++++++++++++
>>>>    4 files changed, 399 insertions(+)
>>>>    create mode 100644 drivers/clocksource/timer-jh7110.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 7a7bd8bd80e9..91c09b399131 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -20473,6 +20473,13 @@ S:    Maintained
>>>>    F:    Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>>>>    F:    sound/soc/starfive/jh7110_tdm.c
>>>>    +STARFIVE JH7110 TIMER DRIVER
>>>> +M:    Samin Guo <samin.guo@starfivetech.com>
>>>> +M:    Xingyu Wu <xingyu.wu@starfivetech.com>
>>>> +S:    Supported
>>>> +F:    Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
>>>> +F:    drivers/clocksource/timer-jh7110.c
>>>> +
>>>>    STARFIVE JH71X0 CLOCK DRIVERS
>>>>    M:    Emil Renner Berthing <kernel@esmil.dk>
>>>>    M:    Hal Feng <hal.feng@starfivetech.com>
>>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>>> index 0ba0dc4ecf06..821abcc1e517 100644
>>>> --- a/drivers/clocksource/Kconfig
>>>> +++ b/drivers/clocksource/Kconfig
>>>> @@ -641,6 +641,17 @@ config RISCV_TIMER
>>>>          is accessed via both the SBI and the rdcycle instruction.  This is
>>>>          required for all RISC-V systems.
>>>>    +config STARFIVE_JH7110_TIMER
>>>> +    bool "Timer for the STARFIVE JH7110 SoC"
>>>> +    depends on ARCH_STARFIVE || COMPILE_TEST
>>>
>>> You may want to use ARCH_STARFIVE only if the platform can make this timer optional. Otherwise, set the option from the platform Kconfig and put the bool "bla bla" if COMPILE_TEST
>>
>> Yes, this timer only be used on the StarFive SoC. So I intend to modify to this:
>>
>> bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST
>> depends on ARCH_STARFIVE
> 
> In this case, you should change the platform config and select the timer from there. Remove the depends on ARCH_STARFIVE so it is possible enable cross test compilation. Otherwise COMPILE_TEST will not work on other platforms.
> 
> [ ... ]
> 

It is not a kernel timer or clocksource. It will not work on other platforms and is just used on the JH7110 SoC.
I think I needn't remove it. Maybe I modify to this:

bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST
depends on ARCH_STARFIVE || COMPILE_TEST

>>>> +struct jh7110_clkevt {
>>>> +    struct clock_event_device evt;
>>>> +    struct clocksource cs;
>>>> +    bool cs_is_valid;
>>>> +    struct clk *clk;
>>>> +    struct reset_control *rst;
>>>> +    u32 rate;
>>>> +    u32 reload_val;
>>>> +    void __iomem *base;
>>>> +    char name[sizeof("jh7110-timer.chX")];
>>>> +};
>>>> +
>>>> +struct jh7110_timer_priv {
>>>> +    struct clk *pclk;
>>>> +    struct reset_control *prst;
>>>> +    struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX];
>>>
>>> Why do you need several clock events and clock sources ?
>>
>> This timer has four counters (channels) which run independently. So each counter can have its own clock event and clock source to configure different settings.
> 
> The kernel only needs one clocksource. Usually multiple clockevents are per-cpu based system.
> 
> The driver does not seem to have a per cpu timer but just initializing multiple clockevents which will end up unused, wasting energy.
> 
> 

The board of the StarFive JH7110 SoC has two types of timer : riscv-timer and jh7110-timer. It boots by riscv-timer(clocksource) and the jh7110-timer is optional and additional.
I think I should initialize the four channels of jh7110-timer as clockevents not clocksource pre-cpu.

Thanks,
Xingyu Wu

On 27/10/2023 11:17, Xingyu Wu wrote:
> On 2023/10/25 22:39, Daniel Lezcano wrote:
>> 
>> Hi Xingyu,
>> 
>> 
>> On 25/10/2023 11:04, Xingyu Wu wrote:
>>> On 2023/10/24 22:56, Daniel Lezcano wrote:
>>>> 
>>>> Hi Xingyu,
>>>> 
>>>> 
>>>> On 19/10/2023 07:35, Xingyu Wu wrote:
>>>>> Add timer driver for the StarFive JH7110 SoC.
>>>> 
>>>> As it is a new timer, please add a proper nice description
>>>> explaining the timer hardware, thanks.
>>> 
>>> OK. Will add the description in next version.
>>> 
>>>> 
>>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> --- 
>>>>> MAINTAINERS                        |   7 + 
>>>>> drivers/clocksource/Kconfig        |  11 + 
>>>>> drivers/clocksource/Makefile       |   1 + 
>>>>> drivers/clocksource/timer-jh7110.c | 380
>>>>> +++++++++++++++++++++++++++++ 4 files changed, 399
>>>>> insertions(+) create mode 100644
>>>>> drivers/clocksource/timer-jh7110.c
>>>>> 
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS index
>>>>> 7a7bd8bd80e9..91c09b399131 100644 --- a/MAINTAINERS +++
>>>>> b/MAINTAINERS @@ -20473,6 +20473,13 @@ S:    Maintained F:
>>>>> Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>>>>>
>>>>> 
F:    sound/soc/starfive/jh7110_tdm.c
>>>>> +STARFIVE JH7110 TIMER DRIVER +M:    Samin Guo
>>>>> <samin.guo@starfivetech.com> +M:    Xingyu Wu
>>>>> <xingyu.wu@starfivetech.com> +S:    Supported +F:
>>>>> Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
>>>>>
>>>>> 
+F:    drivers/clocksource/timer-jh7110.c
>>>>> + STARFIVE JH71X0 CLOCK DRIVERS M:    Emil Renner Berthing
>>>>> <kernel@esmil.dk> M:    Hal Feng <hal.feng@starfivetech.com> 
>>>>> diff --git a/drivers/clocksource/Kconfig
>>>>> b/drivers/clocksource/Kconfig index
>>>>> 0ba0dc4ecf06..821abcc1e517 100644 ---
>>>>> a/drivers/clocksource/Kconfig +++
>>>>> b/drivers/clocksource/Kconfig @@ -641,6 +641,17 @@ config
>>>>> RISCV_TIMER is accessed via both the SBI and the rdcycle
>>>>> instruction.  This is required for all RISC-V systems. 
>>>>> +config STARFIVE_JH7110_TIMER +    bool "Timer for the
>>>>> STARFIVE JH7110 SoC" +    depends on ARCH_STARFIVE ||
>>>>> COMPILE_TEST
>>>> 
>>>> You may want to use ARCH_STARFIVE only if the platform can make
>>>> this timer optional. Otherwise, set the option from the
>>>> platform Kconfig and put the bool "bla bla" if COMPILE_TEST
>>> 
>>> Yes, this timer only be used on the StarFive SoC. So I intend to
>>> modify to this:
>>> 
>>> bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST depends
>>> on ARCH_STARFIVE
>> 
>> In this case, you should change the platform config and select the
>> timer from there. Remove the depends on ARCH_STARFIVE so it is
>> possible enable cross test compilation. Otherwise COMPILE_TEST will
>> not work on other platforms.
>> 
>> [ ... ]
>> 
> 
> It is not a kernel timer or clocksource. It will not work on other
> platforms and is just used on the JH7110 SoC. I think I needn't
> remove it. Maybe I modify to this:
> 
> bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST depends on
> ARCH_STARFIVE || COMPILE_TEST

I think there is a misunderstanding.

If we want to compile on x86 drivers for other platforms, we select 
COMPILE_TEST so we can enable the timer and do compilation testing.

In this case, we may want to compile the STARFIVE JH7110 on x86 just to 
double check it is correctly compiling (eg. we do changes impacting all 
the drivers). If the ARCH_STARFIVE dependency is set, then that won't be 
possible.

So it should be:

bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST
...

And in arch/riscv/Kconfig.socs

config SOC_STARFIVE
     ...
     select STARFIVE_JH7110_TIMER
     ...

>>>>> +struct jh7110_clkevt { +    struct clock_event_device evt; +
>>>>> struct clocksource cs; +    bool cs_is_valid; +    struct clk
>>>>> *clk; +    struct reset_control *rst; +    u32 rate; +    u32
>>>>> reload_val; +    void __iomem *base; +    char
>>>>> name[sizeof("jh7110-timer.chX")]; +}; + +struct
>>>>> jh7110_timer_priv { +    struct clk *pclk; +    struct
>>>>> reset_control *prst; +    struct jh7110_clkevt
>>>>> clkevt[JH7110_TIMER_CH_MAX];
>>>> 
>>>> Why do you need several clock events and clock sources ?
>>> 
>>> This timer has four counters (channels) which run independently.
>>> So each counter can have its own clock event and clock source to
>>> configure different settings.
>> 
>> The kernel only needs one clocksource. Usually multiple clockevents
>> are per-cpu based system.
>> 
>> The driver does not seem to have a per cpu timer but just
>> initializing multiple clockevents which will end up unused, wasting
>> energy.
>> 
>> 
> 
> The board of the StarFive JH7110 SoC has two types of timer :
> riscv-timer and jh7110-timer. It boots by riscv-timer(clocksource)
> and the jh7110-timer is optional and additional. I think I should
> initialize the four channels of jh7110-timer as clockevents not
> clocksource pre-cpu.

If no clocksource is needed on this SoC because riscv timers are used, 
then it is not useful to register a clocksource for this timer and the 
corresponding code can go away.

If the clockevent is optional why do you need this driver at all?

On 2023/10/27 21:34, Daniel Lezcano wrote:
> 
> On 27/10/2023 11:17, Xingyu Wu wrote:
>> On 2023/10/25 22:39, Daniel Lezcano wrote:
>>>
>>> Hi Xingyu,
>>>
>>>
>>> On 25/10/2023 11:04, Xingyu Wu wrote:
>>>> On 2023/10/24 22:56, Daniel Lezcano wrote:
>>>>>
>>>>> Hi Xingyu,
>>>>>
>>>>>
>>>>> On 19/10/2023 07:35, Xingyu Wu wrote:
>>>>>> Add timer driver for the StarFive JH7110 SoC.
>>>>>
>>>>> As it is a new timer, please add a proper nice description
>>>>> explaining the timer hardware, thanks.
>>>>
>>>> OK. Will add the description in next version.
>>>>
>>>>>
>>>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> --- MAINTAINERS                        |   7 + drivers/clocksource/Kconfig        |  11 + drivers/clocksource/Makefile       |   1 + drivers/clocksource/timer-jh7110.c | 380
>>>>>> +++++++++++++++++++++++++++++ 4 files changed, 399
>>>>>> insertions(+) create mode 100644
>>>>>> drivers/clocksource/timer-jh7110.c
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS index
>>>>>> 7a7bd8bd80e9..91c09b399131 100644 --- a/MAINTAINERS +++
>>>>>> b/MAINTAINERS @@ -20473,6 +20473,13 @@ S:    Maintained F:
>>>>>> Documentation/devicetree/bindings/sound/starfive,jh7110-tdm.yaml
>>>>>>
>>>>>>
> F:    sound/soc/starfive/jh7110_tdm.c
>>>>>> +STARFIVE JH7110 TIMER DRIVER +M:    Samin Guo
>>>>>> <samin.guo@starfivetech.com> +M:    Xingyu Wu
>>>>>> <xingyu.wu@starfivetech.com> +S:    Supported +F:
>>>>>> Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml
>>>>>>
>>>>>>
> +F:    drivers/clocksource/timer-jh7110.c
>>>>>> + STARFIVE JH71X0 CLOCK DRIVERS M:    Emil Renner Berthing
>>>>>> <kernel@esmil.dk> M:    Hal Feng <hal.feng@starfivetech.com> diff --git a/drivers/clocksource/Kconfig
>>>>>> b/drivers/clocksource/Kconfig index
>>>>>> 0ba0dc4ecf06..821abcc1e517 100644 ---
>>>>>> a/drivers/clocksource/Kconfig +++
>>>>>> b/drivers/clocksource/Kconfig @@ -641,6 +641,17 @@ config
>>>>>> RISCV_TIMER is accessed via both the SBI and the rdcycle
>>>>>> instruction.  This is required for all RISC-V systems. +config STARFIVE_JH7110_TIMER +    bool "Timer for the
>>>>>> STARFIVE JH7110 SoC" +    depends on ARCH_STARFIVE ||
>>>>>> COMPILE_TEST
>>>>>
>>>>> You may want to use ARCH_STARFIVE only if the platform can make
>>>>> this timer optional. Otherwise, set the option from the
>>>>> platform Kconfig and put the bool "bla bla" if COMPILE_TEST
>>>>
>>>> Yes, this timer only be used on the StarFive SoC. So I intend to
>>>> modify to this:
>>>>
>>>> bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST depends
>>>> on ARCH_STARFIVE
>>>
>>> In this case, you should change the platform config and select the
>>> timer from there. Remove the depends on ARCH_STARFIVE so it is
>>> possible enable cross test compilation. Otherwise COMPILE_TEST will
>>> not work on other platforms.
>>>
>>> [ ... ]
>>>
>>
>> It is not a kernel timer or clocksource. It will not work on other
>> platforms and is just used on the JH7110 SoC. I think I needn't
>> remove it. Maybe I modify to this:
>>
>> bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST depends on
>> ARCH_STARFIVE || COMPILE_TEST
> 
> I think there is a misunderstanding.
> 
> If we want to compile on x86 drivers for other platforms, we select COMPILE_TEST so we can enable the timer and do compilation testing.
> 
> In this case, we may want to compile the STARFIVE JH7110 on x86 just to double check it is correctly compiling (eg. we do changes impacting all the drivers). If the ARCH_STARFIVE dependency is set, then that won't be possible.
> 
> So it should be:
> 
> bool "Timer for the STARFIVE JH7110 SoC" if COMPILE_TEST
> ...
> 
> And in arch/riscv/Kconfig.socs
> 
> config SOC_STARFIVE
>     ...
>     select STARFIVE_JH7110_TIMER
>     ...
> 
>>>>>> +struct jh7110_clkevt { +    struct clock_event_device evt; +
>>>>>> struct clocksource cs; +    bool cs_is_valid; +    struct clk
>>>>>> *clk; +    struct reset_control *rst; +    u32 rate; +    u32
>>>>>> reload_val; +    void __iomem *base; +    char
>>>>>> name[sizeof("jh7110-timer.chX")]; +}; + +struct
>>>>>> jh7110_timer_priv { +    struct clk *pclk; +    struct
>>>>>> reset_control *prst; +    struct jh7110_clkevt
>>>>>> clkevt[JH7110_TIMER_CH_MAX];
>>>>>
>>>>> Why do you need several clock events and clock sources ?
>>>>
>>>> This timer has four counters (channels) which run independently.
>>>> So each counter can have its own clock event and clock source to
>>>> configure different settings.
>>>
>>> The kernel only needs one clocksource. Usually multiple clockevents
>>> are per-cpu based system.
>>>
>>> The driver does not seem to have a per cpu timer but just
>>> initializing multiple clockevents which will end up unused, wasting
>>> energy.
>>>
>>>
>>
>> The board of the StarFive JH7110 SoC has two types of timer :
>> riscv-timer and jh7110-timer. It boots by riscv-timer(clocksource)
>> and the jh7110-timer is optional and additional. I think I should
>> initialize the four channels of jh7110-timer as clockevents not
>> clocksource pre-cpu.
> 
> If no clocksource is needed on this SoC because riscv timers are used, then it is not useful to register a clocksource for this timer and the corresponding code can go away.
> 
> If the clockevent is optional why do you need this driver at all?
> 
> 
> 

Hi Daniel,

Sorry, maybe I didn't express it clearly enough. I use this jh7110-timer as a global timer on the SoC and riscv-timer as cpu local timer. So these are something different.

These four counters in this jh7110-timer are exactly the same and independent of each other. If this timer is used as a global timer, do I use only one or all of the counters to register clocksource and clockevent?

Thanks,
Xingyu Wu

Hi Xingyu,

On 02/11/2023 14:15, Xingyu Wu wrote:

[ ... ]

>>>>>>> +struct jh7110_clkevt { +    struct clock_event_device
>>>>>>> evt; + struct clocksource cs; +    bool cs_is_valid; +
>>>>>>> struct clk *clk; +    struct reset_control *rst; +    u32
>>>>>>> rate; +    u32 reload_val; +    void __iomem *base; +
>>>>>>> char name[sizeof("jh7110-timer.chX")]; +}; + +struct 
>>>>>>> jh7110_timer_priv { +    struct clk *pclk; +    struct 
>>>>>>> reset_control *prst; +    struct jh7110_clkevt 
>>>>>>> clkevt[JH7110_TIMER_CH_MAX];
>>>>>> 
>>>>>> Why do you need several clock events and clock sources ?
>>>>> 
>>>>> This timer has four counters (channels) which run
>>>>> independently. So each counter can have its own clock event
>>>>> and clock source to configure different settings.
>>>> 
>>>> The kernel only needs one clocksource. Usually multiple
>>>> clockevents are per-cpu based system.
>>>> 
>>>> The driver does not seem to have a per cpu timer but just 
>>>> initializing multiple clockevents which will end up unused,
>>>> wasting energy.
>>>> 
>>>> 
>>> 
>>> The board of the StarFive JH7110 SoC has two types of timer : 
>>> riscv-timer and jh7110-timer. It boots by
>>> riscv-timer(clocksource) and the jh7110-timer is optional and
>>> additional. I think I should initialize the four channels of
>>> jh7110-timer as clockevents not clocksource pre-cpu.
>> 
>> If no clocksource is needed on this SoC because riscv timers are
>> used, then it is not useful to register a clocksource for this
>> timer and the corresponding code can go away.
>> 
>> If the clockevent is optional why do you need this driver at all?
>> 
>> 
>> 
> 
> Hi Daniel,
> 
> Sorry, maybe I didn't express it clearly enough. I use this
> jh7110-timer as a global timer on the SoC and riscv-timer as cpu
> local timer. So these are something different.
> 
> These four counters in this jh7110-timer are exactly the same and
> independent of each other. If this timer is used as a global timer,
> do I use only one or all of the counters to register clocksource and
> clockevent?

Yes.

The global timer is only there when the CPU is powered down at idle 
time, so the time framework will switch to the broadcast timer and there 
can be only one instance.

If you register all the counters, only one will be used by the kernel, 
so it pointless to add them all.

On the clocksource side, you may want to question if it is really 
useful. The riscv has a clocksource with a higher rate and flagged as 
continuous [1]. So if the JH7110 clocksource is registered, it won't be 
used too.

Hope that helps

   -- Daniel

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n68

On 2023/11/2 22:29, Daniel Lezcano wrote:
> 
> Hi Xingyu,
> 
> On 02/11/2023 14:15, Xingyu Wu wrote:
> 
> [ ... ]
> 
>>>>>>>> +struct jh7110_clkevt { +    struct clock_event_device
>>>>>>>> evt; + struct clocksource cs; +    bool cs_is_valid; +
>>>>>>>> struct clk *clk; +    struct reset_control *rst; +    u32
>>>>>>>> rate; +    u32 reload_val; +    void __iomem *base; +
>>>>>>>> char name[sizeof("jh7110-timer.chX")]; +}; + +struct jh7110_timer_priv { +    struct clk *pclk; +    struct reset_control *prst; +    struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX];
>>>>>>>
>>>>>>> Why do you need several clock events and clock sources ?
>>>>>>
>>>>>> This timer has four counters (channels) which run
>>>>>> independently. So each counter can have its own clock event
>>>>>> and clock source to configure different settings.
>>>>>
>>>>> The kernel only needs one clocksource. Usually multiple
>>>>> clockevents are per-cpu based system.
>>>>>
>>>>> The driver does not seem to have a per cpu timer but just initializing multiple clockevents which will end up unused,
>>>>> wasting energy.
>>>>>
>>>>>
>>>>
>>>> The board of the StarFive JH7110 SoC has two types of timer : riscv-timer and jh7110-timer. It boots by
>>>> riscv-timer(clocksource) and the jh7110-timer is optional and
>>>> additional. I think I should initialize the four channels of
>>>> jh7110-timer as clockevents not clocksource pre-cpu.
>>>
>>> If no clocksource is needed on this SoC because riscv timers are
>>> used, then it is not useful to register a clocksource for this
>>> timer and the corresponding code can go away.
>>>
>>> If the clockevent is optional why do you need this driver at all?
>>>
>>>
>>>
>>
>> Hi Daniel,
>>
>> Sorry, maybe I didn't express it clearly enough. I use this
>> jh7110-timer as a global timer on the SoC and riscv-timer as cpu
>> local timer. So these are something different.
>>
>> These four counters in this jh7110-timer are exactly the same and
>> independent of each other. If this timer is used as a global timer,
>> do I use only one or all of the counters to register clocksource and
>> clockevent?
> 
> Yes.
> 
> The global timer is only there when the CPU is powered down at idle time, so the time framework will switch to the broadcast timer and there can be only one instance.
> 
> If you register all the counters, only one will be used by the kernel, so it pointless to add them all.
> 
> On the clocksource side, you may want to question if it is really useful. The riscv has a clocksource with a higher rate and flagged as continuous [1]. So if the JH7110 clocksource is registered, it won't be used too.
> 
> Hope that helps
> 
>   -- Daniel
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n68
> 

Thanks. The riscv-timer has a clocksource with a higher rating but a clockevent with lower rating[1] than jh7110-timer. I tested the jh7110-timer as clockevent and flagged as one shot, which could do some of the works instead of riscv-timer. And the current_clockevent changed to jh7110-timer.

Because the jh7110-timer works as clocksource with lower rating and only will be used as global timer at CPU idle time. Is it necessary to be registered as clocksource? If not, should it just be registered as clockevent?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n45

Thanks,
Xingyu Wu

On 08/11/2023 04:45, Xingyu Wu wrote:
> On 2023/11/2 22:29, Daniel Lezcano wrote:

[ ... ]

> Thanks. The riscv-timer has a clocksource with a higher rating but a
> clockevent with lower rating[1] than jh7110-timer. I tested the
> jh7110-timer as clockevent and flagged as one shot, which could do
> some of the works instead of riscv-timer. And the current_clockevent
> changed to jh7110-timer.
> 
> Because the jh7110-timer works as clocksource with lower rating and
> only will be used as global timer at CPU idle time. Is it necessary
> to be registered as clocksource? If not, should it just be registered
> as clockevent?

Yes, you can register the clockevent without the clocksource.

You mentioned the JH7110 has a better rating than the CPU architected 
timers. The rating is there to "choose" the best timer, so it is up to 
the author of the driver check against which timers it compares on the 
platform.

Usually, CPU timers are the best.

It is surprising the timer-riscv has a so low rating. You may double 
check if jh7110 is really better. If it is the case, then implementing a 
clockevent per cpu would make more sense, otherwise one clockevent as a 
global timer is enough.

Unused clocksource, clockevents should be stopped in case the firmware 
let them in a undetermined state.


> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n45
>
>  Thanks, Xingyu Wu

On 2023/11/8 17:10, Daniel Lezcano wrote:
> On 08/11/2023 04:45, Xingyu Wu wrote:
>> On 2023/11/2 22:29, Daniel Lezcano wrote:
> 
> [ ... ]
> 
>> Thanks. The riscv-timer has a clocksource with a higher rating but a
>> clockevent with lower rating[1] than jh7110-timer. I tested the
>> jh7110-timer as clockevent and flagged as one shot, which could do
>> some of the works instead of riscv-timer. And the current_clockevent
>> changed to jh7110-timer.
>>
>> Because the jh7110-timer works as clocksource with lower rating and
>> only will be used as global timer at CPU idle time. Is it necessary
>> to be registered as clocksource? If not, should it just be registered
>> as clockevent?
> 
> Yes, you can register the clockevent without the clocksource.
> 
> You mentioned the JH7110 has a better rating than the CPU architected timers. The rating is there to "choose" the best timer, so it is up to the author of the driver check against which timers it compares on the platform.
> 
> Usually, CPU timers are the best.
> 
> It is surprising the timer-riscv has a so low rating. You may double check if jh7110 is really better. If it is the case, then implementing a clockevent per cpu would make more sense, otherwise one clockevent as a global timer is enough.
> 
> Unused clocksource, clockevents should be stopped in case the firmware let them in a undetermined state.
> 
> 

The interrupts of jh7110-timer each channel are global interrupts like SPI(Shared Peripheral Interrupt) not PPI (Private Peripheral Interrupt). They are up to PLIC to select which core to respond to. So it is hard to implement a clockevent per cpu core. I tested this with request_percpu_irq() and it failed.

I think it is enough to implement a clockevent as a global timer. Thank you for your advice.

Best regards,
Xingyu Wu

>> [1
>> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n45
>>
>>  Thanks, Xingyu Wu
>

On 2023-11-08 11:51 PM, Xingyu Wu wrote:
> On 2023/11/8 17:10, Daniel Lezcano wrote:
>> On 08/11/2023 04:45, Xingyu Wu wrote:
>>> On 2023/11/2 22:29, Daniel Lezcano wrote:
>> 
>> [ ... ]
>> 
>>> Thanks. The riscv-timer has a clocksource with a higher rating but a 
>>> clockevent with lower rating[1] than jh7110-timer. I tested the 
>>> jh7110-timer as clockevent and flagged as one shot, which could do some
>>> of the works instead of riscv-timer. And the current_clockevent changed
>>> to jh7110-timer.
>>> 
>>> Because the jh7110-timer works as clocksource with lower rating and only
>>> will be used as global timer at CPU idle time. Is it necessary to be
>>> registered as clocksource? If not, should it just be registered as
>>> clockevent?
>> 
>> Yes, you can register the clockevent without the clocksource.
>> 
>> You mentioned the JH7110 has a better rating than the CPU architected
>> timers. The rating is there to "choose" the best timer, so it is up to the
>> author of the driver check against which timers it compares on the
>> platform.
>> 
>> Usually, CPU timers are the best.
>> 
>> It is surprising the timer-riscv has a so low rating. You may double check
>> if jh7110 is really better. If it is the case, then implementing a
>> clockevent per cpu would make more sense, otherwise one clockevent as a
>> global timer is enough.

The timer-riscv clockevent has a low rating because it requires a call to
firmware to set the timer, as well as a trap to firmware to handle the
interrupt, which both add overhead. Implementations which support the Sstc
extension[1] do not require firmware assistance to implement the clockevent, so
in that case we register the clockevent with a higher rating.

[1]: https://github.com/riscv/riscv-time-compare

>> Unused clocksource, clockevents should be stopped in case the firmware let
>> them in a undetermined state.
> 
> The interrupts of jh7110-timer each channel are global interrupts like
> SPI(Shared Peripheral Interrupt) not PPI (Private Peripheral Interrupt). They
> are up to PLIC to select which core to respond to. So it is hard to implement
> a clockevent per cpu core. I tested this with request_percpu_irq() and it
> failed.

You cannot use request_percpu_irq(), but the driver should be able to set the
affinity of each IRQ to a separate CPU.

Regards,
Samuel

> I think it is enough to implement a clockevent as a global timer. Thank you
> for your advice.
> 
> Best regards, Xingyu Wu

Hi Samuel,

On 10/11/2023 18:40, Samuel Holland wrote:
> On 2023-11-08 11:51 PM, Xingyu Wu wrote:
>> On 2023/11/8 17:10, Daniel Lezcano wrote:
>>> On 08/11/2023 04:45, Xingyu Wu wrote:
>>>> On 2023/11/2 22:29, Daniel Lezcano wrote:
>>>
>>> [ ... ]
>>>
>>>> Thanks. The riscv-timer has a clocksource with a higher rating but a
>>>> clockevent with lower rating[1] than jh7110-timer. I tested the
>>>> jh7110-timer as clockevent and flagged as one shot, which could do some
>>>> of the works instead of riscv-timer. And the current_clockevent changed
>>>> to jh7110-timer.
>>>>
>>>> Because the jh7110-timer works as clocksource with lower rating and only
>>>> will be used as global timer at CPU idle time. Is it necessary to be
>>>> registered as clocksource? If not, should it just be registered as
>>>> clockevent?
>>>
>>> Yes, you can register the clockevent without the clocksource.
>>>
>>> You mentioned the JH7110 has a better rating than the CPU architected
>>> timers. The rating is there to "choose" the best timer, so it is up to the
>>> author of the driver check against which timers it compares on the
>>> platform.
>>>
>>> Usually, CPU timers are the best.
>>>
>>> It is surprising the timer-riscv has a so low rating. You may double check
>>> if jh7110 is really better. If it is the case, then implementing a
>>> clockevent per cpu would make more sense, otherwise one clockevent as a
>>> global timer is enough.
> 
> The timer-riscv clockevent has a low rating because it requires a call to
> firmware to set the timer, as well as a trap to firmware to handle the
> interrupt, which both add overhead. Implementations which support the Sstc
> extension[1] do not require firmware assistance to implement the clockevent, so
> in that case we register the clockevent with a higher rating.
> 
> [1]: https://github.com/riscv/riscv-time-compare

Thanks for the pointer and the clarification.

>>> Unused clocksource, clockevents should be stopped in case the firmware let
>>> them in a undetermined state.
>>
>> The interrupts of jh7110-timer each channel are global interrupts like
>> SPI(Shared Peripheral Interrupt) not PPI (Private Peripheral Interrupt). They
>> are up to PLIC to select which core to respond to. So it is hard to implement
>> a clockevent per cpu core. I tested this with request_percpu_irq() and it
>> failed.
> 
> You cannot use request_percpu_irq(), but the driver should be able to set the
> affinity of each IRQ to a separate CPU.

Absolutely. And given the bad rating of the local timers, it may be 
worth to implement this driver in a per CPU (affinity set) basis.

At the first glance, the arm_global_timer can be used as an example.

Note in this case, you may want to double check what does with an idle 
state with a local timer stop flag and this timer which is always on.

On 2023/11/11 2:02, Daniel Lezcano wrote:
> 
> Hi Samuel,
> 
> On 10/11/2023 18:40, Samuel Holland wrote:
>> On 2023-11-08 11:51 PM, Xingyu Wu wrote:
>>> On 2023/11/8 17:10, Daniel Lezcano wrote:
>>>> On 08/11/2023 04:45, Xingyu Wu wrote:
>>>>> On 2023/11/2 22:29, Daniel Lezcano wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>> Thanks. The riscv-timer has a clocksource with a higher rating but a
>>>>> clockevent with lower rating[1] than jh7110-timer. I tested the
>>>>> jh7110-timer as clockevent and flagged as one shot, which could do some
>>>>> of the works instead of riscv-timer. And the current_clockevent changed
>>>>> to jh7110-timer.
>>>>>
>>>>> Because the jh7110-timer works as clocksource with lower rating and only
>>>>> will be used as global timer at CPU idle time. Is it necessary to be
>>>>> registered as clocksource? If not, should it just be registered as
>>>>> clockevent?
>>>>
>>>> Yes, you can register the clockevent without the clocksource.
>>>>
>>>> You mentioned the JH7110 has a better rating than the CPU architected
>>>> timers. The rating is there to "choose" the best timer, so it is up to the
>>>> author of the driver check against which timers it compares on the
>>>> platform.
>>>>
>>>> Usually, CPU timers are the best.
>>>>
>>>> It is surprising the timer-riscv has a so low rating. You may double check
>>>> if jh7110 is really better. If it is the case, then implementing a
>>>> clockevent per cpu would make more sense, otherwise one clockevent as a
>>>> global timer is enough.
>>
>> The timer-riscv clockevent has a low rating because it requires a call to
>> firmware to set the timer, as well as a trap to firmware to handle the
>> interrupt, which both add overhead. Implementations which support the Sstc
>> extension[1] do not require firmware assistance to implement the clockevent, so
>> in that case we register the clockevent with a higher rating.
>>
>> [1]: https://github.com/riscv/riscv-time-compare
> 
> Thanks for the pointer and the clarification.
> 
>>>> Unused clocksource, clockevents should be stopped in case the firmware let
>>>> them in a undetermined state.
>>>
>>> The interrupts of jh7110-timer each channel are global interrupts like
>>> SPI(Shared Peripheral Interrupt) not PPI (Private Peripheral Interrupt). They
>>> are up to PLIC to select which core to respond to. So it is hard to implement
>>> a clockevent per cpu core. I tested this with request_percpu_irq() and it
>>> failed.
>>
>> You cannot use request_percpu_irq(), but the driver should be able to set the
>> affinity of each IRQ to a separate CPU.
> 
> Absolutely. And given the bad rating of the local timers, it may be worth to implement this driver in a per CPU (affinity set) basis.
> 
> At the first glance, the arm_global_timer can be used as an example.
> 
> Note in this case, you may want to double check what does with an idle state with a local timer stop flag and this timer which is always on.
> 
> 
> 

Hi Daniel and Samuel,

Thanks for your pointers. I will check it. If it works, I will send the new version of this patch.

Best regards,
Xingyu Wu