Patchwork [U-Boot] mx5/6 timer: Use defined CONFIG_SYS_MX*_CLK32

login
register
mail settings
Submitter Benoît Thébaudeau
Date Aug. 14, 2012, 3:01 p.m.
Message ID <1314655933.2403506.1344956467227.JavaMail.root@advansee.com>
Download mbox | patch
Permalink /patch/177303/
State Rejected
Delegated to: Stefano Babic
Headers show

Comments

Benoît Thébaudeau - Aug. 14, 2012, 3:01 p.m.
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
Cc: Stefano Babic <sbabic@denx.de>
---
 .../arch/arm/cpu/armv7/imx-common/timer.c          |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Stefano Babic - Aug. 17, 2012, 7:43 p.m.
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. 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

We have two defines, both for nothing. I prefer a patch dropping
completely this dead code as to try to use it...

Best regards,
Stefano Babic
Benoît Thébaudeau - Aug. 17, 2012, 7:52 p.m.
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.

> 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.

> 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.

Best regards,
Benoît
Stefano Babic - Aug. 17, 2012, 8:51 p.m.
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.

Best regards,
Stefano
Benoît Thébaudeau - Aug. 17, 2012, 9:03 p.m.
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
Stefano Babic - Aug. 17, 2012, 9:09 p.m.
On 17/08/2012 23:03, Benoît Thébaudeau wrote:

> 
> 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.

Yes, this is exactly what I meant.

Best regards,
Stefano

Patch

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;