[U-Boot] [PATCH] powerpc: Consolidate bootcount_{store|load} for PowerPC

[Michael Zaidman]

Hi Stefan,

Good work, you obviously dived dipper than my patch did. Please see my
comments below.

On Mon, Apr 26, 2010 at 5:28 PM, Stefan Roese <sr at denx.de> wrote:
> This patch consolidates bootcount_{store|load} for PowerPC by
> implementing a common version in arch/powerpc/lib/bootcount.c. This
> code is now used by all PowerPC variants that currently have these
> functions implemented.
>
> The functions now use the proper IO-accessor functions to read/write the
> values.
>
> This code also supports two different bootcount versions:
>
> a) Use 2 seperate words (2 * 32bit) to store the bootcounter
> b) Use only 1 word (2* 16bit) to store the bootcounter
>
> Version b) was already used by MPC5xxx.
>
> Signed-off-by: Stefan Roese <sr at denx.de>
> Cc: Michael Zaidman <michael.zaidman at gmail.com>
> Cc: Wolfgang Denk <wd at denx.de>
> Cc: Kim Phillips <kim.phillips at freescale.com>
> Cc: Anatolij Gustschin <agust at denx.de>
> ---
>  arch/powerpc/cpu/mpc5xxx/cpu.c      |   20 --------
>  arch/powerpc/cpu/mpc8260/commproc.c |   24 ---------
>  arch/powerpc/cpu/mpc83xx/cpu.c      |   30 ------------
>  arch/powerpc/cpu/mpc8xx/commproc.c  |   26 ----------
>  arch/powerpc/cpu/ppc4xx/commproc.c  |   24 ---------
>  arch/powerpc/lib/Makefile           |    1 +
>  arch/powerpc/lib/bootcount.c        |   90 +++++++++++++++++++++++++++++++++++
>  7 files changed, 91 insertions(+), 124 deletions(-)
>  create mode 100644 arch/powerpc/lib/bootcount.c
>

[snip]

It makes sense to keep some measure of flexibility by giving to the
user possibility to override the CONFIG_SYS_BOOTCOUNT_ADDR definition.
It can be easily achieved by adding the following lines:

#ifdef CONFIG_SYS_BOOTCOUNT_ADDR
#define _BOOTCOUNT_ADDR CONFIG_SYS_BOOTCOUNT_ADDR
#else

> +#if defined(CONFIG_MPC5xxx)
> +#define CONFIG_BOOTCOUNT_ADDR  (MPC5XXX_CDM_BRDCRMB)
> +#define CONFIG_SYS_BOOTCOUNT_USE_32BIT

Also CONFIG_ as stated in u-boot's README should specify selectable by
user options. I suggest omitting of all the locally defined CONFIG_ in
your code.

> +#endif /* defined(CONFIG_MPC5xxx) */
> +
> +#if defined(CONFIG_MPC821) || defined(CONFIG_MPC823) || \
> +    defined(CONFIG_MPC850) || defined(CONFIG_MPC852T) || \
> +    defined(CONFIG_MPC855) || \
> +    defined(CONFIG_MPC860) || defined(CONFIG_MPC866) || \
> +    defined(CONFIG_MPC885)

This fragment does not cover all mpc8xx permutations. We can cover
them all by the following code:

#if	defined(CONFIG_MPC821) || defined(CONFIG_MPC823) || \
	defined(CONFIG_MPC855) || defined(CONFIG_MPC855T)|| \
	defined(CONFIG_MPC850) || defined(CONFIG_MPC86x)

I just used the CONFIG_MPC86x which has been already constructed by common.h
and accomplished the list by CPUs that for unknown to me reason are
not mentioned in common.h Probably, the better way is to add them into
the common.h...

Best regards,
Michael