| Message ID | 1434637557-4856-2-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + > 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". > - 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: " >
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: " > >
On Wed, Jun 24, 2015 at 01:19:10PM +0300, Imre Deak wrote: > On ke, 2015-06-24 at 15:37 +0530, Jindal, Sonika wrote: > > On 6/18/2015 7:55 PM, Imre Deak wrote: > > > @@ -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. Concur with Imre here, comments shouldn't ever explain what the code does, but why. If you need to explain what the code does in comments, then it needs to be refactored (helper function with clear name extracted, better naming of variables/defines, ...). Imo almost all the comments in this code should be remove because they're obvious. -Daniel
Looks good to me: Reviewed-by: Sonika Jindal <sonika.jindal@intel.com> On 6/24/2015 3:49 PM, Imre Deak wrote: > 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. > Hmm correct, checked the description now.. >> >>> + >>> 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. > Yes :). Maybe clean up the other comments in the next attempt. >> >>> - 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: " >>> > >
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; + 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 */ - 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: "
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(-)