Re: [PATCH] ARM: arria10: xload: retry FPGA configuration

[Sascha Hauer <sha@pengutronix.de>]

On Wed, Oct 11, 2023 at 10:37:50AM +0200, Steffen Trumtrar wrote:
> From: Bruno Knittel <bruno.knittel@bruker.com>
> 
> The Intel documentation states that the FPGA configuration might fail.
> This has been observed on the Enclustra AA1+ board where up to 4 retries
> where required to configure the FPGA.
> 
> Debugging session showed that the data where correctly read from the
> eMMC but yet the configuration failed.
> 
> This commit introduces a retry loop on the FPGA configuration.
> Up to 10 retries (arbitrary) are attempted.
> As the hardware can't be used anyway without the FPGA loaded, this
> doesn't introduce any boot time problems. Taking longer is better than
> just hang()ing.
> 
> Signed-off-by: Bruno Knittel <bruno.knittel@bruker.com>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  arch/arm/mach-socfpga/arria10-xload.c | 47 +++++++++++++++++----------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/arria10-xload.c b/arch/arm/mach-socfpga/arria10-xload.c
> index 9d54a1de58..0e49452c17 100644
> --- a/arch/arm/mach-socfpga/arria10-xload.c
> +++ b/arch/arm/mach-socfpga/arria10-xload.c
> @@ -359,33 +359,46 @@ int arria10_load_fpga(int offset, int bitstream_size)
>  	int ret;
>  	uint32_t count;
>  	uint32_t size = bitstream_size / SECTOR_SIZE;
> +	uint32_t retryCount;
>  
>  	if (offset)
>  		offset = offset / SECTOR_SIZE;
>  
> -	count = offset;
> +	/* Up to 4 retires have been seen on the Enclustra Mercury AA1+ board, as FPGA configuration is mandatory

Respect. Most people retire only once ;)

> +	 * to be able to continue the boot, take some margin and try up to 10 times
> +	 */
> +	for (retryCount = 0; retryCount < 10; ++retryCount) {
> +		count = offset;
>  
> -	arria10_read_blocks(buf, count + bitstream.first_sec, SZ_16K);
> +		arria10_read_blocks(buf, count + bitstream.first_sec, SZ_16K);
>  
> -	count += SZ_16K / SECTOR_SIZE;
> +		count += SZ_16K / SECTOR_SIZE;
>  
> -	ret = a10_fpga_init(buf);
> -	if (ret)
> -		hang();
> +		ret = a10_fpga_init(buf);
> +		if (ret)
> +			continue;
>  
> -	while (count <= size) {
> -		ret = a10_fpga_write(buf, SZ_16K);
> -		if (ret == -ENOSPC)
> -			break;
> -		count += SZ_16K / SECTOR_SIZE;
> -		ret = arria10_read_blocks(buf, count, SZ_16K);
> -	}
> +		while (count <= size) {
> +			ret = a10_fpga_write(buf, SZ_16K);
> +			if (ret == -ENOSPC)
> +				continue;

You probably want to continue the outer for() loop, but this continues
the inner while() loop.

Generally I think both the patch and the resulting code would look nicer
when you do the retry loop in a separate function.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |