Re: [PATCH 1/2] drm/i915/vlv: Make the vlv_dpio_read/vlv_dpio_write more PHY centric

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 30, 2013 at 11:11:17AM +0800, Chon Ming Lee wrote:
> vlv_dpio_read/write should be describe more in PHY centric instead of
> display controller centric.
> Create a enum dpio_channel for channel index and enum dpio_phy for PHY
> index.  This should better to gather for upcoming platform.
> 
> v2: Rebase the code based on
> drm/i915/vlv: Fix typo in the DPIO register define.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |   13 +++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h       |    2 ++
>  drivers/gpu/drm/i915/intel_display.c  |   16 ++++++++++++----
>  drivers/gpu/drm/i915/intel_dp.c       |    9 ++++-----
>  drivers/gpu/drm/i915/intel_drv.h      |    7 ++++---
>  drivers/gpu/drm/i915/intel_hdmi.c     |    9 ++++-----
>  drivers/gpu/drm/i915/intel_sideband.c |   13 ++-----------
>  7 files changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2731fbb..b1609ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -88,6 +88,18 @@ enum port {
>  };
>  #define port_name(p) ((p) + 'A')
>  
> +#define I915_NUM_PHYS_VLV 1
> +
> +enum dpio_channel {
> +	DPIO_CH0,
> +	DPIO_CH1
> +};
> +
> +enum dpio_phy {
> +	DPIO_PHY0,
> +	DPIO_PHY1
> +};
> +
>  enum intel_display_power_domain {
>  	POWER_DOMAIN_PIPE_A,
>  	POWER_DOMAIN_PIPE_B,
> @@ -1401,6 +1413,7 @@ typedef struct drm_i915_private {
>  	int num_shared_dpll;
>  	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
>  	struct intel_ddi_plls ddi_plls;
> +	int vlv_phy[I915_NUM_PHYS_VLV];
>  
>  	/* Reclocking support */
>  	bool render_reclock_avail;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f7ecad2..dd8ff3b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -447,6 +447,8 @@
>  #define DPIO_TX3_SWING_CTL4(pipe) _PIPE(pipe, _DPIO_TX3_SWING_CTL4_A, \
>  					_DPIO_TX3_SWING_CTL4_B)
>  
> +#define DPIO_PHY_PORT(pipe)		(dev_priv->vlv_phy[pipe >> 1])

I don't think this vlv_phy[] thing really helps things. It does the
pipe->phy->iosf port mapping in a bit of a magic way.

Maybe rename the vlv_phy[] to something like dpio_phy_iosf_port[] and
have two macros like so:
 DPIO_PHY(pipe) ((pipe) >> 1)
 DPIO_PHY_IOSF_PORT(phy) (dev_priv->dpio_phy_iosf_port[phy])

> +
>  /*
>   * Per pipe/PLL DPIO regs
>   */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8f40ae3..c08f9f8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1361,6 +1361,7 @@ static void intel_init_dpio(struct drm_device *dev)
>  	if (!IS_VALLEYVIEW(dev))
>  		return;
>  
> +	dev_priv->vlv_phy[DPIO_PHY0] = IOSF_PORT_DPIO;
>  	/*
>  	 * From VLV2A0_DP_eDP_DPIO_driver_vbios_notes_10.docx -
>  	 *  6.	De-assert cmn_reset/side_reset. Same as VLV X0.
> @@ -1494,18 +1495,25 @@ static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	POSTING_READ(DPLL(pipe));
>  }
>  
> -void vlv_wait_port_ready(struct drm_i915_private *dev_priv, int port)
> +void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> +		struct intel_digital_port *dport)
>  {
>  	u32 port_mask;
>  
> -	if (!port)
> +	switch (dport->port) {
> +	case PORT_B:
>  		port_mask = DPLL_PORTB_READY_MASK;
> -	else
> +		break;
> +	case PORT_C:
>  		port_mask = DPLL_PORTC_READY_MASK;
> +		break;
> +	default:
> +		BUG();
> +	}
>  
>  	if (wait_for((I915_READ(DPLL(0)) & port_mask) == 0, 1000))
>  		WARN(1, "timed out waiting for port %c ready: 0x%08x\n",
> -		     'B' + port, I915_READ(DPLL(0)));
> +		     'B' + dport->port, I915_READ(DPLL(0)));
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b3cc333..5d00c83 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1831,7 +1831,7 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> -	int port = vlv_dport_to_channel(dport);
> +	enum dpio_channel port = vlv_dport_to_channel(dport);
>  	int pipe = intel_crtc->pipe;
>  	struct edp_power_seq power_seq;
>  	u32 val;
> @@ -1839,7 +1839,6 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
>  	mutex_lock(&dev_priv->dpio_lock);
>  
>  	val = vlv_dpio_read(dev_priv, pipe, DPIO_DATA_LANE_A(port));
> -	val = 0;

Unrelated change.

The rest looks fine to me.

>  	if (pipe)
>  		val |= (1<<21);
>  	else
> @@ -1858,7 +1857,7 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
>  
>  	intel_enable_dp(encoder);
>  
> -	vlv_wait_port_ready(dev_priv, port);
> +	vlv_wait_port_ready(dev_priv, dport);
>  }
>  
>  static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
> @@ -1868,7 +1867,7 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc =
>  		to_intel_crtc(encoder->base.crtc);
> -	int port = vlv_dport_to_channel(dport);
> +	enum dpio_channel port = vlv_dport_to_channel(dport);
>  	int pipe = intel_crtc->pipe;
>  
>  	/* Program Tx lane resets to default */
> @@ -2025,7 +2024,7 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp *intel_dp)
>  	unsigned long demph_reg_value, preemph_reg_value,
>  		uniqtranscale_reg_value;
>  	uint8_t train_set = intel_dp->train_set[0];
> -	int port = vlv_dport_to_channel(dport);
> +	enum dpio_channel port = vlv_dport_to_channel(dport);
>  	int pipe = intel_crtc->pipe;
>  
>  	switch (train_set & DP_TRAIN_PRE_EMPHASIS_MASK) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9d2624f..0526d43 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -490,9 +490,9 @@ vlv_dport_to_channel(struct intel_digital_port *dport)
>  {
>  	switch (dport->port) {
>  	case PORT_B:
> -		return 0;
> +		return DPIO_CH0;
>  	case PORT_C:
> -		return 1;
> +		return DPIO_CH1;
>  	default:
>  		BUG();
>  	}
> @@ -637,7 +637,8 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
>  void intel_wait_for_vblank(struct drm_device *dev, int pipe);
>  void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
> -void vlv_wait_port_ready(struct drm_i915_private *dev_priv, int port);
> +void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> +			 struct intel_digital_port *dport);
>  bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  				struct drm_display_mode *mode,
>  				struct intel_load_detect_pipe *old);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 51a8336..49977c9 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1081,7 +1081,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc =
>  		to_intel_crtc(encoder->base.crtc);
> -	int port = vlv_dport_to_channel(dport);
> +	enum dpio_channel port = vlv_dport_to_channel(dport);
>  	int pipe = intel_crtc->pipe;
>  	u32 val;
>  
> @@ -1091,7 +1091,6 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>  	/* Enable clock channels for this port */
>  	mutex_lock(&dev_priv->dpio_lock);
>  	val = vlv_dpio_read(dev_priv, pipe, DPIO_DATA_LANE_A(port));
> -	val = 0;
>  	if (pipe)
>  		val |= (1<<21);
>  	else
> @@ -1124,7 +1123,7 @@ static void vlv_hdmi_pre_enable(struct intel_encoder *encoder)
>  
>  	intel_enable_hdmi(encoder);
>  
> -	vlv_wait_port_ready(dev_priv, port);
> +	vlv_wait_port_ready(dev_priv, dport);
>  }
>  
>  static void vlv_hdmi_pre_pll_enable(struct intel_encoder *encoder)
> @@ -1134,7 +1133,7 @@ static void vlv_hdmi_pre_pll_enable(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc =
>  		to_intel_crtc(encoder->base.crtc);
> -	int port = vlv_dport_to_channel(dport);
> +	enum dpio_channel port = vlv_dport_to_channel(dport);
>  	int pipe = intel_crtc->pipe;
>  
>  	if (!IS_VALLEYVIEW(dev))
> @@ -1169,7 +1168,7 @@ static void vlv_hdmi_post_disable(struct intel_encoder *encoder)
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>  	struct intel_crtc *intel_crtc =
>  		to_intel_crtc(encoder->base.crtc);
> -	int port = vlv_dport_to_channel(dport);
> +	enum dpio_channel port = vlv_dport_to_channel(dport);
>  	int pipe = intel_crtc->pipe;
>  
>  	/* Reset lanes to avoid HDMI flicker (VLV w/a) */
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index 9944d81..159210f 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -160,27 +160,18 @@ void vlv_gps_core_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
>  			PUNIT_OPCODE_REG_WRITE, reg, &val);
>  }
>  
> -static u32 vlv_get_phy_port(enum pipe pipe)
> -{
> -	u32 port = IOSF_PORT_DPIO;
> -
> -	WARN_ON ((pipe != PIPE_A) && (pipe != PIPE_B));
> -
> -	return port;
> -}
> -
>  u32 vlv_dpio_read(struct drm_i915_private *dev_priv, enum pipe pipe, int reg)
>  {
>  	u32 val = 0;
>  
> -	vlv_sideband_rw(dev_priv, DPIO_DEVFN, vlv_get_phy_port(pipe),
> +	vlv_sideband_rw(dev_priv, DPIO_DEVFN, DPIO_PHY_PORT(pipe),
>  			DPIO_OPCODE_REG_READ, reg, &val);
>  	return val;
>  }
>  
>  void vlv_dpio_write(struct drm_i915_private *dev_priv, enum pipe pipe, int reg, u32 val)
>  {
> -	vlv_sideband_rw(dev_priv, DPIO_DEVFN, vlv_get_phy_port(pipe),
> +	vlv_sideband_rw(dev_priv, DPIO_DEVFN, DPIO_PHY_PORT(pipe),
>  			DPIO_OPCODE_REG_WRITE, reg, &val);
>  }
>  
> -- 
> 1.7.7.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC