Re: [PATCH 1/2] drm/i915/ehl/dsi: Set lane latency optimization for DW1

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2019-06-19 at 15:38 -0700, Matt Roper wrote:
> On Tue, Jun 18, 2019 at 12:59:59PM -0700, José Roberto de Souza
> wrote:
> > From: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > 
> > EHL has 2 additional steps in the DSI sequence, this is one of then
> > the lane latency optimization for DW1.
> > 
> > BSpec: 20597
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/icl_dsi.c | 11 +++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h        |  2 ++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> > b/drivers/gpu/drm/i915/display/icl_dsi.c
> > index 74448e6bf749..ee85428b309f 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > @@ -403,6 +403,17 @@ static void
> > gen11_dsi_config_phy_lanes_sequence(struct intel_encoder *encoder)
> >  		tmp &= ~FRC_LATENCY_OPTIM_MASK;
> >  		tmp |= FRC_LATENCY_OPTIM_VAL(0x5);
> >  		I915_WRITE(ICL_PORT_TX_DW2_GRP(port), tmp);
> > +		/* For EHL set latency optimization for PCS_DW1 lanes
> > */
> > +		if (IS_ELKHARTLAKE(dev_priv)) {
> > +			tmp = I915_READ(ICL_PORT_PCS_DW1_AUX(port));
> > +			tmp &= ~LATENCY_OPTIM_MASK;
> > +			tmp |= LATENCY_OPTIM_VAL(0);
> > +			I915_WRITE(ICL_PORT_PCS_DW1_AUX(port), tmp);
> > +			tmp = I915_READ(ICL_PORT_PCS_DW1_LN0(port));
> > +			tmp &= ~LATENCY_OPTIM_MASK;
> > +			tmp |= LATENCY_OPTIM_VAL(0x1);
> > +			I915_WRITE(ICL_PORT_PCS_DW1_GRP(port), tmp);
> > +		}
> 
> Minor nitpick, but these sequences might be slightly easier to read
> if
> there was a blank line separating each R/M/W chunk.

Agreed, I was avoiding change the patch because it is not mine but this
will not hurt anyone.

> 
> The changes here look correct according to the description on bspec
> page
> 20597 although it looks like the bspec authors forgot to update the
> 'Valid Values' section for these bits on page 20398; not sure if you
> want to file a bspec defect about that or not.

Well the missing value is 0, that is the default after reset so we are
safe here.

> 
> >  	}
> >  
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index d6483b5dc8e5..1f2c3ebdf87b 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1896,6 +1896,8 @@ enum i915_power_well_id {
> >  #define ICL_PORT_PCS_DW1_GRP(port)	_MMIO(_ICL_PORT_PCS_DW_GRP(1,
> > port))
> >  #define ICL_PORT_PCS_DW1_LN0(port)	_MMIO(_ICL_PORT_PCS_DW_LN(1, 0,
> > port))
> >  #define   COMMON_KEEPER_EN		(1 << 26)
> > +#define   LATENCY_OPTIM_MASK		(0x3 << 2)
> > +#define   LATENCY_OPTIM_VAL(x)		((x) << 2)
> 
> Should we try to include part of the name of the register in these
> definitions (e.g., DW1_LATENCY_OPTIM)?  I'm not sure if we should
> worry
> about people mixing up these vs the FRC_LATENCY_OPTIM defines farther
> down for the TX_DW2 register.

I also prefer to include the register name to the bits but the general
rule is keep consistent with around code or fix everything, as this is
not mine patch I left this way.

> 
> Up to you on whether you think it's worth clarifying the
> naming.  Either
> way,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Thanks

> 
> >  
> >  /* CNL/ICL Port TX registers */
> >  #define _CNL_PORT_TX_AE_GRP_OFFSET		0x162340
> > -- 
> > 2.22.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx