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.bene...@benettiengineering.com>
>> Signed-off-by: Jesse Taube <mr.bossman...@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.bene...@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-giulio.bene...@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?

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

Reply via email to