Skip to content

Conversation

@KozhinovAlexander
Copy link
Contributor

@KozhinovAlexander KozhinovAlexander commented Nov 29, 2020

Define multiple SRAM sections within sram0. With this PR each stm32h745 based board can use either whole sram0 (choose zephyr,sram = &sram0;) or only parts of it (choose zephyr,sram = &ahb_sram1; for example).
This step is necessary toward whole SRAM utilization for projects.

Same behavior for SRAM realized in linux kernel.

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

These changes do not follow Linux Kernel docs. In their model, SRAM entries are children with relative addresses, I can't see this structure here neither updated bindings. A use case should also be provided.

@KozhinovAlexander KozhinovAlexander force-pushed the stm32h745_sram_sections branch 2 times, most recently from 154990d to 771d41e Compare December 1, 2020 18:52
@KozhinovAlexander
Copy link
Contributor Author

@gmarull Thank you for your review. I've prepared changes. Please take a look.

These changes do not follow Linux Kernel docs. In their model, SRAM entries are children with relative addresses, I can't see this structure here neither updated bindings.

Fixed. Please see dts/arm/st/h7/stm32h745.dtsi within latest push.

A use case should also be provided.

You cane take a look at boards/arm/nucleo_h745zi_q/nucleo_h745zi_q_m4.dts for sample usage.

But generally following use case are possible now:

  1. Choose whole sram0 with 864KiB size for stm32h745:

chosen {
zephyr,sram = &sram0;
};

  1. Choose whole ahb_sram2 with 128KiB size for stm32h745 as part of sram0:

chosen {
zephyr,sram = &ahb_sram2;
};

Other use cases are similar to 1 and 2. Both were tested with my nucleo_h745zi_q board and civetweb/websocket_server sample and working fine for me.

@KozhinovAlexander KozhinovAlexander removed the DNM This PR should not be merged (Do Not Merge) label Dec 1, 2020
@erwango erwango added RFC Request For Comments: want input from the community DNM This PR should not be merged (Do Not Merge) labels Dec 2, 2020
@erwango erwango added the dev-review To be discussed in dev-review meeting label Dec 3, 2020
Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

LGTM

@galak galak removed the dev-review To be discussed in dev-review meeting label Dec 3, 2020
@galak
Copy link
Contributor

galak commented Dec 3, 2020

For now i'd suggest carving out the sram region for ethernet and putting a phandle property in the ethernet node that references that sram region.

As far as the other regions go, are you trying to treat them as on large system memory pool? If so, for now we have to fake that out in DTS.

@KozhinovAlexander
Copy link
Contributor Author

KozhinovAlexander commented Dec 3, 2020

@galak Thak you for your review

For now i'd suggest carving out the sram region for ethernet and putting a phandle property in the ethernet node that references that sram region.

I would prefer to wait with merge instead, untill we're done wit MPU SRAM #30403 configuration for ethernet. I will be able update this PR continuously.

As far as the other regions go, are you trying to treat them as on large system memory pool? If so, for now we have to fake that out in DTS.

Yes, the purpose of this PR to give access to all SRAM regions of the Zephyr application, if it needs it. Do you have an example, how to fake it out in DT?

@KozhinovAlexander KozhinovAlexander force-pushed the stm32h745_sram_sections branch 3 times, most recently from a48eda8 to 40b4246 Compare December 20, 2020 16:21
@KozhinovAlexander KozhinovAlexander force-pushed the stm32h745_sram_sections branch 3 times, most recently from 80b7d41 to 42c832f Compare January 12, 2021 21:31
@KozhinovAlexander
Copy link
Contributor Author

@erwango Can you confirm this memory model works on your board (for example with hello_world and/or civetweb webscoket server)?

@KozhinovAlexander
Copy link
Contributor Author

@gmarull Please review

@KozhinovAlexander KozhinovAlexander removed the DNM This PR should not be merged (Do Not Merge) label Jan 19, 2021
@KozhinovAlexander KozhinovAlexander force-pushed the stm32h745_sram_sections branch 2 times, most recently from f9904b8 to c95aebe Compare January 20, 2021 20:16
Copy link
Member

Choose a reason for hiding this comment

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

mmio-sram compatible does not specify ranges as a property nor child bindings (here we have a parent and a child with the same compatible). I think this needs more discussion as well as proper support in Zephyr, not just definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmio-sram compatible does not specify ranges as a property nor child bindings (here we have a parent and a child with the same compatible). I think this needs more discussion as well as proper support in Zephyr, not just definitions.

Where can it be better discussed?

Copy link
Member

Choose a reason for hiding this comment

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

@Nukersson #dev-review meeting

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to have these as one node v separate nodes for each SRAM ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a need to have these as one node v separate nodes for each SRAM ?

With this approach I am able to use all SRAM sections of H7 together.

Copy link
Contributor

Choose a reason for hiding this comment

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

My initial feeling is that I'd suggest that be done as a board specific setting of CONFIG_SRAM_SIZE/CONFIG_SRAM_BASE_ADDRESS. I'm concerned about the precedence set of describing multiple SRAM regions this way and what assumptions one can really make.

What are you pointing zephyr,sram at in the case you are using all the SRAM sections together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial feeling is that I'd suggest that be done as a board specific setting of CONFIG_SRAM_SIZE/CONFIG_SRAM_BASE_ADDRESS. I'm concerned about the precedence set of describing multiple SRAM regions this way and what assumptions one can really make.

What are you pointing zephyr,sram at in the case you are using all the SRAM sections together?

Compiling samples/hello_world with zephyr,sram = &axi_sram; gives:

[129/136] Linking C executable zephyr/zephyr_prebuilt.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       16580 B         1 MB      1.58%
            DTCM:          0 GB       128 KB      0.00%
            SRAM:        4512 B       512 KB      0.86%
        IDT_LIST:         120 B         2 KB      5.86%
[136/136] Linking C executable zephyr/zephyr.elf

and with zephyr,sram = &sram0; gives:

[129/136] Linking C executable zephyr/zephyr_prebuilt.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       16580 B         1 MB      1.58%
            DTCM:          0 GB       128 KB      0.00%
            SRAM:        4512 B       864 KB      0.51%
        IDT_LIST:         120 B         2 KB      5.86%
[136/136] Linking C executable zephyr/zephyr.elf

Especially see difference in Region Size of SRAM section.

Thus my approach still allows using of each separate SRAM section within the whole SRAM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

I still see this as an application specific need and not a generic solution. It just happens that all the SRAM regions are contiguous so things work out that way.

In addition, the way you are using ranges + reg is technically in violation of DTS. If you have a range the node that has that shouldn't have a reg that overlaps that range region. Since you are effectively claiming 2 different device nodes for the same reg region than.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Jan 25, 2021

Choose a reason for hiding this comment

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

Ok. I see your point. Thank you very much. But how would you put different memory banks of some SoC with non-consequtive memory addresses together? Would you use device-tree or linker skript?
The application would be best utilization of all memory banks of some SoC like stm32h743/5

@KozhinovAlexander KozhinovAlexander force-pushed the stm32h745_sram_sections branch 2 times, most recently from c0d323a to ca8e05f Compare January 23, 2021 20:19
Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

Current solution is not generic enough. Makes assumptions about SRAM regions being contiguous which may not be valid on all platforms with multiple SRAM regions.

Would rather see something on the linker side that tries to take all the 'mmio-sram' regions from DTS and coalesce them. Or support multiple different memory regions in the core of Zephyr.

Alexander Kozhinov added 5 commits January 25, 2021 22:19
define only single sram0
define sram sections within sram0

Signed-off-by: Alexander Kozhinov <AlexanderKozhinov@yandex.com>
change choosen sram name

Signed-off-by: Alexander Kozhinov <AlexanderKozhinov@yandex.com>
change choosen sram name to axi_sram

Signed-off-by: Alexander Kozhinov <AlexanderKozhinov@yandex.com>
change choosen sram name

Signed-off-by: Alexander Kozhinov <AlexanderKozhinov@yandex.com>
change choosen sram name to axi_sram

Signed-off-by: Alexander Kozhinov <AlexanderKozhinov@yandex.com>
@KozhinovAlexander
Copy link
Contributor Author

Closed since not generic

@KozhinovAlexander KozhinovAlexander deleted the stm32h745_sram_sections branch January 31, 2021 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards area: Devicetree area: Memory Management platform: STM32 ST Micro STM32 RFC Request For Comments: want input from the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants