Hi Jesse,

On 2/8/21 1:24 AM, Jesse Taube wrote:

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 Cc: Stefano Babic tsbabic@denx.de Cc: Giulio Benetti giulio.benetti@benettiengineering.com Cc: Jesse Taube mr.bossman075@gmail.com

All these ^^^ Cc in commit log are useless in this case, they are useful only in the case people are not listed when calling get_maintainers.pl or because you specifically need them to be in Cc. Your name/e-mail should be present as SoB and not as Cc because you're sending the patch, so add Signed-off-by instead of Cc for you entry and in case you did some modification write it like:

Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com Signed-off-by: Jesse Taube mr.bossman075@gmail.com [Jesse: changed this, set this to etc.]

This way authorship is kept, but you write what you have done on this patch.

---

drivers/timer/Kconfig | 7 ++ drivers/timer/Makefile | 1 + drivers/timer/imx-gpt-timer.c | 118 ++++++++++++++++++++++++++++++++++ 3 files changed, 126 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..c4e8c12a42 --- /dev/null +++ b/drivers/timer/imx-gpt-timer.c @@ -0,0 +1,118 @@ +// 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

+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 int imx_gpt_timer_get_count(struct udevice *dev, u64 *count) +{

• struct imx_gpt_timer_priv *priv = dev_get_priv(dev);
• struct imx_gpt_timer_regs *regs = priv->base;
• *count = readl(&regs->cnt);
• 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 prescaler;
• u32 rate, pr;
• int ret;
• addr = dev_read_addr(dev);
• if (addr == FDT_ADDR_T_NONE)

• return -EINVAL;
  

• priv->base = (struct imx_gpt_timer_regs *)addr;
• /* TODO: Retrieve clock-source */

This is something that prevents driver from working so a patch like this should not be accepted.

• /* TODO: Retrieve prescaler */

ditto

• 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);
• /* we set timer prescaler to obtain a 1MHz timer counter frequency */

+// pr = (rate / CONFIG_SYS_HZ_CLOCK) - 1;

This ^^^ was another TODO: I've put there, ditto. Also take care to never leave commented code.

• writel(pr, &regs->pr);
• /* Set timer frequency to 1MHz */
• uc_priv->clock_rate = rate / prescaler;
• /* start timer */

+// setbits_le32(&regs->cr1, CR1_CEN);

ditto

• 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_alloc_size = sizeof(struct imx_gpt_timer_priv),

This break builds and makes the patch not bisectable.

• .probe = imx_gpt_timer_probe,
• .ops = &imx_gpt_timer_ops,

+};

Best regards

Hi Jesse,

this patch should have been squashed with patch 1/4 as already suggested.

On 2/8/21 1:24 AM, Jesse Taube wrote:

Signed-off-by: Jesse Taube mr.bossman075@gmail.com Cc: Stefano Babic tsbabic@denx.de Cc: Giulio Benetti giulio.benetti@benettiengineering.com Cc: Jesse Taube mr.bossman075@gmail.com

This timer driver is using GPT Timer (General Purpose Timer) available on almost all i.MX SoCs family. Add code to enable timer. Add code get a defualt prescaler. Add defines for register masks.

---

drivers/timer/imx-gpt-timer.c | 50 +++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/timer/imx-gpt-timer.c b/drivers/timer/imx-gpt-timer.c index c4e8c12a42..ed6487af97 100644 --- a/drivers/timer/imx-gpt-timer.c +++ b/drivers/timer/imx-gpt-timer.c @@ -13,8 +13,29 @@

#include <asm/io.h>

-#define GPT_CR_SWR 0x00008000 +#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

Please avoid one whiteline

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

ditto

+/* +ipg_clk ipg_clk_root Peripheral clock +ipg_clk_32k ckil_sync_clk_root Low-frequency reference clock (32 kHz) +ipg_clk_highfreq perclk_clk_root High-frequency reference clock +ipg_clk_s ipg_clk_root Peripheral access clock +*/ struct imx_gpt_timer_regs { u32 cr; u32 pr; @@ -32,14 +53,12 @@ struct imx_gpt_timer_priv { struct imx_gpt_timer_regs *base; };

-static int imx_gpt_timer_get_count(struct udevice *dev, u64 *count) +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;

• *count = readl(&regs->cnt);
• return 0;

• return readl(&regs->cnt); }

  static int imx_gpt_timer_probe(struct udevice *dev)

@@ -50,7 +69,7 @@ static int imx_gpt_timer_probe(struct udevice *dev) struct clk clk; fdt_addr_t addr; u32 prescaler;

• u32 rate, pr;

• u32 rate; int ret;

  addr = dev_read_addr(dev);

@@ -84,16 +103,20 @@ static int imx_gpt_timer_probe(struct udevice *dev)

/* Get timer clock rate */ rate = clk_get_rate(&clk);

• if((int)rate <= 0){

• debug("Could not get clock rate, setting to default value...\n");
  

• rate = 1056000000UL;
  

• }

Here I don't agree, having a timer ticking at 1ms(1Khz) doesn't justify having a parent clock of 1Ghz. What I've thought here was using 24M crystal directly. But obviously this is determined by clk_get_rate().

So IMHO I would keep 24M only as source and only if needed I would make it different. Using only that reference you make this driver accepting only a 24M clock, but to begin it's enough for me. Most of i.MX* SoC setup exactly timer using 24M source clock.

Also using ipg clock implies that clock drivers supports it, but it doesn't so this driver shouldn't work as is.

Best regards