Re: [PATCH 15/16] fpga: machxo2: extend erase timeout for machxo2 FPGA

[Xu Yilun <yilun.xu@intel.com>]

On 2022-08-25 at 16:13:42 +0200, Johannes Zink wrote:
> Measurements showed that some FPGAs take significantly longer than the
> default wait function supplied. The datasheet inidicates up to 30
> seconds erase times for some MachXO2 FPGAs, depending on the number of
> LUTs (and the corresponding configuration flash size).
> 
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> ---
>  drivers/fpga/machxo2-common.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c
> index ccf9a50fc590..e8967cdee2c6 100644
> --- a/drivers/fpga/machxo2-common.c
> +++ b/drivers/fpga/machxo2-common.c
> @@ -17,6 +17,8 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/property.h>
> +#include <linux/iopoll.h>
> +#include <linux/time.h>
>  #include "machxo2-common.h"
>  
>  #define MACHXO2_LOW_DELAY_USEC          5
> @@ -24,6 +26,8 @@
>  #define MACHXO2_REFRESH_USEC            4800
>  #define MACHXO2_MAX_BUSY_LOOP           128
>  #define MACHXO2_MAX_REFRESH_LOOP        16
> +#define MACHXO2_MAX_ERASE_USEC          (30 * USEC_PER_SEC)
> +#define MACHXO2_ERASE_USEC_SLEEP        (20 * USEC_PER_MSEC)
>  
>  #define MACHXO2_PAGE_SIZE               16
>  #define MACHXO2_BUF_SIZE                (MACHXO2_PAGE_SIZE + 4)
> @@ -54,6 +58,18 @@
>  #define ISC_ERASE_FEATURE_ROW	BIT(17)
>  #define ISC_ERASE_UFM		BIT(19)
>  
> +static inline int machxo2_wait_until_not_busy_timeout(struct machxo2_common_priv *priv)
> +{
> +	int ret, pollret;
> +	u32 status = MACHXO2_BUSY;
> +
> +	pollret = read_poll_timeout(priv->get_status, ret,
> +				    (ret && ret != -EAGAIN) || !(status & MACHXO2_BUSY),
> +				    MACHXO2_ERASE_USEC_SLEEP, MACHXO2_MAX_ERASE_USEC,
> +				    true, priv, &status);

Why just taking care of erase timeout? I see the busy wait in many
places.

> +
> +	return ret ?: pollret;
> +}
>  
>  static inline u8 get_err(u32 status)
>  {
> @@ -114,6 +130,12 @@ static int machxo2_cleanup(struct fpga_manager *mgr)
>  	if (ret)
>  		goto fail;
>  
> +	ret = machxo2_wait_until_not_busy_timeout(priv);
> +	if (ret) {
> +		dev_err(&mgr->dev, "Erase operation failed (%d)", ret);
> +		goto fail;
> +	}
> +
>  	ret = machxo2_wait_until_not_busy(priv);

Is this line still needed?

>  	if (ret)
>  		goto fail;
> @@ -192,9 +214,11 @@ static int machxo2_write_init(struct fpga_manager *mgr,
>  	if (ret)
>  		goto fail;
>  
> -	ret = machxo2_wait_until_not_busy(priv);
> -	if (ret)
> +	ret = machxo2_wait_until_not_busy_timeout(priv);
> +	if (ret) {
> +		dev_err(&mgr->dev, "Erase operation failed (%d)", ret);
>  		goto fail;
> +	}
>  
>  	priv->get_status(priv, &status);
>  	if (status & MACHXO2_FAIL) {
> -- 
> 2.30.2
>