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

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

[Jesse]

From: Jesse Taube <mr.bossman075@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.

Jesse Taube (1):
  timer: imx-gpt: Add timer support for i.MX SoCs family

 drivers/timer/Kconfig         |   7 ++
 drivers/timer/Makefile        |   1 +
 drivers/timer/imx-gpt-timer.c | 153 ++++++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+)
 create mode 100644 drivers/timer/imx-gpt-timer.c
---
V1->V2:
* Fixed indentation
* Fixed capitals
* Made timer work on only 24MHz clock
V2->V3:
* Fixed indentation
* Made implementation imatate the Linux kernel
* Fix wrong definition
V3->V4:
* Fixed indentation
* Made bit manipluation into its own function.
---
-- 
2.30.0

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

[Jesse]

From: Jesse Taube <mr.bossman075@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@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 | 153 ++++++++++++++++++++++++++++++++++
 3 files changed, 161 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..e7e46c8037
--- /dev/null
+++ b/drivers/timer/imx-gpt-timer.c
@@ -0,0 +1,153 @@
+// 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_EN_24M			0x00000400
+#define GPT_CR_EN			0x00000001
+#define GPT_PR_PRESCALER		0x00000FFF
+
+#define GPT_CLKSRC_IPG_CLK		(1 << 6)
+#define GPT_CLKSRC_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 inline void imx_gpt_setup_24M(struct imx_gpt_timer_regs *regs)
+{
+	writel(GPT_CLKSRC_IPG_CLK_24M | GPT_CR_EN_24M, &regs->cr);
+}
+
+static inline void imx_gpt_setup_per(struct imx_gpt_timer_regs *regs)
+{
+	writel(GPT_CLKSRC_IPG_CLK, &regs->cr);
+}
+
+static inline void imx_gpt_sof_reset(struct imx_gpt_timer_regs *regs)
+{
+	/* Reset the timer */
+	setbits_le32(&regs->cr, GPT_CR_SWR);
+
+	/* Wait for timer to finish reset */
+	while (readl(&regs->cr) & GPT_CR_SWR)
+		;
+}
+
+static int imx_gpt_setup(u32 rate, struct imx_gpt_timer_regs *regs)
+{
+	u32 prescaler;
+
+	imx_gpt_sof_reset(regs);
+
+	/* Set timer frequency to 1MHz */
+	prescaler = (rate / CONFIG_SYS_HZ_CLOCK) - 1;
+
+	if (prescaler > GPT_PR_PRESCALER)
+		return -EINVAL;
+	/* We set timer prescaler to obtain a 1MHz timer counter frequency */
+	writel(prescaler, &regs->pr);
+
+	if (rate != 24000000UL)
+		imx_gpt_setup_per(regs);
+	else
+		imx_gpt_setup_24M(regs);
+
+	/* Start timer */
+	setbits_le32(&regs->cr, GPT_CR_EN);
+
+	return 0;
+}
+
+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 rate;
+	int ret;
+
+	addr = dev_read_addr(dev);
+	if (addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+
+	priv->base = (struct imx_gpt_timer_regs *)addr;
+	regs = priv->base;
+
+	uc_priv->clock_rate = CONFIG_SYS_HZ_CLOCK;
+
+	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;
+	}
+	/* Get timer clock rate */
+	rate = clk_get_rate(&clk);
+	if (rate <= 0) {
+		dev_err(dev, "Could not get clock rate...\n");
+		return -EINVAL;
+	}
+
+	ret = imx_gpt_setup(rate, regs);
+	if (ret) {
+		dev_err(dev, "Could not setup timer\n");
+		return ret;
+	}
+	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 v4 0/1] timer: imx-gpt: Add timer support for i.MX SoCs family

[Giulio Benetti]

Hi Jesse,

On 2/14/21 4:55 AM, Jesse wrote:
> From: Jesse Taube <mr.bossman075@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.
> 
> Jesse Taube (1):
>    timer: imx-gpt: Add timer support for i.MX SoCs family

Since you're only sending a patch you should not include a cover letter. 
A cover letter should be included when your patches are more than one 
and they deal the same subject. So here you should only write a summary 
about it, like:
'''
Subject: Add i.MX GPT timer support

This pachset adds i.MX GPT timer support and a modify to dts for 
imxrt1050-ekv that enables you to check its functionality.
'''

>   drivers/timer/Kconfig         |   7 ++
>   drivers/timer/Makefile        |   1 +
>   drivers/timer/imx-gpt-timer.c | 153 ++++++++++++++++++++++++++++++++++
>   3 files changed, 161 insertions(+)
>   create mode 100644 drivers/timer/imx-gpt-timer.c
> ---
> V1->V2:
> * Fixed indentation
> * Fixed capitals
> * Made timer work on only 24MHz clock
> V2->V3:
> * Fixed indentation
> * Made implementation imatate the Linux kernel
> * Fix wrong definition
> V3->V4:
> * Fixed indentation
> * Made bit manipluation into its own function.
> ---

You should move ^^^ these changes as they are in patch 1/1.

Also take care that this patch is difficult to test alone since a 
defconfig(this is easy to enable) and a dts(this is really welcome) are 
missing, then you should also include at least the dts you were working 
at least. And that makes sense to have a patchset again then(because of 
2 patches).

-- 
Giulio Benetti
Benetti Engineering sas

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

[Giulio Benetti]

On 2/14/21 4:55 AM, Jesse wrote:
> From: Jesse Taube <mr.bossman075@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.

Still too many columns ^^^

> 
> 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 | 153 ++++++++++++++++++++++++++++++++++
>   3 files changed, 161 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..e7e46c8037
> --- /dev/null
> +++ b/drivers/timer/imx-gpt-timer.c
> @@ -0,0 +1,153 @@
> +// 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_EN_24M			0x00000400

Too any tabs                               ^^^^

> +#define GPT_CR_EN			0x00000001
> +#define GPT_PR_PRESCALER		0x00000FFF
> +
> +#define GPT_CLKSRC_IPG_CLK		(1 << 6)
> +#define GPT_CLKSRC_IPG_CLK_24M		(5 << 6)

Ditto 					  ^^^

> +
> +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 inline void imx_gpt_setup_24M(struct imx_gpt_timer_regs *regs)
> +{
> +	writel(GPT_CLKSRC_IPG_CLK_24M | GPT_CR_EN_24M, &regs->cr);
> +}
> +
> +static inline void imx_gpt_setup_per(struct imx_gpt_timer_regs *regs)
> +{
> +	writel(GPT_CLKSRC_IPG_CLK, &regs->cr);
> +}
> +
> +static inline void imx_gpt_sof_reset(struct imx_gpt_timer_regs *regs)
> +{
> +	/* Reset the timer */
> +	setbits_le32(&regs->cr, GPT_CR_SWR);
> +
> +	/* Wait for timer to finish reset */
> +	while (readl(&regs->cr) & GPT_CR_SWR)
> +		;
> +}
> +
> +static int imx_gpt_setup(u32 rate, struct imx_gpt_timer_regs *regs)
> +{
> +	u32 prescaler;
> +
> +	imx_gpt_sof_reset(regs);

This function ^^^ can be avoided at the moment since it's called only 
from here and it only set a bit and loop-check it.

> +
> +	/* Set timer frequency to 1MHz */
> +	prescaler = (rate / CONFIG_SYS_HZ_CLOCK) - 1;
> +
> +	if (prescaler > GPT_PR_PRESCALER)
> +		return -EINVAL;
> +	/* We set timer prescaler to obtain a 1MHz timer counter frequency */
> +	writel(prescaler, &regs->pr);

Prescaler setting is specific to PER case and should go inside the two 
functions imx_gpt_setup_per/24M().

> +
> +	if (rate != 24000000UL)
> +		imx_gpt_setup_per(regs);

...                   this ^^^

> +	else
> +		imx_gpt_setup_24M(regs);

...                and this ^^^


Also, I've taken a look again at linux driver[1] and like here in u-boot 
they set as prescaler to 8 for 24M to obtina 3Mhz. And that is for 
compatibility with u-boot I think, so here I would try to set prescaler 
for having timer frequency set to 3Mhz instead of 1Mhz. And this could 
be true for the PER clock source, but of course you have to set 24M 
prescaler(to 8) or PER prescaler(the one you use) the way already do.

[1]: 
https://github.com/torvalds/linux/blob/master/drivers/clocksource/timer-imx-gpt.c#L301-L312

For the rest it looks good. On next iteration, patch should be ready to 
be tested on my imxrt1050-evk.

Thank you
Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> +
> +	/* Start timer */
> +	setbits_le32(&regs->cr, GPT_CR_EN);
> +
> +	return 0;
> +}
> +
> +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 rate;
> +	int ret;
> +
> +	addr = dev_read_addr(dev);
> +	if (addr == FDT_ADDR_T_NONE)
> +		return -EINVAL;
> +
> +	priv->base = (struct imx_gpt_timer_regs *)addr;
> +	regs = priv->base;
> +
> +	uc_priv->clock_rate = CONFIG_SYS_HZ_CLOCK;
> +
> +	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;
> +	}
> +	/* Get timer clock rate */
> +	rate = clk_get_rate(&clk);
> +	if (rate <= 0) {
> +		dev_err(dev, "Could not get clock rate...\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = imx_gpt_setup(rate, regs);
> +	if (ret) {
> +		dev_err(dev, "Could not setup timer\n");
> +		return ret;
> +	}
> +	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,
> +};
>