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 Fri, Oct 12, 2018 at 03:58:52PM -0700, Lucas De Marchi wrote:
> On Fri, Oct 12, 2018 at 03:25:37PM -0700, Rodrigo Vivi wrote:
> > 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.
> 
> 
> I agree with the reasonings, but in this particular case I think it's simpler since
> the spec itself is now modular wrt combophy instance. See 29482, although it continues
> to define the individual values, this is not always the case.

hmm... makes sense...

> 
> 
> > 
> > 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...
> 
> ok

thanks, with this changed feel free to already add
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>

> 
> thanks
> Lucas De Marchi
> 
> > 
> > >  
> > > -#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