diff mbox series

[13/15] drm/i915: Split alds/rkl from icl_ddi_combo_{enable, disable}_clock()

Message ID 20210201183343.15292-14-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Clean up the DDI clock routing mess | expand

Commit Message

Ville Syrjälä Feb. 1, 2021, 6:33 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since .{enable,disable}_clock() are already vfuncs it's a bit silly to
have if-ladders inside them. Just provide specialized version for adlp
and rkl so we don't need any of that.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 93 ++++++++++++++++--------
 1 file changed, 62 insertions(+), 31 deletions(-)

Comments

Lucas De Marchi Feb. 1, 2021, 7:22 p.m. UTC | #1 
On Mon, Feb 01, 2021 at 08:33:41PM +0200, Ville Syrjälä wrote:
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>Since .{enable,disable}_clock() are already vfuncs it's a bit silly to
>have if-ladders inside them. Just provide specialized version for adlp
>and rkl so we don't need any of that.

s/alds/adl-s/

s/adlp/adl-s/


>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/display/intel_ddi.c | 93 ++++++++++++++++--------
> 1 file changed, 62 insertions(+), 31 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>index 1bd2aa86183d..bafb754d1b66 100644
>--- a/drivers/gpu/drm/i915/display/intel_ddi.c
>+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>@@ -3127,28 +3127,6 @@ static u32 icl_dpclka_cfgcr0_clk_off(struct drm_i915_private *dev_priv,
> 	return 0;
> }
>
>-static u32 icl_dpclka_cfgcr0_clk_sel(struct drm_i915_private *dev_priv,
>-				     enum intel_dpll_id id, enum phy phy)

ok, but why do we even add them in this series if we are going to
remove. Just makes it harder to review.

For this end state:


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>-{
>-	if (IS_ALDERLAKE_S(dev_priv))
>-		return id << ADLS_DPCLKA_CFGCR_DDI_SHIFT(phy);
>-	else if (IS_ROCKETLAKE(dev_priv))
>-		return RKL_DPCLKA_CFGCR0_DDI_CLK_SEL(id, phy);
>-	else
>-		return ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(id, phy);
>-}
>-
>-static u32 icl_dpclka_cfgcr0_clk_sel_mask(struct drm_i915_private *dev_priv,
>-					  enum phy phy)
>-{
>-	if (IS_ALDERLAKE_S(dev_priv))
>-		return ADLS_DPCLKA_CFGCR_DDI_CLK_SEL_MASK(phy);
>-	else if (IS_ROCKETLAKE(dev_priv))
>-		return RKL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
>-	else
>-		return ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
>-}
>-
> static i915_reg_t icl_dpclka_cfgcr0_reg(struct drm_i915_private *i915,
> 					enum phy phy)
> {
>@@ -3184,6 +3162,56 @@ static void _cnl_ddi_disable_clock(struct drm_i915_private *i915, i915_reg_t reg
> 	mutex_unlock(&i915->dpll.lock);
> }
>
>+static void adls_ddi_enable_clock(struct intel_encoder *encoder,
>+				  const struct intel_crtc_state *crtc_state)
>+{
>+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>+	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>+
>+	if (drm_WARN_ON(&dev_priv->drm, !pll))
>+		return;
>+
>+	_cnl_ddi_enable_clock(dev_priv, ADLS_DPCLKA_CFGCR(phy),
>+			      ADLS_DPCLKA_CFGCR_DDI_CLK_SEL_MASK(phy),
>+			      pll->info->id << ADLS_DPCLKA_CFGCR_DDI_SHIFT(phy),
>+			      ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
>+}
>+
>+static void adls_ddi_disable_clock(struct intel_encoder *encoder)
>+{
>+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>+
>+	_cnl_ddi_disable_clock(dev_priv, ADLS_DPCLKA_CFGCR(phy),
>+			       ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
>+}
>+
>+static void rkl_ddi_enable_clock(struct intel_encoder *encoder,
>+				 const struct intel_crtc_state *crtc_state)
>+{
>+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>+	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>+
>+	if (drm_WARN_ON(&dev_priv->drm, !pll))
>+		return;
>+
>+	_cnl_ddi_enable_clock(dev_priv, ICL_DPCLKA_CFGCR0,
>+			      RKL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy),
>+			      RKL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, phy),
>+			      RKL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
>+}
>+
>+static void rkl_ddi_disable_clock(struct intel_encoder *encoder)
>+{
>+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>+
>+	_cnl_ddi_disable_clock(dev_priv, ICL_DPCLKA_CFGCR0,
>+			       RKL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
>+}
>+
> static void dg1_ddi_enable_clock(struct intel_encoder *encoder,
> 				 const struct intel_crtc_state *crtc_state)
> {
>@@ -3228,10 +3256,10 @@ static void icl_ddi_combo_enable_clock(struct intel_encoder *encoder,
> 	if (drm_WARN_ON(&dev_priv->drm, !pll))
> 		return;
>
>-	_cnl_ddi_enable_clock(dev_priv, icl_dpclka_cfgcr0_reg(dev_priv, phy),
>-			      icl_dpclka_cfgcr0_clk_sel_mask(dev_priv, phy),
>-			      icl_dpclka_cfgcr0_clk_sel(dev_priv, pll->info->id, phy),
>-			      icl_dpclka_cfgcr0_clk_off(dev_priv, phy));
>+	_cnl_ddi_enable_clock(dev_priv, ICL_DPCLKA_CFGCR0,
>+			      ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy),
>+			      ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, phy),
>+			      ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
> }
>
> static void icl_ddi_combo_disable_clock(struct intel_encoder *encoder)
>@@ -3239,8 +3267,8 @@ static void icl_ddi_combo_disable_clock(struct intel_encoder *encoder)
> 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> 	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>
>-	_cnl_ddi_disable_clock(dev_priv, icl_dpclka_cfgcr0_reg(dev_priv, phy),
>-			       icl_dpclka_cfgcr0_clk_off(dev_priv, phy));
>+	_cnl_ddi_disable_clock(dev_priv, ICL_DPCLKA_CFGCR0,
>+			       ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
> }
>
> static void dg1_sanitize_port_clk_off(struct drm_i915_private *dev_priv,
>@@ -5654,9 +5682,12 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> 	encoder->cloneable = 0;
> 	encoder->pipe_mask = ~0;
>
>-	if (IS_ALDERLAKE_S(dev_priv) || IS_ROCKETLAKE(dev_priv)) {
>-		encoder->enable_clock = icl_ddi_combo_enable_clock;
>-		encoder->disable_clock = icl_ddi_combo_disable_clock;
>+	if (IS_ALDERLAKE_S(dev_priv)) {
>+		encoder->enable_clock = adls_ddi_enable_clock;
>+		encoder->disable_clock = adls_ddi_disable_clock;
>+	} else if (IS_ROCKETLAKE(dev_priv)) {
>+		encoder->enable_clock = rkl_ddi_enable_clock;
>+		encoder->disable_clock = rkl_ddi_disable_clock;
> 	} else if (IS_DG1(dev_priv)) {
> 		encoder->enable_clock = dg1_ddi_enable_clock;
> 		encoder->disable_clock = dg1_ddi_disable_clock;
>-- 
>2.26.2
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Feb. 1, 2021, 7:31 p.m. UTC | #2 
On Mon, Feb 01, 2021 at 11:22:39AM -0800, Lucas De Marchi wrote:
> On Mon, Feb 01, 2021 at 08:33:41PM +0200, Ville Syrjälä wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Since .{enable,disable}_clock() are already vfuncs it's a bit silly to
> >have if-ladders inside them. Just provide specialized version for adlp
> >and rkl so we don't need any of that.
> 
> s/alds/adl-s/
> 
> s/adlp/adl-s/
> 
> 
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 93 ++++++++++++++++--------
> > 1 file changed, 62 insertions(+), 31 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >index 1bd2aa86183d..bafb754d1b66 100644
> >--- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >@@ -3127,28 +3127,6 @@ static u32 icl_dpclka_cfgcr0_clk_off(struct drm_i915_private *dev_priv,
> > 	return 0;
> > }
> >
> >-static u32 icl_dpclka_cfgcr0_clk_sel(struct drm_i915_private *dev_priv,
> >-				     enum intel_dpll_id id, enum phy phy)
> 
> ok, but why do we even add them in this series if we are going to
> remove. Just makes it harder to review.

I had to increase the SNR before I could see what the code was
trying to do. I guess I could now go back and drop the first
two patches.
Ville Syrjälä Feb. 1, 2021, 7:38 p.m. UTC | #3 
On Mon, Feb 01, 2021 at 09:31:49PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 01, 2021 at 11:22:39AM -0800, Lucas De Marchi wrote:
> > On Mon, Feb 01, 2021 at 08:33:41PM +0200, Ville Syrjälä wrote:
> > >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > >Since .{enable,disable}_clock() are already vfuncs it's a bit silly to
> > >have if-ladders inside them. Just provide specialized version for adlp
> > >and rkl so we don't need any of that.
> > 
> > s/alds/adl-s/
> > 
> > s/adlp/adl-s/
> > 
> > 
> > >
> > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >---
> > > drivers/gpu/drm/i915/display/intel_ddi.c | 93 ++++++++++++++++--------
> > > 1 file changed, 62 insertions(+), 31 deletions(-)
> > >
> > >diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > >index 1bd2aa86183d..bafb754d1b66 100644
> > >--- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > >+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > >@@ -3127,28 +3127,6 @@ static u32 icl_dpclka_cfgcr0_clk_off(struct drm_i915_private *dev_priv,
> > > 	return 0;
> > > }
> > >
> > >-static u32 icl_dpclka_cfgcr0_clk_sel(struct drm_i915_private *dev_priv,
> > >-				     enum intel_dpll_id id, enum phy phy)
> > 
> > ok, but why do we even add them in this series if we are going to
> > remove. Just makes it harder to review.
> 
> I had to increase the SNR before I could see what the code was
> trying to do. I guess I could now go back and drop the first
> two patches.

One counter argument would be that we already had
icl_dpclka_cfgcr0_clk_off(), so not unifying the approach first
means the other refactorings have to deal with two different
styles, and thus could end up looking even more confusing.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 1bd2aa86183d..bafb754d1b66 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3127,28 +3127,6 @@  static u32 icl_dpclka_cfgcr0_clk_off(struct drm_i915_private *dev_priv,
 	return 0;
 }
 
-static u32 icl_dpclka_cfgcr0_clk_sel(struct drm_i915_private *dev_priv,
-				     enum intel_dpll_id id, enum phy phy)
-{
-	if (IS_ALDERLAKE_S(dev_priv))
-		return id << ADLS_DPCLKA_CFGCR_DDI_SHIFT(phy);
-	else if (IS_ROCKETLAKE(dev_priv))
-		return RKL_DPCLKA_CFGCR0_DDI_CLK_SEL(id, phy);
-	else
-		return ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(id, phy);
-}
-
-static u32 icl_dpclka_cfgcr0_clk_sel_mask(struct drm_i915_private *dev_priv,
-					  enum phy phy)
-{
-	if (IS_ALDERLAKE_S(dev_priv))
-		return ADLS_DPCLKA_CFGCR_DDI_CLK_SEL_MASK(phy);
-	else if (IS_ROCKETLAKE(dev_priv))
-		return RKL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
-	else
-		return ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy);
-}
-
 static i915_reg_t icl_dpclka_cfgcr0_reg(struct drm_i915_private *i915,
 					enum phy phy)
 {
@@ -3184,6 +3162,56 @@  static void _cnl_ddi_disable_clock(struct drm_i915_private *i915, i915_reg_t reg
 	mutex_unlock(&i915->dpll.lock);
 }
 
+static void adls_ddi_enable_clock(struct intel_encoder *encoder,
+				  const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+
+	if (drm_WARN_ON(&dev_priv->drm, !pll))
+		return;
+
+	_cnl_ddi_enable_clock(dev_priv, ADLS_DPCLKA_CFGCR(phy),
+			      ADLS_DPCLKA_CFGCR_DDI_CLK_SEL_MASK(phy),
+			      pll->info->id << ADLS_DPCLKA_CFGCR_DDI_SHIFT(phy),
+			      ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
+}
+
+static void adls_ddi_disable_clock(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+
+	_cnl_ddi_disable_clock(dev_priv, ADLS_DPCLKA_CFGCR(phy),
+			       ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
+}
+
+static void rkl_ddi_enable_clock(struct intel_encoder *encoder,
+				 const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	const struct intel_shared_dpll *pll = crtc_state->shared_dpll;
+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+
+	if (drm_WARN_ON(&dev_priv->drm, !pll))
+		return;
+
+	_cnl_ddi_enable_clock(dev_priv, ICL_DPCLKA_CFGCR0,
+			      RKL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy),
+			      RKL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, phy),
+			      RKL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
+}
+
+static void rkl_ddi_disable_clock(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
+
+	_cnl_ddi_disable_clock(dev_priv, ICL_DPCLKA_CFGCR0,
+			       RKL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
+}
+
 static void dg1_ddi_enable_clock(struct intel_encoder *encoder,
 				 const struct intel_crtc_state *crtc_state)
 {
@@ -3228,10 +3256,10 @@  static void icl_ddi_combo_enable_clock(struct intel_encoder *encoder,
 	if (drm_WARN_ON(&dev_priv->drm, !pll))
 		return;
 
-	_cnl_ddi_enable_clock(dev_priv, icl_dpclka_cfgcr0_reg(dev_priv, phy),
-			      icl_dpclka_cfgcr0_clk_sel_mask(dev_priv, phy),
-			      icl_dpclka_cfgcr0_clk_sel(dev_priv, pll->info->id, phy),
-			      icl_dpclka_cfgcr0_clk_off(dev_priv, phy));
+	_cnl_ddi_enable_clock(dev_priv, ICL_DPCLKA_CFGCR0,
+			      ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy),
+			      ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, phy),
+			      ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
 }
 
 static void icl_ddi_combo_disable_clock(struct intel_encoder *encoder)
@@ -3239,8 +3267,8 @@  static void icl_ddi_combo_disable_clock(struct intel_encoder *encoder)
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
 
-	_cnl_ddi_disable_clock(dev_priv, icl_dpclka_cfgcr0_reg(dev_priv, phy),
-			       icl_dpclka_cfgcr0_clk_off(dev_priv, phy));
+	_cnl_ddi_disable_clock(dev_priv, ICL_DPCLKA_CFGCR0,
+			       ICL_DPCLKA_CFGCR0_DDI_CLK_OFF(phy));
 }
 
 static void dg1_sanitize_port_clk_off(struct drm_i915_private *dev_priv,
@@ -5654,9 +5682,12 @@  void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	encoder->cloneable = 0;
 	encoder->pipe_mask = ~0;
 
-	if (IS_ALDERLAKE_S(dev_priv) || IS_ROCKETLAKE(dev_priv)) {
-		encoder->enable_clock = icl_ddi_combo_enable_clock;
-		encoder->disable_clock = icl_ddi_combo_disable_clock;
+	if (IS_ALDERLAKE_S(dev_priv)) {
+		encoder->enable_clock = adls_ddi_enable_clock;
+		encoder->disable_clock = adls_ddi_disable_clock;
+	} else if (IS_ROCKETLAKE(dev_priv)) {
+		encoder->enable_clock = rkl_ddi_enable_clock;
+		encoder->disable_clock = rkl_ddi_disable_clock;
 	} else if (IS_DG1(dev_priv)) {
 		encoder->enable_clock = dg1_ddi_enable_clock;
 		encoder->disable_clock = dg1_ddi_disable_clock;