From: Daniel Vetter <daniel@ffwll.ch>
To: "Jindal, Sonika" <sonika.jindal@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v5] drm/i915/bxt: eDP Panel Power sequencing
Date: Thu, 18 Jun 2015 08:48:04 +0200 [thread overview]
Message-ID: <20150618064804.GH23637@phenom.ffwll.local> (raw)
In-Reply-To: <000C66961D35964B9714611E548C10AD0C223347@BGSMSX104.gar.corp.intel.com>
On Thu, Jun 18, 2015 at 06:08:46AM +0000, Jindal, Sonika wrote:
> Looks good to me.
> Reviewed-by: Sonika Jindal <sonika.jindal@intel.com>
>
> > -----Original Message-----
> > From: Kannan, Vandana
> > Sent: Thursday, June 18, 2015 11:01 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Jindal, Sonika; Kannan, Vandana
> > Subject: [PATCH v5] drm/i915/bxt: eDP Panel Power sequencing
> >
> > 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
> >
> > v4: Jani's comments
> > - modify ironlake_get_pp_control() - dont set unlock key for bxt
> >
> > v5: Sonika's comments addressed
> > - check alignment
> > - move pp_ctrl_reg write (after ironlake_get_pp_control())
> > to !IS_BROXTON case.
> > - check before subtracting 1 for t11_t12
> >
> > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> > Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
When resending patches please always include reviewers from previous
rounds in the Cc: list of the patch. Otherwise it's very likely that
they'll miss the patch in the overall mail flood on intel-gfx. Adding
Jani&Sonika.
Queued for -next (assuming Jani's ok too), thanks for the patch.
-Daniel
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 13 +++++++
> > drivers/gpu/drm/i915/intel_dp.c | 82
> > ++++++++++++++++++++++++++++++++---------
> > 2 files changed, 78 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 0b979ad..27eb49e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6391,6 +6391,8 @@ enum skl_disp_power_wells {
> > #define PCH_PP_CONTROL 0xc7204
> > #define PANEL_UNLOCK_REGS (0xabcd << 16)
> > #define PANEL_UNLOCK_MASK (0xffff << 16)
> > +#define BXT_POWER_CYCLE_DELAY_MASK (0x1f0)
> > +#define BXT_POWER_CYCLE_DELAY_SHIFT 4
> > #define EDP_FORCE_VDD (1 << 3)
> > #define EDP_BLC_ENABLE (1 << 2)
> > #define PANEL_POWER_RESET (1 << 1)
> > @@ -6419,6 +6421,17 @@ enum skl_disp_power_wells {
> > #define PANEL_POWER_CYCLE_DELAY_MASK (0x1f)
> > #define PANEL_POWER_CYCLE_DELAY_SHIFT 0
> >
> > +/* BXT PPS changes - 2nd set of PPS registers */
> > +#define _BXT_PP_STATUS2 0xc7300
> > +#define _BXT_PP_CONTROL2 0xc7304
> > +#define _BXT_PP_ON_DELAYS2 0xc7308
> > +#define _BXT_PP_OFF_DELAYS2 0xc730c
> > +
> > +#define BXT_PP_STATUS(n) ((!n) ? PCH_PP_STATUS : _BXT_PP_STATUS2)
> > +#define BXT_PP_CONTROL(n) ((!n) ? PCH_PP_CONTROL :
> > _BXT_PP_CONTROL2)
> > +#define BXT_PP_ON_DELAYS(n) ((!n) ? PCH_PP_ON_DELAYS :
> > _BXT_PP_ON_DELAYS2)
> > +#define BXT_PP_OFF_DELAYS(n) ((!n) ? PCH_PP_OFF_DELAYS :
> > _BXT_PP_OFF_DELAYS2)
> > +
> > #define PCH_DP_B 0xe4100
> > #define PCH_DPB_AUX_CH_CTL 0xe4110
> > #define PCH_DPB_AUX_CH_DATA1 0xe4114
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index f52eef1..c2145e7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -567,7 +567,9 @@ static u32 _pp_ctrl_reg(struct intel_dp *intel_dp) {
> > struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >
> > - if (HAS_PCH_SPLIT(dev))
> > + if (IS_BROXTON(dev))
> > + return BXT_PP_CONTROL(0);
> > + else if (HAS_PCH_SPLIT(dev))
> > return PCH_PP_CONTROL;
> > else
> > return
> > VLV_PIPE_PP_CONTROL(vlv_power_sequencer_pipe(intel_dp));
> > @@ -577,7 +579,9 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp) {
> > struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >
> > - if (HAS_PCH_SPLIT(dev))
> > + if (IS_BROXTON(dev))
> > + return BXT_PP_STATUS(0);
> > + else if (HAS_PCH_SPLIT(dev))
> > return PCH_PP_STATUS;
> > else
> > return
> > VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
> > @@ -1702,8 +1706,10 @@ static u32 ironlake_get_pp_control(struct
> > intel_dp *intel_dp)
> > lockdep_assert_held(&dev_priv->pps_mutex);
> >
> > control = I915_READ(_pp_ctrl_reg(intel_dp));
> > - control &= ~PANEL_UNLOCK_MASK;
> > - control |= PANEL_UNLOCK_REGS;
> > + if (!IS_BROXTON(dev)) {
> > + control &= ~PANEL_UNLOCK_MASK;
> > + control |= PANEL_UNLOCK_REGS;
> > + }
> > return control;
> > }
> >
> > @@ -5092,8 +5098,8 @@ intel_dp_init_panel_power_sequencer(struct
> > drm_device *dev,
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct edp_power_seq cur, vbt, spec,
> > *final = &intel_dp->pps_delays;
> > - u32 pp_on, pp_off, pp_div, pp;
> > - int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg;
> > + u32 pp_on, pp_off, pp_div = 0, pp_ctl = 0;
> > + int pp_ctrl_reg, pp_on_reg, pp_off_reg, pp_div_reg = 0;
> >
> > lockdep_assert_held(&dev_priv->pps_mutex);
> >
> > @@ -5101,7 +5107,16 @@ intel_dp_init_panel_power_sequencer(struct
> > drm_device *dev,
> > if (final->t11_t12 != 0)
> > return;
> >
> > - if (HAS_PCH_SPLIT(dev)) {
> > + if (IS_BROXTON(dev)) {
> > + /*
> > + * TODO: BXT has 2 sets of PPS registers.
> > + * Correct Register for Broxton need to be identified
> > + * using VBT. hardcoding for now
> > + */
> > + pp_ctrl_reg = BXT_PP_CONTROL(0);
> > + pp_on_reg = BXT_PP_ON_DELAYS(0);
> > + pp_off_reg = BXT_PP_OFF_DELAYS(0);
> > + } else if (HAS_PCH_SPLIT(dev)) {
> > pp_ctrl_reg = PCH_PP_CONTROL;
> > pp_on_reg = PCH_PP_ON_DELAYS;
> > pp_off_reg = PCH_PP_OFF_DELAYS;
> > @@ -5117,12 +5132,14 @@ intel_dp_init_panel_power_sequencer(struct
> > drm_device *dev,
> >
> > /* 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);
> > + pp_ctl = ironlake_get_pp_control(intel_dp);
> >
> > 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)) {
> > + I915_WRITE(pp_ctrl_reg, pp_ctl);
> > + pp_div = I915_READ(pp_div_reg);
> > + }
> >
> > /* Pull timing values out of registers */
> > cur.t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >> @@ -
> > 5137,8 +5154,17 @@ 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)) {
> > + u16 tmp = (pp_ctl & BXT_POWER_CYCLE_DELAY_MASK) >>
> > + BXT_POWER_CYCLE_DELAY_SHIFT;
> > + if (tmp > 0)
> > + cur.t11_t12 = (tmp - 1) * 1000;
> > + else
> > + cur.t11_t12 = 0;
> > + } 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",
> > cur.t1_t3, cur.t8, cur.t9, cur.t10, cur.t11_t12); @@ -5195,13
> > +5221,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);
> >
> > - if (HAS_PCH_SPLIT(dev)) {
> > + if (IS_BROXTON(dev)) {
> > + /*
> > + * TODO: BXT has 2 sets of PPS registers.
> > + * Correct Register for Broxton need to be identified
> > + * using VBT. hardcoding for now
> > + */
> > + pp_ctrl_reg = BXT_PP_CONTROL(0);
> > + pp_on_reg = BXT_PP_ON_DELAYS(0);
> > + pp_off_reg = BXT_PP_OFF_DELAYS(0);
> > +
> > + } else if (HAS_PCH_SPLIT(dev)) {
> > pp_on_reg = PCH_PP_ON_DELAYS;
> > pp_off_reg = PCH_PP_OFF_DELAYS;
> > pp_div_reg = PCH_PP_DIVISOR;
> > @@ -5227,9 +5263,16 @@
> > intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> > (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
> > /* Compute the divisor for the pp clock, simply match the Bspec
> > * formula. */
> > - pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
> > - pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> > - << PANEL_POWER_CYCLE_DELAY_SHIFT);
> > + if (IS_BROXTON(dev)) {
> > + pp_div = I915_READ(pp_ctrl_reg);
> > + pp_div &= ~BXT_POWER_CYCLE_DELAY_MASK;
> > + pp_div |= (DIV_ROUND_UP((seq->t11_t12 + 1), 1000)
> > + << BXT_POWER_CYCLE_DELAY_SHIFT);
> > + } else {
> > + pp_div = ((100 * div)/2 - 1) <<
> > PP_REFERENCE_DIVIDER_SHIFT;
> > + pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> > + << PANEL_POWER_CYCLE_DELAY_SHIFT);
> > + }
> >
> > /* Haswell doesn't have any port selection bits for the panel
> > * power sequencer any more. */
> > @@ -5246,11 +5289,16 @@
> > intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
> >
> > I915_WRITE(pp_on_reg, pp_on);
> > I915_WRITE(pp_off_reg, pp_off);
> > - I915_WRITE(pp_div_reg, pp_div);
> > + if (IS_BROXTON(dev))
> > + I915_WRITE(pp_ctrl_reg, pp_div);
> > + else
> > + I915_WRITE(pp_div_reg, pp_div);
> >
> > DRM_DEBUG_KMS("panel power sequencer register settings:
> > PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n",
> > I915_READ(pp_on_reg),
> > I915_READ(pp_off_reg),
> > + IS_BROXTON(dev) ?
> > + (I915_READ(pp_ctrl_reg) &
> > BXT_POWER_CYCLE_DELAY_MASK) :
> > I915_READ(pp_div_reg));
> > }
> >
> > --
> > 2.0.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
prev parent reply other threads:[~2015-06-18 6:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-12 10:27 [PATCH v4] drm/i915/bxt: eDP Panel Power sequencing Vandana Kannan
2015-06-17 9:15 ` Jindal, Sonika
2015-06-17 9:22 ` Kannan, Vandana
2015-06-18 5:30 ` [PATCH v5] " Vandana Kannan
2015-06-18 6:08 ` Jindal, Sonika
2015-06-18 6:48 ` Daniel Vetter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150618064804.GH23637@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=sonika.jindal@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.