| Message ID | 20240102115741.118525-4-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: C20 clock state verification | expand |
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 >
> -----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 > >
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]); } }
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(-)