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> >> > > + } >> > >> > 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 -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
