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

[Giulio Benetti <giulio.benetti@benettiengineering.com>]

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@benettiengineering.com 
> <mailto:giulio.benetti@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 at 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
> 
> 
>