-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drivers: spi: stm32h7: Use SPI FIFO #63173
drivers: spi: stm32h7: Use SPI FIFO #63173
Conversation
99aecd1
to
344be08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fifo handling needs more thoughts I think
static void spi_stm32_send_next_frame(SPI_TypeDef *spi, | ||
struct spi_stm32_data *data) | ||
{ | ||
const uint8_t frame_size = SPI_WORD_SIZE_GET(data->ctx.config->operation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps it would be better to store the frame size in struct spi_stm32_data directly, so getting it would be faster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cleaner and easier to retrieve. However, the same information would be replicated in two places, with the inconsistency problems that could carry. I don't have a strong opinion on this so I'm happy to add a new attribute to the struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More pointer dereference might increase the risk of cache miss. And since in this case you are only reaching for a small info, so better storing it in data right away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have taken a look at this. There are other places in which ctx.config
is accessed; to be consistent, all these references should be modified. This is out of this PR's scope as I conceived it.
drivers/spi/spi_ll_stm32.c
Outdated
LL_SPI_TransmitData16(spi, tx_frame); | ||
spi_context_update_tx(&data->ctx, 2, 1); | ||
break; | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need that. Once you configured your controller, frame size will not change.
Actually, you could just have an if/else since you only handle 2 frame size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need that. Once you configured your controller, frame size will not change.
You mean the switch
statement? I replaced the if ... else ...
by switch
because it leaves the code in a better position to support more frame sizes in the future (H7 has quite a lot of them). However, I don't mind reverting this back to if ... else ...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frame size can be of below a byte or above, but less than 2 bytes etc... (I.E.: 4, 6, ... 10, 12...). But in the end you handle everything on byte-basis. So even if it's below a byte: you feed one byte (the controller will only take the relevant n bits representing the frame). Above a byte but below 2bytes: you feed 2 bytes. etc...
Stm32's HAL does not seem to provide anything but LL_SPI_TransmitData<8/16/32>.
if you enable sizes above 16bits, then perhaps keep the switch. if/else if/else is less clean than a switch in such case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I will revert to if else
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
drivers/spi/spi_ll_stm32.c
Outdated
uint8_t packet_size = SPI_PACKET_SIZE_GET(data->ctx.config->operation); | ||
|
||
for (int i = 0; i < packet_size; i++) { | ||
spi_stm32_send_next_frame(spi, data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how stm32's SPI controllers work, generally fifo interrupt threshold is meant to make sure that the controller is not going to starve of frames to send (if there are way more to be sent, that is). Which means that between the time you get the interrupt and handle it, the controller may have had time to continue sending frames left in the fifo, which in return, means that it went over the threshold so you'll need to feed more frames than the configured threshold. It can also be true in receiving mode in a RX only transaction type where the controller can send the dummy bytes by itself (I don't kmow if that applies to this controller though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how stm32's SPI controllers work, generally fifo interrupt threshold is meant to make sure that the controller is not going to starve of frames to send (if there are way more to be sent, that is).
That's my understanding as well, yes.
[...] you'll need to feed more frames than the configured threshold.
If I understood you correctly, you are suggesting replacing this:
if (ll_func_tx_is_not_full(spi)) {
spi_stm32_send_next_packet(spi, data);
}
by this:
while (ll_func_tx_is_not_full(spi)) {
spi_stm32_send_next_packet(spi, data);
}
If this is done, the interrupt handler takes more time to finish; this might be undesirable. However, as you mention, there are less chances of "starving" the FIFO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, make it a while loop. The little extra time spent doing so, is less of an issue for the system than handling more interrupts due to a threshold that could hit more often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a while loop here caused several problems produced by the fact that SPI supports transmissions in which tx is bigger than rx or vice-versa. The current implementation takes care of this, defaulting to the classic way of transmitting in case of rx > tx (which causes more problems that require further modifications).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this and found a clean way to support the rx > tx case, so this is no longer a problem.
Kind reminder to review this @tbursztyka. I agree with several of your comments, but others require a further discussion. |
@dgastonochoa @tbursztyka @teburd can you attend the dev meeting this week? |
@MaureenHelm I won't be able to attend to the dev meeting today, but I will the next week. Maybe you want to consider postponing the review of this PR until then if it's more convenient for you, @tbursztyka and @teburd. |
No problem, I'll push it out to next week. |
344be08
to
ef0413e
Compare
@teburd @tbursztyka could you please take a look? The current proposal avoids the mentioned conflictive points. |
3ed1e09
to
27f3188
Compare
Avoind calling startMasterTransfer multiple times in a transaction by moving it to the transceive() function. Signed-off-by: Daniel Gaston Ochoa <dgastonochoa@gmail.com>
In H7, TXP indicates when its FIFO has room for, at least, one packet. Thus, rename ll_func_tx_is_empty as ll_func_tx_is_not_full, to be consistent in all platforms. Signed-off-by: Daniel Gaston Ochoa <dgastonochoa@gmail.com>
Simplify and clarify spi_stm32_shift_m by splitting it in 3 smaller functions with clear names. Signed-off-by: Daniel Gaston Ochoa <dgastonochoa@gmail.com>
14663e3
to
b048871
Compare
Use H7 SPI FIFO to improve performance. Signed-off-by: Daniel Gaston Ochoa <dgastonochoa@gmail.com>
b048871
to
bffa0c6
Compare
All feedback has been applied, @teburd and @tbursztyka please could you take a look? |
@teburd, @tbursztyka please could you confirm whether or not you intent to review this at some point? |
@tbursztyka I am dismissing your stale review here as it appears all your comments have been addressed. If not, please re-review and re-request changes.
@dgastonochoa with However, with Any ideas what it can be? |
Hi @GeorgeCGV, I have run the spi_loopback tests again with interrupts enable in with I imagine you are seeing this error not in the loopback tests, but in some other scenario, right? Would it be possible to run the loopback tests on H730 to see if the problem is reproduced there? Does the problem reproduce without enabling SPI interrupts? When you say "SPI is kept busy", do you mean that |
With disabled interrupts it is not getting into a freezing state. But I am observing overruns and some devices fail.
Yes, it gets stuck in
Therefore, both return
Would need to do some HW modifications. |
This seems to indicate that the SPI driver has send/received all the expected bytes, as the Would it be possible to confirm the activity on the bus when the error occurs? Also, is it possible to double check that is |
it is H7, so we go and check the transfer size. The Different SPI peripherals fail at different times. To trigger the issue I have to power cycle the board multiple times. When it gets stuck it is always within What is also observed is the corruption of transmitted and/or received data. That doesn't happen every time... but often enough.
Nothing happens on the bus when it stucks: Maybe it is related somehow to having multiple slave devices... |
In that picture you sent there is a lot of cross-talk to CS, is it sure that that is caused bu the logic analyzer probes? Could you try to reduce the SPI SCK frequency? Could you give more details about your setup? (SCK frequency, is this being tested on a stand-alone nucleo board etc.)
This doesn't sound good either. |
@dgastonochoa it is a zoomed out screen. The clocks are validated and set to be within supported range. It is a custom device based on H730. Multiple SPIs are used where each has multiple targets (slaves). After reducing the clock the issue started to appear every time. With higher clock the issue required board restart to trigger it. Sized down connected zoo to one device per SPI (to eliminate possible issue(s) from having multiple devices). Zephyr version is pretty much up to date: f69641f7d204864aa26f8bdd9fecab259e535da2. SPI without FIFO, the device works: SPI with FIFO, the device doesn't work and it is stuck as described above (within It is clearly visible that the CS (chip select) is dramatically off..., It is software controlled by the driver. Most probably that is where the problems are coming from. |
@GeorgeCGV please raise an issue to follow this up and make it visible |
@dgastonochoa I opened an issue (#66486) that looks different from #66326, but I'm not sure. @erwango just closed it, but I'm kindly ask you to take a look to it and see if it might be the same. Thanks. |
This is a proposal to support different "packet sizes" in the SPI driver. In this context, a packet size bigger than one allows to write more than one frame to the SPI's TxFIFO (in case it has one) so that delays can be avoided:
To ilustrate the first point, here are some captures of a transmission with the stm32h7 SPI current driver:
As you can see above, there are delays between each frame. This can dramatically impair performance when sending large amounts of data.
If a bigger packet size is used for the same transmission, the delays disappear:
EDIT
After the received feedback, this PR will no longer use the FIFO threshold. It now implements the use of the FIFO but without using modifying the FIFO threshold (so it defaults to 1). This still fixes the performance problem described above.
Test plan
Connect an STM32H7 board to the PC with pins D11 and D12 connected.
Open a minicom or equivalent terminal to read the test report.
Execute the following commands:
And verify all tests pass.
EDIT
The tests above are no longer necessary as the use of the FIFO threshold has been discarded. Thus, run the already existing spi loopback tests and verify they pass:
EDIT 2
This PR has also been tested with 2 separated nucleo boards (H743), one acting as SPI master and the other as SPI slave.