[5/5] drm/i915/bxt: add DDI port HW readout support
diff mbox

Message ID 1434637557-4856-5-git-send-email-imre.deak@intel.com
State New
Headers show

Commit Message

Imre Deak June 18, 2015, 2:25 p.m. UTC
Add support for reading out the HW state for DDI ports. Since the actual
programming is very similar to the CHV/VLV DPIO PLL programming we can
reuse much of the logic from there.

This fixes the state checker failures I saw on my BXT with HDMI output.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
 drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
 2 files changed, 29 insertions(+), 8 deletions(-)

Comments

Ville Syrjälä June 22, 2015, 1:44 p.m. UTC | #1
On Thu, Jun 18, 2015 at 05:25:57PM +0300, Imre Deak wrote:
> Add support for reading out the HW state for DDI ports. Since the actual
> programming is very similar to the CHV/VLV DPIO PLL programming we can
> reuse much of the logic from there.
> 
> This fixes the state checker failures I saw on my BXT with HDMI output.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
>  drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
>  2 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bba0691..fcf6ad5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1169,10 +1169,12 @@ enum skl_disp_power_wells {
>  #define _PORT_PLL_EBB_0_A		0x162034
>  #define _PORT_PLL_EBB_0_B		0x6C034
>  #define _PORT_PLL_EBB_0_C		0x6C340
> -#define   PORT_PLL_P1_MASK		(0x07 << 13)
> -#define   PORT_PLL_P1(x)		((x)  << 13)
> -#define   PORT_PLL_P2_MASK		(0x1f << 8)
> -#define   PORT_PLL_P2(x)		((x)  << 8)
> +#define   PORT_PLL_P1_SHIFT		13
> +#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
> +#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
> +#define   PORT_PLL_P2_SHIFT		8
> +#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
> +#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
>  #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
>  						_PORT_PLL_EBB_0_B,	\
>  						_PORT_PLL_EBB_0_C)
> @@ -1192,8 +1194,9 @@ enum skl_disp_power_wells {
>  /* PORT_PLL_0_A */
>  #define   PORT_PLL_M2_MASK		0xFF
>  /* PORT_PLL_1_A */
> -#define   PORT_PLL_N_MASK		(0x0F << 8)
> -#define   PORT_PLL_N(x)			((x) << 8)
> +#define   PORT_PLL_N_SHIFT		8
> +#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
> +#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
>  /* PORT_PLL_2_A */
>  #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
>  /* PORT_PLL_3_A */
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ca970ba..6859068 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -971,8 +971,26 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
>  static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
>  				enum intel_dpll_id dpll)
>  {
> -	/* FIXME formula not available in bspec */
> -	return 0;
> +	struct intel_shared_dpll *pll;
> +	struct intel_dpll_hw_state *state;
> +	intel_clock_t clock;
> +
> +	/* For DDI ports we always use a shared PLL. */
> +	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
> +		return 0;
> +
> +	pll = &dev_priv->shared_dplls[dpll];
> +	state = &pll->config.hw_state;
> +
> +	clock.m1 = 2;

For chv I opted to read out the m1 divider from PLL_DW1 too even though
2 is the only supported value. Doing so might catch some cases where the
register gets misprogrammed.

But that's a minor issue, so even without this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
> +	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
> +		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;

I just realized we're missing the fracnen check from chv_crtc_clock_get().
Care to send a patch to add the check there as well?

> +	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;
> +	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
> +	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
> +
> +	return vlv_calc_port_clock(100000, &clock);
>  }
>  
>  static void bxt_ddi_clock_get(struct intel_encoder *encoder,
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Imre Deak June 22, 2015, 2:22 p.m. UTC | #2
On ma, 2015-06-22 at 16:44 +0300, Ville Syrjälä wrote:
> On Thu, Jun 18, 2015 at 05:25:57PM +0300, Imre Deak wrote:
> > Add support for reading out the HW state for DDI ports. Since the actual
> > programming is very similar to the CHV/VLV DPIO PLL programming we can
> > reuse much of the logic from there.
> > 
> > This fixes the state checker failures I saw on my BXT with HDMI output.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
> >  drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
> >  2 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index bba0691..fcf6ad5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1169,10 +1169,12 @@ enum skl_disp_power_wells {
> >  #define _PORT_PLL_EBB_0_A		0x162034
> >  #define _PORT_PLL_EBB_0_B		0x6C034
> >  #define _PORT_PLL_EBB_0_C		0x6C340
> > -#define   PORT_PLL_P1_MASK		(0x07 << 13)
> > -#define   PORT_PLL_P1(x)		((x)  << 13)
> > -#define   PORT_PLL_P2_MASK		(0x1f << 8)
> > -#define   PORT_PLL_P2(x)		((x)  << 8)
> > +#define   PORT_PLL_P1_SHIFT		13
> > +#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
> > +#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
> > +#define   PORT_PLL_P2_SHIFT		8
> > +#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
> > +#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
> >  #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
> >  						_PORT_PLL_EBB_0_B,	\
> >  						_PORT_PLL_EBB_0_C)
> > @@ -1192,8 +1194,9 @@ enum skl_disp_power_wells {
> >  /* PORT_PLL_0_A */
> >  #define   PORT_PLL_M2_MASK		0xFF
> >  /* PORT_PLL_1_A */
> > -#define   PORT_PLL_N_MASK		(0x0F << 8)
> > -#define   PORT_PLL_N(x)			((x) << 8)
> > +#define   PORT_PLL_N_SHIFT		8
> > +#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
> > +#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
> >  /* PORT_PLL_2_A */
> >  #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
> >  /* PORT_PLL_3_A */
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index ca970ba..6859068 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -971,8 +971,26 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
> >  static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
> >  				enum intel_dpll_id dpll)
> >  {
> > -	/* FIXME formula not available in bspec */
> > -	return 0;
> > +	struct intel_shared_dpll *pll;
> > +	struct intel_dpll_hw_state *state;
> > +	intel_clock_t clock;
> > +
> > +	/* For DDI ports we always use a shared PLL. */
> > +	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
> > +		return 0;
> > +
> > +	pll = &dev_priv->shared_dplls[dpll];
> > +	state = &pll->config.hw_state;
> > +
> > +	clock.m1 = 2;
> 
> For chv I opted to read out the m1 divider from PLL_DW1 too even though
> 2 is the only supported value. Doing so might catch some cases where the
> register gets misprogrammed.

I haven't found the BXT counterpart for this at least when looking
through BSpec. PORT_PLL_1[2:0] is "i_fbpredivratio" and it reads 0 for
me.

> But that's a minor issue, so even without this is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
> > +	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
> > +		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;
> 
> I just realized we're missing the fracnen check from chv_crtc_clock_get().
> Care to send a patch to add the check there as well?

Ok.

> > +	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;

Btw, now that I looked through BSpec and the code again, this looks like
also be tied to 1, so we'd need to check for this in
bxt_ddi_pll_select(). chv_find_best_dpll() does the right thing already.

> > +	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
> > +	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
> > +
> > +	return vlv_calc_port_clock(100000, &clock);
> >  }
> >  
> >  static void bxt_ddi_clock_get(struct intel_encoder *encoder,
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Ville Syrjälä June 22, 2015, 2:40 p.m. UTC | #3
On Mon, Jun 22, 2015 at 05:22:33PM +0300, Imre Deak wrote:
> On ma, 2015-06-22 at 16:44 +0300, Ville Syrjälä wrote:
> > On Thu, Jun 18, 2015 at 05:25:57PM +0300, Imre Deak wrote:
> > > Add support for reading out the HW state for DDI ports. Since the actual
> > > programming is very similar to the CHV/VLV DPIO PLL programming we can
> > > reuse much of the logic from there.
> > > 
> > > This fixes the state checker failures I saw on my BXT with HDMI output.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
> > >  drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
> > >  2 files changed, 29 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index bba0691..fcf6ad5 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1169,10 +1169,12 @@ enum skl_disp_power_wells {
> > >  #define _PORT_PLL_EBB_0_A		0x162034
> > >  #define _PORT_PLL_EBB_0_B		0x6C034
> > >  #define _PORT_PLL_EBB_0_C		0x6C340
> > > -#define   PORT_PLL_P1_MASK		(0x07 << 13)
> > > -#define   PORT_PLL_P1(x)		((x)  << 13)
> > > -#define   PORT_PLL_P2_MASK		(0x1f << 8)
> > > -#define   PORT_PLL_P2(x)		((x)  << 8)
> > > +#define   PORT_PLL_P1_SHIFT		13
> > > +#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
> > > +#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
> > > +#define   PORT_PLL_P2_SHIFT		8
> > > +#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
> > > +#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
> > >  #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
> > >  						_PORT_PLL_EBB_0_B,	\
> > >  						_PORT_PLL_EBB_0_C)
> > > @@ -1192,8 +1194,9 @@ enum skl_disp_power_wells {
> > >  /* PORT_PLL_0_A */
> > >  #define   PORT_PLL_M2_MASK		0xFF
> > >  /* PORT_PLL_1_A */
> > > -#define   PORT_PLL_N_MASK		(0x0F << 8)
> > > -#define   PORT_PLL_N(x)			((x) << 8)
> > > +#define   PORT_PLL_N_SHIFT		8
> > > +#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
> > > +#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
> > >  /* PORT_PLL_2_A */
> > >  #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
> > >  /* PORT_PLL_3_A */
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index ca970ba..6859068 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -971,8 +971,26 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
> > >  static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
> > >  				enum intel_dpll_id dpll)
> > >  {
> > > -	/* FIXME formula not available in bspec */
> > > -	return 0;
> > > +	struct intel_shared_dpll *pll;
> > > +	struct intel_dpll_hw_state *state;
> > > +	intel_clock_t clock;
> > > +
> > > +	/* For DDI ports we always use a shared PLL. */
> > > +	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
> > > +		return 0;
> > > +
> > > +	pll = &dev_priv->shared_dplls[dpll];
> > > +	state = &pll->config.hw_state;
> > > +
> > > +	clock.m1 = 2;
> > 
> > For chv I opted to read out the m1 divider from PLL_DW1 too even though
> > 2 is the only supported value. Doing so might catch some cases where the
> > register gets misprogrammed.
> 
> I haven't found the BXT counterpart for this at least when looking
> through BSpec. PORT_PLL_1[2:0] is "i_fbpredivratio" and it reads 0 for
> me.

That's the one.

> 
> > But that's a minor issue, so even without this is
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > > +	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
> > > +	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
> > > +		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;
> > 
> > I just realized we're missing the fracnen check from chv_crtc_clock_get().
> > Care to send a patch to add the check there as well?
> 
> Ok.
> 
> > > +	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;
> 
> Btw, now that I looked through BSpec and the code again, this looks like
> also be tied to 1, so we'd need to check for this in
> bxt_ddi_pll_select(). chv_find_best_dpll() does the right thing already.

You mean WARN if n != 1? I suppose it can't hurt.

> 
> > > +	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
> > > +	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
> > > +
> > > +	return vlv_calc_port_clock(100000, &clock);
> > >  }
> > >  
> > >  static void bxt_ddi_clock_get(struct intel_encoder *encoder,
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bba0691..fcf6ad5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1169,10 +1169,12 @@  enum skl_disp_power_wells {
 #define _PORT_PLL_EBB_0_A		0x162034
 #define _PORT_PLL_EBB_0_B		0x6C034
 #define _PORT_PLL_EBB_0_C		0x6C340
-#define   PORT_PLL_P1_MASK		(0x07 << 13)
-#define   PORT_PLL_P1(x)		((x)  << 13)
-#define   PORT_PLL_P2_MASK		(0x1f << 8)
-#define   PORT_PLL_P2(x)		((x)  << 8)
+#define   PORT_PLL_P1_SHIFT		13
+#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
+#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
+#define   PORT_PLL_P2_SHIFT		8
+#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
+#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
 #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
 						_PORT_PLL_EBB_0_B,	\
 						_PORT_PLL_EBB_0_C)
@@ -1192,8 +1194,9 @@  enum skl_disp_power_wells {
 /* PORT_PLL_0_A */
 #define   PORT_PLL_M2_MASK		0xFF
 /* PORT_PLL_1_A */
-#define   PORT_PLL_N_MASK		(0x0F << 8)
-#define   PORT_PLL_N(x)			((x) << 8)
+#define   PORT_PLL_N_SHIFT		8
+#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
+#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
 /* PORT_PLL_2_A */
 #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
 /* PORT_PLL_3_A */
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ca970ba..6859068 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -971,8 +971,26 @@  static void hsw_ddi_clock_get(struct intel_encoder *encoder,
 static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
 				enum intel_dpll_id dpll)
 {
-	/* FIXME formula not available in bspec */
-	return 0;
+	struct intel_shared_dpll *pll;
+	struct intel_dpll_hw_state *state;
+	intel_clock_t clock;
+
+	/* For DDI ports we always use a shared PLL. */
+	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
+		return 0;
+
+	pll = &dev_priv->shared_dplls[dpll];
+	state = &pll->config.hw_state;
+
+	clock.m1 = 2;
+	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
+	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
+		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;
+	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;
+	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
+	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
+
+	return vlv_calc_port_clock(100000, &clock);
 }
 
 static void bxt_ddi_clock_get(struct intel_encoder *encoder,