All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: drm/i915: Make MMIO_PORT flexible.
Date: Mon, 12 Jun 2017 17:56:33 +0300	[thread overview]
Message-ID: <20170612145633.GI12629@intel.com> (raw)
In-Reply-To: <1496941505-4502-1-git-send-email-rodrigo.vivi@intel.com>

On Fri, Jun 09, 2017 at 02:07:33PM -0700, Rodrigo Vivi wrote:
> In the past the magic reg_a + (reg_b - reg_a) * port
> was enough because every new register that was appearing
> on new platforms was respecting the same gap.
> 
> However on more recent platforms we start seeing many
> registers that doesn't respect the gap.

Do you have a list of such registers?

> So picking from
> a list of registers seems to be the more general approach
> that covers all cases.
> 
> So let's make MMIO_PORT use _PICK directly so we don't

How much is this going to blow up the code size?

> need to create MMIO_PORT3 MMIO_PORT5 MMIO_PORT6 or
> whatever number of different ports with different registers
> that doesn't respect the gap.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 57 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b6d69e2..9a57a17 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -57,9 +57,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define _TRANS(tran, a, b) ((a) + (tran)*((b)-(a)))
>  #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b))
>  #define _PORT(port, a, b) ((a) + (port)*((b)-(a)))
> -#define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b))
> +#define _MMIO_PORT(port, ...) _MMIO(_PICK(port, __VA_ARGS__))
>  #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PICK(pipe, a, b, c))
> -#define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PICK(pipe, a, b, c))
>  #define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__)
>  #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c))
>  
> @@ -1576,7 +1575,8 @@ enum skl_disp_power_wells {
>  #define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
>  #define   BXT_PHY_LANE_ENABLED		(1 << 8)
>  #define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
> -							 _BXT_PHY_CTL_DDI_B)
> +							 _BXT_PHY_CTL_DDI_B, \
> +							 _BXT_PHY_CTL_DDI_C)
>  
>  #define _PHY_CTL_FAMILY_EDP		0x64C80
>  #define _PHY_CTL_FAMILY_DDI		0x64C90
> @@ -1595,7 +1595,9 @@ enum skl_disp_power_wells {
>  #define   PORT_PLL_REF_SEL		(1 << 27)
>  #define   PORT_PLL_POWER_ENABLE		(1 << 26)
>  #define   PORT_PLL_POWER_STATE		(1 << 25)
> -#define BXT_PORT_PLL_ENABLE(port)	_MMIO_PORT(port, _PORT_PLL_A, _PORT_PLL_B)
> +#define BXT_PORT_PLL_ENABLE(port)	_MMIO_PORT(port, _PORT_PLL_A, \
> +							 _PORT_PLL_B, \
> +							 _PORT_PLL_C)
>  
>  #define _PORT_PLL_EBB_0_A		0x162034
>  #define _PORT_PLL_EBB_0_B		0x6C034
> @@ -2453,7 +2455,7 @@ enum skl_disp_power_wells {
>  #define _VLV_AUD_PORT_EN_B_DBG		(VLV_DISPLAY_BASE + 0x62F20)
>  #define _VLV_AUD_PORT_EN_C_DBG		(VLV_DISPLAY_BASE + 0x62F30)
>  #define _VLV_AUD_PORT_EN_D_DBG		(VLV_DISPLAY_BASE + 0x62F34)
> -#define VLV_AUD_PORT_EN_DBG(port)	_MMIO_PORT3((port) - PORT_B,	   \
> +#define VLV_AUD_PORT_EN_DBG(port)	_MMIO_PORT((port) - PORT_B,	    \
>  						    _VLV_AUD_PORT_EN_B_DBG, \
>  						    _VLV_AUD_PORT_EN_C_DBG, \
>  						    _VLV_AUD_PORT_EN_D_DBG)

There's really no need to change all the platforms. So would probably be
better to limit the PICK to where we actually need it.

> @@ -4821,7 +4823,10 @@ enum {
>  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
>  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
>  
> -#define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> +#define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, \
> +						 _DPB_AUX_CH_CTL, \
> +						 _DPC_AUX_CH_CTL, \
> +						 _DPD_AUX_CH_CTL)
>  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
>  
>  #define   DP_AUX_CH_CTL_SEND_BUSY	    (1 << 31)
> @@ -7214,7 +7219,9 @@ enum {
>  #define _PCH_DPD_AUX_CH_DATA4	0xe4320
>  #define _PCH_DPD_AUX_CH_DATA5	0xe4324
>  
> -#define PCH_DP_AUX_CH_CTL(port)		_MMIO_PORT((port) - PORT_B, _PCH_DPB_AUX_CH_CTL, _PCH_DPC_AUX_CH_CTL)
> +#define PCH_DP_AUX_CH_CTL(port)		_MMIO_PORT((port) - PORT_B, \
> +						   _PCH_DPB_AUX_CH_CTL, \
> +						   _PCH_DPC_AUX_CH_CTL)
>  #define PCH_DP_AUX_CH_DATA(port, i)	_MMIO(_PORT((port) - PORT_B, _PCH_DPB_AUX_CH_DATA1, _PCH_DPC_AUX_CH_DATA1) + (i) * 4) /* 5 registers */

You changed CTL but didn't change DATA. Why?

Also note that port D is missing from CTL now!

>  
>  /* CPT */
> @@ -7844,7 +7851,14 @@ enum {
>  /* DisplayPort Transport Control */
>  #define _DP_TP_CTL_A			0x64040
>  #define _DP_TP_CTL_B			0x64140
> -#define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
> +#define _DP_TP_CTL_C			0x64240
> +#define _DP_TP_CTL_D			0x64340
> +#define _DP_TP_CTL_E			0x64440
> +#define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, \
> +					 _DP_TP_CTL_B, \
> +					 _DP_TP_CTL_C, \
> +					 _DP_TP_CTL_D, \
> +					 _DP_TP_CTL_E)
>  #define  DP_TP_CTL_ENABLE			(1<<31)
>  #define  DP_TP_CTL_MODE_SST			(0<<27)
>  #define  DP_TP_CTL_MODE_MST			(1<<27)
> @@ -7862,7 +7876,14 @@ enum {
>  /* DisplayPort Transport Status */
>  #define _DP_TP_STATUS_A			0x64044
>  #define _DP_TP_STATUS_B			0x64144
> -#define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> +#define _DP_TP_STATUS_C			0x64244
> +#define _DP_TP_STATUS_D			0x64344
> +#define _DP_TP_STATUS_E			0x64444
> +#define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, \
> +					    _DP_TP_STATUS_B, \
> +					    _DP_TP_STATUS_C, \
> +					    _DP_TP_STATUS_D, \
> +					    _DP_TP_STATUS_E)
>  #define  DP_TP_STATUS_IDLE_DONE			(1<<25)
>  #define  DP_TP_STATUS_ACT_SENT			(1<<24)
>  #define  DP_TP_STATUS_MODE_STATUS_MST		(1<<23)
> @@ -7874,7 +7895,14 @@ enum {
>  /* DDI Buffer Control */
>  #define _DDI_BUF_CTL_A				0x64000
>  #define _DDI_BUF_CTL_B				0x64100
> -#define DDI_BUF_CTL(port) _MMIO_PORT(port, _DDI_BUF_CTL_A, _DDI_BUF_CTL_B)
> +#define _DDI_BUF_CTL_C				0x64200
> +#define _DDI_BUF_CTL_D				0x64300
> +#define _DDI_BUF_CTL_E				0x64400
> +#define DDI_BUF_CTL(port) _MMIO_PORT(port, _DDI_BUF_CTL_A, \
> +					   _DDI_BUF_CTL_B, \
> +					   _DDI_BUF_CTL_C, \
> +					   _DDI_BUF_CTL_D, \
> +					   _DDI_BUF_CTL_E)
>  #define  DDI_BUF_CTL_ENABLE			(1<<31)
>  #define  DDI_BUF_TRANS_SELECT(n)	((n) << 24)
>  #define  DDI_BUF_EMP_MASK			(0xf<<24)
> @@ -7973,7 +8001,14 @@ enum {
>  /* Port clock selection */
>  #define _PORT_CLK_SEL_A			0x46100
>  #define _PORT_CLK_SEL_B			0x46104
> -#define PORT_CLK_SEL(port) _MMIO_PORT(port, _PORT_CLK_SEL_A, _PORT_CLK_SEL_B)
> +#define _PORT_CLK_SEL_C			0x46108
> +#define _PORT_CLK_SEL_D			0x4610C
> +#define _PORT_CLK_SEL_E			0x46110
> +#define PORT_CLK_SEL(port) _MMIO_PORT(port, _PORT_CLK_SEL_A, \
> +				            _PORT_CLK_SEL_B, \
> +				            _PORT_CLK_SEL_C, \
> +				            _PORT_CLK_SEL_D, \
> +					    _PORT_CLK_SEL_E)
>  #define  PORT_CLK_SEL_LCPLL_2700	(0<<29)
>  #define  PORT_CLK_SEL_LCPLL_1350	(1<<29)
>  #define  PORT_CLK_SEL_LCPLL_810		(2<<29)
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  parent reply	other threads:[~2017-06-12 14:56 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-08 17:05 [PATCH] drm/i915: Make MMIO_PORT flexible Rodrigo Vivi
2017-06-08 17:35 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-06-12  7:32 ` ✓ Fi.CI.BAT: success " Patchwork
2017-06-12 14:56 ` Ville Syrjälä [this message]
2017-06-12 16:28   ` Vivi, Rodrigo
2017-06-12 17:11     ` Ville Syrjälä
2017-06-12 17:26       ` Rodrigo Vivi
2017-06-13 12:49         ` Ville Syrjälä
2017-06-15 19:33           ` Jani Nikula

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170612145633.GI12629@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=jani.nikula@intel.com \
    --cc=rodrigo.vivi@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.