[PATCH v2 3/3] drm/i915/display: Cleanup mplla/mpllb selection

[PATCH v2 3/3] drm/i915/display: Cleanup mplla/mpllb selection

[Mika Kahola]

The function intel_c20_use_mplla() is not really
widely used and can be replaced with the more suitable

pll->tx[0] & C20_PHY_USE_MPLLB

expression. Let's remove the intel_c20_use_mplla()
alltogether and replace mplla/mpllb selection by
checking mpllb bit.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 39 ++++++++------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index fc7211675b2f..d0b6b4e439e1 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -2096,15 +2096,6 @@ int intel_cx0pll_calc_state(struct intel_crtc_state *crtc_state,
 	return intel_c20pll_calc_state(crtc_state, encoder);
 }
 
-static bool intel_c20_use_mplla(u32 clock)
-{
-	/* 10G and 20G rates use MPLLA */
-	if (clock == 1000000 || clock == 2000000)
-		return true;
-
-	return false;
-}
-
 static int intel_c20pll_calc_port_clock(struct intel_encoder *encoder,
 					const struct intel_c20pll_state *pll_state)
 {
@@ -2221,12 +2212,12 @@ void intel_c20pll_dump_hw_state(struct drm_i915_private *i915,
 	drm_dbg_kms(&i915->drm, "cmn[0] = 0x%.4x, cmn[1] = 0x%.4x, cmn[2] = 0x%.4x, cmn[3] = 0x%.4x\n",
 		    hw_state->cmn[0], hw_state->cmn[1], hw_state->cmn[2], hw_state->cmn[3]);
 
-	if (intel_c20_use_mplla(hw_state->clock)) {
-		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
-			drm_dbg_kms(&i915->drm, "mplla[%d] = 0x%.4x\n", i, hw_state->mplla[i]);
-	} else {
+	if (hw_state->tx[0] & C20_PHY_USE_MPLLB) {
 		for (i = 0; i < ARRAY_SIZE(hw_state->mpllb); i++)
 			drm_dbg_kms(&i915->drm, "mpllb[%d] = 0x%.4x\n", i, hw_state->mpllb[i]);
+	} else {
+		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
+			drm_dbg_kms(&i915->drm, "mplla[%d] = 0x%.4x\n", i, hw_state->mplla[i]);
 	}
 }
 
@@ -2373,27 +2364,27 @@ static void intel_c20_pll_program(struct drm_i915_private *i915,
 	}
 
 	/* 3.3 mpllb or mplla configuration */
-	if (intel_c20_use_mplla(clock)) {
-		for (i = 0; i < ARRAY_SIZE(pll_state->mplla); i++) {
+	if (pll_state->tx[0] & C20_PHY_USE_MPLLB) {
+		for (i = 0; i < ARRAY_SIZE(pll_state->mpllb); i++) {
 			if (cntx)
 				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
-						     PHY_C20_A_MPLLA_CNTX_CFG(i),
-						     pll_state->mplla[i]);
+						     PHY_C20_A_MPLLB_CNTX_CFG(i),
+						     pll_state->mpllb[i]);
 			else
 				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
-						     PHY_C20_B_MPLLA_CNTX_CFG(i),
-						     pll_state->mplla[i]);
+						     PHY_C20_B_MPLLB_CNTX_CFG(i),
+						     pll_state->mpllb[i]);
 		}
 	} else {
-		for (i = 0; i < ARRAY_SIZE(pll_state->mpllb); i++) {
+		for (i = 0; i < ARRAY_SIZE(pll_state->mplla); i++) {
 			if (cntx)
 				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
-						     PHY_C20_A_MPLLB_CNTX_CFG(i),
-						     pll_state->mpllb[i]);
+						     PHY_C20_A_MPLLA_CNTX_CFG(i),
+						     pll_state->mplla[i]);
 			else
 				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
-						     PHY_C20_B_MPLLB_CNTX_CFG(i),
-						     pll_state->mpllb[i]);
+						     PHY_C20_B_MPLLA_CNTX_CFG(i),
+						     pll_state->mplla[i]);
 		}
 	}
 
-- 
2.34.1

[PATCH v2 0/3] drm/i915/display: C20 clock state verification

[Mika Kahola]

Add pll selection check for C20 as well as
clock state verification0. We have been relying
on sw state to select A or B pll's. This is incorrect
as the hw might see this selection differently. This
patch fixes this shortcoming by reading pll selection
for both sw and hw states and compares if these two
selections match.

While at it, cleanup mpllb selection by removing intel_c20_use_mplla()
function as redundant.

Fixes: 59be90248b42 ("drm/i915/mtl: C20 state verification")

v2: reword commit message and include fix to a
    original commit (Imre)
    Compare pll selection (Jani)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Mika Kahola (3):
  drm/i915/display: Fix C20 pll selection for state verification
  drm/i915/display: Store hw clock for C20
  drm/i915/display: Cleanup mplla/mpllb selection

 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 153 ++++++++++---------
 1 file changed, 78 insertions(+), 75 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] drm/i915/display: Fix C20 pll selection for state verification

[Mika Kahola]

Add pll selection check for C20 as well as
clock state verification0. We have been relying
on sw state to select A or B pll's. This is incorrect
as the hw might see this selection differently. This
patch fixes this shortcoming by reading pll selection
for both sw and hw states and compares if these two
selections match.

Fixes: 59be90248b42 ("drm/i915/mtl: C20 state verification")

v2: reword commit message and include fix to a
    original commit (Imre)
    Compare pll selection (Jani)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 25 ++++++++++++--------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 884a1da36089..6b25e195232f 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -3067,24 +3067,29 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
 {
 	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
 	const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
-	bool use_mplla;
+	bool sw_use_mpllb = mpll_sw_state->tx[0] & C20_PHY_USE_MPLLB;
+	bool hw_use_mpllb = mpll_hw_state->tx[0] & C20_PHY_USE_MPLLB;
 	int i;
 
-	use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
-	if (use_mplla) {
-		for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
-			I915_STATE_WARN(i915, mpll_hw_state->mplla[i] != mpll_sw_state->mplla[i],
-					"[CRTC:%d:%s] mismatch in C20MPLLA: Register[%d] (expected 0x%04x, found 0x%04x)",
-					crtc->base.base.id, crtc->base.name, i,
-					mpll_sw_state->mplla[i], mpll_hw_state->mplla[i]);
-		}
-	} else {
+	I915_STATE_WARN(i915, sw_use_mpllb != hw_use_mpllb,
+			"[CRTC:%d:%s] mismatch in C20: Register MPLLB selection (expected %d, found %d)",
+			crtc->base.base.id, crtc->base.name,
+			sw_use_mpllb, hw_use_mpllb);
+
+	if (hw_use_mpllb) {
 		for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mpllb); i++) {
 			I915_STATE_WARN(i915, mpll_hw_state->mpllb[i] != mpll_sw_state->mpllb[i],
 					"[CRTC:%d:%s] mismatch in C20MPLLB: Register[%d] (expected 0x%04x, found 0x%04x)",
 					crtc->base.base.id, crtc->base.name, i,
 					mpll_sw_state->mpllb[i], mpll_hw_state->mpllb[i]);
 		}
+	} else {
+		for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
+			I915_STATE_WARN(i915, mpll_hw_state->mplla[i] != mpll_sw_state->mplla[i],
+					"[CRTC:%d:%s] mismatch in C20MPLLA: Register[%d] (expected 0x%04x, found 0x%04x)",
+					crtc->base.base.id, crtc->base.name, i,
+					mpll_sw_state->mplla[i], mpll_hw_state->mplla[i]);
+		}
 	}
 
 	for (i = 0; i < ARRAY_SIZE(mpll_sw_state->tx); i++) {
-- 
2.34.1

[PATCH v2 2/3] drm/i915/display: Store hw clock for C20

[Mika Kahola]

We can calculate the hw port clock during the hw readout
and store it as pll_state->clock for C20 state verification.
In order to do that we need to move intel_c20pll_calc_port_clock()
function.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 97 +++++++++++---------
 1 file changed, 52 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 6b25e195232f..fc7211675b2f 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -2105,6 +2105,51 @@ static bool intel_c20_use_mplla(u32 clock)
 	return false;
 }
 
+static int intel_c20pll_calc_port_clock(struct intel_encoder *encoder,
+					const struct intel_c20pll_state *pll_state)
+{
+	unsigned int frac, frac_en, frac_quot, frac_rem, frac_den;
+	unsigned int multiplier, refclk = 38400;
+	unsigned int tx_clk_div;
+	unsigned int ref_clk_mpllb_div;
+	unsigned int fb_clk_div4_en;
+	unsigned int ref, vco;
+	unsigned int tx_rate_mult;
+	unsigned int tx_rate = REG_FIELD_GET(C20_PHY_TX_RATE, pll_state->tx[0]);
+
+	if (pll_state->tx[0] & C20_PHY_USE_MPLLB) {
+		tx_rate_mult = 1;
+		frac_en = REG_FIELD_GET(C20_MPLLB_FRACEN, pll_state->mpllb[6]);
+		frac_quot = pll_state->mpllb[8];
+		frac_rem =  pll_state->mpllb[9];
+		frac_den =  pll_state->mpllb[7];
+		multiplier = REG_FIELD_GET(C20_MULTIPLIER_MASK, pll_state->mpllb[0]);
+		tx_clk_div = REG_FIELD_GET(C20_MPLLB_TX_CLK_DIV_MASK, pll_state->mpllb[0]);
+		ref_clk_mpllb_div = REG_FIELD_GET(C20_REF_CLK_MPLLB_DIV_MASK, pll_state->mpllb[6]);
+		fb_clk_div4_en = 0;
+	} else {
+		tx_rate_mult = 2;
+		frac_en = REG_FIELD_GET(C20_MPLLA_FRACEN, pll_state->mplla[6]);
+		frac_quot = pll_state->mplla[8];
+		frac_rem =  pll_state->mplla[9];
+		frac_den =  pll_state->mplla[7];
+		multiplier = REG_FIELD_GET(C20_MULTIPLIER_MASK, pll_state->mplla[0]);
+		tx_clk_div = REG_FIELD_GET(C20_MPLLA_TX_CLK_DIV_MASK, pll_state->mplla[1]);
+		ref_clk_mpllb_div = REG_FIELD_GET(C20_REF_CLK_MPLLB_DIV_MASK, pll_state->mplla[6]);
+		fb_clk_div4_en = REG_FIELD_GET(C20_FB_CLK_DIV4_EN, pll_state->mplla[0]);
+	}
+
+	if (frac_en)
+		frac = frac_quot + DIV_ROUND_CLOSEST(frac_rem, frac_den);
+	else
+		frac = 0;
+
+	ref = DIV_ROUND_CLOSEST(refclk * (1 << (1 + fb_clk_div4_en)), 1 << ref_clk_mpllb_div);
+	vco = DIV_ROUND_CLOSEST_ULL(mul_u32_u32(ref, (multiplier << (17 - 2)) + frac) >> 17, 10);
+
+	return vco << tx_rate_mult >> tx_clk_div >> tx_rate;
+}
+
 static void intel_c20pll_readout_hw_state(struct intel_encoder *encoder,
 					  struct intel_c20pll_state *pll_state)
 {
@@ -2160,6 +2205,8 @@ static void intel_c20pll_readout_hw_state(struct intel_encoder *encoder,
 		}
 	}
 
+	pll_state->clock = intel_c20pll_calc_port_clock(encoder, pll_state);
+
 	intel_cx0_phy_transaction_end(encoder, wakeref);
 }
 
@@ -2408,51 +2455,6 @@ static int intel_c10pll_calc_port_clock(struct intel_encoder *encoder,
 	return tmpclk;
 }
 
-static int intel_c20pll_calc_port_clock(struct intel_encoder *encoder,
-					const struct intel_c20pll_state *pll_state)
-{
-	unsigned int frac, frac_en, frac_quot, frac_rem, frac_den;
-	unsigned int multiplier, refclk = 38400;
-	unsigned int tx_clk_div;
-	unsigned int ref_clk_mpllb_div;
-	unsigned int fb_clk_div4_en;
-	unsigned int ref, vco;
-	unsigned int tx_rate_mult;
-	unsigned int tx_rate = REG_FIELD_GET(C20_PHY_TX_RATE, pll_state->tx[0]);
-
-	if (pll_state->tx[0] & C20_PHY_USE_MPLLB) {
-		tx_rate_mult = 1;
-		frac_en = REG_FIELD_GET(C20_MPLLB_FRACEN, pll_state->mpllb[6]);
-		frac_quot = pll_state->mpllb[8];
-		frac_rem =  pll_state->mpllb[9];
-		frac_den =  pll_state->mpllb[7];
-		multiplier = REG_FIELD_GET(C20_MULTIPLIER_MASK, pll_state->mpllb[0]);
-		tx_clk_div = REG_FIELD_GET(C20_MPLLB_TX_CLK_DIV_MASK, pll_state->mpllb[0]);
-		ref_clk_mpllb_div = REG_FIELD_GET(C20_REF_CLK_MPLLB_DIV_MASK, pll_state->mpllb[6]);
-		fb_clk_div4_en = 0;
-	} else {
-		tx_rate_mult = 2;
-		frac_en = REG_FIELD_GET(C20_MPLLA_FRACEN, pll_state->mplla[6]);
-		frac_quot = pll_state->mplla[8];
-		frac_rem =  pll_state->mplla[9];
-		frac_den =  pll_state->mplla[7];
-		multiplier = REG_FIELD_GET(C20_MULTIPLIER_MASK, pll_state->mplla[0]);
-		tx_clk_div = REG_FIELD_GET(C20_MPLLA_TX_CLK_DIV_MASK, pll_state->mplla[1]);
-		ref_clk_mpllb_div = REG_FIELD_GET(C20_REF_CLK_MPLLB_DIV_MASK, pll_state->mplla[6]);
-		fb_clk_div4_en = REG_FIELD_GET(C20_FB_CLK_DIV4_EN, pll_state->mplla[0]);
-	}
-
-	if (frac_en)
-		frac = frac_quot + DIV_ROUND_CLOSEST(frac_rem, frac_den);
-	else
-		frac = 0;
-
-	ref = DIV_ROUND_CLOSEST(refclk * (1 << (1 + fb_clk_div4_en)), 1 << ref_clk_mpllb_div);
-	vco = DIV_ROUND_CLOSEST_ULL(mul_u32_u32(ref, (multiplier << (17 - 2)) + frac) >> 17, 10);
-
-	return vco << tx_rate_mult >> tx_clk_div >> tx_rate;
-}
-
 static void intel_program_port_clock_ctl(struct intel_encoder *encoder,
 					 const struct intel_crtc_state *crtc_state,
 					 bool lane_reversal)
@@ -3071,6 +3073,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
 	bool hw_use_mpllb = mpll_hw_state->tx[0] & C20_PHY_USE_MPLLB;
 	int i;
 
+	I915_STATE_WARN(i915, mpll_hw_state->clock != mpll_sw_state->clock,
+			"[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
+			crtc->base.base.id, crtc->base.name,
+			mpll_sw_state->clock, mpll_hw_state->clock);
+
 	I915_STATE_WARN(i915, sw_use_mpllb != hw_use_mpllb,
 			"[CRTC:%d:%s] mismatch in C20: Register MPLLB selection (expected %d, found %d)",
 			crtc->base.base.id, crtc->base.name,
-- 
2.34.1

[PATCH v2 3/3] drm/i915/display: Cleanup mplla/mpllb selection

[Mika Kahola]

The function intel_c20_use_mplla() is not really
widely used and can be replaced with the more suitable

pll->tx[0] & C20_PHY_USE_MPLLB

expression. Let's remove the intel_c20_use_mplla()
alltogether and replace mplla/mpllb selection by
checking mpllb bit.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 39 ++++++++------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index fc7211675b2f..d0b6b4e439e1 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -2096,15 +2096,6 @@ int intel_cx0pll_calc_state(struct intel_crtc_state *crtc_state,
 	return intel_c20pll_calc_state(crtc_state, encoder);
 }
 
-static bool intel_c20_use_mplla(u32 clock)
-{
-	/* 10G and 20G rates use MPLLA */
-	if (clock == 1000000 || clock == 2000000)
-		return true;
-
-	return false;
-}
-
 static int intel_c20pll_calc_port_clock(struct intel_encoder *encoder,
 					const struct intel_c20pll_state *pll_state)
 {
@@ -2221,12 +2212,12 @@ void intel_c20pll_dump_hw_state(struct drm_i915_private *i915,
 	drm_dbg_kms(&i915->drm, "cmn[0] = 0x%.4x, cmn[1] = 0x%.4x, cmn[2] = 0x%.4x, cmn[3] = 0x%.4x\n",
 		    hw_state->cmn[0], hw_state->cmn[1], hw_state->cmn[2], hw_state->cmn[3]);
 
-	if (intel_c20_use_mplla(hw_state->clock)) {
-		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
-			drm_dbg_kms(&i915->drm, "mplla[%d] = 0x%.4x\n", i, hw_state->mplla[i]);
-	} else {
+	if (hw_state->tx[0] & C20_PHY_USE_MPLLB) {
 		for (i = 0; i < ARRAY_SIZE(hw_state->mpllb); i++)
 			drm_dbg_kms(&i915->drm, "mpllb[%d] = 0x%.4x\n", i, hw_state->mpllb[i]);
+	} else {
+		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
+			drm_dbg_kms(&i915->drm, "mplla[%d] = 0x%.4x\n", i, hw_state->mplla[i]);
 	}
 }
 
@@ -2373,27 +2364,27 @@ static void intel_c20_pll_program(struct drm_i915_private *i915,
 	}
 
 	/* 3.3 mpllb or mplla configuration */
-	if (intel_c20_use_mplla(clock)) {
-		for (i = 0; i < ARRAY_SIZE(pll_state->mplla); i++) {
+	if (pll_state->tx[0] & C20_PHY_USE_MPLLB) {
+		for (i = 0; i < ARRAY_SIZE(pll_state->mpllb); i++) {
 			if (cntx)
 				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
-						     PHY_C20_A_MPLLA_CNTX_CFG(i),
-						     pll_state->mplla[i]);
+						     PHY_C20_A_MPLLB_CNTX_CFG(i),
+						     pll_state->mpllb[i]);
 			else
 				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
-						     PHY_C20_B_MPLLA_CNTX_CFG(i),
-						     pll_state->mplla[i]);
+						     PHY_C20_B_MPLLB_CNTX_CFG(i),
+						     pll_state->mpllb[i]);
 		}
 	} else {
-		for (i = 0; i < ARRAY_SIZE(pll_state->mpllb); i++) {
+		for (i = 0; i < ARRAY_SIZE(pll_state->mplla); i++) {
 			if (cntx)
 				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
-						     PHY_C20_A_MPLLB_CNTX_CFG(i),
-						     pll_state->mpllb[i]);
+						     PHY_C20_A_MPLLA_CNTX_CFG(i),
+						     pll_state->mplla[i]);
 			else
 				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
-						     PHY_C20_B_MPLLB_CNTX_CFG(i),
-						     pll_state->mpllb[i]);
+						     PHY_C20_B_MPLLA_CNTX_CFG(i),
+						     pll_state->mplla[i]);
 		}
 	}
 
-- 
2.34.1

Re: [PATCH v2 3/3] drm/i915/display: Cleanup mplla/mpllb selection

[Imre Deak]

On Tue, Jan 02, 2024 at 01:57:41PM +0200, Mika Kahola wrote:
> The function intel_c20_use_mplla() is not really
> widely used and can be replaced with the more suitable
> 
> pll->tx[0] & C20_PHY_USE_MPLLB
> 
> expression. Let's remove the intel_c20_use_mplla()
> alltogether and replace mplla/mpllb selection by
> checking mpllb bit.
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 39 ++++++++------------
>  1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index fc7211675b2f..d0b6b4e439e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2096,15 +2096,6 @@ int intel_cx0pll_calc_state(struct intel_crtc_state *crtc_state,
>  	return intel_c20pll_calc_state(crtc_state, encoder);
>  }
>  
> -static bool intel_c20_use_mplla(u32 clock)
> -{
> -	/* 10G and 20G rates use MPLLA */
> -	if (clock == 1000000 || clock == 2000000)
> -		return true;
> -
> -	return false;
> -}
> -
>  static int intel_c20pll_calc_port_clock(struct intel_encoder *encoder,
>  					const struct intel_c20pll_state *pll_state)
>  {
> @@ -2221,12 +2212,12 @@ void intel_c20pll_dump_hw_state(struct drm_i915_private *i915,
>  	drm_dbg_kms(&i915->drm, "cmn[0] = 0x%.4x, cmn[1] = 0x%.4x, cmn[2] = 0x%.4x, cmn[3] = 0x%.4x\n",
>  		    hw_state->cmn[0], hw_state->cmn[1], hw_state->cmn[2], hw_state->cmn[3]);
>  
> -	if (intel_c20_use_mplla(hw_state->clock)) {
> -		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
> -			drm_dbg_kms(&i915->drm, "mplla[%d] = 0x%.4x\n", i, hw_state->mplla[i]);
> -	} else {
> +	if (hw_state->tx[0] & C20_PHY_USE_MPLLB) {

The above could be in a helper.

>  		for (i = 0; i < ARRAY_SIZE(hw_state->mpllb); i++)
>  			drm_dbg_kms(&i915->drm, "mpllb[%d] = 0x%.4x\n", i, hw_state->mpllb[i]);
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
> +			drm_dbg_kms(&i915->drm, "mplla[%d] = 0x%.4x\n", i, hw_state->mplla[i]);
>  	}
>  }
>  
> @@ -2373,27 +2364,27 @@ static void intel_c20_pll_program(struct drm_i915_private *i915,
>  	}
>  
>  	/* 3.3 mpllb or mplla configuration */
> -	if (intel_c20_use_mplla(clock)) {
> -		for (i = 0; i < ARRAY_SIZE(pll_state->mplla); i++) {
> +	if (pll_state->tx[0] & C20_PHY_USE_MPLLB) {
> +		for (i = 0; i < ARRAY_SIZE(pll_state->mpllb); i++) {
>  			if (cntx)
>  				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
> -						     PHY_C20_A_MPLLA_CNTX_CFG(i),
> -						     pll_state->mplla[i]);
> +						     PHY_C20_A_MPLLB_CNTX_CFG(i),
> +						     pll_state->mpllb[i]);

Imo, at one point intel_c20pll_state should be converted to use only one
mpll array instead of mplla/b and define a PHY_C20_MPLL_CNTX_CFG(cntx, pll, idx)
macro instead of the above ones.

The patchset looks ok:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  			else
>  				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
> -						     PHY_C20_B_MPLLA_CNTX_CFG(i),
> -						     pll_state->mplla[i]);
> +						     PHY_C20_B_MPLLB_CNTX_CFG(i),
> +						     pll_state->mpllb[i]);
>  		}
>  	} else {
> -		for (i = 0; i < ARRAY_SIZE(pll_state->mpllb); i++) {
> +		for (i = 0; i < ARRAY_SIZE(pll_state->mplla); i++) {
>  			if (cntx)
>  				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
> -						     PHY_C20_A_MPLLB_CNTX_CFG(i),
> -						     pll_state->mpllb[i]);
> +						     PHY_C20_A_MPLLA_CNTX_CFG(i),
> +						     pll_state->mplla[i]);
>  			else
>  				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
> -						     PHY_C20_B_MPLLB_CNTX_CFG(i),
> -						     pll_state->mpllb[i]);
> +						     PHY_C20_B_MPLLA_CNTX_CFG(i),
> +						     pll_state->mplla[i]);
>  		}
>  	}
>  
> -- 
> 2.34.1
> 

RE: [PATCH v2 3/3] drm/i915/display: Cleanup mplla/mpllb selection

[Kahola, Mika]

> -----Original Message-----
> From: Deak, Imre <imre.deak@intel.com>
> Sent: Wednesday, January 3, 2024 6:16 PM
> To: Kahola, Mika <mika.kahola@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v2 3/3] drm/i915/display: Cleanup mplla/mpllb selection
> 
> On Tue, Jan 02, 2024 at 01:57:41PM +0200, Mika Kahola wrote:
> > The function intel_c20_use_mplla() is not really widely used and can
> > be replaced with the more suitable
> >
> > pll->tx[0] & C20_PHY_USE_MPLLB
> >
> > expression. Let's remove the intel_c20_use_mplla() alltogether and
> > replace mplla/mpllb selection by checking mpllb bit.
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 39
> > ++++++++------------
> >  1 file changed, 15 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index fc7211675b2f..d0b6b4e439e1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -2096,15 +2096,6 @@ int intel_cx0pll_calc_state(struct intel_crtc_state *crtc_state,
> >  	return intel_c20pll_calc_state(crtc_state, encoder);  }
> >
> > -static bool intel_c20_use_mplla(u32 clock) -{
> > -	/* 10G and 20G rates use MPLLA */
> > -	if (clock == 1000000 || clock == 2000000)
> > -		return true;
> > -
> > -	return false;
> > -}
> > -
> >  static int intel_c20pll_calc_port_clock(struct intel_encoder *encoder,
> >  					const struct intel_c20pll_state *pll_state)  { @@ -2221,12
> > +2212,12 @@ void intel_c20pll_dump_hw_state(struct drm_i915_private *i915,
> >  	drm_dbg_kms(&i915->drm, "cmn[0] = 0x%.4x, cmn[1] = 0x%.4x, cmn[2] = 0x%.4x, cmn[3] = 0x%.4x\n",
> >  		    hw_state->cmn[0], hw_state->cmn[1], hw_state->cmn[2],
> > hw_state->cmn[3]);
> >
> > -	if (intel_c20_use_mplla(hw_state->clock)) {
> > -		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
> > -			drm_dbg_kms(&i915->drm, "mplla[%d] = 0x%.4x\n", i, hw_state->mplla[i]);
> > -	} else {
> > +	if (hw_state->tx[0] & C20_PHY_USE_MPLLB) {
> 
> The above could be in a helper.

I was thinking about to put this on helper but then decided to go with this approach. There are couple of places elsewhere where this line is used as well. I will provide a follow up patch to get these defined as helper functions.

> 
> >  		for (i = 0; i < ARRAY_SIZE(hw_state->mpllb); i++)
> >  			drm_dbg_kms(&i915->drm, "mpllb[%d] = 0x%.4x\n", i,
> > hw_state->mpllb[i]);
> > +	} else {
> > +		for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
> > +			drm_dbg_kms(&i915->drm, "mplla[%d] = 0x%.4x\n", i,
> > +hw_state->mplla[i]);
> >  	}
> >  }
> >
> > @@ -2373,27 +2364,27 @@ static void intel_c20_pll_program(struct drm_i915_private *i915,
> >  	}
> >
> >  	/* 3.3 mpllb or mplla configuration */
> > -	if (intel_c20_use_mplla(clock)) {
> > -		for (i = 0; i < ARRAY_SIZE(pll_state->mplla); i++) {
> > +	if (pll_state->tx[0] & C20_PHY_USE_MPLLB) {
> > +		for (i = 0; i < ARRAY_SIZE(pll_state->mpllb); i++) {
> >  			if (cntx)
> >  				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
> > -						     PHY_C20_A_MPLLA_CNTX_CFG(i),
> > -						     pll_state->mplla[i]);
> > +						     PHY_C20_A_MPLLB_CNTX_CFG(i),
> > +						     pll_state->mpllb[i]);
> 
> Imo, at one point intel_c20pll_state should be converted to use only one mpll array instead of mplla/b and define a
> PHY_C20_MPLL_CNTX_CFG(cntx, pll, idx) macro instead of the above ones.
> 

Thanks for the review!

> The patchset looks ok:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> >  			else
> >  				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
> > -						     PHY_C20_B_MPLLA_CNTX_CFG(i),
> > -						     pll_state->mplla[i]);
> > +						     PHY_C20_B_MPLLB_CNTX_CFG(i),
> > +						     pll_state->mpllb[i]);
> >  		}
> >  	} else {
> > -		for (i = 0; i < ARRAY_SIZE(pll_state->mpllb); i++) {
> > +		for (i = 0; i < ARRAY_SIZE(pll_state->mplla); i++) {
> >  			if (cntx)
> >  				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
> > -						     PHY_C20_A_MPLLB_CNTX_CFG(i),
> > -						     pll_state->mpllb[i]);
> > +						     PHY_C20_A_MPLLA_CNTX_CFG(i),
> > +						     pll_state->mplla[i]);
> >  			else
> >  				intel_c20_sram_write(i915, encoder->port, INTEL_CX0_LANE0,
> > -						     PHY_C20_B_MPLLB_CNTX_CFG(i),
> > -						     pll_state->mpllb[i]);
> > +						     PHY_C20_B_MPLLA_CNTX_CFG(i),
> > +						     pll_state->mplla[i]);
> >  		}
> >  	}
> >
> > --
> > 2.34.1
> >