| Message ID | 20230919073049.1418-1-kuanlim.lee@starfivetech.com |
|---|---|
| State | Accepted |
| Commit | c202426d6ac6bffccf19e958dc176f7d29d0528e |
| Delegated to: | Andes |
| Headers | show |
| Series | timer: starfive: Add Starfive timer support | expand |
Can anyone help review this patch please? > -----Original Message----- > From: KuanLim.Lee <kuanlim.lee@starfivetech.com> > Sent: Tuesday, September 19, 2023 3:31 PM > To: u-boot@lists.denx.de > Cc: WeiLiang Lim <weiliang.lim@starfivetech.com>; KuanLim.Lee > <kuanlim.lee@starfivetech.com> > Subject: [PATCH] timer: starfive: Add Starfive timer support > > 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@starfivetech.com> > Reviewed-by: Wei Liang Lim <weiliang.lim@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 > > 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. > + > 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@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); } > + > +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)) > + 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 */ > + 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); > + 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", > + .id = UCLASS_TIMER, > + .of_match = starfive_ids, > + .probe = starfive_probe, > + .ops = &starfive_ops, > + .priv_auto = sizeof(struct starfive_timer_priv), > +}; > -- > 2.34.1
On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee <kuanlim.lee@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@starfivetech.com> > Reviewed-by: Wei Liang Lim <weiliang.lim@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@chromium.org> nits 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? > + > 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@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. > + > +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 > + 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 */ > + 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 ? > + 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", 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
Hi Simon, > -----Original Message----- > From: Simon Glass <sjg@google.com> > Sent: Wednesday, October 4, 2023 10:11 AM > To: KuanLim.Lee <kuanlim.lee@starfivetech.com> > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang.lim@starfivetech.com> > Subject: Re: [PATCH] timer: starfive: Add Starfive timer support > > On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee <kuanlim.lee@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@starfivetech.com> > > Reviewed-by: Wei Liang Lim <weiliang.lim@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@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@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
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@google.com> > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang.lim@starfivetech.com> > Subject: RE: [PATCH] timer: starfive: Add Starfive timer support > > Hi Simon, > > > -----Original Message----- > > From: Simon Glass <sjg@google.com> > > Sent: Wednesday, October 4, 2023 10:11 AM > > To: KuanLim.Lee <kuanlim.lee@starfivetech.com> > > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang.lim@starfivetech.com> > > Subject: Re: [PATCH] timer: starfive: Add Starfive timer support > > > > On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee > > <kuanlim.lee@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@starfivetech.com> > > > Reviewed-by: Wei Liang Lim <weiliang.lim@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@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@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
Hi, On Wed, 4 Oct 2023 at 03:49, KuanLim.Lee <kuanlim.lee@starfivetech.com> wrote: > > Hi Simon, > > > -----Original Message----- > > From: Simon Glass <sjg@google.com> > > Sent: Wednesday, October 4, 2023 10:11 AM > > To: KuanLim.Lee <kuanlim.lee@starfivetech.com> > > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang.lim@starfivetech.com> > > Subject: Re: [PATCH] timer: starfive: Add Starfive timer support > > > > On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee <kuanlim.lee@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@starfivetech.com> > > > Reviewed-by: Wei Liang Lim <weiliang.lim@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@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@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
Hi Simon, If there are no more issue, I will send out version 2 patch. > -----Original Message----- > From: Simon Glass <sjg@google.com> > Sent: Tuesday, October 10, 2023 9:45 PM > To: KuanLim.Lee <kuanlim.lee@starfivetech.com> > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang.lim@starfivetech.com> > Subject: Re: [PATCH] timer: starfive: Add Starfive timer support > > Hi, > > On Wed, 4 Oct 2023 at 03:49, KuanLim.Lee <kuanlim.lee@starfivetech.com> > wrote: > > > > Hi Simon, > > > > > -----Original Message----- > > > From: Simon Glass <sjg@google.com> > > > Sent: Wednesday, October 4, 2023 10:11 AM > > > To: KuanLim.Lee <kuanlim.lee@starfivetech.com> > > > Cc: u-boot@lists.denx.de; WeiLiang Lim > > > <weiliang.lim@starfivetech.com> > > > Subject: Re: [PATCH] timer: starfive: Add Starfive timer support > > > > > > On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee > > > <kuanlim.lee@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@starfivetech.com> > > > > Reviewed-by: Wei Liang Lim <weiliang.lim@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@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@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
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' <sjg@google.com> > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang.lim@starfivetech.com> > Subject: RE: [PATCH] timer: starfive: Add Starfive timer support > > Hi Simon, > > > -----Original Message----- > > From: Simon Glass <sjg@google.com> > > Sent: Wednesday, October 4, 2023 10:11 AM > > To: KuanLim.Lee <kuanlim.lee@starfivetech.com> > > Cc: u-boot@lists.denx.de; WeiLiang Lim <weiliang.lim@starfivetech.com> > > Subject: Re: [PATCH] timer: starfive: Add Starfive timer support > > > > On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee > > <kuanlim.lee@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@starfivetech.com> > > > Reviewed-by: Wei Liang Lim <weiliang.lim@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@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@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
On Tues, 10 Oct 2023 at 21:45, Simon Glass <sjg@google.com> wrote: > On Wed, 4 Oct 2023 at 03:49, KuanLim.Lee > <kuanlim.lee@starfivetech.com> > wrote: > > > > Hi Simon, > > On Wed, 4 Oct 2023 at 10:11, Simon Glass <sjg@google.com> wrote: > > > On Tue, 19 Sept 2023 at 06:08, Kuan Lim Lee > > > <kuanlim.lee@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@starfivetech.com> > > > > Reviewed-by: Wei Liang Lim <weiliang.lim@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@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@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 Hi Simon, Please tell me if I am doing something wrong, I will do my best to correct it. Best Regards, KuanLim Lee
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. + 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@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); +} + +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)) + 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 */ + 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); + 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", + .id = UCLASS_TIMER, + .of_match = starfive_ids, + .probe = starfive_probe, + .ops = &starfive_ops, + .priv_auto = sizeof(struct starfive_timer_priv), +};