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

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-giuli...

...

• 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