[Intel-gfx] [PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking
Jindal, Sonika
sonika.jindal at intel.com
Wed Jun 24 03:39:49 PDT 2015
Looks good to me:
Reviewed-by: Sonika Jindal <sonika.jindal at 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 at 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: "
>>>
>
>
More information about the Intel-gfx
mailing list