[U-Boot] [PATCH V3 1/4] add TI DA8xx support: DA8xx includes

Tue Oct 27 21:09:34 CET 2009

Dear Nick Thompson,

In message <4AE5DFFD.2090801 at gefanuc.com> you wrote:
> Provides initial support for TI OMAP-L1x/DA8xx SoC devices.
> See http://www.ti.com
> 
> The DA8xx devices are similar to DaVinci devices but have a differing
> memory map and updated peripheral versions.
> 
> Signed-off-by: Nick Thompson <nick.thompson at gefanuc.com>
> ---
...
> +/* required by davinci drivers */

Is this really an essential comment? Drop it!

>  #define	REG(addr)	(*(volatile unsigned int *)(addr))
> -#define REG_P(addr)	((volatile unsigned int *)(addr))
>  
> +/* required by davinci drivers */

Ditto.

>  typedef volatile unsigned int	dv_reg;
>  typedef volatile unsigned int *	dv_reg_p;
...

> +/* for LPSCs in PSC1, 32 + actual id is being used for differentiation */
> +#define DAVINCI_LPSC_BASE	32
> +#define DAVINCI_LPSC_USB11	(DAVINCI_LPSC_BASE + 1)
> +#define DAVINCI_LPSC_USB20	(DAVINCI_LPSC_BASE + 2)
> +#define DAVINCI_LPSC_GPIO	(DAVINCI_LPSC_BASE + 3)
> +#define DAVINCI_LPSC_UHPI	(DAVINCI_LPSC_BASE + 4)
> +#define DAVINCI_LPSC_EMAC	(DAVINCI_LPSC_BASE + 5)
> +#define DAVINCI_LPSC_DDR_EMIF	(DAVINCI_LPSC_BASE + 6)
> +#define DAVINCI_LPSC_McASP0	(DAVINCI_LPSC_BASE + 7)
> +#define DAVINCI_LPSC_McASP1	(DAVINCI_LPSC_BASE + 8)
> +#define DAVINCI_LPSC_McASP2	(DAVINCI_LPSC_BASE + 9)
> +#define DAVINCI_LPSC_SPI1	(DAVINCI_LPSC_BASE + 10)
> +#define DAVINCI_LPSC_I2C1	(DAVINCI_LPSC_BASE + 11)
> +#define DAVINCI_LPSC_UART1	(DAVINCI_LPSC_BASE + 12)
> +#define DAVINCI_LPSC_UART2	(DAVINCI_LPSC_BASE + 13)
> +#define DAVINCI_LPSC_LCDC	(DAVINCI_LPSC_BASE + 16)
> +#define DAVINCI_LPSC_ePWM	(DAVINCI_LPSC_BASE + 17)
> +#define DAVINCI_LPSC_eCAP	(DAVINCI_LPSC_BASE + 20)
> +#define DAVINCI_LPSC_eQEP	(DAVINCI_LPSC_BASE + 21)
> +#define DAVINCI_LPSC_SCR_P0	(DAVINCI_LPSC_BASE + 22)
> +#define DAVINCI_LPSC_SCR_P1	(DAVINCI_LPSC_BASE + 23)
> +#define DAVINCI_LPSC_CR_P3	(DAVINCI_LPSC_BASE + 26)
> +#define DAVINCI_LPSC_L3_CBA_RAM	(DAVINCI_LPSC_BASE + 31)

I think you actually want to use a C struct here, like in many other
places.

Please do not access device registers using plain pointers like (base
address plus offet), but always use I/O accessors with C structs, so
we can have strict type checking by the compiler.

> +#else /* CONFIG_SOC_DA8XX */
> +
> +#define PSC0_MDCTL		(DAVINCI_PSC0_BASE + 0xa00)
> +#define PSC0_MDSTAT		(DAVINCI_PSC0_BASE + 0x800)
> +#define PSC0_PTCMD		(DAVINCI_PSC0_BASE + 0x120)
> +#define PSC0_PTSTAT		(DAVINCI_PSC0_BASE + 0x128)
> +
> +#define PSC1_MDCTL		(DAVINCI_PSC1_BASE + 0xa00)
> +#define PSC1_MDSTAT		(DAVINCI_PSC1_BASE + 0x800)
> +#define PSC1_PTCMD		(DAVINCI_PSC1_BASE + 0x120)
> +#define PSC1_PTSTAT		(DAVINCI_PSC1_BASE + 0x128)

One C struct with a pointer to it?

> +/* Some PLL defines */
> +#define PLL0_PLLCTL		(DAVINCI_PLL_CNTRL0_BASE + 0x100)
> +#define PLL0_PLLM		(DAVINCI_PLL_CNTRL0_BASE + 0x110)
> +#define PLL0_PREDIV		(DAVINCI_PLL_CNTRL0_BASE + 0x114)
> +#define PLL0_POSTDIV		(DAVINCI_PLL_CNTRL0_BASE + 0x128)
> +#define PLL0_DIV1		(DAVINCI_PLL_CNTRL0_BASE + 0x118)
> +#define PLL0_DIV2		(DAVINCI_PLL_CNTRL0_BASE + 0x11c)
> +#define PLL0_DIV3		(DAVINCI_PLL_CNTRL0_BASE + 0x120)
> +#define PLL0_DIV4		(DAVINCI_PLL_CNTRL0_BASE + 0x160)
> +#define PLL0_DIV5		(DAVINCI_PLL_CNTRL0_BASE + 0x164)
> +#define PLL0_DIV6		(DAVINCI_PLL_CNTRL0_BASE + 0x168)
> +#define PLL0_DIV7		(DAVINCI_PLL_CNTRL0_BASE + 0x16c)
> +#define PLL0_DIV8		(DAVINCI_PLL_CNTRL0_BASE + 0x170)
> +#define PLL0_DIV9		(DAVINCI_PLL_CNTRL0_BASE + 0x114)

C struct!

> +/* Boot config */
> +#define PINMUX0			(DAVINCI_BOOTCFG_BASE + 0x120)
> +#define PINMUX1			(DAVINCI_BOOTCFG_BASE + 0x124)
> +#define PINMUX2			(DAVINCI_BOOTCFG_BASE + 0x128)
> +#define PINMUX3			(DAVINCI_BOOTCFG_BASE + 0x12c)
> +#define PINMUX4			(DAVINCI_BOOTCFG_BASE + 0x130)
> +#define PINMUX5			(DAVINCI_BOOTCFG_BASE + 0x134)
> +#define PINMUX6			(DAVINCI_BOOTCFG_BASE + 0x138)
> +#define PINMUX7			(DAVINCI_BOOTCFG_BASE + 0x13c)
> +#define PINMUX8			(DAVINCI_BOOTCFG_BASE + 0x140)
> +#define PINMUX9			(DAVINCI_BOOTCFG_BASE + 0x144)
> +#define PINMUX10		(DAVINCI_BOOTCFG_BASE + 0x148)
> +#define PINMUX11		(DAVINCI_BOOTCFG_BASE + 0x14c)
> +#define PINMUX12		(DAVINCI_BOOTCFG_BASE + 0x150)
> +#define PINMUX13		(DAVINCI_BOOTCFG_BASE + 0x154)
> +#define PINMUX14		(DAVINCI_BOOTCFG_BASE + 0x158)
> +#define PINMUX15		(DAVINCI_BOOTCFG_BASE + 0x15C)
> +#define PINMUX16		(DAVINCI_BOOTCFG_BASE + 0x160)
> +#define PINMUX17		(DAVINCI_BOOTCFG_BASE + 0x164)
> +#define PINMUX18		(DAVINCI_BOOTCFG_BASE + 0x168)
> +#define PINMUX19		(DAVINCI_BOOTCFG_BASE + 0x16c)
> +#define SUSPSRC			(DAVINCI_BOOTCFG_BASE + 0x170)
> +#define CFGCHIP0		(DAVINCI_BOOTCFG_BASE + 0x17c)
> +#define CFGCHIP2		(DAVINCI_BOOTCFG_BASE + 0x184)

C struct!

And so on. Please fix globally, also in the other patches.

...
> +/*
> + * Memory Info
> + */
> +#define CONFIG_SYS_MALLOC_LEN	(0x10000 + 1*1024*1024) /* malloc() len */
> +#define CONFIG_SYS_GBL_DATA_SIZE	128 /* reserved for initial data */
> +#define PHYS_SDRAM_1		DAVINCI_DDR_EMIF_DATA_BASE /* DDR Start */
> +#define PHYS_SDRAM_1_SIZE	0x04000000 /* SDRAM size 64MB */

Please consider using get_ram_size() instead.

> +/*
> + * U-Boot commands
> + */
> +#include <config_cmd_default.h>
> +#define CONFIG_CMD_ENV
> +#define CONFIG_CMD_ASKENV
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_DIAG
> +#define CONFIG_CMD_MII
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_SAVES
> +#define CONFIG_CMD_MEMORY
> +#undef CONFIG_CMD_BDI

Why?

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
Prepare for tomorrow -- get ready.
	-- Edith Keeler, "The City On the Edge of Forever",
	   stardate unknown