All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/1] Modular FIA
@ 2019-07-11 20:58 Lucas De Marchi
  2019-07-11 20:58 ` [PATCH v3 1/1] drm/i915: Add modular FIA Lucas De Marchi
  2019-07-11 22:06 ` ✓ Fi.CI.BAT: success for Modular FIA (rev4) Patchwork
  0 siblings, 2 replies; 8+ messages in thread
From: Lucas De Marchi @ 2019-07-11 20:58 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lucas De Marchi

https://patchwork.freedesktop.org/series/63175/

Changes:
  - Remove already merged patches
  - Move enum phy_fia to the header and use it

Anusha Srivatsa (1):
  drm/i915: Add modular FIA

 drivers/gpu/drm/i915/display/intel_display.h |  6 +++
 drivers/gpu/drm/i915/display/intel_tc.c      | 43 ++++++++++++++++----
 drivers/gpu/drm/i915/i915_reg.h              | 13 ++++--
 drivers/gpu/drm/i915/intel_device_info.h     |  1 +
 drivers/gpu/drm/i915/intel_drv.h             |  1 +
 5 files changed, 52 insertions(+), 12 deletions(-)

-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH v3 1/1] drm/i915: Add modular FIA
  2019-07-11 20:58 [PATCH v3 0/1] Modular FIA Lucas De Marchi
@ 2019-07-11 20:58 ` Lucas De Marchi
  2019-07-11 23:15   ` Summers, Stuart
  2019-07-11 22:06 ` ✓ Fi.CI.BAT: success for Modular FIA (rev4) Patchwork
  1 sibling, 1 reply; 8+ messages in thread
From: Lucas De Marchi @ 2019-07-11 20:58 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lucas De Marchi

From: Anusha Srivatsa <anusha.srivatsa@intel.com>

Some platforms may have Modular FIA. If Modular FIA is used in the SOC,
then Display Driver will access the additional instances of
FIA based on pre-assigned offset in GTTMADDR space.

Each Modular FIA instance has its own IOSF Sideband Port ID
and it houses only 2 Type-C Port. In SOC that has more than
two Type-C Ports, there are multiple instances of Modular FIA.
Gunit will need to use different destination ID when it access
different pair of Type-C Port.

The DFLEXDPSP register has Modular FIA bit starting on Tiger Lake.  If
Modular FIA is used in the SOC, this register bit exists in all the
instances of Modular FIA. IOM FW is required to program only the MF bit
in first FIA instance that houses the Type-C Port 0 and Port 1, for
Display Driver to read from.

v2 (Lucas):
  - Move all accesses to FIA to be contained in intel_tc.c, along with
    display_fia that is now called tc_phy_fia
  - Save the fia instance number on intel_digital_port, so we don't have
    to query if modular FIA is used on every access
v3 (Lucas): Make function static
v4 (Lucas): Move enum phy_fia to the header and use it in
   intel_digital_port (suggested by Ville)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.h |  6 +++
 drivers/gpu/drm/i915/display/intel_tc.c      | 43 ++++++++++++++++----
 drivers/gpu/drm/i915/i915_reg.h              | 13 ++++--
 drivers/gpu/drm/i915/intel_device_info.h     |  1 +
 drivers/gpu/drm/i915/intel_drv.h             |  1 +
 5 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 8a4a57ef82a2..8b048976f7b4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -243,6 +243,12 @@ enum phy {
 
 #define phy_name(a) ((a) + 'A')
 
+enum phy_fia {
+	FIA1,
+	FIA2,
+	FIA3,
+};
+
 #define for_each_pipe(__dev_priv, __p) \
 	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++)
 
diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index f44ee4bfe7c8..9400da4f7916 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -22,6 +22,24 @@ static const char *tc_port_mode_name(enum tc_port_mode mode)
 	return names[mode];
 }
 
+static bool has_modular_fia(struct drm_i915_private *i915)
+{
+	if (!INTEL_INFO(i915)->display.has_modular_fia)
+		return false;
+
+	return intel_uncore_read(&i915->uncore,
+				 PORT_TX_DFLEXDPSP(FIA1)) & MODULAR_FIA_MASK;
+}
+
+static enum phy_fia tc_port_to_fia(struct drm_i915_private *i915,
+				   enum tc_port tc_port)
+{
+	if (!has_modular_fia(i915))
+		return FIA1;
+
+	return tc_port / 2;
+}
+
 u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
 {
 	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
@@ -29,7 +47,8 @@ u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
 	struct intel_uncore *uncore = &i915->uncore;
 	u32 lane_mask;
 
-	lane_mask = intel_uncore_read(uncore, PORT_TX_DFLEXDPSP);
+	lane_mask = intel_uncore_read(uncore,
+				      PORT_TX_DFLEXDPSP(dig_port->tc_phy_fia));
 
 	WARN_ON(lane_mask == 0xffffffff);
 
@@ -78,7 +97,8 @@ void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
 
 	WARN_ON(lane_reversal && dig_port->tc_mode != TC_PORT_LEGACY);
 
-	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPMLE1);
+	val = intel_uncore_read(uncore,
+				PORT_TX_DFLEXDPMLE1(dig_port->tc_phy_fia));
 	val &= ~DFLEXDPMLE1_DPMLETC_MASK(tc_port);
 
 	switch (required_lanes) {
@@ -97,7 +117,8 @@ void intel_tc_port_set_fia_lane_count(struct intel_digital_port *dig_port,
 		MISSING_CASE(required_lanes);
 	}
 
-	intel_uncore_write(uncore, PORT_TX_DFLEXDPMLE1, val);
+	intel_uncore_write(uncore,
+			   PORT_TX_DFLEXDPMLE1(dig_port->tc_phy_fia), val);
 }
 
 static void tc_port_fixup_legacy_flag(struct intel_digital_port *dig_port,
@@ -129,7 +150,8 @@ static u32 tc_port_live_status_mask(struct intel_digital_port *dig_port)
 	u32 mask = 0;
 	u32 val;
 
-	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPSP);
+	val = intel_uncore_read(uncore,
+				PORT_TX_DFLEXDPSP(dig_port->tc_phy_fia));
 
 	if (val == 0xffffffff) {
 		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, nothing connected\n",
@@ -159,7 +181,8 @@ static bool icl_tc_phy_status_complete(struct intel_digital_port *dig_port)
 	struct intel_uncore *uncore = &i915->uncore;
 	u32 val;
 
-	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPPMS);
+	val = intel_uncore_read(uncore,
+				PORT_TX_DFLEXDPPMS(dig_port->tc_phy_fia));
 	if (val == 0xffffffff) {
 		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assuming not complete\n",
 			      dig_port->tc_port_name);
@@ -177,7 +200,8 @@ static bool icl_tc_phy_set_safe_mode(struct intel_digital_port *dig_port,
 	struct intel_uncore *uncore = &i915->uncore;
 	u32 val;
 
-	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
+	val = intel_uncore_read(uncore,
+				PORT_TX_DFLEXDPCSSS(dig_port->tc_phy_fia));
 	if (val == 0xffffffff) {
 		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, can't set safe-mode to %s\n",
 			      dig_port->tc_port_name,
@@ -190,7 +214,8 @@ static bool icl_tc_phy_set_safe_mode(struct intel_digital_port *dig_port,
 	if (!enable)
 		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
 
-	intel_uncore_write(uncore, PORT_TX_DFLEXDPCSSS, val);
+	intel_uncore_write(uncore,
+			   PORT_TX_DFLEXDPCSSS(dig_port->tc_phy_fia), val);
 
 	if (enable && wait_for(!icl_tc_phy_status_complete(dig_port), 10))
 		DRM_DEBUG_KMS("Port %s: PHY complete clear timed out\n",
@@ -206,7 +231,8 @@ static bool icl_tc_phy_is_in_safe_mode(struct intel_digital_port *dig_port)
 	struct intel_uncore *uncore = &i915->uncore;
 	u32 val;
 
-	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
+	val = intel_uncore_read(uncore,
+				PORT_TX_DFLEXDPCSSS(dig_port->tc_phy_fia));
 	if (val == 0xffffffff) {
 		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assume safe mode\n",
 			      dig_port->tc_port_name);
@@ -503,4 +529,5 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy)
 	mutex_init(&dig_port->tc_lock);
 	dig_port->tc_legacy_port = is_legacy;
 	dig_port->tc_link_refcount = 0;
+	dig_port->tc_phy_fia = tc_port_to_fia(i915, tc_port);
 }
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 95b9ca1fda2e..d0510022013c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2203,9 +2203,13 @@ enum i915_power_well_id {
 #define   DW6_OLDO_DYN_PWR_DOWN_EN	(1 << 28)
 
 #define FIA1_BASE			0x163000
+#define FIA2_BASE			0x16E000
+#define FIA3_BASE			0x16F000
+#define _FIA(fia)			_PICK((fia), FIA1_BASE, FIA2_BASE, FIA3_BASE)
+#define _MMIO_FIA(fia, off)		_MMIO(_FIA(fia) + (off))
 
 /* ICL PHY DFLEX registers */
-#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + 0x008C0)
+#define PORT_TX_DFLEXDPMLE1(fia)	_MMIO_FIA((fia),  0x008C0)
 #define   DFLEXDPMLE1_DPMLETC_MASK(tc_port)	(0xf << (4 * (tc_port)))
 #define   DFLEXDPMLE1_DPMLETC_ML0(tc_port)	(1 << (4 * (tc_port)))
 #define   DFLEXDPMLE1_DPMLETC_ML1_0(tc_port)	(3 << (4 * (tc_port)))
@@ -11484,17 +11488,18 @@ enum skl_power_gate {
 						_ICL_DSC1_RC_BUF_THRESH_1_UDW_PB, \
 						_ICL_DSC1_RC_BUF_THRESH_1_UDW_PC)
 
-#define PORT_TX_DFLEXDPSP			_MMIO(FIA1_BASE + 0x008A0)
+#define PORT_TX_DFLEXDPSP(fia)			_MMIO_FIA((fia), 0x008A0)
+#define   MODULAR_FIA_MASK			(1 << 4)
 #define   TC_LIVE_STATE_TBT(tc_port)		(1 << ((tc_port) * 8 + 6))
 #define   TC_LIVE_STATE_TC(tc_port)		(1 << ((tc_port) * 8 + 5))
 #define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)	((tc_port) * 8)
 #define   DP_LANE_ASSIGNMENT_MASK(tc_port)	(0xf << ((tc_port) * 8))
 #define   DP_LANE_ASSIGNMENT(tc_port, x)	((x) << ((tc_port) * 8))
 
-#define PORT_TX_DFLEXDPPMS				_MMIO(FIA1_BASE + 0x00890)
+#define PORT_TX_DFLEXDPPMS(fia)			_MMIO_FIA((fia), 0x00890)
 #define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1 << (tc_port))
 
-#define PORT_TX_DFLEXDPCSSS			_MMIO(FIA1_BASE + 0x00894)
+#define PORT_TX_DFLEXDPCSSS(fia)		_MMIO_FIA((fia), 0x00894)
 #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 << (tc_port))
 
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index ddafc819bf30..e9dc86ed517b 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -136,6 +136,7 @@ enum intel_ppgtt_type {
 	func(has_gmch); \
 	func(has_hotplug); \
 	func(has_ipc); \
+	func(has_modular_fia); \
 	func(has_overlay); \
 	func(has_psr); \
 	func(overlay_needs_physical); \
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 770f9f6aad84..e8ecbd55476e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1245,6 +1245,7 @@ struct intel_digital_port {
 	bool tc_legacy_port:1;
 	char tc_port_name[8];
 	enum tc_port_mode tc_mode;
+	enum phy_fia tc_phy_fia;
 
 	void (*write_infoframe)(struct intel_encoder *encoder,
 				const struct intel_crtc_state *crtc_state,
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* ✓ Fi.CI.BAT: success for Modular FIA (rev4)
  2019-07-11 20:58 [PATCH v3 0/1] Modular FIA Lucas De Marchi
  2019-07-11 20:58 ` [PATCH v3 1/1] drm/i915: Add modular FIA Lucas De Marchi
@ 2019-07-11 22:06 ` Patchwork
  1 sibling, 0 replies; 8+ messages in thread
From: Patchwork @ 2019-07-11 22:06 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

== Series Details ==

Series: Modular FIA (rev4)
URL   : https://patchwork.freedesktop.org/series/63175/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6465 -> Patchwork_13629
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/

Known issues
------------

  Here are the changes found in Patchwork_13629 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       [PASS][1] -> [INCOMPLETE][2] ([fdo#107718])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6465/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][3] -> [FAIL][4] ([fdo#109485])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6465/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@prime_busy@basic-wait-after-default:
    - fi-icl-u3:          [PASS][5] -> [DMESG-WARN][6] ([fdo#107724])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6465/fi-icl-u3/igt@prime_busy@basic-wait-after-default.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/fi-icl-u3/igt@prime_busy@basic-wait-after-default.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [PASS][7] -> [DMESG-WARN][8] ([fdo#106387]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6465/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * {igt@gem_ctx_switch@legacy-render}:
    - fi-icl-guc:         [INCOMPLETE][9] ([fdo#107713]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6465/fi-icl-guc/igt@gem_ctx_switch@legacy-render.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/fi-icl-guc/igt@gem_ctx_switch@legacy-render.html

  * igt@gem_exec_create@basic:
    - fi-cml-u:           [INCOMPLETE][11] ([fdo#110566]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6465/fi-cml-u/igt@gem_exec_create@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/fi-cml-u/igt@gem_exec_create@basic.html

  * igt@i915_selftest@live_contexts:
    - fi-skl-iommu:       [INCOMPLETE][13] ([fdo#111050]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6465/fi-skl-iommu/igt@i915_selftest@live_contexts.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/fi-skl-iommu/igt@i915_selftest@live_contexts.html

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-dsi:         [INCOMPLETE][15] ([fdo#107713] / [fdo#108569]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6465/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#110566]: https://bugs.freedesktop.org/show_bug.cgi?id=110566
  [fdo#111050]: https://bugs.freedesktop.org/show_bug.cgi?id=111050


Participating hosts (53 -> 46)
------------------------------

  Missing    (7): fi-kbl-soraka fi-byt-squawks fi-bsw-cyan fi-icl-y fi-bdw-samus fi-byt-clapper fi-skl-6600u 


Build changes
-------------

  * Linux: CI_DRM_6465 -> Patchwork_13629

  CI_DRM_6465: 91f0552e964f1126335af43fd4fb6716bbddc7ec @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5094: d7f140b5b02d054183a74842b4579cf7f5533927 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13629: 9570f60f2f05a77fd998f5771961f58db99fc180 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

9570f60f2f05 drm/i915: Add modular FIA

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13629/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] drm/i915: Add modular FIA
  2019-07-11 20:58 ` [PATCH v3 1/1] drm/i915: Add modular FIA Lucas De Marchi
@ 2019-07-11 23:15   ` Summers, Stuart
  2019-07-11 23:49     ` Lucas De Marchi
  0 siblings, 1 reply; 8+ messages in thread
From: Summers, Stuart @ 2019-07-11 23:15 UTC (permalink / raw)
  To: intel-gfx, De Marchi, Lucas


[-- Attachment #1.1: Type: text/plain, Size: 10524 bytes --]

On Thu, 2019-07-11 at 13:58 -0700, Lucas De Marchi wrote:
> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> 
> Some platforms may have Modular FIA. If Modular FIA is used in the
> SOC,
> then Display Driver will access the additional instances of
> FIA based on pre-assigned offset in GTTMADDR space.
> 
> Each Modular FIA instance has its own IOSF Sideband Port ID
> and it houses only 2 Type-C Port. In SOC that has more than
> two Type-C Ports, there are multiple instances of Modular FIA.
> Gunit will need to use different destination ID when it access
> different pair of Type-C Port.
> 
> The DFLEXDPSP register has Modular FIA bit starting on Tiger
> Lake.  If
> Modular FIA is used in the SOC, this register bit exists in all the
> instances of Modular FIA. IOM FW is required to program only the MF
> bit
> in first FIA instance that houses the Type-C Port 0 and Port 1, for
> Display Driver to read from.
> 
> v2 (Lucas):
>   - Move all accesses to FIA to be contained in intel_tc.c, along
> with
>     display_fia that is now called tc_phy_fia
>   - Save the fia instance number on intel_digital_port, so we don't
> have
>     to query if modular FIA is used on every access
> v3 (Lucas): Make function static
> v4 (Lucas): Move enum phy_fia to the header and use it in
>    intel_digital_port (suggested by Ville)
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.h |  6 +++
>  drivers/gpu/drm/i915/display/intel_tc.c      | 43 ++++++++++++++++
> ----
>  drivers/gpu/drm/i915/i915_reg.h              | 13 ++++--
>  drivers/gpu/drm/i915/intel_device_info.h     |  1 +
>  drivers/gpu/drm/i915/intel_drv.h             |  1 +
>  5 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> b/drivers/gpu/drm/i915/display/intel_display.h
> index 8a4a57ef82a2..8b048976f7b4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -243,6 +243,12 @@ enum phy {
>  
>  #define phy_name(a) ((a) + 'A')
>  
> +enum phy_fia {
> +	FIA1,
> +	FIA2,
> +	FIA3,
> +};
> +
>  #define for_each_pipe(__dev_priv, __p) \
>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes;
> (__p)++)
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> b/drivers/gpu/drm/i915/display/intel_tc.c
> index f44ee4bfe7c8..9400da4f7916 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -22,6 +22,24 @@ static const char *tc_port_mode_name(enum
> tc_port_mode mode)
>  	return names[mode];
>  }
>  
> +static bool has_modular_fia(struct drm_i915_private *i915)
> +{
> +	if (!INTEL_INFO(i915)->display.has_modular_fia)
> +		return false;
> +
> +	return intel_uncore_read(&i915->uncore,
> +				 PORT_TX_DFLEXDPSP(FIA1)) &
> MODULAR_FIA_MASK;
> +}
> +
> +static enum phy_fia tc_port_to_fia(struct drm_i915_private *i915,
> +				   enum tc_port tc_port)
> +{
> +	if (!has_modular_fia(i915))
> +		return FIA1;
> +
> +	return tc_port / 2;

I realize this is described in the commit message, but would be nice to
have a brief comment describing why we need this conversion.

> +}
> +
>  u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
>  {
>  	struct drm_i915_private *i915 = to_i915(dig_port-
> >base.base.dev);
> @@ -29,7 +47,8 @@ u32 intel_tc_port_get_lane_mask(struct
> intel_digital_port *dig_port)
>  	struct intel_uncore *uncore = &i915->uncore;
>  	u32 lane_mask;
>  
> -	lane_mask = intel_uncore_read(uncore, PORT_TX_DFLEXDPSP);
> +	lane_mask = intel_uncore_read(uncore,
> +				      PORT_TX_DFLEXDPSP(dig_port-
> >tc_phy_fia));
>  
>  	WARN_ON(lane_mask == 0xffffffff);
>  
> @@ -78,7 +97,8 @@ void intel_tc_port_set_fia_lane_count(struct
> intel_digital_port *dig_port,
>  
>  	WARN_ON(lane_reversal && dig_port->tc_mode != TC_PORT_LEGACY);
>  
> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPMLE1);
> +	val = intel_uncore_read(uncore,
> +				PORT_TX_DFLEXDPMLE1(dig_port-
> >tc_phy_fia));
>  	val &= ~DFLEXDPMLE1_DPMLETC_MASK(tc_port);
>  
>  	switch (required_lanes) {
> @@ -97,7 +117,8 @@ void intel_tc_port_set_fia_lane_count(struct
> intel_digital_port *dig_port,
>  		MISSING_CASE(required_lanes);
>  	}
>  
> -	intel_uncore_write(uncore, PORT_TX_DFLEXDPMLE1, val);
> +	intel_uncore_write(uncore,
> +			   PORT_TX_DFLEXDPMLE1(dig_port->tc_phy_fia),
> val);
>  }
>  
>  static void tc_port_fixup_legacy_flag(struct intel_digital_port
> *dig_port,
> @@ -129,7 +150,8 @@ static u32 tc_port_live_status_mask(struct
> intel_digital_port *dig_port)
>  	u32 mask = 0;
>  	u32 val;
>  
> -	val = intel_uncore_rea
> d(uncore, PORT_TX_DFLEXDPSP);
> +	val = intel_uncore_read(uncore,
> +				PORT_TX_DFLEXDPSP(dig_port-
> >tc_phy_fia));
>  
>  	if (val == 0xffffffff) {
>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, nothing
> connected\n",
> @@ -159,7 +181,8 @@ static bool icl_tc_phy_status_complete(struct
> intel_digital_port *dig_port)
>  	struct intel_uncore *uncore = &i915->uncore;
>  	u32 val;
>  
> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPPMS);
> +	val = intel_uncore_read(uncore,
> +				PORT_TX_DFLEXDPPMS(dig_port-
> >tc_phy_fia));
>  	if (val == 0xffffffff) {
>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assuming not
> complete\n",
>  			      dig_port->tc_port_name);
> @@ -177,7 +200,8 @@ static bool icl_tc_phy_set_safe_mode(struct
> intel_digital_port *dig_port,
>  	struct intel_uncore *uncore = &i915->uncore;
>  	u32 val;
>  
> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
> +	val = intel_uncore_read(uncore,
> +				PORT_TX_DFLEXDPCSSS(dig_port-
> >tc_phy_fia));
>  	if (val == 0xffffffff) {
>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, can't set safe-
> mode to %s\n",
>  			      dig_port->tc_port_name,
> @@ -190,7 +214,8 @@ static bool icl_tc_phy_set_safe_mode(struct
> intel_digital_port *dig_port,
>  	if (!enable)
>  		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>  
> -	intel_uncore_write(uncore, PORT_TX_DFLEXDPCSSS, val);
> +	intel_uncore_write(uncore,
> +			   PORT_TX_DFLEXDPCSSS(dig_port->tc_phy_fia),
> val);
>  
>  	if (enable && wait_for(!icl_tc_phy_status_complete(dig_port),
> 10))
>  		DRM_DEBUG_KMS("Port %s: PHY complete clear timed
> out\n",
> @@ -206,7 +231,8 @@ static bool icl_tc_phy_is_in_safe_mode(struct
> intel_digital_port *dig_port)
>  	struct intel_uncore *uncore = &i915->uncore;
>  	u32 val;
>  
> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
> +	val = intel_uncore_read(uncore,
> +				PORT_TX_DFLEXDPCSSS(dig_port-
> >tc_phy_fia));
>  	if (val == 0xffffffff) {
>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assume safe
> mode\n",
>  			      dig_port->tc_port_name);
> @@ -503,4 +529,5 @@ void intel_tc_port_init(struct intel_digital_port
> *dig_port, bool is_legacy)
>  	mutex_init(&dig_port->tc_lock);
>  	dig_port->tc_legacy_port = is_legacy;
>  	dig_port->tc_link_refcount = 0;
> +	dig_port->tc_phy_fia = tc_port_to_fia(i915, tc_port);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 95b9ca1fda2e..d0510022013c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2203,9 +2203,13 @@ enum i915_power_well_id {
>  #define   DW6_OLDO_DYN_PWR_DOWN_EN	(1 << 28)
>  
>  #define FIA1_BASE			0x163000
> +#define FIA2_BASE			0x16E000
> +#define FIA3_BASE			0x16F000
> +#define _FIA(fia)			_PICK((fia), FIA1_BASE,
> FIA2_BASE, FIA3_BASE)
> +#define _MMIO_FIA(fia, off)		_MMIO(_FIA(fia) + (off))
>  
>  /* ICL PHY DFLEX registers */
> -#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + 0x008C0)
> +#define PORT_TX_DFLEXDPMLE1(fia)	_MMIO_FIA((fia),  0x008C0)
>  #define   DFLEXDPMLE1_DPMLETC_MASK(tc_port)	(0xf << (4 *
> (tc_port)))
>  #define   DFLEXDPMLE1_DPMLETC_ML0(tc_port)	(1 << (4 * (tc_port)))
>  #define   DFLEXDPMLE1_DPMLETC_ML1_0(tc_port)	(3 << (4 * (tc_port)))
> @@ -11484,17 +11488,18 @@ enum skl_power_gate {
>  						_ICL_DSC1_RC_BUF_THRESH
> _1_UDW_PB, \
>  						_ICL_DSC1_RC_BUF_THRESH
> _1_UDW_PC)
>  
> -#define PORT_TX_DFLEXDPSP			_MMIO(FIA1_BASE +
> 0x008A0)
> +#define PORT_TX_DFLEXDPSP(fia)			_MMIO_FIA((fia)
> , 0x008A0)
> +#define   MODULAR_FIA_MASK			(1 << 4)
>  #define   TC_LIVE_STATE_TBT(tc_port)		(1 << ((tc_port) * 8 +
> 6))
>  #define   TC_LIVE_STATE_TC(tc_port)		(1 << ((tc_port) * 8 +
> 5))
>  #define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)	((tc_port) * 8)
>  #define   DP_LANE_ASSIGNMENT_MASK(tc_port)	(0xf << ((tc_port) *
> 8))
>  #define   DP_LANE_ASSIGNMENT(tc_port, x)	((x) << ((tc_port) *
> 8))
>  
> -#define PORT_TX_DFLEXDPPMS				_MMIO(FIA1_BASE
> + 0x00890)
> +#define PORT_TX_DFLEXDPPMS(fia)			_MMIO_FIA((fia)
> , 0x00890)
>  #define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1 <<
> (tc_port))
>  
> -#define PORT_TX_DFLEXDPCSSS			_MMIO(FIA1_BASE +
> 0x00894)
> +#define PORT_TX_DFLEXDPCSSS(fia)		_MMIO_FIA((fia),
> 0x00894)
>  #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 <<
> (tc_port))
>  
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> b/drivers/gpu/drm/i915/intel_device_info.h
> index ddafc819bf30..e9dc86ed517b 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -136,6 +136,7 @@ enum intel_ppgtt_type {
>  	func(has_gmch); \
>  	func(has_hotplug); \
>  	func(has_ipc); \
> +	func(has_modular_fia); \

If we have a register to tell us whether the platform supports this,
why do we need this feature flag?

Thanks,
Stuart

>  	func(has_overlay); \
>  	func(has_psr); \
>  	func(overlay_needs_physical); \
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 770f9f6aad84..e8ecbd55476e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1245,6 +1245,7 @@ struct intel_digital_port {
>  	bool tc_legacy_port:1;
>  	char tc_port_name[8];
>  	enum tc_port_mode tc_mode;
> +	enum phy_fia tc_phy_fia;
>  
>  	void (*write_infoframe)(struct intel_encoder *encoder,
>  				const struct intel_crtc_state
> *crtc_state,

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] drm/i915: Add modular FIA
  2019-07-11 23:15   ` Summers, Stuart
@ 2019-07-11 23:49     ` Lucas De Marchi
  2019-07-12  0:06       ` Summers, Stuart
  2019-07-12 12:55       ` Ville Syrjälä
  0 siblings, 2 replies; 8+ messages in thread
From: Lucas De Marchi @ 2019-07-11 23:49 UTC (permalink / raw)
  To: Summers, Stuart; +Cc: intel-gfx

On Thu, Jul 11, 2019 at 04:15:42PM -0700, Summers, Stuart wrote:
>On Thu, 2019-07-11 at 13:58 -0700, Lucas De Marchi wrote:
>> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>
>> Some platforms may have Modular FIA. If Modular FIA is used in the
>> SOC,
>> then Display Driver will access the additional instances of
>> FIA based on pre-assigned offset in GTTMADDR space.
>>
>> Each Modular FIA instance has its own IOSF Sideband Port ID
>> and it houses only 2 Type-C Port. In SOC that has more than
>> two Type-C Ports, there are multiple instances of Modular FIA.
>> Gunit will need to use different destination ID when it access
>> different pair of Type-C Port.
>>
>> The DFLEXDPSP register has Modular FIA bit starting on Tiger
>> Lake.  If
>> Modular FIA is used in the SOC, this register bit exists in all the
>> instances of Modular FIA. IOM FW is required to program only the MF
>> bit
>> in first FIA instance that houses the Type-C Port 0 and Port 1, for
>> Display Driver to read from.
>>
>> v2 (Lucas):
>>   - Move all accesses to FIA to be contained in intel_tc.c, along
>> with
>>     display_fia that is now called tc_phy_fia
>>   - Save the fia instance number on intel_digital_port, so we don't
>> have
>>     to query if modular FIA is used on every access
>> v3 (Lucas): Make function static
>> v4 (Lucas): Move enum phy_fia to the header and use it in
>>    intel_digital_port (suggested by Ville)
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.h |  6 +++
>>  drivers/gpu/drm/i915/display/intel_tc.c      | 43 ++++++++++++++++
>> ----
>>  drivers/gpu/drm/i915/i915_reg.h              | 13 ++++--
>>  drivers/gpu/drm/i915/intel_device_info.h     |  1 +
>>  drivers/gpu/drm/i915/intel_drv.h             |  1 +
>>  5 files changed, 52 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
>> b/drivers/gpu/drm/i915/display/intel_display.h
>> index 8a4a57ef82a2..8b048976f7b4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>> @@ -243,6 +243,12 @@ enum phy {
>>
>>  #define phy_name(a) ((a) + 'A')
>>
>> +enum phy_fia {
>> +	FIA1,
>> +	FIA2,
>> +	FIA3,
>> +};
>> +
>>  #define for_each_pipe(__dev_priv, __p) \
>>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes;
>> (__p)++)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
>> b/drivers/gpu/drm/i915/display/intel_tc.c
>> index f44ee4bfe7c8..9400da4f7916 100644
>> --- a/drivers/gpu/drm/i915/display/intel_tc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
>> @@ -22,6 +22,24 @@ static const char *tc_port_mode_name(enum
>> tc_port_mode mode)
>>  	return names[mode];
>>  }
>>
>> +static bool has_modular_fia(struct drm_i915_private *i915)
>> +{
>> +	if (!INTEL_INFO(i915)->display.has_modular_fia)
>> +		return false;
>> +
>> +	return intel_uncore_read(&i915->uncore,
>> +				 PORT_TX_DFLEXDPSP(FIA1)) &
>> MODULAR_FIA_MASK;
>> +}
>> +
>> +static enum phy_fia tc_port_to_fia(struct drm_i915_private *i915,
>> +				   enum tc_port tc_port)
>> +{
>> +	if (!has_modular_fia(i915))
>> +		return FIA1;
>> +
>> +	return tc_port / 2;
>
>I realize this is described in the commit message, but would be nice to
>have a brief comment describing why we need this conversion.
>
>> +}
>> +
>>  u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
>>  {
>>  	struct drm_i915_private *i915 = to_i915(dig_port-
>> >base.base.dev);
>> @@ -29,7 +47,8 @@ u32 intel_tc_port_get_lane_mask(struct
>> intel_digital_port *dig_port)
>>  	struct intel_uncore *uncore = &i915->uncore;
>>  	u32 lane_mask;
>>
>> -	lane_mask = intel_uncore_read(uncore, PORT_TX_DFLEXDPSP);
>> +	lane_mask = intel_uncore_read(uncore,
>> +				      PORT_TX_DFLEXDPSP(dig_port-
>> >tc_phy_fia));
>>
>>  	WARN_ON(lane_mask == 0xffffffff);
>>
>> @@ -78,7 +97,8 @@ void intel_tc_port_set_fia_lane_count(struct
>> intel_digital_port *dig_port,
>>
>>  	WARN_ON(lane_reversal && dig_port->tc_mode != TC_PORT_LEGACY);
>>
>> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPMLE1);
>> +	val = intel_uncore_read(uncore,
>> +				PORT_TX_DFLEXDPMLE1(dig_port-
>> >tc_phy_fia));
>>  	val &= ~DFLEXDPMLE1_DPMLETC_MASK(tc_port);
>>
>>  	switch (required_lanes) {
>> @@ -97,7 +117,8 @@ void intel_tc_port_set_fia_lane_count(struct
>> intel_digital_port *dig_port,
>>  		MISSING_CASE(required_lanes);
>>  	}
>>
>> -	intel_uncore_write(uncore, PORT_TX_DFLEXDPMLE1, val);
>> +	intel_uncore_write(uncore,
>> +			   PORT_TX_DFLEXDPMLE1(dig_port->tc_phy_fia),
>> val);
>>  }
>>
>>  static void tc_port_fixup_legacy_flag(struct intel_digital_port
>> *dig_port,
>> @@ -129,7 +150,8 @@ static u32 tc_port_live_status_mask(struct
>> intel_digital_port *dig_port)
>>  	u32 mask = 0;
>>  	u32 val;
>>
>> -	val = intel_uncore_rea
>> d(uncore, PORT_TX_DFLEXDPSP);
>> +	val = intel_uncore_read(uncore,
>> +				PORT_TX_DFLEXDPSP(dig_port-
>> >tc_phy_fia));
>>
>>  	if (val == 0xffffffff) {
>>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, nothing
>> connected\n",
>> @@ -159,7 +181,8 @@ static bool icl_tc_phy_status_complete(struct
>> intel_digital_port *dig_port)
>>  	struct intel_uncore *uncore = &i915->uncore;
>>  	u32 val;
>>
>> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPPMS);
>> +	val = intel_uncore_read(uncore,
>> +				PORT_TX_DFLEXDPPMS(dig_port-
>> >tc_phy_fia));
>>  	if (val == 0xffffffff) {
>>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assuming not
>> complete\n",
>>  			      dig_port->tc_port_name);
>> @@ -177,7 +200,8 @@ static bool icl_tc_phy_set_safe_mode(struct
>> intel_digital_port *dig_port,
>>  	struct intel_uncore *uncore = &i915->uncore;
>>  	u32 val;
>>
>> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
>> +	val = intel_uncore_read(uncore,
>> +				PORT_TX_DFLEXDPCSSS(dig_port-
>> >tc_phy_fia));
>>  	if (val == 0xffffffff) {
>>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, can't set safe-
>> mode to %s\n",
>>  			      dig_port->tc_port_name,
>> @@ -190,7 +214,8 @@ static bool icl_tc_phy_set_safe_mode(struct
>> intel_digital_port *dig_port,
>>  	if (!enable)
>>  		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>>
>> -	intel_uncore_write(uncore, PORT_TX_DFLEXDPCSSS, val);
>> +	intel_uncore_write(uncore,
>> +			   PORT_TX_DFLEXDPCSSS(dig_port->tc_phy_fia),
>> val);
>>
>>  	if (enable && wait_for(!icl_tc_phy_status_complete(dig_port),
>> 10))
>>  		DRM_DEBUG_KMS("Port %s: PHY complete clear timed
>> out\n",
>> @@ -206,7 +231,8 @@ static bool icl_tc_phy_is_in_safe_mode(struct
>> intel_digital_port *dig_port)
>>  	struct intel_uncore *uncore = &i915->uncore;
>>  	u32 val;
>>
>> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
>> +	val = intel_uncore_read(uncore,
>> +				PORT_TX_DFLEXDPCSSS(dig_port-
>> >tc_phy_fia));
>>  	if (val == 0xffffffff) {
>>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assume safe
>> mode\n",
>>  			      dig_port->tc_port_name);
>> @@ -503,4 +529,5 @@ void intel_tc_port_init(struct intel_digital_port
>> *dig_port, bool is_legacy)
>>  	mutex_init(&dig_port->tc_lock);
>>  	dig_port->tc_legacy_port = is_legacy;
>>  	dig_port->tc_link_refcount = 0;
>> +	dig_port->tc_phy_fia = tc_port_to_fia(i915, tc_port);
>>  }
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 95b9ca1fda2e..d0510022013c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2203,9 +2203,13 @@ enum i915_power_well_id {
>>  #define   DW6_OLDO_DYN_PWR_DOWN_EN	(1 << 28)
>>
>>  #define FIA1_BASE			0x163000
>> +#define FIA2_BASE			0x16E000
>> +#define FIA3_BASE			0x16F000
>> +#define _FIA(fia)			_PICK((fia), FIA1_BASE,
>> FIA2_BASE, FIA3_BASE)
>> +#define _MMIO_FIA(fia, off)		_MMIO(_FIA(fia) + (off))
>>
>>  /* ICL PHY DFLEX registers */
>> -#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + 0x008C0)
>> +#define PORT_TX_DFLEXDPMLE1(fia)	_MMIO_FIA((fia),  0x008C0)
>>  #define   DFLEXDPMLE1_DPMLETC_MASK(tc_port)	(0xf << (4 *
>> (tc_port)))
>>  #define   DFLEXDPMLE1_DPMLETC_ML0(tc_port)	(1 << (4 * (tc_port)))
>>  #define   DFLEXDPMLE1_DPMLETC_ML1_0(tc_port)	(3 << (4 * (tc_port)))
>> @@ -11484,17 +11488,18 @@ enum skl_power_gate {
>>  						_ICL_DSC1_RC_BUF_THRESH
>> _1_UDW_PB, \
>>  						_ICL_DSC1_RC_BUF_THRESH
>> _1_UDW_PC)
>>
>> -#define PORT_TX_DFLEXDPSP			_MMIO(FIA1_BASE +
>> 0x008A0)
>> +#define PORT_TX_DFLEXDPSP(fia)			_MMIO_FIA((fia)
>> , 0x008A0)
>> +#define   MODULAR_FIA_MASK			(1 << 4)
>>  #define   TC_LIVE_STATE_TBT(tc_port)		(1 << ((tc_port) * 8 +
>> 6))
>>  #define   TC_LIVE_STATE_TC(tc_port)		(1 << ((tc_port) * 8 +
>> 5))
>>  #define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)	((tc_port) * 8)
>>  #define   DP_LANE_ASSIGNMENT_MASK(tc_port)	(0xf << ((tc_port) *
>> 8))
>>  #define   DP_LANE_ASSIGNMENT(tc_port, x)	((x) << ((tc_port) *
>> 8))
>>
>> -#define PORT_TX_DFLEXDPPMS				_MMIO(FIA1_BASE
>> + 0x00890)
>> +#define PORT_TX_DFLEXDPPMS(fia)			_MMIO_FIA((fia)
>> , 0x00890)
>>  #define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1 <<
>> (tc_port))
>>
>> -#define PORT_TX_DFLEXDPCSSS			_MMIO(FIA1_BASE +
>> 0x00894)
>> +#define PORT_TX_DFLEXDPCSSS(fia)		_MMIO_FIA((fia),
>> 0x00894)
>>  #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 <<
>> (tc_port))
>>
>>  #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h
>> b/drivers/gpu/drm/i915/intel_device_info.h
>> index ddafc819bf30..e9dc86ed517b 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.h
>> +++ b/drivers/gpu/drm/i915/intel_device_info.h
>> @@ -136,6 +136,7 @@ enum intel_ppgtt_type {
>>  	func(has_gmch); \
>>  	func(has_hotplug); \
>>  	func(has_ipc); \
>> +	func(has_modular_fia); \
>
>If we have a register to tell us whether the platform supports this,
>why do we need this feature flag?

because the platform may not have the register bit that tells if the we
have modular FIA, as is the case with Ice Lake. The bit is reserved
there. This is actually "may_have_modular_fia" rather than
"has_modular_fia", but the name is pretty bad to make it that way.

Lucas De Marchi

>
>Thanks,
>Stuart
>
>>  	func(has_overlay); \
>>  	func(has_psr); \
>>  	func(overlay_needs_physical); \
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 770f9f6aad84..e8ecbd55476e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1245,6 +1245,7 @@ struct intel_digital_port {
>>  	bool tc_legacy_port:1;
>>  	char tc_port_name[8];
>>  	enum tc_port_mode tc_mode;
>> +	enum phy_fia tc_phy_fia;
>>
>>  	void (*write_infoframe)(struct intel_encoder *encoder,
>>  				const struct intel_crtc_state
>> *crtc_state,


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] drm/i915: Add modular FIA
  2019-07-11 23:49     ` Lucas De Marchi
@ 2019-07-12  0:06       ` Summers, Stuart
  2019-07-12 12:55       ` Ville Syrjälä
  1 sibling, 0 replies; 8+ messages in thread
From: Summers, Stuart @ 2019-07-12  0:06 UTC (permalink / raw)
  To: De Marchi, Lucas; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 12548 bytes --]

On Thu, 2019-07-11 at 16:49 -0700, Lucas De Marchi wrote:
> On Thu, Jul 11, 2019 at 04:15:42PM -0700, Summers, Stuart wrote:
> > On Thu, 2019-07-11 at 13:58 -0700, Lucas De Marchi wrote:
> > > From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > 
> > > Some platforms may have Modular FIA. If Modular FIA is used in
> > > the
> > > SOC,
> > > then Display Driver will access the additional instances of
> > > FIA based on pre-assigned offset in GTTMADDR space.
> > > 
> > > Each Modular FIA instance has its own IOSF Sideband Port ID
> > > and it houses only 2 Type-C Port. In SOC that has more than
> > > two Type-C Ports, there are multiple instances of Modular FIA.
> > > Gunit will need to use different destination ID when it access
> > > different pair of Type-C Port.
> > > 
> > > The DFLEXDPSP register has Modular FIA bit starting on Tiger
> > > Lake.  If
> > > Modular FIA is used in the SOC, this register bit exists in all
> > > the
> > > instances of Modular FIA. IOM FW is required to program only the
> > > MF
> > > bit
> > > in first FIA instance that houses the Type-C Port 0 and Port 1,
> > > for
> > > Display Driver to read from.
> > > 
> > > v2 (Lucas):
> > >   - Move all accesses to FIA to be contained in intel_tc.c, along
> > > with
> > >     display_fia that is now called tc_phy_fia
> > >   - Save the fia instance number on intel_digital_port, so we
> > > don't
> > > have
> > >     to query if modular FIA is used on every access
> > > v3 (Lucas): Make function static
> > > v4 (Lucas): Move enum phy_fia to the header and use it in
> > >    intel_digital_port (suggested by Ville)
> > > 
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.h |  6 +++
> > >  drivers/gpu/drm/i915/display/intel_tc.c      | 43
> > > ++++++++++++++++
> > > ----
> > >  drivers/gpu/drm/i915/i915_reg.h              | 13 ++++--
> > >  drivers/gpu/drm/i915/intel_device_info.h     |  1 +
> > >  drivers/gpu/drm/i915/intel_drv.h             |  1 +
> > >  5 files changed, 52 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> > > b/drivers/gpu/drm/i915/display/intel_display.h
> > > index 8a4a57ef82a2..8b048976f7b4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -243,6 +243,12 @@ enum phy {
> > > 
> > >  #define phy_name(a) ((a) + 'A')
> > > 
> > > +enum phy_fia {
> > > +	FIA1,
> > > +	FIA2,
> > > +	FIA3,
> > > +};
> > > +
> > >  #define for_each_pipe(__dev_priv, __p) \
> > >  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes;
> > > (__p)++)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> > > b/drivers/gpu/drm/i915/display/intel_tc.c
> > > index f44ee4bfe7c8..9400da4f7916 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > > @@ -22,6 +22,24 @@ static const char *tc_port_mode_name(enum
> > > tc_port_mode mode)
> > >  	return names[mode];
> > >  }
> > > 
> > > +static bool has_modular_fia(struct drm_i915_private *i915)
> > > +{
> > > +	if (!INTEL_INFO(i915)->display.has_modular_fia)
> > > +		return false;
> > > +
> > > +	return intel_uncore_read(&i915->uncore,
> > > +				 PORT_TX_DFLEXDPSP(FIA1)) &
> > > MODULAR_FIA_MASK;
> > > +}
> > > +
> > > +static enum phy_fia tc_port_to_fia(struct drm_i915_private
> > > *i915,
> > > +				   enum tc_port tc_port)
> > > +{
> > > +	if (!has_modular_fia(i915))
> > > +		return FIA1;
> > > +
> > > +	return tc_port / 2;
> > 
> > I realize this is described in the commit message, but would be
> > nice to
> > have a brief comment describing why we need this conversion.
> > 
> > > +}
> > > +
> > >  u32 intel_tc_port_get_lane_mask(struct intel_digital_port
> > > *dig_port)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(dig_port-
> > > > base.base.dev);
> > > 
> > > @@ -29,7 +47,8 @@ u32 intel_tc_port_get_lane_mask(struct
> > > intel_digital_port *dig_port)
> > >  	struct intel_uncore *uncore = &i915->uncore;
> > >  	u32 lane_mask;
> > > 
> > > -	lane_mask = intel_uncore_read(uncore, PORT_TX_DFLEXDPSP);
> > > +	lane_mask = intel_uncore_read(uncore,
> > > +				      PORT_TX_DFLEXDPSP(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	WARN_ON(lane_mask == 0xffffffff);
> > > 
> > > @@ -78,7 +97,8 @@ void intel_tc_port_set_fia_lane_count(struct
> > > intel_digital_port *dig_port,
> > > 
> > >  	WARN_ON(lane_reversal && dig_port->tc_mode != TC_PORT_LEGACY);
> > > 
> > > -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPMLE1);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPMLE1(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	val &= ~DFLEXDPMLE1_DPMLETC_MASK(tc_port);
> > > 
> > >  	switch (required_lanes) {
> > > @@ -97,7 +117,8 @@ void intel_tc_port_set_fia_lane_count(struct
> > > intel_digital_port *dig_port,
> > >  		MISSING_CASE(required_lanes);
> > >  	}
> > > 
> > > -	intel_uncore_write(uncore, PORT_TX_DFLEXDPMLE1, val);
> > > +	intel_uncore_write(uncore,
> > > +			   PORT_TX_DFLEXDPMLE1(dig_port->tc_phy_fia),
> > > val);
> > >  }
> > > 
> > >  static void tc_port_fixup_legacy_flag(struct intel_digital_port
> > > *dig_port,
> > > @@ -129,7 +150,8 @@ static u32 tc_port_live_status_mask(struct
> > > intel_digital_port *dig_port)
> > >  	u32 mask = 0;
> > >  	u32 val;
> > > 
> > > -	val = intel_uncore_rea
> > > d(uncore, PORT_TX_DFLEXDPSP);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPSP(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	if (val == 0xffffffff) {
> > >  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, nothingo make it
> > > t
> > > connected\n",
> > > @@ -159,7 +181,8 @@ static bool icl_tc_phy_status_complete(struct
> > > intel_digital_port *dig_port)
> > >  	struct intel_uncore *uncore = &i915->uncore;
> > >  	u32 val;
> > > 
> > > -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPPMS);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPPMS(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	if (val == 0xffffffff) {
> > >  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assuming not
> > > complete\n",
> > >  			      dig_port->tc_port_name);
> > > @@ -177,7 +200,8 @@ static bool icl_tc_phy_set_safe_mode(struct
> > > intel_digital_port *dig_port,
> > >  	struct intel_uncore *uncore = &i915->uncore;
> > >  	u32 val;
> > > 
> > > -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPCSSS(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	if (val == 0xffffffff) {
> > >  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, can't set safe-
> > > mode to %s\n",
> > >  			      dig_port->tc_port_name,
> > > @@ -190,7 +214,8 @@ static bool icl_tc_phy_set_safe_mode(struct
> > > intel_digital_port *dig_port,
> > >  	if (!enable)
> > >  		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > > 
> > > -	intel_uncore_write(uncore, PORT_TX_DFLEXDPCSSS, val);
> > > +	intel_uncore_write(uncore,
> > > +			   PORT_TX_DFLEXDPCSSS(dig_port->tc_phy_fia),
> > > val);
> > > 
> > >  	if (enable && wait_for(!icl_tc_phy_status_complete(dig_port),
> > > 10))
> > >  		DRM_DEBUG_KMS("Port %s: PHY complete clear timed
> > > out\n",
> > > @@ -206,7 +231,8 @@ static bool icl_tc_phy_is_in_safe_mode(struct
> > > intel_digital_port *dig_port)
> > >  	struct intel_uncore *uncore = &i915->uncore;
> > >  	u32 val;
> > > 
> > > -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
> > > +	val = intel_uncore_read(uncore,
> > > +				PORT_TX_DFLEXDPCSSS(dig_port-
> > > > tc_phy_fia));
> > > 
> > >  	if (val == 0xffffffff) {
> > >  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assume safe
> > > mode\n",
> > >  			      dig_port->tc_port_name);
> > > @@ -503,4 +529,5 @@ void intel_tc_port_init(struct
> > > intel_digital_port
> > > *dig_port, bool is_legacy)
> > >  	mutex_init(&dig_port->tc_lock);
> > >  	dig_port->tc_legacy_port = is_legacy;
> > >  	dig_port->tc_link_refcount = 0;
> > > +	dig_port->tc_phy_fia = tc_port_to_fia(i915, tc_port);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 95b9ca1fda2e..d0510022013c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2203,9 +2203,13 @@ enum i915_power_well_id {
> > >  #define   DW6_OLDO_DYN_PWR_DOWN_EN	(1 << 28)
> > > 
> > >  #define FIA1_BASE			0x163000
> > > +#define FIA2_BASE			0x16E000
> > > +#define FIA3_BASE			0x16F000
> > > +#define _FIA(fia)			_PICK((fia), FIA1_BASE,
> > > FIA2_BASE, FIA3_BASE)
> > > +#define _MMIO_FIA(fia, off)		_MMIO(_FIA(fia) +
> > > (off))
> > > 
> > >  /* ICL PHY DFLEX registers */
> > > -#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE +
> > > 0x008C0)
> > > +#define PORT_TX_DFLEXDPMLE1(fia)	_MMIO_FIA((fia),  0x008C0)
> > >  #define   DFLEXDPMLE1_DPMLETC_MASK(tc_port)	(0xf << (4 *
> > > (tc_port)))
> > >  #define   DFLEXDPMLE1_DPMLETC_ML0(tc_port)	(1 << (4 *
> > > (tc_port)))
> > >  #define   DFLEXDPMLE1_DPMLETC_ML1_0(tc_port)	(3 << (4 *
> > > (tc_port)))
> > > @@ -11484,17 +11488,18 @@ enum skl_power_gate {
> > >  						_ICL_DSC1_RC_BUF_THRESH
> > > _1_UDW_PB, \
> > >  						_ICL_DSC1_RC_BUF_THRESH
> > > _1_UDW_PC)
> > > 
> > > -#define PORT_TX_DFLEXDPSP			_MMIO(FIA1_BASE
> > > +
> > > 0x008A0)
> > > +#define PORT_TX_DFLEXDPSP(fia)			_MMIO_FIA((fia)
> > > , 0x008A0)
> > > +#define   MODULAR_FIA_MASK			(1 << 4)
> > >  #define   TC_LIVE_STATE_TBT(tc_port)		(1 <<
> > > ((tc_port) * 8 +
> > > 6))
> > >  #define   TC_LIVE_STATE_TC(tc_port)		(1 <<
> > > ((tc_port) * 8 +
> > > 5))
> > >  #define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)	((tc_port) * 8)
> > >  #define   DP_LANE_ASSIGNMENT_MASK(tc_port)	(0xf <<
> > > ((tc_port) *
> > > 8))
> > >  #define   DP_LANE_ASSIGNMENT(tc_port, x)	((x) << ((tc_port) *
> > > 8))
> > > 
> > > -#define PORT_TX_DFLEXDPPMS				_MMIO(F
> > > IA1_BASE
> > > + 0x00890)
> > > +#define PORT_TX_DFLEXDPPMS(fia)			_MMIO_FIA((fia)
> > > , 0x00890)
> > >  #define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1 <<
> > > (tc_port))
> > > 
> > > -#define PORT_TX_DFLEXDPCSSS			_MMIO(FIA1_BASE
> > > +
> > > 0x00894)
> > > +#define PORT_TX_DFLEXDPCSSS(fia)		_MMIO_FIA((fia),
> > > 0x00894)
> > >  #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 <<
> > > (tc_port))
> > > 
> > >  #endif /* _I915_REG_H_ */
> > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> > > b/drivers/gpu/drm/i915/intel_device_info.h
> > > index ddafc819bf30..e9dc86ed517b 100644
> > > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > > @@ -136,6 +136,7 @@ enum intel_ppgtt_type {
> > >  	func(has_gmch); \
> > >  	func(has_hotplug); \
> > >  	func(has_ipc); \
> > > +	func(has_modular_fia); \
> > 
> > If we have a register to tell us whether the platform supports
> > this,
> > why do we need this feature flag?
> 
> because the platform may not have the register bit that tells if the
> we
> have modular FIA, as is the case with Ice Lake. The bit is reserved
> there. This is actually "may_have_modular_fia" rather than
> "has_modular_fia", but the name is pretty bad to make it that way.

Ah, right, makes sense.

I would still like a comment above if possible, but not a blocker:
Reviewed-by: Stuart Summers <stuart.summers@intel.com>

Thanks,
Stuart

> 
> Lucas De Marchi
> 
> > 
> > Thanks,
> > Stuart
> > 
> > >  	func(has_overlay); \
> > >  	func(has_psr); \
> > >  	func(overlay_needs_physical); \
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 770f9f6aad84..e8ecbd55476e 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1245,6 +1245,7 @@ struct intel_digital_port {
> > >  	bool tc_legacy_port:1;
> > >  	char tc_port_name[8];
> > >  	enum tc_port_mode tc_mode;
> > > +	enum phy_fia tc_phy_fia;
> > > 
> > >  	void (*write_infoframe)(struct intel_encoder *encoder,
> > >  				const struct intel_crtc_state
> > > *crtc_state,
> 
> 

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] drm/i915: Add modular FIA
  2019-07-11 23:49     ` Lucas De Marchi
  2019-07-12  0:06       ` Summers, Stuart
@ 2019-07-12 12:55       ` Ville Syrjälä
  2019-07-12 15:27         ` Lucas De Marchi
  1 sibling, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2019-07-12 12:55 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

On Thu, Jul 11, 2019 at 04:49:07PM -0700, Lucas De Marchi wrote:
> On Thu, Jul 11, 2019 at 04:15:42PM -0700, Summers, Stuart wrote:
> >On Thu, 2019-07-11 at 13:58 -0700, Lucas De Marchi wrote:
> >> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >>
> >> Some platforms may have Modular FIA. If Modular FIA is used in the
> >> SOC,
> >> then Display Driver will access the additional instances of
> >> FIA based on pre-assigned offset in GTTMADDR space.
> >>
> >> Each Modular FIA instance has its own IOSF Sideband Port ID
> >> and it houses only 2 Type-C Port. In SOC that has more than
> >> two Type-C Ports, there are multiple instances of Modular FIA.
> >> Gunit will need to use different destination ID when it access
> >> different pair of Type-C Port.
> >>
> >> The DFLEXDPSP register has Modular FIA bit starting on Tiger
> >> Lake.  If
> >> Modular FIA is used in the SOC, this register bit exists in all the
> >> instances of Modular FIA. IOM FW is required to program only the MF
> >> bit
> >> in first FIA instance that houses the Type-C Port 0 and Port 1, for
> >> Display Driver to read from.
> >>
> >> v2 (Lucas):
> >>   - Move all accesses to FIA to be contained in intel_tc.c, along
> >> with
> >>     display_fia that is now called tc_phy_fia
> >>   - Save the fia instance number on intel_digital_port, so we don't
> >> have
> >>     to query if modular FIA is used on every access
> >> v3 (Lucas): Make function static
> >> v4 (Lucas): Move enum phy_fia to the header and use it in
> >>    intel_digital_port (suggested by Ville)
> >>
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_display.h |  6 +++
> >>  drivers/gpu/drm/i915/display/intel_tc.c      | 43 ++++++++++++++++
> >> ----
> >>  drivers/gpu/drm/i915/i915_reg.h              | 13 ++++--
> >>  drivers/gpu/drm/i915/intel_device_info.h     |  1 +
> >>  drivers/gpu/drm/i915/intel_drv.h             |  1 +
> >>  5 files changed, 52 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> >> b/drivers/gpu/drm/i915/display/intel_display.h
> >> index 8a4a57ef82a2..8b048976f7b4 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> >> @@ -243,6 +243,12 @@ enum phy {
> >>
> >>  #define phy_name(a) ((a) + 'A')
> >>
> >> +enum phy_fia {
> >> +	FIA1,
> >> +	FIA2,
> >> +	FIA3,
> >> +};
> >> +
> >>  #define for_each_pipe(__dev_priv, __p) \
> >>  	for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes;
> >> (__p)++)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> >> b/drivers/gpu/drm/i915/display/intel_tc.c
> >> index f44ee4bfe7c8..9400da4f7916 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> >> @@ -22,6 +22,24 @@ static const char *tc_port_mode_name(enum
> >> tc_port_mode mode)
> >>  	return names[mode];
> >>  }
> >>
> >> +static bool has_modular_fia(struct drm_i915_private *i915)
> >> +{
> >> +	if (!INTEL_INFO(i915)->display.has_modular_fia)
> >> +		return false;
> >> +
> >> +	return intel_uncore_read(&i915->uncore,
> >> +				 PORT_TX_DFLEXDPSP(FIA1)) &
> >> MODULAR_FIA_MASK;
> >> +}
> >> +
> >> +static enum phy_fia tc_port_to_fia(struct drm_i915_private *i915,
> >> +				   enum tc_port tc_port)
> >> +{
> >> +	if (!has_modular_fia(i915))
> >> +		return FIA1;
> >> +
> >> +	return tc_port / 2;
> >
> >I realize this is described in the commit message, but would be nice to
> >have a brief comment describing why we need this conversion.
> >
> >> +}
> >> +
> >>  u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port)
> >>  {
> >>  	struct drm_i915_private *i915 = to_i915(dig_port-
> >> >base.base.dev);
> >> @@ -29,7 +47,8 @@ u32 intel_tc_port_get_lane_mask(struct
> >> intel_digital_port *dig_port)
> >>  	struct intel_uncore *uncore = &i915->uncore;
> >>  	u32 lane_mask;
> >>
> >> -	lane_mask = intel_uncore_read(uncore, PORT_TX_DFLEXDPSP);
> >> +	lane_mask = intel_uncore_read(uncore,
> >> +				      PORT_TX_DFLEXDPSP(dig_port-
> >> >tc_phy_fia));
> >>
> >>  	WARN_ON(lane_mask == 0xffffffff);
> >>
> >> @@ -78,7 +97,8 @@ void intel_tc_port_set_fia_lane_count(struct
> >> intel_digital_port *dig_port,
> >>
> >>  	WARN_ON(lane_reversal && dig_port->tc_mode != TC_PORT_LEGACY);
> >>
> >> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPMLE1);
> >> +	val = intel_uncore_read(uncore,
> >> +				PORT_TX_DFLEXDPMLE1(dig_port-
> >> >tc_phy_fia));
> >>  	val &= ~DFLEXDPMLE1_DPMLETC_MASK(tc_port);
> >>
> >>  	switch (required_lanes) {
> >> @@ -97,7 +117,8 @@ void intel_tc_port_set_fia_lane_count(struct
> >> intel_digital_port *dig_port,
> >>  		MISSING_CASE(required_lanes);
> >>  	}
> >>
> >> -	intel_uncore_write(uncore, PORT_TX_DFLEXDPMLE1, val);
> >> +	intel_uncore_write(uncore,
> >> +			   PORT_TX_DFLEXDPMLE1(dig_port->tc_phy_fia),
> >> val);
> >>  }
> >>
> >>  static void tc_port_fixup_legacy_flag(struct intel_digital_port
> >> *dig_port,
> >> @@ -129,7 +150,8 @@ static u32 tc_port_live_status_mask(struct
> >> intel_digital_port *dig_port)
> >>  	u32 mask = 0;
> >>  	u32 val;
> >>
> >> -	val = intel_uncore_rea
> >> d(uncore, PORT_TX_DFLEXDPSP);
> >> +	val = intel_uncore_read(uncore,
> >> +				PORT_TX_DFLEXDPSP(dig_port-
> >> >tc_phy_fia));
> >>
> >>  	if (val == 0xffffffff) {
> >>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, nothing
> >> connected\n",
> >> @@ -159,7 +181,8 @@ static bool icl_tc_phy_status_complete(struct
> >> intel_digital_port *dig_port)
> >>  	struct intel_uncore *uncore = &i915->uncore;
> >>  	u32 val;
> >>
> >> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPPMS);
> >> +	val = intel_uncore_read(uncore,
> >> +				PORT_TX_DFLEXDPPMS(dig_port-
> >> >tc_phy_fia));
> >>  	if (val == 0xffffffff) {
> >>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assuming not
> >> complete\n",
> >>  			      dig_port->tc_port_name);
> >> @@ -177,7 +200,8 @@ static bool icl_tc_phy_set_safe_mode(struct
> >> intel_digital_port *dig_port,
> >>  	struct intel_uncore *uncore = &i915->uncore;
> >>  	u32 val;
> >>
> >> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
> >> +	val = intel_uncore_read(uncore,
> >> +				PORT_TX_DFLEXDPCSSS(dig_port-
> >> >tc_phy_fia));
> >>  	if (val == 0xffffffff) {
> >>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, can't set safe-
> >> mode to %s\n",
> >>  			      dig_port->tc_port_name,
> >> @@ -190,7 +214,8 @@ static bool icl_tc_phy_set_safe_mode(struct
> >> intel_digital_port *dig_port,
> >>  	if (!enable)
> >>  		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> >>
> >> -	intel_uncore_write(uncore, PORT_TX_DFLEXDPCSSS, val);
> >> +	intel_uncore_write(uncore,
> >> +			   PORT_TX_DFLEXDPCSSS(dig_port->tc_phy_fia),
> >> val);
> >>
> >>  	if (enable && wait_for(!icl_tc_phy_status_complete(dig_port),
> >> 10))
> >>  		DRM_DEBUG_KMS("Port %s: PHY complete clear timed
> >> out\n",
> >> @@ -206,7 +231,8 @@ static bool icl_tc_phy_is_in_safe_mode(struct
> >> intel_digital_port *dig_port)
> >>  	struct intel_uncore *uncore = &i915->uncore;
> >>  	u32 val;
> >>
> >> -	val = intel_uncore_read(uncore, PORT_TX_DFLEXDPCSSS);
> >> +	val = intel_uncore_read(uncore,
> >> +				PORT_TX_DFLEXDPCSSS(dig_port-
> >> >tc_phy_fia));
> >>  	if (val == 0xffffffff) {
> >>  		DRM_DEBUG_KMS("Port %s: PHY in TCCOLD, assume safe
> >> mode\n",
> >>  			      dig_port->tc_port_name);
> >> @@ -503,4 +529,5 @@ void intel_tc_port_init(struct intel_digital_port
> >> *dig_port, bool is_legacy)
> >>  	mutex_init(&dig_port->tc_lock);
> >>  	dig_port->tc_legacy_port = is_legacy;
> >>  	dig_port->tc_link_refcount = 0;
> >> +	dig_port->tc_phy_fia = tc_port_to_fia(i915, tc_port);
> >>  }
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> b/drivers/gpu/drm/i915/i915_reg.h
> >> index 95b9ca1fda2e..d0510022013c 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -2203,9 +2203,13 @@ enum i915_power_well_id {
> >>  #define   DW6_OLDO_DYN_PWR_DOWN_EN	(1 << 28)
> >>
> >>  #define FIA1_BASE			0x163000
> >> +#define FIA2_BASE			0x16E000
> >> +#define FIA3_BASE			0x16F000
> >> +#define _FIA(fia)			_PICK((fia), FIA1_BASE,
> >> FIA2_BASE, FIA3_BASE)
> >> +#define _MMIO_FIA(fia, off)		_MMIO(_FIA(fia) + (off))
> >>
> >>  /* ICL PHY DFLEX registers */
> >> -#define PORT_TX_DFLEXDPMLE1		_MMIO(FIA1_BASE + 0x008C0)
> >> +#define PORT_TX_DFLEXDPMLE1(fia)	_MMIO_FIA((fia),  0x008C0)
> >>  #define   DFLEXDPMLE1_DPMLETC_MASK(tc_port)	(0xf << (4 *
> >> (tc_port)))
> >>  #define   DFLEXDPMLE1_DPMLETC_ML0(tc_port)	(1 << (4 * (tc_port)))
> >>  #define   DFLEXDPMLE1_DPMLETC_ML1_0(tc_port)	(3 << (4 * (tc_port)))
> >> @@ -11484,17 +11488,18 @@ enum skl_power_gate {
> >>  						_ICL_DSC1_RC_BUF_THRESH
> >> _1_UDW_PB, \
> >>  						_ICL_DSC1_RC_BUF_THRESH
> >> _1_UDW_PC)
> >>
> >> -#define PORT_TX_DFLEXDPSP			_MMIO(FIA1_BASE +
> >> 0x008A0)
> >> +#define PORT_TX_DFLEXDPSP(fia)			_MMIO_FIA((fia)
> >> , 0x008A0)
> >> +#define   MODULAR_FIA_MASK			(1 << 4)
> >>  #define   TC_LIVE_STATE_TBT(tc_port)		(1 << ((tc_port) * 8 +
> >> 6))
> >>  #define   TC_LIVE_STATE_TC(tc_port)		(1 << ((tc_port) * 8 +
> >> 5))
> >>  #define   DP_LANE_ASSIGNMENT_SHIFT(tc_port)	((tc_port) * 8)
> >>  #define   DP_LANE_ASSIGNMENT_MASK(tc_port)	(0xf << ((tc_port) *
> >> 8))
> >>  #define   DP_LANE_ASSIGNMENT(tc_port, x)	((x) << ((tc_port) *
> >> 8))
> >>
> >> -#define PORT_TX_DFLEXDPPMS				_MMIO(FIA1_BASE
> >> + 0x00890)
> >> +#define PORT_TX_DFLEXDPPMS(fia)			_MMIO_FIA((fia)
> >> , 0x00890)
> >>  #define   DP_PHY_MODE_STATUS_COMPLETED(tc_port)		(1 <<
> >> (tc_port))
> >>
> >> -#define PORT_TX_DFLEXDPCSSS			_MMIO(FIA1_BASE +
> >> 0x00894)
> >> +#define PORT_TX_DFLEXDPCSSS(fia)		_MMIO_FIA((fia),
> >> 0x00894)
> >>  #define   DP_PHY_MODE_STATUS_NOT_SAFE(tc_port)		(1 <<
> >> (tc_port))
> >>
> >>  #endif /* _I915_REG_H_ */
> >> diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> >> b/drivers/gpu/drm/i915/intel_device_info.h
> >> index ddafc819bf30..e9dc86ed517b 100644
> >> --- a/drivers/gpu/drm/i915/intel_device_info.h
> >> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> >> @@ -136,6 +136,7 @@ enum intel_ppgtt_type {
> >>  	func(has_gmch); \
> >>  	func(has_hotplug); \
> >>  	func(has_ipc); \
> >> +	func(has_modular_fia); \
> >
> >If we have a register to tell us whether the platform supports this,
> >why do we need this feature flag?
> 
> because the platform may not have the register bit that tells if the we
> have modular FIA, as is the case with Ice Lake. The bit is reserved
> there. This is actually "may_have_modular_fia" rather than
> "has_modular_fia", but the name is pretty bad to make it that way.

Are we expecting this to ping-pong between various gen12+ platforms?
If not then IMO a flag is overkill.

> 
> Lucas De Marchi
> 
> >
> >Thanks,
> >Stuart
> >
> >>  	func(has_overlay); \
> >>  	func(has_psr); \
> >>  	func(overlay_needs_physical); \
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> b/drivers/gpu/drm/i915/intel_drv.h
> >> index 770f9f6aad84..e8ecbd55476e 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1245,6 +1245,7 @@ struct intel_digital_port {
> >>  	bool tc_legacy_port:1;
> >>  	char tc_port_name[8];
> >>  	enum tc_port_mode tc_mode;
> >> +	enum phy_fia tc_phy_fia;
> >>
> >>  	void (*write_infoframe)(struct intel_encoder *encoder,
> >>  				const struct intel_crtc_state
> >> *crtc_state,
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] drm/i915: Add modular FIA
  2019-07-12 12:55       ` Ville Syrjälä
@ 2019-07-12 15:27         ` Lucas De Marchi
  0 siblings, 0 replies; 8+ messages in thread
From: Lucas De Marchi @ 2019-07-12 15:27 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Fri, Jul 12, 2019 at 03:55:01PM +0300, Ville Syrjälä wrote:
>> because the platform may not have the register bit that tells if the we
>> have modular FIA, as is the case with Ice Lake. The bit is reserved
>> there. This is actually "may_have_modular_fia" rather than
>> "has_modular_fia", but the name is pretty bad to make it that way.
>
>Are we expecting this to ping-pong between various gen12+ platforms?
>If not then IMO a flag is overkill.

I would not expect a ping pong, but I don't really know. Since currently
there's only one we can change the check to be gen >= 12. Or we can
leave as is and change it to be a gen check when next platforms come.

Lucas De Marchi

>
>>
>> Lucas De Marchi
>>
>> >
>> >Thanks,
>> >Stuart
>> >
>> >>  	func(has_overlay); \
>> >>  	func(has_psr); \
>> >>  	func(overlay_needs_physical); \
>> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> >> b/drivers/gpu/drm/i915/intel_drv.h
>> >> index 770f9f6aad84..e8ecbd55476e 100644
>> >> --- a/drivers/gpu/drm/i915/intel_drv.h
>> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> >> @@ -1245,6 +1245,7 @@ struct intel_digital_port {
>> >>  	bool tc_legacy_port:1;
>> >>  	char tc_port_name[8];
>> >>  	enum tc_port_mode tc_mode;
>> >> +	enum phy_fia tc_phy_fia;
>> >>
>> >>  	void (*write_infoframe)(struct intel_encoder *encoder,
>> >>  				const struct intel_crtc_state
>> >> *crtc_state,
>>
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>-- 
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-07-12 15:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-11 20:58 [PATCH v3 0/1] Modular FIA Lucas De Marchi
2019-07-11 20:58 ` [PATCH v3 1/1] drm/i915: Add modular FIA Lucas De Marchi
2019-07-11 23:15   ` Summers, Stuart
2019-07-11 23:49     ` Lucas De Marchi
2019-07-12  0:06       ` Summers, Stuart
2019-07-12 12:55       ` Ville Syrjälä
2019-07-12 15:27         ` Lucas De Marchi
2019-07-11 22:06 ` ✓ Fi.CI.BAT: success for Modular FIA (rev4) Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.