[U-Boot] [PATCH v2 5/5] board: ti: AM57xx: Add detection logic for AM57xx-evm

[Igor Grinberg <grinberg@compulab.co.il>]

On 11/03/15 17:09, Steven Kipisz wrote:
> On 11/03/2015 07:29 AM, Igor Grinberg wrote:
>> Hi Steve,
>>
>> On 11/03/15 14:22, Steve Kipisz wrote:
>>
>> [...]
>>
>>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>>> ---
>>> v2 Based on:
>>>   master     a6104737 ARM: at91: sama5: change the environment address to 0x6000
>>>
>>> Build testing: MAKEALL -s omap4 -s omap5 (no warnings/build errors)
>>>     Boot Testing:
>>>     am57xx_evm_nodt_config: http://pastebin.ubuntu.com/13039296/
>>>     beagle_x15_config: http://pastebin.ubuntu.com/13039331/
>>>
>>> Changes in v2 (since v1):
>>>     - move the board detection code into the new routine
>>>       do_board_detect
>>>     - eliminate board.h and move the ix_xxx into board.c
>>>     - redo commit message to be more clear
>>>
>>> v1:  http://marc.info/?t=144608007900002&r=1&w=2
>>>       http://marc.info/?t=144608007900004&r=1&w=2
>>>     (mailing list squashed original submission)
>>
>> [...]
>>
>>> +#define is_x15()    board_am_is("BBRDX15_")
>>> +#define is_am572x_evm()    board_am_is("AM572PM_")
>>
>> I think board_is_* much more appropriate here...
>>
> Ok. so board_is_x15 and board_is_am572x_evm

Yep. Sounds better.

>>> +
>>>   #ifdef CONFIG_DRIVER_TI_CPSW
>>>   #include <cpsw.h>
>>>   #endif
>>> @@ -246,6 +249,54 @@ struct vcores_data beagle_x15_volts = {
>>>       .iva.pmic        = &tps659038,
>>>   };
>>>
>>> +#ifdef CONFIG_SPL_BUILD
>>> +/* No env to setup for SPL */
>>> +static inline void setup_board_eeprom_env(void) { }
>>> +
>>> +/* Override function to read eeprom information */
>>> +void do_board_detect(void)
>>> +{
>>> +    struct ti_am_eeprom *ep;
>>> +    int rc;
>>> +
>>> +    rc = ti_i2c_eeprom_am_get(CONFIG_EEPROM_BUS_ADDRESS,
>>> +                  CONFIG_EEPROM_CHIP_ADDRESS, &ep);
>>> +    if (rc)
>>> +        printf("ti_i2c_eeprom_init failed %d\n", rc);
>>> +}
>>
>> Do you really need this in SPL?
> 
> Yes. We need to detect the board to determine DDR setup, pin mux, iodelay. All of that needs to be done in SPL. X15 and EVM are the same, but more boards will be added that have some differences.

Ok.

>>
>>> +
>>> +#else    /* CONFIG_SPL_BUILD */
>>> +
>>> +static void setup_board_eeprom_env(void)
>>> +{
>>> +    char *name = NULL;
>>
>> How about:
>>
>>     char *name = "beagle_x15";
>>
>>> +    int rc;
>>> +    struct ti_am_eeprom_printable p;
>>> +
>>> +    rc = ti_i2c_eeprom_am_get_print(CONFIG_EEPROM_BUS_ADDRESS,
>>> +                    CONFIG_EEPROM_CHIP_ADDRESS, &p);
>>> +    if (rc) {
>>> +        printf("Invalid EEPROM data(@0x%p). Default to X15\n",
>>> +               TI_AM_EEPROM_DATA);
>>> +        goto invalid_eeprom;
>>> +    }
>>> +
>>> +    if (is_x15())
>>> +        name = "beagle_x15";
>>
>> This will not be needed if the above comment is implemented.
>>
>>> +    else if (is_am572x_evm())
>>> +        name = "am57xx_evm";
>>> +    else
>>> +        printf("Unidentified board claims %s in eeprom header\n",
>>> +               p.name);
>>> +
>>> +invalid_eeprom:
>>> +    set_board_info_env(name, "beagle_x15", p.version, p.serial);
>>
>> If the above comment is implemented, no more need for the
>> default_name parameter...
>>
> Ok, I'll look at that.
>>> +}
>>> +
>>> +/* Eeprom is alread read by SPL.. nothing more to do here.. */
>>> +
>>> +#endif    /* CONFIG_SPL_BUILD */
>>
>> [...]
>>
>>
> Steve K.
> 

-- 
Regards,
Igor.