[PATCH v3 1/1] timer: imx-gpt: Add timer support for i.MX SoCs family
Giulio Benetti
giulio.benetti at benettiengineering.com
Fri Feb 12 11:13:25 CET 2021
+Cc Sean
On 2/12/21 2:11 AM, Jesse wrote:
> From: Jesse Taube <mr.bossman075 at gmail.com>
>
> This timer driver is using GPT Timer (General Purpose Timer)
> available on almost all i.MX SoCs family.
> Since this driver is only meant to provide u-boot's timer and counter,
> and most of the i.MX* SoCs use a 24Mhz crystal,
> let's only deal with that specific source.
>
> Signed-off-by: Giulio Benetti <giulio.benetti at benettiengineering.com>
> Signed-off-by: Jesse Taube <mr.bossman075 at gmail.com>
> ---
Here it should go what you've listed in cover letter. Changes must be
listed patch per patch. So here you add:
V1->V2:
...
V2->3:
...
---
Note that lines between triple dashes are ignored when applying patch,
so that doesn't impact the final result of the patch.
> drivers/timer/Kconfig | 7 ++
> drivers/timer/Makefile | 1 +
> drivers/timer/imx-gpt-timer.c | 149 ++++++++++++++++++++++++++++++++++
> 3 files changed, 157 insertions(+)
> create mode 100644 drivers/timer/imx-gpt-timer.c
>
> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> index 80743a2551..ee81dfa776 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -227,4 +227,11 @@ config MCHP_PIT64B_TIMER
> Select this to enable support for Microchip 64-bit periodic
> interval timer.
>
> +config IMX_GPT_TIMER
> + bool "NXP i.MX GPT timer support"
> + depends on TIMER
> + help
> + Select this to enable support for the timer found on
> + NXP i.MX devices.
> +
> endmenu
> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
> index eb5c48cc6c..e214ba7268 100644
> --- a/drivers/timer/Makefile
> +++ b/drivers/timer/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_STM32_TIMER) += stm32_timer.o
> obj-$(CONFIG_X86_TSC_TIMER) += tsc_timer.o
> 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
> diff --git a/drivers/timer/imx-gpt-timer.c b/drivers/timer/imx-gpt-timer.c
> new file mode 100644
> index 0000000000..62db6e663a
> --- /dev/null
> +++ b/drivers/timer/imx-gpt-timer.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2020
> + * Author(s): Giulio Benetti <giulio.benetti at benettiengineering.com>
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <timer.h>
> +#include <dm/device_compat.h>
> +
> +#include <asm/io.h>
> +
> +#define GPT_CR_SWR 0x00008000
> +#define GPT_CR_CLKSRC 0x000001C0
> +#define GPT_CR_EN_24M 0x00000400
> +#define GPT_CR_EN 0x00000001
> +#define GPT_PR_PRESCALER 0x00000FFF
> +#define GPT_PR_PRESCALER24M 0x0000F000
> +
> +#define NO_CLOCK (0)
> +#define IPG_CLK (1 << 6)
this ^^^ and
> +#define IPG_CLK_HF (2 << 6)
> +#define IPG_EXT (3 << 6)
this ^^^ have wrong tab number. Check editor settings to tabs=8 spaces.
I use mceditor and there tabs and spaces are highlighted, try to check
with that for example.
> +#define IPG_CLK_32K (4 << 6)
> +#define IPG_CLK_24M (5 << 6)
Here ^^^ it would be better to add a prefix like GPT_CLKSRC_ to any
value. Also, please don't list macros that are not used. So you should
only define GPT_CLKSRC_IPG_CLK and GPT_CLKSRC_IPG_CLK_24M.
> +
> +struct imx_gpt_timer_regs {
> + u32 cr;
> + u32 pr;
> + u32 sr;
> + u32 ir;
> + u32 ocr1;
> + u32 ocr2;
> + u32 ocr3;
> + u32 icr1;
> + u32 icr2;
> + u32 cnt;
> +};
> +
> +struct imx_gpt_timer_priv {
> + struct imx_gpt_timer_regs *base;
> +};
> +
> +static u64 imx_gpt_timer_get_count(struct udevice *dev)
> +{
> + struct imx_gpt_timer_priv *priv = dev_get_priv(dev);
> + struct imx_gpt_timer_regs *regs = priv->base;
> +
> + return readl(®s->cnt);
> +}
> +
> +u32 imxrt_gpt_setup_ctrl(u32 rate)
imxrt_gpt_ should be imx_gpt_timer_ and must be static since it's private.
> +{
> + u32 ctlr = 0;
> +
> + if (rate == 24000000UL) {
> + ctlr |= IPG_CLK_24M;
> + ctlr |= GPT_CR_EN_24M;
This should be a one line OR. And that way you can remove brackets since
there are only 1 statement per case.
> + } else {
> + ctlr |= IPG_CLK;
> + }
see above for brackets
> +
> + return ctlr;
> +}
> +
> +static int imx_gpt_timer_probe(struct udevice *dev)
> +{
> + struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> + struct imx_gpt_timer_priv *priv = dev_get_priv(dev);
> + struct imx_gpt_timer_regs *regs;
> + struct clk clk;
> + fdt_addr_t addr;
> + u32 prescaler;
> + u32 rate;
> + u32 ctlr;
> + int ret;
> +
> + addr = dev_read_addr(dev);
> + if (addr == FDT_ADDR_T_NONE)
> + return -EINVAL;
> +
> + priv->base = (struct imx_gpt_timer_regs *)addr;
> +
> + ret = clk_get_by_index(dev, 0, &clk);
> + if (ret < 0)
> + return ret;
> +
> + ret = clk_enable(&clk);
> + if (ret) {
> + dev_err(dev, "Failed to enable clock\n");
> + return ret;
> + }
> +
> + regs = priv->base;
> +
> + /* Reset the timer */
> + setbits_le32(®s->cr, GPT_CR_SWR);
> +
> + /* Wait for timer to finish reset */
> + while (readl(®s->cr) & GPT_CR_SWR)
> + ;
> +
> + /* Get timer clock rate */
> + rate = clk_get_rate(&clk);
> + if (rate <= 0) {
> + dev_err(dev, "Could not get clock rate...\n");
> + return -EINVAL;
> + }
> +
> + /* Set timer frequency to 1MHz */
> + uc_priv->clock_rate = CONFIG_SYS_HZ_CLOCK;
> + prescaler = (rate / CONFIG_SYS_HZ_CLOCK) - 1;
> +
> + if (prescaler > 0xfff) {
> + dev_err(dev, "Could not set prescaler...\n");
> + return -EINVAL;
> + }
> + /* We set timer prescaler to obtain a 1MHz timer counter frequency */
> + writel(prescaler, ®s->pr);
prescaler setting regards only !24M way. So you should check for
rate!=24M and set 24M prescaler otherwise. Obviously it's better to
create a function for doing this. So...
> + ctlr = imxrt_gpt_setup_ctrl(rate);
> +
> + /* Start timer */
> + writel(ctlr, ®s->cr);
> + setbits_le32(®s->cr, GPT_CR_EN);
I would create a function imx_gpt_timer_setup(). Inside that function I
would call 2 different functions named imx_gpt_timer_setup_24M() and
imx_gpt_timer_setup_per(). Inside them I would set registers according
to the 2 different cases. Of course all common registers writing, like
when you Start timer, I would list them before and after checking rate
and calling specific functions, something like:
int imx_gpt_timer_setup()
{
<Reset Timer regs write code>
if (rate == 24000000UL)
imx_gpt_timer_setup_24M();
else
imx_gpt_timer_setup_per();
<Start Timer regs write code>
}
and call it from probe where now you directly set registers.
What about this?
Best regards
--
Giulio Benetti
Benetti Engineering sas
> + return 0;
> +}
> +
> +static const struct timer_ops imx_gpt_timer_ops = {
> + .get_count = imx_gpt_timer_get_count,
> +};
> +
> +static const struct udevice_id imx_gpt_timer_ids[] = {
> + { .compatible = "fsl,imxrt-gpt" },
> + {}
> +};
> +
> +U_BOOT_DRIVER(imx_gpt_timer) = {
> + .name = "imx_gpt_timer",
> + .id = UCLASS_TIMER,
> + .of_match = imx_gpt_timer_ids,
> + .priv_auto = sizeof(struct imx_gpt_timer_priv),
> + .probe = imx_gpt_timer_probe,
> + .ops = &imx_gpt_timer_ops,
> +};
>
More information about the U-Boot
mailing list