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

[PATCH v3 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")
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

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 v3 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.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

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 v3 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>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

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..a7b919eb0b2a 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;
+	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 v3 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>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

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 a7b919eb0b2a..9488c8fbaefd 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

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

[kernel test robot]

Hi Luca,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.9-rc2]
[also build test WARNING on next-20200828]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Luca-Ceresoli/fpga-manager-xilinx-spi-remove-stray-comment/20200829-040041
base:    d012a7190fc1fd72ed48911e77ca97ba4521bccd
compiler: nds32le-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

>> drivers/fpga/xilinx-spi.c:183:10: warning: Uninitialized variable: expired [uninitvar]
    while (!expired) {
            ^

# https://github.com/0day-ci/linux/commit/5ae295c0b82631de73665c58df85ec3ed8567a8e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Luca-Ceresoli/fpga-manager-xilinx-spi-remove-stray-comment/20200829-040041
git checkout 5ae295c0b82631de73665c58df85ec3ed8567a8e
vim +183 drivers/fpga/xilinx-spi.c

061c97d13f1a69 Anatolij Gustschin 2017-03-23  168  
061c97d13f1a69 Anatolij Gustschin 2017-03-23  169  static int xilinx_spi_write_complete(struct fpga_manager *mgr,
061c97d13f1a69 Anatolij Gustschin 2017-03-23  170  				     struct fpga_image_info *info)
061c97d13f1a69 Anatolij Gustschin 2017-03-23  171  {
061c97d13f1a69 Anatolij Gustschin 2017-03-23  172  	struct xilinx_spi_conf *conf = mgr->priv;
629463d1acc532 Luca Ceresoli      2020-08-28  173  	unsigned long timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
629463d1acc532 Luca Ceresoli      2020-08-28  174  	bool expired;
629463d1acc532 Luca Ceresoli      2020-08-28  175  	int done;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  176  	int ret;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  177  
629463d1acc532 Luca Ceresoli      2020-08-28  178  	/*
629463d1acc532 Luca Ceresoli      2020-08-28  179  	 * This loop is carefully written such that if the driver is
629463d1acc532 Luca Ceresoli      2020-08-28  180  	 * scheduled out for more than 'timeout', we still check for DONE
629463d1acc532 Luca Ceresoli      2020-08-28  181  	 * before giving up and we apply 8 extra CCLK cycles in all cases.
629463d1acc532 Luca Ceresoli      2020-08-28  182  	 */
629463d1acc532 Luca Ceresoli      2020-08-28 @183  	while (!expired) {
629463d1acc532 Luca Ceresoli      2020-08-28  184  		expired = time_after(jiffies, timeout);
061c97d13f1a69 Anatolij Gustschin 2017-03-23  185  
629463d1acc532 Luca Ceresoli      2020-08-28  186  		done = get_done_gpio(mgr);
629463d1acc532 Luca Ceresoli      2020-08-28  187  		if (done < 0)
629463d1acc532 Luca Ceresoli      2020-08-28  188  			return done;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  189  
061c97d13f1a69 Anatolij Gustschin 2017-03-23  190  		ret = xilinx_spi_apply_cclk_cycles(conf);
061c97d13f1a69 Anatolij Gustschin 2017-03-23  191  		if (ret)
061c97d13f1a69 Anatolij Gustschin 2017-03-23  192  			return ret;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  193  
629463d1acc532 Luca Ceresoli      2020-08-28  194  		if (done)
629463d1acc532 Luca Ceresoli      2020-08-28  195  			return 0;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  196  	}
061c97d13f1a69 Anatolij Gustschin 2017-03-23  197  
70cac0e8c9ec83 Luca Ceresoli      2020-08-28  198  	dev_err(&mgr->dev, "Timeout after config data transfer\n");
061c97d13f1a69 Anatolij Gustschin 2017-03-23  199  	return -ETIMEDOUT;
061c97d13f1a69 Anatolij Gustschin 2017-03-23  200  }
061c97d13f1a69 Anatolij Gustschin 2017-03-23  201  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[Luca Ceresoli]

Hi,

On 29/08/20 05:30, kernel test robot wrote:
> Hi Luca,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on v5.9-rc2]
> [also build test WARNING on next-20200828]
> [cannot apply to xlnx/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Luca-Ceresoli/fpga-manager-xilinx-spi-remove-stray-comment/20200829-040041
> base:    d012a7190fc1fd72ed48911e77ca97ba4521bccd
> compiler: nds32le-linux-gcc (GCC) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> 
> cppcheck warnings: (new ones prefixed by >>)
> 
>>> drivers/fpga/xilinx-spi.c:183:10: warning: Uninitialized variable: expired [uninitvar]
>     while (!expired) {
>             ^

Oh dear, Please ignore this patch, v4 will be coming with this fixed.

-- 
Luca

[PATCH v3 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.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

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 9488c8fbaefd..c2e0a42cd4f2 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 v3 5/5] fpga manager: xilinx-spi: provide better diagnostics on programming failure

[Tom Rix]

On 8/28/20 12:58 PM, 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.
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

This patch set looks good to me.

Thanks

Tom

Reviewed-by: Tom Rix <trix@redhat.com>