[U-Boot] [PATCH 1/5] imx: gpt: Add 24Mhz OSC clock source support for GPT

[Stefano Babic <sbabic@denx.de>]

Hi Ye,

On 24/10/2014 14:32, Ye.Li wrote:
> Introduce a new configuration "CONFIG_MXC_GPT_HCLK". When it is set,
> the GPT will use 24Mhz OSC as clock source. Otherwise, the GPT will
> use 32Khz OSC as clock source.
> 
> Since only the GPT on iMX6 series provide the clock source option for
> 24Mhz OSC. For other series(MX5), if the configuration is set, the
> perclk will be selected as clock source.

But if the source option is possible only for i.MX6, why should be
possible to set it for MX5 ? It should be hidden in the SOC
configuration and not in board configuration.

> 
> MX6Q/D Rev 1.0 and MX6SL can't use the 24Mhz OSC clock source option,
> so select the perclk for them. For MX6SL, we will set the OSC 24Mhz to
> perclk in CCM, so eventually the clock comes from OSC 24Mhz.
> 

I am trying to understand. The explanation here does not reflect in the
implementation.  From the implementation, it is possible to set it even
with wrong revision.

Anyway, I do not think it is correct to put it as a configuration
option. This makes that we need different binaries for different
revisions of the SOC. It should be checked at runtime if the revision is
correct to set this clock as source. gpt_has_clk_source_osc has a check,
but does it make sense that CONFIG_MXC_GPT_HCLK can be set in any case ?

> Signed-off-by: Ye.Li <B37916@freescale.com>
> ---

Is this a second version of the patchset ? What are the changes ? Please
add version number to your patchset and write a history about changes. I
can suggest to use Simon's patman to post your patches.

>  arch/arm/imx-common/timer.c |   76 +++++++++++++++++++++++++++++++++++++------
>  1 files changed, 66 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/imx-common/timer.c b/arch/arm/imx-common/timer.c
> index c63f78f..f7e49bd 100644
> --- a/arch/arm/imx-common/timer.c
> +++ b/arch/arm/imx-common/timer.c
> @@ -2,7 +2,7 @@
>   * (C) Copyright 2007
>   * Sascha Hauer, Pengutronix
>   *
> - * (C) Copyright 2009 Freescale Semiconductor, Inc.
> + * (C) Copyright 2009-2014 Freescale Semiconductor, Inc.

I have already complain about this. There are a lot of commits after the
file was merged into u-boot, and a lot of them not directly by
Freescale. I do not think it is legally correct to change the Copyright.

>   *
>   * SPDX-License-Identifier:	GPL-2.0+
>   */
> @@ -12,6 +12,7 @@
>  #include <div64.h>
>  #include <asm/arch/imx-regs.h>
>  #include <asm/arch/clock.h>
> +#include <asm/arch/sys_proto.h>
>  
>  /* General purpose timers registers */
>  struct mxc_gpt {
> @@ -26,23 +27,60 @@ static struct mxc_gpt *cur_gpt = (struct mxc_gpt *)GPT1_BASE_ADDR;
>  
>  /* General purpose timers bitfields */
>  #define GPTCR_SWR		(1 << 15)	/* Software reset */
> +#define GPTCR_24MEN	    (1 << 10)	/* Enable 24MHz clock input from crystal */
>  #define GPTCR_FRR		(1 << 9)	/* Freerun / restart */
> -#define GPTCR_CLKSOURCE_32	(4 << 6)	/* Clock source */
> +#define GPTCR_CLKSOURCE_32	(4 << 6)	/* Clock source 32khz */
> +#define GPTCR_CLKSOURCE_OSC	(5 << 6)	/* Clock source OSC */
> +#define GPTCR_CLKSOURCE_PRE	(1 << 6)	/* Clock source PRECLK */
> +#define GPTCR_CLKSOURCE_MASK (0x7 << 6)
>  #define GPTCR_TEN		1		/* Timer enable */
>  
> +#define GPTPR_PRESCALER24M_SHIFT 12
> +#define GPTPR_PRESCALER24M_MASK (0xF << GPTPR_PRESCALER24M_SHIFT)
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +static inline int gpt_has_clk_source_osc(void)
> +{
> +#if defined(CONFIG_MX6)
> +	if (((is_cpu_type(MXC_CPU_MX6Q) || is_cpu_type(MXC_CPU_MX6D))
> +		&& (is_soc_rev(CHIP_REV_1_0) > 0))
> +		|| is_cpu_type(MXC_CPU_MX6DL) || is_cpu_type(MXC_CPU_MX6SOLO)
> +		|| is_cpu_type(MXC_CPU_MX6SX))
> +		return 1;
> +
> +	return 0;
> +#else
> +	return 0;
> +#endif

We are generally trying to get rid of #ifdef, but it seems we are not
going in the right direction. is_cpu_type() should return false if the
CPU is not what we expect - any chance for having this function for all
i.MXes ? We can then get rid of a lot of nasty #ifdef. What about moving
this function centrally, maybe in arch/arm/include/asm/imx-common/ ?

> +}
> +
> +static inline ulong gpt_get_clk(void)
> +{
> +#ifdef CONFIG_MXC_GPT_HCLK
> +	if (gpt_has_clk_source_osc())
> +		return MXC_HCLK >> 3;
> +	else
> +		return mxc_get_clock(MXC_IPG_PERCLK);
> +#else
> +	return MXC_CLK32;
> +#endif
> +}
>  static inline unsigned long long tick_to_time(unsigned long long tick)
>  {
> +	ulong gpt_clk = gpt_get_clk();
> +
>  	tick *= CONFIG_SYS_HZ;
> -	do_div(tick, MXC_CLK32);
> +	do_div(tick, gpt_clk);
>  
>  	return tick;
>  }
>  
>  static inline unsigned long long us_to_tick(unsigned long long usec)
>  {
> -	usec = usec * MXC_CLK32 + 999999;
> +	ulong gpt_clk = gpt_get_clk();
> +
> +	usec = usec * gpt_clk + 999999;
>  	do_div(usec, 1000000);
>  
>  	return usec;
> @@ -59,11 +97,29 @@ int timer_init(void)
>  	for (i = 0; i < 100; i++)
>  		__raw_writel(0, &cur_gpt->control);
>  
> -	__raw_writel(0, &cur_gpt->prescaler); /* 32Khz */
> -
> -	/* Freerun Mode, PERCLK1 input */
>  	i = __raw_readl(&cur_gpt->control);
> -	__raw_writel(i | GPTCR_CLKSOURCE_32 | GPTCR_TEN, &cur_gpt->control);
> +	i &= ~GPTCR_CLKSOURCE_MASK;
> +
> +#ifdef CONFIG_MXC_GPT_HCLK
> +	if (gpt_has_clk_source_osc()) {
> +		i |= GPTCR_CLKSOURCE_OSC | GPTCR_TEN;
> +
> +		/* For DL/S, SX, they has 24M OSC Enable bit and need to set prescaler */
> +		if (is_cpu_type(MXC_CPU_MX6DL) || is_cpu_type(MXC_CPU_MX6SOLO)
> +			|| is_cpu_type(MXC_CPU_MX6SX)) {
> +			i |= GPTCR_24MEN;
> +
> +			/* Produce 3Mhz clock */
> +			__raw_writel((7 << GPTPR_PRESCALER24M_SHIFT), &cur_gpt->prescaler);
> +		}
> +	} else {
> +		i |= GPTCR_CLKSOURCE_PRE | GPTCR_TEN;
> +	}
> +#else
> +	__raw_writel(0, &cur_gpt->prescaler); /* 32Khz */
> +	i |= GPTCR_CLKSOURCE_32 | GPTCR_TEN;
> +#endif
> +	__raw_writel(i, &cur_gpt->control);
>  
>  	gd->arch.tbl = __raw_readl(&cur_gpt->counter);
>  	gd->arch.tbu = 0;
> @@ -86,7 +142,7 @@ ulong get_timer_masked(void)
>  {
>  	/*
>  	 * get_ticks() returns a long long (64 bit), it wraps in
> -	 * 2^64 / MXC_CLK32 = 2^64 / 2^15 = 2^49 ~ 5 * 10^14 (s) ~
> +	 * 2^64 / GPT_CLK = 2^64 / 2^15 = 2^49 ~ 5 * 10^14 (s) ~
>  	 * 5 * 10^9 days... and get_ticks() * CONFIG_SYS_HZ wraps in
>  	 * 5 * 10^6 days - long enough.
>  	 */
> @@ -117,5 +173,5 @@ void __udelay(unsigned long usec)
>   */
>  ulong get_tbclk(void)
>  {
> -	return MXC_CLK32;
> +	return gpt_get_clk();
>  }
> 


Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================