[PATCH] timer: starfive: Add Starfive timer support

[KuanLim.Lee]

Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg at google.com>
> Sent: Wednesday, October 4, 2023 10:11 AM
> To: KuanLim.Lee <kuanlim.lee at starfivetech.com>
> Cc: u-boot at lists.denx.de; WeiLiang Lim <weiliang.lim at starfivetech.com>
> Subject: Re: [PATCH] timer: starfive: Add Starfive timer support
> 
> On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee <kuanlim.lee at starfivetech.com>
> wrote:
> >
> > Add timer driver in Starfive SoC. It is an timer that outside of CPU
> > core and inside Starfive SoC.
> >
> > Signed-off-by: Kuan Lim Lee <kuanlim.lee at starfivetech.com>
> > Reviewed-by: Wei Liang Lim <weiliang.lim at starfivetech.com>
> > ---
> >  drivers/timer/Kconfig          |  7 +++
> >  drivers/timer/Makefile         |  1 +
> >  drivers/timer/starfive-timer.c | 94
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 102 insertions(+)
> >  create mode 100644 drivers/timer/starfive-timer.c
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> nits below
What are the nits?
How do the nits to be solved? 
> 
> >
> > diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index
> > 915b2af160..a98be9dfae 100644
> > --- a/drivers/timer/Kconfig
> > +++ b/drivers/timer/Kconfig
> > @@ -326,4 +326,11 @@ config XILINX_TIMER
> >           Select this to enable support for the timer found on
> >           any Xilinx boards (axi timer).
> >
> > +config STARFIVE_TIMER
> > +       bool "Starfive timer support"
> > +       depends on TIMER
> > +       help
> > +         Select this to enable support for the timer found on
> > +         Starfive SoC.
> 
> What resolution is the timer? How is it clocked? Is there only one channel?
Timer will be driven by clock pulses from clocks. The clocks are defined in device tree.
Four channels timer, each timer has own clock source. 
Clock source frequency is gotten by clk_get_by_index () and clk_get_rate().
The timer counter register is a 32bits size register.
> 
> > +
> >  endmenu
> > diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile index
> > 1ca74805fd..1ef814970b 100644
> > --- a/drivers/timer/Makefile
> > +++ b/drivers/timer/Makefile
> > @@ -34,3 +34,4 @@ obj-$(CONFIG_MTK_TIMER)               += mtk_timer.o
> >  obj-$(CONFIG_MCHP_PIT64B_TIMER)        += mchp-pit64b-timer.o
> >  obj-$(CONFIG_IMX_GPT_TIMER)    += imx-gpt-timer.o
> >  obj-$(CONFIG_XILINX_TIMER)     += xilinx-timer.o
> > +obj-$(CONFIG_STARFIVE_TIMER)   += starfive-timer.o
> > diff --git a/drivers/timer/starfive-timer.c
> > b/drivers/timer/starfive-timer.c new file mode 100644 index
> > 0000000000..816402fdbf
> > --- /dev/null
> > +++ b/drivers/timer/starfive-timer.c
> > @@ -0,0 +1,94 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2022 StarFive, Inc. All rights reserved.
> > + *   Author: Lee Kuan Lim <kuanlim.lee at starfivetech.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <clk.h>
> > +#include <dm.h>
> > +#include <time.h>
> > +#include <timer.h>
> > +#include <asm/io.h>
> > +#include <dm/device-internal.h>
> > +#include <linux/err.h>
> > +
> > +#define        STF_TIMER_INT_STATUS    0x00
> > +#define STF_TIMER_CTL          0x04
> > +#define STF_TIMER_LOAD         0x08
> > +#define STF_TIMER_ENABLE       0x10
> > +#define STF_TIMER_RELOAD       0x14
> > +#define STF_TIMER_VALUE                0x18
> > +#define STF_TIMER_INT_CLR      0x20
> > +#define STF_TIMER_INT_MASK     0x24
> > +
> > +struct starfive_timer_priv {
> > +       void __iomem *base;
> > +       u32 timer_size;
> > +};
> > +
> > +static u64 notrace starfive_get_count(struct udevice *dev) {
> > +       struct starfive_timer_priv *priv = dev_get_priv(dev);
> > +
> > +       /* Read decrement timer value and convert to increment value */
> > +       return priv->timer_size - readl(priv->base + STF_TIMER_VALUE);
> > +}
> 
> As an enhancement, you could provide a timer_early_get_count() function and
> an easly setup, so you can use bootstage.
> 
Is this a must? If so, I must define some fixed address to get the timer count,
enable timer, clock source frequency because I can't get base address and
frequency from the device tree in early stage.
> > +
> > +static const struct timer_ops starfive_ops = {
> > +       .get_count = starfive_get_count, };
> > +
> > +static int starfive_probe(struct udevice *dev) {
> > +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > +       struct starfive_timer_priv *priv = dev_get_priv(dev);
> > +       int timer_channel;
> > +       struct clk clk;
> > +       int ret;
> > +
> > +       priv->base = dev_read_addr_ptr(dev);
> > +       if (IS_ERR(priv->base))
> 
> if (!priv->base)
>     return -EINVAL
> 
Noted.
> > +               return PTR_ERR(priv->base);
> > +
> > +       timer_channel = dev_read_u32_default(dev, "channel", 0);
> > +       priv->base = priv->base + (0x40 * timer_channel);
> > +
> > +       /* Get clock rate from channel selectecd*/
> > +       ret = clk_get_by_index(dev, timer_channel, &clk);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = clk_enable(&clk);
> > +       if (ret)
> > +               return ret;
> > +       uc_priv->clock_rate = clk_get_rate(&clk);
> > +
> > +       /* Initiate timer, channel 0 */
> > +       /* Unmask Interrupt Mask */
> 
> multi-line comment style is:
> 
>  /*
>   * line 1
>   * line 2
>   */
> 
Noted.
> > +       writel(0, priv->base + STF_TIMER_INT_MASK);
> > +       /* Single run mode Setting */
> > +       if (dev_read_bool(dev, "single-run"))
> > +               writel(1, priv->base + STF_TIMER_CTL);
> > +       /* Set Reload value */
> > +       priv->timer_size = dev_read_u32_default(dev, "timer-size",
> > + 0xffffffff);
> 
> -1U  ?
> 
Will replace 0xffffffff to -1U
> > +       writel(priv->timer_size, priv->base + STF_TIMER_LOAD);
> > +       /* Enable to start timer */
> > +       writel(1, priv->base + STF_TIMER_ENABLE);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id starfive_ids[] = {
> > +       { .compatible = "starfive,jh8100-timers" },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(jh8100_starfive_timer) = {
> > +       .name           = "jh8100_starfive_timer",
> 
Will use name " starfive_timer" instead of "jh8100_starfive_timer"
> What is jh8100 ? Do you need that?
> 
> > +       .id             = UCLASS_TIMER,
> > +       .of_match       = starfive_ids,
> > +       .probe          = starfive_probe,
> > +       .ops            = &starfive_ops,
> > +       .priv_auto      = sizeof(struct starfive_timer_priv),
> > +};
> > --
> > 2.34.1
> >
> 
> Regards,
> Simon