[PATCH v2 0/1] Add driver support for the IMX General Purpose Timer (GPT) available

[PATCH v2 0/1] Add driver support for the IMX General Purpose Timer (GPT) available

[Jesse Taube]

Giulio Benetti (3):
  timer: imx-gpt: Add timer support for i.MX SoCs family

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 | 132 ++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+)
 create mode 100644 drivers/timer/imx-gpt-timer.c

---
V1->V2:
* Fixed indentation
* Fixed capitals
* Made timer work on only 24MHz clock
---
-- 
2.30.0

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

[Jesse Taube]

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 | 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
+
+#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);
+	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 v2 1/1] timer: imx-gpt: Add timer support for i.MX SoCs family

[Giulio Benetti]

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

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

[Sean Anderson]

On 2/10/21 12:49 PM, Giulio Benetti wrote:
 > 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.

Why is it necessary? What are the constraints which require only using
the 24MHz reference?

--Sean

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

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

[Giulio Benetti]

On 2/10/21 6:52 PM, Sean Anderson wrote:
> 
> 
> On 2/10/21 12:49 PM, Giulio Benetti wrote:
>   > 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.
> 
> Why is it necessary? What are the constraints which require only using
> the 24MHz reference?

To tell the truth there are no constraints, the reason is that I'm 
trying to keep this driver compatible and reusable by imx6* and imx7* 
since they setup the timer this way:
https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-imx/timer.c#L80-99

But if this timer needs to support any kind of clock source then let's 
modify it, but it gets more complicated and I think it could be done if 
needed.

-- 
Giulio Benetti
Benetti Engineering sas

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

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

[Giulio Benetti]

Hi Jesse, Sean,

On 2/10/21 9:13 PM, Giulio Benetti wrote:
[SNIP]
>>    >
>>    >
>>    >> +    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.
>>
>> Why is it necessary? What are the constraints which require only using
>> the 24MHz reference?
> 
> To tell the truth there are no constraints, the reason is that I'm
> trying to keep this driver compatible and reusable by imx6* and imx7*
> since they setup the timer this way:
> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-imx/timer.c#L80-99
> 
> But if this timer needs to support any kind of clock source then let's
> modify it, but it gets more complicated and I think it could be done if
> needed.

This is the Linux way to make it work even if clock source is not 24Mhz:
https://github.com/torvalds/linux/blob/master/drivers/clocksource/timer-imx-gpt.c#L314-L329

It checks if clock source is 24Mhz and set CLKSRC(named V2_TPRER_PRE24M) 
to IPG_CLK_24M(named MXC_TPRER). Otherwise it uses peripheral clock and 
that's all.

At this point it's easier than I thought.

Jesse, can you please add that handling imitating Linux driver?

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

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

[Giulio Benetti]

Hi Jesse,

I re-add all people and ML in Cc so they can follow. Next time reply to all.

On 2/10/21 8:00 PM, Jesse Taube wrote:
> 
> On 2/10/21 12:49 PM, Giulio Benetti wrote:
>> 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.
> I still don't exactly understand what you mean here. Should I press tab
> 3 times exactly instead of making the values line up. 

You need to check using an editor with whitespaces and tabs options 
enabled and tabs set to 8 spaces how that code looks like.

> In the commit log
> it seems to change weirdly because of the '+' in front of the define.

Here it is the same, this should be due to the editor you use for commit 
log.

>>> +
>>> +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".
> Ah this makes sense now.
>>
>>> +??????? 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.
> 
> The IPG_CLK_24M needs a different prescaler and a second enable bit.
> This will completely bypass all other clock sources making the check for
> the clock rate useless. 

Yes, in the operative way yes, you could also avoid passing clock source 
through dts, but this way we check that the right clock source is passed 
to dts, and that is the correct way to work I think.

> It will also mean that even if we don't have a
> correct clock source it will work at the correct timing.

Yes if they provide 24Mhz.

I wanted it to be like that at the moment, because a lot of imx SoCs 
setup GPT like that. Take a look here:
https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-imx/timer.c#L80-99

This way the imx6* and imx7 could add their .compatible to this driver 
in the future. If someone will need some specific tweaking then they 
would send a patch adding such new DT property, like using 32K source.
But that comes next IMHO, otherwise we should describe entirely GPT 
peripheral but what we need at the moment is getting 1ms tick like lot 
of other imx SoCs already do.

The other chance would be to treat all the clock sources possibilities,
but, at least for me, to begin this is sufficient and can be improved later.

> I changed it to use only the 24MHz clock and ignore all others,

Ok

> at some
> point it would be nice to have it only as a backup clock if the ccm was
> not configured.

I don't understand what you mean with backup clock can you elaborate more?

Thank you

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

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

[Jesse T]

On Wed, Feb 10, 2021 at 3:09 PM Giulio Benetti <
giulio.benetti@benettiengineering.com> wrote:

> Hi Jesse,
>
> I re-add all people and ML in Cc so they can follow. Next time reply to
> all.
>
> On 2/10/21 8:00 PM, Jesse Taube wrote:
> >
> > On 2/10/21 12:49 PM, Giulio Benetti wrote:
> >> 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.
> > I still don't exactly understand what you mean here. Should I press tab
> > 3 times exactly instead of making the values line up.
>
> You need to check using an editor with whitespaces and tabs options
> enabled and tabs set to 8 spaces how that code looks like.
>
> > In the commit log
> > it seems to change weirdly because of the '+' in front of the define.
>
> Here it is the same, this should be due to the editor you use for commit
> log.
>
> >>> +
> >>> +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".
> > Ah this makes sense now.
> >>
> >>> +        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.
> >
> > The IPG_CLK_24M needs a different prescaler and a second enable bit.
> > This will completely bypass all other clock sources making the check for
> > the clock rate useless.
>
> Yes, in the operative way yes, you could also avoid passing clock source
> through dts, but this way we check that the right clock source is passed
> to dts, and that is the correct way to work I think.
>
> > It will also mean that even if we don't have a
> > correct clock source it will work at the correct timing.
>
> Yes if they provide 24Mhz.
>
> I wanted it to be like that at the moment, because a lot of imx SoCs
> setup GPT like that. Take a look here:
>
> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-imx/timer.c#L80-99
>
> This way the imx6* and imx7 could add their .compatible to this driver
> in the future. If someone will need some specific tweaking then they
> would send a patch adding such new DT property, like using 32K source.
> But that comes next IMHO, otherwise we should describe entirely GPT
> peripheral but what we need at the moment is getting 1ms tick like lot
> of other imx SoCs already do.
>
> The other chance would be to treat all the clock sources possibilities,
> but, at least for me, to begin this is sufficient and can be improved
> later.
>
> > I changed it to use only the 24MHz clock and ignore all others,
>
> Ok
>
> > at some
> > point it would be nice to have it only as a backup clock if the ccm was
> > not configured.
>
> I don't understand what you mean with backup clock can you elaborate more?
>
If we have a clock source that is 0, we can still use the 24MHz clock as
that is an always known source, and isn't controlled by the ccm. Therefore
if we have a dummy clock the soc will still have delays and timeouts at the
right time. But this would make it so that we never return an error from
clk_get_rate(&clk);

> Thank you
>
> Best regards
> --
> Giulio Benetti
> Benetti Engineering sas
>

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

[Giulio Benetti]

On 2/11/21 7:54 PM, Jesse T wrote:
[SNIP]
>      >
>      > The IPG_CLK_24M needs a different prescaler and a second enable bit.
>      > This will completely bypass all other clock sources making the
>     check for
>      > the clock rate useless.
> 
>     Yes, in the operative way yes, you could also avoid passing clock
>     source
>     through dts, but this way we check that the right clock source is
>     passed
>     to dts, and that is the correct way to work I think.
> 
>      > It will also mean that even if we don't have a
>      > correct clock source it will work at the correct timing.
> 
>     Yes if they provide 24Mhz.
> 
>     I wanted it to be like that at the moment, because a lot of imx SoCs
>     setup GPT like that. Take a look here:
>     https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-imx/timer.c#L80-99
> 
>     This way the imx6* and imx7 could add their .compatible to this driver
>     in the future. If someone will need some specific tweaking then they
>     would send a patch adding such new DT property, like using 32K source.
>     But that comes next IMHO, otherwise we should describe entirely GPT
>     peripheral but what we need at the moment is getting 1ms tick like lot
>     of other imx SoCs already do.
> 
>     The other chance would be to treat all the clock sources possibilities,
>     but, at least for me, to begin this is sufficient and can be
>     improved later.
> 
>      > I changed it to use only the 24MHz clock and ignore all others,
> 
>     Ok
> 
>      > at some
>      > point it would be nice to have it only as a backup clock if the
>     ccm was
>      > not configured.
> 
>     I don't understand what you mean with backup clock can you elaborate
>     more?
> 
> If we have a clock source that is 0, we can still use the 24MHz clock as 
> that is an always known source, and isn't?controlled by the ccm. 
> Therefore if we have a dummy clock the soc will still have delays and 
> timeouts at the right time. But this would make it so that we never 
> return an error from clk_get_rate(&clk);

I'm not sure it is that safe. I'd prefer something that doesn't work at
all rather than something that works by a default specified inside a 
driver. Since we're trying to imitate linux driver I would avoid this 
solution. Often code is exchanged between u-boot, linux etc. so I'd 
prefer to keep it like linux does.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> 
>     Thank you
> 
>     Best regards
>     -- 
>     Giulio Benetti
>     Benetti Engineering sas
> 

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

[Giulio Benetti]

Hi Jesse,

On 2/11/21 7:54 PM, Jesse T wrote:
[SNIP]

>      >>> +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".
>      > Ah this makes sense now.

Here it's my bad, clk_get_rate() really returns ulong but can return a 
negative number too, so my patch has been dropped. You can verify it in 
clk-uclass.c where function is implemented, in get_rate() function 
pointer is null the return -ENOSYS. Then please declare rate as ulong 
and check it in if statement with IS_ERR_VALUE(). That way you're sure 
it's not negative.

Thank you
Best regards

-- 
Giulio Benetti
Benetti Engineering sas

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

[Jesse T]

sry for top posting I'm on my phone just b4 bed. any way the comment in the
header file says it returns an int. I don't remember what it actually
returns but it should have more clarity. as for moving all the bit
manipulation to a separate init function , I would essentially have to
remake the poll function without the clock driver stuff. I'm assuming u did
this so that it could be made more portable to other soc's. What I'm going
to do is declare the function with the parameters being the timers udevice
struct. and define it based on the soc family. similar to how the Linux
kernel does it

On Fri, Feb 12, 2021, 8:22 PM Giulio Benetti <
giulio.benetti@benettiengineering.com> wrote:

> Hi Jesse,
>
> On 2/11/21 7:54 PM, Jesse T wrote:
> [SNIP]
>
> >      >>> +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".
> >      > Ah this makes sense now.
>
> Here it's my bad, clk_get_rate() really returns ulong but can return a
> negative number too, so my patch has been dropped. You can verify it in
> clk-uclass.c where function is implemented, in get_rate() function
> pointer is null the return -ENOSYS. Then please declare rate as ulong
> and check it in if statement with IS_ERR_VALUE(). That way you're sure
> it's not negative.
>
> Thank you
> Best regards
>
> --
> Giulio Benetti
> Benetti Engineering sas
>
>
>
>

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

[Giulio Benetti]

Hi Jesse,

On 2/13/21 4:10 AM, Jesse T wrote:
> sry for top posting I'm on my phone just b4 bed. any way the comment in 
> the header file says it returns an int.

No, it says ulong

> I don't remember what it 
> actually returns but it should have more clarity. 

I've sent a new patch to clarify it and you're in Cc.

as for moving all the
> bit manipulation to a separate init function , I would essentially have 
> to remake the poll function without the clock driver stuff. I'm assuming 
> u did this so that it could be made more portable to other soc's. What 
> I'm going to do is declare the function with the parameters being the 
> timers udevice struct. and define it based on the soc family. similar to 
> how the Linux kernel does it

As as I can understand it's ok, I've given you an example below, so try
following that one. Waiting for new patch.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> 
> On Fri, Feb 12, 2021, 8:22 PM Giulio Benetti 
> <giulio.benetti@benettiengineering.com 
> <mailto:giulio.benetti@benettiengineering.com>> wrote:
> 
>     Hi Jesse,
> 
>     On 2/11/21 7:54 PM, Jesse T wrote:
>     [SNIP]
> 
>      >? ? ? >>> +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".
>      >? ? ? > Ah this makes sense now.
> 
>     Here it's my bad, clk_get_rate() really returns ulong but can return a
>     negative number too, so my patch has been dropped. You can verify it in
>     clk-uclass.c where function is implemented, in get_rate() function
>     pointer is null the return -ENOSYS. Then please declare rate as ulong
>     and check it in if statement with IS_ERR_VALUE(). That way you're sure
>     it's not negative.
> 
>     Thank you
>     Best regards
> 
>     -- 
>     Giulio Benetti
>     Benetti Engineering sas
> 
> 
> 

[PATCH v2 0/1] Add driver support for the IMX General Purpose Timer (GPT) available

[Giulio Benetti]

On 2/10/21 1:04 AM, Jesse Taube wrote:
> Giulio Benetti (3):
>    timer: imx-gpt: Add timer support for i.MX SoCs family
> 
> Jesse Taube (1):
>    timer: imx-gpt: Add timer support for i.MX SoCs family

There's something strange here, patchset is malformed since above there 
are 4 patches listed while subject speaks about only 1 patch 0/1.

So please, try to fix it while sending to yourself with git send-mail 
--to "you", otherwise you spam ML.

>   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
> 
> ---
> V1->V2:
> * Fixed indentation
> * Fixed capitals
> * Made timer work on only 24MHz clock

This is the right way.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas