From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?= <benoit.thebaudeau@advansee.com>
Date: Fri, 17 Aug 2012 23:03:45 +0200 (CEST)
Subject: [U-Boot] [PATCH] mx5/6 timer: Use defined CONFIG_SYS_MX*_CLK32
In-Reply-To: <502EAEC7.7030101@denx.de>
Message-ID: <1454692757.2524424.1345237425370.JavaMail.root@advansee.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Stefano,

> On 17/08/2012 21:52, Beno?t Th?baudeau wrote:
> > Hi Stefano,
> > 
> >> On 14/08/2012 17:01, Beno?t Th?baudeau wrote:
> >>> Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> >>> Cc: Stefano Babic <sbabic@denx.de>
> >>> ---
> >>
> 
> Hi Beno?t,
> 
> >>
> >>>  .../arch/arm/cpu/armv7/imx-common/timer.c          |    6 +++++-
> >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git
> >>> u-boot-4d3c95f.orig/arch/arm/cpu/armv7/imx-common/timer.c
> >>> u-boot-4d3c95f/arch/arm/cpu/armv7/imx-common/timer.c
> >>> index 1645ff8..ad67367 100644
> >>> --- u-boot-4d3c95f.orig/arch/arm/cpu/armv7/imx-common/timer.c
> >>> +++ u-boot-4d3c95f/arch/arm/cpu/armv7/imx-common/timer.c
> >>> @@ -44,7 +44,11 @@ static struct mxc_gpt *cur_gpt = (struct
> >>> mxc_gpt
> >>> *)GPT1_BASE_ADDR;
> >>>  #define GPTCR_FRR		(1 << 9)	/* Freerun / restart */
> >>>  #define GPTCR_CLKSOURCE_32	(4 << 6)	/* Clock source */
> >>>  #define GPTCR_TEN		1		/* Timer enable */
> >>> -#define CLK_32KHZ		32768		/* 32Khz input */
> >>> +#if defined(CONFIG_MX51) || defined(CONFIG_MX53)
> >>> +#define CLK_32KHZ		CONFIG_SYS_MX5_CLK32
> >>> +#elif defined(CONFIG_MX6Q)
> >>> +#define CLK_32KHZ		CONFIG_SYS_MX6_CLK32
> >>> +#endif
> >>>  
> >>>  DECLARE_GLOBAL_DATA_PTR;
> >>>  
> >>
> >> Frankly I do not see the advantage to use the CONFIG_SYS defines.
> > 
> > It can be useful if 32000 Hz is used instead of 32768 Hz for some
> > boards.
> 
> Then it is the exception, and we should introduce a CONFIG_ that only
> *that* board should set, and not that all boards must have.
> 
> 
> > 
> >> On
> >> the
> >> other hand, checking this patch I see that  CONFIG_SYS_MX5_CLK32
> >> and
> >> CONFIG_SYS_MX6_CLK32 are dead code.
> >>
> >> I do not find drivers using, but all boards define them:
> >>
> >>  grep -r CONFIG_SYS_MX5_CLK32 *
> >> include/configs/efikamx.h:#define CONFIG_SYS_MX5_CLK32		32768
> >> include/configs/mx53loco.h:#define CONFIG_SYS_MX5_CLK32		32768
> >> include/configs/mx53ard.h:#define CONFIG_SYS_MX5_CLK32		32768
> >> include/configs/mx51evk.h:#define CONFIG_SYS_MX5_CLK32		32768
> >> include/configs/vision2.h:#define CONFIG_SYS_MX5_CLK32		32768
> >> include/configs/mx53evk.h:#define CONFIG_SYS_MX5_CLK32		32768
> >> include/configs/ima3-mx53.h:#define CONFIG_SYS_MX5_CLK32		32768
> >> include/configs/mx53smd.h:#define CONFIG_SYS_MX5_CLK32		32768
> >>
> >> and
> >>
> >> include/configs/mx6qsabrelite.h.orig:#define CONFIG_SYS_MX6_CLK32
> >>  32768
> >> include/configs/mx6qarm2.h:#define CONFIG_SYS_MX6_CLK32		32768
> >> include/configs/mx6qsabrelite.h:#define CONFIG_SYS_MX6_CLK32
> >> 	       32768
> > 
> > Indeed.
> 
> All boards here use 32768, none of them set it to 32000.
> 
> >> We have two defines, both for nothing. I prefer a patch dropping
> >> completely this dead code as to try to use it...
> > 
> > See my use case above, and tell me if it made you change your mind.
> 
> Your use case is surely for the tx25. But I do not see the case here.
> This patch alignes the MX5/MX6 to the MX25, where maybe this case
> could
> be treated better. All MX25 board must set CONFIG_MX25_CLK32 -
> instead
> that only the tx25 set a different value.

OK, then for mx25/35/5/6, would you like in timer.c something like:
#ifdef CONFIG_SYS_MX{*}_CLK32
#define CLK_32KHZ		CONFIG_SYS_MX{*}_CLK32
#else
#define CLK_32KHZ		32768
#endif

{*}: 25, 35, 5 or 6

In that way, this definition could be removed from most boards, and that would
still allow exceptions and be future-proof.

Best regards,
Beno?t