| Message ID | 20231219145402.7879-1-xingyu.wu@starfivetech.com |
|---|---|
| Headers | show |
| Series | Add timer driver for StarFive JH7110 RISC-V SoC | expand |
Xingyu Wu wrote: > Add timer driver for the StarFive JH7110 SoC and select it by > CONFIG_SOC_STARFIVE. > > This timer has four free-running and independent 32-bit counters. > Each channel(counter) can trigger an interrupt when timeout even > CPU is sleeping. So this timer is used as global timer and register > clockevent for each CPU core after riscv-timer registration on the > StarFive JH7110 SoC. > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> > --- > MAINTAINERS | 7 + > arch/riscv/Kconfig.socs | 1 + > drivers/clocksource/Kconfig | 9 + > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-jh7110.c | 360 +++++++++++++++++++++++++++++ > include/linux/cpuhotplug.h | 1 + > 6 files changed, 379 insertions(+) > create mode 100644 drivers/clocksource/timer-jh7110.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9104430e148e..fe0e803606a5 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20617,6 +20617,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> Last time I sent a mail to samin.guo@starfivetech.com it bounced. Was that just a temporary error? /Emil > +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>
On 2023/12/20 21:59, Emil Renner Berthing wrote: > Xingyu Wu wrote: >> Add timer driver for the StarFive JH7110 SoC and select it by >> CONFIG_SOC_STARFIVE. >> >> This timer has four free-running and independent 32-bit counters. >> Each channel(counter) can trigger an interrupt when timeout even >> CPU is sleeping. So this timer is used as global timer and register >> clockevent for each CPU core after riscv-timer registration on the >> StarFive JH7110 SoC. >> >> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com> >> --- >> MAINTAINERS | 7 + >> arch/riscv/Kconfig.socs | 1 + >> drivers/clocksource/Kconfig | 9 + >> drivers/clocksource/Makefile | 1 + >> drivers/clocksource/timer-jh7110.c | 360 +++++++++++++++++++++++++++++ >> include/linux/cpuhotplug.h | 1 + >> 6 files changed, 379 insertions(+) >> create mode 100644 drivers/clocksource/timer-jh7110.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 9104430e148e..fe0e803606a5 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -20617,6 +20617,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> > > Last time I sent a mail to samin.guo@starfivetech.com it bounced. Was that just > a temporary error? > > /Emil Oh, This email has been deactivated and I don't have his other personal email. I had dropped it in the driver but forget it here. Will fix. Thanks, Xingyu Wu > >> +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>
Hi Daniel, Could you please help to review this patch and give your comments if you have time? Thanks. Best regards, Xingyu Wu -----邮件原件----- 发件人: Xingyu Wu <xingyu.wu@starfivetech.com> 发送时间: 2023年12月19日 22:54 收件人: Daniel Lezcano <daniel.lezcano@linaro.org>; Thomas Gleixner <tglx@linutronix.de>; Emil Renner Berthing <emil.renner.berthing@canonical.com>; Christophe JAILLET <christophe.jaillet@wanadoo.fr> 抄送: linux-riscv@lists.infradead.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Paul Walmsley <paul.walmsley@sifive.com>; Palmer Dabbelt <palmer@dabbelt.com>; Albert Ou <aou@eecs.berkeley.edu>; Philipp Zabel <p.zabel@pengutronix.de>; Walker Chen <walker.chen@starfivetech.com>; Xingyu Wu <xingyu.wu@starfivetech.com>; linux-kernel@vger.kernel.org; Conor Dooley <conor@kernel.org> 主题: [PATCH v8 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. This timer is used as global timer and register clockevent for each CPU core after riscv-timer registration on the StarFive JH7110 SoC. Changes since v7: - Rebased on 6.7-rc6. - Modified the Kconfig file and added selection in SOC_STARFIVE. - Used the timer as a global timer and registered as clockevent for each CPU core. - Dropped the timeout function in the interrupt handler callback. - Changed the way in the functions of jh7110_timer_tick_resume() and jh7110_timer_resume(). - Dropped the registration of clocksource in the probe. v7: https://lore.kernel.org/all/20231019053501.46899-1-xingyu.wu@starfivetech.com/ 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/Kconfig.socs | 1 + arch/riscv/boot/dts/starfive/jh7110.dtsi | 20 + drivers/clocksource/Kconfig | 9 + drivers/clocksource/Makefile | 1 + drivers/clocksource/timer-jh7110.c | 360 ++++++++++++++++++ include/linux/cpuhotplug.h | 1 + 8 files changed, 495 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/starfive,jh7110-timer.yaml create mode 100644 drivers/clocksource/timer-jh7110.c -- 2.25.1
On Tue, Dec 19 2023 at 22:54, Xingyu Wu wrote: > + > +struct jh7110_clkevt { > + struct clk *clk; > + struct reset_control *rst; > + void __iomem *base; > + u32 reload_val; > + int irq; > + char name[sizeof("jh7110-timer.chX")]; > +}; > + > +struct jh7110_timer_priv { > + struct reset_control *prst; > + struct device *dev; > + struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX]; > +}; Please format your data structures according to documentation: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers > +/* IRQ handler for the timer */ > +static irqreturn_t jh7110_timer_interrupt(int irq, void *data) > +{ > + struct clock_event_device *evt = data; > + struct jh7110_clkevt *clkevt = &jh7110_timer_info.clkevt[0]; > + u8 cpu_id = smp_processor_id(); > + u32 reg = readl(clkevt->base + JH7110_TIMER_INT_STATUS); https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations > + /* Check interrupt status and channel(cpu) ID */ > + if (!(reg & BIT(cpu_id))) > + return IRQ_NONE; > + > + clkevt = &jh7110_timer_info.clkevt[cpu_id]; > + writel(JH7110_TIMER_INT_CLR_ENA, (clkevt->base + JH7110_TIMER_INT_CLR)); > + > + 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 = JH7110_PERCPU_GET_CLKEVT; > + > + writel(JH7110_TIMER_MODE_CONTIN, clkevt->base + JH7110_TIMER_CTL); > + return 0; > +} > + > +static int jh7110_timer_set_oneshot(struct clock_event_device *evt) > +{ > + struct jh7110_clkevt *clkevt = JH7110_PERCPU_GET_CLKEVT; > + > + writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL); > + return 0; > +} > + > +static int jh7110_timer_set_next_event(unsigned long next, > + struct clock_event_device *evt) > +{ > + struct jh7110_clkevt *clkevt = JH7110_PERCPU_GET_CLKEVT; > + > + writel(JH7110_TIMER_MODE_SINGLE, clkevt->base + JH7110_TIMER_CTL); > + writel(next, clkevt->base + JH7110_TIMER_LOAD); > + > + return jh7110_timer_start(clkevt); > +} > + > +static DEFINE_PER_CPU(struct clock_event_device, jh7110_clock_event) = { > + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT, > + .rating = JH7110_CLOCKEVENT_RATING, > + .set_state_shutdown = jh7110_timer_shutdown, > + .set_state_periodic = jh7110_timer_set_periodic, > + .set_state_oneshot = jh7110_timer_set_oneshot, > + .set_state_oneshot_stopped = jh7110_timer_shutdown, > + .tick_resume = jh7110_timer_tick_resume, > + .set_next_event = jh7110_timer_set_next_event, > + .suspend = jh7110_timer_suspend, > + .resume = jh7110_timer_resume, > +}; See formatting rules. > +static int jh7110_timer_starting_cpu(unsigned int cpu) > +{ > + struct clock_event_device *evt = per_cpu_ptr(&jh7110_clock_event, cpu); > + struct jh7110_timer_priv *priv = &jh7110_timer_info; > + int ret; > + u32 rate; > + > + if (cpu >= JH7110_TIMER_CH_MAX) > + return -ENOMEM; > + > + ret = clk_prepare_enable(priv->clkevt[cpu].clk); > + if (ret) > + return ret; > + > + ret = reset_control_deassert(priv->clkevt[cpu].rst); > + if (ret) > + return ret; > + > + rate = clk_get_rate(priv->clkevt[cpu].clk); > + evt->cpumask = cpumask_of(cpu); > + evt->irq = priv->clkevt[cpu].irq; > + evt->name = priv->clkevt[cpu].name; > + clockevents_config_and_register(evt, rate, JH7110_TIMER_MIN_TICKS, > + JH7110_TIMER_MAX_TICKS); > + > + ret = devm_request_irq(priv->dev, evt->irq, jh7110_timer_interrupt, > + IRQF_TIMER | IRQF_IRQPOLL, > + evt->name, evt); How is this all supposed to work from a CPU hotplug state callback which runs in the early bringup phase with interrupts disabled? clk_prepare() which is invoked from clk_prepare_enable() can sleep and devm_request_irq() can sleep too. Did you ever test this with the required debug config options enabled? https://www.kernel.org/doc/html/latest/process/submit-checklist.html Obviously not. > + if (ret) > + return ret; > + > + ret = irq_set_affinity(evt->irq, cpumask_of(cpu)); > + if (ret) > + return ret; > + /* Use one-shot mode */ > + writel(JH7110_TIMER_MODE_SINGLE, (priv->clkevt[cpu].base + JH7110_TIMER_CTL)); > + > + return jh7110_timer_start(&priv->clkevt[cpu]); > +} > + > +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 (!IS_ERR_OR_NULL(priv->clkevt[i].clk)) > + clk_disable_unprepare(priv->clkevt[i].clk); Same problem. And of course this does not undo the other steps of jh7110_timer_starting_cpu() so if you offline a CPU and then online it again the callback will fail because the clockevent is already registered and the interrupt requested. Clearly untested too. Thanks, tglx