Re: [PATCH 7/8] drm/i915: Configure AUX_CH_CTL when enabling the AUX power domain

["Souza, Jose" <jose.souza@intel.com>]

On Tue, 2018-10-30 at 17:40 +0200, Imre Deak wrote:
> Most of the AUX_CH_CTL flags are concerned with DP AUX transfer
> parameters. As opposed to this the flag specifying the thunderbolt
> vs.
> non-thunderbolt mode of the port is not related to AUX transfers at
> all
> (rather it's repurposed to enable either TBT or non-TBT PHY HW
> blocks).
> The programming has to be done before enabling the corresponding AUX
> power well, so make it part of the power well code.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108548
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

If respinning this patch please consider the comments bellow but nice
catch.

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 69
> +++++++++++++++++++++++++++++----
>  2 files changed, 62 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index c57b701f72a7..dbf894835cb2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -921,6 +921,7 @@ struct i915_power_well_desc {
>  			/* The pw is backing the VGA functionality */
>  			bool has_vga:1;
>  			bool has_fuses:1;
> +			bool is_tc_tbt;
>  		} hsw;
>  	};
>  	const struct i915_power_well_ops *ops;
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 5f5416eb9644..eed17440a4a7 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -465,6 +465,44 @@ icl_combo_phy_aux_power_well_disable(struct
> drm_i915_private *dev_priv,
>  	hsw_wait_for_power_well_disable(dev_priv, power_well);
>  }
>  
> +#define ICL_AUX_PW_TO_CH(pw_idx)	\
> +	((pw_idx) - ICL_PW_CTL_IDX_AUX_A + AUX_CH_A)
> +
> +static void
> +icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> +				 struct i915_power_well *power_well)
> +{
> +	const struct i915_power_well_regs *regs = power_well->desc-
> >hsw.regs;
> +	int pw_idx = power_well->desc->hsw.idx;
> +	enum aux_ch aux_ch = ICL_AUX_PW_TO_CH(pw_idx);
> +	u32 val;
> +
> +	val = I915_READ(DP_AUX_CH_CTL(aux_ch));
> +	val &= ~DP_AUX_CH_CTL_TBT_IO;
> +	if (power_well->desc->hsw.is_tc_tbt)
> +		val |= DP_AUX_CH_CTL_TBT_IO;
> +	I915_WRITE(DP_AUX_CH_CTL(aux_ch), val);
> +
> +	val = I915_READ(regs->driver);
> +	I915_WRITE(regs->driver, val | HSW_PWR_WELL_CTL_REQ(pw_idx));
> +
> +	hsw_wait_for_power_well_enable(dev_priv, power_well);

Minor but you could call hsw_power_well_enable() after write to
DP_AUX_CH_CTL instead of duplicate code.
 
> +}
> +
> +static void
> +icl_tc_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
> +				  struct i915_power_well *power_well)
> +{
> +	const struct i915_power_well_regs *regs = power_well->desc-
> >hsw.regs;
> +	int pw_idx = power_well->desc->hsw.idx;
> +	u32 val;
> +
> +	val = I915_READ(regs->driver);
> +	I915_WRITE(regs->driver, val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
> +
> +	hsw_wait_for_power_well_disable(dev_priv, power_well);
> +}

Minor too you could use the hsw_power_well_disable() instead of
duplicate code.

> +
>  /*
>   * We should only use the power well if we explicitly asked the
> hardware to
>   * enable it, so check if it's enabled and also check if we've
> requested it to
> @@ -2725,6 +2763,13 @@ static const struct i915_power_well_ops
> icl_combo_phy_aux_power_well_ops = {
>  	.is_enabled = hsw_power_well_enabled,
>  };
>  
> +static const struct i915_power_well_ops
> icl_tc_phy_aux_power_well_ops = {
> +	.sync_hw = hsw_power_well_sync_hw,
> +	.enable = icl_tc_phy_aux_power_well_enable,
> +	.disable = icl_tc_phy_aux_power_well_disable,
> +	.is_enabled = hsw_power_well_enabled,
> +};
> +
>  static const struct i915_power_well_regs icl_aux_power_well_regs = {
>  	.bios	= ICL_PWR_WELL_CTL_AUX1,
>  	.driver	= ICL_PWR_WELL_CTL_AUX2,
> @@ -2870,81 +2915,89 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
>  	{
>  		.name = "AUX C",
>  		.domains = ICL_AUX_C_IO_POWER_DOMAINS,
> -		.ops = &hsw_power_well_ops,
> +		.ops = &icl_tc_phy_aux_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  		{
>  			.hsw.regs = &icl_aux_power_well_regs,
>  			.hsw.idx = ICL_PW_CTL_IDX_AUX_C,
> +			.hsw.is_tc_tbt = false,
>  		},
>  	},
>  	{
>  		.name = "AUX D",
>  		.domains = ICL_AUX_D_IO_POWER_DOMAINS,
> -		.ops = &hsw_power_well_ops,
> +		.ops = &icl_tc_phy_aux_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  		{
>  			.hsw.regs = &icl_aux_power_well_regs,
>  			.hsw.idx = ICL_PW_CTL_IDX_AUX_D,
> +			.hsw.is_tc_tbt = false,
>  		},
>  	},
>  	{
>  		.name = "AUX E",
>  		.domains = ICL_AUX_E_IO_POWER_DOMAINS,
> -		.ops = &hsw_power_well_ops,
> +		.ops = &icl_tc_phy_aux_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  		{
>  			.hsw.regs = &icl_aux_power_well_regs,
>  			.hsw.idx = ICL_PW_CTL_IDX_AUX_E,
> +			.hsw.is_tc_tbt = false,
>  		},
>  	},
>  	{
>  		.name = "AUX F",
>  		.domains = ICL_AUX_F_IO_POWER_DOMAINS,
> -		.ops = &hsw_power_well_ops,
> +		.ops = &icl_tc_phy_aux_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  		{
>  			.hsw.regs = &icl_aux_power_well_regs,
>  			.hsw.idx = ICL_PW_CTL_IDX_AUX_F,
> +			.hsw.is_tc_tbt = false,
>  		},
>  	},
>  	{
>  		.name = "AUX TBT1",
>  		.domains = ICL_AUX_TBT1_IO_POWER_DOMAINS,
> -		.ops = &hsw_power_well_ops,
> +		.ops = &icl_tc_phy_aux_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  		{
>  			.hsw.regs = &icl_aux_power_well_regs,
>  			.hsw.idx = ICL_PW_CTL_IDX_AUX_TBT1,
> +			.hsw.is_tc_tbt = true,
>  		},
>  	},
>  	{
>  		.name = "AUX TBT2",
>  		.domains = ICL_AUX_TBT2_IO_POWER_DOMAINS,
> -		.ops = &hsw_power_well_ops,
> +		.ops = &icl_tc_phy_aux_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  		{
>  			.hsw.regs = &icl_aux_power_well_regs,
>  			.hsw.idx = ICL_PW_CTL_IDX_AUX_TBT2,
> +			.hsw.is_tc_tbt = true,
>  		},
>  	},
>  	{
>  		.name = "AUX TBT3",
>  		.domains = ICL_AUX_TBT3_IO_POWER_DOMAINS,
> -		.ops = &hsw_power_well_ops,
> +		.ops = &icl_tc_phy_aux_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  		{
>  			.hsw.regs = &icl_aux_power_well_regs,
>  			.hsw.idx = ICL_PW_CTL_IDX_AUX_TBT3,
> +			.hsw.is_tc_tbt = true,
>  		},
>  	},
>  	{
>  		.name = "AUX TBT4",
>  		.domains = ICL_AUX_TBT4_IO_POWER_DOMAINS,
> -		.ops = &hsw_power_well_ops,
> +		.ops = &icl_tc_phy_aux_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  		{
>  			.hsw.regs = &icl_aux_power_well_regs,
>  			.hsw.idx = ICL_PW_CTL_IDX_AUX_TBT4,
> +			.hsw.is_tc_tbt = true,
>  		},
>  	},
>  	{
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx