Re: [PATCH 71/89] drm/i915/skl: Implementation of SKL display power well support

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 12405 bytes --]

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 <satheeshakrishna.m@intel.com>
> > 
> > This patch implements core logic of SKL display power well.
> > 
> > FIXME: hsw_pwr needs to go. The audio guys promised us that they'll do a proper
> > implementation for skl+.
> > 
> > v2: Addressed Imre's comments
> > 	- Added respective DDIs under power well #1 and #2
> > 	- Simplified repetitive code in power well programming
> > 
> > v3: Implemented Imre's comments
> > 	- Further simplified power well programming
> > 	- Made sure that PW 1 is enabled prior to PW 2
> > 
> > v4: Fix minor conflict with the the cherryview support (Damien)
> > 
> > v5: Add the PLL power domain to the always on power well (Damien)
> > 
> > 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)
> > 
> > Signed-off-by: Satheeshakrishna M <satheeshakrishna.m@intel.com> (v3,v6)
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |  13 +++
> >  drivers/gpu/drm/i915/intel_pm.c | 207 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 220 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_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,
> >  };
> >  
> > +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 = 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/intel_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,
> >  	}
> >  }
> >  
> > +#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 = false;
> > +
> > +	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> > +	fuse_status = 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)) {
> 
> The spec says 5 us not 5 ms, so we could just wait for 1 ms. The same
> applies to similar places below.
> 
> > +			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:
> 
> This would be a driver bug, so it needs a WARN().
> 
> > +		return;
> > +	}
> > +
> > +	req_mask = SKL_POWER_WELL_REQ(power_well->data);
> > +	state_mask = 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");
> 
> 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.
> 
> With the above changes, this looks ok:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> > +			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> > +				state_mask), 20))
> > +				DRM_ERROR("%s enable timeout\n",
> > +					power_well->name);
> > +			check_fuse_status = 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 == 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 == 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_i915_private *dev_priv,
> >  	hsw_set_power_well(dev_priv, power_well, false);
> >  }
> >  
> > +static bool skl_power_well_enabled(struct drm_i915_private *dev_priv,
> > +					struct i915_power_well *power_well)
> > +{
> > +	uint32_t mask = SKL_POWER_WELL_REQ(power_well->data) |
> > +		SKL_POWER_WELL_STATE(power_well->data);
> > +
> > +	return (I915_READ(HSW_PWR_WELL_DRIVER) & mask) == 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 *dev_priv,
> >  					   struct i915_power_well *power_well)
> >  {
> > @@ -7787,6 +7939,13 @@ static const struct i915_power_well_ops hsw_power_well_ops = {
> >  	.is_enabled = hsw_power_well_enabled,
> >  };
> >  
> > +static const struct i915_power_well_ops skl_power_well_ops = {
> > +	.sync_hw = skl_power_well_sync_hw,
> > +	.enable = skl_power_well_enable,
> > +	.disable = skl_power_well_disable,
> > +	.is_enabled = skl_power_well_enabled,
> > +};
> > +
> >  static struct i915_power_well hsw_power_wells[] = {
> >  	{
> >  		.name = "always-on",
> > @@ -8009,6 +8168,51 @@ static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_pr
> >  	return NULL;
> >  }
> >  
> > +static struct i915_power_well skl_power_wells[] = {
> > +	{
> > +		.name = "always-on",
> > +		.always_on = 1,
> > +		.domains = SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS,
> > +		.ops = &i9xx_always_on_power_well_ops,
> > +	},
> > +	{
> > +		.name = "power well 1",
> > +		.domains = SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS,
> > +		.ops = &skl_power_well_ops,
> > +		.data = SKL_DISP_PW_1,
> > +	},
> > +	{
> > +		.name = "power well 2",
> > +		.domains = SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS,
> > +		.ops = &skl_power_well_ops,
> > +		.data = SKL_DISP_PW_2,
> > +	},
> > +	{
> > +		.name = "DDI A/E power well",
> > +		.domains = SKL_DISPLAY_DDI_A_E_POWER_DOMAINS,
> > +		.ops = &skl_power_well_ops,
> > +		.data = SKL_DISP_PW_DDI_A_E,
> > +	},
> > +	{
> > +		.name = "DDI B power well",
> > +		.domains = SKL_DISPLAY_DDI_B_POWER_DOMAINS,
> > +		.ops = &skl_power_well_ops,
> > +		.data = SKL_DISP_PW_DDI_B,
> > +	},
> > +	{
> > +		.name = "DDI C power well",
> > +		.domains = SKL_DISPLAY_DDI_C_POWER_DOMAINS,
> > +		.ops = &skl_power_well_ops,
> > +		.data = SKL_DISP_PW_DDI_C,
> > +	},
> > +	{
> > +		.name = "DDI D power well",
> > +		.domains = SKL_DISPLAY_DDI_D_POWER_DOMAINS,
> > +		.ops = &skl_power_well_ops,
> > +		.data = SKL_DISP_PW_DDI_D,
> > +	},
> > +};
> > +
> >  #define set_power_wells(power_domains, __power_wells) ({		\
> >  	(power_domains)->power_wells = (__power_wells);			\
> >  	(power_domains)->power_well_count = ARRAY_SIZE(__power_wells);	\
> > @@ -8030,6 +8234,9 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
> >  	} else if (IS_BROADWELL(dev_priv->dev)) {
> >  		set_power_wells(power_domains, bdw_power_wells);
> >  		hsw_pwr = power_domains;
> > +	} else if (IS_SKYLAKE(dev_priv->dev)) {
> > +		set_power_wells(power_domains, skl_power_wells);
> > +		hsw_pwr = power_domains;
> >  	} else if (IS_CHERRYVIEW(dev_priv->dev)) {
> >  		set_power_wells(power_domains, chv_power_wells);
> >  	} else if (IS_VALLEYVIEW(dev_priv->dev)) {
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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