Re: [Intel-gfx] [PATCH 10/14] drm/i915/tgl: Fix dkl phy register space addressing

2019-09-18 Thread Souza, Jose 
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
>  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 
> > ---
> >  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_

Re: [Intel-gfx] [PATCH 10/14] drm/i915/tgl: Fix dkl phy register space addressing

2019-09-14 Thread Lucas De Marchi 
On Fri, Sep 13, 2019 at 3:33 PM José Roberto de Souza
 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 
> ---
>  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.

Also I think all these fixes should be squashed in their correspondent
patches in this series with changelogs
updated.

>
> /* 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,

[Intel-gfx] [PATCH 10/14] drm/i915/tgl: Fix dkl phy register space addressing

2019-09-13 Thread José Roberto de Souza 
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 
---
 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);
+   I915_WRITE(HIP_INDEX_REG(tc_port), val);
 
/* 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