* [PATCH v2 2/5] fpga manager: xilinx-spi: remove final dot from dev_err() strings
2020-08-27 14:32 [PATCH v2 1/5] fpga manager: xilinx-spi: remove stray comment Luca Ceresoli
@ 2020-08-27 14:32 ` Luca Ceresoli
2020-08-27 14:32 ` [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation Luca Ceresoli
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Luca Ceresoli @ 2020-08-27 14:32 UTC (permalink / raw)
To: linux-fpga
Cc: Luca Ceresoli, Moritz Fischer, Tom Rix, Michal Simek,
linux-arm-kernel, linux-kernel, Anatolij Gustschin
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 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation
2020-08-27 14:32 [PATCH v2 1/5] fpga manager: xilinx-spi: remove stray comment Luca Ceresoli
2020-08-27 14:32 ` [PATCH v2 2/5] fpga manager: xilinx-spi: remove final dot from dev_err() strings Luca Ceresoli
@ 2020-08-27 14:32 ` Luca Ceresoli
2020-08-27 18:59 ` Tom Rix
2020-08-27 14:32 ` [PATCH v2 4/5] fpga manager: xilinx-spi: add error checking after gpiod_get_value() Luca Ceresoli
2020-08-27 14:32 ` [PATCH v2 5/5] fpga manager: xilinx-spi: provide better diagnostics on programming failure Luca Ceresoli
3 siblings, 1 reply; 11+ messages in thread
From: Luca Ceresoli @ 2020-08-27 14:32 UTC (permalink / raw)
To: linux-fpga
Cc: Luca Ceresoli, Moritz Fischer, Tom Rix, Michal Simek,
linux-arm-kernel, linux-kernel, Anatolij Gustschin
In preparation to add error checking for gpiod_get_value(), rework
the loop to avoid the duplication of these lines:
if (gpiod_get_value(conf->done))
return xilinx_spi_apply_cclk_cycles(conf);
There is little advantage in this rework with current code. However
error checking will expand these two lines to five, making code
duplication more annoying.
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
This patch is new in v2
---
drivers/fpga/xilinx-spi.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index 01f494172379..cfc933d70f52 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -151,22 +151,19 @@ 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);
int ret;
- if (gpiod_get_value(conf->done))
- return xilinx_spi_apply_cclk_cycles(conf);
-
- timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
+ while (true) {
+ if (gpiod_get_value(conf->done))
+ return xilinx_spi_apply_cclk_cycles(conf);
- while (time_before(jiffies, timeout)) {
+ if (time_after(jiffies, timeout))
+ break;
ret = xilinx_spi_apply_cclk_cycles(conf);
if (ret)
return ret;
-
- if (gpiod_get_value(conf->done))
- return xilinx_spi_apply_cclk_cycles(conf);
}
dev_err(&mgr->dev, "Timeout after config data transfer\n");
--
2.28.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation
2020-08-27 14:32 ` [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation Luca Ceresoli
@ 2020-08-27 18:59 ` Tom Rix
2020-08-27 19:26 ` Luca Ceresoli
0 siblings, 1 reply; 11+ messages in thread
From: Tom Rix @ 2020-08-27 18:59 UTC (permalink / raw)
To: Luca Ceresoli, linux-fpga
Cc: Moritz Fischer, Michal Simek, linux-arm-kernel, linux-kernel,
Anatolij Gustschin
On 8/27/20 7:32 AM, Luca Ceresoli wrote:
> In preparation to add error checking for gpiod_get_value(), rework
> the loop to avoid the duplication of these lines:
>
> if (gpiod_get_value(conf->done))
> return xilinx_spi_apply_cclk_cycles(conf);
>
> There is little advantage in this rework with current code. However
> error checking will expand these two lines to five, making code
> duplication more annoying.
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> ---
>
> This patch is new in v2
> ---
> drivers/fpga/xilinx-spi.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index 01f494172379..cfc933d70f52 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -151,22 +151,19 @@ 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);
> int ret;
>
> - if (gpiod_get_value(conf->done))
> - return xilinx_spi_apply_cclk_cycles(conf);
> -
> - timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
> + while (true) {
> + if (gpiod_get_value(conf->done))
> + return xilinx_spi_apply_cclk_cycles(conf);
>
> - while (time_before(jiffies, timeout)) {
> + if (time_after(jiffies, timeout))
> + break;
>
> ret = xilinx_spi_apply_cclk_cycles(conf);
> if (ret)
> return ret;
> -
> - if (gpiod_get_value(conf->done))
> - return xilinx_spi_apply_cclk_cycles(conf);
> }
Do you need another
if (gpiod_get_value(conf->done))
return xilinx_spi_apply_cclk_cycles(conf);
here to cover the chance of sleeping in the loop ?
Tom
>
> dev_err(&mgr->dev, "Timeout after config data transfer\n");
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation
2020-08-27 18:59 ` Tom Rix
@ 2020-08-27 19:26 ` Luca Ceresoli
[not found] ` <27bbb896-fedf-6a3a-7220-5c57239a3b87@redhat.com>
0 siblings, 1 reply; 11+ messages in thread
From: Luca Ceresoli @ 2020-08-27 19:26 UTC (permalink / raw)
To: Tom Rix, linux-fpga
Cc: Moritz Fischer, Michal Simek, linux-arm-kernel, linux-kernel,
Anatolij Gustschin
Hi Tom,
thanks for the prompt feedback!
On 27/08/20 20:59, Tom Rix wrote:
>
> On 8/27/20 7:32 AM, Luca Ceresoli wrote:
>> In preparation to add error checking for gpiod_get_value(), rework
>> the loop to avoid the duplication of these lines:
>>
>> if (gpiod_get_value(conf->done))
>> return xilinx_spi_apply_cclk_cycles(conf);
>>
>> There is little advantage in this rework with current code. However
>> error checking will expand these two lines to five, making code
>> duplication more annoying.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> ---
>>
>> This patch is new in v2
>> ---
>> drivers/fpga/xilinx-spi.c | 15 ++++++---------
>> 1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
>> index 01f494172379..cfc933d70f52 100644
>> --- a/drivers/fpga/xilinx-spi.c
>> +++ b/drivers/fpga/xilinx-spi.c
>> @@ -151,22 +151,19 @@ 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);
>> int ret;
>>
>> - if (gpiod_get_value(conf->done))
>> - return xilinx_spi_apply_cclk_cycles(conf);
>> -
>> - timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
>> + while (true) {
>> + if (gpiod_get_value(conf->done))
>> + return xilinx_spi_apply_cclk_cycles(conf);
>>
>> - while (time_before(jiffies, timeout)) {
>> + if (time_after(jiffies, timeout))
>> + break;
>>
>> ret = xilinx_spi_apply_cclk_cycles(conf);
>> if (ret)
>> return ret;
>> -
>> - if (gpiod_get_value(conf->done))
>> - return xilinx_spi_apply_cclk_cycles(conf);
>> }
>
> Do you need another
>
> if (gpiod_get_value(conf->done))
> return xilinx_spi_apply_cclk_cycles(conf);
>
> here to cover the chance of sleeping in the loop ?
If I got your question correctly: if we get here it's because of a
timeout, thus programming has failed (DONE didn't come up after some
time), and checking it one more here seems pointless.
Does this reply your question?
--
Luca
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/5] fpga manager: xilinx-spi: add error checking after gpiod_get_value()
2020-08-27 14:32 [PATCH v2 1/5] fpga manager: xilinx-spi: remove stray comment Luca Ceresoli
2020-08-27 14:32 ` [PATCH v2 2/5] fpga manager: xilinx-spi: remove final dot from dev_err() strings Luca Ceresoli
2020-08-27 14:32 ` [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation Luca Ceresoli
@ 2020-08-27 14:32 ` Luca Ceresoli
2020-08-27 19:04 ` Tom Rix
2020-08-27 14:32 ` [PATCH v2 5/5] fpga manager: xilinx-spi: provide better diagnostics on programming failure Luca Ceresoli
3 siblings, 1 reply; 11+ messages in thread
From: Luca Ceresoli @ 2020-08-27 14:32 UTC (permalink / raw)
To: linux-fpga
Cc: Luca Ceresoli, Moritz Fischer, Tom Rix, Michal Simek,
linux-arm-kernel, linux-kernel, Anatolij Gustschin
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>
---
This patch is new in v2
---
drivers/fpga/xilinx-spi.c | 40 ++++++++++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index cfc933d70f52..bcc740ec42f5 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 (!gpiod_get_value(conf->done))
+ if (ret < 0)
+ dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
+
+ 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;
}
@@ -155,7 +174,10 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
int ret;
while (true) {
- if (gpiod_get_value(conf->done))
+ ret = get_done_gpio(mgr);
+ if (ret < 0)
+ return ret;
+ if (ret)
return xilinx_spi_apply_cclk_cycles(conf);
if (time_after(jiffies, timeout))
--
2.28.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 5/5] fpga manager: xilinx-spi: provide better diagnostics on programming failure
2020-08-27 14:32 [PATCH v2 1/5] fpga manager: xilinx-spi: remove stray comment Luca Ceresoli
` (2 preceding siblings ...)
2020-08-27 14:32 ` [PATCH v2 4/5] fpga manager: xilinx-spi: add error checking after gpiod_get_value() Luca Ceresoli
@ 2020-08-27 14:32 ` Luca Ceresoli
2020-08-27 19:09 ` Tom Rix
3 siblings, 1 reply; 11+ messages in thread
From: Luca Ceresoli @ 2020-08-27 14:32 UTC (permalink / raw)
To: linux-fpga
Cc: Luca Ceresoli, Moritz Fischer, Tom Rix, Michal Simek,
linux-arm-kernel, linux-kernel, Anatolij Gustschin
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 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 bcc740ec42f5..ea0351be8eee 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -188,7 +188,21 @@ static int xilinx_spi_write_complete(struct fpga_manager *mgr,
return ret;
}
- 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
^ permalink raw reply related [flat|nested] 11+ messages in thread