Re: [Intel-gfx] [PATCH 2/5] drm/i915: Use intel_de_rmw() in bdw cdclk programming

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 30 Apr 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the hand rolled rmw sequences with intel_de_rmw().
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index d5314f9acf83..1ef8da2bf856 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -745,7 +745,6 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
>  			  enum pipe pipe)
>  {
>  	int cdclk = cdclk_config->cdclk;
> -	u32 val;
>  	int ret;
>  
>  	if (drm_WARN(&dev_priv->drm,
> @@ -765,9 +764,8 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
>  		return;
>  	}
>  
> -	val = intel_de_read(dev_priv, LCPLL_CTL);
> -	val |= LCPLL_CD_SOURCE_FCLK;
> -	intel_de_write(dev_priv, LCPLL_CTL, val);
> +	intel_de_rmw(dev_priv, LCPLL_CTL,
> +		     0, LCPLL_CD_SOURCE_FCLK);
>  
>  	/*
>  	 * According to the spec, it should be enough to poll for this 1 us.
> @@ -777,14 +775,11 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
>  			LCPLL_CD_SOURCE_FCLK_DONE, 100))
>  		drm_err(&dev_priv->drm, "Switching to FCLK failed\n");
>  
> -	val = intel_de_read(dev_priv, LCPLL_CTL);
> -	val &= ~LCPLL_CLK_FREQ_MASK;
> -	val |= bdw_cdclk_freq_sel(cdclk);
> -	intel_de_write(dev_priv, LCPLL_CTL, val);
> +	intel_de_rmw(dev_priv, LCPLL_CTL,
> +		     LCPLL_CLK_FREQ_MASK, bdw_cdclk_freq_sel(cdclk));
>  
> -	val = intel_de_read(dev_priv, LCPLL_CTL);
> -	val &= ~LCPLL_CD_SOURCE_FCLK;
> -	intel_de_write(dev_priv, LCPLL_CTL, val);
> +	intel_de_rmw(dev_priv, LCPLL_CTL,
> +		     LCPLL_CD_SOURCE_FCLK, 0);

These are one of those cases where I wonder about that

	if (val != old)
		intel_uncore_write(uncore, reg, val);

check in intel_uncore_rmw().

I guess it's fine, but I get that uneasy feeling.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  
>  	if (wait_for_us((intel_de_read(dev_priv, LCPLL_CTL) &
>  			 LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx