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

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 27 Jul 2018, "Chauhan, Madhav" <madhav.chauhan@intel.com> wrote:
>> -----Original Message-----
>> From: Chauhan, Madhav
>> Sent: Friday, July 20, 2018 12:06 AM
>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
>> Zanoni, Paulo R <paulo.r.zanoni@intel.com>; Vivi, Rodrigo
>> <rodrigo.vivi@intel.com>
>> Subject: RE: [Intel-gfx] [PATCH v5 01/13] drm/i915/icl: Configure lane
>> sequencing of combo phy transmitter
>> 
>> > -----Original Message-----
>> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> > Sent: Thursday, July 19, 2018 9:42 PM
>> > To: Chauhan, Madhav <madhav.chauhan@intel.com>
>> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
>> > <jani.nikula@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com>;
>> > Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> > Subject: Re: [Intel-gfx] [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@intel.com>
>> > > ---
>> > >  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@intel.com>

>> > > +	}
>> >
>> > 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@lists.freedesktop.org
>> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Ville Syrjälä
>> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx