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

[Tom <Tom.Rix@windriver.com>]

Nick Thompson wrote:
> Wolfgang Denk wrote:
>> Dear Nick Thompson,
>>
>> In message <4AE5DFFD.2090801@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@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
> 

There are a large number of #define's in davinci that could be
converted to structures.  My opinion is that da8xx sharing code
with davinci is a big enough change.  The reworking of basic
davici register calling through structs falls outside of what
I expect.

The cleanup on the calling I am looking for is switching
for direct access to registers to calling them through readl/writel.

Tom