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

[Jesse T]

sry for top posting I'm on my phone just b4 bed. any way the comment in the
header file says it returns an int. I don't remember what it actually
returns but it should have more clarity. as for moving all the bit
manipulation to a separate init function , I would essentially have to
remake the poll function without the clock driver stuff. I'm assuming u did
this so that it could be made more portable to other soc's. What I'm going
to do is declare the function with the parameters being the timers udevice
struct. and define it based on the soc family. similar to how the Linux
kernel does it

On Fri, Feb 12, 2021, 8:22 PM Giulio Benetti <
giulio.benetti at benettiengineering.com> wrote:

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