* [RFC][PATCH 2/2] drm/i915: Don't write shared DSI port bits multiple time
2015-09-10 12:53 [PATCH 1/2] drm/i915: Check dual_link instead of port==A|C for DSI ville.syrjala
@ 2015-09-10 12:53 ` ville.syrjala
2015-09-10 13:35 ` [PATCH 1/2] drm/i915: Check dual_link instead of port==A|C for DSI Jani Nikula
1 sibling, 0 replies; 4+ messages in thread
From: ville.syrjala @ 2015-09-10 12:53 UTC (permalink / raw)
To: intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Some DSI bits are present only in one port A register, and not present
in a port C register. Currently we write such bits multiple times
while looping over the ports. This doesn't seem entirely sane as the
effect of the bit has already occurred by the time we've written it for
port A, so any port C register we're supposed to write before frobbing
the shared bit are now done after the frobbing.
I'm not ure if this is a real problem or not, but it's suspicious at the
very least. So change the code so that we write the appropriate registers
in both ports first, then frob the shared bit, and the continue with the
rest of the registers on both ports.
So this patch is totally untested, and potentially utter garbage,
hence sending out as RFC only.
Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_dsi.c | 55 +++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 57768c0..0e8dcee 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -352,18 +352,19 @@ static void intel_dsi_device_ready(struct intel_encoder *encoder)
band_gap_reset(dev_priv);
for_each_dsi_port(port, intel_dsi->ports) {
-
I915_WRITE(MIPI_DEVICE_READY(port), ULPS_STATE_ENTER);
usleep_range(2500, 3000);
+ }
- /* Enable MIPI PHY transparent latch
- * Common bit for both MIPI Port A & MIPI Port C
- * No similar bit in MIPI Port C reg
- */
- val = I915_READ(MIPI_PORT_CTRL(PORT_A));
- I915_WRITE(MIPI_PORT_CTRL(PORT_A), val | LP_OUTPUT_HOLD);
- usleep_range(1000, 1500);
+ /* Enable MIPI PHY transparent latch
+ * Common bit for both MIPI Port A & MIPI Port C
+ * No similar bit in MIPI Port C reg
+ */
+ val = I915_READ(MIPI_PORT_CTRL(PORT_A));
+ I915_WRITE(MIPI_PORT_CTRL(PORT_A), val | LP_OUTPUT_HOLD);
+ usleep_range(1000, 1500);
+ for_each_dsi_port(port, intel_dsi->ports) {
I915_WRITE(MIPI_DEVICE_READY(port), ULPS_STATE_EXIT);
usleep_range(2500, 3000);
@@ -538,21 +539,23 @@ static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
ULPS_STATE_ENTER);
usleep_range(2000, 2500);
+ }
- /* Wait till Clock lanes are in LP-00 state for MIPI Port A
- * only. MIPI Port C has no similar bit for checking
- */
- if (wait_for(((I915_READ(MIPI_PORT_CTRL(PORT_A)) & AFE_LATCHOUT)
- == 0x00000), 30))
- DRM_ERROR("DSI LP not going Low\n");
+ /* Wait till Clock lanes are in LP-00 state for MIPI Port A
+ * only. MIPI Port C has no similar bit for checking
+ */
+ if (wait_for(((I915_READ(MIPI_PORT_CTRL(PORT_A)) & AFE_LATCHOUT)
+ == 0x00000), 30))
+ DRM_ERROR("DSI LP not going Low\n");
- /* Disable MIPI PHY transparent latch
- * Common bit for both MIPI Port A & MIPI Port C
- */
- val = I915_READ(MIPI_PORT_CTRL(PORT_A));
- I915_WRITE(MIPI_PORT_CTRL(PORT_A), val & ~LP_OUTPUT_HOLD);
- usleep_range(1000, 1500);
+ /* Disable MIPI PHY transparent latch
+ * Common bit for both MIPI Port A & MIPI Port C
+ */
+ val = I915_READ(MIPI_PORT_CTRL(PORT_A));
+ I915_WRITE(MIPI_PORT_CTRL(PORT_A), val & ~LP_OUTPUT_HOLD);
+ usleep_range(1000, 1500);
+ for_each_dsi_port(port, intel_dsi->ports) {
I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
usleep_range(2000, 2500);
}
@@ -776,13 +779,13 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder)
mode_hdisplay += intel_dsi->pixel_overlap;
}
- for_each_dsi_port(port, intel_dsi->ports) {
- /* escape clock divider, 20MHz, shared for A and C.
- * device ready must be off when doing this! txclkesc? */
- tmp = I915_READ(MIPI_CTRL(PORT_A));
- tmp &= ~ESCAPE_CLOCK_DIVIDER_MASK;
- I915_WRITE(MIPI_CTRL(PORT_A), tmp | ESCAPE_CLOCK_DIVIDER_1);
+ /* escape clock divider, 20MHz, shared for A and C.
+ * device ready must be off when doing this! txclkesc? */
+ tmp = I915_READ(MIPI_CTRL(PORT_A));
+ tmp &= ~ESCAPE_CLOCK_DIVIDER_MASK;
+ I915_WRITE(MIPI_CTRL(PORT_A), tmp | ESCAPE_CLOCK_DIVIDER_1);
+ for_each_dsi_port(port, intel_dsi->ports) {
/* read request priority is per pipe */
tmp = I915_READ(MIPI_CTRL(port));
tmp &= ~READ_REQUEST_PRIORITY_MASK;
--
2.4.6
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] drm/i915: Check dual_link instead of port==A|C for DSI
2015-09-10 12:53 [PATCH 1/2] drm/i915: Check dual_link instead of port==A|C for DSI ville.syrjala
2015-09-10 12:53 ` [RFC][PATCH 2/2] drm/i915: Don't write shared DSI port bits multiple time ville.syrjala
@ 2015-09-10 13:35 ` Jani Nikula
2015-09-10 13:37 ` Ville Syrjälä
1 sibling, 1 reply; 4+ messages in thread
From: Jani Nikula @ 2015-09-10 13:35 UTC (permalink / raw)
To: ville.syrjala, intel-gfx
On Thu, 10 Sep 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Everywhere else we check for intel_dsi->dual_link instead of the
> port mask. So do the same when setting up the lane configuration
> in MIPI_PORT_CTRL.
>
> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_dsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 781c267..57768c0 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -304,7 +304,7 @@ static void intel_dsi_port_enable(struct intel_encoder *encoder)
> temp &= ~LANE_CONFIGURATION_MASK;
> temp &= ~DUAL_LINK_MODE_MASK;
>
> - if (intel_dsi->ports == ((1 << PORT_A) | (1 << PORT_C))) {
> + if (intel_dsi->dual_link) {
I think having the whole intel_dsi->dual_link field is an unnecessary
extra level of indirection. If there's more than one port, it's dual
link. There could be a helper to get that info from ->ports.
That said, being in line for the time being is more important than doing
the above.
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> temp |= (intel_dsi->dual_link - 1)
> << DUAL_LINK_MODE_SHIFT;
> temp |= intel_crtc->pipe ?
> --
> 2.4.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread