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

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

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.