[U-Boot] [PATCH v5 2/3] ARM1176: TI: TNETV107X soc initial support

[Cyril Chemparathy]

Hi Tom,

Thanks for the excellent review.

[...]
>> +static struct async_emif_config default_async_emif_config[NUM_CS] = {
>> +     {                       /* CS0 */
>> +             .mode           = ASYNC_EMIF_MODE_NAND,
>> +             .select_strobe  = ASYNC_EMIF_CS0_SELECT_STROBE,
>> +             .extend_wait    = ASYNC_EMIF_CS0_EXTEND_WAIT,
>> +             .wr_setup       = ASYNC_EMIF_CS0_WR_SETUP,
>> +             .wr_strobe      = ASYNC_EMIF_CS0_WR_STROBE,
>> +             .wr_hold        = ASYNC_EMIF_CS0_WR_HOLD,
>> +             .rd_setup       = ASYNC_EMIF_CS0_RD_SETUP,
>> +             .rd_strobe      = ASYNC_EMIF_CS0_RD_STROBE,
>> +             .rd_hold        = ASYNC_EMIF_CS0_RD_HOLD,
>> +             .turn_around    = ASYNC_EMIF_CS0_TURN_AROUND,
>> +             .width          = ASYNC_EMIF_CS0_WIDTH,
>> +     },
[...]

> A better place for all the preceding code would be in an arch *.h
> The NUM_CS should likely be a CONFIG_ paramter.

I have moved this over to board code instead, since the board should
determine these based on the connected device's timings.

[...]
>> +static const struct pll_init_data plls[] = {
>> +     [SYS_PLL] = {
>> +             .config_enable          = CONFIG_PLL_SYS_CONFIG,
>> +             .pll                    = SYS_PLL,
>> +             .internal_osc           = CONFIG_PLL_SYS_INT_OSC,
>> +             .external_freq          = CONFIG_PLL_SYS_EXT_FREQ,
>> +#if (CONFIG_PLL_SYS_CONFIG)
>> +             .pll_freq               = CONFIG_PLL_SYS_PLL_FREQ,
>> +             .div_freq = {
>> +                     CONFIG_PLL_SYS_ARM1176_CLK,
>> +                     CONFIG_PLL_SYS_DSP_CLK,
>> +                     CONFIG_PLL_SYS_DDR_CLK,
>> +                     CONFIG_PLL_SYS_FULL_CLK,
>> +                     CONFIG_PLL_SYS_LCD_CLK,
>> +                     CONFIG_PLL_SYS_VLYNQ_REF_CLK,
>> +                     CONFIG_PLL_SYS_TSC_CLK,
>> +                     CONFIG_PLL_SYS_HALF_CLK,
>> +             },
>> +#endif
>> +     },
[...]

> Similar to above, the preceeding code is better placed in an arch *.h
> Please move.

Again, moved these initializations to board code instead.  Some boards
may opt to run at different rates.

[...]
>> +static const struct pin_config pin_table[] = {
>> +     TNETV107X_MUX_CFG(0, 0, MUX_MODE_1),
>> +     TNETV107X_MUX_CFG(0, 0, MUX_MODE_2),
> 
> Change these immedates '0, 0' into #defines
> Apply globally

These are register indices and bit shifts, not really amenable to macro
definition.  I have put in a comment for the column contents.

Regards
Cyril.