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

Jesse T mr.bossman075 at gmail.com
Thu Feb 11 19:54:01 CET 2021 

On Wed, Feb 10, 2021 at 3:09 PM Giulio Benetti <
giulio.benetti at 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 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.
> > 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@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
>

More information about the U-Boot mailing list