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

[Giulio Benetti]

Hi Jesse,

On 2/11/21 7:54 PM, Jesse T wrote:
[SNIP]

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

Here it's my bad, clk_get_rate() really returns ulong but can return a 
negative number too, so my patch has been dropped. You can verify it in 
clk-uclass.c where function is implemented, in get_rate() function 
pointer is null the return -ENOSYS. Then please declare rate as ulong 
and check it in if statement with IS_ERR_VALUE(). That way you're sure 
it's not negative.

Thank you
Best regards

-- 
Giulio Benetti
Benetti Engineering sas