public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
From: Imre Deak <imre.deak@intel.com>
To: "Jindal, Sonika" <sonika.jindal@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking
Date: Wed, 24 Jun 2015 13:19:10 +0300	[thread overview]
Message-ID: <1435141150.17499.2.camel@intel.com> (raw)
In-Reply-To: <558A814A.907@intel.com>

On ke, 2015-06-24 at 15:37 +0530, Jindal, Sonika wrote:
> 
> On 6/18/2015 7:55 PM, Imre Deak wrote:
> > Although we have a fixed setting for the PLL9 and EBB4 registers, it
> > still makes sense to check them together with the rest of PLL registers.
> >
> > While at it also remove a redundant comment about 10 bit clock enabling.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
> >   drivers/gpu/drm/i915/i915_reg.h      |  3 ++-
> >   drivers/gpu/drm/i915/intel_ddi.c     | 16 +++++++++++++---
> >   drivers/gpu/drm/i915/intel_display.c |  6 ++++--
> >   4 files changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 491ef0c..bf235ff 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -366,7 +366,8 @@ struct intel_dpll_hw_state {
> >   	uint32_t cfgcr1, cfgcr2;
> >
> >   	/* bxt */
> > -	uint32_t ebb0, pll0, pll1, pll2, pll3, pll6, pll8, pll10, pcsdw12;
> > +	uint32_t ebb0, ebb4, pll0, pll1, pll2, pll3, pll6, pll8, pll9, pll10,
> > +		 pcsdw12;
> >   };
> >
> >   struct intel_shared_dpll_config {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 4bbc85a..bba0691 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1207,7 +1207,8 @@ enum skl_disp_power_wells {
> >   /* PORT_PLL_8_A */
> >   #define   PORT_PLL_TARGET_CNT_MASK	0x3FF
> >   /* PORT_PLL_9_A */
> > -#define  PORT_PLL_LOCK_THRESHOLD_MASK	0xe
> > +#define  PORT_PLL_LOCK_THRESHOLD_SHIFT	1
> > +#define  PORT_PLL_LOCK_THRESHOLD_MASK	(0x7 << PORT_PLL_LOCK_THRESHOLD_SHIFT)
> >   /* PORT_PLL_10_A */
> >   #define  PORT_PLL_DCO_AMP_OVR_EN_H	(1<<27)
> >   #define  PORT_PLL_DCO_AMP_MASK		0x3c00
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index bdc5677..ca970ba 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1476,11 +1476,15 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
> >
> >   	crtc_state->dpll_hw_state.pll8 = targ_cnt;
> >
> > +	crtc_state->dpll_hw_state.pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
> > +
> >   	if (dcoampovr_en_h)
> >   		crtc_state->dpll_hw_state.pll10 = PORT_PLL_DCO_AMP_OVR_EN_H;
> >
> >   	crtc_state->dpll_hw_state.pll10 |= PORT_PLL_DCO_AMP(dco_amp);
> >
> > +	crtc_state->dpll_hw_state.ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
> You can add " | PORT_PLL_RECALIBRATE" and check for this one as well in 
> bxt_ddi_pll_get_hw_state.

I'm not sure. This bit is cleared after the PLL gets enabled, so we'd
have a mismatch when reading out the HW state. I'd regard this as a
control bit that's not part of the programmed state.

> 
> > +
> >   	crtc_state->dpll_hw_state.pcsdw12 =
> >   		LANESTAGGER_STRAP_OVRD | lanestagger;
> >
> > @@ -2414,7 +2418,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >
> >   	temp = I915_READ(BXT_PORT_PLL(port, 9));
> >   	temp &= ~PORT_PLL_LOCK_THRESHOLD_MASK;
> > -	temp |= (5 << 1);
> > +	temp |= pll->config.hw_state.pll9;
> >   	I915_WRITE(BXT_PORT_PLL(port, 9), temp);
> >
> >   	temp = I915_READ(BXT_PORT_PLL(port, 10));
> > @@ -2427,8 +2431,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >   	temp = I915_READ(BXT_PORT_PLL_EBB_4(port));
> >   	temp |= PORT_PLL_RECALIBRATE;
> >   	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
> > -	/* Enable 10 bit clock */
> I think it is OK to keep this comment here because all the steps are 
> mentioned in comments, even "disable 10 bit clock".

Yea, but some of those comments just say what is really obvious from the
code right afterwards.

> 
> > -	temp |= PORT_PLL_10BIT_CLK_ENABLE;
> > +	temp &= ~PORT_PLL_10BIT_CLK_ENABLE;
> > +	temp |= pll->config.hw_state.ebb4;
> >   	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
> >
> >   	/* Enable PLL */
> > @@ -2481,6 +2485,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >   	hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port));
> >   	hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK;
> >
> > +	hw_state->ebb4 = I915_READ(BXT_PORT_PLL_EBB_4(port));
> > +	hw_state->ebb4 &= PORT_PLL_10BIT_CLK_ENABLE;
> > +
> >   	hw_state->pll0 = I915_READ(BXT_PORT_PLL(port, 0));
> >   	hw_state->pll0 &= PORT_PLL_M2_MASK;
> >
> > @@ -2501,6 +2508,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >   	hw_state->pll8 = I915_READ(BXT_PORT_PLL(port, 8));
> >   	hw_state->pll8 &= PORT_PLL_TARGET_CNT_MASK;
> >
> > +	hw_state->pll9 = I915_READ(BXT_PORT_PLL(port, 9));
> > +	hw_state->pll9 &= PORT_PLL_LOCK_THRESHOLD_MASK;
> > +
> >   	hw_state->pll10 = I915_READ(BXT_PORT_PLL(port, 10));
> >   	hw_state->pll10 &= PORT_PLL_DCO_AMP_OVR_EN_H |
> >   			   PORT_PLL_DCO_AMP_MASK;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9149410..6f79680 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11905,17 +11905,19 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >   	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
> >
> >   	if (IS_BROXTON(dev)) {
> > -		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, "
> > +		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
> >   			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
> > -			      "pll6: 0x%x, pll8: 0x%x, pcsdw12: 0x%x\n",
> > +			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pcsdw12: 0x%x\n",
> >   			      pipe_config->ddi_pll_sel,
> >   			      pipe_config->dpll_hw_state.ebb0,
> > +			      pipe_config->dpll_hw_state.ebb4,
> >   			      pipe_config->dpll_hw_state.pll0,
> >   			      pipe_config->dpll_hw_state.pll1,
> >   			      pipe_config->dpll_hw_state.pll2,
> >   			      pipe_config->dpll_hw_state.pll3,
> >   			      pipe_config->dpll_hw_state.pll6,
> >   			      pipe_config->dpll_hw_state.pll8,
> > +			      pipe_config->dpll_hw_state.pll9,
> >   			      pipe_config->dpll_hw_state.pcsdw12);
> >   	} else if (IS_SKYLAKE(dev)) {
> >   		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: "
> >


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2015-06-24 10:19 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-18 14:25 [PATCH 1/5] drm/i915/bxt: mask off the DPLL state checker bits we don't program Imre Deak
2015-06-18 14:25 ` [PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking Imre Deak
2015-06-24 10:07   ` Jindal, Sonika
2015-06-24 10:19     ` Imre Deak [this message]
2015-06-24 10:39       ` Daniel Vetter
2015-06-24 10:39       ` Jindal, Sonika
2015-06-18 14:25 ` [PATCH 3/5] drm/i915/bxt: add PLL10 to the PLL state dumper Imre Deak
2015-06-24 10:40   ` Jindal, Sonika
2015-06-18 14:25 ` [PATCH 4/5] drm/i915/vlv: factor out vlv_calc_port_clock Imre Deak
2015-06-22 13:33   ` Ville Syrjälä
2015-06-22 13:52     ` Imre Deak
2015-06-22 20:35   ` [PATCH v2 4/5] drm/i915/vlv: move the vlv PLL helper next to its platform counterparts Imre Deak
2015-06-30  3:13     ` Jindal, Sonika
2015-06-22 20:35   ` [PATCH v2 4.1/5] drm/i915: calculate the port clock rate along with other PLL params Imre Deak
2015-06-24 12:53     ` Ville Syrjälä
2015-06-30  9:56       ` Daniel Vetter
2015-06-24 10:16   ` [PATCH 4/5] drm/i915/vlv: factor out vlv_calc_port_clock Jindal, Sonika
2015-06-24 10:20     ` Imre Deak
2015-06-18 14:25 ` [PATCH 5/5] drm/i915/bxt: add DDI port HW readout support Imre Deak
2015-06-22 13:44   ` Ville Syrjälä
2015-06-22 14:22     ` Imre Deak
2015-06-22 14:40       ` Ville Syrjälä
2015-06-22 20:35   ` [PATCH v2 " Imre Deak
2015-06-24 10:40 ` [PATCH 1/5] drm/i915/bxt: mask off the DPLL state checker bits we don't program Jindal, Sonika

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1435141150.17499.2.camel@intel.com \
    --to=imre.deak@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=sonika.jindal@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox