Re: [PATCH 1/2] drm/i915/icl: Factor out combo PHY lane power setup helper

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 24 Apr 2019, Imre Deak <imre.deak@intel.com> wrote:
> Factor out the combo PHY lane power configuration code to a separate
> helper; it will be also needed by the next patch adding the same
> configuration for DDI ports.
>
> While at it also add support to handle lane reversal which wasn't
> needed for DSI, but will be needed by DDI ports.
>
> Also, remove the macros for the power down flags, they aren't
> needed any more since we now calculate the power down mask. Many of
> those macro values (mostly the ones for the currently unused reversed
> lane configs) were actually undefined in the spec and didn't make much
> sense either.

Only PWR_DOWN_LN_1 is undefined in the spec.

>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Madhav Chauhan <madhav.chauhan@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
>  drivers/gpu/drm/i915/i915_reg.h        |  9 ---------
>  drivers/gpu/drm/i915/icl_dsi.c         | 26 +++-----------------------
>  drivers/gpu/drm/i915/intel_combo_phy.c | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dc74d33c20aa..1207ef080aae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3515,6 +3515,9 @@ void icl_combo_phys_init(struct drm_i915_private *dev_priv);
>  void icl_combo_phys_uninit(struct drm_i915_private *dev_priv);
>  void cnl_combo_phys_init(struct drm_i915_private *dev_priv);
>  void cnl_combo_phys_uninit(struct drm_i915_private *dev_priv);
> +void icl_combo_phy_power_up_lanes(struct drm_i915_private *dev_priv,
> +				  enum port port, int lane_count,
> +				  bool lane_reversal);
>  
>  int intel_gpu_freq(struct drm_i915_private *dev_priv, int val);
>  int intel_freq_opcode(struct drm_i915_private *dev_priv, int val);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b74824f0b5b1..29f16bc40b0a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1807,15 +1807,6 @@ enum i915_power_well_id {
>  #define  PG_SEQ_DELAY_OVERRIDE_MASK	(3 << 25)
>  #define  PG_SEQ_DELAY_OVERRIDE_SHIFT	25
>  #define  PG_SEQ_DELAY_OVERRIDE_ENABLE	(1 << 24)
> -#define  PWR_UP_ALL_LANES		(0x0 << 4)
> -#define  PWR_DOWN_LN_3_2_1		(0xe << 4)
> -#define  PWR_DOWN_LN_3_2		(0xc << 4)
> -#define  PWR_DOWN_LN_3			(0x8 << 4)
> -#define  PWR_DOWN_LN_2_1_0		(0x7 << 4)
> -#define  PWR_DOWN_LN_1_0		(0x3 << 4)
> -#define  PWR_DOWN_LN_1			(0x2 << 4)
> -#define  PWR_DOWN_LN_3_1		(0xa << 4)
> -#define  PWR_DOWN_LN_3_1_0		(0xb << 4)
>  #define  PWR_DOWN_LN_MASK		(0xf << 4)
>  #define  PWR_DOWN_LN_SHIFT		4
>  
> diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
> index 9d962ea1e635..88959517b668 100644
> --- a/drivers/gpu/drm/i915/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/icl_dsi.c
> @@ -363,30 +363,10 @@ static void gen11_dsi_power_up_lanes(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	enum port port;
> -	u32 tmp;
> -	u32 lane_mask;
>  
> -	switch (intel_dsi->lane_count) {
> -	case 1:
> -		lane_mask = PWR_DOWN_LN_3_1_0;
> -		break;
> -	case 2:
> -		lane_mask = PWR_DOWN_LN_3_1;
> -		break;
> -	case 3:
> -		lane_mask = PWR_DOWN_LN_3;
> -		break;
> -	case 4:
> -	default:
> -		lane_mask = PWR_UP_ALL_LANES;
> -		break;
> -	}
> -
> -	for_each_dsi_port(port, intel_dsi->ports) {
> -		tmp = I915_READ(ICL_PORT_CL_DW10(port));
> -		tmp &= ~PWR_DOWN_LN_MASK;
> -		I915_WRITE(ICL_PORT_CL_DW10(port), tmp | lane_mask);
> -	}

I look at that and it takes me maybe 10 seconds to figure out what's
going on, assuming the PWR_DOWN_LN_* macros are right.

> +	for_each_dsi_port(port, intel_dsi->ports)
> +		icl_combo_phy_power_up_lanes(dev_priv, port,
> +					     intel_dsi->lane_count, false);
>  }
>  
>  static void gen11_dsi_config_phy_lanes_sequence(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c b/drivers/gpu/drm/i915/intel_combo_phy.c
> index 2bf4359d7e41..e3ed584eca47 100644
> --- a/drivers/gpu/drm/i915/intel_combo_phy.c
> +++ b/drivers/gpu/drm/i915/intel_combo_phy.c
> @@ -203,6 +203,37 @@ static bool icl_combo_phy_verify_state(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> +static uint8_t reverse_nibble_bits(uint8_t val)
> +{
> +#define MOVBIT(v, from, to) (!!((v) & BIT(from)) << (to))
> +#define SWPBIT(v, b1, b2) (MOVBIT((v), (b1), (b2)) | MOVBIT((v), (b2), (b1)))
> +
> +	return SWPBIT(val, 0, 3) | SWPBIT(val, 1, 2);
> +#undef SWPBIT
> +#undef MOVBIT
> +}
> +
> +void icl_combo_phy_power_up_lanes(struct drm_i915_private *dev_priv,
> +				  enum port port, int lane_count,
> +				  bool lane_reversal)
> +{
> +	u32 pwr_down_mask;
> +	u32 val;
> +
> +	WARN_ON((u32)lane_count > 4);
> +
> +	pwr_down_mask = BIT(lane_count) - 1;
> +	if (lane_reversal)
> +		pwr_down_mask = reverse_nibble_bits(pwr_down_mask);
> +
> +	pwr_down_mask = ~pwr_down_mask & 0xf;
> +
> +	val = I915_READ(ICL_PORT_CL_DW10(port));
> +	val &= ~PWR_DOWN_LN_MASK;
> +	I915_WRITE(ICL_PORT_CL_DW10(port),
> +		   val | (pwr_down_mask << PWR_DOWN_LN_SHIFT));
> +}

Maybe I'm slow today, but it took me 15-30 minutes to figure out that
this screws up the 1 and 2 lane DSI values.

The unshifted value changes:

DSI x1: 1011b -> 1110b
DSI x2: 1010b -> 1100b

DSI uses different values from DDI.

There *is* value in keeping it simple and mechanic instead of trying to
force it into a formula.


BR,
Jani.


> +
>  void icl_combo_phys_init(struct drm_i915_private *dev_priv)
>  {
>  	enum port port;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx