-
Notifications
You must be signed in to change notification settings - Fork 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
cpu/stm32/periph: add FMC/FSMC support for STM32 #19843
Conversation
571abcf
to
db198d1
Compare
aeac869
to
33c8ff9
Compare
From the testing procedure:
Should the test application rather go in tests/periph/fmc since it's designed as a peripheral driver? |
BTW I tested the application on stm32f723e-disco and stm32f429i-disc1 and it works (e.g. I get a |
I don't have a strong preference here. I was thinking about that but I couldn't finally decide it 🤔 |
cpu/stm32/Makefile.dep
Outdated
ifneq (,$(filter periph_fmc_%,$(USEMODULE))) | ||
USEMODULE += periph_fmc | ||
endif | ||
|
||
ifneq (,$(filter periph_fmc,$(USEMODULE))) | ||
FEATURES_OPTIONAL += periph_fmc_16bit | ||
FEATURES_OPTIONAL += periph_fmc_nor_sram | ||
FEATURES_OPTIONAL += periph_fmc_sdram | ||
endif |
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.
This looks like a circular dependency resolution (periph_fmc depends on periph_fmc_sdram/nor_sram/16bit
and then if periph_fmc
is used, include periph_fmc_sdram/nor_sram/16bit
if possible.
Why not just:
ifneq (,$(filter periph_fmc_%,$(USEMODULE))) | |
USEMODULE += periph_fmc | |
endif | |
ifneq (,$(filter periph_fmc,$(USEMODULE))) | |
FEATURES_OPTIONAL += periph_fmc_16bit | |
FEATURES_OPTIONAL += periph_fmc_nor_sram | |
FEATURES_OPTIONAL += periph_fmc_sdram | |
endif | |
ifneq (,$(filter periph_fmc_%,$(USEMODULE))) | |
FEATURES_REQUIRED += periph_fmc | |
endif |
here, and let the user include the specific implementation. e.g in the test application use:
FEATURES_OPTIONAL += periph_fmc_16bit
FEATURES_OPTIONAL += periph_fmc_nor_sram
FEATURES_OPTIONAL += periph_fmc_sdram
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.
🤔 good question. In this case, I'm really not sure how to do it right. Because FMC only works if all required modules are enabled my intention was that the user cannot decide which features are used. This is decided by the defined features for the board. It would not work if the user only enabled periph_fmc
for example.
I think the situation for FMC is a bit different than for periph_uart_*
or periph_spi_*
where additional features are enabled but the peripheral already works with the base module.
Would it be an option just to remove the circular dependency?
ifneq (,$(filter periph_fmc_%,$(USEMODULE))) | |
USEMODULE += periph_fmc | |
endif | |
ifneq (,$(filter periph_fmc,$(USEMODULE))) | |
FEATURES_OPTIONAL += periph_fmc_16bit | |
FEATURES_OPTIONAL += periph_fmc_nor_sram | |
FEATURES_OPTIONAL += periph_fmc_sdram | |
endif | |
ifneq (,$(filter periph_fmc,$(USEMODULE))) | |
FEATURES_OPTIONAL += periph_fmc_16bit | |
FEATURES_OPTIONAL += periph_fmc_nor_sram | |
FEATURES_OPTIONAL += periph_fmc_sdram | |
endif |
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.
Alternatively we could add in board's Makefile.dep
for SDRAM for example:
ifneq (,$(filter periph_fmc,$(USEMODULE)))
FEATURES_REQUIRED += periph_fmc_16bit
FEATURES_REQUIRED += periph_fmc_sdram
endif
But this would require to add required features in Makefile.features
and in Makefile.dep
.
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.
Just do like I proposed in https://github.com/RIOT-OS/RIOT/pull/19843/files/33c8ff931ee36c0762f0d8e46fe14c7480bc3221#diff-b85e38a85615fbf0712b3a1de6006c631efbdb025ed9d8fd96d8df28f49f7656. Since periph_fmc*
are defined as features, only the boards that provide these features can build the application. Using FEATURES_REQUIRED += periph_fmc
in cpu/stm32/Makefile.dep
does the magic. And it is important to use FEATURES_REQUIRED instead of USEMODULE.
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.
At least the features periph_fmc_16bit
and periph_fmc_32bit
must not be understood as optional additional functionalities, but as mandatory functionalities in case the FMC has to operate with 16 bits and 32 bits respectively.
The periph_sdram
and periph_nor_sram
features, on the other hand, could indeed be considered optional to control whether the SDRAM or the SRAM is used, e.g. when FMC is used for display but the SDRAM or the SRAM should not be used 🤔
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.
And later when FMC will be used for display stuff, we could add:
ifneq (,$(filter disp_dev,$(USEMODULE)))
FEATURES_REQUIRED += periph_fmc_sdram
endif
in a board Makefile.dep
if it should use the sdram fmc with its display.
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.
At least the features
periph_fmc_16bit
andperiph_fmc_32bit
must not be understood as optional additional functionalities, but as mandatory functionalities in case the FMC has to operate with 16 bits and 32 bits respectively.The
periph_sdram
andperiph_nor_sram
features, on the other hand, could indeed be considered optional to control whether the SDRAM or the SRAM is used, e.g. when FMC is used for display but the SDRAM or the SRAM should not be used thinking
Hmm, ok. So maybe in the application Makefile use:
FEATURES_REQUIRED += periph_fmc
FEATURES_OPTIONAL += periph_fmc_sram
FEATURES_OPTIONAL += periph_fmc_sdram
and in the board Makefile.dep:
ifneq (,$(filter periph_fmc,$(USEMODULE)))
FEATURES_REQUIRED += periph_fmc_16bit # or 32bit depending on what the board is providing
endif
What do you think?
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.
Hmm, ok. So maybe in the application Makefile use:
FEATURES_REQUIRED += periph_fmc FEATURES_OPTIONAL += periph_fmc_sram FEATURES_OPTIONAL += periph_fmc_sdramand in the board Makefile.dep:
ifneq (,$(filter periph_fmc,$(USEMODULE))) FEATURES_REQUIRED += periph_fmc_16bit # or 32bit depending on what the board is providing endifWhat do you think?
This is exactly like I understand the dependencies. I will try it in that way.
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.
Ok, I made the following changes:
- The test application is now in
tests/periph/fmc
. periph_fmc_sdram
andperiph_fmc_nor_sram
are enabled byFEATURES_OPTIONAL
in test application.periph_fmc
is still enabled withUSEMODULE
in the test application to be consistent withCONFIG_MODULE_PERIPH_FMC=y
which requires featureperiph_fmc
so that only boards with this feature can be compiled.periph_fmc_16bit
is enabled byFEATURE_REQUIRED
in boardMakefile.dep
.MODULE_PERIPH_FMC_16BIT
andMODULE_PERIPH_FMC_32BIT
are now hidden in Kconfig and enabled byHAVE_PERIPH_FMC_16BIT
andHAVE_PERIPH_FMC_32BIT
.- RAM is enabled as heap in
tests/sys/malloc
if it is compiled withUSEMODULE=periph_fmc_sdram
orUSEMODULE=periph_fmc_nor_sram
. I'm not sure whether we should add FMC asFEATURES_OPTIONAL
here. - If SDRAM or NOR/PSRAM/SRAM is configured in board's
periph_conf.h
but the corresponding feature is not enabeled it is not initialized. It doesn't trigger an assertion anymore but only prints now a warning that the corresponding feature is not enabeld.
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'm not sure whether we should add FMC as
FEATURES_OPTIONAL
here.
I don't know either. It could maybe increase the testing time because the heap can be much larger, so more message to print.
periph_fmc
is still enabled withUSEMODULE
in the test application to be consistent withCONFIG_MODULE_PERIPH_FMC=y
which requires featureperiph_fmc
so that only boards with this feature can be compiled.
In the Makefile, it should still be included using "FEATURES_REQUIRED += ". The features is turned into a USEMODULE
automatically by the build system somewhere else. But it's important to use FEATURES_REQUIRED here so the build system is able to know what boards is supported (and it's also used by Murdock to skip the build on boards that doesn't provide this feature, I think).
Then I'd say put the application in |
I will move it. I was just a bit unsure since FMC is STM32 specific like LTDC. That's why I also placed all type declaration in |
Build failed: |
I found the problem with the Because A18 is pulled down, Now it is working as expected. If the compilation in CI fail, I will provide the fix. Otherwise I will open a very small PR. |
In any case, this tells me that the test app needs to be improved. Although I tried to cross-fill the memory, I did not see this problem. Probably filling the memory linearly with its addresses should deteect such addressing problems. |
Hm, the compilation in Murdock succeeded but the merge was blocked by bors 🤔 |
a7129eb
to
3eec3ae
Compare
To detect misconfigurations of addresses and sizes, the whole memory is filled word-wise with it's addresses. If the content doesn't match, it prints the address and the content. f
3eec3ae
to
6fdc6ef
Compare
@aabadie I took the chance of the unsuccessful bors action to provide the fix for the RIOT/boards/stm32f723e-disco/include/periph_conf.h Lines 303 to 304 in 3eec3ae
I also provide in commit 3eec3ae an improvement of the test app which now actually detects address or size misconfigurations. With the previous misconfiguration of the size for the
where it becomes immediately clear that the write operation to |
This is because I manually restarted the failed job from Murdock UI but apparently that doesn't work with bors... |
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.
bors merge
19843: cpu/stm32/periph: add FMC/FSMC support for STM32 r=aabadie a=gschorcht ### Contribution description The PR provides a driver for STM32 FMC/FSMC peripherals. It supports: - NOR Flashes, - PSRAMs/SRAMs, - SDRAMs, and - Display Controllers with MCU 8-/16-bit parallel interface. NAND Flashes are not yet supported. To use the FMC/FSMC, the `periph_fmc` module has to be enabled. To keep required data structures and resulting code as small as possible, a couple of pseudomodules are defined that have to be used in addition to the `periph_fmc` module to enable supported features. These are: | Module | Feature | |:-----------------------|:----------------------------------------| | `periph_fmc_nor_sram` | enable NOR Flash and PSRAM/SRAM support | | `periph_fmc_sdram` | enable SDRAM support | | `periph_fmc_16bit` | enable 16-bit support | | `periph_fmc_32bit` | enable 32-bit support | The board has then to define - the corresponding features according to the connected external device, - the peripheral configuration of the FMC/FSMC of type `fmc_conf_t,` - the configuration of the FMC banks which describe the connected external devices. The PR includes the support for - `stm32f429i-disc1` with 8 MByte SDRAM - `stm32f746g-disco` with 16 MByte SDRAM - `stm32l496g-disco` with 1 MByte SRAM - `stm32f723e-disco` with 1 MByte SRAM. To use the RAM connected to the FMC as heap, the board defines `FMC_RAM_ADDR` and ` FMC_RAM_LEN`. For that purpose: - the linker symbols `_fmc_ram_addr` and `_fmc_ram_len` are set, - a memory region `fcmram` is added in linker script for the FMC RAM based on these `_fcm_ram_*` linker symbols - a section for the FMC RAM is defined in this memory region that defines the heap by setting `_sheap3` and `_eheap3` and - the number of heaps is set to 4 to use `_sheap3` and `_eheap3` even though `_sheap1` and `_eheap1` (the backup RAM) and `_sheap2` and `_eheap2` (SRAM4) are not present. Once the `drivers/st77xx` and `drivers/lcd` changes are merged, the display for boards like the `stm32l496g-disco` and `stm32f723e-disco` can also use the FMC peripheral. ~**NOTE**: The PR includes the fix in PR #19842 for now (commit 560fea1).~ ### Testing procedure 1. Use one of the boards above and flash `tests/driver/stm32_fmc`, for example: ``` BOARD=stm32f429i-disc1 make -j8 -C tests/drivers/stm32_fmc flash test ``` The test should succeed. **NOTE**: There is still a problem with `stm32f746g-disco`. It crashes with a hard-fault on accessing the upper 8 MByte of the 16 MByte. 2. Use the board above and flash `tests/sys/malloc`, for example: ``` USEMODULE=periph_fmc CFLAGS='-DCHUNK_SIZE=4096 -DDEBUG_ASSERT_VERBOSE' \ BOARD=stm32f429i-disc1 make -j8 -C tests/sys/malloc ``` The FMC RAM should be used for `malloc`. On `stm32f746g-disco` for example ``` ... Allocated 4096 Bytes at 0x2002d7c8, total 184672 Allocated 4096 Bytes at 0x2002e7e0, total 188776 Allocated 4096 Bytes at 0xd0000008, total 192880 Allocated 4096 Bytes at 0xd0001010, total 196984 Allocated 4096 Bytes at 0xd0002018, total 201088 ... Allocated 4096 Bytes at 0xd07fd6d0, total 8544520 Allocated 4096 Bytes at 0xd07fe6e8, total 8548624 Allocations count: 2083 ``` ### Issues/PRs references ~Depends on PR #19842~ Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
@aabadie Do you have a |
I only have stm32f429i-disc1 and stm32f723e-disco. @vincent-d added stm32f769i-disco in #7051 maybe he has one. |
Ok, I will provide the FMC config for the |
Build failed: |
This is very annoying. It fails randomly 80% of the time and is unrelated. bors merge |
19843: cpu/stm32/periph: add FMC/FSMC support for STM32 r=aabadie a=gschorcht ### Contribution description The PR provides a driver for STM32 FMC/FSMC peripherals. It supports: - NOR Flashes, - PSRAMs/SRAMs, - SDRAMs, and - Display Controllers with MCU 8-/16-bit parallel interface. NAND Flashes are not yet supported. To use the FMC/FSMC, the `periph_fmc` module has to be enabled. To keep required data structures and resulting code as small as possible, a couple of pseudomodules are defined that have to be used in addition to the `periph_fmc` module to enable supported features. These are: | Module | Feature | |:-----------------------|:----------------------------------------| | `periph_fmc_nor_sram` | enable NOR Flash and PSRAM/SRAM support | | `periph_fmc_sdram` | enable SDRAM support | | `periph_fmc_16bit` | enable 16-bit support | | `periph_fmc_32bit` | enable 32-bit support | The board has then to define - the corresponding features according to the connected external device, - the peripheral configuration of the FMC/FSMC of type `fmc_conf_t,` - the configuration of the FMC banks which describe the connected external devices. The PR includes the support for - `stm32f429i-disc1` with 8 MByte SDRAM - `stm32f746g-disco` with 16 MByte SDRAM - `stm32l496g-disco` with 1 MByte SRAM - `stm32f723e-disco` with 1 MByte SRAM. To use the RAM connected to the FMC as heap, the board defines `FMC_RAM_ADDR` and ` FMC_RAM_LEN`. For that purpose: - the linker symbols `_fmc_ram_addr` and `_fmc_ram_len` are set, - a memory region `fcmram` is added in linker script for the FMC RAM based on these `_fcm_ram_*` linker symbols - a section for the FMC RAM is defined in this memory region that defines the heap by setting `_sheap3` and `_eheap3` and - the number of heaps is set to 4 to use `_sheap3` and `_eheap3` even though `_sheap1` and `_eheap1` (the backup RAM) and `_sheap2` and `_eheap2` (SRAM4) are not present. Once the `drivers/st77xx` and `drivers/lcd` changes are merged, the display for boards like the `stm32l496g-disco` and `stm32f723e-disco` can also use the FMC peripheral. ~**NOTE**: The PR includes the fix in PR #19842 for now (commit 560fea1).~ ### Testing procedure 1. Use one of the boards above and flash `tests/driver/stm32_fmc`, for example: ``` BOARD=stm32f429i-disc1 make -j8 -C tests/drivers/stm32_fmc flash test ``` The test should succeed. **NOTE**: There is still a problem with `stm32f746g-disco`. It crashes with a hard-fault on accessing the upper 8 MByte of the 16 MByte. 2. Use the board above and flash `tests/sys/malloc`, for example: ``` USEMODULE=periph_fmc CFLAGS='-DCHUNK_SIZE=4096 -DDEBUG_ASSERT_VERBOSE' \ BOARD=stm32f429i-disc1 make -j8 -C tests/sys/malloc ``` The FMC RAM should be used for `malloc`. On `stm32f746g-disco` for example ``` ... Allocated 4096 Bytes at 0x2002d7c8, total 184672 Allocated 4096 Bytes at 0x2002e7e0, total 188776 Allocated 4096 Bytes at 0xd0000008, total 192880 Allocated 4096 Bytes at 0xd0001010, total 196984 Allocated 4096 Bytes at 0xd0002018, total 201088 ... Allocated 4096 Bytes at 0xd07fd6d0, total 8544520 Allocated 4096 Bytes at 0xd07fe6e8, total 8548624 Allocations count: 2083 ``` ### Issues/PRs references ~Depends on PR #19842~ Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
bors cancel |
Canceled. |
bors merge |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
@aabadie Many thanks for your review, your patience and your valuable advises. I really appriciate it 😄 |
Contribution description
The PR provides a driver for STM32 FMC/FSMC peripherals. It supports:
NAND Flashes are not yet supported.
To use the FMC/FSMC, the
periph_fmc
module has to be enabled. To keep required data structures and resulting code as small as possible, a couple of pseudomodules are defined that have to be used in addition to theperiph_fmc
module to enable supported features. These are:periph_fmc_nor_sram
periph_fmc_sdram
periph_fmc_16bit
periph_fmc_32bit
The board has then to define
fmc_conf_t,
The PR includes the support for
stm32f429i-disc1
with 8 MByte SDRAMstm32f746g-disco
with 16 MByte SDRAMstm32l496g-disco
with 1 MByte SRAMstm32f723e-disco
with 1 MByte SRAM.To use the RAM connected to the FMC as heap, the board defines
FMC_RAM_ADDR
andFMC_RAM_LEN
. For that purpose:_fmc_ram_addr
and_fmc_ram_len
are set,fcmram
is added in linker script for the FMC RAM based on these_fcm_ram_*
linker symbols_sheap3
and_eheap3
and_sheap3
and_eheap3
even though_sheap1
and_eheap1
(the backup RAM) and_sheap2
and_eheap2
(SRAM4) are not present.Once the
drivers/st77xx
anddrivers/lcd
changes are merged, the display for boards likethe
stm32l496g-disco
andstm32f723e-disco
can also use the FMC peripheral.NOTE: The PR includes the fix in PR #19842 for now (commit 560fea1).Testing procedure
Use one of the boards above and flash
tests/driver/stm32_fmc
, for example:The test should succeed.
NOTE: There is still a problem withstm32f746g-disco
. It crashes with a hard-fault on accessing the upper 8 MByte of the 16 MByte.Use the board above and flash
tests/sys/malloc
, for example:The FMC RAM should be used for
malloc
. Onstm32f746g-disco
for exampleIssues/PRs references
Depends on PR #19842