On Wed, Feb 10, 2021 at 3:09 PM Giulio Benetti < giulio.bene...@benettiengineering.com> wrote:
> Hi Jesse, > > I re-add all people and ML in Cc so they can follow. Next time reply to > all. > > On 2/10/21 8:00 PM, Jesse Taube wrote: > > > > On 2/10/21 12:49 PM, Giulio Benetti wrote: > >> On 2/10/21 1:04 AM, Jesse Taube wrote: > >>> This timer driver is using GPT Timer (General Purpose Timer) > >>> available on almost all i.MX SoCs family. > >>> Since this driver is only meant to provide u-boot's timer and > >>> counter, and most of the i.MX* SoCs use a 24Mhz crystal, let's only > >>> deal with that specific source. > >> > >> We have a problem with columns on commit log, they shouldn't be longer > >> than 72 cols, so please check the editor you're using for commit log > >> writing and set 72 cols costrains. I use nano and you only need to set > >> in .gitconfig under [core]: > >> editor = nano -b -r 72 > >> > >> This way nano automatically put a CR. > >> > >>> > >>> Signed-off-by: Giulio Benetti <giulio.bene...@benettiengineering.com> > >>> Signed-off-by: Jesse Taube <mr.bossman...@gmail.com> > >>> > >>> --- > >>> drivers/timer/Kconfig | 7 ++ > >>> drivers/timer/Makefile | 1 + > >>> drivers/timer/imx-gpt-timer.c | 132 > ++++++++++++++++++++++++++++++++++ > >>> 3 files changed, 140 insertions(+) > >>> create mode 100644 drivers/timer/imx-gpt-timer.c > >>> > >>> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig > >>> index 80743a2551..ee81dfa776 100644 > >>> --- a/drivers/timer/Kconfig > >>> +++ b/drivers/timer/Kconfig > >>> @@ -227,4 +227,11 @@ config MCHP_PIT64B_TIMER > >>> Select this to enable support for Microchip 64-bit periodic > >>> interval timer. > >>> +config IMX_GPT_TIMER > >>> + bool "NXP i.MX GPT timer support" > >>> + depends on TIMER > >>> + help > >>> + Select this to enable support for the timer found on > >>> + NXP i.MX devices. > >>> + > >>> endmenu > >>> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile > >>> index eb5c48cc6c..e214ba7268 100644 > >>> --- a/drivers/timer/Makefile > >>> +++ b/drivers/timer/Makefile > >>> @@ -25,3 +25,4 @@ obj-$(CONFIG_STM32_TIMER) += stm32_timer.o > >>> obj-$(CONFIG_X86_TSC_TIMER) += tsc_timer.o > >>> obj-$(CONFIG_MTK_TIMER) += mtk_timer.o > >>> obj-$(CONFIG_MCHP_PIT64B_TIMER) += mchp-pit64b-timer.o > >>> +obj-$(CONFIG_IMX_GPT_TIMER) += imx-gpt-timer.o > >>> diff --git a/drivers/timer/imx-gpt-timer.c > >>> b/drivers/timer/imx-gpt-timer.c > >>> new file mode 100644 > >>> index 0000000000..58c37db300 > >>> --- /dev/null > >>> +++ b/drivers/timer/imx-gpt-timer.c > >>> @@ -0,0 +1,132 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >>> +/* > >>> + * Copyright (C) 2020 > >>> + * Author(s): Giulio Benetti <giulio.bene...@benettiengineering.com> > >>> + */ > >>> + > >>> +#include <common.h> > >>> +#include <clk.h> > >>> +#include <dm.h> > >>> +#include <fdtdec.h> > >>> +#include <timer.h> > >>> +#include <dm/device_compat.h> > >>> + > >>> +#include <asm/io.h> > >>> + > >>> +#define GPT_CR_SWR 0x00008000 > >>> +#define GPT_CR_CLKSRC 0x000001C0 > >>> +#define GPT_CR_EN_24M 0x00004000 > >>> +#define GPT_CR_EN 0x00000001 > >>> +#define GPT_PR_PRESCALER 0x00000FFF > >>> +#define GPT_PR_PRESCALER24M 0x0000F000 > >> > >> Here ^^^ and > >> > >>> +#define NO_CLOCK (0) > >>> +#define IPG_CLK (1 << 6) > >>> +#define IPG_CLK_HF (2 << 6) > >>> +#define IPG_EXT (3 << 6) > >>> +#define IPG_CLK_32K (4 << 6) > >>> +#define IPG_CLK_24M (5 << 6) > >> here you still have different tab numbers. Enable in your editor the > >> option to show whitespaces and tabs, that way everything it easier. > > I still don't exactly understand what you mean here. Should I press tab > > 3 times exactly instead of making the values line up. > > You need to check using an editor with whitespaces and tabs options > enabled and tabs set to 8 spaces how that code looks like. > > > In the commit log > > it seems to change weirdly because of the '+' in front of the define. > > Here it is the same, this should be due to the editor you use for commit > log. > > >>> + > >>> +struct imx_gpt_timer_regs { > >>> + u32 cr; > >>> + u32 pr; > >>> + u32 sr; > >>> + u32 ir; > >>> + u32 ocr1; > >>> + u32 ocr2; > >>> + u32 ocr3; > >>> + u32 icr1; > >>> + u32 icr2; > >>> + u32 cnt; > >>> +}; > >>> + > >>> +struct imx_gpt_timer_priv { > >>> + struct imx_gpt_timer_regs *base; > >>> +}; > >>> + > >>> +static u64 imx_gpt_timer_get_count(struct udevice *dev) > >>> +{ > >>> + struct imx_gpt_timer_priv *priv = dev_get_priv(dev); > >>> + struct imx_gpt_timer_regs *regs = priv->base; > >>> + > >>> + return readl(®s->cnt); > >>> +} > >>> + > >>> +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(®s->cr, GPT_CR_SWR); > >>> + > >>> + /* Wait for timer to finish reset */ > >>> + while (readl(®s->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.bene...@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. > >> > >>> + dev_err(dev, "Could not get clock rate...\n"); > >>> + return -EINVAL; > >>> + } > >>> + /* Only support 24MHz clock */ > >> > >> Extend to /* Only support 24MHz crystal clock */ otherwise it seems > >> that every 24Mhz clock is accepted and it's not that way. > >> > >> I don't know a sure way to be bounded to crystal clock, suggestions > >> are welcome. > >> > >>> + if (rate != 24000000UL) { > >>> + dev_err(dev, "Clock rate other than 24MHz not > supported...\n"); > >>> + return -EINVAL; > >>> + } > >>> + /* We set timer prescaler to obtain a 1MHz timer counter > >>> frequency */ > >>> + prescaler = (rate / CONFIG_SYS_HZ_CLOCK) - 1; > >> > >> Here you should check against maximum value of PRESCALER(0xFFF), if > >> prescaler is > 0xFFF then you have to return with error. > >> > >>> + writel(GPT_PR_PRESCALER & prescaler, ®s->pr); > >>> + /* Set timer frequency to 1MHz */ > >>> + uc_priv->clock_rate = CONFIG_SYS_HZ_CLOCK; > >>> + > >>> + clrbits_le32(®s->cr, GPT_CR_CLKSRC); > >>> + setbits_le32(®s->cr, IPG_CLK); > >> > >> Here IPG_CLK should be IPG_CLK_24M as discussed previously. > > > > The IPG_CLK_24M needs a different prescaler and a second enable bit. > > This will completely bypass all other clock sources making the check for > > the clock rate useless. > > Yes, in the operative way yes, you could also avoid passing clock source > through dts, but this way we check that the right clock source is passed > to dts, and that is the correct way to work I think. > > > It will also mean that even if we don't have a > > correct clock source it will work at the correct timing. > > Yes if they provide 24Mhz. > > I wanted it to be like that at the moment, because a lot of imx SoCs > setup GPT like that. Take a look here: > > https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-imx/timer.c#L80-99 > > This way the imx6* and imx7 could add their .compatible to this driver > in the future. If someone will need some specific tweaking then they > would send a patch adding such new DT property, like using 32K source. > But that comes next IMHO, otherwise we should describe entirely GPT > peripheral but what we need at the moment is getting 1ms tick like lot > of other imx SoCs already do. > > The other chance would be to treat all the clock sources possibilities, > but, at least for me, to begin this is sufficient and can be improved > later. > > > I changed it to use only the 24MHz clock and ignore all others, > > Ok > > > at some > > point it would be nice to have it only as a backup clock if the ccm was > > not configured. > > I don't understand what you mean with backup clock can you elaborate more? > If we have a clock source that is 0, we can still use the 24MHz clock as that is an always known source, and isn't controlled by the ccm. Therefore if we have a dummy clock the soc will still have delays and timeouts at the right time. But this would make it so that we never return an error from clk_get_rate(&clk); > Thank you > > Best regards > -- > Giulio Benetti > Benetti Engineering sas >