Re: [PATCH v2 5/9] accel/ivpu: Move ivpu_fw_load() to ivpu_fw_init()

[Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx>]

On 8/28/2023 3:47 AM, Stanislaw Gruszka wrote:
> From: Jacek Lawrynowicz <jacek.lawrynowicz@xxxxxxxxxxxxxxx>
>
> ivpu_fw_load() doesn't have to be called separately in ivpu_dev_init().
>
> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@xxxxxxxxxxxxxxx>
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>
> ---
>   drivers/accel/ivpu/ivpu_drv.c | 4 ----
>   drivers/accel/ivpu/ivpu_fw.c  | 6 +++---
>   drivers/accel/ivpu/ivpu_fw.h  | 2 +-
>   3 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index 5310b54f036d..fa0680ba9340 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -561,10 +561,6 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
>   	if (ret)
>   		goto err_ipc_fini;
>   
> -	ret = ivpu_fw_load(vdev);
> -	if (ret)
> -		goto err_job_done_thread_fini;
> -
>   	ret = ivpu_boot(vdev);
>   	if (ret)
>   		goto err_job_done_thread_fini;
> diff --git a/drivers/accel/ivpu/ivpu_fw.c b/drivers/accel/ivpu/ivpu_fw.c
> index 9b6ecd3e9537..32a1ea322ca2 100644
> --- a/drivers/accel/ivpu/ivpu_fw.c
> +++ b/drivers/accel/ivpu/ivpu_fw.c
> @@ -301,6 +301,8 @@ int ivpu_fw_init(struct ivpu_device *vdev)
>   	if (ret)
>   		goto err_fw_release;
>   
> +	ivpu_fw_load(vdev);
> +
>   	return 0;
>   
>   err_fw_release:
> @@ -314,7 +316,7 @@ void ivpu_fw_fini(struct ivpu_device *vdev)
>   	ivpu_fw_release(vdev);
>   }
>   
> -int ivpu_fw_load(struct ivpu_device *vdev)
> +void ivpu_fw_load(struct ivpu_device *vdev)
>   {
>   	struct ivpu_fw_info *fw = vdev->fw;
>   	u64 image_end_offset = fw->image_load_offset + fw->image_size;
> @@ -331,8 +333,6 @@ int ivpu_fw_load(struct ivpu_device *vdev)
>   	}
>   
>   	wmb(); /* Flush WC buffers after writing fw->mem */
> -
> -	return 0;
>   }
>   
>   static void ivpu_fw_boot_params_print(struct ivpu_device *vdev, struct vpu_boot_params *boot_params)
> diff --git a/drivers/accel/ivpu/ivpu_fw.h b/drivers/accel/ivpu/ivpu_fw.h
> index 8567fdf925fe..10ae2847f0ef 100644
> --- a/drivers/accel/ivpu/ivpu_fw.h
> +++ b/drivers/accel/ivpu/ivpu_fw.h
> @@ -31,7 +31,7 @@ struct ivpu_fw_info {
>   
>   int ivpu_fw_init(struct ivpu_device *vdev);
>   void ivpu_fw_fini(struct ivpu_device *vdev);
> -int ivpu_fw_load(struct ivpu_device *vdev);
> +void ivpu_fw_load(struct ivpu_device *vdev);

If ivpu_fw_load() is defined in ivpu_fw.c and only used in ivpu_fw.c, do 
you need it in this header file anymore?  Seems like you could remove 
this, and also make the function static.

>   void ivpu_fw_boot_params_setup(struct ivpu_device *vdev, struct vpu_boot_params *bp);
>   
>   static inline bool ivpu_fw_is_cold_boot(struct ivpu_device *vdev)