Skip to contentSkip to conversation

drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

PATCH In review Imre Deak 3 participants 16 messages Jun 15, 2018 – Jun 21, 2018

Subsystems (2)
lore
Share & tools
lore

Imre Deak

author
#0 lore raw

PATCH 1/2 drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Atm we're zeroing out fields in MG_PLL_BIAS and MG_PLL_TDC_COLDST_BIAS if refclk is 38.4MHz, whereas the spec tells us to preserve them. Although the calculated values mostly match the register defaults even for the 38.4MHz case, there are some differences wrt. what BIOS programs (I noticed at least differences in the MG_PLL_BIAS/IREFTRIM and MG_PLL_BIAS/BIASCAL_EN fields). In the lack of further info on how to program these fields, just do what the spec says and preserve the BIOS state.

v2:
- Preserve the BIOS programmed reg fields instead of programming them.

Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 63 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
 2 files changed, 47 insertions(+), 18 deletions(-)
drivers/gpu/drm/i915/intel_dpll_mgr.c +45 -18
index 132fe63e042a..d4c7bacbe83e 100644
@@ -2812,25 +2812,31 @@ static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,
 				MG_PLL_SSC_FLLEN |
 				MG_PLL_SSC_STEPSIZE(ssc_stepsize);
 
-	pll_state->mg_pll_tdc_coldst_bias = MG_PLL_TDC_COLDST_COLDSTART;
+	pll_state->mg_pll_tdc_coldst_bias = MG_PLL_TDC_COLDST_COLDSTART |
+					    MG_PLL_TDC_COLDST_IREFINT_EN |
+					    MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |
+					    MG_PLL_TDC_TDCOVCCORR_EN |
+					    MG_PLL_TDC_TDCSEL(3);
 
-	if (refclk_khz != 38400) {
-		pll_state->mg_pll_tdc_coldst_bias |=
-			MG_PLL_TDC_COLDST_IREFINT_EN |
-			MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |
-			MG_PLL_TDC_COLDST_COLDSTART |
-			MG_PLL_TDC_TDCOVCCORR_EN |
-			MG_PLL_TDC_TDCSEL(3);
+	pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
+				 MG_PLL_BIAS_INIT_DCOAMP(0x3F) |
+				 MG_PLL_BIAS_BIAS_BONUS(10) |
+				 MG_PLL_BIAS_BIASCAL_EN |
+				 MG_PLL_BIAS_CTRIM(12) |
+				 MG_PLL_BIAS_VREF_RDAC(4) |
+				 MG_PLL_BIAS_IREFTRIM(iref_trim);
 
-		pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
-					 MG_PLL_BIAS_INIT_DCOAMP(0x3F) |
-					 MG_PLL_BIAS_BIAS_BONUS(10) |
-					 MG_PLL_BIAS_BIASCAL_EN |
-					 MG_PLL_BIAS_CTRIM(12) |
-					 MG_PLL_BIAS_VREF_RDAC(4) |
-					 MG_PLL_BIAS_IREFTRIM(iref_trim);
+	if (refclk_khz == 38400) {
+		pll_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
+		pll_state->mg_pll_bias_mask = 0;
+	} else {
+		pll_state->mg_pll_tdc_coldst_bias_mask = -1U;
+		pll_state->mg_pll_bias_mask = -1U;
 	}
 
+	pll_state->mg_pll_tdc_coldst_bias &= pll_state->mg_pll_tdc_coldst_bias_mask;
+	pll_state->mg_pll_bias &= pll_state->mg_pll_bias_mask;
+
 	return true;
 }
 
@@ -2948,9 +2954,21 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
 		hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(port));
 		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(port));
+
 		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(port));
 		hw_state->mg_pll_tdc_coldst_bias =
 			I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+
+		if (dev_priv->cdclk.hw.ref == 38400) {
+			hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
+			hw_state->mg_pll_bias_mask = 0;
+		} else {
+			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
+			hw_state->mg_pll_bias_mask = -1U;
+		}
+
+		hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
+		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
 		break;
 	default:
 		MISSING_CASE(id);
@@ -2978,6 +2996,7 @@ static void icl_mg_pll_write(struct drm_i915_private *dev_priv,
 {
 	struct intel_dpll_hw_state *hw_state = &pll->state.hw_state;
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
+	u32 val;
 
 	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
 	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
@@ -2988,9 +3007,17 @@ static void icl_mg_pll_write(struct drm_i915_private *dev_priv,
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
 	I915_WRITE(MG_PLL_FRAC_LOCK(port), hw_state->mg_pll_frac_lock);
 	I915_WRITE(MG_PLL_SSC(port), hw_state->mg_pll_ssc);
-	I915_WRITE(MG_PLL_BIAS(port), hw_state->mg_pll_bias);
-	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port),
-		   hw_state->mg_pll_tdc_coldst_bias);
+
+	val = I915_READ(MG_PLL_BIAS(port));
+	val &= ~hw_state->mg_pll_bias_mask;
+	val |= hw_state->mg_pll_bias;
+	I915_WRITE(MG_PLL_BIAS(port), val);
+
+	val = I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+	val &= ~hw_state->mg_pll_tdc_coldst_bias_mask;
+	val |= hw_state->mg_pll_tdc_coldst_bias;
+	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port), val);
+
 	POSTING_READ(MG_PLL_TDC_COLDST_BIAS(port));
 }
 
drivers/gpu/drm/i915/intel_dpll_mgr.h +2 -0
index ba925c7ee482..7e522cf4f13f 100644
@@ -180,6 +180,8 @@ struct intel_dpll_hw_state {
 	uint32_t mg_pll_ssc;
 	uint32_t mg_pll_bias;
 	uint32_t mg_pll_tdc_coldst_bias;
+	uint32_t mg_pll_bias_mask;
+	uint32_t mg_pll_tdc_coldst_bias_mask;
 };
 
 /**
Signature · 6 lines
2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#1 lore raw

PATCH 2/2 drm/i915/icl: Do read-modify-write as needed during MG PLL programming

Some MG PLL registers have fields that need to be preserved at their HW default or BIOS programmed values. So make sure we preserve them.

Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/i915_reg.h       | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 33 +++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_reg.h +13 -0
index b8c0ebd50889..506e6896f8ea 100644
@@ -9005,6 +9005,7 @@ enum skl_power_gate {
 #define _MG_REFCLKIN_CTL_PORT3				0x16A92C
 #define _MG_REFCLKIN_CTL_PORT4				0x16B92C
 #define   MG_REFCLKIN_CTL_OD_2_MUX(x)			((x) << 8)
+#define   MG_REFCLKIN_CTL_OD_2_MUX_MASK			(0x7 << 8)
 #define MG_REFCLKIN_CTL(port) _MMIO_PORT((port) - PORT_C, \
 					 _MG_REFCLKIN_CTL_PORT1, \
 					 _MG_REFCLKIN_CTL_PORT2)
@@ -9014,7 +9015,9 @@ enum skl_power_gate {
 #define _MG_CLKTOP2_CORECLKCTL1_PORT3			0x16A8D8
 #define _MG_CLKTOP2_CORECLKCTL1_PORT4			0x16B8D8
 #define   MG_CLKTOP2_CORECLKCTL1_B_DIVRATIO(x)		((x) << 16)
+#define   MG_CLKTOP2_CORECLKCTL1_B_DIVRATIO_MASK	(0xff << 16)
 #define   MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO(x)		((x) << 8)
+#define   MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK	(0xff << 8)
 #define MG_CLKTOP2_CORECLKCTL1(port) _MMIO_PORT((port) - PORT_C, \
 						_MG_CLKTOP2_CORECLKCTL1_PORT1, \
 						_MG_CLKTOP2_CORECLKCTL1_PORT2)
@@ -9024,9 +9027,13 @@ enum skl_power_gate {
 #define _MG_CLKTOP2_HSCLKCTL_PORT3			0x16A8D4
 #define _MG_CLKTOP2_HSCLKCTL_PORT4			0x16B8D4
 #define   MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL(x)		((x) << 16)
+#define   MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK	(0x1 << 16)
 #define   MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL(x)	((x) << 14)
+#define   MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK	(0x3 << 14)
 #define   MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO(x)		((x) << 12)
+#define   MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK		(0x3 << 12)
 #define   MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO(x)		((x) << 8)
+#define   MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK		(0xf << 8)
 #define MG_CLKTOP2_HSCLKCTL(port) _MMIO_PORT((port) - PORT_C, \
 					     _MG_CLKTOP2_HSCLKCTL_PORT1, \
 					     _MG_CLKTOP2_HSCLKCTL_PORT2)
@@ -9100,12 +9107,18 @@ enum skl_power_gate {
 #define _MG_PLL_BIAS_PORT3				0x16AA14
 #define _MG_PLL_BIAS_PORT4				0x16BA14
 #define   MG_PLL_BIAS_BIAS_GB_SEL(x)			((x) << 30)
+#define   MG_PLL_BIAS_BIAS_GB_SEL_MASK			(0x3 << 30)
 #define   MG_PLL_BIAS_INIT_DCOAMP(x)			((x) << 24)
+#define   MG_PLL_BIAS_INIT_DCOAMP_MASK			(0x3f << 24)
 #define   MG_PLL_BIAS_BIAS_BONUS(x)			((x) << 16)
+#define   MG_PLL_BIAS_BIAS_BONUS_MASK			(0xff << 16)
 #define   MG_PLL_BIAS_BIASCAL_EN			(1 << 15)
 #define   MG_PLL_BIAS_CTRIM(x)				((x) << 8)
+#define   MG_PLL_BIAS_CTRIM_MASK			(0x1f << 8)
 #define   MG_PLL_BIAS_VREF_RDAC(x)			((x) << 5)
+#define   MG_PLL_BIAS_VREF_RDAC_MASK			(0x7 << 5)
 #define   MG_PLL_BIAS_IREFTRIM(x)			((x) << 0)
+#define   MG_PLL_BIAS_IREFTRIM_MASK			(0x1f << 0)
 #define MG_PLL_BIAS(port) _MMIO_PORT((port) - PORT_C, _MG_PLL_BIAS_PORT1, \
 				     _MG_PLL_BIAS_PORT2)
 
drivers/gpu/drm/i915/intel_dpll_mgr.c +29 -4
index d4c7bacbe83e..3ffc219a2407 100644
@@ -2945,10 +2945,21 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	case DPLL_ID_ICL_MGPLL4:
 		port = icl_mg_pll_id_to_port(id);
 		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(port));
+		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+
 		hw_state->mg_clktop2_coreclkctl1 =
 			I915_READ(MG_CLKTOP2_CORECLKCTL1(port));
+		hw_state->mg_clktop2_coreclkctl1 &=
+			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+
 		hw_state->mg_clktop2_hsclkctl =
 			I915_READ(MG_CLKTOP2_HSCLKCTL(port));
+		hw_state->mg_clktop2_hsclkctl &= (
+			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
+			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK);
+
 		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(port));
 		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(port));
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
@@ -2998,10 +3009,24 @@ static void icl_mg_pll_write(struct drm_i915_private *dev_priv,
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
 	u32 val;
 
-	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
-	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
-		   hw_state->mg_clktop2_coreclkctl1);
-	I915_WRITE(MG_CLKTOP2_HSCLKCTL(port), hw_state->mg_clktop2_hsclkctl);
+	val = I915_READ(MG_REFCLKIN_CTL(port));
+	val &= ~MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+	val |= hw_state->mg_refclkin_ctl;
+	I915_WRITE(MG_REFCLKIN_CTL(port), val);
+
+	val = I915_READ(MG_CLKTOP2_CORECLKCTL1(port));
+	val &= ~MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+	val |= hw_state->mg_clktop2_coreclkctl1;
+	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port), val);
+
+	val = I915_READ(MG_CLKTOP2_HSCLKCTL(port));
+	val &= ~(MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
+		 MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+		 MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+		 MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK);
+	val |= hw_state->mg_clktop2_hsclkctl;
+	I915_WRITE(MG_CLKTOP2_HSCLKCTL(port), val);
+
 	I915_WRITE(MG_PLL_DIV0(port), hw_state->mg_pll_div0);
 	I915_WRITE(MG_PLL_DIV1(port), hw_state->mg_pll_div1);
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
Signature · 6 lines
2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#2 lore raw

PATCH v2 2/2 drm/i915/icl: Do read-modify-write as needed during MG PLL programming

Some MG PLL registers have fields that need to be preserved at their HW default or BIOS programmed values. So make sure we preserve them.

v2:
- Add comment to icl_mg_pll_write() explaining the need for register masks. (Vandita)
- Fix patchwork checkpatch warning.

Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/i915_reg.h       | 13 ++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 39 +++++++++++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_reg.h +13 -0
index c6c9e4f9a718..6e587d49b936 100644
@@ -9405,6 +9405,7 @@ enum skl_power_gate {
 #define _MG_REFCLKIN_CTL_PORT3				0x16A92C
 #define _MG_REFCLKIN_CTL_PORT4				0x16B92C
 #define   MG_REFCLKIN_CTL_OD_2_MUX(x)			((x) << 8)
+#define   MG_REFCLKIN_CTL_OD_2_MUX_MASK			(0x7 << 8)
 #define MG_REFCLKIN_CTL(port) _MMIO_PORT((port) - PORT_C, \
 					 _MG_REFCLKIN_CTL_PORT1, \
 					 _MG_REFCLKIN_CTL_PORT2)
@@ -9414,7 +9415,9 @@ enum skl_power_gate {
 #define _MG_CLKTOP2_CORECLKCTL1_PORT3			0x16A8D8
 #define _MG_CLKTOP2_CORECLKCTL1_PORT4			0x16B8D8
 #define   MG_CLKTOP2_CORECLKCTL1_B_DIVRATIO(x)		((x) << 16)
+#define   MG_CLKTOP2_CORECLKCTL1_B_DIVRATIO_MASK	(0xff << 16)
 #define   MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO(x)		((x) << 8)
+#define   MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK	(0xff << 8)
 #define MG_CLKTOP2_CORECLKCTL1(port) _MMIO_PORT((port) - PORT_C, \
 						_MG_CLKTOP2_CORECLKCTL1_PORT1, \
 						_MG_CLKTOP2_CORECLKCTL1_PORT2)
@@ -9424,9 +9427,13 @@ enum skl_power_gate {
 #define _MG_CLKTOP2_HSCLKCTL_PORT3			0x16A8D4
 #define _MG_CLKTOP2_HSCLKCTL_PORT4			0x16B8D4
 #define   MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL(x)		((x) << 16)
+#define   MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK	(0x1 << 16)
 #define   MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL(x)	((x) << 14)
+#define   MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK	(0x3 << 14)
 #define   MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO(x)		((x) << 12)
+#define   MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK		(0x3 << 12)
 #define   MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO(x)		((x) << 8)
+#define   MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK		(0xf << 8)
 #define MG_CLKTOP2_HSCLKCTL(port) _MMIO_PORT((port) - PORT_C, \
 					     _MG_CLKTOP2_HSCLKCTL_PORT1, \
 					     _MG_CLKTOP2_HSCLKCTL_PORT2)
@@ -9500,12 +9507,18 @@ enum skl_power_gate {
 #define _MG_PLL_BIAS_PORT3				0x16AA14
 #define _MG_PLL_BIAS_PORT4				0x16BA14
 #define   MG_PLL_BIAS_BIAS_GB_SEL(x)			((x) << 30)
+#define   MG_PLL_BIAS_BIAS_GB_SEL_MASK			(0x3 << 30)
 #define   MG_PLL_BIAS_INIT_DCOAMP(x)			((x) << 24)
+#define   MG_PLL_BIAS_INIT_DCOAMP_MASK			(0x3f << 24)
 #define   MG_PLL_BIAS_BIAS_BONUS(x)			((x) << 16)
+#define   MG_PLL_BIAS_BIAS_BONUS_MASK			(0xff << 16)
 #define   MG_PLL_BIAS_BIASCAL_EN			(1 << 15)
 #define   MG_PLL_BIAS_CTRIM(x)				((x) << 8)
+#define   MG_PLL_BIAS_CTRIM_MASK			(0x1f << 8)
 #define   MG_PLL_BIAS_VREF_RDAC(x)			((x) << 5)
+#define   MG_PLL_BIAS_VREF_RDAC_MASK			(0x7 << 5)
 #define   MG_PLL_BIAS_IREFTRIM(x)			((x) << 0)
+#define   MG_PLL_BIAS_IREFTRIM_MASK			(0x1f << 0)
 #define MG_PLL_BIAS(port) _MMIO_PORT((port) - PORT_C, _MG_PLL_BIAS_PORT1, \
 				     _MG_PLL_BIAS_PORT2)
 
drivers/gpu/drm/i915/intel_dpll_mgr.c +35 -4
index 8543a1015e21..ae47cdb80d54 100644
@@ -2992,10 +2992,21 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	} else {
 		port = icl_mg_pll_id_to_port(dev_priv, id);
 		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(port));
+		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+
 		hw_state->mg_clktop2_coreclkctl1 =
 			I915_READ(MG_CLKTOP2_CORECLKCTL1(port));
+		hw_state->mg_clktop2_coreclkctl1 &=
+			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+
 		hw_state->mg_clktop2_hsclkctl =
 			I915_READ(MG_CLKTOP2_HSCLKCTL(port));
+		hw_state->mg_clktop2_hsclkctl &=
+			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
+			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
+
 		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(port));
 		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(port));
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
@@ -3051,10 +3062,30 @@ static void icl_mg_pll_write(struct drm_i915_private *dev_priv,
 	enum port port = icl_mg_pll_id_to_port(dev_priv, pll->info->id);
 	u32 val;
 
-	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
-	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
-		   hw_state->mg_clktop2_coreclkctl1);
-	I915_WRITE(MG_CLKTOP2_HSCLKCTL(port), hw_state->mg_clktop2_hsclkctl);
+	/*
+	 * Some of the following registers have reserved fields, so program
+	 * these with RMW based on a mask. The mask can be fixed or generated
+	 * during the calc/readout phase if the mask depends on some other HW
+	 * state like refclk, see icl_calc_mg_pll_state().
+	 */
+	val = I915_READ(MG_REFCLKIN_CTL(port));
+	val &= ~MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+	val |= hw_state->mg_refclkin_ctl;
+	I915_WRITE(MG_REFCLKIN_CTL(port), val);
+
+	val = I915_READ(MG_CLKTOP2_CORECLKCTL1(port));
+	val &= ~MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+	val |= hw_state->mg_clktop2_coreclkctl1;
+	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port), val);
+
+	val = I915_READ(MG_CLKTOP2_HSCLKCTL(port));
+	val &= ~(MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
+		 MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+		 MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+		 MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK);
+	val |= hw_state->mg_clktop2_hsclkctl;
+	I915_WRITE(MG_CLKTOP2_HSCLKCTL(port), val);
+
 	I915_WRITE(MG_PLL_DIV0(port), hw_state->mg_pll_div0);
 	I915_WRITE(MG_PLL_DIV1(port), hw_state->mg_pll_div1);
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
Signature · 6 lines
2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#3 lore raw

PATCH v3 2/2 drm/i915/icl: Do read-modify-write as needed during MG PLL programming

Some MG PLL registers have fields that need to be preserved at their HW default or BIOS programmed values. So make sure we preserve them.

v2:
- Add comment to icl_mg_pll_write() explaining the need for register masks. (Vandita)
- Fix patchwork checkpatch warning.

v3:
- Rebase on drm-tip.

Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/i915_reg.h       | 13 ++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 39 +++++++++++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_reg.h +13 -0
index 4bfd7a9bd75f..65b222287ee4 100644
@@ -9047,6 +9047,7 @@ enum skl_power_gate {
 #define _MG_REFCLKIN_CTL_PORT3				0x16A92C
 #define _MG_REFCLKIN_CTL_PORT4				0x16B92C
 #define   MG_REFCLKIN_CTL_OD_2_MUX(x)			((x) << 8)
+#define   MG_REFCLKIN_CTL_OD_2_MUX_MASK			(0x7 << 8)
 #define MG_REFCLKIN_CTL(port) _MMIO_PORT((port) - PORT_C, \
 					 _MG_REFCLKIN_CTL_PORT1, \
 					 _MG_REFCLKIN_CTL_PORT2)
@@ -9056,7 +9057,9 @@ enum skl_power_gate {
 #define _MG_CLKTOP2_CORECLKCTL1_PORT3			0x16A8D8
 #define _MG_CLKTOP2_CORECLKCTL1_PORT4			0x16B8D8
 #define   MG_CLKTOP2_CORECLKCTL1_B_DIVRATIO(x)		((x) << 16)
+#define   MG_CLKTOP2_CORECLKCTL1_B_DIVRATIO_MASK	(0xff << 16)
 #define   MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO(x)		((x) << 8)
+#define   MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK	(0xff << 8)
 #define MG_CLKTOP2_CORECLKCTL1(port) _MMIO_PORT((port) - PORT_C, \
 						_MG_CLKTOP2_CORECLKCTL1_PORT1, \
 						_MG_CLKTOP2_CORECLKCTL1_PORT2)
@@ -9066,9 +9069,13 @@ enum skl_power_gate {
 #define _MG_CLKTOP2_HSCLKCTL_PORT3			0x16A8D4
 #define _MG_CLKTOP2_HSCLKCTL_PORT4			0x16B8D4
 #define   MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL(x)		((x) << 16)
+#define   MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK	(0x1 << 16)
 #define   MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL(x)	((x) << 14)
+#define   MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK	(0x3 << 14)
 #define   MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO(x)		((x) << 12)
+#define   MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK		(0x3 << 12)
 #define   MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO(x)		((x) << 8)
+#define   MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK		(0xf << 8)
 #define MG_CLKTOP2_HSCLKCTL(port) _MMIO_PORT((port) - PORT_C, \
 					     _MG_CLKTOP2_HSCLKCTL_PORT1, \
 					     _MG_CLKTOP2_HSCLKCTL_PORT2)
@@ -9142,12 +9149,18 @@ enum skl_power_gate {
 #define _MG_PLL_BIAS_PORT3				0x16AA14
 #define _MG_PLL_BIAS_PORT4				0x16BA14
 #define   MG_PLL_BIAS_BIAS_GB_SEL(x)			((x) << 30)
+#define   MG_PLL_BIAS_BIAS_GB_SEL_MASK			(0x3 << 30)
 #define   MG_PLL_BIAS_INIT_DCOAMP(x)			((x) << 24)
+#define   MG_PLL_BIAS_INIT_DCOAMP_MASK			(0x3f << 24)
 #define   MG_PLL_BIAS_BIAS_BONUS(x)			((x) << 16)
+#define   MG_PLL_BIAS_BIAS_BONUS_MASK			(0xff << 16)
 #define   MG_PLL_BIAS_BIASCAL_EN			(1 << 15)
 #define   MG_PLL_BIAS_CTRIM(x)				((x) << 8)
+#define   MG_PLL_BIAS_CTRIM_MASK			(0x1f << 8)
 #define   MG_PLL_BIAS_VREF_RDAC(x)			((x) << 5)
+#define   MG_PLL_BIAS_VREF_RDAC_MASK			(0x7 << 5)
 #define   MG_PLL_BIAS_IREFTRIM(x)			((x) << 0)
+#define   MG_PLL_BIAS_IREFTRIM_MASK			(0x1f << 0)
 #define MG_PLL_BIAS(port) _MMIO_PORT((port) - PORT_C, _MG_PLL_BIAS_PORT1, \
 				     _MG_PLL_BIAS_PORT2)
 
drivers/gpu/drm/i915/intel_dpll_mgr.c +35 -4
index d4c7bacbe83e..57342364fd30 100644
@@ -2945,10 +2945,21 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	case DPLL_ID_ICL_MGPLL4:
 		port = icl_mg_pll_id_to_port(id);
 		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(port));
+		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+
 		hw_state->mg_clktop2_coreclkctl1 =
 			I915_READ(MG_CLKTOP2_CORECLKCTL1(port));
+		hw_state->mg_clktop2_coreclkctl1 &=
+			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+
 		hw_state->mg_clktop2_hsclkctl =
 			I915_READ(MG_CLKTOP2_HSCLKCTL(port));
+		hw_state->mg_clktop2_hsclkctl &=
+			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
+			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
+
 		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(port));
 		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(port));
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
@@ -2998,10 +3009,30 @@ static void icl_mg_pll_write(struct drm_i915_private *dev_priv,
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
 	u32 val;
 
-	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
-	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
-		   hw_state->mg_clktop2_coreclkctl1);
-	I915_WRITE(MG_CLKTOP2_HSCLKCTL(port), hw_state->mg_clktop2_hsclkctl);
+	/*
+	 * Some of the following registers have reserved fields, so program
+	 * these with RMW based on a mask. The mask can be fixed or generated
+	 * during the calc/readout phase if the mask depends on some other HW
+	 * state like refclk, see icl_calc_mg_pll_state().
+	 */
+	val = I915_READ(MG_REFCLKIN_CTL(port));
+	val &= ~MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+	val |= hw_state->mg_refclkin_ctl;
+	I915_WRITE(MG_REFCLKIN_CTL(port), val);
+
+	val = I915_READ(MG_CLKTOP2_CORECLKCTL1(port));
+	val &= ~MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+	val |= hw_state->mg_clktop2_coreclkctl1;
+	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port), val);
+
+	val = I915_READ(MG_CLKTOP2_HSCLKCTL(port));
+	val &= ~(MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
+		 MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+		 MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+		 MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK);
+	val |= hw_state->mg_clktop2_hsclkctl;
+	I915_WRITE(MG_CLKTOP2_HSCLKCTL(port), val);
+
 	I915_WRITE(MG_PLL_DIV0(port), hw_state->mg_pll_div0);
 	I915_WRITE(MG_PLL_DIV1(port), hw_state->mg_pll_div1);
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
Signature · 6 lines
2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Reviewed-by on v3 2/2
#4 lore raw

Re: [PATCH v3 2/2] drm/i915/icl: Do read-modify-write as needed during MG PLL programming

Quoted 19 lines

-----Original Message-----
From: Deak, Imre
Sent: Tuesday, June 19, 2018 10:11 PM
To: [email protected]
Cc: Kulkarni, Vandita <[email protected]>; Zanoni, Paulo R <[email protected]>; Ausmus, James <[email protected]>
Subject: [PATCH v3 2/2] drm/i915/icl: Do read-modify-write as needed during MG PLL programming

Some MG PLL registers have fields that need to be preserved at their HW default or BIOS programmed values. So make sure we preserve them.

v2:
- Add comment to icl_mg_pll_write() explaining the need for register masks. (Vandita)
- Fix patchwork checkpatch warning.

Quoted 8 lines

v3:
- Rebase on drm-tip.

Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>

Looks good to me.

Quoted 150 lines
---
 drivers/gpu/drm/i915/i915_reg.h       | 13 ++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 39
+++++++++++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 4 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_reg.h index 4bfd7a9bd75f..65b222287ee4 100644

drivers/gpu/drm/i915/i915_reg.h +1 -0
@@ -9047,6 +9047,7 @@ enum skl_power_gate {
 #define _MG_REFCLKIN_CTL_PORT3				0x16A92C
 #define _MG_REFCLKIN_CTL_PORT4				0x16B92C
 #define   MG_REFCLKIN_CTL_OD_2_MUX(x)			((x) << 8)
+#define   MG_REFCLKIN_CTL_OD_2_MUX_MASK			(0x7
 << 8)
 #define MG_REFCLKIN_CTL(port) _MMIO_PORT((port) - PORT_C, \
 					 _MG_REFCLKIN_CTL_PORT1, \
drivers/gpu/drm/i915/i915_reg.h +1 -0
 					 _MG_REFCLKIN_CTL_PORT2)
@@ -9056,7 +9057,9 @@ enum skl_power_gate {
 #define _MG_CLKTOP2_CORECLKCTL1_PORT3
 	0x16A8D8
 #define _MG_CLKTOP2_CORECLKCTL1_PORT4
 	0x16B8D8
 #define   MG_CLKTOP2_CORECLKCTL1_B_DIVRATIO(x)		((x)
 << 16)
+#define   MG_CLKTOP2_CORECLKCTL1_B_DIVRATIO_MASK	(0xff << 16)
 #define   MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO(x)		((x)

<< 8)
+#define MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK (0xff << 8)
#define MG_CLKTOP2_CORECLKCTL1(port) _MMIO_PORT((port) - PORT_C,
\

	_MG_CLKTOP2_CORECLKCTL1_PORT1, \

	_MG_CLKTOP2_CORECLKCTL1_PORT2)
drivers/gpu/drm/i915/i915_reg.h +4 -0
@@ -9066,9 +9069,13 @@ enum skl_power_gate {
 #define _MG_CLKTOP2_HSCLKCTL_PORT3			0x16A8D4
 #define _MG_CLKTOP2_HSCLKCTL_PORT4			0x16B8D4
 #define   MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL(x)		((x)
 << 16)
+#define   MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK	(0x1 << 16)
 #define   MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL(x)	((x) << 14)
+#define   MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK	(0x3
 << 14)
 #define   MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO(x)		((x) << 12)
+#define   MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK		(0x3
 << 12)
 #define   MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO(x)		((x) << 8)
+#define   MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK		(0xf

<< 8)
#define MG_CLKTOP2_HSCLKCTL(port) _MMIO_PORT((port) - PORT_C, \

_MG_CLKTOP2_HSCLKCTL_PORT1, \

_MG_CLKTOP2_HSCLKCTL_PORT2)

drivers/gpu/drm/i915/i915_reg.h +6 -0
@@ -9142,12 +9149,18 @@ enum skl_power_gate {
 #define _MG_PLL_BIAS_PORT3				0x16AA14
 #define _MG_PLL_BIAS_PORT4				0x16BA14
 #define   MG_PLL_BIAS_BIAS_GB_SEL(x)			((x) << 30)
+#define   MG_PLL_BIAS_BIAS_GB_SEL_MASK			(0x3
 << 30)
 #define   MG_PLL_BIAS_INIT_DCOAMP(x)			((x) << 24)
+#define   MG_PLL_BIAS_INIT_DCOAMP_MASK			(0x3f
 << 24)
 #define   MG_PLL_BIAS_BIAS_BONUS(x)			((x) << 16)
+#define   MG_PLL_BIAS_BIAS_BONUS_MASK			(0xff << 16)
 #define   MG_PLL_BIAS_BIASCAL_EN			(1 << 15)
 #define   MG_PLL_BIAS_CTRIM(x)				((x) << 8)
+#define   MG_PLL_BIAS_CTRIM_MASK			(0x1f << 8)
 #define   MG_PLL_BIAS_VREF_RDAC(x)			((x) << 5)
+#define   MG_PLL_BIAS_VREF_RDAC_MASK			(0x7 << 5)
 #define   MG_PLL_BIAS_IREFTRIM(x)			((x) << 0)
+#define   MG_PLL_BIAS_IREFTRIM_MASK			(0x1f << 0)
 #define MG_PLL_BIAS(port) _MMIO_PORT((port) - PORT_C,

_MG_PLL_BIAS_PORT1, \

drivers/gpu/drm/i915/i915_reg.h +0 -0
 				     _MG_PLL_BIAS_PORT2)
drivers/gpu/drm/i915/intel_dpll_mgr.c +7 -0
index d4c7bacbe83e..57342364fd30 100644
@@ -2945,10 +2945,21 @@ static bool icl_pll_get_hw_state(struct
 drm_i915_private *dev_priv,
 	case DPLL_ID_ICL_MGPLL4:
 		port = icl_mg_pll_id_to_port(id);
 		hw_state->mg_refclkin_ctl =
 I915_READ(MG_REFCLKIN_CTL(port));
+		hw_state->mg_refclkin_ctl &=
 MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+
 		hw_state->mg_clktop2_coreclkctl1 =
 			I915_READ(MG_CLKTOP2_CORECLKCTL1(port));
+		hw_state->mg_clktop2_coreclkctl1 &=
+			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+
 		hw_state->mg_clktop2_hsclkctl =
 			I915_READ(MG_CLKTOP2_HSCLKCTL(port));
+		hw_state->mg_clktop2_hsclkctl &=
+			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK

|

drivers/gpu/drm/i915/intel_dpll_mgr.c +4 -0
+			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
+
 		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(port));
 		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(port));
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port)); @@ -

2998,10 +3009,30 @@ static void icl_mg_pll_write(struct drm_i915_private *dev_priv,

drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -4
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
 	u32 val;
 
-	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
-	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
-		   hw_state->mg_clktop2_coreclkctl1);
-	I915_WRITE(MG_CLKTOP2_HSCLKCTL(port), hw_state-
Quoted 1 line

mg_clktop2_hsclkctl);

drivers/gpu/drm/i915/intel_dpll_mgr.c +3 -0
+	/*
+	 * Some of the following registers have reserved fields, so program
+	 * these with RMW based on a mask. The mask can be fixed or

generated

drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -0
+	 * during the calc/readout phase if the mask depends on some

other HW

drivers/gpu/drm/i915/intel_dpll_mgr.c +20 -1
+	 * state like refclk, see icl_calc_mg_pll_state().
+	 */
+	val = I915_READ(MG_REFCLKIN_CTL(port));
+	val &= ~MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+	val |= hw_state->mg_refclkin_ctl;
+	I915_WRITE(MG_REFCLKIN_CTL(port), val);
+
+	val = I915_READ(MG_CLKTOP2_CORECLKCTL1(port));
+	val &= ~MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+	val |= hw_state->mg_clktop2_coreclkctl1;
+	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port), val);
+
+	val = I915_READ(MG_CLKTOP2_HSCLKCTL(port));
+	val &= ~(MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
+		 MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+		 MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+		 MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK);
+	val |= hw_state->mg_clktop2_hsclkctl;
+	I915_WRITE(MG_CLKTOP2_HSCLKCTL(port), val);
+
 	I915_WRITE(MG_PLL_DIV0(port), hw_state->mg_pll_div0);
 	I915_WRITE(MG_PLL_DIV1(port), hw_state->mg_pll_div1);
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
--
Quoted 1 line

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#5 lore raw

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

In reply to Imre Deak
== Series Details ==

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz
URL : https://patchwork.freedesktop.org/series/44836/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
b74cd00bcd1f drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz
53a13e6c6e2b drm/i915/icl: Do read-modify-write as needed during MG PLL programming
-:88: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#88: FILE: drivers/gpu/drm/i915/intel_dpll_mgr.c:2957:
+		hw_state->mg_clktop2_hsclkctl &= (

total: 0 errors, 0 warnings, 1 checks, 96 lines checked

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#6 lore raw

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

In reply to Imre Deak
== Series Details ==

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz
URL : https://patchwork.freedesktop.org/series/44836/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4325 -> Patchwork_9329 =

== Summary - WARNING ==

Minor unknown changes coming with Patchwork_9329 need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_9329, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://patchwork.freedesktop.org/api/1.0/series/44836/revisions/1/mbox/

== Possible new issues ==

Here are the unknown changes that may have been introduced in Patchwork_9329:

=== IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        SKIP -> PASS

== Known issues ==

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

=== IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       PASS -> FAIL (fdo#106744)

    igt@kms_pipe_crc_basic@read-crc-pipe-c:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-c-frame-sequence:
      fi-glk-j4005:       DMESG-WARN (fdo#106000, fdo#106097) -> PASS

fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744

== Participating hosts (43 -> 38) ==

Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u

== Build changes ==

* Linux: CI_DRM_4325 -> Patchwork_9329

CI_DRM_4325: 4275ebe85ad179007c49b7bcf78d340b7681871e @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4520: 91f5d4665b07f073c78abd3cd4b8e0e347dbf638 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9329: 53a13e6c6e2b34622403c66aa9b8f08015ff9236 @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

53a13e6c6e2b drm/i915/icl: Do read-modify-write as needed during MG PLL programming b74cd00bcd1f drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9329/issues.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#7 lore raw

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

In reply to Imre Deak
== Series Details ==

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz
URL : https://patchwork.freedesktop.org/series/44836/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4325_full -> Patchwork_9329_full =

== Summary - FAILURE ==

Serious unknown changes coming with Patchwork_9329_full absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_9329_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

== Possible new issues ==

Here are the unknown changes that may have been introduced in Patchwork_9329_full:

=== IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          PASS -> DMESG-FAIL

    
    ==== Warnings ====

    igt@drv_selftest@live_execlists:
      shard-apl:          SKIP -> PASS +1

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          PASS -> SKIP +1

    igt@gem_exec_schedule@deep-render:
      shard-kbl:          SKIP -> PASS

== Known issues ==

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

=== IGT changes ===

    ==== Issues hit ====

    igt@gem_wait@write-wait-blt:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    igt@kms_flip_tiling@flip-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-glk:          FAIL (fdo#105347) -> PASS

    igt@drv_selftest@live_hangcheck:
      shard-apl:          DMESG-FAIL -> PASS
      shard-glk:          DMESG-FAIL -> PASS

    igt@gem_partial_pwrite_pread@writes-after-reads-uncached:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
      shard-glk:          FAIL (fdo#105703) -> PASS

    igt@kms_vblank@pipe-c-accuracy-idle:
      shard-glk:          FAIL (fdo#102583) -> PASS

fdo#102583 https://bugs.freedesktop.org/show_bug.cgi?id=102583
fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703

== Participating hosts (5 -> 5) ==

No changes in participating hosts

== Build changes ==

* Linux: CI_DRM_4325 -> Patchwork_9329

CI_DRM_4325: 4275ebe85ad179007c49b7bcf78d340b7681871e @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4520: 91f5d4665b07f073c78abd3cd4b8e0e347dbf638 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9329: 53a13e6c6e2b34622403c66aa9b8f08015ff9236 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9329/shards.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#8 lore raw
In reply to Imre Deak
Quoted 135 lines

-----Original Message-----
From: Deak, Imre
Sent: Friday, June 15, 2018 8:09 PM
To: [email protected]
Cc: Kulkarni, Vandita <[email protected]>; Zanoni, Paulo R <[email protected]>; Ausmus, James <[email protected]>
Subject: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Atm we're zeroing out fields in MG_PLL_BIAS and MG_PLL_TDC_COLDST_BIAS if refclk is 38.4MHz, whereas the spec tells us to preserve them.
Although the calculated values mostly match the register defaults even for the 38.4MHz case, there are some differences wrt. what BIOS programs (I noticed at least differences in the MG_PLL_BIAS/IREFTRIM and MG_PLL_BIAS/BIASCAL_EN fields). In the lack of further info on how to program these fields, just do what the spec says and preserve the BIOS state.

v2:
- Preserve the BIOS programmed reg fields instead of programming them.

Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 63
+++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
 2 files changed, 47 insertions(+), 18 deletions(-)
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -1
index 132fe63e042a..d4c7bacbe83e 100644
@@ -2812,25 +2812,31 @@ static bool icl_calc_mg_pll_state(struct
 intel_crtc_state *crtc_state,
 				MG_PLL_SSC_FLLEN |
 				MG_PLL_SSC_STEPSIZE(ssc_stepsize);
 
-	pll_state->mg_pll_tdc_coldst_bias =
 MG_PLL_TDC_COLDST_COLDSTART;
+	pll_state->mg_pll_tdc_coldst_bias =

MG_PLL_TDC_COLDST_COLDSTART |
+
MG_PLL_TDC_COLDST_IREFINT_EN |
+ MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -4
+					    MG_PLL_TDC_TDCOVCCORR_EN |
+					    MG_PLL_TDC_TDCSEL(3);
 
-	if (refclk_khz != 38400) {
-		pll_state->mg_pll_tdc_coldst_bias |=
-			MG_PLL_TDC_COLDST_IREFINT_EN |
-

MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +7 -5
-			MG_PLL_TDC_COLDST_COLDSTART |
-			MG_PLL_TDC_TDCOVCCORR_EN |
-			MG_PLL_TDC_TDCSEL(3);
+	pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
+				 MG_PLL_BIAS_INIT_DCOAMP(0x3F) |
+				 MG_PLL_BIAS_BIAS_BONUS(10) |
+				 MG_PLL_BIAS_BIASCAL_EN |
+				 MG_PLL_BIAS_CTRIM(12) |
+				 MG_PLL_BIAS_VREF_RDAC(4) |
+				 MG_PLL_BIAS_IREFTRIM(iref_trim);
 
-		pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
-					 MG_PLL_BIAS_INIT_DCOAMP(0x3F)

|

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -5
-					 MG_PLL_BIAS_BIAS_BONUS(10) |
-					 MG_PLL_BIAS_BIASCAL_EN |
-					 MG_PLL_BIAS_CTRIM(12) |
-					 MG_PLL_BIAS_VREF_RDAC(4) |
-					 MG_PLL_BIAS_IREFTRIM(iref_trim);
+	if (refclk_khz == 38400) {
+		pll_state->mg_pll_tdc_coldst_bias_mask =

MG_PLL_TDC_COLDST_COLDSTART;

drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+		pll_state->mg_pll_bias_mask = 0;
+	} else {
+		pll_state->mg_pll_tdc_coldst_bias_mask = -1U;
+		pll_state->mg_pll_bias_mask = -1U;
 	}
 
+	pll_state->mg_pll_tdc_coldst_bias &= pll_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +13 -0
+	pll_state->mg_pll_bias &= pll_state->mg_pll_bias_mask;
+
 	return true;
 }
@@ -2948,9 +2954,21 @@ static bool icl_pll_get_hw_state(struct
 drm_i915_private *dev_priv,
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
 		hw_state->mg_pll_frac_lock =
 I915_READ(MG_PLL_FRAC_LOCK(port));
 		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(port));
+
 		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(port));
 		hw_state->mg_pll_tdc_coldst_bias =
 			I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+
+		if (dev_priv->cdclk.hw.ref == 38400) {
+			hw_state->mg_pll_tdc_coldst_bias_mask =
 MG_PLL_TDC_COLDST_COLDSTART;
+			hw_state->mg_pll_bias_mask = 0;
+		} else {
+			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
+			hw_state->mg_pll_bias_mask = -1U;
+		}
+
+		hw_state->mg_pll_tdc_coldst_bias &= hw_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -0
+		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
 		break;
 	default:
 		MISSING_CASE(id);
@@ -2978,6 +2996,7 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,  {
 	struct intel_dpll_hw_state *hw_state = &pll->state.hw_state;
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
+	u32 val;
 
 	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
 	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
@@ -2988,9 +3007,17 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
 	I915_WRITE(MG_PLL_FRAC_LOCK(port), hw_state-
 >mg_pll_frac_lock);

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct · line 3017

Quoted 10 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +6 -3
 	I915_WRITE(MG_PLL_SSC(port), hw_state->mg_pll_ssc);
-	I915_WRITE(MG_PLL_BIAS(port), hw_state->mg_pll_bias);
-	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port),
-		   hw_state->mg_pll_tdc_coldst_bias);
+
+	val = I915_READ(MG_PLL_BIAS(port));
+	val &= ~hw_state->mg_pll_bias_mask;
+	val |= hw_state->mg_pll_bias;
+	I915_WRITE(MG_PLL_BIAS(port), val);
+

Looks like we are writing the exact read value for pll_bias when ref_clk is 38400, we can skip this write or is it mandatory to write?

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3020

Quoted 5 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+	val = I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+	val &= ~hw_state->mg_pll_tdc_coldst_bias_mask;
+	val |= hw_state->mg_pll_tdc_coldst_bias;
+	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port), val);
+

Same here.

Quoted 18 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 	POSTING_READ(MG_PLL_TDC_COLDST_BIAS(port));

}

drivers/gpu/drm/i915/intel_dpll_mgr.h +2 -1
index ba925c7ee482..7e522cf4f13f 100644
@@ -180,6 +180,8 @@ struct intel_dpll_hw_state {
 	uint32_t mg_pll_ssc;
 	uint32_t mg_pll_bias;
 	uint32_t mg_pll_tdc_coldst_bias;
+	uint32_t mg_pll_bias_mask;
+	uint32_t mg_pll_tdc_coldst_bias_mask;
 };
 
 /**
--
Quoted 1 line

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

author
#9 lore raw
Quoted 135 lines · Vandita
Quoted 135 lines

-----Original Message-----
From: Deak, Imre
Sent: Friday, June 15, 2018 8:09 PM
To: [email protected]
Cc: Kulkarni, Vandita <[email protected]>; Zanoni, Paulo R <[email protected]>; Ausmus, James <[email protected]>
Subject: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Atm we're zeroing out fields in MG_PLL_BIAS and MG_PLL_TDC_COLDST_BIAS if refclk is 38.4MHz, whereas the spec tells us to preserve them.
Although the calculated values mostly match the register defaults even for the 38.4MHz case, there are some differences wrt. what BIOS programs (I noticed at least differences in the MG_PLL_BIAS/IREFTRIM and MG_PLL_BIAS/BIASCAL_EN fields). In the lack of further info on how to program these fields, just do what the spec says and preserve the BIOS state.

v2:
- Preserve the BIOS programmed reg fields instead of programming them.

Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 63
+++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
 2 files changed, 47 insertions(+), 18 deletions(-)
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -1
index 132fe63e042a..d4c7bacbe83e 100644
@@ -2812,25 +2812,31 @@ static bool icl_calc_mg_pll_state(struct
 intel_crtc_state *crtc_state,
 				MG_PLL_SSC_FLLEN |
 				MG_PLL_SSC_STEPSIZE(ssc_stepsize);
 
-	pll_state->mg_pll_tdc_coldst_bias =
 MG_PLL_TDC_COLDST_COLDSTART;
+	pll_state->mg_pll_tdc_coldst_bias =

MG_PLL_TDC_COLDST_COLDSTART |
+
MG_PLL_TDC_COLDST_IREFINT_EN |
+ MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -4
+					    MG_PLL_TDC_TDCOVCCORR_EN |
+					    MG_PLL_TDC_TDCSEL(3);
 
-	if (refclk_khz != 38400) {
-		pll_state->mg_pll_tdc_coldst_bias |=
-			MG_PLL_TDC_COLDST_IREFINT_EN |
-

MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +7 -5
-			MG_PLL_TDC_COLDST_COLDSTART |
-			MG_PLL_TDC_TDCOVCCORR_EN |
-			MG_PLL_TDC_TDCSEL(3);
+	pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
+				 MG_PLL_BIAS_INIT_DCOAMP(0x3F) |
+				 MG_PLL_BIAS_BIAS_BONUS(10) |
+				 MG_PLL_BIAS_BIASCAL_EN |
+				 MG_PLL_BIAS_CTRIM(12) |
+				 MG_PLL_BIAS_VREF_RDAC(4) |
+				 MG_PLL_BIAS_IREFTRIM(iref_trim);
 
-		pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
-					 MG_PLL_BIAS_INIT_DCOAMP(0x3F)

|

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -5
-					 MG_PLL_BIAS_BIAS_BONUS(10) |
-					 MG_PLL_BIAS_BIASCAL_EN |
-					 MG_PLL_BIAS_CTRIM(12) |
-					 MG_PLL_BIAS_VREF_RDAC(4) |
-					 MG_PLL_BIAS_IREFTRIM(iref_trim);
+	if (refclk_khz == 38400) {
+		pll_state->mg_pll_tdc_coldst_bias_mask =

MG_PLL_TDC_COLDST_COLDSTART;

drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+		pll_state->mg_pll_bias_mask = 0;
+	} else {
+		pll_state->mg_pll_tdc_coldst_bias_mask = -1U;
+		pll_state->mg_pll_bias_mask = -1U;
 	}
 
+	pll_state->mg_pll_tdc_coldst_bias &= pll_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +13 -0
+	pll_state->mg_pll_bias &= pll_state->mg_pll_bias_mask;
+
 	return true;
 }
@@ -2948,9 +2954,21 @@ static bool icl_pll_get_hw_state(struct
 drm_i915_private *dev_priv,
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
 		hw_state->mg_pll_frac_lock =
 I915_READ(MG_PLL_FRAC_LOCK(port));
 		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(port));
+
 		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(port));
 		hw_state->mg_pll_tdc_coldst_bias =
 			I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+
+		if (dev_priv->cdclk.hw.ref == 38400) {
+			hw_state->mg_pll_tdc_coldst_bias_mask =
 MG_PLL_TDC_COLDST_COLDSTART;
+			hw_state->mg_pll_bias_mask = 0;
+		} else {
+			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
+			hw_state->mg_pll_bias_mask = -1U;
+		}
+
+		hw_state->mg_pll_tdc_coldst_bias &= hw_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -0
+		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
 		break;
 	default:
 		MISSING_CASE(id);
@@ -2978,6 +2996,7 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,  {
 	struct intel_dpll_hw_state *hw_state = &pll->state.hw_state;
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
+	u32 val;
 
 	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
 	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
@@ -2988,9 +3007,17 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
 	I915_WRITE(MG_PLL_FRAC_LOCK(port), hw_state-
 >mg_pll_frac_lock);
Quoted 12 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3015

Quoted 10 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +6 -3
 	I915_WRITE(MG_PLL_SSC(port), hw_state->mg_pll_ssc);
-	I915_WRITE(MG_PLL_BIAS(port), hw_state->mg_pll_bias);
-	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port),
-		   hw_state->mg_pll_tdc_coldst_bias);
+
+	val = I915_READ(MG_PLL_BIAS(port));
+	val &= ~hw_state->mg_pll_bias_mask;
+	val |= hw_state->mg_pll_bias;
+	I915_WRITE(MG_PLL_BIAS(port), val);
+

Looks like we are writing the exact read value for pll_bias when ref_clk is 38400, we can skip this write or is it mandatory to write?

I'm still hoping that we'll get the proper programming steps for the 38.4MHz case too, at which point we can remove the special casing during the calculation step and write here the calculated values in all cases. It's also not a performance critical part, so I'd rather avoid adding more special casing.

Quoted 6 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3020

Quoted 5 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+	val = I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+	val &= ~hw_state->mg_pll_tdc_coldst_bias_mask;
+	val |= hw_state->mg_pll_tdc_coldst_bias;
+	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port), val);
+

Same here.

We have to write MG_PLL_TDC_COLDST_COLDSTART in all cases.

Quoted 16 lines
Quoted 16 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 	POSTING_READ(MG_PLL_TDC_COLDST_BIAS(port));

}

drivers/gpu/drm/i915/intel_dpll_mgr.h +2 -0
index ba925c7ee482..7e522cf4f13f 100644
@@ -180,6 +180,8 @@ struct intel_dpll_hw_state {
 	uint32_t mg_pll_ssc;
 	uint32_t mg_pll_bias;
 	uint32_t mg_pll_tdc_coldst_bias;
+	uint32_t mg_pll_bias_mask;
+	uint32_t mg_pll_tdc_coldst_bias_mask;
 };
 
Quoted 4 lines
Quoted 3 lines
drivers/gpu/drm/i915/intel_dpll_mgr.h +0 -1
 /**
--

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#10 lore raw
Quoted 162 lines

-----Original Message-----
From: Deak, Imre
Sent: Monday, June 18, 2018 2:45 PM
To: Kulkarni, Vandita <[email protected]>
Cc: [email protected]; Zanoni, Paulo R <[email protected]>; Ausmus, James <[email protected]>
Subject: Re: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Quoted 8 lines · Vandita
Quoted 8 lines

-----Original Message-----
From: Deak, Imre
Sent: Friday, June 15, 2018 8:09 PM
To: [email protected]
Cc: Kulkarni, Vandita <[email protected]>; Zanoni, Paulo R <[email protected]>; Ausmus, James

<[email protected]>

Quoted 13 lines
Quoted 13 lines

Subject: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Atm we're zeroing out fields in MG_PLL_BIAS and MG_PLL_TDC_COLDST_BIAS if refclk is 38.4MHz, whereas the spec tells us to preserve them.
Although the calculated values mostly match the register defaults even for the 38.4MHz case, there are some differences wrt. what BIOS programs (I noticed at least differences in the MG_PLL_BIAS/IREFTRIM and MG_PLL_BIAS/BIASCAL_EN fields). In the lack of further info on how to program these fields, just do what the spec says and preserve the BIOS state.

Quoted 2 lines
Quoted 2 lines

v2:
- Preserve the BIOS programmed reg fields instead of programming

them.

Quoted 112 lines
Quoted 112 lines
Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 63
+++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
 2 files changed, 47 insertions(+), 18 deletions(-)
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -1
index 132fe63e042a..d4c7bacbe83e 100644
@@ -2812,25 +2812,31 @@ static bool icl_calc_mg_pll_state(struct
 intel_crtc_state *crtc_state,
 				MG_PLL_SSC_FLLEN |
 				MG_PLL_SSC_STEPSIZE(ssc_stepsize);
 
-	pll_state->mg_pll_tdc_coldst_bias =
 MG_PLL_TDC_COLDST_COLDSTART;
+	pll_state->mg_pll_tdc_coldst_bias =

MG_PLL_TDC_COLDST_COLDSTART |
+
MG_PLL_TDC_COLDST_IREFINT_EN |
+ MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -4
+					    MG_PLL_TDC_TDCOVCCORR_EN |
+					    MG_PLL_TDC_TDCSEL(3);
 
-	if (refclk_khz != 38400) {
-		pll_state->mg_pll_tdc_coldst_bias |=
-			MG_PLL_TDC_COLDST_IREFINT_EN |
-

MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +7 -5
-			MG_PLL_TDC_COLDST_COLDSTART |
-			MG_PLL_TDC_TDCOVCCORR_EN |
-			MG_PLL_TDC_TDCSEL(3);
+	pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
+				 MG_PLL_BIAS_INIT_DCOAMP(0x3F) |
+				 MG_PLL_BIAS_BIAS_BONUS(10) |
+				 MG_PLL_BIAS_BIASCAL_EN |
+				 MG_PLL_BIAS_CTRIM(12) |
+				 MG_PLL_BIAS_VREF_RDAC(4) |
+				 MG_PLL_BIAS_IREFTRIM(iref_trim);
 
-		pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
-					 MG_PLL_BIAS_INIT_DCOAMP(0x3F)

|

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -5
-					 MG_PLL_BIAS_BIAS_BONUS(10) |
-					 MG_PLL_BIAS_BIASCAL_EN |
-					 MG_PLL_BIAS_CTRIM(12) |
-					 MG_PLL_BIAS_VREF_RDAC(4) |
-					 MG_PLL_BIAS_IREFTRIM(iref_trim);
+	if (refclk_khz == 38400) {
+		pll_state->mg_pll_tdc_coldst_bias_mask =

MG_PLL_TDC_COLDST_COLDSTART;

drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+		pll_state->mg_pll_bias_mask = 0;
+	} else {
+		pll_state->mg_pll_tdc_coldst_bias_mask = -1U;
+		pll_state->mg_pll_bias_mask = -1U;
 	}
 
+	pll_state->mg_pll_tdc_coldst_bias &= pll_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +13 -0
+	pll_state->mg_pll_bias &= pll_state->mg_pll_bias_mask;
+
 	return true;
 }
@@ -2948,9 +2954,21 @@ static bool icl_pll_get_hw_state(struct
 drm_i915_private *dev_priv,
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
 		hw_state->mg_pll_frac_lock =
 I915_READ(MG_PLL_FRAC_LOCK(port));
 		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(port));
+
 		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(port));
 		hw_state->mg_pll_tdc_coldst_bias =
 			I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+
+		if (dev_priv->cdclk.hw.ref == 38400) {
+			hw_state->mg_pll_tdc_coldst_bias_mask =
 MG_PLL_TDC_COLDST_COLDSTART;
+			hw_state->mg_pll_bias_mask = 0;
+		} else {
+			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
+			hw_state->mg_pll_bias_mask = -1U;
+		}
+
+		hw_state->mg_pll_tdc_coldst_bias &= hw_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -0
+		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
 		break;
 	default:
 		MISSING_CASE(id);
@@ -2978,6 +2996,7 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,  {
 	struct intel_dpll_hw_state *hw_state = &pll->state.hw_state;
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
+	u32 val;
 
 	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
 	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
@@ -2988,9 +3007,17 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
 	I915_WRITE(MG_PLL_FRAC_LOCK(port), hw_state-
 >mg_pll_frac_lock);
Quoted 12 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3015

Quoted 10 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +6 -3
 	I915_WRITE(MG_PLL_SSC(port), hw_state->mg_pll_ssc);
-	I915_WRITE(MG_PLL_BIAS(port), hw_state->mg_pll_bias);
-	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port),
-		   hw_state->mg_pll_tdc_coldst_bias);
+
+	val = I915_READ(MG_PLL_BIAS(port));
+	val &= ~hw_state->mg_pll_bias_mask;
+	val |= hw_state->mg_pll_bias;
+	I915_WRITE(MG_PLL_BIAS(port), val);
+

Looks like we are writing the exact read value for pll_bias when ref_clk is 38400, we can skip this write or is it mandatory to write?

Quoted 5 lines

I'm still hoping that we'll get the proper programming steps for the 38.4MHz case too, at which point we can remove the special casing during the calculation step and write here the calculated values in all cases. It's also not a performance critical part, so I'd rather avoid adding more special casing.

Thanks for the clarification.
I saw that algo in the bspec said to program only when ref_clk!=38400 and leave as default in 38400 case.
In which case we wouldn't need the masks. We can directly check the ref clk. I think, may be we can leave the programming to default for 38400 and handle it when bspec gets updated with new calculation.

The rest of the patch looks good to me.

Quoted 30 lines
Quoted 7 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3020

Quoted 6 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+	val = I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+	val &= ~hw_state->mg_pll_tdc_coldst_bias_mask;
+	val |= hw_state->mg_pll_tdc_coldst_bias;
+	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port), val);
+

Same here.

We have to write MG_PLL_TDC_COLDST_COLDSTART in all cases.

Quoted 20 lines
Quoted 19 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 	POSTING_READ(MG_PLL_TDC_COLDST_BIAS(port));

}

drivers/gpu/drm/i915/intel_dpll_mgr.h +2 -1
index ba925c7ee482..7e522cf4f13f 100644
@@ -180,6 +180,8 @@ struct intel_dpll_hw_state {
 	uint32_t mg_pll_ssc;
 	uint32_t mg_pll_bias;
 	uint32_t mg_pll_tdc_coldst_bias;
+	uint32_t mg_pll_bias_mask;
+	uint32_t mg_pll_tdc_coldst_bias_mask;
 };
 
 /**
--

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

author
#11 lore raw
Quoted 166 lines · Vandita
Quoted 160 lines

-----Original Message-----
From: Deak, Imre
Sent: Monday, June 18, 2018 2:45 PM
To: Kulkarni, Vandita <[email protected]>
Cc: [email protected]; Zanoni, Paulo R <[email protected]>; Ausmus, James <[email protected]>
Subject: Re: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Quoted 8 lines · Vandita
Quoted 8 lines

-----Original Message-----
From: Deak, Imre
Sent: Friday, June 15, 2018 8:09 PM
To: [email protected]
Cc: Kulkarni, Vandita <[email protected]>; Zanoni, Paulo R <[email protected]>; Ausmus, James

<[email protected]>

Quoted 13 lines
Quoted 13 lines

Subject: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Atm we're zeroing out fields in MG_PLL_BIAS and MG_PLL_TDC_COLDST_BIAS if refclk is 38.4MHz, whereas the spec tells us to preserve them.
Although the calculated values mostly match the register defaults even for the 38.4MHz case, there are some differences wrt. what BIOS programs (I noticed at least differences in the MG_PLL_BIAS/IREFTRIM and MG_PLL_BIAS/BIASCAL_EN fields). In the lack of further info on how to program these fields, just do what the spec says and preserve the BIOS state.

Quoted 2 lines
Quoted 2 lines

v2:
- Preserve the BIOS programmed reg fields instead of programming

them.

Quoted 112 lines
Quoted 112 lines
Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 63
+++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
 2 files changed, 47 insertions(+), 18 deletions(-)
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -1
index 132fe63e042a..d4c7bacbe83e 100644
@@ -2812,25 +2812,31 @@ static bool icl_calc_mg_pll_state(struct
 intel_crtc_state *crtc_state,
 				MG_PLL_SSC_FLLEN |
 				MG_PLL_SSC_STEPSIZE(ssc_stepsize);
 
-	pll_state->mg_pll_tdc_coldst_bias =
 MG_PLL_TDC_COLDST_COLDSTART;
+	pll_state->mg_pll_tdc_coldst_bias =

MG_PLL_TDC_COLDST_COLDSTART |
+
MG_PLL_TDC_COLDST_IREFINT_EN |
+ MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -4
+					    MG_PLL_TDC_TDCOVCCORR_EN |
+					    MG_PLL_TDC_TDCSEL(3);
 
-	if (refclk_khz != 38400) {
-		pll_state->mg_pll_tdc_coldst_bias |=
-			MG_PLL_TDC_COLDST_IREFINT_EN |
-

MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +7 -5
-			MG_PLL_TDC_COLDST_COLDSTART |
-			MG_PLL_TDC_TDCOVCCORR_EN |
-			MG_PLL_TDC_TDCSEL(3);
+	pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
+				 MG_PLL_BIAS_INIT_DCOAMP(0x3F) |
+				 MG_PLL_BIAS_BIAS_BONUS(10) |
+				 MG_PLL_BIAS_BIASCAL_EN |
+				 MG_PLL_BIAS_CTRIM(12) |
+				 MG_PLL_BIAS_VREF_RDAC(4) |
+				 MG_PLL_BIAS_IREFTRIM(iref_trim);
 
-		pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
-					 MG_PLL_BIAS_INIT_DCOAMP(0x3F)

|

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -5
-					 MG_PLL_BIAS_BIAS_BONUS(10) |
-					 MG_PLL_BIAS_BIASCAL_EN |
-					 MG_PLL_BIAS_CTRIM(12) |
-					 MG_PLL_BIAS_VREF_RDAC(4) |
-					 MG_PLL_BIAS_IREFTRIM(iref_trim);
+	if (refclk_khz == 38400) {
+		pll_state->mg_pll_tdc_coldst_bias_mask =

MG_PLL_TDC_COLDST_COLDSTART;

drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+		pll_state->mg_pll_bias_mask = 0;
+	} else {
+		pll_state->mg_pll_tdc_coldst_bias_mask = -1U;
+		pll_state->mg_pll_bias_mask = -1U;
 	}
 
+	pll_state->mg_pll_tdc_coldst_bias &= pll_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +13 -0
+	pll_state->mg_pll_bias &= pll_state->mg_pll_bias_mask;
+
 	return true;
 }
@@ -2948,9 +2954,21 @@ static bool icl_pll_get_hw_state(struct
 drm_i915_private *dev_priv,
 		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(port));
 		hw_state->mg_pll_frac_lock =
 I915_READ(MG_PLL_FRAC_LOCK(port));
 		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(port));
+
 		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(port));
 		hw_state->mg_pll_tdc_coldst_bias =
 			I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+
+		if (dev_priv->cdclk.hw.ref == 38400) {
+			hw_state->mg_pll_tdc_coldst_bias_mask =
 MG_PLL_TDC_COLDST_COLDSTART;
+			hw_state->mg_pll_bias_mask = 0;
+		} else {
+			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
+			hw_state->mg_pll_bias_mask = -1U;
+		}
+
+		hw_state->mg_pll_tdc_coldst_bias &= hw_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -0
+		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
 		break;
 	default:
 		MISSING_CASE(id);
@@ -2978,6 +2996,7 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,  {
 	struct intel_dpll_hw_state *hw_state = &pll->state.hw_state;
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
+	u32 val;
 
 	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state->mg_refclkin_ctl);
 	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
@@ -2988,9 +3007,17 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
 	I915_WRITE(MG_PLL_FRAC_LOCK(port), hw_state-
 >mg_pll_frac_lock);
Quoted 12 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3015

Quoted 10 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +6 -3
 	I915_WRITE(MG_PLL_SSC(port), hw_state->mg_pll_ssc);
-	I915_WRITE(MG_PLL_BIAS(port), hw_state->mg_pll_bias);
-	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port),
-		   hw_state->mg_pll_tdc_coldst_bias);
+
+	val = I915_READ(MG_PLL_BIAS(port));
+	val &= ~hw_state->mg_pll_bias_mask;
+	val |= hw_state->mg_pll_bias;
+	I915_WRITE(MG_PLL_BIAS(port), val);
+

Looks like we are writing the exact read value for pll_bias when ref_clk is 38400, we can skip this write or is it mandatory to write?

Quoted 5 lines

I'm still hoping that we'll get the proper programming steps for the 38.4MHz case too, at which point we can remove the special casing during the calculation step and write here the calculated values in all cases. It's also not a performance critical part, so I'd rather avoid adding more special casing.

Quoted 3 lines

Thanks for the clarification. I saw that algo in the bspec said to program only when ref_clk!=38400 and leave as default in 38400 case. In which case we wouldn't need the masks.

Yes, and that's what this code does, we leave this register unchanged in case of ref_clk=38.4MHz. We do need the masks as the generic way to say which fields in which registers to change. This function doesn't need to care about some other state outside of intel_dpll_hw_state.

Quoted 1 line

We can directly check the ref clk.

I'd like to have the full state included in intel_dpll_hw_state, as that is used by the HW/SW state checker and the shared PLL framework to see if a PLL can be reused by doing a memcmp on this struct.

Quoted 2 lines

I think, may be we can leave the programming to default for 38400 and handle it when bspec gets updated with new calculation.

Yes, the patch leaves this register at its default in the 38.4MHz case.

Quoted 32 lines

The rest of the patch looks good to me.

Quoted 30 lines
Quoted 7 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3020

Quoted 6 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+	val = I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+	val &= ~hw_state->mg_pll_tdc_coldst_bias_mask;
+	val |= hw_state->mg_pll_tdc_coldst_bias;
+	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port), val);
+

Same here.

We have to write MG_PLL_TDC_COLDST_COLDSTART in all cases.

Quoted 20 lines
Quoted 19 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 	POSTING_READ(MG_PLL_TDC_COLDST_BIAS(port));

}

drivers/gpu/drm/i915/intel_dpll_mgr.h +2 -1
index ba925c7ee482..7e522cf4f13f 100644
@@ -180,6 +180,8 @@ struct intel_dpll_hw_state {
 	uint32_t mg_pll_ssc;
 	uint32_t mg_pll_bias;
 	uint32_t mg_pll_tdc_coldst_bias;
+	uint32_t mg_pll_bias_mask;
+	uint32_t mg_pll_tdc_coldst_bias_mask;
 };
 
 /**
--

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reviewed-by
#12 lore raw
Quoted 203 lines

-----Original Message-----
From: Deak, Imre
Sent: Tuesday, June 19, 2018 3:43 PM
To: Kulkarni, Vandita <[email protected]>
Cc: [email protected]; Zanoni, Paulo R <[email protected]>; Ausmus, James <[email protected]>
Subject: Re: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Quoted 6 lines · Vandita
Quoted 6 lines

-----Original Message-----
From: Deak, Imre
Sent: Monday, June 18, 2018 2:45 PM
To: Kulkarni, Vandita <[email protected]>
Cc: [email protected]; Zanoni, Paulo R <[email protected]>; Ausmus, James

<[email protected]>

Quoted 16 lines
Quoted 16 lines

Subject: Re: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Quoted 8 lines · Vandita
Quoted 8 lines

-----Original Message-----
From: Deak, Imre
Sent: Friday, June 15, 2018 8:09 PM
To: [email protected]
Cc: Kulkarni, Vandita <[email protected]>; Zanoni, Paulo R <[email protected]>; Ausmus, James

<[email protected]>

Quoted 3 lines
Quoted 3 lines

Subject: [PATCH 1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

Atm we're zeroing out fields in MG_PLL_BIAS and MG_PLL_TDC_COLDST_BIAS if refclk is 38.4MHz, whereas the spec tells us to preserve them.
Although the calculated values mostly match the register defaults even for the 38.4MHz case, there are some differences wrt. what BIOS programs (I noticed at least differences in the MG_PLL_BIAS/IREFTRIM and MG_PLL_BIAS/BIASCAL_EN fields). In

the

Quoted 18 lines
Quoted 18 lines
Quoted 5 lines
Quoted 5 lines

lack of further info on how to program these fields, just do what the spec says and preserve the BIOS state.

v2:
- Preserve the BIOS programmed reg fields instead of programming

them.

Quoted 12 lines
Quoted 12 lines
Cc Vandita Kulkarni <[email protected]>
Cc Paulo Zanoni <[email protected]>
Cc James Ausmus <[email protected]>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 63
+++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
 2 files changed, 47 insertions(+), 18 deletions(-)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool

Quoted 6 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
index 132fe63e042a..d4c7bacbe83e 100644
@@ -2812,25 +2812,31 @@ static bool

icl_calc_mg_pll_state(struct

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state, · line 2814

Quoted 3 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 intel_crtc_state *crtc_state,
 				MG_PLL_SSC_FLLEN |
 
	MG_PLL_SSC_STEPSIZE(ssc_stepsize);
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -1
-	pll_state->mg_pll_tdc_coldst_bias =
 MG_PLL_TDC_COLDST_COLDSTART;
+	pll_state->mg_pll_tdc_coldst_bias =

MG_PLL_TDC_COLDST_COLDSTART |
+
MG_PLL_TDC_COLDST_IREFINT_EN |
+ MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) | +

MG_PLL_TDC_TDCOVCCORR_EN |

Quoted 12 lines
Quoted 12 lines
Quoted 12 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,

Quoted 12 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -4
+					    MG_PLL_TDC_TDCSEL(3);
 
-	if (refclk_khz != 38400) {
-		pll_state->mg_pll_tdc_coldst_bias |=
-			MG_PLL_TDC_COLDST_IREFINT_EN |
-

MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |

drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -3
-			MG_PLL_TDC_COLDST_COLDSTART |
-			MG_PLL_TDC_TDCOVCCORR_EN |
-			MG_PLL_TDC_TDCSEL(3);
+	pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
+				 MG_PLL_BIAS_INIT_DCOAMP(0x3F)

|

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,

Quoted 7 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -1
+				 MG_PLL_BIAS_BIAS_BONUS(10) |
+				 MG_PLL_BIAS_BIASCAL_EN |
+				 MG_PLL_BIAS_CTRIM(12) |
+				 MG_PLL_BIAS_VREF_RDAC(4) |
+				 MG_PLL_BIAS_IREFTRIM(iref_trim);
 
-		pll_state->mg_pll_bias =

MG_PLL_BIAS_BIAS_GB_SEL(3) |

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

-

MG_PLL_BIAS_INIT_DCOAMP(0x3F)

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

|
-

MG_PLL_BIAS_BIAS_BONUS(10) |

Quoted 1 line
Quoted 1 line
Quoted 1 line

drivers/gpu/drm/i915/intel_dpll_mgr.c

Quoted 1 line
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -1
-					 MG_PLL_BIAS_BIASCAL_EN

|

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c

Quoted 2 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -2
-					 MG_PLL_BIAS_CTRIM(12) |
-

MG_PLL_BIAS_VREF_RDAC(4) |

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

-

MG_PLL_BIAS_IREFTRIM(iref_trim);

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -0
+	if (refclk_khz == 38400) {
+		pll_state->mg_pll_tdc_coldst_bias_mask =

MG_PLL_TDC_COLDST_COLDSTART;

drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+		pll_state->mg_pll_bias_mask = 0;
+	} else {
+		pll_state->mg_pll_tdc_coldst_bias_mask = -1U;
+		pll_state->mg_pll_bias_mask = -1U;
 	}
 
+	pll_state->mg_pll_tdc_coldst_bias &= pll_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool icl_pll_get_hw_state(struct · line 2955

Quoted 8 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +2 -0
+	pll_state->mg_pll_bias &= pll_state->mg_pll_bias_mask;
+
 	return true;
 }
@@ -2948,9 +2954,21 @@ static bool icl_pll_get_hw_state(struct
 drm_i915_private *dev_priv,
 		hw_state->mg_pll_lf =

I915_READ(MG_PLL_LF(port));

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool icl_pll_get_hw_state(struct · line 2958

Quoted 3 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 		hw_state->mg_pll_frac_lock =
 I915_READ(MG_PLL_FRAC_LOCK(port));
 		hw_state->mg_pll_ssc =

I915_READ(MG_PLL_SSC(port));

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool icl_pll_get_hw_state(struct · line 2960

Quoted 2 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -0
+
 		hw_state->mg_pll_bias =

I915_READ(MG_PLL_BIAS(port));

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool icl_pll_get_hw_state(struct · line 2962

Quoted 2 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 		hw_state->mg_pll_tdc_coldst_bias =
 
	I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,

Quoted 7 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +6 -0
+
+		if (dev_priv->cdclk.hw.ref == 38400) {
+			hw_state->mg_pll_tdc_coldst_bias_mask =
 MG_PLL_TDC_COLDST_COLDSTART;
+			hw_state->mg_pll_bias_mask = 0;
+		} else {
+			hw_state->mg_pll_tdc_coldst_bias_mask =

-1U;

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c

Quoted 6 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +4 -0
+			hw_state->mg_pll_bias_mask = -1U;
+		}
+
+		hw_state->mg_pll_tdc_coldst_bias &= hw_state-
Quoted 1 line

mg_pll_tdc_coldst_bias_mask;

drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -0
+		hw_state->mg_pll_bias &= hw_state-

mg_pll_bias_mask;

Quoted 6 lines
Quoted 6 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct · line 2997

Quoted 6 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 		break;
 	default:
 		MISSING_CASE(id);
@@ -2978,6 +2996,7 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,  {
 	struct intel_dpll_hw_state *hw_state = &pll-

state.hw_state;

Quoted 27 lines
Quoted 4 lines
Quoted 4 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv,

Quoted 4 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -0
 	enum port port = icl_mg_pll_id_to_port(pll->info->id);
+	u32 val;
 
 	I915_WRITE(MG_REFCLKIN_CTL(port), hw_state-

mg_refclkin_ctl);

Quoted 22 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 	I915_WRITE(MG_CLKTOP2_CORECLKCTL1(port),
@@ -2988,9 +3007,17 @@ static void icl_mg_pll_write(struct
 drm_i915_private *dev_priv,
 	I915_WRITE(MG_PLL_LF(port), hw_state->mg_pll_lf);
 	I915_WRITE(MG_PLL_FRAC_LOCK(port), hw_state-
 >mg_pll_frac_lock);
Quoted 12 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3015

Quoted 10 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +6 -3
 	I915_WRITE(MG_PLL_SSC(port), hw_state->mg_pll_ssc);
-	I915_WRITE(MG_PLL_BIAS(port), hw_state->mg_pll_bias);
-	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port),
-		   hw_state->mg_pll_tdc_coldst_bias);
+
+	val = I915_READ(MG_PLL_BIAS(port));
+	val &= ~hw_state->mg_pll_bias_mask;
+	val |= hw_state->mg_pll_bias;
+	I915_WRITE(MG_PLL_BIAS(port), val);
+

Looks like we are writing the exact read value for pll_bias when ref_clk is 38400, we can skip this write or is it mandatory to write?

I'm still hoping that we'll get the proper programming steps for the 38.4MHz case too, at which point we can remove the special casing during the calculation step and write here the calculated values in all

cases.

Quoted 6 lines
Quoted 2 lines

It's also not a performance critical part, so I'd rather avoid adding more special casing.

Thanks for the clarification. I saw that algo in the bspec said to program only when ref_clk!=38400 and leave as default in 38400 case. In which case we wouldn't need the masks.

Quoted 8 lines

Yes, and that's what this code does, we leave this register unchanged in case of ref_clk=38.4MHz. We do need the masks as the generic way to say which fields in which registers to change. This function doesn't need to care about some other state outside of intel_dpll_hw_state.

Quoted 1 line

We can directly check the ref clk.

I'd like to have the full state included in intel_dpll_hw_state,

Ok. So the mask with 0 represents do not alter any bit of the register. Looks fine to me.
Thanks for the clarification.

as that is used

Quoted 40 lines

by the HW/SW state checker and the shared PLL framework to see if a PLL can be reused by doing a memcmp on this struct.

Quoted 2 lines

I think, may be we can leave the programming to default for 38400 and handle it when bspec gets updated with new calculation.

Yes, the patch leaves this register at its default in the 38.4MHz case.

Quoted 32 lines

The rest of the patch looks good to me.

Quoted 30 lines
Quoted 7 lines

drivers/gpu/drm/i915/intel_dpll_mgr.c static void icl_mg_pll_write(struct drm_i915_private *dev_priv, · line 3020

Quoted 6 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +5 -0
+	val = I915_READ(MG_PLL_TDC_COLDST_BIAS(port));
+	val &= ~hw_state->mg_pll_tdc_coldst_bias_mask;
+	val |= hw_state->mg_pll_tdc_coldst_bias;
+	I915_WRITE(MG_PLL_TDC_COLDST_BIAS(port), val);
+

Same here.

We have to write MG_PLL_TDC_COLDST_COLDSTART in all cases.

Quoted 20 lines
Quoted 19 lines
drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0
 	POSTING_READ(MG_PLL_TDC_COLDST_BIAS(port));

}

drivers/gpu/drm/i915/intel_dpll_mgr.h +2 -1
index ba925c7ee482..7e522cf4f13f 100644
@@ -180,6 +180,8 @@ struct intel_dpll_hw_state {
 	uint32_t mg_pll_ssc;
 	uint32_t mg_pll_bias;
 	uint32_t mg_pll_tdc_coldst_bias;
+	uint32_t mg_pll_bias_mask;
+	uint32_t mg_pll_tdc_coldst_bias_mask;
 };
 
 /**
--

2.13.2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#13 lore raw

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz (rev2)

In reply to Imre Deak
== Series Details ==

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz (rev2)
URL : https://patchwork.freedesktop.org/series/44836/
State : failure

== Summary ==

Applying: drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz
Applying: drm/i915/icl: Do read-modify-write as needed during MG PLL programming
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/i915_reg.h).
error: could not build fake ancestor
Patch failed at 0002 drm/i915/icl: Do read-modify-write as needed during MG PLL programming Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#14 lore raw

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz (rev3)

In reply to Imre Deak
== Series Details ==

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz (rev3)
URL : https://patchwork.freedesktop.org/series/44836/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4343 -> Patchwork_9360 =

== Summary - FAILURE ==

Serious unknown changes coming with Patchwork_9360 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_9360, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://patchwork.freedesktop.org/api/1.0/series/44836/revisions/3/mbox/

== Possible new issues ==

Here are the unknown changes that may have been introduced in Patchwork_9360:

=== IGT changes ===

    ==== Possible regressions ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-glk-j4005:       PASS -> FAIL

== Known issues ==

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

=== IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097

== Participating hosts (42 -> 38) ==

Missing (4): fi-ctg-p8600 fi-ilk-m540 fi-bsw-cyan fi-hsw-4200u

== Build changes ==

* Linux: CI_DRM_4343 -> Patchwork_9360

CI_DRM_4343: 93475d62c73031c5b4625eaa64b5c0f4079b2f3f @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4524: 9ab9268fa7eeda0a7ea6eb2ab02bb6c5b9c91ba0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9360: 01529685e7fbbd57d3f7de473799a45bcd0783e0 @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

01529685e7fb drm/i915/icl: Do read-modify-write as needed during MG PLL programming e89ce575e104 drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9360/issues.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

author
#15 lore raw

Re: ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz (rev3)

Quoted 5 lines · Patchwork

== Series Details ==

Series: series starting with [1/2] drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz (rev3)
URL : https://patchwork.freedesktop.org/series/44836/
State : failure

Thanks for the reviews pushed both patches to -dinq. See below for the explanation on failure.

Quoted 18 lines

== Summary ==

= CI Bug Log - changes from CI_DRM_4343 -> Patchwork_9360 =

== Summary - FAILURE ==

Serious unknown changes coming with Patchwork_9360 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in Patchwork_9360, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://patchwork.freedesktop.org/api/1.0/series/44836/revisions/3/mbox/

== Possible new issues ==

Quoted 8 lines

Here are the unknown changes that may have been introduced in Patchwork_9360:

=== IGT changes ===

    ==== Possible regressions ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-glk-j4005:       PASS -> FAIL

These changes are ICL specific, so should not affect GLK.

Quoted 42 lines

== Known issues ==

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

=== IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       DMESG-WARN (fdo#106000) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097

== Participating hosts (42 -> 38) ==

Missing (4): fi-ctg-p8600 fi-ilk-m540 fi-bsw-cyan fi-hsw-4200u

== Build changes ==

* Linux: CI_DRM_4343 -> Patchwork_9360

CI_DRM_4343: 93475d62c73031c5b4625eaa64b5c0f4079b2f3f @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4524: 9ab9268fa7eeda0a7ea6eb2ab02bb6c5b9c91ba0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9360: 01529685e7fbbd57d3f7de473799a45bcd0783e0 @ git://anongit.freedesktop.org/gfx-ci/linux

Quoted 8 lines

== Linux commits ==

01529685e7fb drm/i915/icl: Do read-modify-write as needed during MG PLL programming e89ce575e104 drm/i915/icl: Fix MG PLL setup when refclk is 38.4MHz

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9360/issues.html

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx