On 2017-11-27 11:00 +0200, Laurent Pinchart wrote:
On Monday, 27 November 2017 06:05:03 EET Archit Taneja wrote:
On 2017-11-05 11:41 -0500, Nick Bowler wrote:
[...]
Bisection implicates the following commit:
181e0ef092a4952aa523c5b9cb21394cf43bcd46 is the first bad commit commit 181e0ef092a4952aa523c5b9cb21394cf43bcd46 Author: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Date: Mon Mar 6 01:35:57 2017 +0200
drm: bridge: dw-hdmi: Fix the PHY power up sequence
[...]
The two main things the commit below does it to a) correctly wait on the TX_PHY_LOCK bit to be asserted and b) use usleep_range() instead of udelay().
Another difference is that the PWDN and TMDS signals, in theory needed for Gen1 PHYs only, are not set anymore for Gen2 PHYs. Nick, could you test the following change to see if it makes a difference ?
I do not notice any difference with this change applied on top of Linux 4.15-rc1.
A note about the test setup: I had to remove the test equipment so I no longer have any information about the video mode from the sink side (like in the photos). Thus, with the current setup, I am using the presense or absense of audio to determine whether the issue is present or not.
The test procedure is: boot up, start music, then hotplug the hdmi four times. If sound is heard after all four connections, PASS; otherwise FAIL.
- I retested on 4.15-rc1 to confirm that the issue is still present (it is).
- I applied the functional revert from earlier on top of 4.15-rc1 and the problem is fixed.
- Returning to 4.15-rc1 and applying this next patch -- the issue is present again (no change in behaviour compared to 4.15-rc1).
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/ bridge/synopsys/dw-hdmi.c index b172139502d6..1c18ff1bf24a 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1104,14 +1104,14 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) unsigned int i; u8 val;
- if (phy->gen == 1) {
dw_hdmi_phy_enable_powerdown(hdmi, false);
- dw_hdmi_phy_enable_powerdown(hdmi, false);
/* Toggle TMDS enable. */dw_hdmi_phy_enable_tmds(hdmi, 0);dw_hdmi_phy_enable_tmds(hdmi, 1);
- /* Toggle TMDS enable. */
- dw_hdmi_phy_enable_tmds(hdmi, 0);
- dw_hdmi_phy_enable_tmds(hdmi, 1);
- if (phy->gen == 1) return 0;
}
dw_hdmi_phy_gen2_txpwron(hdmi, 1); dw_hdmi_phy_gen2_pddq(hdmi, 0);
I don't see (b) being a problem. About (a), it's possible that the bit above is interpreted differently on a rockchip SoC versus a renesas chip. Could you print the value of HDMI_PHY_STAT0 that's read back?
[...]
As an experiment, could you forcefully return 0 instead of -ETIMEDOUT and see if things return back to normal?
I did both of these tests at once by applying the below patch on top of 4.15-rc1. There is no change in behaviour compared to 4.15-rc1 (except for the added printouts).
With this, every time after inserting the cable the following is printed:
[ 128.002965] dwhdmi-rockchip ff980000.hdmi: 0: HDMI_PHY_STAT0: f2 [ 128.004614] dwhdmi-rockchip ff980000.hdmi: 1: HDMI_PHY_STAT0: f3 [ 128.013752] dwhdmi-rockchip ff980000.hdmi: 0: HDMI_PHY_STAT0: f2 [ 128.015605] dwhdmi-rockchip ff980000.hdmi: 1: HDMI_PHY_STAT0: f3
And there is no difference in output between working and non-working cases. I've attached the full log; I manually logged extra messages to give context from the test procedure:
"hdmi (not) working" - after bootup or connecting the cable (indicating test pass/fail) "hdmi disconnect" - after unplugging the cable.
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index bf14214fa464..0358f6020fb4 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1118,7 +1118,11 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi)
/* Wait for PHY PLL lock */ for (i = 0; i < 5; ++i) { - val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK; + val = hdmi_readb(hdmi, HDMI_PHY_STAT0); + + dev_info(hdmi->dev, "%u: HDMI_PHY_STAT0: %.2hhx\n", i, val); + + val &= HDMI_PHY_TX_PHY_LOCK; if (val) break;
@@ -1127,7 +1131,7 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi)
if (!val) { dev_err(hdmi->dev, "PHY PLL failed to lock\n"); - return -ETIMEDOUT; + return 0; }
dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i);
Let me know if there's anything else I should try.
Thanks, Nick