Skip to content

RFC: drivers: stm32: SPI: Check that SPI buffers are in a nocache region #57786

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 28, 2023
Merged

RFC: drivers: stm32: SPI: Check that SPI buffers are in a nocache region #57786

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions drivers/spi/spi_ll_stm32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,36 @@ LOG_MODULE_REGISTER(spi_ll_stm32);
#endif /* CONFIG_SOC_SERIES_STM32MP1X */

#ifdef CONFIG_SPI_STM32_DMA

#ifdef CONFIG_SOC_SERIES_STM32H7X

#define IS_NOCACHE_MEM_REGION(node_id) \
COND_CODE_1(DT_ENUM_HAS_VALUE(node_id, zephyr_memory_attr, RAM_NOCACHE), \
(1), \
(0))
Comment on lines +57 to +60
Copy link
Contributor

@carlocaione carlocaione Jul 28, 2023

Choose a reason for hiding this comment

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

A better way to do this: #60850


#define GET_MEM_REGION(node_id) \
{ \
.start = DT_REG_ADDR(node_id), \
.end = (DT_REG_ADDR(node_id) + DT_REG_SIZE(node_id)) - 1, \
},

#define GET_MEM_REGION_IF_NOCACHE(node_id) \
COND_CODE_1(IS_NOCACHE_MEM_REGION(node_id), \
( \
GET_MEM_REGION(node_id) \
), ())

struct mem_region {
uintptr_t start;
uintptr_t end;
};

static const struct mem_region nocache_mem_regions[] = {
DT_MEMORY_ATTR_FOREACH_NODE(GET_MEM_REGION_IF_NOCACHE)
};
#endif /* CONFIG_SOC_SERIES_STM32H7X */

/* dummy value used for transferring NOP when tx buf is null
* and use as dummy sink for when rx buf is null
*/
Expand Down Expand Up @@ -723,6 +753,34 @@ static int wait_dma_rx_tx_done(const struct device *dev)
return res;
}

#ifdef CONFIG_SOC_SERIES_STM32H7X
static bool buf_in_nocache(uintptr_t buf, size_t len_bytes)
{
for (size_t i = 0; i < ARRAY_SIZE(nocache_mem_regions); i++) {
const struct mem_region *mem_reg = &nocache_mem_regions[i];

const bool buf_within_bounds =
(buf >= mem_reg->start) && ((buf + len_bytes - 1) <= mem_reg->end);
Copy link
Contributor

@carlocaione carlocaione Jul 28, 2023

Choose a reason for hiding this comment

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

why you do not remove the -1 from mem_reg->end and just:

(buf + len_bytes) < mem_reg->end

Copy link
Contributor Author

@dgastonochoa dgastonochoa Jul 28, 2023

Choose a reason for hiding this comment

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

I did this on purpose. In general terms, this is checking if a number X is within a closed interval. If you would need to do this, you would naturally use <= and >=. I wanted to follow this pattern as it seems more intuitive. Otherwise, the bool variable is called "within_bounds" but it seems to not be doing exactly that. It seams more clear to me this way. In other contexts (most of them possibly), I would do as you say.

if (buf_within_bounds) {
return true;
}
}
return false;
}

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 (!buf_in_nocache((uintptr_t)buf->buf, buf->len)) {
return false;
}
}
return true;
}
#endif /* CONFIG_SOC_SERIES_STM32H7X */

static int transceive_dma(const struct device *dev,
const struct spi_config *config,
const struct spi_buf_set *tx_bufs,
Expand All @@ -744,6 +802,13 @@ static int transceive_dma(const struct device *dev,
return -ENOTSUP;
}

#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;
Copy link
Member

@hubertmis hubertmis Jul 13, 2023

Choose a reason for hiding this comment

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

Do I understand correctly this PR is not compatible with existing Zephyr samples or drivers using SPI like the following?

zephyr/drivers/flash/spi_nor.c

Line 301 in 7c418a0

uint8_t buf[5] = { 0 };

zephyr/drivers/sensor/bmp388/bmp388_spi.c

Line 55 in 7c418a0

ret = spi_transceive_dt(&bus->spi, &tx, &rx);

These modules allocate SPI buffers on stacks or any other memory, not forcing them to be in non-cachable regions.

Copy link
Contributor Author

@dgastonochoa dgastonochoa Jul 13, 2023

Choose a reason for hiding this comment

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

I wouldn't say exactly not compatible. In these 2 cases, if STM32H7 SPI is used and DMA is enabled, these functions will silently fail, so I still think it's better to return an error code.

Copy link
Member

@hubertmis hubertmis Jul 13, 2023

Choose a reason for hiding this comment

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

Of course, I prefer returning an error code instead of silently failing. But I was thinking if we could aim for something like:

if (tx_bufs != NULL && !spi_buf_set_in_nocache(tx_bufs)) {
		copy_to_nocache_bufs(nocache_tx_bufs, tx_bufs);
}
if (rx_bufs != NULL && !spi_buf_set_in_nocache(rx_bufs)) {
		copy_to_nocache_bufs(nocache_rx_bufs, rx_bufs);
}

Copy link
Contributor

@Benni77 Benni77 Feb 13, 2024

Choose a reason for hiding this comment

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

Of course, I prefer returning an error code instead of silently failing. But I was thinking if we could aim for something like: ...

Hi, I am currently facing the task of installing BLE on the STM32H747 via hci_spi. As long as I use CONFIG_SPI_STM32=y or CONFIG_SPI_STM32_INTERRUPT=y, it works fine. But with CONFIG_SPI_STM32_DMA=y I get error -14 (EFAULT), from exactly this line.

Am I right in assuming that the hci_spi driver - at least on the STM32H7 - currently has no DMA support? Would the buffers in drivers/bluetooth/hci/spi.c have to be provided with __attribute__((__section__(".nocache"))) on the STM32H7?

Any suggestions on how I should proceed to use hci_spi with DMA?
Thanks for the help!

Copy link
Contributor Author

@dgastonochoa dgastonochoa Feb 13, 2024

Choose a reason for hiding this comment

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

Am I right in assuming that the hci_spi driver - at least on the STM32H7 - currently has no DMA support? Would the buffers in drivers/bluetooth/hci/spi.c have to be provided with attribute((section(".nocache"))) on the STM32H7?

Hi, if you want to use SPI in H7 with DMA enabled, the buffers provided to the SPI transceive function must be in a nocache memory region. The preferred way to do this explained in this document: ${ZEPHYR_HOME}/doc/services/mem_mgmt/index.rst

In summary, you can declare a memory region as nocache in devicetree, then use __attribute__((section(...))) to place the required buffer in such region.

Copy link
Contributor

@Benni77 Benni77 Feb 13, 2024

Choose a reason for hiding this comment

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

In summary, you can declare a memory region as nocache in devicetree, then use __attribute__((section(...))) to place the required buffer in such region.

Cool thanks for the info, that helps me a lot. Together with the tests here: #66688 I can somehow implement this.

However, as long as I use the hci_spi driver from zephyr, I also have to edit the driver to realize this.
In your opinion, does it make sense to open an issue (feature request) for this?

Copy link
Member

@erwango erwango Feb 14, 2024

Choose a reason for hiding this comment

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

@Benni77 Yes, it makes sense. We need to be able to return a meaningful error in such case.

}
#endif /* CONFIG_SOC_SERIES_STM32H7X */

spi_context_lock(&data->ctx, asynchronous, cb, userdata, config);

k_sem_reset(&data->status_sem);
Expand Down