Re: [PATCH 1/3] drm/i915: Disable M2 frac division for integer case

From: "Purushothaman, Vijay A" <vijay.a.purushothaman@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Intel Graphics <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/3] drm/i915: Disable M2 frac division for integer case
Date: Thu, 05 Mar 2015 19:19:30 +0530	[thread overview]
Message-ID: <54F85EEA.6030304@linux.intel.com> (raw)
In-Reply-To: <20150303153623.GE11371@intel.com>

On 3/3/2015 9:06 PM, Ville Syrjälä wrote:
> On Tue, Mar 03, 2015 at 08:41:54PM +0530, Vijay Purushothaman wrote:
>> v2 : Handle M2 frac division for both M2 frac and int cases
>>
>> v3 : Addressed Ville's review comments. Cleared the old bits for RMW
>>
>> Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h      |    1 +
>>   drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++------
>>   2 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 55143cb..8200e98 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1029,6 +1029,7 @@ enum skl_disp_power_wells {
>>   #define  DPIO_CHV_FIRST_MOD		(0 << 8)
>>   #define  DPIO_CHV_SECOND_MOD		(1 << 8)
>>   #define  DPIO_CHV_FEEDFWD_GAIN_SHIFT	0
>> +#define  DPIO_CHV_FEEDFWD_GAIN_MASK		(0xF << 0)
>>   #define CHV_PLL_DW3(ch) _PIPE(ch, _CHV_PLL_DW3_CH0, _CHV_PLL_DW3_CH1)
>>   
>>   #define _CHV_PLL_DW6_CH0		0x8018
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 7298796..15904a8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6131,6 +6131,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
>>   	enum dpio_channel port = vlv_pipe_to_channel(pipe);
>>   	u32 loopfilter, intcoeff;
>>   	u32 bestn, bestm1, bestm2, bestp1, bestp2, bestm2_frac;
>> +	u32 dpio_val;
>>   	int refclk;
>>   
>>   	bestn = pipe_config->dpll.n;
>> @@ -6139,6 +6140,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
>>   	bestm2 = pipe_config->dpll.m2 >> 22;
>>   	bestp1 = pipe_config->dpll.p1;
>>   	bestp2 = pipe_config->dpll.p2;
>> +	dpio_val = 0;
>>   
>>   	/*
>>   	 * Enable Refclk and SSC
>> @@ -6163,13 +6165,23 @@ static void chv_prepare_pll(struct intel_crtc *crtc,
>>   			DPIO_CHV_M1_DIV_BY_2 |
>>   			1 << DPIO_CHV_N_DIV_SHIFT);
>>   
>> -	/* M2 fraction division */
>> -	vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW2(port), bestm2_frac);
>> +	if (bestm2_frac) {
>> +		/* M2 fraction division */
>> +		vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW2(port), bestm2_frac);
>>   
>> -	/* M2 fraction division enable */
>> -	vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port),
>> -		       DPIO_CHV_FRAC_DIV_EN |
>> -		       (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT));
>> +		/* M2 fraction division enable */
>> +		dpio_val = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW3(port));
>> +		dpio_val |= DPIO_CHV_FRAC_DIV_EN;
>> +		dpio_val &= ~DPIO_CHV_FEEDFWD_GAIN_MASK;
>> +		dpio_val |= (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT);
>> +		vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port), dpio_val);
>> +
>> +	} else {
>> +		/* M2 fraction division disable */
>> +		dpio_val = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW3(port));
>> +		dpio_val &= ~DPIO_CHV_FRAC_DIV_EN;
>> +		vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port), dpio_val);
> You're not writing the feedfwd gain here. The register docs list it as
> 'Feedforwad gain for fractional mode/SSC mode PLL'. The SSC part there
> makes me think these bits might mean something even if the fractional
> divider is not used.
>
> At least I don't see any harm in setting it even if the fractional
> divider is not used. So with that in mind I'd probably write this as
> something like:
>
> val = read(PLL_DW3)
> val &= ~(FRAC_DIV_EN | FEEDFWD_GAIN_MASK)
> val |= 2 << FEEDFWD_GAIN_SHIFT;
> if (bestm2_trac)
> 	val |= FRAC_DIV_EN;
> write(PLL_DW3, val);
>
> This should also make it less likely we will accidentally update
> only one of the branches in the future when both need changing.
Makes sense. I will send v4 with this change and add your signed-off-by tag.

Thanks,
Vijay
>
>> +	}
>>   
>>   	/* Loop filter */
>>   	refclk = i9xx_get_refclk(crtc, 0);
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx