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

[Nazle Asmade, Muhammad Nazim Amirul]

On 26/6/2026 10:46 pm, Xu Yilun wrote:
> [You don't often get email from yilun.xu@xxxxxxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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.
The driver by default will poll for CFG_RDY=0 as a notification from the 
endpoint device that it has completed its teardown sequence on its end, 
so the driver can safely restart a new session. In the event where the 
driver timed out on polling for the teardown complete notification, the 
driver will proceed to a recovery flow by first switching the clock 
(HIP_CLK_SEL) on the end point that feed the CFG_RDY register, and then 
reattempt to read the CFG_RDY register again. The CFG_RDY register may 
get stuck on HIP_CLK_SEL=0 if the device has exited usermode prior to 
the poll, and hence the driver will have to switch the clock and then 
reattempt the read again. And this explains why we need to at least 
reattempt teardown one more time, as the 2nd teardown includes the clock 
switch>
>> 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.
Teardown waits for CFG_RDY to clear. A bad bitstream can leave that 
stuck, so teardown times out while the device is still in CvP mode.

Retrying teardown alone is not enough. Exiting CvP mode first returns 
the hardware to idle, so the next teardown can finish cleanly. The 2nd 
teardown does more than just retry, but also include CVP_MODE clear and 
clock switch.>
>> 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?
Basically by doing another retry always works and sufficient because 
when first teardown fails, switching HIP_CLK_SEL will make the 
CVP_STATUS register becomes responsive again>
>>
>> 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.
Agreed, will create 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.
Agreed. Will use int ret, retry; without initializing them.
> 
>> +
>> +     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;
Will fix in v2>
>> +             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?
Agreed. altera_cvp_teardown() already does dev_err() on the CFG_RDY 
timeout. In v2 I will drop the per-retry dev_warn(), and only print one 
final dev_err() after attempts fail.
> 
> 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.
Agreed on removing the per-iteration "retrying" print, keep it as one 
time only.

Hi Yilun,
Replied as above,  sorry for the delay as I was distracted by some 
issues. Will repost fixes above in V2 if you are ok with replied above.

BR,
Nazim