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

[Giulio Benetti]

Hi Jesse,

On 2/13/21 4:10 AM, Jesse T wrote:
> 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.

No, it says ulong

> I don't remember what it 
> actually returns but it should have more clarity. 

I've sent a new patch to clarify it and you're in Cc.

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

As as I can understand it's ok, I've given you an example below, so try
following that one. Waiting for new patch.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> 
> On Fri, Feb 12, 2021, 8:22 PM Giulio Benetti 
> <giulio.benetti at benettiengineering.com 
> <mailto: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
> 
> 
>