stm32h7 use non cacheable region by alias

[KozhinovAlexander]

Use none-cacheable region by alias (e.g. DT_ALIAS(sramnocache)) instad of node-label (e.g. DT_NODELABEL(sram3)).

This PR allows defining no-cacheable RAM in any of existion sram3, sram4 or backup-sram. It provides more flexibility than current fixed naming implementation. For example current none-cacheable region for stm32h7 series is named sram3, but atm32h723 does not have auch named sram. Thus it is simpler and flexibler to define it with label by pointing on ram region of your choice.

this PR needed for the next-step: definifg none-cacheable region's size by getting value as DT attribute!

[JordanYates]

This adds a great deal of complexity to the linker script to address a single use case. I am also not sure how these changes are required for the stated goal of defining the size from devicetree, it is already possible to get the size of SRAM3 in linker scripts.

I think this solution is much easier to support and generalize if memory region names were derived directly from the devicetree node label property, as then GROUP_DATA_LINK_IN(sramnocache, sramnocache) becomes GROUP_DATA_LINK_IN(DT_LABEL(DT_ALIAS(sramnocache)),...) , removing the need for the core linker file changes.

[JordanYates]

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.

[KozhinovAlexander]

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.

[JordanYates]

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.

[KozhinovAlexander]

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?

[JordanYates]

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.

[JordanYates]

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.

[KozhinovAlexander]

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

[JordanYates]

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.

[KozhinovAlexander]

Do You have then any better ideas?

[JordanYates]

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.

[KozhinovAlexander]

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.

[JordanYates]

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.

[KozhinovAlexander]

This adds a great deal of complexity to the linker script to address a single use case. I am also not sure how these changes are required for the stated goal of defining the size from devicetree, it is already possible to get the size of SRAM3 in linker scripts.

I think this solution is much easier to support and generalize if memory region names were derived directly from the devicetree node label property, as then GROUP_DATA_LINK_IN(sramnocache, sramnocache) becomes GROUP_DATA_LINK_IN(DT_LABEL(DT_ALIAS(sramnocache)),...) , removing the need for the core linker file changes.

I disagree with it. I think aliases are more flexible for users later.

[KozhinovAlexander]

@erwango Can you please take a look on it? I need this mostly for stm32h723 but it is of course scalable solution.

[KozhinovAlexander]

@JordanYates Do you have any progress with your PR? Can we somehow consolidate our work?

[KozhinovAlexander]

@JordanYates Do you have any progress with your PR? Can we somehow consolidate our work?

[JordanYates]

@JordanYates Do you have any progress with your PR? Can we somehow consolidate our work?

Yes my PR is progressing, we just need to finalize which property we are using for the name generation.
This will be discussed and hopefully resolved at the next dev-review meeting (Thursday), if not before then.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.