Re: [PATCH 7/8] drm/i915/icl: Introduce new macros to get combophy registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 03, 2018 at 12:52:02PM +0530, Mahesh Kumar wrote:
> From: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> 
> combo-phy register instances are at same offset from base for each
> combo-phy port, i.e.
> 
> Port A base offset: 0x16200
> Port B base offset: 0x6C000
> 
> All the other addresses for both ports can be derived by calculating
> offset to these base addresses.
> 
> PORT_CL_DW_OFFSET	0x0
> PORT_CL_DW<x>		0 + x * 4
> 
> PORT_COMP_OFFSET	0x100
> PORT_COMP_DW<x>		0x100 + x * 4
> 
> PORT_PCS_AUX_OFFSET     0x300
> PORT_PCS_GRP_OFFSET     0x600
> PORT_PCS_LN<y>_OFFSET   0x800 + y * 0x100
> 
> PORT_TX_AUX_OFFSET      0x380
> PORT_TX_GRP_OFFSET      0x680
> PORT_TX_LN<y>_OFFSET    0x880 + y * 0x100

well, in the past I was in favor of trying to find
logic and simplify as much as possible the register offsets and bits.

However nowadays I'm more inclined to keep them explicit for some
reasons.

1. Another developer when adding some workaround later might
search the code for the reg offset, not finding and adding it again.
In this case the risk is minimal because it is only a risk of
duplicating the offset definition.

2. HW Architects when planing to remove some bits consult us
to see the impact and I always get myself searching for the offsets.
In this case the risk is higher because if we can't find the hidden
offset we might underestimate the impact on a future hw generation.

3. I'm checking to see if there are better ways to get
spec changes to notify us that we have to change something.
It would be harder to parse, but I know it is possible.

Well, maybe a tool for 3 could already answer the item above
in a automated and more reliable way... But while we don't have
such tool maybe it is better to keep explicit.

But I'm not blocking or anything like that. I'm just brainstorming
some points here from my view on this.

another detail below:

> 
> And inside each PORT_TX_[AUX|GRP|LN] we add `dw * 4`.
> 
> Based on original patch by Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 154 ++++++++++++++--------------------------
>  1 file changed, 54 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e3ac65f5aa81..eaf3e0d529d3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1658,21 +1658,21 @@ enum i915_power_well_id {
>  /*
>   * CNL/ICL Port/COMBO-PHY Registers
>   */
> +#define _ICL_COMBOPHY_A			0x162000
> +#define _ICL_COMBOPHY_B			0x6C000
> +#define _ICL_COMBOPHY(port)		_PICK(port, _ICL_COMBOPHY_A, \
> +					      _ICL_COMBOPHY_B)
> +
>  /* CNL/ICL Port CL_DW registers */
> -#define CNL_PORT_CL1CM_DW5	_MMIO(0x162014)
> +#define _ICL_PORT_CL_DW(port, dw)	(_ICL_COMBOPHY(port) + \
> +					 4 * (dw))

probably better if inverted DW(dw, port)
because reg definition is DW#dw_#port and I got
confused when reviewing items below...

>  
> -#define _ICL_PORT_CL_DW5_A	0x162014
> -#define _ICL_PORT_CL_DW5_B	0x6C014
> -#define ICL_PORT_CL_DW5(port)	_MMIO_PORT(port, _ICL_PORT_CL_DW5_A, \
> -						 _ICL_PORT_CL_DW5_B)
> +#define CNL_PORT_CL1CM_DW5		_MMIO(0x162014)
> +#define ICL_PORT_CL_DW5(port)		_MMIO(_ICL_PORT_CL_DW(port, 5))
>  #define   CL_POWER_DOWN_ENABLE		(1 << 4)
>  #define   SUS_CLOCK_CONFIG		(3 << 0)
>  
> -#define _CNL_PORT_CL_DW10_A		0x162028
> -#define _ICL_PORT_CL_DW10_B		0x6c028
> -#define ICL_PORT_CL_DW10(port)		_MMIO_PORT(port,	\
> -						   _CNL_PORT_CL_DW10_A, \
> -						   _ICL_PORT_CL_DW10_B)
> +#define ICL_PORT_CL_DW10(port)		_MMIO(_ICL_PORT_CL_DW(port, 10))
>  #define  PG_SEQ_DELAY_OVERRIDE_MASK	(3 << 25)
>  #define  PG_SEQ_DELAY_OVERRIDE_SHIFT	25
>  #define  PG_SEQ_DELAY_OVERRIDE_ENABLE	(1 << 24)
> @@ -1688,31 +1688,23 @@ enum i915_power_well_id {
>  #define  PWR_DOWN_LN_MASK		(0xf << 4)
>  #define  PWR_DOWN_LN_SHIFT		4
>  
> -#define _ICL_PORT_CL_DW12_A		0x162030
> -#define _ICL_PORT_CL_DW12_B		0x6C030
> +#define ICL_PORT_CL_DW12(port)		_MMIO(_ICL_PORT_CL_DW(port, 12))
>  #define   ICL_LANE_ENABLE_AUX		(1 << 0)
> -#define ICL_PORT_CL_DW12(port)		_MMIO_PORT((port),		\
> -						   _ICL_PORT_CL_DW12_A, \
> -						   _ICL_PORT_CL_DW12_B)
>  
>  /* CNL/ICL Port COMP_DW registers */
> +#define _ICL_PORT_COMP			0x100
> +#define _ICL_PORT_COMP_DW(port, dw)	(_ICL_COMBOPHY(port) + \
> +					 _ICL_PORT_COMP + 4 * (dw))
> +
>  #define CNL_PORT_COMP_DW0		_MMIO(0x162100)
> -#define _ICL_PORT_COMP_DW0_A		0x162100
> -#define _ICL_PORT_COMP_DW0_B		0x6C100
> -#define ICL_PORT_COMP_DW0(port)		_MMIO_PORT(port, _ICL_PORT_COMP_DW0_A, \
> -							 _ICL_PORT_COMP_DW0_B)
> +#define ICL_PORT_COMP_DW0(port)		_MMIO(_ICL_PORT_COMP_DW(port, 0))
>  #define   COMP_INIT			(1 << 31)
>  
>  #define CNL_PORT_COMP_DW1		_MMIO(0x162104)
> -#define _ICL_PORT_COMP_DW1_A		0x162104
> -#define _ICL_PORT_COMP_DW1_B		0x6C104
> -#define ICL_PORT_COMP_DW1(port)		_MMIO_PORT(port, _ICL_PORT_COMP_DW1_A, \
> -							 _ICL_PORT_COMP_DW1_B)
> +#define ICL_PORT_COMP_DW1(port)		_MMIO(_ICL_PORT_COMP_DW(port, 1))
> +
>  #define CNL_PORT_COMP_DW3		_MMIO(0x16210c)
> -#define _ICL_PORT_COMP_DW3_A		0x16210C
> -#define _ICL_PORT_COMP_DW3_B		0x6C10C
> -#define ICL_PORT_COMP_DW3(port)		_MMIO_PORT(port, _ICL_PORT_COMP_DW3_A, \
> -							 _ICL_PORT_COMP_DW3_B)
> +#define ICL_PORT_COMP_DW3(port)		_MMIO(_ICL_PORT_COMP_DW(port, 3))
>  #define   PROCESS_INFO_DOT_0		(0 << 26)
>  #define   PROCESS_INFO_DOT_1		(1 << 26)
>  #define   PROCESS_INFO_DOT_4		(2 << 26)
> @@ -1725,17 +1717,10 @@ enum i915_power_well_id {
>  #define   VOLTAGE_INFO_SHIFT		24
>  
>  #define CNL_PORT_COMP_DW9		_MMIO(0x162124)
> -#define _ICL_PORT_COMP_DW9_A		0x162124
> -#define _ICL_PORT_COMP_DW9_B		0x6C124
> -#define ICL_PORT_COMP_DW9(port)		_MMIO_PORT(port, _ICL_PORT_COMP_DW9_A, \
> -							 _ICL_PORT_COMP_DW9_B)
> +#define ICL_PORT_COMP_DW9(port)		_MMIO(_ICL_PORT_COMP_DW((port), 9))
>  
>  #define CNL_PORT_COMP_DW10		_MMIO(0x162128)
> -#define _ICL_PORT_COMP_DW10_A		0x162128
> -#define _ICL_PORT_COMP_DW10_B		0x6C128
> -#define ICL_PORT_COMP_DW10(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_COMP_DW10_A, \
> -						   _ICL_PORT_COMP_DW10_B)
> +#define ICL_PORT_COMP_DW10(port)	_MMIO(_ICL_PORT_COMP_DW((port), 10))
>  
>  /* CNL/ICL Port PCS registers */
>  #define _CNL_PORT_PCS_DW1_GRP_AE	0x162304
> @@ -1755,7 +1740,6 @@ enum i915_power_well_id {
>  						    _CNL_PORT_PCS_DW1_GRP_D, \
>  						    _CNL_PORT_PCS_DW1_GRP_AE, \
>  						    _CNL_PORT_PCS_DW1_GRP_F))
> -
>  #define CNL_PORT_PCS_DW1_LN0(port)	_MMIO(_PICK(port, \
>  						    _CNL_PORT_PCS_DW1_LN0_AE, \
>  						    _CNL_PORT_PCS_DW1_LN0_B, \
> @@ -1764,21 +1748,18 @@ enum i915_power_well_id {
>  						    _CNL_PORT_PCS_DW1_LN0_AE, \
>  						    _CNL_PORT_PCS_DW1_LN0_F))
>  
> -#define _ICL_PORT_PCS_DW1_GRP_A		0x162604
> -#define _ICL_PORT_PCS_DW1_GRP_B		0x6C604
> -#define _ICL_PORT_PCS_DW1_LN0_A		0x162804
> -#define _ICL_PORT_PCS_DW1_LN0_B		0x6C804
> -#define _ICL_PORT_PCS_DW1_AUX_A		0x162304
> -#define _ICL_PORT_PCS_DW1_AUX_B		0x6c304
> -#define ICL_PORT_PCS_DW1_GRP(port)	_MMIO_PORT(port,\
> -						   _ICL_PORT_PCS_DW1_GRP_A, \
> -						   _ICL_PORT_PCS_DW1_GRP_B)
> -#define ICL_PORT_PCS_DW1_LN0(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_PCS_DW1_LN0_A, \
> -						   _ICL_PORT_PCS_DW1_LN0_B)
> -#define ICL_PORT_PCS_DW1_AUX(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_PCS_DW1_AUX_A, \
> -						   _ICL_PORT_PCS_DW1_AUX_B)
> +#define _ICL_PORT_PCS_AUX		0x300
> +#define _ICL_PORT_PCS_GRP		0x600
> +#define _ICL_PORT_PCS_LN(ln)		(0x800 + (ln) * 0x100)
> +#define _ICL_PORT_PCS_DW_AUX(port, dw)	(_ICL_COMBOPHY(port) + \
> +					 _ICL_PORT_PCS_AUX + 4 * (dw))
> +#define _ICL_PORT_PCS_DW_GRP(port, dw)	(_ICL_COMBOPHY(port) + \
> +					 _ICL_PORT_PCS_GRP + 4 * (dw))
> +#define _ICL_PORT_PCS_DW_LN(port, dw, ln) (_ICL_COMBOPHY(port) + \
> +					  _ICL_PORT_PCS_LN(ln) + 4 * (dw))
> +#define ICL_PORT_PCS_DW1_AUX(port)	_MMIO(_ICL_PORT_PCS_DW_AUX(port, 1))
> +#define ICL_PORT_PCS_DW1_GRP(port)	_MMIO(_ICL_PORT_PCS_DW_GRP(port, 1))
> +#define ICL_PORT_PCS_DW1_LN0(port)	_MMIO(_ICL_PORT_PCS_DW_LN(port, 1, 0))
>  #define   COMMON_KEEPER_EN		(1 << 26)
>  
>  /* CNL/ICL Port TX registers */
> @@ -1809,23 +1790,22 @@ enum i915_power_well_id {
>  					       _CNL_PORT_TX_F_LN0_OFFSET) + \
>  					       4 * (dw))
>  
> +#define _ICL_PORT_TX_AUX		0x380
> +#define _ICL_PORT_TX_GRP		0x680
> +#define _ICL_PORT_TX_LN(ln)		(0x880 + (ln) * 0x100)
> +
> +#define _ICL_PORT_TX_DW_AUX(port, dw)	(_ICL_COMBOPHY(port) + \
> +					 _ICL_PORT_TX_AUX + 4 * (dw))
> +#define _ICL_PORT_TX_DW_GRP(port, dw)	(_ICL_COMBOPHY(port) + \
> +					 _ICL_PORT_TX_GRP + 4 * (dw))
> +#define _ICL_PORT_TX_DW_LN(port, dw, ln) (_ICL_COMBOPHY(port) + \
> +					  _ICL_PORT_TX_LN(ln) + 4 * (dw))
> +
>  #define CNL_PORT_TX_DW2_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP((port), 2))
>  #define CNL_PORT_TX_DW2_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0((port), 2))
> -#define _ICL_PORT_TX_DW2_GRP_A		0x162688
> -#define _ICL_PORT_TX_DW2_GRP_B		0x6C688
> -#define _ICL_PORT_TX_DW2_LN0_A		0x162888
> -#define _ICL_PORT_TX_DW2_LN0_B		0x6C888
> -#define _ICL_PORT_TX_DW2_AUX_A		0x162388
> -#define _ICL_PORT_TX_DW2_AUX_B		0x6c388
> -#define ICL_PORT_TX_DW2_GRP(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_TX_DW2_GRP_A, \
> -						   _ICL_PORT_TX_DW2_GRP_B)
> -#define ICL_PORT_TX_DW2_LN0(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_TX_DW2_LN0_A, \
> -						   _ICL_PORT_TX_DW2_LN0_B)
> -#define ICL_PORT_TX_DW2_AUX(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_TX_DW2_AUX_A, \
> -						   _ICL_PORT_TX_DW2_AUX_B)
> +#define ICL_PORT_TX_DW2_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX((port), 2))
> +#define ICL_PORT_TX_DW2_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP((port), 2))
> +#define ICL_PORT_TX_DW2_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN((port), 2, 0))
>  #define   SWING_SEL_UPPER(x)		(((x) >> 3) << 15)
>  #define   SWING_SEL_UPPER_MASK		(1 << 15)
>  #define   SWING_SEL_LOWER(x)		(((x) & 0x7) << 11)
> @@ -1842,24 +1822,10 @@ enum i915_power_well_id {
>  #define CNL_PORT_TX_DW4_LN(port, ln)   _MMIO(_CNL_PORT_TX_DW_LN0((port), 4) + \
>  					   ((ln) * (_CNL_PORT_TX_DW4_LN1_AE - \
>  						    _CNL_PORT_TX_DW4_LN0_AE)))
> -#define _ICL_PORT_TX_DW4_GRP_A		0x162690
> -#define _ICL_PORT_TX_DW4_GRP_B		0x6C690
> -#define _ICL_PORT_TX_DW4_LN0_A		0x162890
> -#define _ICL_PORT_TX_DW4_LN1_A		0x162990
> -#define _ICL_PORT_TX_DW4_LN0_B		0x6C890
> -#define _ICL_PORT_TX_DW4_AUX_A		0x162390
> -#define _ICL_PORT_TX_DW4_AUX_B		0x6c390
> -#define ICL_PORT_TX_DW4_GRP(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_TX_DW4_GRP_A, \
> -						   _ICL_PORT_TX_DW4_GRP_B)
> -#define ICL_PORT_TX_DW4_LN(port, ln)	_MMIO(_PORT(port, \
> -						   _ICL_PORT_TX_DW4_LN0_A, \
> -						   _ICL_PORT_TX_DW4_LN0_B) + \
> -					     ((ln) * (_ICL_PORT_TX_DW4_LN1_A - \
> -						      _ICL_PORT_TX_DW4_LN0_A)))
> -#define ICL_PORT_TX_DW4_AUX(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_TX_DW4_AUX_A, \
> -						   _ICL_PORT_TX_DW4_AUX_B)
> +#define ICL_PORT_TX_DW4_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX((port), 4))
> +#define ICL_PORT_TX_DW4_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP((port), 4))
> +#define ICL_PORT_TX_DW4_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN((port), 4, 0))
> +#define ICL_PORT_TX_DW4_LN(port, ln)	_MMIO(_ICL_PORT_TX_DW_LN((port), 4, ln))
>  #define   LOADGEN_SELECT		(1 << 31)
>  #define   POST_CURSOR_1(x)		((x) << 12)
>  #define   POST_CURSOR_1_MASK		(0x3F << 12)
> @@ -1870,21 +1836,9 @@ enum i915_power_well_id {
>  
>  #define CNL_PORT_TX_DW5_GRP(port)	_MMIO(_CNL_PORT_TX_DW_GRP((port), 5))
>  #define CNL_PORT_TX_DW5_LN0(port)	_MMIO(_CNL_PORT_TX_DW_LN0((port), 5))
> -#define _ICL_PORT_TX_DW5_GRP_A		0x162694
> -#define _ICL_PORT_TX_DW5_GRP_B		0x6C694
> -#define _ICL_PORT_TX_DW5_LN0_A		0x162894
> -#define _ICL_PORT_TX_DW5_LN0_B		0x6C894
> -#define _ICL_PORT_TX_DW5_AUX_A		0x162394
> -#define _ICL_PORT_TX_DW5_AUX_B		0x6c394
> -#define ICL_PORT_TX_DW5_GRP(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_TX_DW5_GRP_A, \
> -						   _ICL_PORT_TX_DW5_GRP_B)
> -#define ICL_PORT_TX_DW5_LN0(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_TX_DW5_LN0_A, \
> -						   _ICL_PORT_TX_DW5_LN0_B)
> -#define ICL_PORT_TX_DW5_AUX(port)	_MMIO_PORT(port, \
> -						   _ICL_PORT_TX_DW5_AUX_A, \
> -						   _ICL_PORT_TX_DW5_AUX_B)
> +#define ICL_PORT_TX_DW5_AUX(port)	_MMIO(_ICL_PORT_TX_DW_AUX((port), 5))
> +#define ICL_PORT_TX_DW5_GRP(port)	_MMIO(_ICL_PORT_TX_DW_GRP((port), 5))
> +#define ICL_PORT_TX_DW5_LN0(port)	_MMIO(_ICL_PORT_TX_DW_LN((port), 5, 0))
>  #define   TX_TRAINING_EN		(1 << 31)
>  #define   TAP2_DISABLE			(1 << 30)
>  #define   TAP3_DISABLE			(1 << 29)
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux