[U-Boot-Users] [U-Boot Users][PATCH] ADS5121 Rev 3 Configuration Changes

[Wolfgang Denk]

In message <200805031714718.SM04052 at cw4mb41> you wrote:
> Configuration changes added to support ADS5121 Rev 3 version of the board.
> This includes changes to the ref clock, memory size (from 256 to 512), the
> addition of PCI functionality and Makefile options to build either the Rev 3
> or Rev 2 U-Boot. The new README explains these options.

Do we really need a separate Makefile target and thus separate images
for  this?  Isn't  there  a  way  to  detect  the  hardware  revision
automatically in the software?

> 
> Signed-off-by: Martha J Marx <mmarx at si...>
> ---
 Makefile                  |    9 ++++-----
>  board/ads5121/README      |    9 +++++++++
>  board/ads5121/ads5121.c   |   34 ++++++++++++++++++++++++++++++----
>  cpu/mpc512x/cpu.c         |    8 ++++++--
>  include/configs/ads5121.h |   27 +++++++++++++++++++++++----
>  5 files changed, 72 insertions(+), 15 deletions(-)
>  create mode 100644 board/ads5121/README

Your patch cannot be applied because it was corrupted (line  wrapped)
by  your mailer (if that is a valid name for something as Outlook). I
strongly recommend to use git-send-email to post patches.

> diff --git a/Makefile b/Makefile
> index ac0a17f..f91c75b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -742,12 +742,11 @@ motionpro_config:	unconfig
>  ## MPC512x Systems
>  #########################################################################
>  ads5121_config \
> -ads5121_PCI_config \
> -	:		 unconfig
> +ads5121_256_config	\
...
> +To configure for the Rev 2 ADS5121 type
> +make ads5121_256_config

That's not exactly intuitive - when you call it "Rev 2" hadrware, then
also use that name as the make target.

> @@ -50,7 +52,6 @@ int board_early_init_f (void)
>  {
>  	volatile immap_t *im = (immap_t *) CFG_IMMR;
>  	u32 lpcaw;
> -
>  	/*
>  	 * Initialize Local Window for the CPLD registers access (CS2
> selects
^^^^^^^^^^^^^^^^^^^^^^
Line wrapped.

>  	 * the CPLD chip)
> @@ -58,7 +59,6 @@ int board_early_init_f (void)
>  	im->sysconf.lpcs2aw = CSAW_START(CFG_CPLD_BASE) |
>  			      CSAW_STOP(CFG_CPLD_BASE, CFG_CPLD_SIZE);
>  	im->lpc.cs_cfg[2] = CFG_CS2_CFG;
> -
>  	/*
>  	 * According to MPC5121e RM, configuring local access windows should
>  	 * be followed by a dummy read of the config register that was

I don't like these white space changes. Please leave the blank  lines
as it was before.

>  	 * Without this the flash identification routine fails, as it needs
> to issue
^^^^^^^^^^^^^^^^^^^^^^
Line wrapped.

>  
> +#ifdef CONFIG_ADS5121_256
> +	*((volatile u8 *)(CFG_CPLD_BASE + 0x08)) = 0xC1;
> +#else
> +	if (*((u8 *)(CFG_CPLD_BASE + 0x08)) & 0x04) {
> +
> +		*((volatile u8 *)(CFG_CPLD_BASE + 0x08)) = 0xC1;
> +	} else {
> +	/* running from Backup flash */
^^^^^^^^^^^^^^^^^^^^^
Indentation wrong.

> +		*((volatile u8 *)(CFG_CPLD_BASE + 0x08)) = 0x32;
> +	}
> +#endif
> +	/*
> +	 * Configure Flash Speed
> +	 */
> +	*((volatile u32 *)(CFG_IMMR + LPC_OFFSET + CS0_CONFIG)) =
> CFG_CS0_CFG;
^^^^^^^^^^^^^^^^^^^^^^
Line wrapped.

> @@ -143,6 +144,9 @@ void ft_cpu_setup(void *blob, bd_t *bd)
>  	do_fixup_by_path(blob, eth_path, "local-mac-address",
> bd->bi_enetaddr, 6, 0);
>  
>  	/* this is so old kernels with old device trees will boot */
> +

Do NOT add an empty line here.

>  	do_fixup_by_path_u32(blob, "/" OF_SOC_OLD, "bus-frequency",
> bd->bi_ipsfreq, 0);
^^^^^^^^^^^^^^^^^^^^^^
Line wrapped.

> +	do_fixup_by_path(blob, eth_path_old, "local-mac-address",
> bd->bi_enetaddr, 6, 0);
^^^^^^^^^^^^^^^^^^^^^^
Line wrapped.

> +	do_fixup_by_path(blob, eth_path_old, "address", bd->bi_enetaddr, 6,
> 0);
^^^^^^^^^^^^^^^^^^^^^^
Line wrapped.

>  }
>  #endif
> diff --git a/include/configs/ads5121.h b/include/configs/ads5121.h
> index 81e7c1e..5f82855 100644
> --- a/include/configs/ads5121.h
> +++ b/include/configs/ads5121.h
> @@ -48,9 +48,15 @@
>  
>  /* CONFIG_PCI is defined at config time */
>  
> +#ifdef CONFIG_ADS5121_256
>  #define CFG_MPC512X_CLKIN	66000000	/* in Hz */
> +#else
> +#define CFG_MPC512X_CLKIN	33333333	/* in Hz */

Is this 33.333333 MHz or 33.0 MHz ?

> @@ -60,7 +66,11 @@
>  /*
>   * DDR Setup - manually set all parameters as there's no SPD etc.
>   */
> +#ifdef CONFIG_ADS5121_256
>  #define CFG_DDR_SIZE		256		/* MB */
> +#else
> +#define CFG_DDR_SIZE		512		/* MB */
> +#endif

We should use auto-sizing for the RAM like all "correct" ports do.

> @@ -175,8 +191,11 @@
>  
>  #define CFG_SRAM_BASE		0x30000000
>  #define CFG_SRAM_SIZE		0x00020000	/* 128 KB */
> -

Do not delete this empty line.

> +#ifdef ADS5121_256_CONFIG
>  #define CFG_CS0_CFG		0x05059310	/* ALE active low, data size
> 4bytes */
^^^^^^^^^^^^^^^^^^^^
Line wrapped.

> +#else
> +#define CFG_CS0_CFG		0x05059310	/* ALE active low, data size
> 4bytes */
^^^^^^^^^^^^^^^^^^^^
Line wrapped.

> +#endif
>  #define CFG_CS2_CFG		0x05059010	/* ALE active low, data size
> 1byte */
^^^^^^^^^^^^^^^^^^^^
Line wrapped.

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
Eureka!                                                 -- Archimedes