Message ID | 20221021002024.390052-5-anusha.srivatsa@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Prep series - CDCLK code churn | expand |
On 20.10.2022 17:20, Anusha Srivatsa wrote: > No functional change. Introduce dg2_cdclk_squash_programming and > move squash_ctl register programming bits to this. > > Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 23 +++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 8701796788e3..b692186c8f02 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -1705,6 +1705,18 @@ static void bxt_cdclk_pll(struct drm_i915_private *i915, int vco) > > } > > +static void dg2_cdclk_squash_programming(struct drm_i915_private *i915, > + u16 waveform) > +{ > + u32 squash_ctl = 0; > + > + if (waveform) > + squash_ctl = CDCLK_SQUASH_ENABLE | > + CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform; > + > + intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); > +} > + > static void bxt_set_cdclk(struct drm_i915_private *dev_priv, > const struct intel_cdclk_config *cdclk_config, > enum pipe pipe) > @@ -1752,15 +1764,8 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, > else > clock = cdclk; > > - if (HAS_CDCLK_SQUASH(dev_priv)) { > - u32 squash_ctl = 0; > - > - if (waveform) > - squash_ctl = CDCLK_SQUASH_ENABLE | > - CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform; > - > - intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl); > - } > + if (HAS_CDCLK_SQUASH(dev_priv)) > + dg2_cdclk_squash_programming(dev_priv, waveform); Is it possible to move also the function cdclk_squash_waveform() inside dg2_cdclk_squash_programming()? Regards, Bala > > val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) | > bxt_cdclk_cd2x_pipe(dev_priv, pipe) | > -- > 2.25.1 >
On Thu, 20 Oct 2022, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote: > No functional change. Introduce dg2_cdclk_squash_programming and > move squash_ctl register programming bits to this. > > Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 23 +++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 8701796788e3..b692186c8f02 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -1705,6 +1705,18 @@ static void bxt_cdclk_pll(struct drm_i915_private *i915, int vco) > > } > > +static void dg2_cdclk_squash_programming(struct drm_i915_private *i915, > + u16 waveform) Function names that are verbs should preferrably use the imperative mood, i.e. program() instead of programmed(), programs() or programming(). I'm also not sure "programming" or "program" is a very descriptive thing; almost everything here is about "programming" something or other. BR, Jani. > +{ > + u32 squash_ctl = 0; > + > + if (waveform) > + squash_ctl = CDCLK_SQUASH_ENABLE | > + CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform; > + > + intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); > +} > + > static void bxt_set_cdclk(struct drm_i915_private *dev_priv, > const struct intel_cdclk_config *cdclk_config, > enum pipe pipe) > @@ -1752,15 +1764,8 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, > else > clock = cdclk; > > - if (HAS_CDCLK_SQUASH(dev_priv)) { > - u32 squash_ctl = 0; > - > - if (waveform) > - squash_ctl = CDCLK_SQUASH_ENABLE | > - CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform; > - > - intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl); > - } > + if (HAS_CDCLK_SQUASH(dev_priv)) > + dg2_cdclk_squash_programming(dev_priv, waveform); > > val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) | > bxt_cdclk_cd2x_pipe(dev_priv, pipe) |
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Friday, October 21, 2022 1:41 AM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Vivekanandan, Balasubramani > <balasubramani.vivekanandan@intel.com> > Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915/display: Move squash_ctl > register programming to its own function > > On Thu, 20 Oct 2022, Anusha Srivatsa <anusha.srivatsa@intel.com> wrote: > > No functional change. Introduce dg2_cdclk_squash_programming and > move > > squash_ctl register programming bits to this. > > > > Cc: Balasubramani Vivekanandan > <balasubramani.vivekanandan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 23 > > +++++++++++++--------- > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > > b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index 8701796788e3..b692186c8f02 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -1705,6 +1705,18 @@ static void bxt_cdclk_pll(struct > > drm_i915_private *i915, int vco) > > > > } > > > > +static void dg2_cdclk_squash_programming(struct drm_i915_private > *i915, > > + u16 waveform) > > Function names that are verbs should preferrably use the imperative mood, > i.e. program() instead of programmed(), programs() or programming(). > > I'm also not sure "programming" or "program" is a very descriptive thing; > almost everything here is about "programming" something or other. Makes sense. Didn’t have a good feeling about the name in the first place. Thanks for the validation. Anusha > BR, > Jani. > > > > +{ > > + u32 squash_ctl = 0; > > + > > + if (waveform) > > + squash_ctl = CDCLK_SQUASH_ENABLE | > > + CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform; > > + > > + intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); } > > + > > static void bxt_set_cdclk(struct drm_i915_private *dev_priv, > > const struct intel_cdclk_config *cdclk_config, > > enum pipe pipe) > > @@ -1752,15 +1764,8 @@ static void bxt_set_cdclk(struct > drm_i915_private *dev_priv, > > else > > clock = cdclk; > > > > - if (HAS_CDCLK_SQUASH(dev_priv)) { > > - u32 squash_ctl = 0; > > - > > - if (waveform) > > - squash_ctl = CDCLK_SQUASH_ENABLE | > > - CDCLK_SQUASH_WINDOW_SIZE(0xf) | > waveform; > > - > > - intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl); > > - } > > + if (HAS_CDCLK_SQUASH(dev_priv)) > > + dg2_cdclk_squash_programming(dev_priv, waveform); > > > > val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) | > > bxt_cdclk_cd2x_pipe(dev_priv, pipe) | > > -- > Jani Nikula, Intel Open Source Graphics Center
> -----Original Message----- > From: Vivekanandan, Balasubramani > <balasubramani.vivekanandan@intel.com> > Sent: Friday, October 21, 2022 12:08 AM > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Subject: Re: [PATCH 4/4] drm/i915/display: Move squash_ctl register > programming to its own function > > On 20.10.2022 17:20, Anusha Srivatsa wrote: > > No functional change. Introduce dg2_cdclk_squash_programming and > move > > squash_ctl register programming bits to this. > > > > Cc: Balasubramani Vivekanandan > <balasubramani.vivekanandan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 23 > > +++++++++++++--------- > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > > b/drivers/gpu/drm/i915/display/intel_cdclk.c > > index 8701796788e3..b692186c8f02 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > @@ -1705,6 +1705,18 @@ static void bxt_cdclk_pll(struct > > drm_i915_private *i915, int vco) > > > > } > > > > +static void dg2_cdclk_squash_programming(struct drm_i915_private > *i915, > > + u16 waveform) > > +{ > > + u32 squash_ctl = 0; > > + > > + if (waveform) > > + squash_ctl = CDCLK_SQUASH_ENABLE | > > + CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform; > > + > > + intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); } > > + > > static void bxt_set_cdclk(struct drm_i915_private *dev_priv, > > const struct intel_cdclk_config *cdclk_config, > > enum pipe pipe) > > @@ -1752,15 +1764,8 @@ static void bxt_set_cdclk(struct > drm_i915_private *dev_priv, > > else > > clock = cdclk; > > > > - if (HAS_CDCLK_SQUASH(dev_priv)) { > > - u32 squash_ctl = 0; > > - > > - if (waveform) > > - squash_ctl = CDCLK_SQUASH_ENABLE | > > - CDCLK_SQUASH_WINDOW_SIZE(0xf) | > waveform; > > - > > - intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl); > > - } > > + if (HAS_CDCLK_SQUASH(dev_priv)) > > + dg2_cdclk_squash_programming(dev_priv, waveform); > > Is it possible to move also the function cdclk_squash_waveform() inside > dg2_cdclk_squash_programming()? Hmmm. Are you thinking it is better to have both grabbing the squash waveform and programming the squash_ctl registers in one place better? IMO the fact that they are separate makes it more readable. We will have to return waveform and calculate the clock anyway.... Anusha > Regards, > Bala > > > > > val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) | > > bxt_cdclk_cd2x_pipe(dev_priv, pipe) | > > -- > > 2.25.1 > >
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index 8701796788e3..b692186c8f02 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -1705,6 +1705,18 @@ static void bxt_cdclk_pll(struct drm_i915_private *i915, int vco) } +static void dg2_cdclk_squash_programming(struct drm_i915_private *i915, + u16 waveform) +{ + u32 squash_ctl = 0; + + if (waveform) + squash_ctl = CDCLK_SQUASH_ENABLE | + CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform; + + intel_de_write(i915, CDCLK_SQUASH_CTL, squash_ctl); +} + static void bxt_set_cdclk(struct drm_i915_private *dev_priv, const struct intel_cdclk_config *cdclk_config, enum pipe pipe) @@ -1752,15 +1764,8 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, else clock = cdclk; - if (HAS_CDCLK_SQUASH(dev_priv)) { - u32 squash_ctl = 0; - - if (waveform) - squash_ctl = CDCLK_SQUASH_ENABLE | - CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform; - - intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl); - } + if (HAS_CDCLK_SQUASH(dev_priv)) + dg2_cdclk_squash_programming(dev_priv, waveform); val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) | bxt_cdclk_cd2x_pipe(dev_priv, pipe) |
No functional change. Introduce dg2_cdclk_squash_programming and move squash_ctl register programming bits to this. Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.c | 23 +++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)