From patchwork Tue Mar 28 15:40:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Javier Martinez Canillas X-Patchwork-Id: 9650003 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 973B760113 for ; Tue, 28 Mar 2017 15:40:54 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 88D66282E8 for ; Tue, 28 Mar 2017 15:40:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7B6952839C; Tue, 28 Mar 2017 15:40:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id AAF66282E8 for ; Tue, 28 Mar 2017 15:40:53 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 20DF96E28B; Tue, 28 Mar 2017 15:40:52 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from osg.samsung.com (ec2-52-27-115-49.us-west-2.compute.amazonaws.com [52.27.115.49]) by gabe.freedesktop.org (Postfix) with ESMTP id A86706E28B for ; Tue, 28 Mar 2017 15:40:50 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by osg.samsung.com (Postfix) with ESMTP id 07D07A05EF; Tue, 28 Mar 2017 15:41:14 +0000 (UTC) X-Virus-Scanned: amavisd-new at osg.samsung.com Received: from osg.samsung.com ([127.0.0.1]) by localhost (s-opensource.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kI0JOQdggIzD; Tue, 28 Mar 2017 15:41:13 +0000 (UTC) Received: from [192.168.0.10] (unknown [181.121.164.133]) by osg.samsung.com (Postfix) with ESMTPSA id 40FA2A05EC; Tue, 28 Mar 2017 15:41:09 +0000 (UTC) Subject: Re: [PATCH 41/41] drm/bridge: analogix_dp: Properly disable aux chan retries on rockchip To: Doug Anderson , Andrzej Hajda References: <20170310043305.17216-1-seanpaul@chromium.org> <20170310043305.17216-42-seanpaul@chromium.org> From: Javier Martinez Canillas Message-ID: <662cb447-2a40-bc01-440d-6a2817d272a3@osg.samsung.com> Date: Tue, 28 Mar 2017 11:40:40 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Cc: =?UTF-8?B?5b6B5aKeIOeOiw==?= , Lin Huang , Tomeu Vizoso , Shuah Khan , "dri-devel@lists.freedesktop.org" , "open list:ARM/Rockchip SoC..." , Krzysztof Kozlowski , Yakir Yang , =?UTF-8?Q?St=c3=a9phane_Marchesin?= X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP Hello Doug, On 03/22/2017 12:59 PM, Doug Anderson wrote: > Hi, > > On Wed, Mar 22, 2017 at 3:57 AM, Andrzej Hajda wrote: >> On 10.03.2017 05:32, Sean Paul wrote: >>> From: Douglas Anderson >>> >>> 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 >>> Cc: 征增 王 >>> Signed-off-by: Douglas Anderson >>> Signed-off-by: Sean Paul >>> --- >>> 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: Best regards, 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);