Re: [PATCH 7/7] accel/ivpu: Fix VPU clock calculation

From: Jeffrey Hugo <quic_jhugo@quicinc.com>
To: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>,
	<dri-devel@lists.freedesktop.org>
Cc: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>,
	Oded Gabbay <ogabbay@kernel.org>,
	Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
Subject: Re: [PATCH 7/7] accel/ivpu: Fix VPU clock calculation
Date: Wed, 22 Mar 2023 08:52:56 -0600	[thread overview]
Message-ID: <56d8982a-6d86-19d2-74ca-efede0414af7@quicinc.com> (raw)
In-Reply-To: <20230322091900.1982453-8-stanislaw.gruszka@linux.intel.com>

On 3/22/2023 3:19 AM, Stanislaw Gruszka wrote:
> VPU cpu clock frequency depends on the workpoint configuration
> that was granted by the punit. Previously driver was passing
> incorrect frequency to the VPU firmware.

This looks like past tense.  I believe the preference is to use the 
present tense for commit text.  Something like "the driver calculates 
the wrong frequency because it ignores the workpoint config and this 
causes X.  Fix this by using the workpoint config in the freq calculations".

Should this have a fixes tag?  Sounds like this addresses a bug.

> Co-developed-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
> Signed-off-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_hw_mtl.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_hw_mtl.c b/drivers/accel/ivpu/ivpu_hw_mtl.c
> index 98c8a4aa25f0..382ec127be8e 100644
> --- a/drivers/accel/ivpu/ivpu_hw_mtl.c
> +++ b/drivers/accel/ivpu/ivpu_hw_mtl.c
> @@ -29,7 +29,6 @@
>   
>   #define PLL_REF_CLK_FREQ	     (50 * 1000000)
>   #define PLL_SIMULATION_FREQ	     (10 * 1000000)
> -#define PLL_RATIO_TO_FREQ(x)	     ((x) * PLL_REF_CLK_FREQ)
>   #define PLL_DEFAULT_EPP_VALUE	     0x80
>   
>   #define TIM_SAFE_ENABLE		     0xf1d0dead
> @@ -789,6 +788,19 @@ static void ivpu_hw_mtl_wdt_disable(struct ivpu_device *vdev)
>   	REGV_WR32(MTL_VPU_CPU_SS_TIM_GEN_CONFIG, val);
>   }
>   
> +static u32 ivpu_hw_mtl_pll_to_freq(u32 ratio, u32 config)
> +{
> +	u32 pll_clock = PLL_REF_CLK_FREQ * ratio;
> +	u32 cpu_clock;
> +
> +	if ((config & 0xff) == PLL_RATIO_4_3)
> +		cpu_clock = pll_clock * 2 / 4;
> +	else
> +		cpu_clock = pll_clock * 2 / 5;
> +
> +	return cpu_clock;
> +}
> +
>   /* Register indirect accesses */
>   static u32 ivpu_hw_mtl_reg_pll_freq_get(struct ivpu_device *vdev)
>   {
> @@ -800,7 +812,7 @@ static u32 ivpu_hw_mtl_reg_pll_freq_get(struct ivpu_device *vdev)
>   	if (!ivpu_is_silicon(vdev))
>   		return PLL_SIMULATION_FREQ;
>   
> -	return PLL_RATIO_TO_FREQ(pll_curr_ratio);
> +	return ivpu_hw_mtl_pll_to_freq(pll_curr_ratio, vdev->hw->config);
>   }
>   
>   static u32 ivpu_hw_mtl_reg_telemetry_offset_get(struct ivpu_device *vdev)