| Message ID | 20190326103146.24795-15-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
On 26.03.2019 11:31, Tomi Valkeinen wrote: > The driver has a loop after ending link training, where it reads the > DPCD link status and prints an error if that status is not ok. > > The loop is unnecessary, as far as I can understand from DP specs, so > let's remove it. We can also print the more specific errors to help > debugging. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej
Hi Tomi, Thank you for the patch. On Tue, Mar 26, 2019 at 12:31:38PM +0200, Tomi Valkeinen wrote: > The driver has a loop after ending link training, where it reads the > DPCD link status and prints an error if that status is not ok. > > The loop is unnecessary, as far as I can understand from DP specs, so > let's remove it. We can also print the more specific errors to help > debugging. I see in tc_link_training() that the driver checks the channel EQ bits through the TC358767 DP0_LTSTAT register. Does the chip read the link status DPCD registers by itself through the AUX link ? If so we could remove this check completely (unless we don't trust the TC358767 and want to double-check). If not, where does it get the link status from ? > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 62 +++++++++++++++++-------------- > 1 file changed, 35 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 700e161015af..220408db82f7 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -968,34 +968,42 @@ static int tc_main_link_enable(struct tc_data *tc) > if (ret < 0) > goto err_dpcd_write; > > - /* Wait */ > - timeout = 100; > - do { > - udelay(1); > - /* Read DPCD 0x202-0x207 */ > - ret = drm_dp_dpcd_read_link_status(aux, tmp + 2); > - if (ret < 0) > - goto err_dpcd_read; > - } while ((--timeout) && > - !(drm_dp_channel_eq_ok(tmp + 2, tc->link.base.num_lanes))); > + /* Check link status */ > + ret = drm_dp_dpcd_read_link_status(aux, tmp); > + if (ret < 0) > + goto err_dpcd_read; > > - if (timeout == 0) { > - /* Read DPCD 0x200-0x201 */ > - ret = drm_dp_dpcd_read(aux, DP_SINK_COUNT, tmp, 2); > - if (ret < 0) > - goto err_dpcd_read; > - dev_err(dev, "channel(s) EQ not ok\n"); > - dev_info(dev, "0x0200 SINK_COUNT: 0x%02x\n", tmp[0]); > - dev_info(dev, "0x0201 DEVICE_SERVICE_IRQ_VECTOR: 0x%02x\n", > - tmp[1]); > - dev_info(dev, "0x0202 LANE0_1_STATUS: 0x%02x\n", tmp[2]); > - dev_info(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n", > - tmp[4]); > - dev_info(dev, "0x0205 SINK_STATUS: 0x%02x\n", tmp[5]); > - dev_info(dev, "0x0206 ADJUST_REQUEST_LANE0_1: 0x%02x\n", > - tmp[6]); > - > - return -EAGAIN; > + ret = 0; > + > + value = tmp[0] & DP_CHANNEL_EQ_BITS; > + > + if (value != DP_CHANNEL_EQ_BITS) { > + dev_err(tc->dev, "Lane 0 failed: %x\n", value); > + ret = -ENODEV; > + } > + > + if (tc->link.base.num_lanes == 2) { > + value = (tmp[0] >> 4) & DP_CHANNEL_EQ_BITS; > + > + if (value != DP_CHANNEL_EQ_BITS) { > + dev_err(tc->dev, "Lane 1 failed: %x\n", value); > + ret = -ENODEV; > + } > + > + if (!(tmp[2] & DP_INTERLANE_ALIGN_DONE)) { > + dev_err(tc->dev, "Interlane align failed\n"); > + ret = -ENODEV; > + } > + } > + > + if (ret) { > + dev_err(dev, "0x0202 LANE0_1_STATUS: 0x%02x\n", tmp[0]); > + dev_err(dev, "0x0203 LANE2_3_STATUS 0x%02x\n", tmp[1]); > + dev_err(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n", tmp[2]); > + dev_err(dev, "0x0205 SINK_STATUS: 0x%02x\n", tmp[3]); > + dev_err(dev, "0x0206 ADJUST_REQUEST_LANE0_1: 0x%02x\n", tmp[4]); > + dev_err(dev, "0x0207 ADJUST_REQUEST_LANE2_3: 0x%02x\n", tmp[5]); > + goto err; > } > > return 0;
On 21/04/2019 01:06, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Mar 26, 2019 at 12:31:38PM +0200, Tomi Valkeinen wrote: >> The driver has a loop after ending link training, where it reads the >> DPCD link status and prints an error if that status is not ok. >> >> The loop is unnecessary, as far as I can understand from DP specs, so >> let's remove it. We can also print the more specific errors to help >> debugging. > > I see in tc_link_training() that the driver checks the channel EQ bits > through the TC358767 DP0_LTSTAT register. Does the chip read the link > status DPCD registers by itself through the AUX link ? If so we could Yes. I'm not exactly sure at what point TC358767 reads the registers, but I think it reads it at the end of link training loop. In theory the link should stay up after that, but as described in the previous patch, that was not what I saw in every case. So I'd rather have an explicit check here at the end. Tomi
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 700e161015af..220408db82f7 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -968,34 +968,42 @@ static int tc_main_link_enable(struct tc_data *tc) if (ret < 0) goto err_dpcd_write; - /* Wait */ - timeout = 100; - do { - udelay(1); - /* Read DPCD 0x202-0x207 */ - ret = drm_dp_dpcd_read_link_status(aux, tmp + 2); - if (ret < 0) - goto err_dpcd_read; - } while ((--timeout) && - !(drm_dp_channel_eq_ok(tmp + 2, tc->link.base.num_lanes))); + /* Check link status */ + ret = drm_dp_dpcd_read_link_status(aux, tmp); + if (ret < 0) + goto err_dpcd_read; - if (timeout == 0) { - /* Read DPCD 0x200-0x201 */ - ret = drm_dp_dpcd_read(aux, DP_SINK_COUNT, tmp, 2); - if (ret < 0) - goto err_dpcd_read; - dev_err(dev, "channel(s) EQ not ok\n"); - dev_info(dev, "0x0200 SINK_COUNT: 0x%02x\n", tmp[0]); - dev_info(dev, "0x0201 DEVICE_SERVICE_IRQ_VECTOR: 0x%02x\n", - tmp[1]); - dev_info(dev, "0x0202 LANE0_1_STATUS: 0x%02x\n", tmp[2]); - dev_info(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n", - tmp[4]); - dev_info(dev, "0x0205 SINK_STATUS: 0x%02x\n", tmp[5]); - dev_info(dev, "0x0206 ADJUST_REQUEST_LANE0_1: 0x%02x\n", - tmp[6]); - - return -EAGAIN; + ret = 0; + + value = tmp[0] & DP_CHANNEL_EQ_BITS; + + if (value != DP_CHANNEL_EQ_BITS) { + dev_err(tc->dev, "Lane 0 failed: %x\n", value); + ret = -ENODEV; + } + + if (tc->link.base.num_lanes == 2) { + value = (tmp[0] >> 4) & DP_CHANNEL_EQ_BITS; + + if (value != DP_CHANNEL_EQ_BITS) { + dev_err(tc->dev, "Lane 1 failed: %x\n", value); + ret = -ENODEV; + } + + if (!(tmp[2] & DP_INTERLANE_ALIGN_DONE)) { + dev_err(tc->dev, "Interlane align failed\n"); + ret = -ENODEV; + } + } + + if (ret) { + dev_err(dev, "0x0202 LANE0_1_STATUS: 0x%02x\n", tmp[0]); + dev_err(dev, "0x0203 LANE2_3_STATUS 0x%02x\n", tmp[1]); + dev_err(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n", tmp[2]); + dev_err(dev, "0x0205 SINK_STATUS: 0x%02x\n", tmp[3]); + dev_err(dev, "0x0206 ADJUST_REQUEST_LANE0_1: 0x%02x\n", tmp[4]); + dev_err(dev, "0x0207 ADJUST_REQUEST_LANE2_3: 0x%02x\n", tmp[5]); + goto err; } return 0;
The driver has a loop after ending link training, where it reads the DPCD link status and prints an error if that status is not ok. The loop is unnecessary, as far as I can understand from DP specs, so let's remove it. We can also print the more specific errors to help debugging. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/bridge/tc358767.c | 62 +++++++++++++++++-------------- 1 file changed, 35 insertions(+), 27 deletions(-)