* [PATCH] drm/i915: Make MMIO_PORT flexible.
@ 2017-06-08 17:05 Rodrigo Vivi
2017-06-08 17:35 ` ✗ Fi.CI.BAT: failure for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2017-06-08 17:05 UTC (permalink / raw)
To: intel-gfx; +Cc: Jani Nikula, Rodrigo Vivi
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. 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
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)
@@ -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 */
/* 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)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Make MMIO_PORT flexible.
2017-06-08 17:05 [PATCH] drm/i915: Make MMIO_PORT flexible Rodrigo Vivi
@ 2017-06-08 17:35 ` Patchwork
2017-06-12 7:32 ` ✓ Fi.CI.BAT: success " Patchwork
2017-06-12 14:56 ` Ville Syrjälä
2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2017-06-08 17:35 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Make MMIO_PORT flexible.
URL : https://patchwork.freedesktop.org/series/25491/
State : failure
== Summary ==
Series 25491v1 drm/i915: Make MMIO_PORT flexible.
https://patchwork.freedesktop.org/api/1.0/series/25491/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s4-devices:
pass -> DMESG-WARN (fi-kbl-r) fdo#100125
Test kms_busy:
Subgroup basic-flip-default-b:
pass -> DMESG-WARN (fi-skl-6700hq) fdo#101144 +2
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
pass -> FAIL (fi-skl-6700hq) fdo#101154 +7
Test drv_module_reload:
Subgroup basic-reload-final:
pass -> INCOMPLETE (fi-snb-2600)
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#101154 https://bugs.freedesktop.org/show_bug.cgi?id=101154
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:445s
fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:435s
fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:581s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:510s
fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:486s
fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:482s
fi-glk-2a total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:591s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:432s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:415s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:420s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:504s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:471s
fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:470s
fi-kbl-7560u total:278 pass:267 dwarn:1 dfail:0 fail:0 skip:10 time:570s
fi-kbl-r total:278 pass:259 dwarn:1 dfail:0 fail:0 skip:18 time:577s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:458s
fi-skl-6700hq total:278 pass:229 dwarn:1 dfail:0 fail:26 skip:22 time:401s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:466s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:498s
fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:437s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:539s
fi-snb-2600 total:277 pass:248 dwarn:0 dfail:0 fail:0 skip:28
76bd436e051e370698f466af7573d2f146bf559e drm-tip: 2017y-06m-08d-16h-56m-04s UTC integration manifest
3247cc5 drm/i915: Make MMIO_PORT flexible.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4914/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Make MMIO_PORT flexible.
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 ` Patchwork
2017-06-12 14:56 ` Ville Syrjälä
2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2017-06-12 7:32 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Make MMIO_PORT flexible.
URL : https://patchwork.freedesktop.org/series/25491/
State : success
== Summary ==
Series 25491v1 drm/i915: Make MMIO_PORT flexible.
https://patchwork.freedesktop.org/api/1.0/series/25491/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s4-devices:
dmesg-warn -> PASS (fi-kbl-7560u) fdo#100125
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:448s
fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:432s
fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:579s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:513s
fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:480s
fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:485s
fi-glk-2a total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:589s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:434s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:410s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:418s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:483s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:470s
fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:472s
fi-kbl-7560u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:564s
fi-kbl-r total:278 pass:259 dwarn:1 dfail:0 fail:0 skip:18 time:577s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:465s
fi-skl-6700hq total:278 pass:228 dwarn:1 dfail:0 fail:27 skip:22 time:399s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:472s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:474s
fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:435s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:536s
918e5432772d75f4a190be27305d28eeb1ce748e drm-tip: 2017y-06m-11d-11h-12m-29s UTC integration manifest
b7dad2f drm/i915: Make MMIO_PORT flexible.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4932/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: drm/i915: Make MMIO_PORT flexible.
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ä
2017-06-12 16:28 ` Vivi, Rodrigo
2 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2017-06-12 14:56 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: Jani Nikula, intel-gfx
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: drm/i915: Make MMIO_PORT flexible.
2017-06-12 14:56 ` Ville Syrjälä
@ 2017-06-12 16:28 ` Vivi, Rodrigo
2017-06-12 17:11 ` Ville Syrjälä
0 siblings, 1 reply; 9+ messages in thread
From: Vivi, Rodrigo @ 2017-06-12 16:28 UTC (permalink / raw)
To: ville.syrjala@linux.intel.com
Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org
On Mon, 2017-06-12 at 17:56 +0300, Ville Syrjälä wrote:
> 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?
I don't have a list, but I remember seeing a lot on BXT and many of CNL
ones specially for the PORT F ones.
>
> > 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?
well, the initial idea I believe it was to reduce avoiding few new
macros, but during the implementation I saw that we had to actually
list more registers and indeed this end up increasing the 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.
Ok, so, what about leave MMIO_PORT as is and create a MMIO_PICK for the
registers we need list? So we avoid creating one MMIO_PORT* for every
different number of ports.
>
> > @@ -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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: drm/i915: Make MMIO_PORT flexible.
2017-06-12 16:28 ` Vivi, Rodrigo
@ 2017-06-12 17:11 ` Ville Syrjälä
2017-06-12 17:26 ` Rodrigo Vivi
0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2017-06-12 17:11 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org
On Mon, Jun 12, 2017 at 04:28:28PM +0000, Vivi, Rodrigo wrote:
> On Mon, 2017-06-12 at 17:56 +0300, Ville Syrjälä wrote:
> > 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?
>
> I don't have a list, but I remember seeing a lot on BXT and many of CNL
> ones specially for the PORT F ones.
I had a quick look and couldn't find any.
>
> >
> > > 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?
>
> well, the initial idea I believe it was to reduce avoiding few new
> macros, but during the implementation I saw that we had to actually
> list more registers and indeed this end up increasing the 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.
>
> Ok, so, what about leave MMIO_PORT as is and create a MMIO_PICK for the
> registers we need list? So we avoid creating one MMIO_PORT* for every
> different number of ports.
>
> >
> > > @@ -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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: drm/i915: Make MMIO_PORT flexible.
2017-06-12 17:11 ` Ville Syrjälä
@ 2017-06-12 17:26 ` Rodrigo Vivi
2017-06-13 12:49 ` Ville Syrjälä
0 siblings, 1 reply; 9+ messages in thread
From: Rodrigo Vivi @ 2017-06-12 17:26 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org, Vivi, Rodrigo
On Mon, Jun 12, 2017 at 10:11 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Jun 12, 2017 at 04:28:28PM +0000, Vivi, Rodrigo wrote:
>> On Mon, 2017-06-12 at 17:56 +0300, Ville Syrjälä wrote:
>> > 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?
>>
>> I don't have a list, but I remember seeing a lot on BXT and many of CNL
>> ones specially for the PORT F ones.
>
> I had a quick look and couldn't find any.
oh, the BXT ones are using BXT_PHY_BASE...
but for the existent ones is the one that uses MMIO_PORT3:
_VLV_AUD_PORT_EN_B_DBG
>>> 0x62F34-0x62F30
4
>>> 0x62F30-0x62F20
16
and users of MMIO_PORT6 that I just merged on dinq:
CNL_PORT_PCS_DW1_LN0:
>>> 0x162804-0x162E04
-1536
>>> 0x162E04-0x162C04
512
>>> 0x162C04-0x162604
1536
>>> 0x162604-0x162404
512
>
>>
>> >
>> > > 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?
>>
>> well, the initial idea I believe it was to reduce avoiding few new
>> macros, but during the implementation I saw that we had to actually
>> list more registers and indeed this end up increasing the 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.
>>
>> Ok, so, what about leave MMIO_PORT as is and create a MMIO_PICK for the
>> registers we need list? So we avoid creating one MMIO_PORT* for every
>> different number of ports.
>>
>> >
>> > > @@ -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
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: drm/i915: Make MMIO_PORT flexible.
2017-06-12 17:26 ` Rodrigo Vivi
@ 2017-06-13 12:49 ` Ville Syrjälä
2017-06-15 19:33 ` Jani Nikula
0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2017-06-13 12:49 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org, Vivi, Rodrigo
On Mon, Jun 12, 2017 at 10:26:00AM -0700, Rodrigo Vivi wrote:
> On Mon, Jun 12, 2017 at 10:11 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Jun 12, 2017 at 04:28:28PM +0000, Vivi, Rodrigo wrote:
> >> On Mon, 2017-06-12 at 17:56 +0300, Ville Syrjälä wrote:
> >> > 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?
> >>
> >> I don't have a list, but I remember seeing a lot on BXT and many of CNL
> >> ones specially for the PORT F ones.
> >
> > I had a quick look and couldn't find any.
>
> oh, the BXT ones are using BXT_PHY_BASE...
>
> but for the existent ones is the one that uses MMIO_PORT3:
> _VLV_AUD_PORT_EN_B_DBG
> >>> 0x62F34-0x62F30
> 4
> >>> 0x62F30-0x62F20
> 16
>
> and users of MMIO_PORT6 that I just merged on dinq:
>
> CNL_PORT_PCS_DW1_LN0:
> >>> 0x162804-0x162E04
> -1536
> >>> 0x162E04-0x162C04
> 512
> >>> 0x162C04-0x162604
> 1536
> >>> 0x162604-0x162404
> 512
If there's lot of these and they get used a lot then I think the best
option might be to add some kind of phy_port_offsets[] type of thing.
Although it seems we'd need separate offsets for the group vs.
individual lane access.
But for just a few registers that are not used that much and are
purely CNL specific, then I guess just using _PICK for them might be
OK.
>
> >
> >>
> >> >
> >> > > 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?
> >>
> >> well, the initial idea I believe it was to reduce avoiding few new
> >> macros, but during the implementation I saw that we had to actually
> >> list more registers and indeed this end up increasing the 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.
> >>
> >> Ok, so, what about leave MMIO_PORT as is and create a MMIO_PICK for the
> >> registers we need list? So we avoid creating one MMIO_PORT* for every
> >> different number of ports.
> >>
> >> >
> >> > > @@ -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
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: drm/i915: Make MMIO_PORT flexible.
2017-06-13 12:49 ` Ville Syrjälä
@ 2017-06-15 19:33 ` Jani Nikula
0 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2017-06-15 19:33 UTC (permalink / raw)
To: Ville Syrjälä, Rodrigo Vivi
Cc: intel-gfx@lists.freedesktop.org, Vivi, Rodrigo
On Tue, 13 Jun 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> If there's lot of these and they get used a lot then I think the best
> option might be to add some kind of phy_port_offsets[] type of thing.
> Although it seems we'd need separate offsets for the group vs.
> individual lane access.
We have that for pipe_offsets, trans_offsets, palette_offsets, and
cursor_offsets in struct intel_device_info. That's fine for some things,
a bit awkward for some others. It's a bit heavy when there are just few
registers following some scheme.
> But for just a few registers that are not used that much and are
> purely CNL specific, then I guess just using _PICK for them might be
> OK.
Yes, we definitely want to avoid _PICK when ((a) + (port)*((b)-(a))) or
similar will do.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-15 19:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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ä
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).