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

Submitted by Benoît Thébaudeau on Aug. 14, 2012, 3:01 p.m.

Details

Message ID 1314655933.2403506.1344956467227.JavaMail.root@advansee.com
State Rejected
Delegated to: Stefano Babic
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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;