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

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