Fix incorrect QSPI setup #2231

bugadani · 2024-09-25T15:45:14Z

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

I have updated existing examples or added new ones (if applicable).
I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
My changes were added to the CHANGELOG.md in the proper section.
I have added necessary changes to user code to the Migration Guide.
My changes are in accordance to the esp-rs API guidelines

Extra:

I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Two-parter:

QSPI setup was unfixed in Enable QSPI tests on ESP32 and clean up #2198.
We didn't support multi-line address without data. Similar issue in esp-idf: SPI: QSPI failed (IDFGH-3763) espressif/esp-idf#5684

This fixes #2229 and adds test code to make sure we don't break QSPI again. This PR also works around an ESP32 limitation.

Testing

Observed behaviour change using a logic analyzer and also the changed test cases fail before the fix.

bjoernQ

This actually fixes #2229 - weird given the result of the bisect and also manually checking that commit and the one before - Thanks!

bugadani · 2024-09-25T16:05:16Z

Awesome. The problem is, that the RX/TX swap introduced a buffer size issue and maybe your flash chip flashed the last 256 ! characters that it received. So one problem was fixed by restoring the buffer sizes in #2139 but then I managed to break this again, later, which your bisect might have jumped over.

bugadani · 2024-09-25T18:40:26Z

I've been banging my head against ESP32 for a while now, and I think that chip simply can't do a 4-line address phase if it doesn't have data to transmit afterwards. The chip defaults back to 1-line address phase if usr_miso and usr_mosi are clear. I guess we could work around this by sending address as data if there is no data...

Dominaezzz · 2024-09-25T22:06:19Z

Just return an error if it's not supported I think.

bugadani · 2024-09-25T22:11:50Z

Why? It's supported everywhere else, users expect it to work and it's simple to work around.

Dominaezzz · 2024-09-25T22:14:37Z

It's not like all the SPI hardware on all chips works the same. Some have more features that others. It just so happens the ESP32 can't support this one case.

If users want it to "just work" everywhere, they should just use the data phase for everything themselves. Doing it secretly is too hacky imo.

Dominaezzz · 2024-09-25T22:15:43Z

Also, not all implementations of DmaTxBuffer will support the methods you just added.

bugadani · 2024-09-25T22:17:09Z

It's not like all the SPI hardware on all chips works the same.

One of the jobs of the HAL is to equalize support as much as possible.

Also, not all implementations of DmaTxBuffer will support the methods you just added.

Then they will be free to return an error.

Dominaezzz · 2024-09-25T22:22:56Z

Also, not all implementations of DmaTxBuffer will support the methods you just added.

Then they will be free to return an error.

How is that different from just returning an error upfront? User will still end up having to deal with it.

Also users won't be expecting their DmaTxBufs to suddenly contain data they never added. If they put it in a loop, the first iteration would do the right thing, then the second iteration would send the address (in the address phase) + the previous address (in the data phase).

It's not like all the SPI hardware on all chips works the same.

One of the jobs of the HAL is to equalize support as much as possible.

Yeah but there should be a line somewhere I think.

Dominaezzz · 2024-09-25T22:46:37Z

There's the option of not using DMA and just using the CPU Buffer for the data phase. (I don't really like this solution either to be honest, but it's better than tampering with the DMA buffer).

bugadani · 2024-09-26T07:19:32Z

Sorry @bjoernQ I managed to completely hijack this PR, do you want me to send in the original fix separately?

bjoernQ · 2024-09-26T07:22:12Z

Sorry @bjoernQ I managed to completely hijack this PR, do you want me to send in the original fix separately?

No problem - thanks for taking care of this

esp-hal/build.rs

MabezDev

LGTM, thanks!

bugadani · 2024-09-26T12:31:14Z

esp-hal/src/spi/master.rs

+                // on a single line, regardless of its data mode.
+                if bytes_to_write == 0 && address.mode() != SpiDataMode::Single {
+                    let bytes_to_write = address.width().div_ceil(8);
+                    let addr_bytes = address.value().to_le_bytes();


Don't be hasty, I need to verify if I got the endianness right here.

iirc the bit order of the data phase can changed so you'll probably have to read that register bit to decide the endianness here.

SPI_WR_BIT_ORDER affects all (CMD, ADDR and DATA) phases so I think I'm fine here.

That being said, I wanted to look at this with a logic analyzer because this workaround exists due to the hardware not doing what it's supposed to.

Good thing I checked. The bit works as expected, but the address is encoded as big-endian so that had to be aligned.

Demo that we now generate the same waveform with or without data (data is set to 0 to reduce noise). 2 lines only, because my ESP32 devkit doesn't have enough pins 😅

Nice!

I just remembered, will this work as the user expects if they specify a dummy phase?

No, the dummy phase gets moved to before the address which is wrong. But that'll need to be addressed later as there are a few different approaches to deal with that (error, ignore, emulate). This current PR is done the way it is.

Opened #2240 so we don't forget about it.

Dominaezzz · 2024-09-26T12:54:22Z

esp-hal/build.rs

+                "Places the SPI driver in RAM for better performance",
+            ),
+            (
+                "spi-address-workaround",


Do you reckon libraries may want to set this?

There should be one source of truth for the config, and it should lie in the hands of the user. In this case, we've enabled it by default, which is a sane thing to do, and users can opt-out. One thing libraries can do is detect if the setting is enabled and either warn/panic if it should be - but even this I would discourage unless the library functionality depended on it.

JurajSadel

Thanks!

bugadani · 2024-09-26T14:31:45Z

Hold on there's still something not quite right here :)

bugadani added the skip-changelog No changelog modification needed label Sep 25, 2024

bjoernQ linked an issue Sep 25, 2024 that may be closed by this pull request

qspi_flash example broken (at least on ESP32-S2 - most probably for all chips) since b5f0246 #2229

Closed

bjoernQ reviewed Sep 25, 2024

View reviewed changes

bugadani force-pushed the qspi branch from 795972c to fc2624a Compare September 25, 2024 22:02

bugadani removed the skip-changelog No changelog modification needed label Sep 25, 2024

MabezDev requested changes Sep 26, 2024

View reviewed changes

esp-hal/build.rs Outdated Show resolved Hide resolved

MabezDev approved these changes Sep 26, 2024

View reviewed changes

bugadani commented Sep 26, 2024

View reviewed changes

Dominaezzz mentioned this pull request Sep 26, 2024

Migrate I8080 driver to a move based API #2191

Merged

6 tasks

Dominaezzz reviewed Sep 26, 2024

View reviewed changes

JurajSadel approved these changes Sep 26, 2024

View reviewed changes

bugadani added 7 commits September 26, 2024 17:14

Fix QSPI mode on non-ESP32

f03480e

Update tests to count pulses on pins separately

c34d682

Fix ESP32 addressing phase issue

7ceee33

Use defmt's assert

2c13029

Set fastrd bit

c80051d

Apply pulldowns to define signal level when not driven

9718a0d

Transfer address bits in data phase on ESP32

3e7fe9b

bugadani added 7 commits September 26, 2024 17:14

Changelog

eb11a22

Use a separate buffer for the address, make the workaround configurable

2a71c1f

Remove now-unnecessary additions

91e86f6

Force SpiDma to remain Send

a3906d2

Clarify wording, remove prefix

afc9a69

Clean up manual register manipulation

274471f

Fix byte order

4a6a3b8

bugadani force-pushed the qspi branch from 2c39c66 to 4a6a3b8 Compare September 26, 2024 15:14

bugadani enabled auto-merge September 26, 2024 15:15

bugadani mentioned this pull request Sep 26, 2024

ESP32 SPI: Dummy phase is not handled correctly if there is no data phase #2240

Open

bugadani added this pull request to the merge queue Sep 26, 2024

Merged via the queue into esp-rs:main with commit c481c49 Sep 26, 2024
28 checks passed

bugadani deleted the qspi branch September 26, 2024 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect QSPI setup #2231

Fix incorrect QSPI setup #2231

bugadani commented Sep 25, 2024 •

edited

Loading

bjoernQ left a comment

bugadani commented Sep 25, 2024

bugadani commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

bugadani commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

bugadani commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

bugadani commented Sep 26, 2024

bjoernQ commented Sep 26, 2024

MabezDev left a comment

bugadani Sep 26, 2024

Dominaezzz Sep 26, 2024

bugadani Sep 26, 2024

bugadani Sep 26, 2024

bugadani Sep 26, 2024 •

edited

Loading

Dominaezzz Sep 26, 2024

bugadani Sep 26, 2024 •

edited

Loading

Dominaezzz Sep 26, 2024

MabezDev Sep 26, 2024

JurajSadel left a comment

bugadani commented Sep 26, 2024

Fix incorrect QSPI setup #2231

Fix incorrect QSPI setup #2231

Conversation

bugadani commented Sep 25, 2024 • edited Loading

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

bjoernQ left a comment

Choose a reason for hiding this comment

bugadani commented Sep 25, 2024

bugadani commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

bugadani commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

bugadani commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

Dominaezzz commented Sep 25, 2024

bugadani commented Sep 26, 2024

bjoernQ commented Sep 26, 2024

MabezDev left a comment

Choose a reason for hiding this comment

bugadani Sep 26, 2024

Choose a reason for hiding this comment

Dominaezzz Sep 26, 2024

Choose a reason for hiding this comment

bugadani Sep 26, 2024

Choose a reason for hiding this comment

bugadani Sep 26, 2024

Choose a reason for hiding this comment

bugadani Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Dominaezzz Sep 26, 2024

Choose a reason for hiding this comment

bugadani Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Dominaezzz Sep 26, 2024

Choose a reason for hiding this comment

MabezDev Sep 26, 2024

Choose a reason for hiding this comment

JurajSadel left a comment

Choose a reason for hiding this comment

bugadani commented Sep 26, 2024

bugadani commented Sep 25, 2024 •

edited

Loading

bugadani Sep 26, 2024 •

edited

Loading

bugadani Sep 26, 2024 •

edited

Loading