On 5/7/2015 1:07 PM, Jani Nikula wrote:
On Thu, 07 May 2015, Vandana Kannan <vandana.kan...@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
[...]
Apologies.. Guess I was in a hurry the other day. Will take care in this patch and in future./* 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.
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
