[U-Boot] [PATCH 2/2] MPC8308ERDB: minimal support for devboard from Freescale

[Wolfgang Denk]

Dear Kim Phillips,

In message <20100719193356.a02add7e.kim.phillips at freescale.com> you wrote:
>
> +++ b/arch/powerpc/cpu/mpc83xx/acr.c
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (C) 2010 Freescale Semiconductor, Inc.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +
> +void single_cline_write(volatile void *addr, __be32 val)
> +{
> +	out_be32(addr, val);
> +}

Oops?? Why do we need a new file, with a new function, does nothing
else but calling another funtion, and that gets used just a single
time?

Drop this wrapper!

> +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c
> @@ -30,6 +30,8 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +extern void single_cline_write(volatile void *addr, __be32 val);

[Prototypes should always go to appropriate header files!!]

>  void cpu_init_f (volatile immap_t * im)
>  {
> +	__be32 val;
>  	__be32 acr_mask =
>  #ifdef CONFIG_SYS_ACR_PIPE_DEP /* Arbiter pipeline depth */
>  		ACR_PIPE_DEP |
> @@ -213,7 +216,8 @@ void cpu_init_f (volatile immap_t * im)
>  	memset ((void *) gd, 0, sizeof (gd_t));
>  
>  	/* system performance tweaking */
> -	clrsetbits_be32(&im->arbiter.acr, acr_mask, acr_val);
> +	val = __raw_readl(&im->arbiter.acr);

Do we need __raw_readl()? Why would in_be32() not work?

> +	single_cline_write(&im->arbiter.acr, acr_val & (val & ~acr_mask));

Make this:

	out_be32(&im->arbiter.acr, acr_val & (val & ~acr_mask));


> +    . = ALIGN(32);
> +    arch/powerpc/cpu/mpc83xx/acr.o 	(.text)

Ah! I guess this is worth a comment?  But should we not rather aligh
the in_* and out_* functions then?

What is the exact use case when such alignment might be needed?  Can
you guarantee that it's only with this specific register access, and
only for write accesses?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
... bacteriological warfare ... hard to believe we were once foolish
enough to play around with that.
	-- McCoy, "The Omega Glory", stardate unknown