[U-Boot] [PATCH v3 2/2] ftsdmc021: add register definitions of ftsdmc021

[Wolfgang Denk]

Dear Macpaul Lin,

In message <1303797876-28548-2-git-send-email-macpaul at andestech.com> you wrote:
> Support registers definitions of ftsdmc021 SDRAM controller.
> 
> Signed-off-by: Macpaul Lin <macpaul at andestech.com>
> ---
> Changes for v2:
>  - Add __ASSEMBLY__ protecton to register offset for supporting lowlevel_init
> Changes for v3:
>  - Patch: no change. Changed a mail server to resend this.
...
> +#ifdef __ASSEMBLY__
> +#define FTSDMC021_OFFSET_TP1		0x00	/* SDRAM Timing Parameter 1 */
> +#define FTSDMC021_OFFSET_TP2		0x04	/* SDRAM Timing Parameter 2 */
> +#define FTSDMC021_OFFSET_CR1		0x08	/* SDRAM Configuration Reg 1 */
> +#define FTSDMC021_OFFSET_CR2		0x0C	/* SDRAM Configuration Reg 2 */
> +#define FTSDMC021_OFFSET_BANK0_BSR	0x10	/* External Bank Base/Size Reg 0 */
> +#define FTSDMC021_OFFSET_BANK1_BSR	0x14	/* External Bank Base/Size Reg 1 */
> +#define FTSDMC021_OFFSET_BANK2_BSR	0x18	/* External Bank Base/Size Reg 2 */
> +#define FTSDMC021_OFFSET_BANK3_BSR	0x1C	/* External Bank Base/Size Reg 3 */
> +#define FTSDMC021_OFFSET_BANK4_BSR	0x20	/* External Bank Base/Size Reg 4 */
> +#define FTSDMC021_OFFSET_BANK5_BSR	0x24	/* External Bank Base/Size Reg 5 */
> +#define FTSDMC021_OFFSET_BANK6_BSR	0x28	/* External Bank Base/Size Reg 6 */
> +#define FTSDMC021_OFFSET_BANK7_BSR	0x2C	/* External Bank Base/Size Reg 7 */

Lines too long.  Please fix globally.

I think it is generally wrong to manually define these offsets here.
You should use a C struct instead, and auto-generate the offsets if
needed using the asm-offsets approach (see top level Makefile for
details).

> +/* 12-bit base address of external bank.
> + * Default value is 0x800.
> + * The 12-bit equals to the haddr[31:20] of AHB address bus. */
> +#define FTSDMC021_BANK_BASE(x)		((x) & 0xfff)
> +
> +#define FTSDMC021_BANK_SIZE_1M		0x0
> +#define FTSDMC021_BANK_SIZE_2M		0x1
> +#define FTSDMC021_BANK_SIZE_4M		0x2
> +#define FTSDMC021_BANK_SIZE_8M		0x3
> +#define FTSDMC021_BANK_SIZE_16M		0x4
> +#define FTSDMC021_BANK_SIZE_32M		0x5
> +#define FTSDMC021_BANK_SIZE_64M		0x6
> +#define FTSDMC021_BANK_SIZE_128M	0x7
> +#define FTSDMC021_BANK_SIZE_256M	0x8
> +#define FTSDMC021_BANK_SIZE_512M	0x9

Why don't you use a generic macro here, like

	#define FTSDMC021_BANK_SIZE(sz)	(ffs(x) - 21)

?


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
Accident: A condition in which presence of mind is good, but  absence
of body is better.