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

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