| Message ID | 20230721043931.14188-3-matt@traverse.com.au |
|---|---|
| State | Accepted |
| Commit | 7a041fea2dad412f75168b27167d30eb27b07322 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Ten64 updates 2023-07 | expand |
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@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 */
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) { + 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); + 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); + } // 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 */
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@traverse.com.au> --- board/traverse/ten64/ten64.c | 37 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-)