diff mbox series

[4/4] drm/i915/cdclk: Re-use bxt_cdclk_ctl() when sanitizing

Message ID 20240104032150.118954-5-gustavo.sousa@intel.com (mailing list archive)
State New, archived
Headers show
Series Update bxt_sanitize_cdclk() for Xe2_LPD | expand

Commit Message

Gustavo Sousa Jan. 4, 2024, 3:21 a.m. UTC 
That's the function responsible for deriving that register's value; use
it.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++-------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

Comments

Matt Roper Jan. 4, 2024, 11:48 p.m. UTC | #1 
On Thu, Jan 04, 2024 at 12:21:50AM -0300, Gustavo Sousa wrote:
> That's the function responsible for deriving that register's value; use
> it.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++-------------------
>  1 file changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index fbe9aba41c35..26200ee3e23f 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	u32 cdctl, expected;
> -	int cdclk, clock, vco;
> +	int cdclk, vco;
>  
>  	intel_update_cdclk(dev_priv);
>  	intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, "Current CDCLK");
> @@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  	 * so sanitize this register.
>  	 */
>  	cdctl = intel_de_read(dev_priv, CDCLK_CTL);
> +	expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, INVALID_PIPE);
>  
>  	/*
>  	 * Let's ignore the pipe field, since BIOS could have configured the
> @@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  	 * (PIPE_NONE).
>  	 */
>  	cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> -
> -	if (DISPLAY_VER(dev_priv) >= 20)
> -		expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
> -	else
> -		expected = skl_cdclk_decimal(cdclk);
> -
> -	/* Figure out what CD2X divider we should be using for this cdclk */
> -	if (HAS_CDCLK_SQUASH(dev_priv))
> -		clock = dev_priv->display.cdclk.hw.vco / 2;
> -	else
> -		clock = dev_priv->display.cdclk.hw.cdclk;
> -
> -	expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
> -					   dev_priv->display.cdclk.hw.vco);
> -
> -	/*
> -	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
> -	 * enable otherwise.
> -	 */
> -	if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> -	    dev_priv->display.cdclk.hw.cdclk >= 500000)
> -		expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> +	expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
>  
>  	if (cdctl == expected)
>  		/* All well; nothing to sanitize */
> -- 
> 2.43.0
>
Matt Roper Jan. 4, 2024, 11:52 p.m. UTC | #2 
On Thu, Jan 04, 2024 at 03:48:34PM -0800, Matt Roper wrote:
> On Thu, Jan 04, 2024 at 12:21:50AM -0300, Gustavo Sousa wrote:
> > That's the function responsible for deriving that register's value; use
> > it.
> > 
> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Forgot to mention...I think it's a bit jarring when the commit message
starts out referring to something in the headline ("That's the
function...").  It's probably a bit better to just re-state the function
name in the commit message again rather than assuming both get read
together.


Matt

> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++-------------------
> >  1 file changed, 3 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index fbe9aba41c35..26200ee3e23f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 cdctl, expected;
> > -	int cdclk, clock, vco;
> > +	int cdclk, vco;
> >  
> >  	intel_update_cdclk(dev_priv);
> >  	intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, "Current CDCLK");
> > @@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >  	 * so sanitize this register.
> >  	 */
> >  	cdctl = intel_de_read(dev_priv, CDCLK_CTL);
> > +	expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, INVALID_PIPE);
> >  
> >  	/*
> >  	 * Let's ignore the pipe field, since BIOS could have configured the
> > @@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >  	 * (PIPE_NONE).
> >  	 */
> >  	cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> > -
> > -	if (DISPLAY_VER(dev_priv) >= 20)
> > -		expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
> > -	else
> > -		expected = skl_cdclk_decimal(cdclk);
> > -
> > -	/* Figure out what CD2X divider we should be using for this cdclk */
> > -	if (HAS_CDCLK_SQUASH(dev_priv))
> > -		clock = dev_priv->display.cdclk.hw.vco / 2;
> > -	else
> > -		clock = dev_priv->display.cdclk.hw.cdclk;
> > -
> > -	expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
> > -					   dev_priv->display.cdclk.hw.vco);
> > -
> > -	/*
> > -	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
> > -	 * enable otherwise.
> > -	 */
> > -	if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> > -	    dev_priv->display.cdclk.hw.cdclk >= 500000)
> > -		expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> > +	expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> >  
> >  	if (cdctl == expected)
> >  		/* All well; nothing to sanitize */
> > -- 
> > 2.43.0
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
Gustavo Sousa Jan. 5, 2024, 2:09 p.m. UTC | #3 
Quoting Matt Roper (2024-01-04 20:52:32-03:00)
>On Thu, Jan 04, 2024 at 03:48:34PM -0800, Matt Roper wrote:
>> On Thu, Jan 04, 2024 at 12:21:50AM -0300, Gustavo Sousa wrote:
>> > That's the function responsible for deriving that register's value; use
>> > it.
>> > 
>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> 
>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
>Forgot to mention...I think it's a bit jarring when the commit message
>starts out referring to something in the headline ("That's the
>function...").  It's probably a bit better to just re-state the function
>name in the commit message again rather than assuming both get read
>together.

Thanks for the suggestion!

I have sent a v2 updating the body of commit messages, not only for this one but
also for two more patches. I have kept your r-b's. Let me know if that's okay.

--
Gustavo Sousa

>
>
>Matt
>
>> 
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++-------------------
>> >  1 file changed, 3 insertions(+), 23 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > index fbe9aba41c35..26200ee3e23f 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > @@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>> >  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
>> >  {
>> >          u32 cdctl, expected;
>> > -        int cdclk, clock, vco;
>> > +        int cdclk, vco;
>> >  
>> >          intel_update_cdclk(dev_priv);
>> >          intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, "Current CDCLK");
>> > @@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
>> >           * so sanitize this register.
>> >           */
>> >          cdctl = intel_de_read(dev_priv, CDCLK_CTL);
>> > +        expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, INVALID_PIPE);
>> >  
>> >          /*
>> >           * Let's ignore the pipe field, since BIOS could have configured the
>> > @@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
>> >           * (PIPE_NONE).
>> >           */
>> >          cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
>> > -
>> > -        if (DISPLAY_VER(dev_priv) >= 20)
>> > -                expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
>> > -        else
>> > -                expected = skl_cdclk_decimal(cdclk);
>> > -
>> > -        /* Figure out what CD2X divider we should be using for this cdclk */
>> > -        if (HAS_CDCLK_SQUASH(dev_priv))
>> > -                clock = dev_priv->display.cdclk.hw.vco / 2;
>> > -        else
>> > -                clock = dev_priv->display.cdclk.hw.cdclk;
>> > -
>> > -        expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
>> > -                                           dev_priv->display.cdclk.hw.vco);
>> > -
>> > -        /*
>> > -         * Disable SSA Precharge when CD clock frequency < 500 MHz,
>> > -         * enable otherwise.
>> > -         */
>> > -        if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
>> > -            dev_priv->display.cdclk.hw.cdclk >= 500000)
>> > -                expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
>> > +        expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
>> >  
>> >          if (cdctl == expected)
>> >                  /* All well; nothing to sanitize */
>> > -- 
>> > 2.43.0
>> > 
>> 
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
Matt Roper Jan. 5, 2024, 4:07 p.m. UTC | #4 
On Fri, Jan 05, 2024 at 11:09:37AM -0300, Gustavo Sousa wrote:
> Quoting Matt Roper (2024-01-04 20:52:32-03:00)
> >On Thu, Jan 04, 2024 at 03:48:34PM -0800, Matt Roper wrote:
> >> On Thu, Jan 04, 2024 at 12:21:50AM -0300, Gustavo Sousa wrote:
> >> > That's the function responsible for deriving that register's value; use
> >> > it.
> >> > 
> >> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> >> 
> >> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> >
> >Forgot to mention...I think it's a bit jarring when the commit message
> >starts out referring to something in the headline ("That's the
> >function...").  It's probably a bit better to just re-state the function
> >name in the commit message again rather than assuming both get read
> >together.
> 
> Thanks for the suggestion!
> 
> I have sent a v2 updating the body of commit messages, not only for this one but
> also for two more patches. I have kept your r-b's. Let me know if that's okay.

Yep, the r-b's still apply.  Thanks.


Matt

> 
> --
> Gustavo Sousa
> 
> >
> >
> >Matt
> >
> >> 
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 +++-------------------
> >> >  1 file changed, 3 insertions(+), 23 deletions(-)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > index fbe9aba41c35..26200ee3e23f 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > @@ -2051,7 +2051,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >> >  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >> >  {
> >> >          u32 cdctl, expected;
> >> > -        int cdclk, clock, vco;
> >> > +        int cdclk, vco;
> >> >  
> >> >          intel_update_cdclk(dev_priv);
> >> >          intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, "Current CDCLK");
> >> > @@ -2076,6 +2076,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >> >           * so sanitize this register.
> >> >           */
> >> >          cdctl = intel_de_read(dev_priv, CDCLK_CTL);
> >> > +        expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, INVALID_PIPE);
> >> >  
> >> >          /*
> >> >           * Let's ignore the pipe field, since BIOS could have configured the
> >> > @@ -2083,28 +2084,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >> >           * (PIPE_NONE).
> >> >           */
> >> >          cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> >> > -
> >> > -        if (DISPLAY_VER(dev_priv) >= 20)
> >> > -                expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
> >> > -        else
> >> > -                expected = skl_cdclk_decimal(cdclk);
> >> > -
> >> > -        /* Figure out what CD2X divider we should be using for this cdclk */
> >> > -        if (HAS_CDCLK_SQUASH(dev_priv))
> >> > -                clock = dev_priv->display.cdclk.hw.vco / 2;
> >> > -        else
> >> > -                clock = dev_priv->display.cdclk.hw.cdclk;
> >> > -
> >> > -        expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
> >> > -                                           dev_priv->display.cdclk.hw.vco);
> >> > -
> >> > -        /*
> >> > -         * Disable SSA Precharge when CD clock frequency < 500 MHz,
> >> > -         * enable otherwise.
> >> > -         */
> >> > -        if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> >> > -            dev_priv->display.cdclk.hw.cdclk >= 500000)
> >> > -                expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> >> > +        expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
> >> >  
> >> >          if (cdctl == expected)
> >> >                  /* All well; nothing to sanitize */
> >> > -- 
> >> > 2.43.0
> >> > 
> >> 
> >> -- 
> >> Matt Roper
> >> Graphics Software Engineer
> >> Linux GPU Platform Enablement
> >> Intel Corporation
> >
> >-- 
> >Matt Roper
> >Graphics Software Engineer
> >Linux GPU Platform Enablement
> >Intel Corporation
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index fbe9aba41c35..26200ee3e23f 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2051,7 +2051,7 @@  static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
 {
 	u32 cdctl, expected;
-	int cdclk, clock, vco;
+	int cdclk, vco;
 
 	intel_update_cdclk(dev_priv);
 	intel_cdclk_dump_config(dev_priv, &dev_priv->display.cdclk.hw, "Current CDCLK");
@@ -2076,6 +2076,7 @@  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
 	 * so sanitize this register.
 	 */
 	cdctl = intel_de_read(dev_priv, CDCLK_CTL);
+	expected = bxt_cdclk_ctl(dev_priv, &dev_priv->display.cdclk.hw, INVALID_PIPE);
 
 	/*
 	 * Let's ignore the pipe field, since BIOS could have configured the
@@ -2083,28 +2084,7 @@  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
 	 * (PIPE_NONE).
 	 */
 	cdctl &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
-
-	if (DISPLAY_VER(dev_priv) >= 20)
-		expected = MDCLK_SOURCE_SEL_CDCLK_PLL;
-	else
-		expected = skl_cdclk_decimal(cdclk);
-
-	/* Figure out what CD2X divider we should be using for this cdclk */
-	if (HAS_CDCLK_SQUASH(dev_priv))
-		clock = dev_priv->display.cdclk.hw.vco / 2;
-	else
-		clock = dev_priv->display.cdclk.hw.cdclk;
-
-	expected |= bxt_cdclk_cd2x_div_sel(dev_priv, clock,
-					   dev_priv->display.cdclk.hw.vco);
-
-	/*
-	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
-	 * enable otherwise.
-	 */
-	if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
-	    dev_priv->display.cdclk.hw.cdclk >= 500000)
-		expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
+	expected &= ~bxt_cdclk_cd2x_pipe(dev_priv, INVALID_PIPE);
 
 	if (cdctl == expected)
 		/* All well; nothing to sanitize */