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

[PATCH v2 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 v2: none.

Note: Moritz replied "Applied to for-next", but it doesn't show up yet.
---
 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 v2 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 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 v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation

[Luca Ceresoli]

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

[PATCH v2 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>

---

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

[PATCH v2 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 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

Re: [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation

[Tom Rix]

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");

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

[Tom Rix]

On 8/27/20 7:32 AM, 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>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> ---

Thanks, this looks fine.

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

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

[Tom Rix]

On 8/27/20 7:32 AM, 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>
>
> ---
>
> Changes in v2:
>  - also check for gpiod_get_value() errors (Tom Rix)

This looks fine.

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

Re: [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation

[Luca Ceresoli]

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

Re: [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation

[Luca Ceresoli]

Hi Tom,

On 27/08/20 21:34, Tom Rix wrote:
> 
> On 8/27/20 12:26 PM, Luca Ceresoli wrote:
>> 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.
> 
> It may not be pointless, if this routine sleeps because it was scheduled out, when it wakes up a lot of time  happened. You will see this as a timeout but the state may be good.  Another, final check at the end will cover this case.

Oh, now I got your point! Yes, there is this risk, and it exists in
current code as well but with a smaller risk window. Unrolling the
current and new loop code they behave the same except for the position
of the timeout computation (after vs before the first 'if (done) return'
group).

I think this reimplementation is sleep-safe, check for GPIO errors and
also avoid code duplication:

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 = jiffies +
		usecs_to_jiffies(info->config_complete_timeout_us);
	bool expired;
	int done;
	int ret;

	while (!expired) {
		expired = time_after(jiffies, timeout);

		done = get_done_gpio(mgr);
		if (done < 0)
			return done;

		ret = xilinx_spi_apply_cclk_cycles(conf);
		if (ret)
			return ret;

		if (done)
			return 0;
	}

	dev_err(&mgr->dev, "Timeout after config data transfer\n");

	return -ETIMEDOUT;
}

A key point is to assess all the status (expired and done variables)
before taking any action based on it. Then we can unconditionally apply
8 cclk cycles before even checking the actual DONE value, so that we
always do that after DONE has been seen asserted.

Does it look good?

-- 
Luca

Re: [PATCH v2 3/5] fpga manager: xilinx-spi: rework write_complete loop implementation

[Tom Rix]

On 8/27/20 11:38 PM, Luca Ceresoli wrote:
> Hi Tom,
>
> On 27/08/20 21:34, Tom Rix wrote:
>> On 8/27/20 12:26 PM, Luca Ceresoli wrote:
>>> 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.
>> It may not be pointless, if this routine sleeps because it was scheduled out, when it wakes up a lot of time  happened. You will see this as a timeout but the state may be good.  Another, final check at the end will cover this case.
> Oh, now I got your point! Yes, there is this risk, and it exists in
> current code as well but with a smaller risk window. Unrolling the
> current and new loop code they behave the same except for the position
> of the timeout computation (after vs before the first 'if (done) return'
> group).
>
> I think this reimplementation is sleep-safe, check for GPIO errors and
> also avoid code duplication:
>
> 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 = jiffies +
> 		usecs_to_jiffies(info->config_complete_timeout_us);
> 	bool expired;
> 	int done;
> 	int ret;
>
> 	while (!expired) {
> 		expired = time_after(jiffies, timeout);
>
> 		done = get_done_gpio(mgr);
> 		if (done < 0)
> 			return done;
>
> 		ret = xilinx_spi_apply_cclk_cycles(conf);
> 		if (ret)
> 			return ret;
>
> 		if (done)
> 			return 0;
> 	}
>
> 	dev_err(&mgr->dev, "Timeout after config data transfer\n");
>
> 	return -ETIMEDOUT;
> }
>
> A key point is to assess all the status (expired and done variables)
> before taking any action based on it. Then we can unconditionally apply
> 8 cclk cycles before even checking the actual DONE value, so that we
> always do that after DONE has been seen asserted.
>
> Does it look good?

Yes. Thanks for the extra work.

Tom

>