[PATCH v2 1/1] timer: imx-gpt: Add timer support for i.MX SoCs family

[Giulio Benetti <giulio.benetti@benettiengineering.com>]

On 2/10/21 1:04 AM, Jesse Taube wrote:
> 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.

We have a problem with columns on commit log, they shouldn't be longer 
than 72 cols, so please check the editor you're using for commit log 
writing and set 72 cols costrains. I use nano and you only need to set 
in .gitconfig under [core]:
editor = nano -b -r 72

This way nano automatically put a CR.

> 
> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> Signed-off-by: Jesse Taube <mr.bossman075@gmail.com>
> 
> ---
>   drivers/timer/Kconfig         |   7 ++
>   drivers/timer/Makefile        |   1 +
>   drivers/timer/imx-gpt-timer.c | 132 ++++++++++++++++++++++++++++++++++
>   3 files changed, 140 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..58c37db300
> --- /dev/null
> +++ b/drivers/timer/imx-gpt-timer.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2020
> + * Author(s): Giulio Benetti <giulio.benetti@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			0x00004000
> +#define GPT_CR_EN				0x00000001
> +#define GPT_PR_PRESCALER		0x00000FFF
> +#define GPT_PR_PRESCALER24M		0x0000F000

Here ^^^ and

> +#define NO_CLOCK				(0)
> +#define IPG_CLK					(1 << 6)
> +#define IPG_CLK_HF				(2 << 6)
> +#define IPG_EXT					(3 << 6)
> +#define IPG_CLK_32K				(4 << 6)
> +#define IPG_CLK_24M				(5 << 6)

here you still have different tab numbers. Enable in your editor the
option to show whitespaces and tabs, that way everything it easier.

> +
> +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(&regs->cnt);
> +}
> +
> +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;
> +	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(&regs->cr, GPT_CR_SWR);
> +
> +	/* Wait for timer to finish reset */
> +	while (readl(&regs->cr) & GPT_CR_SWR)
> +		;
> +
> +	/* Get timer clock rate */
> +	rate = clk_get_rate(&clk);

clk_get_rate() has a wrong description in include/clk.h saying:
"@return clock rate in Hz, or -ve error code."

This                            ^^^ is wrong.
If you dig into drivers/clk/clk-uclass.c and look for clk_get_rate(), 
you will see that it returns 0 on error and > 0 if succesfull.

I've just sent a patch for this:
https://patchwork.ozlabs.org/project/uboot/patch/20210210173722.4823-1-giulio.benetti at benettiengineering.com/

> +	if ((int)rate <= 0) {

This        ^^^^ is a cast trying to solve the problem above but it's 
not correct. clk_get_rate() returns ulong, not int, so modify "int rate" 
into "ulong rate".

> +		dev_err(dev, "Could not get clock rate...\n");
> +		return -EINVAL;
> +	}
> +	/* Only support 24MHz clock */

Extend to /* Only support 24MHz crystal clock */ otherwise it seems that 
every 24Mhz clock is accepted and it's not that way.

I don't know a sure way to be bounded to crystal clock, suggestions are 
welcome.

> +	if (rate != 24000000UL) {
> +		dev_err(dev, "Clock rate other than 24MHz not supported...\n");
> +		return -EINVAL;
> +	}
> +	/* We set timer prescaler to obtain a 1MHz timer counter frequency */
> +	prescaler = (rate / CONFIG_SYS_HZ_CLOCK) - 1;

Here you should check against maximum value of PRESCALER(0xFFF), if 
prescaler is > 0xFFF then you have to return with error.

> +	writel(GPT_PR_PRESCALER & prescaler, &regs->pr);
> +	/* Set timer frequency to 1MHz */
> +	uc_priv->clock_rate = CONFIG_SYS_HZ_CLOCK;
> +
> +	clrbits_le32(&regs->cr, GPT_CR_CLKSRC);
> +	setbits_le32(&regs->cr, IPG_CLK);

Here IPG_CLK should be IPG_CLK_24M as discussed previously.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> +	/* Start timer */
> +	setbits_le32(&regs->cr, GPT_CR_EN);
> +
> +	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,
> +};
>