Re: [PATCH] fpga: altera-cvp: Retry teardown and reset CVP state on failure

[Xu Yilun]

On Thu, Jun 18, 2026 at 04:24:10AM -0700, muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx wrote:
> From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx>
> 
> If an incorrect bitstream is sent, the teardown may fail due to a
> CFG_RDY timeout. When this happens, reset CVP_MODE and HIP_CLK_SEL

Please don't just tell the register name, tell us what is the hardware
mechanism in nature English.

> bits to clean up the hardware state and return -EAGAIN, allowing the

Please help me understand why the teardown fails and why a clean up save
the world.

> caller to retry. Introduce altera_cvp_recovery() to wrap this retry
> logic with a maximum of CVP_TEARDOWN_MAX_RETRY attempts.

Why should we retry multiple times?

> 
> Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx>
> ---
>  drivers/fpga/altera-cvp.c | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 44badfd11e1b..29faf6f5bde1 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -63,6 +63,8 @@
>  #define ALTERA_CVP_V1_SIZE	4
>  #define ALTERA_CVP_V2_SIZE	4096
>  
> +/* Tear-down retry */
> +#define CVP_TEARDOWN_MAX_RETRY 10
>  /* Optional CvP config error status check for debugging */
>  static bool altera_cvp_chkcfg;
>  
> @@ -305,12 +307,40 @@ static int altera_cvp_teardown(struct fpga_manager *mgr,
>  	/* STEP 15 - poll CVP_CONFIG_READY bit for 0 with 10us timeout */
>  	ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY, 0,
>  				     conf->priv->poll_time_us);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&mgr->dev, "CFG_RDY == 0 timeout\n");
> +		goto error_path;
> +	}
>  
>  	return ret;
> +
> +error_path:
> +	/* reset CVP_MODE and HIP_CLK_SEL bit */
> +	altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val);
> +	val &= ~VSE_CVP_MODE_CTRL_HIP_CLK_SEL;
> +	val &= ~VSE_CVP_MODE_CTRL_CVP_MODE;
> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);

Put the reset in your recovery loop.

And if the code block does a meaningful job, don't copy and paste it
everywhere, make a helper.

> +
> +	return -EAGAIN;
> +
>  }
>  
> +static int altera_cvp_recovery(struct fpga_manager *mgr,
> +			      struct fpga_image_info *info)
> +{
> +	int ret = 0, retry = 0;

Try not to initialize local variables that will always be overwritten
later.

> +
> +	for (retry = 0; retry < CVP_TEARDOWN_MAX_RETRY; retry++) {
> +		ret = altera_cvp_teardown(mgr, info);
> +		if (!ret)
> +			break;

Nothing to do on success, just return 0;

> +		dev_warn(&mgr->dev,
> +				 "%s: [%d] Tear-down failed. Retrying\n",
> +				 __func__,
> +				 retry);

You do dev_err() in altera_cvp_teardown(), does the warn proper print
level?

And not sure if the 10-times retry is expected or not, if yes, you
really don't have to yell out again and again. If not, please find
decent solution.