From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Fri, 27 Aug 2010 11:03:09 +0200
Subject: [U-Boot] [PATCH 2/8] APM82xxx: Add Common register definitions
In-Reply-To: <1282856749-24425-1-git-send-email-tmarri@apm.com>
References: <1282856749-24425-1-git-send-email-tmarri@apm.com>
Message-ID: <201008271103.09129.sr@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Marri,

On Thursday 26 August 2010 23:05:49 tmarri at apm.com wrote:
> From: Tirumala Marri <tmarri@apm.com>
> 
> This patch adds APM82XXX specific definitions, which include
> clock and boot strap.

Please find some comments below.
 
> Signed-off-by: Tirumala R Marri <tmarri@apm.com>
> ---
>  include/ppc440.h |   57
> ++++++++++++++++++++++++++++++++++++++++++++++++++++- include/ppc4xx.h |  
>  7 +++--
>  2 files changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/include/ppc440.h b/include/ppc440.h
> index c807dda..3bd8e98 100644
> --- a/include/ppc440.h
> +++ b/include/ppc440.h
> @@ -58,6 +58,25 @@
> 
>   | Clocking Controller
> 
>  
> +-------------------------------------------------------------------------
> ---*/ /* values for clkcfga register - indirect addressing of these regs */
> +#if defined(CONFIG_APM82XXX)
> +#define CPR0_CLKUPD      0x0020
> +#define CPR0_PLLC        0x0040
> +#define CPR0_PLLC_SEL(pllc)      (((pllc) & 0x01000000) >> 24)
> +#define CPR0_PLLD        0x0060
> +#define CPR0_PLLD_FDV(plld)     (((plld) & 0xff000000) >> 24)
> +#define CPR0_PLLD_FWDVA(plld)    (((plld) & 0x000f0000) >> 16)
> +#define CPR0_CPUD        0x0080
> +#define CPR0_CPUD_CPUDV(cpud)    (((cpud) & 0x07000000) >> 24)
> +#define CPR0_PLB2D       0x00a0
> +#define CPR0_PLB2D_PLB2DV(plb2d) (((plb2d) & 0x06000000) >> 25)
> +#define CPR0_OPBD        0x00c0
> +#define CPR0_OPBD_OPBDV(opbd)    (((opbd) & 0x03000000) >> 24)
> +#define CPR0_PERD       0x00e0
> +#define CPR0_PERD_PERDV(perd)    (((perd) & 0x03000000) >> 24)
> +#define CPR0_DDR2D      0x0100
> +#define CPR0_DDR2D_DDR2DV(ddr2d) (((ddr2d) & 0x06000000) >> 25)
> +#define CLK_ICFG	0x0140
> +#else
>  #define CPR0_PLLC	0x0040
>  #define CPR0_PLLD	0x0060
>  #define CPR0_PRIMAD0	0x0080
> @@ -67,6 +86,7 @@
>  #define CPR0_MALD	0x0100
>  #define CPR0_SPCID	0x0120
>  #define CPR0_ICFG	0x0140
> +#endif /*if defined(CONFIG_APM82XXX) */
> 
>  /* 440EPX boot strap options */
>  #define BOOT_STRAP_OPTION_A	0x00000000
> @@ -1275,7 +1295,36 @@
>  #define SDR0_AHB_CFG		0x370
>  #define SDR0_USB2HOST_CFG	0x371
>  #endif /* CONFIG_460EX || CONFIG_460GT */
> +#if defined(CONFIG_APM82XXX)
> +
> +#define SDR0_DDR0			0x00E1
> +#define SDR0_DDR0_DDRM_ENCODE(n)	((((unsigned long)(n))&0x03)<<29)
> +#define SDR0_DDR0_DDRM_DECODE(n)	((((unsigned long)(n))>>29)&0x03)
> +#define SDR0_DDR0_TUNE_ENCODE(n)	((((unsigned long)(n))&0x2FF)<<0)
> +#define SDR0_DDR0_TUNE_DECODE(n)	((((unsigned long)(n))>>0)&0x2FF)

Add spaces around the operators. I suggest to replace "unsigned int"
with "u32" to reduce the line-size resulting in this:

#define SDR0_DDR0_DDRM_ENCODE(n)	((((u32)(n)) & 0x03) << 29)
#define SDR0_DDR0_DDRM_DECODE(n)	((((u32)(n)) >> 29) & 0x03)
#define SDR0_DDR0_TUNE_ENCODE(n)	((((u32)(n)) & 0x2FF) << 0)
#define SDR0_DDR0_TUNE_DECODE(n)	((((u32)(n)) >> 0) & 0x2FF)

BTW: I couldn't find any reference to this SDR0_DDR0 register (0xe1) in the
APM8xxx users manual. You might want to add this description there.

> +#define SDR_SDSTP1_RL_DECODE(x) ((x & 0x000C0000) >> 18)

Missing braces around "x".

> +#define SDR_SDSTP1_RL_EBC       0x0
> +#define SDR_SDSTP1_RL_NDFC      0x2
> +
> +/* ECID */
> +#define SDR0_ECID0             0x0080
> +#define SDR0_ECID1             0x0081
> +#define SDR0_ECID2             0x0082
> +#define SDR0_ECID3             0x0083
> +
> +/* AHB config. */
> +#define AHB_TOP                 0xA4
> +#define AHB_BOT                 0xA5
> +#define SDR0_AHB_CFG            0x370
> +
> +/* DDR SDRAM Controller clock (CPR register)*/
> +#define SDR0_DDRCE                     0x00E0 /* SDR register */
> +#define CPR0_DDR2D                     0x0100 /* CPR register */
> +#define CPR0_DDR2D_DDR2DV_ENCODE(n)    ((((unsigned long)(n))&0x03)<<25)
> +#define CPR0_DDR2D_DDR2DV_DECODE(n)    ((((unsigned long)(n))>>25)&0x03)

Again, please add spaces and change to u32.

Please change and resubmit. Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de