Skip to content

stm32h7 use non cacheable region by alias #36152

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

Closed
wants to merge 4 commits into from
Closed

stm32h7 use non cacheable region by alias #36152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c409715
boards: arm: nucleo_h745zi_q
Jun 10, 2021
1931067
drivers: ethernet: eth_stm32_hal
Jun 10, 2021
e73f2ce
soc: arm: st_stm32: stm32h7
Jun 10, 2021
849ccb1
arm: aarch32: cortex-M
Jun 10, 2021
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
1 change: 1 addition & 0 deletions boards/arm/nucleo_h745zi_q/nucleo_h745zi_q_m7.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
led0 = &green_led;
pwm-led0 = &red_pwm_led;
sw0 = &user_button;
sramnocache = &sram3;
};
};

Expand Down
2 changes: 1 addition & 1 deletion drivers/ethernet/eth_stm32_hal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ LOG_MODULE_REGISTER(LOG_MODULE_NAME);
#define __eth_stm32_desc __dtcm_noinit_section
#define __eth_stm32_buf __dtcm_noinit_section
#elif defined(CONFIG_SOC_SERIES_STM32H7X) && \
DT_NODE_HAS_STATUS(DT_NODELABEL(sram3), okay)
DT_NODE_HAS_STATUS(DT_ALIAS(sramnocache), okay)
Copy link
Contributor

@JordanYates JordanYates Jun 11, 2021

Choose a reason for hiding this comment

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

This change means that only STM32H7X boards that define sramnocache will work with this driver now.
As this is more than just nucleo_h745zi_q, this will break the driver for many boards without further board updates.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Jun 12, 2021

Choose a reason for hiding this comment

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

No. This is not. The other boards will still work. Please take a look on on first #if and last #else. There are many cases upon sramnocache already covered.
There maybe some documentation effort for stm32 needed to advice users to use sramnocache.

Copy link
Contributor

@JordanYates JordanYates Jun 12, 2021

Choose a reason for hiding this comment

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

Ok, they will still work, but you are changing the behavior for every STM32H7X board apart from nucleo_h745zi_q. This is not necessarily wrong, but it is not explained anywhere in the commits as to why this is a good idea / why the previous way was bad.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Jun 12, 2021

Choose a reason for hiding this comment

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

This is a good point. I would say, it would be nice to add some documentation to the whole he series. But there is no docs for it, as I know. Maybe it can be added to .yaml or the soc? @erwango?

Copy link
Contributor

@JordanYates JordanYates Jun 12, 2021

Choose a reason for hiding this comment

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

The point is not that it is not documented in the code, but that you have provided no justification for the change in the commit messages.

#define __eth_stm32_desc __attribute__((section(".eth_stm32_desc")))
#define __eth_stm32_buf __attribute__((section(".eth_stm32_buf")))
#elif defined(CONFIG_NOCACHE_MEMORY)
Expand Down
32 changes: 27 additions & 5 deletions include/arch/arm/aarch32/cortex_m/scripts/linker.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,33 @@ MEMORY
LINKER_DT_REGION_FROM_NODE(SRAM1, rw, DT_NODELABEL(sram1))
LINKER_DT_REGION_FROM_NODE(SRAM2, rw, DT_NODELABEL(sram2))
/* STM32 alternate RAM configurations */
LINKER_DT_REGION_FROM_NODE(SRAM3, rw, DT_NODELABEL(sram3))
LINKER_DT_REGION_FROM_NODE(SRAM4, rw, DT_NODELABEL(sram4))
LINKER_DT_REGION_FROM_NODE(SDRAM1, rw, DT_NODELABEL(sdram1))
LINKER_DT_REGION_FROM_NODE(SDRAM2, rw, DT_NODELABEL(sdram2))
LINKER_DT_REGION_FROM_NODE(BACKUP_SRAM, rw, DT_NODELABEL(backup_sram))

#if DT_NODE_HAS_STATUS(DT_ALIAS(sramnocache), okay)
DT_REGION_FROM_NODE_STATUS_OKAY(sramnocache, rw, DT_ALIAS(sramnocache))
Copy link
Contributor

@JordanYates JordanYates Jun 11, 2021

Choose a reason for hiding this comment

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

It appears that the purpose of these changes is purely to rename the memory region of whichever node sramnocache points to to sramnocache. This adds a lot of conditional and duplication to achieve this, and will become unreadable as soon as another node also needs to have its name changed.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Jun 12, 2021

Choose a reason for hiding this comment

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

Well, some knowledge in preprocessor and linker skripts is already assumend. But my implementation is simpliest possible for this purpose.

Copy link
Contributor

@JordanYates JordanYates Jun 12, 2021

Choose a reason for hiding this comment

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

Unreadable from the perspective of the number of conditionals that need to be added.
This only looks "simple" because this is the first special case. When someone wants to add DT_ALIAS(sramcache) that changes the name of a different SRAM region, you end up in a case of 2^n combinations.
I disagree that this is the simplest way possible to change the name of a single memory region.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Jun 12, 2021

Choose a reason for hiding this comment

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

Do You have then any better ideas?

Copy link
Contributor

@JordanYates JordanYates Jun 12, 2021

Choose a reason for hiding this comment

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

Yes, the PR which mentions this one, #36196.
This allows both changing the memory region name, and automatically extracting the region name from a chosen or alias node.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Jun 13, 2021

Choose a reason for hiding this comment

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

Ok. Thanks for that. I see your point. Of course, using some script is more elegant solution, than using ifdefs in linker skript. Moreover it definitely increase readability.
Anyway I still would prefer defining special use cases like none-cacheable memory regions by fixed aliases instead of fixed label names. The reason for that is that, labels may be changed b the user and used for other stuff than linker script. Thus your solution fixes label names, which is not completely cosy to use. I think, if you would add aliases in your PR it would be best one solution based on good readable script and you would not need to change lable names in multiple boards.
For example: Adding aliases like sramnocache256b, sramnocache16k - would increase usability.

Copy link
Contributor

@JordanYates JordanYates Jun 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I have no problem with using fixed aliases instead of fixed label names, in fact I think that is definitely what you should do. That the memory region name is generated from the label is beside the point. The point is that there is a defined mapping from a devicetree node to the name of the region generated by that node.

As I noted above, GROUP_DATA_LINK_IN(sramnocache, sramnocache) becomes GROUP_DATA_LINK_IN(DT_LABEL(DT_ALIAS(sramnocache)),DT_LABEL(DT_ALIAS(sramnocache))). At that point you can change the label to "ALEXANDERS_SRAM" and everything will still work fine.


#if !DT_SAME_NODE(DT_ALIAS(sramnocache), DT_NODELABEL(sram3))
DT_REGION_FROM_NODE_STATUS_OKAY(SRAM3, rw, DT_NODELABEL(sram3))
#endif
#if !DT_SAME_NODE(DT_ALIAS(sramnocache), DT_NODELABEL(sram4))
DT_REGION_FROM_NODE_STATUS_OKAY(SRAM4, rw, DT_NODELABEL(sram4))
#endif
#if !DT_SAME_NODE(DT_ALIAS(sramnocache), DT_NODELABEL(sdram1))
DT_REGION_FROM_NODE_STATUS_OKAY(SDRAM1, rw, DT_NODELABEL(sdram1))
#endif
#if !DT_SAME_NODE(DT_ALIAS(sramnocache), DT_NODELABEL(sdram2))
DT_REGION_FROM_NODE_STATUS_OKAY(SDRAM2, rw, DT_NODELABEL(sdram2))
#endif
#if !DT_SAME_NODE(DT_ALIAS(sramnocache), DT_NODELABEL(backup_sram))
DT_REGION_FROM_NODE_STATUS_OKAY(BACKUP_SRAM, rw, DT_NODELABEL(backup_sram))
#endif

#else
DT_REGION_FROM_NODE_STATUS_OKAY(SRAM3, rw, DT_NODELABEL(sram3))
DT_REGION_FROM_NODE_STATUS_OKAY(SRAM4, rw, DT_NODELABEL(sram4))
DT_REGION_FROM_NODE_STATUS_OKAY(SDRAM1, rw, DT_NODELABEL(sdram1))
DT_REGION_FROM_NODE_STATUS_OKAY(SDRAM2, rw, DT_NODELABEL(sdram2))
DT_REGION_FROM_NODE_STATUS_OKAY(BACKUP_SRAM, rw, DT_NODELABEL(backup_sram))
#endif
/* Used by and documented in include/linker/intlist.ld */
IDT_LIST (wx) : ORIGIN = 0xFFFFF7FF, LENGTH = 2K
}
Expand Down
10 changes: 5 additions & 5 deletions soc/arm/st_stm32/stm32h7/mpu_regions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ static const struct arm_mpu_region mpu_regions[] = {
REGION_FLASH_ATTR(REGION_FLASH_SIZE)),
MPU_REGION_ENTRY("SRAM", CONFIG_SRAM_BASE_ADDRESS,
REGION_RAM_ATTR(REGION_SRAM_SIZE)),
#if DT_NODE_HAS_STATUS(DT_NODELABEL(sram3), okay) && \
#if DT_NODE_HAS_STATUS(DT_ALIAS(sramnocache), okay) && \
DT_NODE_HAS_STATUS(DT_NODELABEL(mac), okay)
MPU_REGION_ENTRY("SRAM3_ETH_BUF",
DT_REG_ADDR(DT_NODELABEL(sram3)),
MPU_REGION_ENTRY("SRAM_NO_CACHE_ETH_BUF",
DT_REG_ADDR(DT_ALIAS(sramnocache)),
REGION_RAM_NOCACHE_ATTR(REGION_16K)),
MPU_REGION_ENTRY("SRAM3_ETH_DESC",
DT_REG_ADDR(DT_NODELABEL(sram3)),
MPU_REGION_ENTRY("SRAM_NO_CACHE_ETH_DESC",
DT_REG_ADDR(DT_ALIAS(sramnocache)),
REGION_PPB_ATTR(REGION_256B)),
#endif
};
Expand Down
10 changes: 5 additions & 5 deletions soc/arm/st_stm32/stm32h7/sections.ld
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
* SPDX-License-Identifier: Apache-2.0
*/

#if DT_NODE_HAS_STATUS(DT_NODELABEL(sram3), okay) && DT_NODE_HAS_STATUS(DT_NODELABEL(mac), okay)
#if DT_NODE_HAS_STATUS(DT_ALIAS(sramnocache), okay) && DT_NODE_HAS_STATUS(DT_NODELABEL(mac), okay)

SECTION_DATA_PROLOGUE(eth_stm32,(NOLOAD),)
{
. = ABSOLUTE(DT_REG_ADDR(DT_NODELABEL(sram3)));
. = ABSOLUTE(DT_REG_ADDR(DT_ALIAS(sramnocache)));
*(.eth_stm32_desc)
. = ABSOLUTE(DT_REG_ADDR(DT_NODELABEL(sram3))) + 256;
. = ABSOLUTE(DT_REG_ADDR(DT_ALIAS(sramnocache))) + 256;
*(.eth_stm32_buf)
. = ABSOLUTE(DT_REG_ADDR(DT_NODELABEL(sram3))) + 16K;
} GROUP_DATA_LINK_IN(SRAM3, SRAM3)
. = ABSOLUTE(DT_REG_ADDR(DT_ALIAS(sramnocache))) + 16K;
} GROUP_DATA_LINK_IN(sramnocache, sramnocache)

#endif