diff mbox

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

Message ID 20170310043305.17216-42-seanpaul@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Paul March 10, 2017, 4:32 a.m. UTC 
From: Douglas Anderson <dianders@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@chromium.org>
Cc: 征增 王 <wzz@rock-chips.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Andrzej Hajda March 22, 2017, 10:57 a.m. UTC | #1 
On 10.03.2017 05:32, Sean Paul wrote:
> From: Douglas Anderson <dianders@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@chromium.org>
> Cc: 征增 王 <wzz@rock-chips.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@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. What is the point
in setting retry interval then?
Was it tested on other analogix users (exynos), I mean this patch and
whole patchset?

Regards
Andrzej

>  	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_HW_RETRY_CTL);
>  
>  	/* Receive AUX Channel DEFER commands equal to DEFFER_COUNT*64 */
Doug Anderson March 22, 2017, 3:59 p.m. UTC | #2 
Hi,

On Wed, Mar 22, 2017 at 3:57 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 10.03.2017 05:32, Sean Paul wrote:
>> From: Douglas Anderson <dianders@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@chromium.org>
>> Cc: 征增 王 <wzz@rock-chips.com>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> Signed-off-by: Sean Paul <seanpaul@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.

-Doug
diff mbox

Patch

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;
+
 	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_HW_RETRY_CTL);
 
 	/* Receive AUX Channel DEFER commands equal to DEFFER_COUNT*64 */