From: Daniel Vetter <daniel@ffwll.ch>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Intel Graphics <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v2 8/9] drm/i915: panel power sequencing for VLV eDP
Date: Thu, 27 Sep 2012 18:59:01 +0200 [thread overview]
Message-ID: <20120927165901.GH2098@bremse> (raw)
In-Reply-To: <20120927082615.18021ac4@jbarnes-desktop>
On Thu, Sep 27, 2012 at 08:26:15AM -0700, Jesse Barnes wrote:
> On Thu, 27 Sep 2012 19:13:08 +0530
> Vijay Purushothaman <vijay.a.purushothaman@intel.com> wrote:
>
> > PPS register offsets have changed in Valleyview.
> >
> > Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
> > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 9 +++
> > drivers/gpu/drm/i915/intel_dp.c | 122 +++++++++++++++++++++++++++------------
> > 2 files changed, 93 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 0fe4aad..0e6258a 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3977,6 +3977,15 @@
> > #define PIPEB_PP_ON_DELAYS 0x61308
> > #define PIPEB_PP_OFF_DELAYS 0x6130c
> > #define PIPEB_PP_DIVISOR 0x61310
> > +#define VLV_PIPE_PP_STATUS(pipe) _PIPE(pipe, PIPEA_PP_STATUS, PIPEB_PP_STATUS)
> > +#define VLV_PIPE_PP_CONTROL(pipe) _PIPE(pipe, PIPEA_PP_CONTROL, PIPEB_PP_CONTROL)
> > +#define VLV_PIPE_PP_ON_DELAYS(pipe) \
> > + _PIPE(pipe, PIPEA_PP_ON_DELAYS, PIPEB_PP_ON_DELAYS)
> > +#define VLV_PIPE_PP_OFF_DELAYS(pipe) \
> > + _PIPE(pipe, PIPEA_PP_OFF_DELAYS, PIPEB_PP_OFF_DELAYS)
> > +#define VLV_PIPE_PP_DIVISOR(pipe) \
> > + _PIPE(pipe, PIPEA_PP_DIVISOR, PIPEB_PP_DIVISOR)
> > +
> >
> > #define PCH_PP_STATUS 0xc7200
> > #define PCH_PP_CONTROL 0xc7204
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 867c568..c58535b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -316,16 +316,20 @@ static bool ironlake_edp_have_panel_power(struct intel_dp *intel_dp)
> > {
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + u32 pp_stat_reg;
> >
> > - return (I915_READ(PCH_PP_STATUS) & PP_ON) != 0;
> > + pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> > + return (I915_READ(pp_stat_reg) & PP_ON) != 0;
> > }
> >
> > static bool ironlake_edp_have_panel_vdd(struct intel_dp *intel_dp)
> > {
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + u32 pp_ctrl_reg;
> >
> > - return (I915_READ(PCH_PP_CONTROL) & EDP_FORCE_VDD) != 0;
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> > + return (I915_READ(pp_ctrl_reg) & EDP_FORCE_VDD) != 0;
> > }
> >
> > static void
> > @@ -333,14 +337,19 @@ intel_dp_check_edp(struct intel_dp *intel_dp)
> > {
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + u32 pp_stat_reg, pp_ctrl_reg;
> >
> > if (!is_edp(intel_dp))
> > return;
> > +
> > + pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> > +
> > if (!ironlake_edp_have_panel_power(intel_dp) && !ironlake_edp_have_panel_vdd(intel_dp)) {
> > WARN(1, "eDP powered off while attempting aux channel communication.\n");
> > DRM_DEBUG_KMS("Status 0x%08x Control 0x%08x\n",
> > - I915_READ(PCH_PP_STATUS),
> > - I915_READ(PCH_PP_CONTROL));
> > + I915_READ(pp_stat_reg),
> > + I915_READ(pp_ctrl_reg));
> > }
> > }
> >
> > @@ -944,16 +953,20 @@ static void ironlake_wait_panel_status(struct intel_dp *intel_dp,
> > {
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + u32 pp_stat_reg, pp_ctrl_reg;
> > +
> > + pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> >
> > DRM_DEBUG_KMS("mask %08x value %08x status %08x control %08x\n",
> > - mask, value,
> > - I915_READ(PCH_PP_STATUS),
> > - I915_READ(PCH_PP_CONTROL));
> > + mask, value,
> > + I915_READ(pp_stat_reg),
> > + I915_READ(pp_ctrl_reg));
> >
> > - if (_wait_for((I915_READ(PCH_PP_STATUS) & mask) == value, 5000, 10)) {
> > + if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000, 10)) {
> > DRM_ERROR("Panel status timeout: status %08x control %08x\n",
> > - I915_READ(PCH_PP_STATUS),
> > - I915_READ(PCH_PP_CONTROL));
> > + I915_READ(pp_stat_reg),
> > + I915_READ(pp_ctrl_reg));
> > }
> > }
> >
> > @@ -980,9 +993,15 @@ static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp)
> > * is locked
> > */
> >
> > -static u32 ironlake_get_pp_control(struct drm_i915_private *dev_priv)
> > +static u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
> > {
> > - u32 control = I915_READ(PCH_PP_CONTROL);
> > + struct drm_device *dev = intel_dp->base.base.dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + u32 control;
> > + u32 pp_ctrl_reg;
> > +
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> > + control = I915_READ(pp_ctrl_reg);
> >
> > control &= ~PANEL_UNLOCK_MASK;
> > control |= PANEL_UNLOCK_REGS;
> > @@ -994,6 +1013,7 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 pp;
> > + u32 pp_stat_reg, pp_ctrl_reg;
> >
> > if (!is_edp(intel_dp))
> > return;
> > @@ -1012,13 +1032,16 @@ static void ironlake_edp_panel_vdd_on(struct intel_dp *intel_dp)
> > if (!ironlake_edp_have_panel_power(intel_dp))
> > ironlake_wait_panel_power_cycle(intel_dp);
> >
> > - pp = ironlake_get_pp_control(dev_priv);
> > + pp = ironlake_get_pp_control(intel_dp);
> > pp |= EDP_FORCE_VDD;
> > - I915_WRITE(PCH_PP_CONTROL, pp);
> > - POSTING_READ(PCH_PP_CONTROL);
> > - DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
> > - I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
> >
> > + pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> > +
> > + I915_WRITE(pp_ctrl_reg, pp);
> > + POSTING_READ(pp_ctrl_reg);
> > + DRM_DEBUG_KMS("PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
> > + I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
> > /*
> > * If the panel wasn't on, delay before accessing aux channel
> > */
> > @@ -1033,17 +1056,21 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 pp;
> > + u32 pp_stat_reg, pp_ctrl_reg;
> >
> > if (!intel_dp->want_panel_vdd && ironlake_edp_have_panel_vdd(intel_dp)) {
> > - pp = ironlake_get_pp_control(dev_priv);
> > + pp = ironlake_get_pp_control(intel_dp);
> > pp &= ~EDP_FORCE_VDD;
> > - I915_WRITE(PCH_PP_CONTROL, pp);
> > - POSTING_READ(PCH_PP_CONTROL);
> >
> > - /* Make sure sequencer is idle before allowing subsequent activity */
> > - DRM_DEBUG_KMS("PCH_PP_STATUS: 0x%08x PCH_PP_CONTROL: 0x%08x\n",
> > - I915_READ(PCH_PP_STATUS), I915_READ(PCH_PP_CONTROL));
> > + pp_stat_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_STATUS : PCH_PP_STATUS;
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> >
> > + I915_WRITE(pp_ctrl_reg, pp);
> > + POSTING_READ(pp_ctrl_reg);
> > +
> > + /* Make sure sequencer is idle before allowing subsequent activity */
> > + DRM_DEBUG_KMS("PP_STATUS: 0x%08x PP_CONTROL: 0x%08x\n",
> > + I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
> > msleep(intel_dp->panel_power_down_delay);
> > }
> > }
> > @@ -1087,6 +1114,7 @@ static void ironlake_edp_panel_on(struct intel_dp *intel_dp)
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 pp;
> > + u32 pp_ctrl_reg;
> >
> > if (!is_edp(intel_dp))
> > return;
> > @@ -1100,7 +1128,7 @@ static void ironlake_edp_panel_on(struct intel_dp *intel_dp)
> >
> > ironlake_wait_panel_power_cycle(intel_dp);
> >
> > - pp = ironlake_get_pp_control(dev_priv);
> > + pp = ironlake_get_pp_control(intel_dp);
> > if (IS_GEN5(dev)) {
> > /* ILK workaround: disable reset around power sequence */
> > pp &= ~PANEL_POWER_RESET;
> > @@ -1112,8 +1140,10 @@ static void ironlake_edp_panel_on(struct intel_dp *intel_dp)
> > if (!IS_GEN5(dev))
> > pp |= PANEL_POWER_RESET;
> >
> > - I915_WRITE(PCH_PP_CONTROL, pp);
> > - POSTING_READ(PCH_PP_CONTROL);
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> > +
> > + I915_WRITE(pp_ctrl_reg, pp);
> > + POSTING_READ(pp_ctrl_reg);
> >
> > ironlake_wait_panel_on(intel_dp);
> >
> > @@ -1129,6 +1159,7 @@ static void ironlake_edp_panel_off(struct intel_dp *intel_dp)
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 pp;
> > + u32 pp_ctrl_reg;
> >
> > if (!is_edp(intel_dp))
> > return;
> > @@ -1137,12 +1168,15 @@ static void ironlake_edp_panel_off(struct intel_dp *intel_dp)
> >
> > WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n");
> >
> > - pp = ironlake_get_pp_control(dev_priv);
> > + pp = ironlake_get_pp_control(intel_dp);
> > /* We need to switch off panel power _and_ force vdd, for otherwise some
> > * panels get very unhappy and cease to work. */
> > pp &= ~(POWER_TARGET_ON | EDP_FORCE_VDD | PANEL_POWER_RESET | EDP_BLC_ENABLE);
> > - I915_WRITE(PCH_PP_CONTROL, pp);
> > - POSTING_READ(PCH_PP_CONTROL);
> > +
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> > +
> > + I915_WRITE(pp_ctrl_reg, pp);
> > + POSTING_READ(pp_ctrl_reg);
> >
> > intel_dp->want_panel_vdd = false;
> >
> > @@ -1154,6 +1188,7 @@ static void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 pp;
> > + u32 pp_ctrl_reg;
> >
> > if (!is_edp(intel_dp))
> > return;
> > @@ -1166,10 +1201,13 @@ static void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
> > * allowing it to appear.
> > */
> > msleep(intel_dp->backlight_on_delay);
> > - pp = ironlake_get_pp_control(dev_priv);
> > + pp = ironlake_get_pp_control(intel_dp);
> > pp |= EDP_BLC_ENABLE;
> > - I915_WRITE(PCH_PP_CONTROL, pp);
> > - POSTING_READ(PCH_PP_CONTROL);
> > +
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> > +
> > + I915_WRITE(pp_ctrl_reg, pp);
> > + POSTING_READ(pp_ctrl_reg);
> > }
> >
> > static void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
> > @@ -1177,15 +1215,17 @@ static void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
> > struct drm_device *dev = intel_dp->base.base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 pp;
> > + u32 pp_ctrl_reg;
> >
> > if (!is_edp(intel_dp))
> > return;
> >
> > DRM_DEBUG_KMS("\n");
> > - pp = ironlake_get_pp_control(dev_priv);
> > + pp = ironlake_get_pp_control(intel_dp);
> > pp &= ~EDP_BLC_ENABLE;
> > - I915_WRITE(PCH_PP_CONTROL, pp);
> > - POSTING_READ(PCH_PP_CONTROL);
> > + pp_ctrl_reg = IS_VALLEYVIEW(dev) ? PIPEA_PP_CONTROL : PCH_PP_CONTROL;
> > + I915_WRITE(pp_ctrl_reg, pp);
> > + POSTING_READ(pp_ctrl_reg);
> > msleep(intel_dp->backlight_off_delay);
> > }
> >
> > @@ -2547,9 +2587,15 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> > u32 pp_on, pp_off, pp_div;
> > struct edid *edid;
> >
> > - pp_on = I915_READ(PCH_PP_ON_DELAYS);
> > - pp_off = I915_READ(PCH_PP_OFF_DELAYS);
> > - pp_div = I915_READ(PCH_PP_DIVISOR);
> > + if (IS_VALLEYVIEW(dev)) {
> > + pp_on = I915_READ(PIPEA_PP_ON_DELAYS);
> > + pp_off = I915_READ(PIPEA_PP_OFF_DELAYS);
> > + pp_div = I915_READ(PIPEA_PP_DIVISOR);
> > + } else {
> > + pp_on = I915_READ(PCH_PP_ON_DELAYS);
> > + pp_off = I915_READ(PCH_PP_OFF_DELAYS);
> > + pp_div = I915_READ(PCH_PP_DIVISOR);
> > + }
> >
> > if (!pp_on || !pp_off || !pp_div) {
> > DRM_INFO("bad panel power sequencing delays, disabling panel\n");
>
> I think Daniel's right that it would be good to abstract this a bit.
> Pretty sure people are building machines with multiple embedded panels
> and using some i2c interface for doing the secondary sequence. A good
> abstraction would help with that too.
I'll happily reiterate that I prefer abstraction once we actually have
more than one user, so I don't mind merging this here ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
next prev parent reply other threads:[~2012-09-27 16:59 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-27 13:43 [PATCH v2 0/9] Enable all display interfaces in Valleyview Vijay Purushothaman
2012-09-27 13:43 ` [PATCH v2 1/9] drm/i915: Set aux clk to 100MHz for Valleyview Vijay Purushothaman
2012-09-27 15:11 ` Jesse Barnes
2012-09-27 13:43 ` [PATCH v2 2/9] drm/i915: Fix SDVO IER and status bits " Vijay Purushothaman
2012-09-27 15:13 ` Jesse Barnes
2012-09-27 13:43 ` [PATCH v2 3/9] drm/i915: Add Valleyview lane control definitions Vijay Purushothaman
2012-09-27 15:17 ` Jesse Barnes
2012-09-27 13:43 ` [PATCH v2 4/9] drm/i915: Program correct m n tu register for Valleyview Vijay Purushothaman
2012-09-27 15:18 ` Jesse Barnes
2012-09-27 13:43 ` [PATCH v2 5/9] drm/i915: Disable CRT hotplug detection for valleyview Vijay Purushothaman
2012-09-27 15:20 ` Jesse Barnes
2012-09-28 14:51 ` Daniel Vetter
2012-09-27 15:34 ` Jesse Barnes
2012-09-27 13:43 ` [PATCH v2 6/9] drm/i915: Enable DisplayPort in Valleyview Vijay Purushothaman
2012-09-27 15:23 ` Jesse Barnes
2012-09-28 15:03 ` Daniel Vetter
2012-09-27 13:43 ` [PATCH v2 7/9] drm/i915: Add eDP support for Valleyview Vijay Purushothaman
2012-09-27 15:24 ` Jesse Barnes
2012-09-28 15:08 ` Daniel Vetter
2012-09-27 13:43 ` [PATCH v2 8/9] drm/i915: panel power sequencing for VLV eDP Vijay Purushothaman
2012-09-27 15:26 ` Jesse Barnes
2012-09-27 16:59 ` Daniel Vetter [this message]
2012-09-27 13:43 ` [PATCH v2 9/9] drm/i915: Fixup HDMI output on Valleyview Vijay Purushothaman
2012-09-27 15:26 ` Jesse Barnes
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=20120927165901.GH2098@bremse \
--to=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jbarnes@virtuousgeek.org \
/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.