On Sat, 2019-09-14 at 00:26 -0700, Lucas De Marchi wrote:
> On Fri, Sep 13, 2019 at 3:33 PM José Roberto de Souza
> <jose.souza@xxxxxxxxx> wrote:
> > It was always modifing register space of the first phy in the
> > HIP_INDEX_REG for all ports while it should shift 8 bits for each
> > port inside of HIP_INDEX_REG.
> >
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 34
> > +++++++++++++++----
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 10 ++++--
> > drivers/gpu/drm/i915/i915_reg.h | 3 ++
> > 3 files changed, 38 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index a6a2e00cc075..981e24120a87 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -2795,7 +2795,10 @@ tgl_dkl_phy_ddi_vswing_sequence(struct
> > intel_encoder *encoder, int link_clock,
> > * All registers programmed here use HIP_INDEX_REG 0 or 1
> > */
> > for (ln = 0; ln < 2; ln++) {
> > - I915_WRITE(HIP_INDEX_REG(tc_port), ln);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, ln);
>
> INDEX_INDEX?
>
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
>
> we don't really care for a RMW here, do we? We don't access these
> registers in parallel for other ports
> (It would be inherently racy if we did), so
>
> I915_WRITE(HIP_INDEX_REG(tc_port), HIP_INDEX_VAL(tc_port,
> ln));
>
> should be sufficient.
>
Okay
> Also I think all these fixes should be squashed in their
> correspondent
> patches in this series with changelogs
> updated.
>
Yeah, I was trying to avoid the fatigue :D
But I will squash in each patch
> > /* All the registers are RMW */
> > val = I915_READ(DKL_TX_DPCNTL0(tc_port));
> > @@ -3134,7 +3137,10 @@ tgl_phy_clock_gating(struct
> > intel_digital_port *dig_port, bool enable)
> > DKL_DP_MODE_CFG_GAONPWR_GATING;
> >
> > for (ln = 0; ln < 2; ln++) {
> > - I915_WRITE(HIP_INDEX_REG(tc_port), ln);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, ln);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> >
> > val = I915_READ(DKL_DP_MODE(tc_port));
> > if (enable)
> > @@ -3249,16 +3255,23 @@ static void tgl_program_dkl_dp_mode(struct
> > intel_digital_port *intel_dig_port)
> > struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > >base.base.dev);
> > enum port port = intel_dig_port->base.port;
> > enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> > - u32 ln0, ln1, lane_mask, pin_mask;
> > + u32 ln0, ln1, lane_mask, pin_mask, val;
> > int num_lanes;
> >
> > if (tc_port == PORT_TC_NONE ||
> > intel_dig_port->tc_mode == TC_PORT_TBT_ALT)
> > return;
> >
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x0);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x0);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> > ln0 = I915_READ(DKL_DP_MODE(tc_port));
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x1);
> > +
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x1);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> > ln1 = I915_READ(DKL_DP_MODE(tc_port));
> >
> > num_lanes = intel_dig_port->dp.lane_count;
> > @@ -3327,9 +3340,16 @@ static void tgl_program_dkl_dp_mode(struct
> > intel_digital_port *intel_dig_port)
> > return;
> > }
> >
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x0);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x0);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> > I915_WRITE(DKL_DP_MODE(tc_port), ln0);
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x1);
> > +
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x1);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> > I915_WRITE(DKL_DP_MODE(tc_port), ln1);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index afc9b609b63d..9304606c1c0a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -3109,7 +3109,10 @@ static bool dkl_pll_get_hw_state(struct
> > drm_i915_private *dev_priv,
> > * All registers read here have the same HIP_INDEX_REG even
> > though
> > * they are on different building blocks
> > */
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x2);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x2);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> >
> > hw_state->mg_refclkin_ctl =
> > I915_READ(DKL_REFCLKIN_CTL(tc_port));
> > hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
> > @@ -3301,7 +3304,10 @@ static void dkl_pll_write(struct
> > drm_i915_private *dev_priv,
> > * All registers programmed here have the same
> > HIP_INDEX_REG even
> > * though on different building block
> > */
> > - I915_WRITE(HIP_INDEX_REG(tc_port), 0x2);
> > + val = I915_READ(HIP_INDEX_REG(tc_port));
> > + val &= ~HIP_INDEX_MASK(tc_port);
> > + val |= HIP_INDEX_INDEX_VAL(tc_port, 0x2);
> > + I915_WRITE(HIP_INDEX_REG(tc_port), val);
> >
> > /* All the registers are RMW */
> > val = I915_READ(DKL_REFCLKIN_CTL(tc_port));
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 95a4232c8e0a..625f458e9b1c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -10265,6 +10265,9 @@ enum skl_power_gate {
> > #define HIP_INDEX_REG(tc_port) _MMIO((tc_port) < 4 \
> > ? _HIP_INDEX_REG0 \
> > : _HIP_INDEX_REG1)
> > +#define _HIP_INDEX_SHIFT(tc_port) (8 * ((tc_port) % 4))
> > +#define HIP_INDEX_MASK(tc_port) (0xff <<
> > _HIP_INDEX_SHIFT(tc_port))
> > +#define HIP_INDEX_INDEX_VAL(tc_port, index) ((index) <<
> > _HIP_INDEX_SHIFT(tc_port))
>
> #define HIP_INDEX_VAL(index) ((index) | (index) << 8 | ( index) << 16
> > (index) << 24)
> ?
I guess the current one is better, more easy to understand.
>
> Lucas De Marchi
>
> > /* BXT display engine PLL */
> > #define BXT_DE_PLL_CTL _MMIO(0x6d000)
> > --
> > 2.23.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
