You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "raiden00pl (via GitHub)" <gi...@apache.org> on 2023/03/15 10:04:14 UTC
[GitHub] [nuttx] raiden00pl opened a new pull request, #8827: arch/nrf52: nvmc and flash should depends on ALLOW_BSD_COMPONENTS=y
raiden00pl opened a new pull request, #8827:
URL: https://github.com/apache/nuttx/pull/8827
## Summary
arch/nrf52: nvmc and flash should depends on ALLOW_BSD_COMPONENTS=y
## Impact
Users using nvmc and flash must allow BSD components (CONFIG_ALLOW_BSD_COMPONENTS=y).
## Testing
CI
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] raiden00pl commented on a diff in pull request #8827: arch/nrf52: nvmc and flash should depends on ALLOW_BSD_COMPONENTS=y
Posted by "raiden00pl (via GitHub)" <gi...@apache.org>.
raiden00pl commented on code in PR #8827:
URL: https://github.com/apache/nuttx/pull/8827#discussion_r1140411354
##########
arch/arm/src/nrf52/nrf52_nvmc.c:
##########
@@ -102,78 +102,6 @@ static inline void nrf_mem_barrier(void)
* Public Functions
****************************************************************************/
-/****************************************************************************
- * Name: nrf_nvmc_enable_icache
- *
- * Description:
- * Enable I-Cache for Flash
- *
- * Input Parameter:
- * flag - Flag to enable or disable.
- *
- * Returned Values:
- * None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_icache(bool flag)
-{
- uint32_t value;
-
- /* Read the current icache configuration */
-
- value = getreg32(NRF52_NVMC_ICACHECNF);
-
- if (flag)
- {
- value |= NVMC_ICACHECNF_CACHEEN;
- }
- else
- {
- value &= ~NVMC_ICACHECNF_CACHEEN;
- }
-
- /* Setup the new icache configuration */
-
- putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
-/****************************************************************************
- * Name: nrf_nvmc_enable_profile
- *
- * Description:
- * Enable profiling I-Cache for flash
- *
- * Input Parameter:
- * flag - Flag to enable or disable.
- *
- * Returned Values:
- * None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_profile(bool flag)
-{
- uint32_t value;
-
- /* Read the current icache configuration */
-
- value = getreg32(NRF52_NVMC_ICACHECNF);
-
- if (flag)
- {
- value |= NVMC_ICACHECNF_CACHEPROFEN;
- }
- else
- {
- value &= ~NVMC_ICACHECNF_CACHEPROFEN;
- }
-
- /* Setup the new icache configuration */
-
- putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
Review Comment:
That's right, these are trivial functions so I wrote them again. One day maybe I will re-write the rest of the flash/nvmc functions.
I don't know if it make sense to add another #ifdefs on the entire file since it is already made in Make.defs.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] raiden00pl commented on a diff in pull request #8827: arch/nrf52: nvmc and flash should depends on ALLOW_BSD_COMPONENTS=y
Posted by "raiden00pl (via GitHub)" <gi...@apache.org>.
raiden00pl commented on code in PR #8827:
URL: https://github.com/apache/nuttx/pull/8827#discussion_r1140962810
##########
arch/arm/src/nrf52/nrf52_nvmc.c:
##########
@@ -102,78 +102,6 @@ static inline void nrf_mem_barrier(void)
* Public Functions
****************************************************************************/
-/****************************************************************************
- * Name: nrf_nvmc_enable_icache
- *
- * Description:
- * Enable I-Cache for Flash
- *
- * Input Parameter:
- * flag - Flag to enable or disable.
- *
- * Returned Values:
- * None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_icache(bool flag)
-{
- uint32_t value;
-
- /* Read the current icache configuration */
-
- value = getreg32(NRF52_NVMC_ICACHECNF);
-
- if (flag)
- {
- value |= NVMC_ICACHECNF_CACHEEN;
- }
- else
- {
- value &= ~NVMC_ICACHECNF_CACHEEN;
- }
-
- /* Setup the new icache configuration */
-
- putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
-/****************************************************************************
- * Name: nrf_nvmc_enable_profile
- *
- * Description:
- * Enable profiling I-Cache for flash
- *
- * Input Parameter:
- * flag - Flag to enable or disable.
- *
- * Returned Values:
- * None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_profile(bool flag)
-{
- uint32_t value;
-
- /* Read the current icache configuration */
-
- value = getreg32(NRF52_NVMC_ICACHECNF);
-
- if (flag)
- {
- value |= NVMC_ICACHECNF_CACHEPROFEN;
- }
- else
- {
- value &= ~NVMC_ICACHECNF_CACHEPROFEN;
- }
-
- /* Setup the new icache configuration */
-
- putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
Review Comment:
Wrapping the entire file with '#ifdef' makes debugging complex preprocessor logic a nightmare (for example, missing '#endif'). Reporting an error without `#else` statement for the rest of the file is much better.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #8827: arch/nrf52: nvmc and flash should depends on ALLOW_BSD_COMPONENTS=y
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #8827:
URL: https://github.com/apache/nuttx/pull/8827
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] hartmannathan commented on a diff in pull request #8827: arch/nrf52: nvmc and flash should depends on ALLOW_BSD_COMPONENTS=y
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on code in PR #8827:
URL: https://github.com/apache/nuttx/pull/8827#discussion_r1140383310
##########
arch/arm/src/nrf52/nrf52_nvmc.c:
##########
@@ -102,78 +102,6 @@ static inline void nrf_mem_barrier(void)
* Public Functions
****************************************************************************/
-/****************************************************************************
- * Name: nrf_nvmc_enable_icache
- *
- * Description:
- * Enable I-Cache for Flash
- *
- * Input Parameter:
- * flag - Flag to enable or disable.
- *
- * Returned Values:
- * None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_icache(bool flag)
-{
- uint32_t value;
-
- /* Read the current icache configuration */
-
- value = getreg32(NRF52_NVMC_ICACHECNF);
-
- if (flag)
- {
- value |= NVMC_ICACHECNF_CACHEEN;
- }
- else
- {
- value &= ~NVMC_ICACHECNF_CACHEEN;
- }
-
- /* Setup the new icache configuration */
-
- putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
-/****************************************************************************
- * Name: nrf_nvmc_enable_profile
- *
- * Description:
- * Enable profiling I-Cache for flash
- *
- * Input Parameter:
- * flag - Flag to enable or disable.
- *
- * Returned Values:
- * None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_profile(bool flag)
-{
- uint32_t value;
-
- /* Read the current icache configuration */
-
- value = getreg32(NRF52_NVMC_ICACHECNF);
-
- if (flag)
- {
- value |= NVMC_ICACHECNF_CACHEPROFEN;
- }
- else
- {
- value &= ~NVMC_ICACHECNF_CACHEPROFEN;
- }
-
- /* Setup the new icache configuration */
-
- putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
Review Comment:
I assume `nrf_nvmc_enable_icache()` and `nrf_nvmc_enable_profile()` are being moved to another file because they are **NOT** part of the BSD-licensed code originating at Nordic?
Since this file has a BSD license, would it make sense to wrap the entire contents of this file with
```
#ifdef ALLOW_BSD_COMPONENTS
.
.
.
#endif /* ALLOW_BSD_COMPONENTS */
```
The same could be done for all non-Apache-2.0-Licensed files (but not in this PR... each one needs to be treated individually).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] hartmannathan commented on a diff in pull request #8827: arch/nrf52: nvmc and flash should depends on ALLOW_BSD_COMPONENTS=y
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on code in PR #8827:
URL: https://github.com/apache/nuttx/pull/8827#discussion_r1140426773
##########
arch/arm/src/nrf52/nrf52_nvmc.c:
##########
@@ -102,78 +102,6 @@ static inline void nrf_mem_barrier(void)
* Public Functions
****************************************************************************/
-/****************************************************************************
- * Name: nrf_nvmc_enable_icache
- *
- * Description:
- * Enable I-Cache for Flash
- *
- * Input Parameter:
- * flag - Flag to enable or disable.
- *
- * Returned Values:
- * None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_icache(bool flag)
-{
- uint32_t value;
-
- /* Read the current icache configuration */
-
- value = getreg32(NRF52_NVMC_ICACHECNF);
-
- if (flag)
- {
- value |= NVMC_ICACHECNF_CACHEEN;
- }
- else
- {
- value &= ~NVMC_ICACHECNF_CACHEEN;
- }
-
- /* Setup the new icache configuration */
-
- putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
-/****************************************************************************
- * Name: nrf_nvmc_enable_profile
- *
- * Description:
- * Enable profiling I-Cache for flash
- *
- * Input Parameter:
- * flag - Flag to enable or disable.
- *
- * Returned Values:
- * None
- *
- ****************************************************************************/
-
-void nrf_nvmc_enable_profile(bool flag)
-{
- uint32_t value;
-
- /* Read the current icache configuration */
-
- value = getreg32(NRF52_NVMC_ICACHECNF);
-
- if (flag)
- {
- value |= NVMC_ICACHECNF_CACHEPROFEN;
- }
- else
- {
- value &= ~NVMC_ICACHECNF_CACHEPROFEN;
- }
-
- /* Setup the new icache configuration */
-
- putreg32(value, NRF52_NVMC_ICACHECNF);
-}
-
Review Comment:
I'm not sure if it's best to handle this in Make.defs or to wrap the contents with ifdefs like I suggested.
Some pros and cons:
Suppose we have a mistake (like the reason for this PR) and a BSD component is included in the build and Kconfig does not have ALLOW_BSD_COMPONENTS.
Doing the ifdef in Make.defs will break the build with a Linker error. These are confusing because the error will say it can't find some symbol, but won't explain why.
Doing the ifdef in the source file keeps the BSD license check in the same place as the BSD licensed content and we can have:
```
#ifndef ALLOW_BSD_COMPONENTS
# error "This file requires Kconfig ALLOW_BSD_COMPONENTS"
#else
.
.
.
#endif /* ALLOW_BSD_COMPONENTS */
```
We can do that for all affected files (but not in this PR). I replied to the dev list just now with thoughts about how we should handle non-Apache-2.0 licenses throughout the codebase: https://lists.apache.org/thread/2tbw5jbvsbtgl0wjkbld9ync2vn1sz4b
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org