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

["Kannan, Vandana" <vandana.kannan@intel.com>]

On 5/7/2015 1:07 PM, Jani Nikula wrote:
> 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
>>

[...]

>>   	/* 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.
>
Apologies.. Guess I was in a hurry the other day. Will take care in this 
patch and in future.

Thanks,
Vandana

> 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);
>>

[...]

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