arm: stm32: Data cache (dcache) management update

[lmajewski]

This patch series fixes problem with enabling at the same time STM32H7's Data cache and CONFIG_NOCACHE_MEMORY option.

On the STM32 it shall be possible to use memory buffers with __nocache attribute with data cache being enabled.
This is not the best possible solution - one could fix cache coherency issues on the DMA driver level, but allows avoiding
performance penalty when data cache is disabled.

This solution has been tested on nucleo_h743zi with littlefs sample program.
west build -p always -b nucleo_h743zi ./zephyr/samples/subsys/fs/littlefs -DCONF_FILE=prj_blk.conf

This patch series comply with point '2.1' of #36471

Fixes: #45739

[erwango]

I don't think we should go with this solution as this is globally the same as #35165 which was already rejected

[lmajewski]

I don't think we should go with this solution as this is globally the same as #35165 which was already rejected

@erwango The #35165 has been already accepted and pulled - the #ifndef CONFIG_NOCACHE_MEMORY is already present in soc/arm/st_stm32/stm32h7/soc_m7.c.

And the current situation (as it is now in Zephyr repo's - main branch) is IMHO just wrong and reduces system performance considerably. Customers are really concerned about this.

With this patch we are going one small step forward - with having DCache enabled with DMA running with __nocache attribute for crucial buffers, which are really small.

Some STM32 drivers do require defining the CONFIG_NOCACHE_MEMORY and as a result, now the DCache is disabled for the whole system.

This patch allows developers to use DCache and also check if their __nocache defined DMA buffers also are handled correctly. Until Zephyr gains proper handling of cache coherency, this would reduce the possible performance degradation (as it is now noticeable by some Zephyr users).

[erwango]

@erwango The #35165 has been already accepted and pulled - the #ifndef CONFIG_NOCACHE_MEMORY is already present in soc/arm/st_stm32/stm32h7/soc_m7.c.

Ok, this is not strictly exact, but in the end you're right.
#35165, which was affecting F7 was indeed rejected, but some time after, #37716, targeting H7 and doing the same exact thing was merged.
So, I do agree a change is required to offer possibility to use DCache and nocache memory sections.
But then, I think DCACHE_DISABLE should be default n and enabled only when required by drivers (like uart async api, cf tests/drivers/uart/uart_async_api/boards/nucleo_h723zg.conf)
Also, I'd suggest to extend to other M7 based cores, at least on STM32, ie STM32F7.

[lmajewski]

@erwango The #35165 has been already accepted and pulled - the #ifndef CONFIG_NOCACHE_MEMORY is already present in soc/arm/st_stm32/stm32h7/soc_m7.c.

Ok, this is not strictly exact, but in the end you're right. #35165, which was affecting F7 was indeed rejected, but some time after, #37716, targeting H7 and doing the same exact thing was merged. So, I do agree a change is required to offer possibility to use DCache and nocache memory sections. But then, I think DCACHE_DISABLE should be default n and enabled only when required by drivers (like uart async api, cf tests/drivers/uart/uart_async_api/boards/nucleo_h723zg.conf) Also, I'd suggest to extend to other M7 based cores, at least on STM32, ie STM32F7.

The problem here is that we already have many systems with DCache disabled when CONFIG_NOCACHE_MEMORY is enabled. This patch series tries to not introduce regressions as much as possible.
It would be the system integrator responsibility to enable the DCACHE explicitly (i.e. CONFIG_DCACHE_DISABLE=n) when he/she is also planning to use nocache memory DMA buffers (as it is with mine nucleo board).

However, I don't mind to follow your guidelines if other developers (i.e. @de-nordic @Laczen @FRASTM @ABOSTM @nvlsianpu ) agree about this approach.

[lmajewski]

@erwango The #35165 has been already accepted and pulled - the #ifndef CONFIG_NOCACHE_MEMORY is already present in soc/arm/st_stm32/stm32h7/soc_m7.c.

Ok, this is not strictly exact, but in the end you're right. #35165, which was affecting F7 was indeed rejected, but some time after, #37716, targeting H7 and doing the same exact thing was merged. So, I do agree a change is required to offer possibility to use DCache and nocache memory sections. But then, I think DCACHE_DISABLE should be default n and enabled only when required by drivers (like uart async api, cf tests/drivers/uart/uart_async_api/boards/nucleo_h723zg.conf) Also, I'd suggest to extend to other M7 based cores, at least on STM32, ie STM32F7.

The problem here is that we already have many systems with DCache disabled when CONFIG_NOCACHE_MEMORY is enabled. This patch series tries to not introduce regressions as much as possible. It would be the system integrator responsibility to enable the DCACHE explicitly (i.e. CONFIG_DCACHE_DISABLE=n) when he/she is also planning to use nocache memory DMA buffers (as it is with mine nucleo board).

However, I don't mind to follow your guidelines if other developers (i.e. @de-nordic @Laczen @FRASTM @ABOSTM @nvlsianpu ) agree about this approach.

Gentle PING on this patch set ....

[erwango]

However, I don't mind to follow your guidelines if other developers (i.e. @de-nordic @Laczen @FRASTM @ABOSTM @nvlsianpu ) agree about this approach.

I'd go for deactivating it by default and add a note in the release note to inform users.

[de-nordic]

However, I don't mind to follow your guidelines if other developers (i.e. @de-nordic @Laczen @FRASTM @ABOSTM @nvlsianpu ) agree about this approach.

I'd go for deactivating it by default and add a note in the release note to inform users.

@erwango
So you want to go with DCACHE_DISABLED=n by default?

Shouldn't, also, the option be dependent on CACHE_MANAGEMENT and disable DCACHE_LINE_SIZE_DETECT and DCACHE_LINE_SIZE when DCACHE_DISABLED=n?

Wouldn't we maybe go further add something like 'HAS_HW_DCACHE', that would be selected by platform and that would then show the DCACHE_* options?

Maybe we can, instead of DCACHE_DISABLED=n, have DCACHE=y defined like

config DCACHE
    bool ...
    default y if HAS_HW_DCACHE

And it would be y for platforms that define the HAS_HW_DCACHE (or whatever) and user projects could still override it to n,

[lmajewski]

However, I don't mind to follow your guidelines if other developers (i.e. @de-nordic @Laczen @FRASTM @ABOSTM @nvlsianpu ) agree about this approach.

I'd go for deactivating it by default and add a note in the release note to inform users.

@erwango So you want to go with DCACHE_DISABLED=n by default?

Shouldn't, also, the option be dependent on CACHE_MANAGEMENT and disable DCACHE_LINE_SIZE_DETECT and DCACHE_LINE_SIZE when DCACHE_DISABLED=n?

Wouldn't we maybe go further add something like 'HAS_HW_DCACHE', that would be selected by platform and that would then show the DCACHE_* options?

Maybe we can, instead of DCACHE_DISABLED=n, have DCACHE=y defined like

config DCACHE
    bool ...
    default y if HAS_HW_DCACHE

And it would be y for platforms that define the HAS_HW_DCACHE (or whatever) and user projects could still override it to n,

@de-nordic Is there any example of HAS_HW_DCACHE usage already present in Zephyr? Personally, I would opt for enabling the DCACHE by default and then, only when integrator finds issues, allow disabling it.

However, such approach could break some existing STM32H7 boards, as they run with DCACHE disabled when drivers with __nocache buffers are used.

[erwango]

However, such approach could break some existing STM32H7 boards, as they run with DCACHE disabled when drivers with __nocache buffers are used.

This is a good example of this non sense situation. This should be fixed. And on these ones, I don't think this should raise issues.
Where I expect issues is on drivers (well, more on applications using these drivers actually) that don't use __nocache buffers, but need some, like uart_async where DMA is used.
For these, in a first step, we can replace in tree current CONFIG_NOCACHE_MEMORY=y by CONFIG_DCACHE=n.
For sure this will have impact on out of tree users for sure, but anyhow current way of handling this on STM32H7 is wrong and should be fixed one day.

[lmajewski]

However, such approach could break some existing STM32H7 boards, as they run with DCACHE disabled when drivers with __nocache buffers are used.

This is a good example of this non sense situation. This should be fixed. And on these ones, I don't think this should raise issues. Where I expect issues is on drivers (well, more on applications using these drivers actually) that don't use __nocache buffers, but need some, like uart_async where DMA is used. For these, in a first step, we can replace in tree current CONFIG_NOCACHE_MEMORY=y by CONFIG_DCACHE=n. For sure this will have impact on out of tree users for sure, but anyhow current way of handling this on STM32H7 is wrong and should be fixed one day.

Please correct my understanding if it is wrong:

• I need to introduce CONFIG_DCACHE for STM32H7 (similar to patches sent with this PR) set by default to y
• All instances of CONFIG_NOCACHE_MEMORY=y shall be replaced by CONFIG_DCACHE=n
• We do accept out-of-tree boards breakage - boards maintainers' are responsible for checking if those will run with DCACHE enabled.
• Support for HAS_HW_DCACHE is for now skipped

Or am I missing something?

[erwango]

Please correct my understanding if it is wrong:

• I need to introduce CONFIG_DCACHE for STM32H7 (similar to patches sent with this PR) set by default to y

Yes. Use CONFIG_DCACHE in STM32H7 soc_m7.c to protect code enabling DCache.

• All instances of CONFIG_NOCACHE_MEMORY=y shall be replaced by CONFIG_DCACHE=n

Yes, for STM32H7 based boards only.

• We do accept out-of-tree boards breakage - boards maintainers' are responsible for checking if those will run with DCACHE enabled.

Unfortunately, I don't see way to generate a smart warning, so I can think of only 2 ways to help out of tree users:

• Add note in doc/releases/release-notes-3.1.rst
• Send a mail to users/devel (I can do it if required).

• Support for HAS_HW_DCACHE is for now skipped

Fine with me. Maybe @de-nordic can comment.

Or am I missing something?

We'll aslo need to get STM32F7 to also use DCACHE, but that's another story.

[lmajewski]

Please correct my understanding if it is wrong:

• I need to introduce CONFIG_DCACHE for STM32H7 (similar to patches sent with this PR) set by default to y

Yes. Use CONFIG_DCACHE in STM32H7 soc_m7.c to protect code enabling DCache.

• All instances of CONFIG_NOCACHE_MEMORY=y shall be replaced by CONFIG_DCACHE=n

Yes, for STM32H7 based boards only.

• We do accept out-of-tree boards breakage - boards maintainers' are responsible for checking if those will run with DCACHE enabled.

Unfortunately, I don't see way to generate a smart warning, so I can think of only 2 ways to help out of tree users:

* Add note in `doc/releases/release-notes-3.1.rst`

* Send a mail to users/devel (I can do it if required).

• Support for HAS_HW_DCACHE is for now skipped

Fine with me. Maybe @de-nordic can comment.

Or am I missing something?

We'll aslo need to get STM32F7 to also use DCACHE, but that's another story.

@de-nordic Do you have any input or comments to the above plan?

[lmajewski]

@nashif @carlescufi Can this patch set be pulled?