Re: [PATCH v2 4/4] drm/i915: Add modular FIA

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jul 10, 2019 at 12:22:51PM -0700, Lucas De Marchi wrote:
> On Wed, Jul 10, 2019 at 09:33:39PM +0300, Ville Syrjälä wrote:
> >On Mon, Jul 08, 2019 at 10:28:15AM -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
> >>
> >> 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>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_tc.c  | 49 ++++++++++++++++++++----
> >>  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 +
> >>  4 files changed, 52 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> >> index f44ee4bfe7c8..671261b55d11 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> >> @@ -8,6 +8,12 @@
> >>  #include "intel_dp_mst.h"
> >>  #include "intel_tc.h"
> >>
> >> +enum phy_fia {
> >> +	FIA1,
> >> +	FIA2,
> >> +	FIA3,
> >> +};
> >> +
> >>  static const char *tc_port_mode_name(enum tc_port_mode mode)
> >>  {
> >>  	static const char * const names[] = {
> >> @@ -22,6 +28,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 +53,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 +103,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 +123,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 +156,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 +187,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 +206,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 +220,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 +237,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 +535,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 5898f59e3dd7..74f2b31443fb 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)))
> >> @@ -11480,17 +11484,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 24c63ed45c6f..7ea979c8c5f7 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;
> >> +	u8 tc_phy_fia;
> >
> >enum phy_fia ?
> 
> but then we need to move its definition to the header.
> 
> Is it really needed? The only place that should care about it is
> intel_tc.c

I think if we have a name for it we should use it. Otherwise it's just
confusing, and someone is going to clean it up accidentally anyway.

> 
> Lucas De Marchi
> 
> >
> >I didn't read theough the nuts and bolts of the register offsets but the
> >approach seems OK to me.
> >
> >Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >>
> >>  	void (*write_infoframe)(struct intel_encoder *encoder,
> >>  				const struct intel_crtc_state *crtc_state,
> >> --
> >> 2.21.0
> >
> >-- 
> >Ville Syrjälä
> >Intel

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