| Message ID | 1314655933.2403506.1344956467227.JavaMail.root@advansee.com |
|---|---|
| State | Rejected |
| Delegated to: | Stefano Babic |
| Headers | show |
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
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
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
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
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
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;
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(-)