Re: [PATCH v5 01/13] drm/i915/icl: Configure lane sequencing of combo phy transmitter

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/11/2018 11:16 PM, Jani Nikula wrote:
On Fri, 27 Jul 2018, "Chauhan, Madhav" <madhav.chauhan@xxxxxxxxx> wrote:
-----Original Message-----
From: Chauhan, Madhav
Sent: Friday, July 20, 2018 12:06 AM
To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nikula, Jani <jani.nikula@xxxxxxxxx>;
Zanoni, Paulo R <paulo.r.zanoni@xxxxxxxxx>; Vivi, Rodrigo
<rodrigo.vivi@xxxxxxxxx>
Subject: RE:  [PATCH v5 01/13] drm/i915/icl: Configure lane
sequencing of combo phy transmitter

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx]
Sent: Thursday, July 19, 2018 9:42 PM
To: Chauhan, Madhav <madhav.chauhan@xxxxxxxxx>
Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nikula, Jani
<jani.nikula@xxxxxxxxx>; Zanoni, Paulo R <paulo.r.zanoni@xxxxxxxxx>;
Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
Subject: Re:  [PATCH v5 01/13] drm/i915/icl: Configure lane
sequencing of combo phy transmitter

On Tue, Jul 10, 2018 at 03:10:02PM +0530, Madhav Chauhan wrote:
This patch set the loadgen select and latency optimization for aux
and transmit lanes of combo phy transmitters. It will be used for
MIPI DSI HS operations.
Thanks for reviewing DSI patches.

v2: Rebase

Signed-off-by: Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
---
  drivers/gpu/drm/i915/icl_dsi.c | 38
++++++++++++++++++++++++++++++++++++++
  1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/icl_dsi.c
b/drivers/gpu/drm/i915/icl_dsi.c index 13830e4..a571339 100644
--- a/drivers/gpu/drm/i915/icl_dsi.c
+++ b/drivers/gpu/drm/i915/icl_dsi.c
@@ -105,10 +105,48 @@ static void gen11_dsi_power_up_lanes(struct
intel_encoder *encoder)
  	}
  }

+static void gen11_dsi_config_phy_lanes_sequence(struct
+intel_encoder
+*encoder) {
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
+	u32 tmp;
+	int lane;
tmp/lane could be moved to into the loops.
Was it due to intel_dsi->ports have no port assigned and
loop for_each_dsi_port() will not proceed further??
If that's the case, these encoder enable/disable function should be called
Only when dsi_init is success and then, intel_dsi->ports have some valid port value.

Please clarify.
Ville's comments are purely about style and readability.

Regards,
Madhav

Same in other patches.
Agree, make sense.
Just to understand
+
+	/* Step 4b(i) set loadgen select for transmit and aux lanes */
+	for_each_dsi_port(port, intel_dsi->ports) {
+		tmp = I915_READ(ICL_PORT_TX_DW4_AUX(port));
+		tmp &= ~LOADGEN_SELECT;
+		I915_WRITE(ICL_PORT_TX_DW4_AUX(port), tmp);
+		for (lane = 0; lane <= 3; lane++) {
+			tmp = I915_READ(ICL_PORT_TX_DW4_LN(port,
lane));
+			tmp &= ~LOADGEN_SELECT;
+			if (lane != 2)
+				tmp |= LOADGEN_SELECT;
+			I915_WRITE(ICL_PORT_TX_DW4_LN(port, lane),
tmp);
+		}
+	}
+
+	/* Step 4b(ii) set latency optimization for transmit and aux lanes */
+	for_each_dsi_port(port, intel_dsi->ports) {
+		tmp = I915_READ(ICL_PORT_TX_DW2_AUX(port));
+		tmp &= ~FRC_LATENCY_OPTIM_MASK;
+		tmp |= FRC_LATENCY_OPTIM_VAL(0x5);
+		I915_WRITE(ICL_PORT_TX_DW2_AUX(port), tmp);
+		tmp = I915_READ(ICL_PORT_TX_DW2_LN0(port));
+		tmp &= ~FRC_LATENCY_OPTIM_MASK;
+		tmp |= FRC_LATENCY_OPTIM_VAL(0x5);
+		I915_WRITE(ICL_PORT_TX_DW2_GRP(port), tmp);
The "read something, modify, write something else" pattern always gives
me the creeps. But I guess reading _GRP is not an option?

Anyway, for the actual content,

Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>

Thanks for the review. Yes, we need to read *only* LN0 and if same value, then write through GRP
register.

Regards,
Madhav


+	}
An empty line here and there would make this a bit more legible.

Same in other patches.
Ok.  Thought this will be additional line, multiple Places in code use this :)

Regards,
Madhav

+}
+
  static void gen11_dsi_enable_port_and_phy(struct intel_encoder
*encoder)  {
  	/* step 4a: power up all lanes of the DDI used by DSI */
  	gen11_dsi_power_up_lanes(encoder);
+
+	/* step 4b: configure lane sequencing of the Combo-PHY
+transmitters
*/
+	gen11_dsi_config_phy_lanes_sequence(encoder);
  }

  static void __attribute__((unused))
--
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux