Re: [PATCH v3] drm/i915/bxt: eDP Panel Power sequencing

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 07 May 2015, Vandana Kannan <vandana.kannan@intel.com> wrote:
> Changes for BXT - added a IS_BROXTON check to use the macro related to PPS
> registers for BXT.
> BXT does not have PP_DIV register. Making changes to handle this.
> Second set of PPS registers have been defined but will be used when VBT
> provides a selection between the 2 sets of registers.
>
> v2:
> [Jani] Added 2nd set of PPS registers and the macro
> Jani's review comments
> 	- remove reference in i915_suspend.c
> 	- Use BXT PP macro
> Squashing all PPS related patches into one.
>
> v3: Jani's review comments addressed
> 	- Use pp_ctl instead of pp
> 	- ironlake_get_pp_control() is not required for BXT
> 	- correct the use of && in the print statement
> 	- drop the shift in the print statement
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 13 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c | 74 ++++++++++++++++++++++++++++++++---------
>  2 files changed, 72 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1b31238..777c51a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6315,6 +6315,8 @@ enum skl_disp_power_wells {
>  #define PCH_PP_CONTROL		0xc7204
>  #define  PANEL_UNLOCK_REGS	(0xabcd << 16)
>  #define  PANEL_UNLOCK_MASK	(0xffff << 16)
> +#define  BXT_POWER_CYCLE_DELAY_MASK	(0x1f0)
> +#define  BXT_POWER_CYCLE_DELAY_SHIFT	4
>  #define  EDP_FORCE_VDD		(1 << 3)
>  #define  EDP_BLC_ENABLE		(1 << 2)
>  #define  PANEL_POWER_RESET	(1 << 1)
> @@ -6343,6 +6345,17 @@ enum skl_disp_power_wells {
>  #define  PANEL_POWER_CYCLE_DELAY_MASK	(0x1f)
>  #define  PANEL_POWER_CYCLE_DELAY_SHIFT	0
>  
> +/* BXT PPS changes - 2nd set of PPS registers */
> +#define _BXT_PP_STATUS2		0xc7300
> +#define _BXT_PP_CONTROL2 	0xc7304
> +#define _BXT_PP_ON_DELAYS2	0xc7308
> +#define _BXT_PP_OFF_DELAYS2	0xc730c
> +
> +#define BXT_PP_STATUS(n)	((!n) ? PCH_PP_STATUS : _BXT_PP_STATUS2)
> +#define BXT_PP_CONTROL(n)	((!n) ? PCH_PP_CONTROL : _BXT_PP_CONTROL2)
> +#define BXT_PP_ON_DELAYS(n)	((!n) ? PCH_PP_ON_DELAYS : _BXT_PP_ON_DELAYS2)
> +#define BXT_PP_OFF_DELAYS(n)	((!n) ? PCH_PP_OFF_DELAYS : _BXT_PP_OFF_DELAYS2)
> +
>  #define PCH_DP_B		0xe4100
>  #define PCH_DPB_AUX_CH_CTL	0xe4110
>  #define PCH_DPB_AUX_CH_DATA1	0xe4114
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bacdec5..3082224 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -559,7 +559,9 @@ static u32 _pp_ctrl_reg(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  
> -	if (HAS_PCH_SPLIT(dev))
> +	if (IS_BROXTON(dev))
> +		return BXT_PP_CONTROL(0);
> +	else if (HAS_PCH_SPLIT(dev))
>  		return PCH_PP_CONTROL;
>  	else
>  		return VLV_PIPE_PP_CONTROL(vlv_power_sequencer_pipe(intel_dp));
> @@ -569,7 +571,9 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  
> -	if (HAS_PCH_SPLIT(dev))
> +	if (IS_BROXTON(dev))
> +		return BXT_PP_STATUS(0);
> +	else if (HAS_PCH_SPLIT(dev))
>  		return PCH_PP_STATUS;
>  	else
>  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
> @@ -4969,8 +4973,8 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct edp_power_seq cur, vbt, spec,
>  		*final = &intel_dp->pps_delays;
> -	u32 pp_on, pp_off, pp_div, pp;
> -	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
> +	u32 pp_on, pp_off, pp_div = 0, pp_ctl = 0;
> +	int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg = 0;
>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
> @@ -4978,7 +4982,16 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
>  	if (final->t11_t12 != 0)
>  		return;
>  
> -	if (HAS_PCH_SPLIT(dev)) {
> +	if (IS_BROXTON(dev)) {
> +		/*
> +		 * TODO: BXT has 2 sets of PPS registers.
> +		 * Correct Register for Broxton need to be identified
> +		 * using VBT. hardcoding for now
> +		 */
> +		pp_ctrl_reg = BXT_PP_CONTROL(0);
> +		pp_on_reg = BXT_PP_ON_DELAYS(0);
> +		pp_off_reg = BXT_PP_OFF_DELAYS(0);
> +	} else if (HAS_PCH_SPLIT(dev)) {
>  		pp_ctrl_reg = PCH_PP_CONTROL;
>  		pp_on_reg = PCH_PP_ON_DELAYS;
>  		pp_off_reg = PCH_PP_OFF_DELAYS;
> @@ -4994,12 +5007,17 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
>  
>  	/* Workaround: Need to write PP_CONTROL with the unlock key as
>  	 * the very first thing. */
> -	pp = ironlake_get_pp_control(intel_dp);
> -	I915_WRITE(pp_ctrl_reg, pp);
> +	if (!IS_BROXTON(dev)) {
> +		pp_ctl = ironlake_get_pp_control(intel_dp);
> +		I915_WRITE(pp_ctrl_reg, pp_ctl);
> +	}

I tried to say you should update the ironlake_get_pp_control function
too. It gets called in a number of places, also for bxt, it adds the
unlock key (0xabcd << 16) to the result and the call sites write that
back to pp ctl. And we shouldn't do that since those bits must be zero
on bxt.

With that function fixed, you can read pp ctl once using that, for all
platforms, and only have the !is_broxton branch below for reading pp div
since you'll already have pp ctl.

---

It is practically impossible to express everything clearly and
exhaustively except by effectively rewriting the patch. But that would
defeat the purpose of you writing the patch and me reviewing... so
please do try to think about all the implications of the review
comments. In the end, we all want to get the review done in as few
rounds as possible.

Thanks,
Jani.


>  
>  	pp_on = I915_READ(pp_on_reg);
>  	pp_off = I915_READ(pp_off_reg);
> -	pp_div = I915_READ(pp_div_reg);
> +	if (!IS_BROXTON(dev))
> +		pp_div = I915_READ(pp_div_reg);
> +	else
> +		pp_ctl = I915_READ(pp_ctrl_reg);
>  
>  	/* Pull timing values out of registers */
>  	cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
> @@ -5014,7 +5032,11 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev,
>  	cur.t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
>  		PANEL_POWER_DOWN_DELAY_SHIFT;
>  
> -	cur.t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
> +	if (IS_BROXTON(dev))
> +		cur.t11_t12 = (((pp_ctl & BXT_POWER_CYCLE_DELAY_MASK) >>
> +			BXT_POWER_CYCLE_DELAY_SHIFT) - 1) * 1000;
> +	else
> +		cur.t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
>  		       PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
>  
>  	DRM_DEBUG_KMS("cur t1_t3 %d t8 %d t9 %d t10 %d t11_t12 %d\n",
> @@ -5072,13 +5094,23 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 pp_on, pp_off, pp_div, port_sel = 0;
>  	int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) : intel_hrawclk(dev);
> -	int pp_on_reg, pp_off_reg, pp_div_reg;
> +	int pp_on_reg, pp_off_reg, pp_div_reg = 0, pp_ctrl_reg;
>  	enum port port = dp_to_dig_port(intel_dp)->port;
>  	const struct edp_power_seq *seq = &intel_dp->pps_delays;
>  
>  	lockdep_assert_held(&dev_priv->pps_mutex);
>  
> -	if (HAS_PCH_SPLIT(dev)) {
> +	if (IS_BROXTON(dev)) {
> +		/*
> +		 * TODO: BXT has 2 sets of PPS registers.
> +		 * Correct Register for Broxton need to be identified
> +		 * using VBT. hardcoding for now
> +		 */
> +		pp_ctrl_reg = BXT_PP_CONTROL(0);
> +		pp_on_reg = BXT_PP_ON_DELAYS(0);
> +		pp_off_reg = BXT_PP_OFF_DELAYS(0);
> +
> +	} else if (HAS_PCH_SPLIT(dev)) {
>  		pp_on_reg = PCH_PP_ON_DELAYS;
>  		pp_off_reg = PCH_PP_OFF_DELAYS;
>  		pp_div_reg = PCH_PP_DIVISOR;
> @@ -5104,9 +5136,16 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  		 (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
>  	/* Compute the divisor for the pp clock, simply match the Bspec
>  	 * formula. */
> -	pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
> -	pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> -			<< PANEL_POWER_CYCLE_DELAY_SHIFT);
> +	if (IS_BROXTON(dev)) {
> +		pp_div = I915_READ(pp_ctrl_reg);
> +		pp_div &= ~BXT_POWER_CYCLE_DELAY_MASK;
> +		pp_div |= (DIV_ROUND_UP((seq->t11_t12 + 1), 1000)
> +				<< BXT_POWER_CYCLE_DELAY_SHIFT);
> +	} else {
> +		pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
> +		pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> +				<< PANEL_POWER_CYCLE_DELAY_SHIFT);
> +	}
>  
>  	/* Haswell doesn't have any port selection bits for the panel
>  	 * power sequencer any more. */
> @@ -5123,11 +5162,16 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  
>  	I915_WRITE(pp_on_reg, pp_on);
>  	I915_WRITE(pp_off_reg, pp_off);
> -	I915_WRITE(pp_div_reg, pp_div);
> +	if (IS_BROXTON(dev))
> +		I915_WRITE(pp_ctrl_reg, pp_div);
> +	else
> +		I915_WRITE(pp_div_reg, pp_div);
>  
>  	DRM_DEBUG_KMS("panel power sequencer register settings: PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n",
>  		      I915_READ(pp_on_reg),
>  		      I915_READ(pp_off_reg),
> +		      IS_BROXTON(dev) ?
> +		      (I915_READ(pp_ctrl_reg) & BXT_POWER_CYCLE_DELAY_MASK) :
>  		      I915_READ(pp_div_reg));
>  }
>  
> -- 
> 2.0.1
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx