[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[Timur Tabi]

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;
 
 /* --------------------------------------------------------------- */
-- 
1.6.5

[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[Scott Wood]

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

[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[Timur Tabi]

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


-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[Scott Wood]

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

[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[Timur Tabi]

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
Linux kernel developer at Freescale

[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[Timur Tabi]

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 */

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[Scott Wood]

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

[U-Boot] [PATCH] mpc85xx: add function prototypes for sys and ddr clocks to speed.c

[Peter Tyser]

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