Hi Jesse,
please use plain-text e-mail instead of html,
On 4/8/21 3:06 AM, Jesse T wrote:
> + if (rate == 24000000UL) {
> + /* Set timer frequency if using 24M clock source */
> + if (prescaler > GPT_PR_PRESCALER24M_MAX)
> + return -EINVAL;
> +
In the datasheet for the imxrt1050 the 24Mhz scaler output goes into the
normal prescaler.The width of the 24Mhz prescaler is only 4 bits it
seems to be meant to divide the 24Mhz to something more reasonable for
the other prescaler.
You're right, I've missed RM Figure 51-2 against 51-1, I was sure that
Prescaler 24M was directly feeding the Timer Counter. Anyway I was
trying to follow what they do in Linux kernel:
https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/timer-imx...
What they try to do as I understand is to avoid using PLL if 3Mhz is the
wished rate and the same I'm trying to do here. Then yes, you're right
that main prescaler can divide again.
I would do something like this...
+ if (rate == 24000000UL) {
+ /* Set timer frequency if using 24M clock source */
+ if (prescaler > (GPT_PR_PRESCALER24M_MAX << 12) |
GPT_PR_PRESCALER_MAX)
I can't understand this check ^^^
+ return -EINVAL;
+ /* Set 24M prescaler */
+ writel(( (prescaler >> 12) <<
GPT_PR_PRESCALER24M_SHIFT), ®s->pr);
+ writel(( (prescaler & GPT_PR_PRESCALER_MASK) <<
GPT_PR_PRESCALER_SHIFT), ®s->pr);
This way ^^^ you set the 2 in-series prescalers with the same value if
it's what you mean.
Ideally we would have the lengths in bits for GPT_PR_PRESCALER_MASK to
replace the 12, but we can also do...
+ if (rate == 24000000UL) {
+ /* Set timer frequency if using 24M clock source */
+ if (prescaler > (GPT_PR_PRESCALER24M_MASK |
GPT_PR_PRESCALER_MAX))
Still don't understand this ^^^ with the 2 or'ed macros.
That gets expanded like:
```
if (prescaler > (GPT_PR_PRESCALER24M_MASK | ((GPT_PR_PRESCALER24M_MASK
GPT_PR_PRESCALER24M_SHIFT)))
```
and to:
```
if (prescaler > (0x0000F000 | (0x0000F000 >> 12)))
```
that results in:
```
if (prescaler > (0x0000F000 | 0xF))
```
so it doesn't make sense to me, maybe did you mean something different
or I'm wrong while expanding?
+ return -EINVAL;
+ /* Set 24M prescaler */
+ writel(prescaler , ®s->pr);
Side note while debugging this i added `KCFLAGS=-DDEBUG` and the boot
would hang with it but without it would boot normally. The cause for the
hang is this loop will never exit, not this only happens while booting
u-boot and not during the spl stage.
/drivers/serial/serial_lpuart.c
@@ -356,7 +356,9 @@ static void _lpuart32_serial_putc(struct
lpuart_serial_plat *plat,
while (true) {
lpuart_read32(plat->flags, &base->stat, &stat);
if ((stat & STAT_TDRE))
break;
Thanks for recalling me this, I still didn't check it, so I'm going
through this and fix it.
Sorry if im being dumb
You're not being dumb
im new to this stuff and thanks for every one for
walking me through this.
You're welcome :-) and thank you for helping me with this driver and
reviewing! Reviewing is a very important part of the process of
upstreaming patches.
Kind regards
--
Giulio Benetti
Benetti Engineering sas
>
> On Wed, Apr 7, 2021 at 3:20 PM Giulio Benetti
> <giulio.benetti@benettiengineering.com
>
mailto:giulio.benetti@benettiengineering.com> wrote:
>
> On 4/7/21 9:02 PM, Giulio Benetti 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.
>
> Sorry, it's not true we deal 24Mhz only, also peripheral clock is dealt.
> So commit log should be:
> ```
> This timer driver is using GPT Timer (General Purpose Timer) available
> on almost all i.MX SoCs family. This driver deals with both 24Mhz
> oscillator as well as peripheral clock.
> ```
>
> Let me know if I need to re-send.
>
> Best regards
> --
> Giulio Benetti
> Benetti Engineering sas
>
> > Signed-off-by: Giulio Benetti
> <giulio.benetti@benettiengineering.com
>
mailto:giulio.benetti@benettiengineering.com>
> > [Giulio: added the driver's stub and handled peripheral clock
> prescaler
> > setting]
> > Signed-off-by: Jesse Taube <mr.bossman075@gmail.com
>
mailto:mr.bossman075@gmail.com>
> > [Jesse: added init, setting prescaler for 24Mhz support and enabling
> > timer]
> > ---
> > drivers/timer/Kconfig | 7 ++
> > drivers/timer/Makefile | 1 +
> > drivers/timer/imx-gpt-timer.c | 162
> ++++++++++++++++++++++++++++++++++
> > 3 files changed, 170 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..a498f2e21c
> > --- /dev/null
> > +++ b/drivers/timer/imx-gpt-timer.c
> > @@ -0,0 +1,162 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2021
> > + * Author(s): Giulio Benetti
> <giulio.benetti@benettiengineering.com
>
mailto:giulio.benetti@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_EN BIT(0)
> > +#define GPT_CR_FRR BIT(9)
> > +#define GPT_CR_EN_24M BIT(10)
> > +#define GPT_CR_SWR BIT(15)
> > +
> > +#define GPT_PR_PRESCALER24M_MASK 0x0000F000
> > +#define GPT_PR_PRESCALER24M_SHIFT 12
> > +#define GPT_PR_PRESCALER24M_MAX (GPT_PR_PRESCALER24M_MASK
> >> GPT_PR_PRESCALER24M_SHIFT)
> > +#define GPT_PR_PRESCALER_MASK 0x00000FFF
> > +#define GPT_PR_PRESCALER_SHIFT 0
> > +#define GPT_PR_PRESCALER_MAX (GPT_PR_PRESCALER_MASK >>
> GPT_PR_PRESCALER_SHIFT)
> > +
> > +#define GPT_CLKSRC_IPG_CLK (1 << 6)
> > +#define GPT_CLKSRC_IPG_CLK_24M (5 << 6)
> > +
> > +/* If CONFIG_SYS_HZ_CLOCK not specified et's default to 3Mhz */
> > +#ifndef CONFIG_SYS_HZ_CLOCK
> > +#define CONFIG_SYS_HZ_CLOCK 3000000
> > +#endif
> > +
> > +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_setup(struct imx_gpt_timer_regs *regs, u32 rate)
> > +{
> > + u32 prescaler = (rate / CONFIG_SYS_HZ_CLOCK) - 1;
> > +
> > + /* Reset the timer */
> > + setbits_le32(®s->cr, GPT_CR_SWR);
> > +
> > + /* Wait for timer to finish reset */
> > + while (readl(®s->cr) & GPT_CR_SWR)
> > + ;
> > +
> > + if (rate == 24000000UL) {
> > + /* Set timer frequency if using 24M clock source */
> > + if (prescaler > GPT_PR_PRESCALER24M_MAX)
> > + return -EINVAL;
> > +
> > + /* Set 24M prescaler */
> > + writel((prescaler << GPT_PR_PRESCALER24M_SHIFT),
> ®s->pr);
> > + /* Set Oscillator as clock source, enable 24M input
> and set gpt
> > + * in free-running mode
> > + */
> > + writel(GPT_CLKSRC_IPG_CLK_24M | GPT_CR_EN_24M |
> GPT_CR_FRR, ®s->cr);
> > + } else {
> > + if (prescaler > GPT_PR_PRESCALER_MAX)
> > + return -EINVAL;
> > +
> > + /* Set prescaler */
> > + writel((prescaler << GPT_PR_PRESCALER_SHIFT),
> ®s->pr);
> > + /* Set Peripheral as clock source and set gpt in
> free-running
> > + * mode
> > + */
> > + writel(GPT_CLKSRC_IPG_CLK | GPT_CR_FRR, ®s->cr);
> > + }
> > +
> > + /* Start timer */
> > + setbits_le32(®s->cr, GPT_CR_EN);
> > +
> > + return 0;
> > +}
> > +
> > +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 clk_rate;
> > + int ret;
> > +
> > + addr = dev_read_addr(dev);
> > + if (addr == FDT_ADDR_T_NONE)
> > + return -EINVAL;
> > +
> > + priv->base = (struct imx_gpt_timer_regs *)addr;
> > + regs = priv->base;
> > +
> > + 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;
> > + }
> > +
> > + /* Get timer clock rate */
> > + clk_rate = clk_get_rate(&clk);
> > + if (clk_rate <= 0) {
> > + dev_err(dev, "Could not get clock rate...\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = imx_gpt_setup(regs, clk_rate);
> > + if (ret) {
> > + dev_err(dev, "Could not setup timer\n");
> > + return ret;
> > + }
> > +
> > + uc_priv->clock_rate = CONFIG_SYS_HZ_CLOCK;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct timer_ops imx_gpt_timer_ops = {
> > + .get_count = imx_gpt_timer_get_count,
> > +};
> > +
> > +static const struct udevice_id imx_gpt_timer_ids[] = {
> > + { .compatible = "fsl,imxrt-gpt" },
> > + {}
> > +};
> > +
> > +U_BOOT_DRIVER(imx_gpt_timer) = {
> > + .name = "imx_gpt_timer",
> > + .id = UCLASS_TIMER,
> > + .of_match = imx_gpt_timer_ids,
> > + .priv_auto = sizeof(struct imx_gpt_timer_priv),
> > + .probe = imx_gpt_timer_probe,
> > + .ops = &imx_gpt_timer_ops,
> > +};
> >
>
