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
