Re: [PATCH 5/9] drm/i915: Setup EDP PSR AUX Registers

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

On Wed, 30 Jan 2013, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> From: Shobhit Kumar <shobhit.kumar@intel.com>
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>
> v2: Created aux_clock_divider function to avoid duplicated code.

Bikeshed, that could be a prep patch, but I won't insist on it.

Other comments below.

>     Unfortunatelly dp_aux_ch and psr_aux_ch aren't so similar to reuse full code.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 13 ++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 89 +++++++++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  3 files changed, 83 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7bb3134..10732dc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1563,6 +1563,19 @@
>  #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B)
>  #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B)
>  
> +#define EDP_PSR_AUX_CTL			0x64810
> +#define EDP_PSR_AUX_DATA1		0x64814
> +#define EDP_PSR_AUX_DATA2		0x64818
> +#define EDP_PSR_AUX_DATA3		0x6481c
> +#define EDP_PSR_AUX_DATA4		0x64820
> +#define EDP_PSR_AUX_DATA5		0x64824
> +#define EDP_PSR_STATUS_CTL		0x64840
> +#define   EDP_PSR_STATUS_MASK		(7<<29)
> +#define EDP_PSR_PERF_CNT		0x64844
> +#define EDP_PSR_DEBUG_CTL		0x64860
> +#define   EDP_PSR_DEBUG_MASK_MEMUP	(1<<26)
> +#define   EDP_PSR_DEBUG_MASK_HPD	(1<<25)
> +
>  /* VGA port control */
>  #define ADPA			0x61100
>  #define PCH_ADPA                0xe1100
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index aeb0ef1..fe83e05 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -364,6 +364,34 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  	return status;
>  }
>  
> +static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* The clock divider is based off the hrawclk,
> +	 * and would like to run at 2MHz. So, take the
> +	 * hrawclk value and divide by 2 and use that
> +	 *
> +	 * Note that PCH attached eDP panels should use a 125MHz input
> +	 * clock divider.
> +	 */
> +	if (is_cpu_edp(intel_dp)) {
> +		if (HAS_DDI(dev))
> +			return intel_ddi_get_cdclk_freq(dev_priv) >> 1;
> +		else if (IS_VALLEYVIEW(dev))
> +			return 100;
> +		else if (IS_GEN6(dev) || IS_GEN7(dev))
> +			return 200; /* SNB & IVB eDP input clock at 400Mhz */
> +		else
> +			return 225; /* eDP input clock at 450Mhz */
> +	} else if (HAS_PCH_SPLIT(dev))
> +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> +	else
> +		return intel_hrawclk(dev) / 2;
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		uint8_t *send, int send_bytes,
> @@ -411,26 +439,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  	}
>  
>  	intel_dp_check_edp(intel_dp);
> -	/* The clock divider is based off the hrawclk,
> -	 * and would like to run at 2MHz. So, take the
> -	 * hrawclk value and divide by 2 and use that
> -	 *
> -	 * Note that PCH attached eDP panels should use a 125MHz input
> -	 * clock divider.
> -	 */
> -	if (is_cpu_edp(intel_dp)) {
> -		if (HAS_DDI(dev))
> -			aux_clock_divider = intel_ddi_get_cdclk_freq(dev_priv) >> 1;
> -		else if (IS_VALLEYVIEW(dev))
> -			aux_clock_divider = 100;
> -		else if (IS_GEN6(dev) || IS_GEN7(dev))
> -			aux_clock_divider = 200; /* SNB & IVB eDP input clock at 400Mhz */
> -		else
> -			aux_clock_divider = 225; /* eDP input clock at 450Mhz */
> -	} else if (HAS_PCH_SPLIT(dev))
> -		aux_clock_divider = DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> -	else
> -		aux_clock_divider = intel_hrawclk(dev) / 2;
> +	aux_clock_divider = get_aux_clock_divider(intel_dp);
>  
>  	if (IS_GEN6(dev))
>  		precharge = 3;
> @@ -1440,6 +1449,46 @@ static bool is_edp_psr(struct intel_dp *intel_dp)
>  	return (is_edp(intel_dp) && (intel_dp->psr_dpcd[0] & 0x1));
>  }
>  
> +static void intel_edp_psr_setup(struct intel_dp *intel_dp)
> +{
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t aux_clock_divider;
> +	int precharge = 0x3;
> +	int msg_size = 5;	/* Header(4) + Message(1) */
> +
> +	/* No need to setup if already done as these setting are persistent
> +	 * until power states are entered */

I find "no need to setup" a not good enough reason to not do it every
time. However, the specs say you're not supposed to fiddle with the aux
regs while PSR is enabled, which *is* a good reason. So maybe update the
comment, so we'll know what we're doing if we decide to change this in
the future?

> +	if (intel_dp->psr_setup)
> +		return;
> +
> +	/* Setup AUX registers */
> +	/* Write command on DPCD 0x0600 */
> +	I915_WRITE(EDP_PSR_AUX_DATA1, 0x80060000);
> +
> +	/* Set the state to normal operation D0 in DPCD 0x0600 */
> +	I915_WRITE(EDP_PSR_AUX_DATA2, 0x01000000);

I really really don't like just slamming magic 32-bit values into
registers. Copy-pasting from intel_dp_aux_native_write() and
intel_dp_aux_ch(), you could do something like this here to clarify a
bit:

	uint16_t address = DP_SET_POWER;
	uint8_t msg[] = {
		AUX_NATIVE_WRITE << 4,
		address >> 8,
		address & 0xff,
		0, /* len - 1 */
		DP_SET_POWER_D0,
	};

	for (i = 0; i < sizeof(msg); i += 4)
		I915_WRITE(EDP_PSR_AUX_DATA1 + i,
			   pack_aux(msg + i, sizeof(msg) - i));

The above could be cleaned up too, maybe there's common bits to be found
with the regular aux write stuff etc. But IMHO with that it's already
much easier to figure out, without looking at the specs, what's going
on, and you can (almost?) ditch the comments too.

BR,
Jani.


> +
> +	aux_clock_divider = get_aux_clock_divider(intel_dp);
> +
> +	I915_WRITE(EDP_PSR_AUX_CTL,
> +		   DP_AUX_CH_CTL_TIME_OUT_400us |
> +		   (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> +		   (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> +		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
> +
> +	/* Setup the debug register */
> +	I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) |
> +			EDP_PSR_DEBUG_MASK_MEMUP |
> +			EDP_PSR_DEBUG_MASK_HPD);
> +
> +	/* This flag can be made to 0 from pm code so as to reinitialize the
> +	 * AUX register in case of power states, returning from which will not
> +	 * maintain the AUX register settings
> +	 */
> +	intel_dp->psr_setup = 1;
> +}
> +
>  static void intel_enable_dp(struct intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d4b3bac..3fa2dd0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -383,6 +383,7 @@ struct intel_dp {
>  	int backlight_on_delay;
>  	int backlight_off_delay;
>  	struct delayed_work panel_vdd_work;
> +	uint8_t psr_setup;
>  	bool want_panel_vdd;
>  	struct intel_connector *attached_connector;
>  };
> -- 
> 1.7.11.7
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel