Re: [PATCH 1/4] drm/i915: Turn get_aux_clock_divider() into per-product vfuncs

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

On Mon, 20 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> A tiny clean-up to allow better code separation between platforms.

Should it also say "per-platform" instead of "per-product" in the
subject?

> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 79 ++++++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h |  2 +
>  2 files changed, 58 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 885d271..08e8e34 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -353,31 +353,49 @@ 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,
> -				      int index)
> +/*
> + * 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

This part of the comment clearly belongs above
i9xx_get_aux_clock_divider.

> + *
> + * Note that PCH attached eDP panels should use a 125MHz input clock divider.
> + */

But I'm not sure where this should go. Is this even valid anymore?

With that comment moved and possible fixed, or just nuked,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +
> +static uint32_t i9xx_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  {
>  	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_VALLEYVIEW(dev)) {
> -		return index ? 0 : 100;
> -	} else if (intel_dig_port->port == PORT_A) {
> -		if (index)
> -			return 0;
> -		if (HAS_DDI(dev))
> -			return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
> -		else if (IS_GEN6(dev) || IS_GEN7(dev))
> +	return index ? 0 : intel_hrawclk(dev) / 2;
> +}
> +
> +static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +
> +	if (index)
> +		return 0;
> +
> +	if (intel_dig_port->port == PORT_A) {
> +		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 {
> +		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> +	}
> +}
> +
> +static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	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;
> +
> +	if (intel_dig_port->port == PORT_A) {
> +		if (index)
> +			return 0;
> +		return DIV_ROUND_CLOSEST(intel_ddi_get_cdclk_freq(dev_priv), 2000);
>  	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
>  		/* Workaround for non-ULT HSW */
>  		switch (index) {
> @@ -385,13 +403,16 @@ static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
>  		case 1: return 72;
>  		default: return 0;
>  		}
> -	} else if (HAS_PCH_SPLIT(dev)) {
> +	} else  {
>  		return index ? 0 : DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> -	} else {
> -		return index ? 0 :intel_hrawclk(dev) / 2;
>  	}
>  }
>  
> +static uint32_t vlv_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> +{
> +	return index ? 0 : 100;
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		uint8_t *send, int send_bytes,
> @@ -450,7 +471,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		goto out;
>  	}
>  
> -	while ((aux_clock_divider = get_aux_clock_divider(intel_dp, clock++))) {
> +	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
>  		/* Must try at least 3 times according to DP spec */
>  		for (try = 0; try < 5; try++) {
>  			/* Load the send data into the aux channel data registers */
> @@ -1613,10 +1634,12 @@ static void intel_edp_psr_enable_sink(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 = get_aux_clock_divider(intel_dp, 0);
> +	uint32_t aux_clock_divider;
>  	int precharge = 0x3;
>  	int msg_size = 5;       /* Header(4) + Message(1) */
>  
> +	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> +
>  	/* Enable PSR in sink */
>  	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
>  		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> @@ -3637,6 +3660,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	const char *name = NULL;
>  	int type, error;
>  
> +	/* intel_dp vfuncs */
> +	if (IS_VALLEYVIEW(dev))
> +		intel_dp->get_aux_clock_divider = vlv_get_aux_clock_divider;
> +	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		intel_dp->get_aux_clock_divider = hsw_get_aux_clock_divider;
> +	else if (HAS_PCH_SPLIT(dev))
> +		intel_dp->get_aux_clock_divider = ilk_get_aux_clock_divider;
> +	else
> +		intel_dp->get_aux_clock_divider = i9xx_get_aux_clock_divider;
> +
>  	/* Preserve the current hw state. */
>  	intel_dp->DP = I915_READ(intel_dp->output_reg);
>  	intel_dp->attached_connector = intel_connector;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2b72b1d..6aeb62a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -492,6 +492,8 @@ struct intel_dp {
>  	unsigned long last_backlight_off;
>  	bool psr_setup_done;
>  	struct intel_connector *attached_connector;
> +
> +	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>  };
>  
>  struct intel_digital_port {
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center