[U-Boot] [PATCH v3 1/2] ftahbc020s: Faraday FTAHBC020s AHB Bus Controller

[Wolfgang Denk]

Dear Macpaul Lin,

In message <1303797876-28548-1-git-send-email-macpaul at andestech.com> you wrote:
> ftahbc020s.h provides basic definitions of this controller
> to help a SoC which use this AHB Controller could
> do scalable software settings in lowlevel_init.S.
> 
> 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.
...
> +/* this section is used by lowlevel_init.S */
> +#define FTAHBC020S_SLAVE_BSR_0		0x00	/* Slave n Base/Size Reg */
> +#define FTAHBC020S_SLAVE_BSR_1		0x04
> +#define FTAHBC020S_SLAVE_BSR_2		0x08
> +#define FTAHBC020S_SLAVE_BSR_3		0x0C
> +#define FTAHBC020S_SLAVE_BSR_4		0x10
> +#define FTAHBC020S_SLAVE_BSR_5		0x14
> +#define FTAHBC020S_SLAVE_BSR_6		0x18
> +#define FTAHBC020S_SLAVE_BSR_7		0x1C
> +#define FTAHBC020S_SLAVE_BSR_8		0x20
> +#define FTAHBC020S_SLAVE_BSR_9		0x24
> +#define FTAHBC020S_SLAVE_BSR_10		0x28

See previous comment: I think this should be done using asm-offsets
instead.

> +#define FTAHBC020S_SLAVE_BSR_BASE(x)	(((x) & 0xFFF) << 20)
> +#define FTAHBC020S_SLAVE_BSR_SIZE(x)	(((x) & 0xF) << 16)
> +
> +#define FTAHBC020S_SLAVE_BSR_SIZE_1M	0x0
> +#define FTAHBC020S_SLAVE_BSR_SIZE_2M	0x1
> +#define FTAHBC020S_SLAVE_BSR_SIZE_4M	0x2
> +#define FTAHBC020S_SLAVE_BSR_SIZE_8M	0x3
> +#define FTAHBC020S_SLAVE_BSR_SIZE_16M	0x4
> +#define FTAHBC020S_SLAVE_BSR_SIZE_32M	0x5
> +#define FTAHBC020S_SLAVE_BSR_SIZE_64M	0x6
> +#define FTAHBC020S_SLAVE_BSR_SIZE_128M	0x7
> +#define FTAHBC020S_SLAVE_BSR_SIZE_256M	0x8
> +#define FTAHBC020S_SLAVE_BSR_SIZE_512M	0x9
> +#define FTAHBC020S_SLAVE_BSR_SIZE_1G	0xA
> +#define FTAHBC020S_SLAVE_BSR_SIZE_2G	0xB

I recommend to use a generic macro here, as recommended for the other
patch.

> +/*
> + * FTAHBC020S_PCR - Priority Control Register
> + */
> +#define FTAHBC020S_PCR_PLEVEL_15	(1 << 15)
> +#define FTAHBC020S_PCR_PLEVEL_14	(1 << 14)
> +#define FTAHBC020S_PCR_PLEVEL_13	(1 << 13)
> +#define FTAHBC020S_PCR_PLEVEL_12	(1 << 12)
> +#define FTAHBC020S_PCR_PLEVEL_11	(1 << 11)
> +#define FTAHBC020S_PCR_PLEVEL_10	(1 << 10)
> +#define FTAHBC020S_PCR_PLEVEL_09	(1 << 9)
> +#define FTAHBC020S_PCR_PLEVEL_08	(1 << 8)
> +#define FTAHBC020S_PCR_PLEVEL_07	(1 << 7)
> +#define FTAHBC020S_PCR_PLEVEL_06	(1 << 6)
> +#define FTAHBC020S_PCR_PLEVEL_05	(1 << 5)
> +#define FTAHBC020S_PCR_PLEVEL_04	(1 << 4)
> +#define FTAHBC020S_PCR_PLEVEL_03	(1 << 3)
> +#define FTAHBC020S_PCR_PLEVEL_02	(1 << 2)
> +#define FTAHBC020S_PCR_PLEVEL_01	(1 << 1)

Ditto here.  Why flooding the code with tons of (mostly) unused
defines?

Use:

	#define FTAHBC020S_PCR_PLEVEL(x)	(1 << (x))


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
Vor allem kein Gedanke! Nichts ist kompromittierender als ein  Gedan-
ke!            - Friedrich Wilhelm Nietzsche _Der Fall Wagner_ (1888)