Add timer driver for the StarFive JH7110 SoC.
Signed-off-by: Xingyu Wu <[email protected]>
---
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 <[email protected]>
+M: Xingyu Wu <[email protected]>
+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 <[email protected]>
M: Hal Feng <[email protected]>
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 <[email protected]>
+ * Samin Guo <[email protected]>
+ */
+
+#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")];
+};
+
+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);
+
+ 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;
+}
+
+/* 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;
+ struct jh7110_clkevt *clkevt = to_jh7110_clkevt(evt);
+
+ if (jh7110_timer_int_clear(clkevt))
+ return IRQ_NONE;
+
+ if (evt->event_handler)
+ evt->event_handler(evt);
+
+ 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");
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("StarFive JH7110 timer driver");
+MODULE_LICENSE("GPL");
--
2.25.1
Xingyu Wu wrote:
> Add timer driver for the StarFive JH7110 SoC.
>
> Signed-off-by: Xingyu Wu <[email protected]>
> ---
> 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 <[email protected]>
> +M: Xingyu Wu <[email protected]>
> +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 <[email protected]>
> M: Hal Feng <[email protected]>
> 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 <[email protected]>
> + * Samin Guo <[email protected]>
> + */
> +
> +#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 <[email protected]>");
> +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 <[email protected]>
> ---
> 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 <[email protected]>
> +M: Xingyu Wu <[email protected]>
> +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 <[email protected]>
> M: Hal Feng <[email protected]>
> 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 <[email protected]>
> + * Samin Guo <[email protected]>
> + */
> +
> +#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 ?
[ ... ]
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> +M: Xingyu Wu <[email protected]>
>> +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 <[email protected]>
>> M: Hal Feng <[email protected]>
>> 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 <[email protected]>
>> + * Samin Guo <[email protected]>
>> + */
>> +
>> +#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 <[email protected]>");
>> +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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> +M: Xingyu Wu <[email protected]>
>> +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 <[email protected]>
>> M: Hal Feng <[email protected]>
>> 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 <[email protected]>
>> + * Samin Guo <[email protected]>
>> + */
>> +
>> +#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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>> +M: Xingyu Wu <[email protected]>
>>> +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 <[email protected]>
>>> M: Hal Feng <[email protected]>
>>> 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.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>>>> +M: Xingyu Wu <[email protected]>
>>>> +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 <[email protected]>
>>>> M: Hal Feng <[email protected]>
>>>> 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 <[email protected]> ---
>>>>> 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
>>>>> <[email protected]> +M: Xingyu Wu
>>>>> <[email protected]> +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
>>>>> <[email protected]> M: Hal Feng <[email protected]>
>>>>> 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?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
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 <[email protected]> --- 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
>>>>>> <[email protected]> +M: Xingyu Wu
>>>>>> <[email protected]> +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
>>>>>> <[email protected]> M: Hal Feng <[email protected]> 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
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog