[PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Sean Anderson]

This series adds support for resetting the QSPI controller if we have a
timeout. I find this greatly improves the stability of the device, which
would tend to break after any timeout.


Sean Anderson (5):
  dt-bindings: spi: zynqmp-qspi: Add reset
  spi: zynqmp-gqspi: Reset device in probe
  spi: zynqmp-gqspi: Abort operations on timeout
  spi: zynqmp-gqspi: Allow interrupting operations
  ARM64: xilinx: zynqmp: Add QSPI reset

 .../bindings/spi/spi-zynqmp-qspi.yaml         |  6 ++
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi        |  1 +
 drivers/spi/spi-zynqmp-gqspi.c                | 64 +++++++++++++++----
 3 files changed, 59 insertions(+), 12 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH 1/5] dt-bindings: spi: zynqmp-qspi: Add reset

[Sean Anderson]

Add a reset to help recover from cancelled operations.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
index 04d4d3b4916d..901e15fcce2d 100644
--- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
@@ -36,12 +36,16 @@ properties:
   power-domains:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg
   - interrupts
   - clock-names
   - clocks
+  - resets
 
 unevaluatedProperties: false
 
@@ -66,6 +70,7 @@ allOf:
 examples:
   - |
     #include <dt-bindings/clock/xlnx-zynqmp-clk.h>
+    #include <dt-bindings/reset/xlnx-zynqmp-resets.h>
     soc {
       #address-cells = <2>;
       #size-cells = <2>;
@@ -76,6 +81,7 @@ examples:
         clock-names = "ref_clk", "pclk";
         interrupts = <0 15 4>;
         interrupt-parent = <&gic>;
+        resets = <&zynqmp_reset ZYNQMP_RESET_QSPI>;
         reg = <0x0 0xff0f0000 0x0 0x1000>,
               <0x0 0xc0000000 0x0 0x8000000>;
       };
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 2/5] spi: zynqmp-gqspi: Reset device in probe

[Sean Anderson]

Ensure we get a clean slate (without any bootloader settings) by
resetting the device before we initialize it.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/spi/spi-zynqmp-gqspi.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 549a6e0c9654..7d138f45b692 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/spi/spi.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
@@ -171,6 +172,7 @@ struct qspi_platform_data {
  * @regs:		Virtual address of the QSPI controller registers
  * @refclk:		Pointer to the peripheral clock
  * @pclk:		Pointer to the APB clock
+ * @reset:		Pointer to reset controller
  * @irq:		IRQ number
  * @dev:		Pointer to struct device
  * @txbuf:		Pointer to the TX buffer
@@ -193,6 +195,7 @@ struct zynqmp_qspi {
 	void __iomem *regs;
 	struct clk *refclk;
 	struct clk *pclk;
+	struct reset_control *reset;
 	int irq;
 	struct device *dev;
 	const void *txbuf;
@@ -351,10 +354,17 @@ static void zynqmp_qspi_set_tapdelay(struct zynqmp_qspi *xqspi, u32 baudrateval)
  *	- Set clock polarity and
  *	- Enable the QSPI controller
  */
-static void zynqmp_qspi_init_hw(struct zynqmp_qspi *xqspi)
+static int zynqmp_qspi_init_hw(struct zynqmp_qspi *xqspi)
 {
 	u32 config_reg, baud_rate_val = 0;
 	ulong clk_rate;
+	int ret;
+
+	ret = reset_control_reset(xqspi->reset);
+	if (ret) {
+		dev_err(xqspi->dev, "Unable to reset: %pe\n", &ret);
+		return ret;
+	}
 
 	/* Select the GQSPI mode */
 	zynqmp_gqspi_write(xqspi, GQSPI_SEL_OFST, GQSPI_SEL_MASK);
@@ -436,6 +446,8 @@ static void zynqmp_qspi_init_hw(struct zynqmp_qspi *xqspi)
 
 	/* Enable the GQSPI */
 	zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, GQSPI_EN_MASK);
+
+	return 0;
 }
 
 /**
@@ -1259,6 +1271,12 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 	if (IS_ERR(xqspi->regs))
 		return PTR_ERR(xqspi->regs);
 
+	xqspi->reset =
+		devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(xqspi->reset))
+		return dev_err_probe(dev, PTR_ERR(xqspi->reset),
+				     "could not get reset\n");
+
 	xqspi->pclk = devm_clk_get(&pdev->dev, "pclk");
 	if (IS_ERR(xqspi->pclk))
 		return dev_err_probe(dev, PTR_ERR(xqspi->pclk),
@@ -1300,7 +1318,9 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
 	xqspi->speed_hz = ctlr->max_speed_hz;
 
 	/* QSPI controller initializations */
-	zynqmp_qspi_init_hw(xqspi);
+	ret = zynqmp_qspi_init_hw(xqspi);
+	if (ret)
+		goto clk_dis_all;
 
 	xqspi->irq = platform_get_irq(pdev, 0);
 	if (xqspi->irq < 0) {
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 3/5] spi: zynqmp-gqspi: Abort operations on timeout

[Sean Anderson]

When an operation times out, we leave the device (and driver) in an
inconsistent state. This generally results in all subsequent operations
timing out. Attempt to address this by resetting/reinitializing the
device when we have a timeout. This tends to be fairly robust.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index 7d138f45b692..cf47466ec982 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1057,6 +1057,21 @@ static unsigned long zynqmp_qspi_timeout(struct zynqmp_qspi *xqspi, u8 bits,
 	return msecs_to_jiffies(timeout + 100);
 }
 
+
+static int zynqmp_qspi_wait(struct zynqmp_qspi *xqspi, unsigned long timeout)
+{
+	int ret;
+
+	ret = wait_for_completion_timeout(&xqspi->data_completion, timeout);
+	if (ret)
+		return 0;
+	dev_err(xqspi->dev, "Operation timed out\n");
+
+	/* Attempt to recover as best we can */
+	zynqmp_qspi_init_hw(xqspi);
+	return -ETIMEDOUT;
+}
+
 /**
  * zynqmp_qspi_exec_op() - Initiates the QSPI transfer
  * @mem: The SPI memory
@@ -1104,11 +1119,9 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
 				   GQSPI_IER_TXNOT_FULL_MASK);
 		timeout = zynqmp_qspi_timeout(xqspi, op->cmd.buswidth,
 					      op->cmd.nbytes);
-		if (!wait_for_completion_timeout(&xqspi->data_completion,
-						 timeout)) {
-			err = -ETIMEDOUT;
+		err = zynqmp_qspi_wait(xqspi, timeout);
+		if (err)
 			goto return_err;
-		}
 	}
 
 	if (op->addr.nbytes) {
@@ -1133,11 +1146,9 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
 				   GQSPI_IER_TXNOT_FULL_MASK);
 		timeout = zynqmp_qspi_timeout(xqspi, op->addr.buswidth,
 					      op->addr.nbytes);
-		if (!wait_for_completion_timeout(&xqspi->data_completion,
-						 timeout)) {
-			err = -ETIMEDOUT;
+		err = zynqmp_qspi_wait(xqspi, timeout);
+		if (err)
 			goto return_err;
-		}
 	}
 
 	if (op->dummy.nbytes) {
@@ -1204,8 +1215,7 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
 		}
 		timeout = zynqmp_qspi_timeout(xqspi, op->data.buswidth,
 					      op->data.nbytes);
-		if (!wait_for_completion_timeout(&xqspi->data_completion, timeout))
-			err = -ETIMEDOUT;
+		err = zynqmp_qspi_wait(xqspi, timeout);
 	}
 
 return_err:
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 4/5] spi: zynqmp-gqspi: Allow interrupting operations

[Sean Anderson]

Some operations (such as reading several megabytes of data from a flash)
can take several seconds or more. Users may want to cancel such
operations. Allow them to do so now that we have a way to recover.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/spi/spi-zynqmp-gqspi.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index cf47466ec982..fa4bff73324e 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1062,14 +1062,24 @@ static int zynqmp_qspi_wait(struct zynqmp_qspi *xqspi, unsigned long timeout)
 {
 	int ret;
 
-	ret = wait_for_completion_timeout(&xqspi->data_completion, timeout);
-	if (ret)
+	/* Only allow interrupting if we can reset the device */
+	if (xqspi->reset)
+		ret = wait_for_completion_interruptible_timeout(&xqspi->data_completion,
+								timeout);
+	else
+		ret = wait_for_completion_timeout(&xqspi->data_completion,
+						  timeout);
+	if (ret > 0)
 		return 0;
-	dev_err(xqspi->dev, "Operation timed out\n");
+
+	if (!ret) {
+		dev_err(xqspi->dev, "Operation timed out\n");
+		ret = -ETIMEDOUT;
+	}
 
 	/* Attempt to recover as best we can */
 	zynqmp_qspi_init_hw(xqspi);
-	return -ETIMEDOUT;
+	return ret;
 }
 
 /**
-- 
2.35.1.1320.gc452695387.dirty

[PATCH 5/5] ARM64: xilinx: zynqmp: Add QSPI reset

[Sean Anderson]

Add a reset to the QSPI.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 467f084c6469..5dac0542a48d 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -986,6 +986,7 @@ qspi: spi@ff0f0000 {
 			#size-cells = <0>;
 			/* iommus = <&smmu 0x873>; */
 			power-domains = <&zynqmp_firmware PD_QSPI>;
+			resets = <&zynqmp_reset ZYNQMP_RESET_QSPI>;
 		};
 
 		psgtr: phy@fd400000 {
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 1/5] dt-bindings: spi: zynqmp-qspi: Add reset

[Michal Simek]

On 1/16/25 23:55, Sean Anderson wrote:
> Add a reset to help recover from cancelled operations.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>   Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> index 04d4d3b4916d..901e15fcce2d 100644
> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> @@ -36,12 +36,16 @@ properties:
>     power-domains:
>       maxItems: 1
>   
> +  resets:
> +    maxItems: 1
> +
>   required:
>     - compatible
>     - reg
>     - interrupts
>     - clock-names
>     - clocks
> +  - resets

In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I 
expect reset is not really required property.

M

Re: [PATCH 3/5] spi: zynqmp-gqspi: Abort operations on timeout

[Michal Simek]

On 1/16/25 23:55, Sean Anderson wrote:
> When an operation times out, we leave the device (and driver) in an
> inconsistent state. This generally results in all subsequent operations
> timing out. Attempt to address this by resetting/reinitializing the
> device when we have a timeout. This tends to be fairly robust.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>   drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
> index 7d138f45b692..cf47466ec982 100644
> --- a/drivers/spi/spi-zynqmp-gqspi.c
> +++ b/drivers/spi/spi-zynqmp-gqspi.c
> @@ -1057,6 +1057,21 @@ static unsigned long zynqmp_qspi_timeout(struct zynqmp_qspi *xqspi, u8 bits,
>   	return msecs_to_jiffies(timeout + 100);
>   }
>   
> +

unnecessary newline here.

> +static int zynqmp_qspi_wait(struct zynqmp_qspi *xqspi, unsigned long timeout)
> +{
> +	int ret;
> +
> +	ret = wait_for_completion_timeout(&xqspi->data_completion, timeout);
> +	if (ret)
> +		return 0;

newline here please

> +	dev_err(xqspi->dev, "Operation timed out\n");
> +
> +	/* Attempt to recover as best we can */
> +	zynqmp_qspi_init_hw(xqspi);
> +	return -ETIMEDOUT;
> +}
> +
>   /**
>    * zynqmp_qspi_exec_op() - Initiates the QSPI transfer
>    * @mem: The SPI memory
> @@ -1104,11 +1119,9 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
>   				   GQSPI_IER_TXNOT_FULL_MASK);
>   		timeout = zynqmp_qspi_timeout(xqspi, op->cmd.buswidth,
>   					      op->cmd.nbytes);
> -		if (!wait_for_completion_timeout(&xqspi->data_completion,
> -						 timeout)) {
> -			err = -ETIMEDOUT;
> +		err = zynqmp_qspi_wait(xqspi, timeout);
> +		if (err)
>   			goto return_err;
> -		}
>   	}
>   
>   	if (op->addr.nbytes) {
> @@ -1133,11 +1146,9 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
>   				   GQSPI_IER_TXNOT_FULL_MASK);
>   		timeout = zynqmp_qspi_timeout(xqspi, op->addr.buswidth,
>   					      op->addr.nbytes);
> -		if (!wait_for_completion_timeout(&xqspi->data_completion,
> -						 timeout)) {
> -			err = -ETIMEDOUT;
> +		err = zynqmp_qspi_wait(xqspi, timeout);
> +		if (err)
>   			goto return_err;
> -		}
>   	}
>   
>   	if (op->dummy.nbytes) {
> @@ -1204,8 +1215,7 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,
>   		}
>   		timeout = zynqmp_qspi_timeout(xqspi, op->data.buswidth,
>   					      op->data.nbytes);
> -		if (!wait_for_completion_timeout(&xqspi->data_completion, timeout))
> -			err = -ETIMEDOUT;
> +		err = zynqmp_qspi_wait(xqspi, timeout);
>   	}
>   
>   return_err:

M

Re: [PATCH 4/5] spi: zynqmp-gqspi: Allow interrupting operations

[Miquel Raynal]

Hello Sean,

On 16/01/2025 at 17:55:20 -05, Sean Anderson <sean.anderson@linux.dev> wrote:

> Some operations (such as reading several megabytes of data from a flash)
> can take several seconds or more. Users may want to cancel such
> operations. Allow them to do so now that we have a way to recover.

I fully agree with the observation, I tried myself interrupting too long
transfers with another spi controller:

e0205d6203c2c ("spi: atmel: Prevent false timeouts on long transfers")

But there were issues reported, so we limited the signals to SIGKILLs:

1ca2761a77349 ("spi: atmel: Do not cancel a transfer upon any signal")

But jffs2 plays with sigkills, so for spi memories it does not work
well, we had to revert:

890188d2d7e4a ("spi: atmel: Prevent spi transfers from being killed")

Same thing was also observed on Zynq7000:

26cfc0dbe43aa ("spi: spi-zynq-qspi: use wait_for_completion_timeout to make zynq_qspi_exec_mem_op not interruptible")

I would however hint to use a specific helper for deriving your timeouts
if you play with spi memories, because it is interesting to adapt the
values nevertheless:

d8e4ebf870187 ("spi: Create a helper to derive adaptive timeouts")

Thanks,
Miquèl

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Thu, Jan 16, 2025 at 05:55:16PM -0500, Sean Anderson wrote:
> This series adds support for resetting the QSPI controller if we have a
> timeout. I find this greatly improves the stability of the device, which
> would tend to break after any timeout.

If you're hitting a timeout that tends to indicate there's already a
serious stability problem...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] dt-bindings: spi: zynqmp-qspi: Add reset

[Sean Anderson]

On 1/17/25 02:14, Michal Simek wrote:
> 
> 
> On 1/16/25 23:55, Sean Anderson wrote:
>> Add a reset to help recover from cancelled operations.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>>   Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> index 04d4d3b4916d..901e15fcce2d 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> @@ -36,12 +36,16 @@ properties:
>>     power-domains:
>>       maxItems: 1
>>   +  resets:
>> +    maxItems: 1
>> +
>>   required:
>>     - compatible
>>     - reg
>>     - interrupts
>>     - clock-names
>>     - clocks
>> +  - resets
> 
> In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property.

It's optional for the driver for backwards compatibility. But for the
devicetree we make it mandatory since it should be included in all new
devicetrees.

--Sean

Re: [PATCH 4/5] spi: zynqmp-gqspi: Allow interrupting operations

[Sean Anderson]

On 1/17/25 03:41, Miquel Raynal wrote:
> Hello Sean,
> 
> On 16/01/2025 at 17:55:20 -05, Sean Anderson <sean.anderson@linux.dev> wrote:
> 
>> Some operations (such as reading several megabytes of data from a flash)
>> can take several seconds or more. Users may want to cancel such
>> operations. Allow them to do so now that we have a way to recover.
> 
> I fully agree with the observation, I tried myself interrupting too long
> transfers with another spi controller:
> 
> e0205d6203c2c ("spi: atmel: Prevent false timeouts on long transfers")
> 
> But there were issues reported, so we limited the signals to SIGKILLs:
> 
> 1ca2761a77349 ("spi: atmel: Do not cancel a transfer upon any signal")
> 
> But jffs2 plays with sigkills, so for spi memories it does not work
> well, we had to revert:
> 
> 890188d2d7e4a ("spi: atmel: Prevent spi transfers from being killed")
> 
> Same thing was also observed on Zynq7000:
> 
> 26cfc0dbe43aa ("spi: spi-zynq-qspi: use wait_for_completion_timeout to make zynq_qspi_exec_mem_op not interruptible")
> 
> I would however hint to use a specific helper for deriving your timeouts
> if you play with spi memories, because it is interesting to adapt the
> values nevertheless:
> 
> d8e4ebf870187 ("spi: Create a helper to derive adaptive timeouts")

Hm, ok. I wasn't sure whether this was allowed, but I saw a lot of
interruptable users under drivers/spi.

I guess I'll drop this patch for v2.

--Sean

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Sean Anderson]

On 1/17/25 08:21, Mark Brown wrote:
> On Thu, Jan 16, 2025 at 05:55:16PM -0500, Sean Anderson wrote:
>> This series adds support for resetting the QSPI controller if we have a
>> timeout. I find this greatly improves the stability of the device, which
>> would tend to break after any timeout.
> 
> If you're hitting a timeout that tends to indicate there's already a
> serious stability problem...

This was mostly hit when I was hacking on the driver. But of course
there are probably still bugs lurking in this driver, so I think it is
good to handle the exceptional conditions in a more-robust way.

--Sean

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

On Fri, Jan 17, 2025 at 11:17:35AM -0500, Sean Anderson wrote:
> On 1/17/25 08:21, Mark Brown wrote:

> > If you're hitting a timeout that tends to indicate there's already a
> > serious stability problem...

> This was mostly hit when I was hacking on the driver. But of course
> there are probably still bugs lurking in this driver, so I think it is
> good to handle the exceptional conditions in a more-robust way.

I'm not saying it's a bad idea, but your changelog is written in a way
that makes it sound like timeouts are normal.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Sean Anderson]

On 1/17/25 11:48, Mark Brown wrote:
> On Fri, Jan 17, 2025 at 11:17:35AM -0500, Sean Anderson wrote:
>> On 1/17/25 08:21, Mark Brown wrote:
> 
>> > If you're hitting a timeout that tends to indicate there's already a
>> > serious stability problem...
> 
>> This was mostly hit when I was hacking on the driver. But of course
>> there are probably still bugs lurking in this driver, so I think it is
>> good to handle the exceptional conditions in a more-robust way.
> 
> I'm not saying it's a bad idea, but your changelog is written in a way
> that makes it sound like timeouts are normal.

I've updated my cover letter for v2 to include the above blurb. Hopefully
that clears things up.

--Sean

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Miquel Raynal]

On 17/01/2025 at 13:21:58 GMT, Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jan 16, 2025 at 05:55:16PM -0500, Sean Anderson wrote:
>> This series adds support for resetting the QSPI controller if we have a
>> timeout. I find this greatly improves the stability of the device, which
>> would tend to break after any timeout.
>
> If you're hitting a timeout that tends to indicate there's already a
> serious stability problem...

Yes, unless the timeout is reached for "good reasons", ie. you request
substantial amounts of data (typically from a memory device) and the
timeout is too short compared to the theoretical time spent in the
transfer. A loaded machine can also increase the number of false
positives I guess.

Cheers,
Miquèl

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On Fri, Jan 17, 2025 at 07:31:08PM +0100, Miquel Raynal wrote:
> On 17/01/2025 at 13:21:58 GMT, Mark Brown <broonie@kernel.org> wrote:

> > If you're hitting a timeout that tends to indicate there's already a
> > serious stability problem...

> Yes, unless the timeout is reached for "good reasons", ie. you request
> substantial amounts of data (typically from a memory device) and the
> timeout is too short compared to the theoretical time spent in the
> transfer. A loaded machine can also increase the number of false
> positives I guess.

I'd argue that all of those are bad reasons, I'd only expect us to time
out when there's a bug - choosing too low a timeout or doing things in a
way that generates timeouts under load is a problem.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Sean Anderson]

On 1/17/25 13:41, Mark Brown wrote:
> On Fri, Jan 17, 2025 at 07:31:08PM +0100, Miquel Raynal wrote:
>> On 17/01/2025 at 13:21:58 GMT, Mark Brown <broonie@kernel.org> wrote:
> 
>> > If you're hitting a timeout that tends to indicate there's already a
>> > serious stability problem...
> 
>> Yes, unless the timeout is reached for "good reasons", ie. you request
>> substantial amounts of data (typically from a memory device) and the
>> timeout is too short compared to the theoretical time spent in the
>> transfer. A loaded machine can also increase the number of false
>> positives I guess.
> 
> I'd argue that all of those are bad reasons, I'd only expect us to time
> out when there's a bug - choosing too low a timeout or doing things in a
> way that generates timeouts under load is a problem.

There's no transmit DMA for this device. So if you are under high load
and make a long transfer, it's possible to time out. I don't know if
it's possible to fix that very easily. The timeout calculation assumes
that data is being transferred at the SPI bus rate.

That said, in the common case (NOR flash) writes don't work like that.
To write a flash, we make a short transfer (such as an eraseblock) and
then poll the status register before making another transfer. With short
transfers there is less probability of timing out because the extra time
makes up more of the duration.

--Sean

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

On Fri, Jan 17, 2025 at 04:46:23PM -0500, Sean Anderson wrote:
> On 1/17/25 13:41, Mark Brown wrote:
> > On Fri, Jan 17, 2025 at 07:31:08PM +0100, Miquel Raynal wrote:

> >> Yes, unless the timeout is reached for "good reasons", ie. you request
> >> substantial amounts of data (typically from a memory device) and the
> >> timeout is too short compared to the theoretical time spent in the
> >> transfer. A loaded machine can also increase the number of false
> >> positives I guess.

> > I'd argue that all of those are bad reasons, I'd only expect us to time
> > out when there's a bug - choosing too low a timeout or doing things in a
> > way that generates timeouts under load is a problem.

> There's no transmit DMA for this device. So if you are under high load
> and make a long transfer, it's possible to time out. I don't know if
> it's possible to fix that very easily. The timeout calculation assumes
> that data is being transferred at the SPI bus rate.

In that case I wouldn't expect the timeout to apply to the whole
operation, or I'd expect a timeout applied waiting for something
interrupt driven to not to be fired unless we stop making forward
progress.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] spi: zynqmp-gqspi: Improve error recovery by resetting

[Sean Anderson]

On 1/20/25 08:49, Mark Brown wrote:
> On Fri, Jan 17, 2025 at 04:46:23PM -0500, Sean Anderson wrote:
>> On 1/17/25 13:41, Mark Brown wrote:
>> > On Fri, Jan 17, 2025 at 07:31:08PM +0100, Miquel Raynal wrote:
> 
>> >> Yes, unless the timeout is reached for "good reasons", ie. you request
>> >> substantial amounts of data (typically from a memory device) and the
>> >> timeout is too short compared to the theoretical time spent in the
>> >> transfer. A loaded machine can also increase the number of false
>> >> positives I guess.
> 
>> > I'd argue that all of those are bad reasons, I'd only expect us to time
>> > out when there's a bug - choosing too low a timeout or doing things in a
>> > way that generates timeouts under load is a problem.
> 
>> There's no transmit DMA for this device. So if you are under high load
>> and make a long transfer, it's possible to time out. I don't know if
>> it's possible to fix that very easily. The timeout calculation assumes
>> that data is being transferred at the SPI bus rate.
> 
> In that case I wouldn't expect the timeout to apply to the whole
> operation, or I'd expect a timeout applied waiting for something
> interrupt driven to not to be fired unless we stop making forward
> progress.  

I don't know if there are any helpers we can use for this. To implement
this we'd need something like schedule_timeout() but where the interrupt
handler calls mod_timer() whenever it does work.

--Sean

Re: [PATCH 1/5] dt-bindings: spi: zynqmp-qspi: Add reset

[Rob Herring]

On Fri, Jan 17, 2025 at 11:12:15AM -0500, Sean Anderson wrote:
> On 1/17/25 02:14, Michal Simek wrote:
> > 
> > 
> > On 1/16/25 23:55, Sean Anderson wrote:
> >> Add a reset to help recover from cancelled operations.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> ---
> >>
> >>   Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> >> index 04d4d3b4916d..901e15fcce2d 100644
> >> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> >> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> >> @@ -36,12 +36,16 @@ properties:
> >>     power-domains:
> >>       maxItems: 1
> >>   +  resets:
> >> +    maxItems: 1
> >> +
> >>   required:
> >>     - compatible
> >>     - reg
> >>     - interrupts
> >>     - clock-names
> >>     - clocks
> >> +  - resets
> > 
> > In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property.
> 
> It's optional for the driver for backwards compatibility. But for the
> devicetree we make it mandatory since it should be included in all new
> devicetrees.

Generally, we discourage new required properties as that's an ABI 
change. The exception is really when optional was a mistake. That's 
arguably the case here if the h/w always has a reset.

Unfortunately, there's not a way to distinguish 'required' from 
'required for new users'.

Rob

Re: [PATCH 1/5] dt-bindings: spi: zynqmp-qspi: Add reset

[Sean Anderson]

On 1/23/25 17:45, Rob Herring wrote:
> On Fri, Jan 17, 2025 at 11:12:15AM -0500, Sean Anderson wrote:
>> On 1/17/25 02:14, Michal Simek wrote:
>> > 
>> > 
>> > On 1/16/25 23:55, Sean Anderson wrote:
>> >> Add a reset to help recover from cancelled operations.
>> >>
>> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> >> ---
>> >>
>> >>   Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++
>> >>   1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> >> index 04d4d3b4916d..901e15fcce2d 100644
>> >> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> >> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> >> @@ -36,12 +36,16 @@ properties:
>> >>     power-domains:
>> >>       maxItems: 1
>> >>   +  resets:
>> >> +    maxItems: 1
>> >> +
>> >>   required:
>> >>     - compatible
>> >>     - reg
>> >>     - interrupts
>> >>     - clock-names
>> >>     - clocks
>> >> +  - resets
>> > 
>> > In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property.
>> 
>> It's optional for the driver for backwards compatibility. But for the
>> devicetree we make it mandatory since it should be included in all new
>> devicetrees.
> 
> Generally, we discourage new required properties as that's an ABI 
> change. The exception is really when optional was a mistake. That's 
> arguably the case here if the h/w always has a reset.

This device has a reset on ZynqMP and Versal.

The driver still considers this property optional, so it's not an ABI break.
But I made it required in the schema to help out the folks at AMD when they
get around to upstreaming the Versal devicetree :)

> Unfortunately, there's not a way to distinguish 'required' from 
> 'required for new users'.

I will add a note to the commit message about this situation.

--Sean

Re: [PATCH 1/5] dt-bindings: spi: zynqmp-qspi: Add reset

[Michal Simek]

On 1/23/25 23:57, Sean Anderson wrote:
> On 1/23/25 17:45, Rob Herring wrote:
>> On Fri, Jan 17, 2025 at 11:12:15AM -0500, Sean Anderson wrote:
>>> On 1/17/25 02:14, Michal Simek wrote:
>>>>
>>>>
>>>> On 1/16/25 23:55, Sean Anderson wrote:
>>>>> Add a reset to help recover from cancelled operations.
>>>>>
>>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>>>> ---
>>>>>
>>>>>    Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++
>>>>>    1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>>>>> index 04d4d3b4916d..901e15fcce2d 100644
>>>>> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>>>>> @@ -36,12 +36,16 @@ properties:
>>>>>      power-domains:
>>>>>        maxItems: 1
>>>>>    +  resets:
>>>>> +    maxItems: 1
>>>>> +
>>>>>    required:
>>>>>      - compatible
>>>>>      - reg
>>>>>      - interrupts
>>>>>      - clock-names
>>>>>      - clocks
>>>>> +  - resets
>>>>
>>>> In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property.
>>>
>>> It's optional for the driver for backwards compatibility. But for the
>>> devicetree we make it mandatory since it should be included in all new
>>> devicetrees.
>>
>> Generally, we discourage new required properties as that's an ABI
>> change. The exception is really when optional was a mistake. That's
>> arguably the case here if the h/w always has a reset.
> 
> This device has a reset on ZynqMP and Versal.
> 
> The driver still considers this property optional, so it's not an ABI break.
> But I made it required in the schema to help out the folks at AMD when they
> get around to upstreaming the Versal devicetree :)

Pretty much every IP block has hardware reset wired. It is just user decision if 
that reset is going to be handled or if low level firmware allows to handle it.

The same logic applies to clocks too.

>> Unfortunately, there's not a way to distinguish 'required' from
>> 'required for new users'.
> 
> I will add a note to the commit message about this situation.

Again even new users don't need to have an access to this feature.
Not sure what's the right way to go here but in our arm64 based chip reset is 
provided via firmware interface and I have no issue to say we are expecting 
reset property to be present but it is up to firmware implementation if a reset 
is actually happening on HW level.

Thanks,
Michal

Re: [PATCH 1/5] dt-bindings: spi: zynqmp-qspi: Add reset

[Rob Herring]

On Thu, Jan 23, 2025 at 05:57:41PM -0500, Sean Anderson wrote:
> On 1/23/25 17:45, Rob Herring wrote:
> > On Fri, Jan 17, 2025 at 11:12:15AM -0500, Sean Anderson wrote:
> >> On 1/17/25 02:14, Michal Simek wrote:
> >> > 
> >> > 
> >> > On 1/16/25 23:55, Sean Anderson wrote:
> >> >> Add a reset to help recover from cancelled operations.
> >> >>
> >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> >> ---
> >> >>
> >> >>   Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++
> >> >>   1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> >> >> index 04d4d3b4916d..901e15fcce2d 100644
> >> >> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> >> >> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
> >> >> @@ -36,12 +36,16 @@ properties:
> >> >>     power-domains:
> >> >>       maxItems: 1
> >> >>   +  resets:
> >> >> +    maxItems: 1
> >> >> +
> >> >>   required:
> >> >>     - compatible
> >> >>     - reg
> >> >>     - interrupts
> >> >>     - clock-names
> >> >>     - clocks
> >> >> +  - resets
> >> > 
> >> > In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property.
> >> 
> >> It's optional for the driver for backwards compatibility. But for the
> >> devicetree we make it mandatory since it should be included in all new
> >> devicetrees.
> > 
> > Generally, we discourage new required properties as that's an ABI 
> > change. The exception is really when optional was a mistake. That's 
> > arguably the case here if the h/w always has a reset.
> 
> This device has a reset on ZynqMP and Versal.
> 
> The driver still considers this property optional, so it's not an ABI break.
> But I made it required in the schema to help out the folks at AMD when they
> get around to upstreaming the Versal devicetree :)

Not 'the driver', but 'a driver'. You can't say what *all* drivers do. 
If I write a new driver and read the schema, then I can say 'resets is 
required so I'll make it required in my new driver'. But then my new 
driver doesn't work with an older DT that didn't have resets which was 
valid at the time.

Rob

Re: [PATCH 1/5] dt-bindings: spi: zynqmp-qspi: Add reset

[Sean Anderson]

On 1/27/25 12:57, Rob Herring wrote:
> On Thu, Jan 23, 2025 at 05:57:41PM -0500, Sean Anderson wrote:
>> On 1/23/25 17:45, Rob Herring wrote:
>> > On Fri, Jan 17, 2025 at 11:12:15AM -0500, Sean Anderson wrote:
>> >> On 1/17/25 02:14, Michal Simek wrote:
>> >> > 
>> >> > 
>> >> > On 1/16/25 23:55, Sean Anderson wrote:
>> >> >> Add a reset to help recover from cancelled operations.
>> >> >>
>> >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> >> >> ---
>> >> >>
>> >> >>   Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml | 6 ++++++
>> >> >>   1 file changed, 6 insertions(+)
>> >> >>
>> >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> >> >> index 04d4d3b4916d..901e15fcce2d 100644
>> >> >> --- a/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> >> >> +++ b/Documentation/devicetree/bindings/spi/spi-zynqmp-qspi.yaml
>> >> >> @@ -36,12 +36,16 @@ properties:
>> >> >>     power-domains:
>> >> >>       maxItems: 1
>> >> >>   +  resets:
>> >> >> +    maxItems: 1
>> >> >> +
>> >> >>   required:
>> >> >>     - compatible
>> >> >>     - reg
>> >> >>     - interrupts
>> >> >>     - clock-names
>> >> >>     - clocks
>> >> >> +  - resets
>> >> > 
>> >> > In 2/5 you are calling devm_reset_control_get_optional_exclusive() that's why I expect reset is not really required property.
>> >> 
>> >> It's optional for the driver for backwards compatibility. But for the
>> >> devicetree we make it mandatory since it should be included in all new
>> >> devicetrees.
>> > 
>> > Generally, we discourage new required properties as that's an ABI 
>> > change. The exception is really when optional was a mistake. That's 
>> > arguably the case here if the h/w always has a reset.
>> 
>> This device has a reset on ZynqMP and Versal.
>> 
>> The driver still considers this property optional, so it's not an ABI break.
>> But I made it required in the schema to help out the folks at AMD when they
>> get around to upstreaming the Versal devicetree :)
> 
> Not 'the driver', but 'a driver'. You can't say what *all* drivers do. 
> If I write a new driver and read the schema, then I can say 'resets is 
> required so I'll make it required in my new driver'. But then my new 
> driver doesn't work with an older DT that didn't have resets which was 
> valid at the time.

OK, I'll add a description to this effect. The humans can read that, and
the machines won't care.

--Sean