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
