[PATCH v4 1/5] fpga manager: xilinx-spi: remove stray comment

[PATCH v4 1/5] fpga manager: xilinx-spi: remove stray comment

[Luca Ceresoli]

Remove comment committed by mistake.

Fixes: dd2784c01d93 ("fpga manager: xilinx-spi: check INIT_B pin during write_init")
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes in v4:
 - add Reviewed-by Tom Rix

Changes in v3: none.

Changes in v2: none.
---
 drivers/fpga/xilinx-spi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index 2967aa2a74e2..502fae0d1d85 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -57,7 +57,6 @@ static int wait_for_init_b(struct fpga_manager *mgr, int value,
 
 	if (conf->init_b) {
 		while (time_before(jiffies, timeout)) {
-			/* dump_state(conf, "wait for init_d .."); */
 			if (gpiod_get_value(conf->init_b) == value)
 				return 0;
 			usleep_range(100, 400);
-- 
2.28.0

[PATCH v4 2/5] fpga manager: xilinx-spi: remove final dot from dev_err() strings

[Luca Ceresoli]

Most dev_err messages in this file have no final dot. Remove the only two
exceptions to make them consistent.

Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes in v4:
 - add Reviewed-by Tom Rix

Changes in v3: none.

Changes in v2:
 - move before the "provide better diagnostics on programming failure"
   patch for clarity
---
 drivers/fpga/xilinx-spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index 502fae0d1d85..01f494172379 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -77,7 +77,7 @@ static int xilinx_spi_write_init(struct fpga_manager *mgr,
 	int err;
 
 	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
-		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
+		dev_err(&mgr->dev, "Partial reconfiguration not supported\n");
 		return -EINVAL;
 	}
 
@@ -169,7 +169,7 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
 			return xilinx_spi_apply_cclk_cycles(conf);
 	}
 
-	dev_err(&mgr->dev, "Timeout after config data transfer.\n");
+	dev_err(&mgr->dev, "Timeout after config data transfer\n");
 	return -ETIMEDOUT;
 }
 
-- 
2.28.0

[PATCH v4 3/5] fpga manager: xilinx-spi: fix write_complete timeout handling

[Luca Ceresoli]

If this routine sleeps because it was scheduled out, it might miss DONE
going asserted and consider it a timeout. This would potentially make the
code return an error even when programming succeeded. Rewrite the loop to
always check DONE after checking if timeout expired so this cannot happen
anymore.

While there, also add error checking for gpiod_get_value(). Also avoid
checking the DONE GPIO in two places, which would make the error-checking
code duplicated and more annoying.

The new loop it written to still guarantee that we apply 8 extra CCLK
cycles after DONE has gone asserted, which is required by the hardware.

Reported-by: Tom Rix <trix@redhat.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes in v4:
 - add Reviewed-by Tom Rix
 - fix uninitialized variable
   (Reported-by: kernel test robot <lkp@intel.com>)

Changes in v3:
 - completely rewrite the loop after Tom pointed out the 'sleep' bug

This patch is new in v2
---
 drivers/fpga/xilinx-spi.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index 01f494172379..fba8eb4866a7 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -151,22 +151,29 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
 				     struct fpga_image_info *info)
 {
 	struct xilinx_spi_conf *conf = mgr->priv;
-	unsigned long timeout;
+	unsigned long timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
+	bool expired = false;
+	int done;
 	int ret;
 
-	if (gpiod_get_value(conf->done))
-		return xilinx_spi_apply_cclk_cycles(conf);
+	/*
+	 * This loop is carefully written such that if the driver is
+	 * scheduled out for more than 'timeout', we still check for DONE
+	 * before giving up and we apply 8 extra CCLK cycles in all cases.
+	 */
+	while (!expired) {
+		expired = time_after(jiffies, timeout);
 
-	timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
-
-	while (time_before(jiffies, timeout)) {
+		done = get_done_gpio(mgr);
+		if (done < 0)
+			return done;
 
 		ret = xilinx_spi_apply_cclk_cycles(conf);
 		if (ret)
 			return ret;
 
-		if (gpiod_get_value(conf->done))
-			return xilinx_spi_apply_cclk_cycles(conf);
+		if (done)
+			return 0;
 	}
 
 	dev_err(&mgr->dev, "Timeout after config data transfer\n");
-- 
2.28.0

[PATCH v4 4/5] fpga manager: xilinx-spi: add error checking after gpiod_get_value()

[Luca Ceresoli]

Current code calls gpiod_get_value() without error checking. Should the
GPIO controller fail, execution would continue without any error message.

Fix by checking for negative error values.

Reported-by: Tom Rix <trix@redhat.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes in v4:
 - add Reviewed-by Tom Rix

Changes in v3:
 - rebase on previous patches

This patch is new in v2
---
 drivers/fpga/xilinx-spi.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index fba8eb4866a7..52aab5a1f0ba 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -27,11 +27,22 @@ struct xilinx_spi_conf {
 	struct gpio_desc *done;
 };
 
-static enum fpga_mgr_states xilinx_spi_state(struct fpga_manager *mgr)
+static int get_done_gpio(struct fpga_manager *mgr)
 {
 	struct xilinx_spi_conf *conf = mgr->priv;
+	int ret;
+
+	ret = gpiod_get_value(conf->done);
+
+	if (ret < 0)
+		dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
 
-	if (!gpiod_get_value(conf->done))
+	return ret;
+}
+
+static enum fpga_mgr_states xilinx_spi_state(struct fpga_manager *mgr)
+{
+	if (!get_done_gpio(mgr))
 		return FPGA_MGR_STATE_RESET;
 
 	return FPGA_MGR_STATE_UNKNOWN;
@@ -57,10 +68,21 @@ static int wait_for_init_b(struct fpga_manager *mgr, int value,
 
 	if (conf->init_b) {
 		while (time_before(jiffies, timeout)) {
-			if (gpiod_get_value(conf->init_b) == value)
+			int ret = gpiod_get_value(conf->init_b);
+
+			if (ret == value)
 				return 0;
+
+			if (ret < 0) {
+				dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
+				return ret;
+			}
+
 			usleep_range(100, 400);
 		}
+
+		dev_err(&mgr->dev, "Timeout waiting for INIT_B to %s\n",
+			value ? "assert" : "deassert");
 		return -ETIMEDOUT;
 	}
 
@@ -85,7 +107,6 @@ static int xilinx_spi_write_init(struct fpga_manager *mgr,
 
 	err = wait_for_init_b(mgr, 1, 1); /* min is 500 ns */
 	if (err) {
-		dev_err(&mgr->dev, "INIT_B pin did not go low\n");
 		gpiod_set_value(conf->prog_b, 0);
 		return err;
 	}
@@ -93,12 +114,10 @@ static int xilinx_spi_write_init(struct fpga_manager *mgr,
 	gpiod_set_value(conf->prog_b, 0);
 
 	err = wait_for_init_b(mgr, 0, 0);
-	if (err) {
-		dev_err(&mgr->dev, "INIT_B pin did not go high\n");
+	if (err)
 		return err;
-	}
 
-	if (gpiod_get_value(conf->done)) {
+	if (get_done_gpio(mgr)) {
 		dev_err(&mgr->dev, "Unexpected DONE pin state...\n");
 		return -EIO;
 	}
-- 
2.28.0

[PATCH v4 5/5] fpga manager: xilinx-spi: provide better diagnostics on programming failure

[Luca Ceresoli]

When the DONE pin does not go high after programming to confirm programming
success, the INIT_B pin provides some info on the reason. Use it if
available to provide a more explanatory error message.

Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes in v4:
 - add Reviewed-by Tom Rix

Changes in v3: none.

Changes in v2:
 - also check for gpiod_get_value() errors (Tom Rix)
---
 drivers/fpga/xilinx-spi.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index 52aab5a1f0ba..824abbbd631e 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -195,7 +195,21 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
 			return 0;
 	}
 
-	dev_err(&mgr->dev, "Timeout after config data transfer\n");
+	if (conf->init_b) {
+		ret = gpiod_get_value(conf->init_b);
+
+		if (ret < 0) {
+			dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
+			return ret;
+		}
+
+		dev_err(&mgr->dev,
+			ret ? "CRC error or invalid device\n"
+			: "Missing sync word or incomplete bitstream\n");
+	} else {
+		dev_err(&mgr->dev, "Timeout after config data transfer\n");
+	}
+
 	return -ETIMEDOUT;
 }
 
-- 
2.28.0

Re: [PATCH v4 1/5] fpga manager: xilinx-spi: remove stray comment

[Moritz Fischer]

On Sun, Aug 30, 2020 at 06:38:46PM +0200, Luca Ceresoli wrote:
> Remove comment committed by mistake.
> 
> Fixes: dd2784c01d93 ("fpga manager: xilinx-spi: check INIT_B pin during write_init")
> Reviewed-by: Tom Rix <trix@redhat.com>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes in v4:
>  - add Reviewed-by Tom Rix
> 
> Changes in v3: none.
> 
> Changes in v2: none.
> ---
>  drivers/fpga/xilinx-spi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index 2967aa2a74e2..502fae0d1d85 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -57,7 +57,6 @@ static int wait_for_init_b(struct fpga_manager *mgr, int value,
>  
>  	if (conf->init_b) {
>  		while (time_before(jiffies, timeout)) {
> -			/* dump_state(conf, "wait for init_d .."); */
>  			if (gpiod_get_value(conf->init_b) == value)
>  				return 0;
>  			usleep_range(100, 400);
> -- 
> 2.28.0
> 
Earlier version was applied to for-next (forgot-push) ...

Thanks,
Moritz

Re: [PATCH v4 2/5] fpga manager: xilinx-spi: remove final dot from dev_err() strings

[Moritz Fischer]

On Sun, Aug 30, 2020 at 06:38:47PM +0200, Luca Ceresoli wrote:
> Most dev_err messages in this file have no final dot. Remove the only two
> exceptions to make them consistent.
> 
> Reviewed-by: Tom Rix <trix@redhat.com>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes in v4:
>  - add Reviewed-by Tom Rix
> 
> Changes in v3: none.
> 
> Changes in v2:
>  - move before the "provide better diagnostics on programming failure"
>    patch for clarity
> ---
>  drivers/fpga/xilinx-spi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index 502fae0d1d85..01f494172379 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -77,7 +77,7 @@ static int xilinx_spi_write_init(struct fpga_manager *mgr,
>  	int err;
>  
>  	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> -		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> +		dev_err(&mgr->dev, "Partial reconfiguration not supported\n");
>  		return -EINVAL;
>  	}
>  
> @@ -169,7 +169,7 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
>  			return xilinx_spi_apply_cclk_cycles(conf);
>  	}
>  
> -	dev_err(&mgr->dev, "Timeout after config data transfer.\n");
> +	dev_err(&mgr->dev, "Timeout after config data transfer\n");
>  	return -ETIMEDOUT;
>  }
>  
> -- 
> 2.28.0
> 
Applied to for-next,

Thanks

Re: [PATCH v4 3/5] fpga manager: xilinx-spi: fix write_complete timeout handling

[Moritz Fischer]

On Sun, Aug 30, 2020 at 06:38:48PM +0200, Luca Ceresoli wrote:
> If this routine sleeps because it was scheduled out, it might miss DONE
> going asserted and consider it a timeout. This would potentially make the
> code return an error even when programming succeeded. Rewrite the loop to
> always check DONE after checking if timeout expired so this cannot happen
> anymore.
> 
> While there, also add error checking for gpiod_get_value(). Also avoid
> checking the DONE GPIO in two places, which would make the error-checking
> code duplicated and more annoying.
> 
> The new loop it written to still guarantee that we apply 8 extra CCLK
> cycles after DONE has gone asserted, which is required by the hardware.
> 
> Reported-by: Tom Rix <trix@redhat.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes in v4:
>  - add Reviewed-by Tom Rix
>  - fix uninitialized variable
>    (Reported-by: kernel test robot <lkp@intel.com>)
> 
> Changes in v3:
>  - completely rewrite the loop after Tom pointed out the 'sleep' bug
> 
> This patch is new in v2
> ---
>  drivers/fpga/xilinx-spi.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index 01f494172379..fba8eb4866a7 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -151,22 +151,29 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
>  				     struct fpga_image_info *info)
>  {
>  	struct xilinx_spi_conf *conf = mgr->priv;
> -	unsigned long timeout;
> +	unsigned long timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
> +	bool expired = false;
> +	int done;
>  	int ret;
>  
> -	if (gpiod_get_value(conf->done))
> -		return xilinx_spi_apply_cclk_cycles(conf);
> +	/*
> +	 * This loop is carefully written such that if the driver is
> +	 * scheduled out for more than 'timeout', we still check for DONE
> +	 * before giving up and we apply 8 extra CCLK cycles in all cases.
> +	 */
> +	while (!expired) {
> +		expired = time_after(jiffies, timeout);
>  
> -	timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
> -
> -	while (time_before(jiffies, timeout)) {
> +		done = get_done_gpio(mgr);
> +		if (done < 0)
> +			return done;
>  
>  		ret = xilinx_spi_apply_cclk_cycles(conf);
>  		if (ret)
>  			return ret;
>  
> -		if (gpiod_get_value(conf->done))
> -			return xilinx_spi_apply_cclk_cycles(conf);
> +		if (done)
> +			return 0;
>  	}
>  
>  	dev_err(&mgr->dev, "Timeout after config data transfer\n");
> -- 
> 2.28.0
> 

Applied to for-next,

Thanks

Re: [PATCH v4 4/5] fpga manager: xilinx-spi: add error checking after gpiod_get_value()

[Moritz Fischer]

On Sun, Aug 30, 2020 at 06:38:49PM +0200, Luca Ceresoli wrote:
> Current code calls gpiod_get_value() without error checking. Should the
> GPIO controller fail, execution would continue without any error message.
> 
> Fix by checking for negative error values.
> 
> Reported-by: Tom Rix <trix@redhat.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes in v4:
>  - add Reviewed-by Tom Rix
> 
> Changes in v3:
>  - rebase on previous patches
> 
> This patch is new in v2
> ---
>  drivers/fpga/xilinx-spi.c | 35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index fba8eb4866a7..52aab5a1f0ba 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -27,11 +27,22 @@ struct xilinx_spi_conf {
>  	struct gpio_desc *done;
>  };
>  
> -static enum fpga_mgr_states xilinx_spi_state(struct fpga_manager *mgr)
> +static int get_done_gpio(struct fpga_manager *mgr)
>  {
>  	struct xilinx_spi_conf *conf = mgr->priv;
> +	int ret;
> +
> +	ret = gpiod_get_value(conf->done);
> +
> +	if (ret < 0)
> +		dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
>  
> -	if (!gpiod_get_value(conf->done))
> +	return ret;
> +}
> +
> +static enum fpga_mgr_states xilinx_spi_state(struct fpga_manager *mgr)
> +{
> +	if (!get_done_gpio(mgr))
>  		return FPGA_MGR_STATE_RESET;
>  
>  	return FPGA_MGR_STATE_UNKNOWN;
> @@ -57,10 +68,21 @@ static int wait_for_init_b(struct fpga_manager *mgr, int value,
>  
>  	if (conf->init_b) {
>  		while (time_before(jiffies, timeout)) {
> -			if (gpiod_get_value(conf->init_b) == value)
> +			int ret = gpiod_get_value(conf->init_b);
> +
> +			if (ret == value)
>  				return 0;
> +
> +			if (ret < 0) {
> +				dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
> +				return ret;
> +			}
> +
>  			usleep_range(100, 400);
>  		}
> +
> +		dev_err(&mgr->dev, "Timeout waiting for INIT_B to %s\n",
> +			value ? "assert" : "deassert");
>  		return -ETIMEDOUT;
>  	}
>  
> @@ -85,7 +107,6 @@ static int xilinx_spi_write_init(struct fpga_manager *mgr,
>  
>  	err = wait_for_init_b(mgr, 1, 1); /* min is 500 ns */
>  	if (err) {
> -		dev_err(&mgr->dev, "INIT_B pin did not go low\n");
>  		gpiod_set_value(conf->prog_b, 0);
>  		return err;
>  	}
> @@ -93,12 +114,10 @@ static int xilinx_spi_write_init(struct fpga_manager *mgr,
>  	gpiod_set_value(conf->prog_b, 0);
>  
>  	err = wait_for_init_b(mgr, 0, 0);
> -	if (err) {
> -		dev_err(&mgr->dev, "INIT_B pin did not go high\n");
> +	if (err)
>  		return err;
> -	}
>  
> -	if (gpiod_get_value(conf->done)) {
> +	if (get_done_gpio(mgr)) {
>  		dev_err(&mgr->dev, "Unexpected DONE pin state...\n");
>  		return -EIO;
>  	}
> -- 
> 2.28.0
> 
Applied to for-next,

Thanks

Re: [PATCH v4 5/5] fpga manager: xilinx-spi: provide better diagnostics on programming failure

[Moritz Fischer]

On Sun, Aug 30, 2020 at 06:38:50PM +0200, Luca Ceresoli wrote:
> When the DONE pin does not go high after programming to confirm programming
> success, the INIT_B pin provides some info on the reason. Use it if
> available to provide a more explanatory error message.
> 
> Reviewed-by: Tom Rix <trix@redhat.com>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes in v4:
>  - add Reviewed-by Tom Rix
> 
> Changes in v3: none.
> 
> Changes in v2:
>  - also check for gpiod_get_value() errors (Tom Rix)
> ---
>  drivers/fpga/xilinx-spi.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index 52aab5a1f0ba..824abbbd631e 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -195,7 +195,21 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
>  			return 0;
>  	}
>  
> -	dev_err(&mgr->dev, "Timeout after config data transfer\n");
> +	if (conf->init_b) {
> +		ret = gpiod_get_value(conf->init_b);
> +
> +		if (ret < 0) {
> +			dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
> +			return ret;
> +		}
> +
> +		dev_err(&mgr->dev,
> +			ret ? "CRC error or invalid device\n"
> +			: "Missing sync word or incomplete bitstream\n");
> +	} else {
> +		dev_err(&mgr->dev, "Timeout after config data transfer\n");
> +	}
> +
>  	return -ETIMEDOUT;
>  }
>  
> -- 
> 2.28.0
> 
Applied to for-next,

Thanks