Skip to content
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

arch/arm64/src/imx9/imx9_lpspi.c: Fix 9-16 bit transfers and dcache i… #304

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

jlaitine
Copy link

…nvalidate

Summary

Impact

Testing

@jlaitine jlaitine requested a review from pussuw October 22, 2024 20:08
@@ -1464,7 +1472,7 @@ static void imx9_lpspi_exchange(struct spi_dev_s *dev,
/* Flush the RX data to ram */

up_invalidate_dcache((uintptr_t)priv->rxbuf,
(uintptr_t)priv->rxbuf + nbytes);
(uintptr_t)priv->rxbuf + DCACHE_ALIGN_UP(nbytes));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handling the cache line alignment should not be the responsibility of the SPI driver, the cache operation must handle this. Does this not work as expected ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalidating a non-aligned area does clean and invalidate, which would be wrong.

But, this memory area is only used as rx buffer so it is never dirty. So you are right, aligning is not necessary in this case. In general it can't be said that aligning wouldn't be the responsibility of the driver... I was a bit hasty here while trying to get things working :)

I'll remove the unnecessary change. Btw. to optimize, we could move the initial invalidate to be only after the buffer allocation, it shouldn't be needed on every transfer?

Copy link

@pussuw pussuw Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buffer start and end must be aligned, this is guaranteed by the DMA safe buffer allocator. I don't know why there are two invalidate operations per transfer, 1 is enough.

The original driver (copy&pasted) did not use DMA safe buffers, so its functionality was unpredictable, maybe some kind of remnant from that.

I think we should remove the other invalidate at line 1393 completely.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove the other invalidate at line 1393 completely.

I addressed this with another patch. Initial invalidation should be done after allocation, to make sure that cache lines are not dirty before first transfers.

Copy link

@pussuw pussuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

There is no need to invalidate the RX buffer before every transfer.
It is never gets dirty, so it is good to invalidate initially after allocation,
and after each transfer.

Signed-off-by: Jukka Laitinen <[email protected]>
@jlaitine jlaitine merged commit adb69b4 into master Oct 23, 2024
10 of 11 checks passed
@jlaitine jlaitine deleted the fix_imx9_lpspi branch October 23, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants