[PATCH 02/16] board: traverse: ten64: ensure retimer reset is done on new board revisions

[Peng Fan]

On 7/21/2023 12:39 PM, Mathew McBride wrote:
> Board revision C (production) and later require the SFP+
> retimer to be turned on (or reset) on boot, by way of issuing
> a command to the board's microcontroller (via I2C).
> 
> The comparison statement here was incorrect, as the board
> ID decrements every revision (from 0xFF downwards),
> so this was matching board RevA,B,C instead of Rev >= C.
> 
> Another oops that transpired when working on this issue,
> is that if the board controller is not called (such as
> CONFIG_TEN64_CONTROLLER=n or earlier board rev), then
> the retimer udevice was not obtained. So the board
> version check has to be moved inside board_cycle_retimer
> (which probes/fetches the retimer device) as well.
> 
> Signed-off-by: Mathew McBride <matt at traverse.com.au>
> ---
>   board/traverse/ten64/ten64.c | 37 ++++++++++++++++++------------------
>   1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/board/traverse/ten64/ten64.c b/board/traverse/ten64/ten64.c
> index 88f22e85d7..df44baf24f 100644
> --- a/board/traverse/ten64/ten64.c
> +++ b/board/traverse/ten64/ten64.c
> @@ -341,20 +341,27 @@ static int board_cycle_retimer(struct udevice **retim_dev)
>   	u8 loop;
>   	struct udevice *uc_dev;
>   	struct udevice *i2cbus;
> +	u32 board_rev = ten64_get_board_rev();
>   
>   	ret = ten64_get_micro_udevice(&uc_dev, &i2cbus);
>   	if (ret)
>   		return ret;
>   
> -	ret = dm_i2c_probe(i2cbus, I2C_RETIMER_ADDR, 0, retim_dev);
> -	if (ret == 0) {
> -		puts("(retimer on, resetting...) ");
> +	/* Retimer power cycle not implemented on early board
> +	 * revisions/controller firmwares
> +	 */
> +	if (IS_ENABLED(CONFIG_TEN64_CONTROLLER) &&
> +	    board_rev <= TEN64_BOARD_REV_C) {
> +		ret = dm_i2c_probe(i2cbus, I2C_RETIMER_ADDR, 0, retim_dev);
> +		if (ret == 0) {

'!ret'. Please run checkpatch.pl.

> +			puts("(retimer on, resetting...) ");
>   
> -		ret = misc_call(uc_dev, TEN64_CNTRL_10G_OFF, NULL, 0, NULL, 0);
> -		mdelay(1000);
> -	}
> +			ret = misc_call(uc_dev, TEN64_CNTRL_10G_OFF, NULL, 0, NULL, 0);

return value not checked?

> +			mdelay(1000);
> +		}
>   
> -	ret = misc_call(uc_dev, TEN64_CNTRL_10G_ON, NULL, 0, NULL, 0);
> +		ret = misc_call(uc_dev, TEN64_CNTRL_10G_ON, NULL, 0, NULL, 0);

This is called whether dm_i2c_probe returns failure or success?
And return value not checked?

Regards,
Peng.

> +	}
>   
>   	// Wait for retimer to come back
>   	for (loop = 0; loop < 5; loop++) {
> @@ -375,19 +382,13 @@ static void ten64_board_retimer_ds110df410_init(void)
>   	u8 reg;
>   	int ret;
>   	struct udevice *retim_dev;
> -	u32 board_rev = ten64_get_board_rev();
>   
>   	puts("Retimer: ");
> -	/* Retimer power cycle not implemented on early board
> -	 * revisions/controller firmwares
> -	 */
> -	if (IS_ENABLED(CONFIG_TEN64_CONTROLLER) &&
> -	    board_rev >= TEN64_BOARD_REV_C) {
> -		ret = board_cycle_retimer(&retim_dev);
> -		if (ret) {
> -			puts("Retimer power on failed\n");
> -			return;
> -		}
> +
> +	ret = board_cycle_retimer(&retim_dev);
> +	if (ret) {
> +		puts("Retimer power on failed\n");
> +		return;
>   	}
>   
>   	/* Access to Control/Shared register */