Hi Simon, Please be reminded that to give some review if it still has some issues.
> -----Original Message----- > From: KuanLim.Lee > Sent: Wednesday, October 4, 2023 5:49 PM > To: 'Simon Glass' <s...@google.com> > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang....@starfivetech.com> > Subject: RE: [PATCH] timer: starfive: Add Starfive timer support > > Hi Simon, > > > -----Original Message----- > > From: Simon Glass <s...@google.com> > > Sent: Wednesday, October 4, 2023 10:11 AM > > To: KuanLim.Lee <kuanlim....@starfivetech.com> > > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang....@starfivetech.com> > > Subject: Re: [PATCH] timer: starfive: Add Starfive timer support > > > > On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee > > <kuanlim....@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....@starfivetech.com> > > > Reviewed-by: Wei Liang Lim <weiliang....@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 <s...@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....@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