[U-Boot] [PATCH v3 2/3] Add TI816X support

[TENART Antoine]

>     +/*
>     + * Values supported 400,531,675,796
>     + *
>     + * On TI8168 rev C, use 400 or 531 MHz !
>
>
> Why?  Is it specific to the EVM or is it general for all rev. C parts.
>   A pointer to an errata would be good

I don't know if there is an errata, but I couldn't get U-Boot working
with DDR_PLL_675 or DDR_PLL_796 on my rev C. This is only base on my
experience.

>     + */
>     +#define DDR_PLL_400
>
>
> DDR PLL setup is related to the type of memory used on a particular
> board. Seems like it should be set in the board config

I think so too.

>     --- /dev/null
>     +++ b/arch/arm/include/asm/arch-am33xx/ddr_defs_ti816x.h
[snip]
>
> Almost everything in this file should be defined on a per board basis,
> not in a generic header file.  Also should combine defines with the PLL
> setup above so there is no possibility of mismatch between the two

Right, it would make sense to move it to board/ti/ti816x.

>     +#define PULLDOWN_EN (0x0 << 4) /* Pull Down Selection */
>     +#define PULLUP_EN   (0x1 << 4) /* Pull Up Selection */
>     +#define PULLUDEN    (0x0 << 3) /* Pull up enabled */
>     +#define PULLUDDIS   (0x1 << 3) /* Pull up disabled */
>     +#define MODE(val)              val /* used for Readability */
>
>
> shouldn't this have parenthesis to protect the parameter like so
> #define MODE(val) (val)

Indeed.

-- 
Antoine