From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 71/89] drm/i915/skl: Implementation of SKL display power well support Date: Tue, 16 Sep 2014 17:19:34 +0300 Message-ID: <1410877174.19704.55.camel@intelbox> References: <1409830075-11139-1-git-send-email-damien.lespiau@intel.com> <1409830075-11139-72-git-send-email-damien.lespiau@intel.com> <1410875789.19704.41.camel@intelbox> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0020257770==" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id EC1C889AAE for ; Tue, 16 Sep 2014 07:20:14 -0700 (PDT) In-Reply-To: <1410875789.19704.41.camel@intelbox> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Damien Lespiau Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0020257770== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-4BVFe0L2aWn4c2u5spWa" --=-4BVFe0L2aWn4c2u5spWa Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2014-09-16 at 16:56 +0300, Imre Deak wrote: > On Thu, 2014-09-04 at 12:27 +0100, Damien Lespiau wrote: > > From: Satheeshakrishna M > >=20 > > This patch implements core logic of SKL display power well. > >=20 > > FIXME: hsw_pwr needs to go. The audio guys promised us that they'll do = a proper > > implementation for skl+. > >=20 > > v2: Addressed Imre's comments > > - Added respective DDIs under power well #1 and #2 > > - Simplified repetitive code in power well programming > >=20 > > v3: Implemented Imre's comments > > - Further simplified power well programming > > - Made sure that PW 1 is enabled prior to PW 2 > >=20 > > v4: Fix minor conflict with the the cherryview support (Damien) > >=20 > > v5: Add the PLL power domain to the always on power well (Damien) > >=20 > > v6: Disable BIOS power well (Imre) > > Use power well data for comparison (Imre) > > Put the PLL power domain into PW1 as its needed for CDCLK (Satheesh= , > > Damien) > >=20 > > Signed-off-by: Satheeshakrishna M (v3,v6= ) > > Signed-off-by: Damien Lespiau > > --- > > drivers/gpu/drm/i915/i915_reg.h | 13 +++ > > drivers/gpu/drm/i915/intel_pm.c | 207 ++++++++++++++++++++++++++++++++= ++++++++ > > 2 files changed, 220 insertions(+) > >=20 > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i91= 5_reg.h > > index 4d072a8..84a0de6 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -541,6 +541,19 @@ enum punit_power_well { > > PUNIT_POWER_WELL_NUM, > > }; > > =20 > > +enum skl_disp_power_wells { > > + SKL_DISP_PW_MISC_IO, > > + SKL_DISP_PW_DDI_A_E, > > + SKL_DISP_PW_DDI_B, > > + SKL_DISP_PW_DDI_C, > > + SKL_DISP_PW_DDI_D, > > + SKL_DISP_PW_1 =3D 14, > > + SKL_DISP_PW_2, > > +}; > > + > > +#define SKL_POWER_WELL_STATE(pw) (1 << (pw * 2)) > > +#define SKL_POWER_WELL_REQ(pw) (1 << ((pw * 2) + 1)) Missed this one: missing parentheses for pw. > > + > > #define PUNIT_REG_PWRGT_CTRL 0x60 > > #define PUNIT_REG_PWRGT_STATUS 0x61 > > #define PUNIT_PWRGT_MASK(power_well) (3 << ((power_well) * 2)) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/int= el_pm.c > > index ec849db..853b596 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -7160,6 +7160,128 @@ static void hsw_set_power_well(struct drm_i915_= private *dev_priv, > > } > > } > > =20 > > +#define SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS ( \ > > + BIT(POWER_DOMAIN_PIPE_B) | \ > > + BIT(POWER_DOMAIN_TRANSCODER_B) | \ > > + BIT(POWER_DOMAIN_PIPE_C) | \ > > + BIT(POWER_DOMAIN_TRANSCODER_C) | \ > > + BIT(POWER_DOMAIN_PIPE_B_PANEL_FITTER) | \ > > + BIT(POWER_DOMAIN_PIPE_C_PANEL_FITTER) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ > > + BIT(POWER_DOMAIN_AUX_B) | \ > > + BIT(POWER_DOMAIN_AUX_C) | \ > > + BIT(POWER_DOMAIN_AUX_D) | \ > > + BIT(POWER_DOMAIN_AUDIO) | \ > > + BIT(POWER_DOMAIN_INIT)) > > +#define SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS ( \ > > + SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \ > > + BIT(POWER_DOMAIN_PLLS) | \ > > + BIT(POWER_DOMAIN_PIPE_A) | \ > > + BIT(POWER_DOMAIN_TRANSCODER_EDP) | \ > > + BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_A_2_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_A_4_LANES) | \ > > + BIT(POWER_DOMAIN_AUX_A) | \ > > + BIT(POWER_DOMAIN_INIT)) > > +#define SKL_DISPLAY_DDI_A_E_POWER_DOMAINS ( \ > > + BIT(POWER_DOMAIN_PORT_DDI_A_2_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_A_4_LANES) | \ > > + BIT(POWER_DOMAIN_INIT)) > > +#define SKL_DISPLAY_DDI_B_POWER_DOMAINS ( \ > > + BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_B_4_LANES) | \ > > + BIT(POWER_DOMAIN_INIT)) > > +#define SKL_DISPLAY_DDI_C_POWER_DOMAINS ( \ > > + BIT(POWER_DOMAIN_PORT_DDI_C_2_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_C_4_LANES) | \ > > + BIT(POWER_DOMAIN_INIT)) > > +#define SKL_DISPLAY_DDI_D_POWER_DOMAINS ( \ > > + BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) | \ > > + BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) | \ > > + BIT(POWER_DOMAIN_INIT)) > > +#define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS ( \ > > + (POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS | \ > > + SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \ > > + SKL_DISPLAY_DDI_A_E_POWER_DOMAINS | \ > > + SKL_DISPLAY_DDI_B_POWER_DOMAINS | \ > > + SKL_DISPLAY_DDI_C_POWER_DOMAINS | \ > > + SKL_DISPLAY_DDI_D_POWER_DOMAINS)) | \ > > + BIT(POWER_DOMAIN_INIT)) > > + > > +static void skl_set_power_well(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well, bool enable) > > +{ > > + uint32_t tmp, fuse_status; > > + uint32_t req_mask, state_mask; > > + bool check_fuse_status =3D false; > > + > > + tmp =3D I915_READ(HSW_PWR_WELL_DRIVER); > > + fuse_status =3D I915_READ(SKL_FUSE_STATUS); > > + > > + switch (power_well->data) { > > + case SKL_DISP_PW_1: > > + if (wait_for((I915_READ(SKL_FUSE_STATUS) & > > + SKL_FUSE_PG0_DIST_STATUS), 5)) { >=20 > The spec says 5 us not 5 ms, so we could just wait for 1 ms. The same > applies to similar places below. >=20 > > + DRM_ERROR("PG0 not enabled\n"); > > + return; > > + } > > + break; > > + case SKL_DISP_PW_2: > > + if (!(fuse_status & SKL_FUSE_PG1_DIST_STATUS)) { > > + DRM_ERROR("PG1 in disabled state\n"); > > + return; > > + } > > + break; > > + case SKL_DISP_PW_DDI_A_E: > > + case SKL_DISP_PW_DDI_B: > > + case SKL_DISP_PW_DDI_C: > > + case SKL_DISP_PW_DDI_D: > > + break; > > + default: >=20 > This would be a driver bug, so it needs a WARN(). >=20 > > + return; > > + } > > + > > + req_mask =3D SKL_POWER_WELL_REQ(power_well->data); > > + state_mask =3D SKL_POWER_WELL_STATE(power_well->data); > > + > > + if (enable) { > > + if (!(tmp & req_mask)) > > + I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask); > > + > > + if (!(tmp & state_mask)) { > > + DRM_DEBUG_KMS("Enabling DDI power well\n"); >=20 > Could be just "Enabling %s", power_well->name. Also we could miss the > message depending on timing, so it needs to go above where the request > bit is set. >=20 > With the above changes, this looks ok: > Reviewed-by: Imre Deak >=20 > > + if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) & > > + state_mask), 20)) > > + DRM_ERROR("%s enable timeout\n", > > + power_well->name); > > + check_fuse_status =3D true; > > + } > > + } else { > > + if (tmp & req_mask) { > > + I915_WRITE(HSW_PWR_WELL_DRIVER, tmp & ~req_mask); > > + POSTING_READ(HSW_PWR_WELL_DRIVER); > > + DRM_DEBUG_KMS("Disabling %s\n", power_well->name); > > + } > > + } > > + > > + if (check_fuse_status) { > > + if (power_well->data =3D=3D SKL_DISP_PW_1) { > > + if (wait_for((I915_READ(SKL_FUSE_STATUS) & > > + SKL_FUSE_PG1_DIST_STATUS), 5)) > > + DRM_ERROR("PG1 distributing status timeout\n"); > > + } else if (power_well->data =3D=3D SKL_DISP_PW_2) { > > + if (wait_for((I915_READ(SKL_FUSE_STATUS) & > > + SKL_FUSE_PG2_DIST_STATUS), 1)) > > + DRM_ERROR("PG2 distributing status timeout\n"); > > + } > > + } > > +} > > + > > static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv, > > struct i915_power_well *power_well) > > { > > @@ -7185,6 +7307,36 @@ static void hsw_power_well_disable(struct drm_i9= 15_private *dev_priv, > > hsw_set_power_well(dev_priv, power_well, false); > > } > > =20 > > +static bool skl_power_well_enabled(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + uint32_t mask =3D SKL_POWER_WELL_REQ(power_well->data) | > > + SKL_POWER_WELL_STATE(power_well->data); > > + > > + return (I915_READ(HSW_PWR_WELL_DRIVER) & mask) =3D=3D mask; > > +} > > + > > +static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + skl_set_power_well(dev_priv, power_well, power_well->count > 0); > > + > > + /* Clear any request made by BIOS as driver is taking over */ > > + I915_WRITE(HSW_PWR_WELL_BIOS, 0); > > +} > > + > > +static void skl_power_well_enable(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + skl_set_power_well(dev_priv, power_well, true); > > +} > > + > > +static void skl_power_well_disable(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + skl_set_power_well(dev_priv, power_well, false); > > +} > > + > > static void i9xx_always_on_power_well_noop(struct drm_i915_private *de= v_priv, > > struct i915_power_well *power_well) > > { > > @@ -7787,6 +7939,13 @@ static const struct i915_power_well_ops hsw_powe= r_well_ops =3D { > > .is_enabled =3D hsw_power_well_enabled, > > }; > > =20 > > +static const struct i915_power_well_ops skl_power_well_ops =3D { > > + .sync_hw =3D skl_power_well_sync_hw, > > + .enable =3D skl_power_well_enable, > > + .disable =3D skl_power_well_disable, > > + .is_enabled =3D skl_power_well_enabled, > > +}; > > + > > static struct i915_power_well hsw_power_wells[] =3D { > > { > > .name =3D "always-on", > > @@ -8009,6 +8168,51 @@ static struct i915_power_well *lookup_power_well= (struct drm_i915_private *dev_pr > > return NULL; > > } > > =20 > > +static struct i915_power_well skl_power_wells[] =3D { > > + { > > + .name =3D "always-on", > > + .always_on =3D 1, > > + .domains =3D SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS, > > + .ops =3D &i9xx_always_on_power_well_ops, > > + }, > > + { > > + .name =3D "power well 1", > > + .domains =3D SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS, > > + .ops =3D &skl_power_well_ops, > > + .data =3D SKL_DISP_PW_1, > > + }, > > + { > > + .name =3D "power well 2", > > + .domains =3D SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS, > > + .ops =3D &skl_power_well_ops, > > + .data =3D SKL_DISP_PW_2, > > + }, > > + { > > + .name =3D "DDI A/E power well", > > + .domains =3D SKL_DISPLAY_DDI_A_E_POWER_DOMAINS, > > + .ops =3D &skl_power_well_ops, > > + .data =3D SKL_DISP_PW_DDI_A_E, > > + }, > > + { > > + .name =3D "DDI B power well", > > + .domains =3D SKL_DISPLAY_DDI_B_POWER_DOMAINS, > > + .ops =3D &skl_power_well_ops, > > + .data =3D SKL_DISP_PW_DDI_B, > > + }, > > + { > > + .name =3D "DDI C power well", > > + .domains =3D SKL_DISPLAY_DDI_C_POWER_DOMAINS, > > + .ops =3D &skl_power_well_ops, > > + .data =3D SKL_DISP_PW_DDI_C, > > + }, > > + { > > + .name =3D "DDI D power well", > > + .domains =3D SKL_DISPLAY_DDI_D_POWER_DOMAINS, > > + .ops =3D &skl_power_well_ops, > > + .data =3D SKL_DISP_PW_DDI_D, > > + }, > > +}; > > + > > #define set_power_wells(power_domains, __power_wells) ({ \ > > (power_domains)->power_wells =3D (__power_wells); \ > > (power_domains)->power_well_count =3D ARRAY_SIZE(__power_wells); \ > > @@ -8030,6 +8234,9 @@ int intel_power_domains_init(struct drm_i915_priv= ate *dev_priv) > > } else if (IS_BROADWELL(dev_priv->dev)) { > > set_power_wells(power_domains, bdw_power_wells); > > hsw_pwr =3D power_domains; > > + } else if (IS_SKYLAKE(dev_priv->dev)) { > > + set_power_wells(power_domains, skl_power_wells); > > + hsw_pwr =3D power_domains; > > } else if (IS_CHERRYVIEW(dev_priv->dev)) { > > set_power_wells(power_domains, chv_power_wells); > > } else if (IS_VALLEYVIEW(dev_priv->dev)) { >=20 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx --=-4BVFe0L2aWn4c2u5spWa Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJUGEb2AAoJEORIIAnNuWDFvNsIAKE/CtHQ9ogGfheI+BG1RvKq 2pViX9yX9f1IzYQihtHSWG2MA3zCBdk5JKaRKq1S4FLFf30FwBzSLy+wgMbsjwfA qe/IFI6kWn1UShVik3HyNQjLCSBX8VaKHuhPN/7I+gS1ODWxX8oukrA3uFJpn9A6 3EhxdiIc4jI0jqX/7lXVCSYMK7odPTHzgAYivbFa+u//ebeR8GN72dsiX9sIlxHg mU4x5pVwUuFZGjxEE7EFD8WXzJlu9+HZkp6lprGfNT/LggSxf0pB2h+X64smGqVn NTDs0ramzx8oPgk7c6WzheFhumEWwLEbtSZX/tgdLqldwH86AYIL/UQe35ULA64= =ovo1 -----END PGP SIGNATURE----- --=-4BVFe0L2aWn4c2u5spWa-- --===============0020257770== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0020257770==--