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

Igor Grinberg
2015-11-03T13:29:01Z

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...

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

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

> +}
> +
> +/* Eeprom is alread read by SPL.. nothing more to do here.. */
> +
> +#endif /* CONFIG_SPL_BUILD */

[...]

--
Regards,
Igor.