[PATCH v2 1/1] timer: imx-gpt: Add timer support for i.MX SoCs family
Giulio Benetti
giulio.benetti at benettiengineering.com
Wed Feb 10 21:13:49 CET 2021
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 at benettiengineering.com>
> >> Signed-off-by: Jesse Taube <mr.bossman075 at 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 at 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(®s->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(®s->cr, GPT_CR_SWR);
> >> +
> >> + /* Wait for timer to finish reset */
> >> + while (readl(®s->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@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, ®s->pr);
> >> + /* Set timer frequency to 1MHz */
> >> + uc_priv->clock_rate = CONFIG_SYS_HZ_CLOCK;
> >> +
> >> + clrbits_le32(®s->cr, GPT_CR_CLKSRC);
> >> + setbits_le32(®s->cr, IPG_CLK);
> >
> > Here IPG_CLK should be IPG_CLK_24M as discussed previously.
> >
> > Best regards
>
More information about the U-Boot
mailing list