You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/03/10 16:04:05 UTC
[GitHub] [incubator-nuttx] AlexanderVasiljev opened a new pull request #529:
Stm32F7: add external ram config
AlexanderVasiljev opened a new pull request #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] AlexanderVasiljev commented on issue #529:
Stm32F7: add external ram config
Posted by GitBox <gi...@apache.org>.
AlexanderVasiljev commented on issue #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#issuecomment-598069507
> Configuration is improperly scoped, improperly name, and in the wrong Kconfig file.
>
> Move unused configuration option to the /board/arm/stm32ft/your-board/Kconfig
>
> Rename the option to CONFIG_YOURBOARD_EXTERNAL_RAM
I would like to insist to apply this patch as is.
1) STM32F7_EXTERNAL_RAM will select ARCH_HAVE_HEAP2, which is used in arch/arm/src/stm32f7/stm32_allocateheap.c. And there is no other way to select ARCH_HAVE_HEAP2. Now it is not obvious that stm32f7 has already support for second heap in arch/arm/src/stm32f7/stm32_allocateheap.c
2) Stm32 and stm32H7 aleady have the same options: STM32H7_EXTERNAL_RAM and STM32_EXTERNAL_RAM. For the sake of uniformity Stm32F7 should have the same option. Otherwise it is frustrating when migrating from stm32f4 to stm32f7 and not finding such a config.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] AlexanderVasiljev commented on a change in pull
request #529: Stm32F7: add external ram config
Posted by GitBox <gi...@apache.org>.
AlexanderVasiljev commented on a change in pull request #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#discussion_r390770344
##########
File path: arch/arm/src/stm32f7/Kconfig
##########
@@ -2136,6 +2136,14 @@ endchoice #"ULPI Selection"
endmenu # OTG_HS Config
+config STM32F7_EXTERNAL_RAM
+ bool "External RAM on FMC"
+ default n
+ depends on STM32F7_FMC
+ select ARCH_HAVE_HEAP2
+ ---help---
+ In addition to internal SDRAM, external RAM may be available through the FMC.
+
Review comment:
The sole purpose of STM32F7_EXTERNAL_RAM is to select ARCH_HAVE_HEAP2.
ARCH_HAVE_HEAP2 is used in arch/arm/src/stm32f7/stm32_allocateheap.c
There is no other way to select ARCH_HAVE_HEAP2.
It is the same as in stm32h7. Stm32H7 has STM32H7_EXTERNAL_RAM, which just selects ARCH_HAVE_HEAP2.
And Stm32 also has this option. STM32_EXTERNAL_RAM
Only Stm32F7 is missing it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #529: Stm32F7: add
external ram config
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#issuecomment-598194555
Okay. I think it is generally bad usage of Kconfig variables. But there seems to be a precedence.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] tritao commented on a change in pull request
#529: Stm32F7: add external ram config
Posted by GitBox <gi...@apache.org>.
tritao commented on a change in pull request #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#discussion_r390825029
##########
File path: arch/arm/src/stm32f7/Kconfig
##########
@@ -2136,6 +2136,14 @@ endchoice #"ULPI Selection"
endmenu # OTG_HS Config
+config STM32F7_EXTERNAL_RAM
+ bool "External RAM on FMC"
+ default n
+ depends on STM32F7_FMC
+ select ARCH_HAVE_HEAP2
+ ---help---
+ In addition to internal SDRAM, external RAM may be available through the FMC.
+
Review comment:
In our own board, which also uses F7, we also had to hack around `ARCH_HAVE_HEAP2` to get external RAM to work.
So I think there's some value to this change, if this were merged, we would switch to this option.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#529: Stm32F7: add external ram config
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#discussion_r390457260
##########
File path: arch/arm/src/stm32f7/Kconfig
##########
@@ -2136,6 +2136,14 @@ endchoice #"ULPI Selection"
endmenu # OTG_HS Config
+config STM32F7_EXTERNAL_RAM
+ bool "External RAM on FMC"
+ default n
+ depends on STM32F7_FMC
+ select ARCH_HAVE_HEAP2
+ ---help---
+ In addition to internal SDRAM, external RAM may be available through the FMC.
+
Review comment:
Where is this new configuration option used? It is not used in arch/arm/src/stm32f7 so it does not belong there.
Do you use it in your board? If so then it belongs in your boards/arm/stm32/<board>/Kconfig
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#529: Stm32F7: add external ram config
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#discussion_r391631787
##########
File path: arch/arm/src/stm32f7/Kconfig
##########
@@ -2136,6 +2136,14 @@ endchoice #"ULPI Selection"
endmenu # OTG_HS Config
+config STM32F7_EXTERNAL_RAM
+ bool "External RAM on FMC"
+ default n
+ depends on STM32F7_FMC
+ select ARCH_HAVE_HEAP2
+ ---help---
+ In addition to internal SDRAM, external RAM may be available through the FMC.
+
Review comment:
Please replace spaces with TABs to match usage in other Kconfig files.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #529: Stm32F7: add
external ram config
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#issuecomment-598212583
Thanks for the update. I am supposed to wait for all of the PR-triggered checks to complete, then I will (squash) merge your changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #529:
Stm32F7: add external ram config
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#issuecomment-598194555
Okay. I think it is generally bad usage of Kconfig variables. But there seems to be a precedence.
I did see an issue in the change: TAB characters are used for indentation in Kconfig files, not spaces. I will merge the change once the file is updated to replace the spaces with TABs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo merged pull request #529: Stm32F7: add
external ram config
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#529: Stm32F7: add external ram config
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #529: Stm32F7: add external ram config
URL: https://github.com/apache/incubator-nuttx/pull/529#discussion_r390457260
##########
File path: arch/arm/src/stm32f7/Kconfig
##########
@@ -2136,6 +2136,14 @@ endchoice #"ULPI Selection"
endmenu # OTG_HS Config
+config STM32F7_EXTERNAL_RAM
+ bool "External RAM on FMC"
+ default n
+ depends on STM32F7_FMC
+ select ARCH_HAVE_HEAP2
+ ---help---
+ In addition to internal SDRAM, external RAM may be available through the FMC.
+
Review comment:
Where is this new configuration option used? It is not used in arch/arm/src/stm32f7 so it does not belong there.
Do you use it in your board? If so then it belongs in your boards/arm/stm32/your-board/Kconfig and it should be named CONFIG_YOURBOARD_EXTERNAL_RAM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services