Re: [PATCH 3/3] accel/ivpu: Split HW IP generation dispatch into helpers

[Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>]

On 01-Oct-26 1:04 PM, Dawid Osuchowski wrote:
> From: Karol Wachowski <karol.wachowski@linux.intel.com>
> 
> ivpu_hw_ip_soc_cpu_boot() and pwr_island_delay_set() keep the result of
> their per-generation switch in locals that are left uninitialized on the
> path past the last case. Building with -Wmaybe-uninitialized reports ret,
> post, post2 and status; Kbuild disables that warning for GCC, so it does
> not show up in a normal build.
> 
> Let every case return directly instead, so no local is needed.
> ivpu_hw_ip_soc_cpu_boot() now dispatches on its own, with the debug
> message moved ahead of the switch so that the boot mode is logged before
> the boot registers are written, and the power island delay values move
> into per-generation helpers. Neither switch has a default: label, so
> -Wswitch reports any enum vpu_hw_ip value added later that is not
> handled, and the path past the switch returns -EINVAL.
> 
> Build the power island delay values from zero instead of reading the
> registers first. POST_DLY, POST1_DLY and POST2_DLY together span every
> writable bit of PWR_ISLAND_EN_POST_DLY, and STATUS_DLY spans every
> writable bit of PWR_ISLAND_STATUS_DLY, so the read-modify-write only
> fetched bits that are immediately overwritten.
> 
> The boot mode is now logged even when the boot fails; no other functional
> changes.
> 
> Signed-off-by: Karol Wachowski <karol.wachowski@linux.intel.com>
> Signed-off-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>
> ---
>   drivers/accel/ivpu/ivpu_hw_ip.c | 64 ++++++++++++++++-----------------
>   1 file changed, 31 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/accel/ivpu/ivpu_hw_ip.c b/drivers/accel/ivpu/ivpu_hw_ip.c
> index bd6bb3fa49ff..36730f4338f0 100644
> --- a/drivers/accel/ivpu/ivpu_hw_ip.c
> +++ b/drivers/accel/ivpu/ivpu_hw_ip.c
> @@ -265,21 +265,35 @@ void ivpu_hw_ip_idle_gen_disable(struct ivpu_device *vdev)
>   }
>   
>   static void
> -pwr_island_delay_set_50xx(struct ivpu_device *vdev, u32 post, u32 post1, u32 post2, u32 status)
> +pwr_island_delay_write(struct ivpu_device *vdev, u32 post, u32 post1, u32 post2, u32 status)
>   {
>   	u32 val;
>   
> -	val = REGV_RD32(VPU_50XX_HOST_SS_AON_PWR_ISLAND_EN_POST_DLY);
> -	val = REG_SET_FLD_NUM(VPU_50XX_HOST_SS_AON_PWR_ISLAND_EN_POST_DLY, POST_DLY, post, val);
> +	val = REG_SET_FLD_NUM(VPU_50XX_HOST_SS_AON_PWR_ISLAND_EN_POST_DLY, POST_DLY, post, 0);
>   	val = REG_SET_FLD_NUM(VPU_50XX_HOST_SS_AON_PWR_ISLAND_EN_POST_DLY, POST1_DLY, post1, val);
>   	val = REG_SET_FLD_NUM(VPU_50XX_HOST_SS_AON_PWR_ISLAND_EN_POST_DLY, POST2_DLY, post2, val);
>   	REGV_WR32(VPU_50XX_HOST_SS_AON_PWR_ISLAND_EN_POST_DLY, val);
>   
> -	val = REGV_RD32(VPU_50XX_HOST_SS_AON_PWR_ISLAND_STATUS_DLY);
> -	val = REG_SET_FLD_NUM(VPU_50XX_HOST_SS_AON_PWR_ISLAND_STATUS_DLY, STATUS_DLY, status, val);
> +	val = REG_SET_FLD_NUM(VPU_50XX_HOST_SS_AON_PWR_ISLAND_STATUS_DLY, STATUS_DLY, status, 0);
>   	REGV_WR32(VPU_50XX_HOST_SS_AON_PWR_ISLAND_STATUS_DLY, val);
>   }
>   
> +static void pwr_island_delay_set_50xx(struct ivpu_device *vdev)
> +{
> +	if (vdev->hw->pll.profiling_freq == PLL_PROFILING_FREQ_HIGH)
> +		pwr_island_delay_write(vdev, 18, 0, 0, 46);
> +	else
> +		pwr_island_delay_write(vdev, 0, 0, 0, 3);
> +}
> +
> +static void pwr_island_delay_set_60xx(struct ivpu_device *vdev)
> +{
> +	if (vdev->hw->pll.profiling_freq == PLL_PROFILING_FREQ_HIGH)
> +		pwr_island_delay_write(vdev, 198, 0, 198, 0);
> +	else
> +		pwr_island_delay_write(vdev, 17, 0, 17, 0);
> +}
> +
>   static void pwr_island_trickle_drive_37xx(struct ivpu_device *vdev, bool enable)
>   {
>   	u32 val = REGV_RD32(VPU_37XX_HOST_SS_AON_PWR_ISLAND_TRICKLE_EN0);
> @@ -675,27 +689,19 @@ static void dpu_active_drive_37xx(struct ivpu_device *vdev, bool enable)
>   
>   static void pwr_island_delay_set(struct ivpu_device *vdev)
>   {
> -	bool high = vdev->hw->pll.profiling_freq == PLL_PROFILING_FREQ_HIGH;
> -	u32 post, post1, post2, status;
> -
>   	switch (ivpu_hw_ip_gen(vdev)) {
>   	case IVPU_HW_IP_37XX:
>   	case IVPU_HW_IP_40XX:
>   		return;
> +
>   	case IVPU_HW_IP_50XX:
> -		post = high ? 18 : 0;
> -		post1 = 0;
> -		post2 = 0;
> -		status = high ? 46 : 3;
> -		break;
> +		pwr_island_delay_set_50xx(vdev);
> +		return;
> +
>   	case IVPU_HW_IP_60XX:
> -		post = high ? 198 : 17;
> -		post1 = 0;
> -		post2 = high ? 198 : 17;
> -		status = 0;
> +		pwr_island_delay_set_60xx(vdev);
> +		return;
>   	}
> -
> -	pwr_island_delay_set_50xx(vdev, post, post1, post2, status);
>   }
>   
>   int ivpu_hw_ip_pwr_domain_enable(struct ivpu_device *vdev)
> @@ -933,30 +939,22 @@ static int soc_cpu_boot_60xx(struct ivpu_device *vdev)
>   
>   int ivpu_hw_ip_soc_cpu_boot(struct ivpu_device *vdev)
>   {
> -	int ret;
> +	ivpu_dbg(vdev, PM, "Booting firmware, mode: %s\n",
> +		 ivpu_fw_is_warm_boot(vdev) ? "warm boot" : "cold boot");
>   
>   	switch (ivpu_hw_ip_gen(vdev)) {
>   	case IVPU_HW_IP_37XX:
> -		ret = soc_cpu_boot_37xx(vdev);
> -		break;
> +		return soc_cpu_boot_37xx(vdev);
>   
>   	case IVPU_HW_IP_40XX:
>   	case IVPU_HW_IP_50XX:
> -		ret = soc_cpu_boot_40xx(vdev);
> -		break;
> +		return soc_cpu_boot_40xx(vdev);
>   
>   	case IVPU_HW_IP_60XX:
> -		ret = soc_cpu_boot_60xx(vdev);
> -		break;
> +		return soc_cpu_boot_60xx(vdev);
>   	}
>   
> -	if (ret)
> -		return ret;
> -
> -	ivpu_dbg(vdev, PM, "Booting firmware, mode: %s\n",
> -		 ivpu_fw_is_warm_boot(vdev) ? "warm boot" : "cold boot");
> -
> -	return 0;
> +	return -EINVAL;
>   }
>   
>   static void wdt_disable_37xx(struct ivpu_device *vdev)
Reviewed-by: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>