Skip to content

stm32h7 spi driver uses SRAM2 buffers in NOCACHE region #61021

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

Closed
wants to merge 2 commits into from

Conversation

FRASTM
Copy link
Collaborator

@FRASTM FRASTM commented Aug 1, 2023

The stm32H7 serie should allocate spi buffers in the SRAM2 memory area with NOCACHE attribute.
This PR is setting this SRAM2 when required by the testcase as this is specific to the application (tests/drivers/sp/spi_loopback)

In the spi_ll_stm32.c driver, when testing the buffer coherency of the spi_buf_set, the pointer but also each elements of the structure have to be checked:

struct spi_buf_set {
	const struct spi_buf *buffers;
	size_t count;
};

This is evaluated during the testcase Start null tx of the tests/drivers/spi/spi_loopback

Fixes #60977

Checks if the spi_buf_set structure can has null elements in case
of stm32H7x devices when NOCACHE ram is used for storing buffers.

Signed-off-by: Francois Ramu <[email protected]>
@FRASTM FRASTM requested a review from erwango August 1, 2023 07:37
@FRASTM FRASTM marked this pull request as ready for review August 1, 2023 07:48
@FRASTM FRASTM requested a review from tbursztyka as a code owner August 1, 2023 07:48
@FRASTM
Copy link
Collaborator Author

FRASTM commented Aug 1, 2023

$ west build -p auto -b nucleo_h743zi tests/drivers/spi/spi_loopback/

Running TESTSUITE spi_loopback                                                                        
===================================================================                                   
START - test_spi_loopback                                                                             
I: SPI test on buffers TX/RX 0x300200a0/0x30020080                                                    
I: SPI test slow config                                                                               
I: Start complete multiple                                                                            
I: Passed                                                                                             
I: Start complete loop                                                                                
I: Passed                                                                                             
I: Start null tx                                                                                      
I: Passed                                                                                             
I: Start half start                                                                                   
I: Passed                                                                                             
I: Skip half end                                                                                      
I: Skip every 4                                                                                       
I: SPI test fast config                                                                               
I: Start complete multiple                                                                            
I: Passed                                                                                             
I: Start complete loop                                                                                
I: Passed                                                                                             
I: Start null tx                                                                                      
I: Passed                                                                                             
I: Start half start                                                                                   
I: Passed                                                                                             
I: Skip half end                                                                                      
I: Skip every 4                                                                                       
I: Start complete loop                                                                                
I: Passed                                                                                             
I: Start complete loop                                                                                
I: Passed                                                                                             
I: All tx/rx passed                                                                                   
 PASS - test_spi_loopback in 0.049 seconds                                                            
===================================================================                                   
TESTSUITE spi_loopback succeeded                                                                      
                                                                                                      
------ TESTSUITE SUMMARY START ------                                                                 
                                                                                                      
SUITE PASS - 100.00% [spi_loopback]: pass = 1, fail = 0, skip = 0, total = 1 duration = 0.049 seconds 
 - PASS - [spi_loopback.test_spi_loopback] duration = 0.049 seconds                                   
                                                                                                      
------ TESTSUITE SUMMARY END ------     

@dgastonochoa
Copy link
Contributor

dgastonochoa commented Aug 2, 2023

Hi, I would like to remark this would still leave us with the problem of nocache regions created by using the CONFIG_NOCACHE_MEMORY + __attribute__((__section__(".nocache"))); mechanism (see #60977 (comment)). spi_buf_set_in_nocache should be modified to check the following symbols:

_nocache_ram_size
_nocache_ram_end
_nocache_ram_start

in case CONFIG_NOCACHE_MEMORY is enabled. This could be done in this PR or in another one; I'm happy to take care of it if you wish.

I believe #52965 has the same problem in address_in_non_cacheable_sram

Configure the SRAM2 of the stm32h7x serie with NOCACHE attribute
to run the tests/drivers/spi/spi_loopback
Connect D11 to D12 to PASS the test

Signed-off-by: Francois Ramu <[email protected]>
@FRASTM
Copy link
Collaborator Author

FRASTM commented Aug 3, 2023

This could be done in this PR or in another one; I'm happy to take care of it if you wish.

Yes, please, because I am really expert of all this stuff.

I believe #52965 has the same problem in address_in_non_cacheable_sram

There are probably many issues/questions about cache and MPU regions around the stm32H7.
(PR #60765, #60977, the pb I see in qspi mode with PR #61122)

@dgastonochoa
Copy link
Contributor

Yes, please, because I am really expert of all this stuff.

No problem. I will wait for this to be merged so I can take advantage of the tests you have implemented. I will consider making my change using this one as base, although I'm not sure about the Zephyr's policy with regards to 'stacked' PRs.

@@ -0,0 +1 @@
CONFIG_NOCACHE_MEMORY=y
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer necessary I believe.

@@ -803,7 +809,8 @@ static int transceive_dma(const struct device *dev,
}

#ifdef CONFIG_SOC_SERIES_STM32H7X
if ((tx_bufs != NULL && !spi_buf_set_in_nocache(tx_bufs)) ||
if ((tx_bufs != NULL && !is_null_buffer(tx_bufs) &&
!spi_buf_set_in_nocache(tx_bufs)) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I have re-read the SPI header code docs. This is what it exactly says about null buffers:

/**
 * @brief SPI buffer structure
 *
 * @param buf is a valid pointer on a data buffer, or NULL otherwise.
 * @param len is the length of the buffer or, if buf is NULL, will be the
 *    length which as to be sent as dummy bytes (as TX buffer) or
 *    the length of bytes that should be skipped (as RX buffer).
 */
struct spi_buf {
	void *buf;
	size_t len;
};

Any buffer in the buf_set could be null and then, dummy bytes should be sent or ignored. Therefore, I would change this function by:

static bool is_dummy_buffer(const struct spi_buf* buf)
{
	return buf->buf == NULL;
}
...
static bool spi_buf_set_in_nocache(const struct spi_buf_set *bufs)
{
	for (size_t i = 0; i < bufs->count; i++) {
		const struct spi_buf *buf = &bufs->buffers[i];

		if (!is_dummy_buffer(buf) &&
			!buf_in_nocache((uintptr_t)buf->buf, buf->len)) {
			return false;
		}
	}
	return true;
}

and leave this as it was:

#ifdef CONFIG_SOC_SERIES_STM32H7X
	if ((tx_bufs != NULL && !spi_buf_set_in_nocache(tx_bufs)) ||
		(rx_bufs != NULL && !spi_buf_set_in_nocache(rx_bufs))) {
		return -EFAULT;
	}
#endif /* CONFIG_SOC_SERIES_STM32H7X */

Copy link
Contributor

@dgastonochoa dgastonochoa Aug 24, 2023

Choose a reason for hiding this comment

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

Hi again, don't worry about this change because I have included it in #61265

@FRASTM
Copy link
Collaborator Author

FRASTM commented Aug 31, 2023

This PR is no more useful as the #61265 covers all the changes

@FRASTM FRASTM closed this Aug 31, 2023
@FRASTM FRASTM deleted the issue60977 branch August 31, 2023 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: SPI SPI bus platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stm32h7 tests: spi_loopback fails due to mem_region
5 participants