[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c
On most Freescale 85xx boards, the CONFIG_SYS_CLK_FREQ and CONFIG_DDR_CLK_FREQ macros are defined like this:
This means that in order to use these macros, the callers must have prototypes for the corresponding functions. On 85xx, only speed.c uses these macros, so let's define the prototypes there. This eliminates the need to define the prototypes in the board config files.
Signed-off-by: Timur Tabi timur@freescale.com ---
I'm posting the patch for review, and if everyone likes it, inclusion in the 85xx repo. Comments welcome.
arch/powerpc/cpu/mpc85xx/speed.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/cpu/mpc85xx/speed.c b/arch/powerpc/cpu/mpc85xx/speed.c index 8132115..724ce9d 100644 --- a/arch/powerpc/cpu/mpc85xx/speed.c +++ b/arch/powerpc/cpu/mpc85xx/speed.c @@ -31,6 +31,10 @@ #include <asm/processor.h> #include <asm/io.h>
+/* Board-specific clock frequency functions. */ +unsigned long calculate_board_sys_clk(void); +unsigned long calculate_board_ddr_clk(void); + DECLARE_GLOBAL_DATA_PTR;
/* --------------------------------------------------------------- */
On 05/21/2010 01:07 PM, Timur Tabi wrote:
On most Freescale 85xx boards, the CONFIG_SYS_CLK_FREQ and CONFIG_DDR_CLK_FREQ macros are defined like this:
Like what?
This means that in order to use these macros, the callers must have prototypes for the corresponding functions. On 85xx, only speed.c uses these macros, so let's define the prototypes there. This eliminates the need to define the prototypes in the board config files.
It also eliminates the utility of the prototype in making sure usage matches the definition, and requires that the user of the macros provide implementation prerequisites.
-Scott
Scott Wood wrote:
On 05/21/2010 01:07 PM, Timur Tabi wrote:
On most Freescale 85xx boards, the CONFIG_SYS_CLK_FREQ and CONFIG_DDR_CLK_FREQ macros are defined like this:
Like what?
Doh. Git commit delete those lines because they began with a "#".
This was supposed to be there:
#define CONFIG_SYS_CLK_FREQ calculate_board_sys_clk() #define CONFIG_DDR_CLK_FREQ calculate_board_ddr_clk()
This means that in order to use these macros, the callers must have prototypes for the corresponding functions. On 85xx, only speed.c uses these macros, so let's define the prototypes there. This eliminates the need to define the prototypes in the board config files.
It also eliminates the utility of the prototype in making sure usage matches the definition, and requires that the user of the macros provide implementation prerequisites.
Hmm... Looks like calculate_board_sys_clk() is defined only on the p2020 currently. Everyone else uses get_board_sys_clk(). So perhaps we need to fix p2020ds to use get_board_sys_clk() instead of calculate_board_sys_clk(). But that's a different problem.
Well, I'm open to suggestions. Wolfgang asked me to find a solution to this:
#ifndef __ASSEMBLY__ unsigned long calculate_board_sys_clk(void); unsigned long calculate_board_ddr_clk(void); #endif #define CONFIG_SYS_CLK_FREQ calculate_board_sys_clk() #define CONFIG_DDR_CLK_FREQ calculate_board_ddr_clk()
He doesn't want the prototypes in the board header file.
-Scott
On 05/21/2010 01:18 PM, Timur Tabi wrote:
Scott Wood wrote:
On 05/21/2010 01:07 PM, Timur Tabi wrote:
On most Freescale 85xx boards, the CONFIG_SYS_CLK_FREQ and CONFIG_DDR_CLK_FREQ macros are defined like this:
Like what?
Doh. Git commit delete those lines because they began with a "#".
Hmm... is there any way to override that and insert such a line into a git commit?
Well, I'm open to suggestions. Wolfgang asked me to find a solution to this:
#ifndef __ASSEMBLY__ unsigned long calculate_board_sys_clk(void); unsigned long calculate_board_ddr_clk(void); #endif #define CONFIG_SYS_CLK_FREQ calculate_board_sys_clk() #define CONFIG_DDR_CLK_FREQ calculate_board_ddr_clk()
He doesn't want the prototypes in the board header file.
I think the board header file (or some header factored out from a set of similar boards, which the board header includes) is exactly where it belongs, given that it's implemented in a board C file, and the C call is not a public API. Some boards just hard code it as a constant. Others might want to call some other function, maybe with arguments (in fact, I see a dummy argument of zero passed in many if not all existing calls to these functions -- why?).
Duplicating large chunks of code is bad, but extremism in avoiding anything that even looks similar to something else, and breaking the logical isolation of said things in the process, is bad as well.
-Scott
Scott Wood wrote:
Doh. Git commit delete those lines because they began with a "#".
Hmm... is there any way to override that and insert such a line into a git commit?
I was going to just insert a blank space or some other character before the #.
I think the board header file (or some header factored out from a set of similar boards, which the board header includes) is exactly where it belongs, given that it's implemented in a board C file, and the C call is not a public API.
Well, you'll have to convince Wolfgang of that, not me. He won't accept my P1022DS board patch until I fix this "problem".
Some boards just hard code it as a constant. Others might want to call some other function, maybe with arguments (in fact, I see a dummy argument of zero passed in many if not all existing calls to these functions -- why?).
Probably some legacy stuff. Once this issue is resolved, I'll probably post a patch that unifies the functions in some way.
Timur Tabi wrote:
Well, you'll have to convince Wolfgang of that, not me. He won't accept my P1022DS board patch until I fix this "problem".
Actually, if you look at Kumar's ICS307 patch, you'll see he fixes this problem for any board that uses the ICS307:
-#ifndef __ASSEMBLY__ -extern unsigned long calculate_board_sys_clk(unsigned long dummy); -extern unsigned long calculate_board_ddr_clk(unsigned long dummy); -/* extern unsigned long get_board_sys_clk(unsigned long dummy); */ -/* extern unsigned long get_board_ddr_clk(unsigned long dummy); */ -#endif -#define CONFIG_SYS_CLK_FREQ calculate_board_sys_clk(0) /* sysclk for MPC85xx */ -#define CONFIG_DDR_CLK_FREQ calculate_board_ddr_clk(0) /* ddrclk for MPC85xx */ +#define CONFIG_SYS_CLK_FREQ get_board_sys_clk() /* sysclk for MPC85xx */ +#define CONFIG_DDR_CLK_FREQ get_board_ddr_clk() /* ddrclk for MPC85xx */ #define CONFIG_ICS307_REFCLK_HZ 33333000 /* ICS307 clock chip ref freq */ -#define CONFIG_GET_CLK_FROM_ICS307 /* decode sysclk and ddrclk freq
from ICS307 instead of switches */
On 05/21/2010 01:36 PM, Timur Tabi wrote:
Scott Wood wrote:
Doh. Git commit delete those lines because they began with a "#".
Hmm... is there any way to override that and insert such a line into a git commit?
I was going to just insert a blank space or some other character before the #.
I think the board header file (or some header factored out from a set of similar boards, which the board header includes) is exactly where it belongs, given that it's implemented in a board C file, and the C call is not a public API.
Well, you'll have to convince Wolfgang of that, not me. He won't accept my P1022DS board patch until I fix this "problem".
I think the "some header factored out from a set of similar boards" (or possibly similar board support idioms) approach would meet his request, though doing that at too fine-grained a level could lead to an unreadable tangle of header files and/or ifdefs.
-Scott
On Fri, 2010-05-21 at 13:34 -0500, Scott Wood wrote:
On 05/21/2010 01:18 PM, Timur Tabi wrote:
Scott Wood wrote:
On 05/21/2010 01:07 PM, Timur Tabi wrote:
On most Freescale 85xx boards, the CONFIG_SYS_CLK_FREQ and CONFIG_DDR_CLK_FREQ macros are defined like this:
Like what?
Doh. Git commit delete those lines because they began with a "#".
Hmm... is there any way to override that and insert such a line into a git commit?
'git commit --cleanup=verbatim' should do it. I think "--cleanup=whitespace" should also work.
Best, Peter
participants (3)
-
Peter Tyser -
Scott Wood -
Timur Tabi