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

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

[Jesse Taube]

From: Mr-Bossman <mr.bossman075@gmail.com>

This timer driver is using GPT Timer (General Purpose Timer) available
on almost all i.MX SoCs family.

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 | 134 ++++++++++++++++++++++++++++++++++
 3 files changed, 142 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..8b6a6200c5
--- /dev/null
+++ b/drivers/timer/imx-gpt-timer.c
@@ -0,0 +1,134 @@
+// 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
+
+#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)
+
+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);
+
+	/* Is  timer clock rate valid, If not go to 24MHz clock*/
+	if ((int)rate <= 0) {
+		dev_err(dev, "Could not get clock rate...\n");
+		return -EINVAL;
+	}
+	/* Only support 24MHz clock */
+	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;
+	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);
+	/* 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,
+};
-- 
2.30.0

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

[Giulio Benetti]

Hi Jesse,

On 2/9/21 9:42 PM, Jesse Taube wrote:
> From: Mr-Bossman <mr.bossman075@gmail.com>
> 
> This timer driver is using GPT Timer (General Purpose Timer) available
> on almost all i.MX SoCs family.

Please provide more explanation about costraints it has. It works only
with a source clock at 24Mhz. So something like:
'''
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 timer counter and most of i.MX* SoCs use 24Mhz crystal
source so let's deal only that specific source.
'''

> 
> 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 | 134 ++++++++++++++++++++++++++++++++++
>   3 files changed, 142 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..8b6a6200c5
> --- /dev/null
> +++ b/drivers/timer/imx-gpt-timer.c
> @@ -0,0 +1,134 @@
> +// 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>
> +

Please remove newline ^^^

> +#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

Please keep the same tabs above as below

> +#define GPT_PR_PRESCALER		0x00000FFF
> +#define GPT_PR_PRESCALER24M		0x0000F000
> +
> +#define NO_CLOCK		(0)
> +#define IPG_CLK			(1 << 6)

ditto

> +#define IPG_CLK_HF		(2 << 6)
> +#define IPG_EXT			(3 << 6)

ditto

> +#define IPG_CLK_32K		(4 << 6)
> +#define IPG_CLK_24M		(5 << 6)
> +
> +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 */

This is a useless comment, don't describe talking functions

> +	rate = clk_get_rate(&clk);
> +
> +	/* Is  timer clock rate valid, If not go to 24MHz clock*/

ditto and 24Mhz is dealt after

-- 
Giulio Benetti
Benetti Engineering sas

> +	if ((int)rate <= 0) {
> +		dev_err(dev, "Could not get clock rate...\n");
> +		return -EINVAL;
> +	}
> +	/* Only support 24MHz clock */
> +	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 */

Capital /* We ... */

> +	prescaler = (rate / CONFIG_SYS_HZ_CLOCK) - 1;
> +	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);
> +	/* start timer */

Capital /* Start ... */

Also, you're resending 4/4 only of this patchset but we would expect 
having a new patchset generated with:
# git format-patch -M -s -o outgoing -4 --cover-letter -v2

-v2 tag every patch as the second iteration of this patchset and it's 
useful to distinguish new patches from old ones.

Also, for every patch you have to indicate what changes from v1->v2.

You should put this infos after commit log end and the 3 dashes like:
---
V1->V2:
* changed this
* modified this
* etc.
---

Of course same goes for V2->V3/while keeping V1->V2 and so on until it's 
accepted. If not all patches have been accepted then next time send only 
the not accepted ones.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> +	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,
> +};
>