[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

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

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