[PATCH] timer: starfive: Add Starfive timer support

KuanLim.Lee kuanlim.lee at starfivetech.com
Tue Oct 10 10:11:19 CEST 2023


Hi Simon,

Do my questions and replies are proper? Sorry for this time is my first time upstream.

> -----Original Message-----
> From: KuanLim.Lee
> Sent: Wednesday, October 4, 2023 5:49 PM
> To: 'Simon Glass' <sjg at google.com>
> Cc: u-boot at lists.denx.de; WeiLiang Lim <weiliang.lim at starfivetech.com>
> Subject: RE: [PATCH] timer: starfive: Add Starfive timer support
> 
> 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


More information about the U-Boot mailing list