Re: [PATCH 3/3] drm/i915/bxt: edp1.4 Intermediate Freq support

["Jindal, Sonika" <sonika.jindal@intel.com>]

On 5/7/2015 2:11 PM, Ville Syrjälä wrote:
> On Thu, May 07, 2015 at 09:52:09AM +0530, Sonika Jindal wrote:
>> BXT supports following intermediate link rates for edp:
>> 2.16GHz, 2.43GHz, 3.24GHz, 4.32GHz.
>> Adding support for programming the intermediate rates.
>>
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_ddi.c |   44 ++++++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/intel_dp.c  |    7 +++++-
>>   2 files changed, 48 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 9c1e74a..c0cb5f7 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1397,8 +1397,7 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>>   			clk_div.lanestagger = 0x04;
>>   		else
>>   			clk_div.lanestagger = 0x02;
>> -	} else if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
>> -			intel_encoder->type == INTEL_OUTPUT_EDP) {
>> +	} else if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT) {
>>   		struct drm_encoder *encoder = &intel_encoder->base;
>>   		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>
>> @@ -1416,8 +1415,49 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>>   			clk_div = bxt_dp_clk_val[0];
>>   			DRM_ERROR("Unknown link rate\n");
>>   		}
>> +	} else if (intel_encoder->type == INTEL_OUTPUT_EDP) {
>> +		struct drm_encoder *encoder = &intel_encoder->base;
>> +		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>> +		int link_rate;
>> +
>> +		/*
>> +		 * If edp1.4 intermediate frequency support is present, we set
>> +		 * link_bw to 0 and a valid rate index in rate_select.
>> +		 */
>> +		if (intel_dp->link_bw)
>> +			link_rate = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
>> +		else
>> +			link_rate = intel_dp->sink_rates[intel_dp->rate_select];
>
> The chosen clock should already be passed in, so no there should be no
> need for this. I see the DP case does has the same issue.
Yes it was copied from DP :)

>
>> +
>> +		switch (link_rate) {
>> +		case 162000:
>> +			clk_div = bxt_dp_clk_val[0];
>> +			break;
>> +		case 216000:
>> +			clk_div = bxt_dp_clk_val[3];
>> +			break;
>> +		case 243000:
>> +			clk_div = bxt_dp_clk_val[4];
>> +			break;
>> +		case 270000:
>> +			clk_div = bxt_dp_clk_val[1];
>> +			break;
>> +		case 324000:
>> +			clk_div = bxt_dp_clk_val[5];
>> +			break;
>> +		case 432000:
>> +			clk_div = bxt_dp_clk_val[6];
>> +			break;
>> +		case 540000:
>> +			clk_div = bxt_dp_clk_val[2];
>> +			break;
>> +		default:
>> +			clk_div = bxt_dp_clk_val[0];
>> +			DRM_ERROR("Unknown link rate\n");
>> +		}
>
> This looks rather fragile. I would suggest storing the link rate in
> the bxt_clk_div structure and just looping through the array looking for
> the correct rate. That will also work for normal DP, so less code in the
> end.
Ok, I will do that.

>
>>   	}
>>
>> +
>
> Spurious whitespace.
:(
>
>>   	crtc_state->dpll_hw_state.ebb0 =
>>   		PORT_PLL_P1(clk_div.p1) | PORT_PLL_P2(clk_div.p2);
>>   	crtc_state->dpll_hw_state.pll0 = clk_div.m2_int;
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index c9d50d1..e6ee7c6 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -85,6 +85,8 @@ static const struct dp_link_dpll chv_dpll[] = {
>>   		{ .p1 = 2, .p2 = 1, .n = 1, .m1 = 2, .m2 = 0x6c00000 } }
>>   };
>>
>> +static const int bxt_rates[] = { 162000, 216000, 243000, 270000,
>> +				  324000, 432000, 540000 };
>>   static const int skl_rates[] = { 162000, 216000, 270000,
>>   				  324000, 432000, 540000 };
>>   static const int chv_rates[] = { 162000, 202500, 210000, 216000,
>> @@ -1161,7 +1163,10 @@ intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates)
>>   static int
>>   intel_dp_source_rates(struct drm_device *dev, const int **source_rates)
>>   {
>> -	if (IS_SKYLAKE(dev)) {
>> +	if (IS_BROXTON(dev)) {
>> +		*source_rates = bxt_rates;
>> +		return ARRAY_SIZE(bxt_rates);
>> +	} else if (IS_SKYLAKE(dev)) {
>>   		*source_rates = skl_rates;
>>   		return ARRAY_SIZE(skl_rates);
>>   	} else if (IS_CHERRYVIEW(dev)) {
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> 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