[PATCH 41/41] drm/bridge: analogix_dp: Properly disable aux chan retries on rockchip

[Javier Martinez Canillas]

Hello Doug,

On 03/22/2017 12:59 PM, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 22, 2017 at 3:57 AM, Andrzej Hajda <a.hajda at samsung.com> wrote:
>> On 10.03.2017 05:32, Sean Paul wrote:
>>> From: Douglas Anderson <dianders at chromium.org>
>>>
>>> The comments in analogix_dp_init_aux() claim that we're disabling aux
>>> channel retries, but then right below it for Rockchip it sets them to
>>> 3.  If we actually need 3 retries for Rockchip then we could adjust
>>> the comment, but it seems more likely that we want the same retry
>>> behavior across all platforms.
>>>
>>> Cc: Stéphane Marchesin <marcheu at chromium.org>
>>> Cc: 征增 王 <wzz at rock-chips.com>
>>> Signed-off-by: Douglas Anderson <dianders at chromium.org>
>>> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>>> ---
>>>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 15 ++++++++-------
>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> index 29d130222636..57dd1991d7de 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> @@ -480,15 +480,16 @@ void analogix_dp_init_aux(struct analogix_dp_device *dp)
>>>
>>>       analogix_dp_reset_aux(dp);
>>>
>>> -     /* Disable AUX transaction H/W retry */
>>> +     /* AUX_BIT_PERIOD_EXPECTED_DELAY doesn't apply to Rockchip IP */
>>>       if (dp->plat_data && is_rockchip(dp->plat_data->dev_type))
>>> -             reg = AUX_BIT_PERIOD_EXPECTED_DELAY(0) |
>>> -                   AUX_HW_RETRY_COUNT_SEL(3) |
>>> -                   AUX_HW_RETRY_INTERVAL_600_MICROSECONDS;
>>> +             reg = 0;
>>>       else
>>> -             reg = AUX_BIT_PERIOD_EXPECTED_DELAY(3) |
>>> -                   AUX_HW_RETRY_COUNT_SEL(0) |
>>> -                   AUX_HW_RETRY_INTERVAL_600_MICROSECONDS;
>>> +             reg = AUX_BIT_PERIOD_EXPECTED_DELAY(3);
>>> +
>>> +     /* Disable AUX transaction H/W retry */
>>> +     reg |= AUX_HW_RETRY_COUNT_SEL(0) |
>>> +            AUX_HW_RETRY_INTERVAL_600_MICROSECONDS;
>>> +
>>
>> As I understand you want to disable H/W retry for all.
> 
> Basically I'm trying to make the code match the comments, which it
> didn't before.
> 
> On the exynos side, it's very clear that we should disable retries.
> The docs say "it is advisable to set this bit field to 0 because
> hardware retry will be valid only when DP Rx does not send any reply."
> 
> On Rockchip, I don't see that in the docs.  However:
> 
> * Since the root IP block is the same/similar, presumably it suffers
> the same issues.  It is plausible that the Rockchip IP block is newer
> and can somehow be made to use the HW retries, but I don't actually
> know that.
> 
> * Presumably elsewhere in the driver we are already assuming that the
> HW retries aren't there and we're doing the work to retry things in
> software.  I don't see any special cases for Rockchip there saying
> something like "on Rockchip, we enable HW retries, so bypass SW
> retries".
> 
>> What is the point
>> in setting retry interval then?
> 
> Probably nothing, but the old exynos code it used to do this.  Note
> that 600 us is actually 0, so we could certainly skip it.
> 
>> Was it tested on other analogix users (exynos), I mean this patch and
>> whole patchset?
> 
> Unless I messed up, this particular patch isn't supposed to change
> anything for exynos.
> 
> For the whole patch series, it's probably worthwhile to add Javier to
> the series (CCed here).  In the past he has been helpful in getting
> things tested on exynos-based boards.
>

I finally had time to test the patch series this morning (sorry for the delay)
on an Exynos5800 Peach Pi Chromebook.

The display works but the series breaks DPMS. The machine hangs if I do this:

$ echo 1 > /sys/devices/platform/exynos-drm/graphics/fb0/blank

I don't have a serial console attached to the machine so I can't see what's
happening but doing git bisect I found that the regression is introduced by:

[PATCH 21/41] drm/bridge: analogix_dp: Ensure edp is disabled when shutting down the panel

The problem is that this patch calls analogix_dp_set_analog_power_down() but
it does so after the PHY was disabled with phy_power_off().

So the following diff on top of the patch series makes things to work again:

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index bc9a0a28ab1d..b480946b647c 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1336,8 +1336,8 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 	if (dp->plat_data->power_off)
 		dp->plat_data->power_off(dp->plat_data);
 
-	phy_power_off(dp->phy);
 	analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
+	phy_power_off(dp->phy);
 
 	clk_disable_unprepare(dp->clock);

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America