Dear Nick Thompson,

In message <4ae5dffd.2090...@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.thomp...@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: w...@denx.de
Prepare for tomorrow -- get ready.
        -- Edith Keeler, "The City On the Edge of Forever",
           stardate unknown
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to