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

[Nick Thompson]

Wolfgang Denk wrote:
> 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.

As I mentioned in the patch introduction "[PATCH V3 0/4]..." there are
several places where out of fashion code is present in these patches.

DA8xx SoC's are considered a member of the DaVinci family of SoC's and
so these headers need to maintain compatibility with the relevant
drivers already in the U-Boot git tree.

Those drivers do not use C structures and I/O accessors and so require
the use of #defines as presented here.

Where possible I have not used legacy code forms in new code, but in
the case of new code that also has to use these same registers, I have
again opted for compatibility.

Of course I could duplicate the definitions as defines /and/ C structs,
or even rewrite all the DaVinci family drivers, but I'm hoping neither
of those options was the intention of your comments here.

Can you please confirm whether my assumptions as correct and acceptable
in this case? I.e. (as Tom Rix put it) Can I get a pass on this one?

I think you have a point in the PINMUX cases below however, as I believe
these #defines are only used in SoC specific code...

Best Regards,
Nick

> 
>> +#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
>