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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
Thanks
>
> >
> > /* CNL/ICL Port TX registers */
> > #define _CNL_PORT_TX_AE_GRP_OFFSET 0x162340
> > --
> > 2.22.0
> >
> > _______________________________________________
> > 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
