| Message ID | 1531215614-6828-2-git-send-email-madhav.chauhan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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. Same in other patches. > + > + /* 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); > + } An empty line here and there would make this a bit more legible. Same in other patches. > +} > + > 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
> -----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. > > Same in other patches. Agree, make sense. > > > + > > + /* 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); > > + } > > 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
> -----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. 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); > > > + } > > > > 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
On Tue, 2018-07-10 at 15:10 +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. > > 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; > + > + /* 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); > + } > +} > I think bspec states that latency optimization should be set only for Transmit lanes 0, 1, 3. Is it fine to use a group access(i.e ICL_PORT_TX_DW2_GRP) here? I think it states also that no latency optimization is needed for the clock lane.
On 9/10/2018 5:50 PM, Lisovskiy, Stanislav wrote: > On Tue, 2018-07-10 at 15:10 +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. >> >> 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; >> + >> + /* 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); >> + } >> +} >> > I think bspec states that latency optimization should be set only for > Transmit lanes 0, 1, 3. Is it fine to use a group access(i.e > ICL_PORT_TX_DW2_GRP) here? I think it states also that no latency > optimization is needed for the clock lane. There is a separate comment added in BSPEC : "The Latency Optimization of the Clock Lane can be either left at it's default value ('h0) or programmed to the same value as the other lanes. If programmed with the same value as the other lanes, then the Group access can be used for PORT_TX_DW2 programming" Regards, Madhav >
On Mon, 2018-09-10 at 20:57 +0530, Madhav Chauhan wrote: > On 9/10/2018 5:50 PM, Lisovskiy, Stanislav wrote: > > On Tue, 2018-07-10 at 15:10 +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. > > > > > > 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; > > > + > > > + /* 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); > > > + } > > > +} > > > > > > > I think bspec states that latency optimization should be set only > > for > > Transmit lanes 0, 1, 3. Is it fine to use a group access(i.e > > ICL_PORT_TX_DW2_GRP) here? I think it states also that no latency > > optimization is needed for the clock lane. > > There is a separate comment added in BSPEC : > "The Latency Optimization of the Clock Lane can be either left at > it's > default value ('h0) > or programmed to the same value as the other lanes. If programmed > with > the same > value as the other lanes, then the Group access can be used for > PORT_TX_DW2 programming" Yep, I saw that, however just wasn't sure did they mean we actually should leave it untouched. Is there actually any difference, if it is programmed or not? If the value is discarded anyway, I guess it should have been stated explicitly. Nevermind then. > > Regards, > Madhav > > > > >
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
On 9/11/2018 11:16 PM, Jani Nikula wrote: > 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> 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@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
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; + + /* 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); + } +} + 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))
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. v2: Rebase Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com> --- drivers/gpu/drm/i915/icl_dsi.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)