[PATCH] timer: starfive: Add Starfive timer support
KuanLim.Lee
kuanlim.lee at starfivetech.com
Wed Oct 11 06:36:22 CEST 2023
Hi Simon,
If there are no more issue, I will send out version 2 patch.
> -----Original Message-----
> From: Simon Glass <sjg at google.com>
> Sent: Tuesday, October 10, 2023 9:45 PM
> 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
>
> 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?
Timer0 bias = 0x00, Timer1 bias = 0x40, Timer2 bias = 0x80, Timer3 bias = 0xc0
Register:
TIMER Interrupts Status (offset = 0x00)
Bit0 - timer_int_ch0 - default: 0x00
Bit1 - timer_int_ch1 - default: 0x00
Bit2 - timer_int_ch2 - default: 0x00
Bit3 - timer_int_ch3 - default: 0x00
TIMER Control Register (offset = 0x04+bias)
Bit[0] - default: 0x0
0: continuous run, 1: single run
Single run mode will stop after the TIMER Value Register is counted to zero and reloaded.
Continuous run mode will keep on running after the TIMER Value Register is counted to zero and reloaded.
TIMER Load Register (offset = 0x08+bias)
Bit[31:0] - default: 0xffffffff
The value is always reloaded into TIMER Value Register
TIMER Enable Register (offset = 0x10+bias)
Bit[0] - default: 0x0
0: disable, 1: enable
TIMER Reload Register (offset = 0x14+bias)
Bit[0] - default: 0x0
Write this register to reload preset value to counter. (Write 0 and 1 are both ok)
Read this register always get '0'.
TIMER Value Register (offset = 0x18+bias)
Bit[31:0] - default: 0xffffffff
Indicates value of the 32-bit counter. Read-Only Register.
TIMER Interrupt Status/Clear Register (offset = 0x20+bias)
Bit[1] - default: 0x0. "timer_int_clr_busy". Read-Only bit.
'1' means that it is clearing interrupt. Bit 0 can NOT be written.
'0' means Bit 0 can be written to clear interrupt.
Bit[0] - default: 0x0. "timer_int_clear_status". Read-Write bit.
Read value represent channel interrupt status.
Write 1 to this bit clear interrupt. Write 0 has no effects.
TIMER Interrupt Mask Register (offset = 0x24+bias)
Bit[0] - default: 0x1
Interrupt Mask Register. 0: Unmask, 1: Mask
It must be unmasked to before using the timer channel.
Current device tree will put the clock source details. So that, driver can get the clock source frequency of
the channel by clk_get_by_index () and clk_get_rate().
clock-names = "ch0", "ch1", "ch2", "ch3", "apb";
I don’t use interrupt here and only choose one of the timer channels to use.
Please tell me if these details are not sufficient.
>
> > >
> > > > +
> > > > 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.
>
Current driver is working well and not going to change in current stage.
I will enhance the driver if need to trace time before driver model.
> > > > +
> > > > +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