Hi, On Wed, Mar 22, 2017 at 3:57 AM, Andrzej Hajda <a.hajda@xxxxxxxxxxx> wrote: > On 10.03.2017 05:32, Sean Paul wrote: >> From: Douglas Anderson <dianders@xxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxx> >> Cc: 征增 王 <wzz@xxxxxxxxxxxxxx> >> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> >> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx> >> --- >> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
