[PATCH] timer: starfive: Add Starfive timer support
Simon Glass
sjg at google.com
Tue Oct 10 15:44:44 CEST 2023
Hi,
On Wed, 4 Oct 2023 at 03:49, KuanLim.Lee <kuanlim.lee at starfivetech.com> wrote:
>
> 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?
they are the things I mentioned below
> >
> > >
> > > 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.
OK so could you put these details in the help?
> >
> > > +
> > > 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.
No it isn't. It is useful for bootstage though. I suspect you can read
DT early, though.
> > > +
> > > +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