You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2020/10/16 15:00:51 UTC
[GitHub] [mynewt-core] nkaje opened a new pull request #2392: da1469x: Fix DMA Multiplexer register access
nkaje opened a new pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392
Configure DMA_REQ_MUX_REG within a critical section to avoid
race conditions. Use common macros provided by the DMA
driver to make changes to this register.
Signed-off-by: Naveen Kaje <na...@juul.com>
----------------------------------------------------------------
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
[GitHub] [mynewt-core] agross-korg commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
agross-korg commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r507932091
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
we should probably just acquire and keep the 6/7 channel pair. I don't see the need to acquire and free every time anyway. We have how many users of the dma channels? ADC, SDADC, and crypto? It appears the ADCs use whatever channel they get assigned from the acquire, which appears to go from channel 0 -> channel 7 in a search (by pairs).
----------------------------------------------------------------
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
[GitHub] [mynewt-core] benmccrea commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
benmccrea commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r508832743
##########
File path: hw/mcu/dialog/da1469x/src/da1469x_dma.c
##########
@@ -116,17 +122,19 @@ da1469x_dma_init(void)
struct da1469x_dma_regs *
da1469x_dma_acquire_single(int cidx)
{
- struct da1469x_dma_regs *chan;
+ struct da1469x_dma_regs *chan = NULL;
+ int sr;
assert(cidx < MCU_DMA_CHAN_MAX);
+ OS_ENTER_CRITICAL(sr);
if (cidx < 0) {
cidx = find_free_single();
if (cidx < 0) {
- return NULL;
+ goto end_single;
Review comment:
@nkaje I think you want to still return NULL for the two error cases, `if (cidx < 0)` and `else if (g_da1469x_dma_acquired & (1 << cidx)`
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-713048559
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
<details>
```diff
@@ -79,9 +79,9 @@
dma_regs->DMA_A_START_REG = (uint32_t)key;
dma_regs->DMA_B_START_REG = (uint32_t)&AES_HASH->CRYPTO_KEYS_START;
dma_regs->DMA_CTRL_REG = DMA_DMA7_CTRL_REG_AINC_Msk |
- DMA_DMA7_CTRL_REG_BINC_Msk |
- (MCU_DMA_BUS_WIDTH_4B << DMA_DMA7_CTRL_REG_BW_Pos) |
- DMA_DMA7_CTRL_REG_DMA_ON_Msk;
+ DMA_DMA7_CTRL_REG_BINC_Msk |
+ (MCU_DMA_BUS_WIDTH_4B << DMA_DMA7_CTRL_REG_BW_Pos) |
+ DMA_DMA7_CTRL_REG_DMA_ON_Msk;
while (dma_regs->DMA_IDX_REG != keylen);
da1469x_dma_release_channel(dma_regs);
```
</details>
#### hw/mcu/dialog/da1469x/src/da1469x_dma.c
<details>
```diff
@@ -39,12 +39,12 @@
*/
#define MCU_DMA_GET_MUX(_cidx) \
- ((DMA->DMA_REQ_MUX_REG >> DA1469X_DMA_MUX_SHIFT(_cidx)) & 0xf)
+ ((DMA->DMA_REQ_MUX_REG >> DA1469X_DMA_MUX_SHIFT(_cidx)) & 0xf)
#define MCU_DMA_SET_MUX(_cidx, _periph) \
- DMA->DMA_REQ_MUX_REG = \
+ DMA->DMA_REQ_MUX_REG = \
((DMA->DMA_REQ_MUX_REG & ~(0xf << DA1469X_DMA_MUX_SHIFT(_cidx))) \
- | ((_periph) & 0xf) << DA1469X_DMA_MUX_SHIFT(_cidx)); \
+ | ((_periph) & 0xf) << DA1469X_DMA_MUX_SHIFT(_cidx)); \
struct da1469x_dma_interrupt_cfg {
```
</details>
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-710100847
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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
[GitHub] [mynewt-core] benmccrea commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
benmccrea commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r506617502
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
@nkaje I don't believe this will work properly. I think the MCU_DMA_SET_MUX macro expects to work on channel-pair requests, where the lower channel index of the pair is passed as cidx.
DMA_REQ_MUX_REG is a 16-bit register. If you pass cidx=7 to this macro, the resulting bit mask and bit set values will be shifted beyond bit 15 in the register:
Example: cidx=7, periph=MCU_DMA_PERIPH_NONE=0xf
(_periph) << ((_cidx) * 2) --> left-shift 0xf by 14 bits = b111100000000000000 = 0x3c000
I think this macro was intended to be used privately in da1469x_dma.c by `da1469x_dma_acquire_single()` and `da1469x_dma_acquire_periph()`.
Take a look at this code in `da1469x_dma_acquire_single()`:
```
/*
* DMA_REQ_MUX_REG applies only to channels < 8
*/
if (cidx < 8) {
MCU_DMA_SET_MUX(cidx, MCU_DMA_PERIPH_NONE);
}
```
Note that this code will not work properly for odd-numbered channels. Even though it is a single channel request, the lower channel # of a channel pair must be passed for it to work properly.
I think we need to do both of the following to really fix this issue:
1. Fix `da1469x_dma_acquire_single()` so it properly works for both even and odd single-channel requests.
2. Use `da1469x_dma_acquire_single()` and `da1469x_dma_release_channel()` in the crypto code for channel 7, so that the DMA driver can function properly when channel 7 is in use.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] andrzej-kaczmarek commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r508696677
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
@agross-korg so what do you propose instead? should we restore configuration for everything that was wiped out during sleep on every wakeup? wouldn't it be wasteful to e.g. configure i2c on every wakeup even though we only use it at times?
----------------------------------------------------------------
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
[GitHub] [mynewt-core] benmccrea commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
benmccrea commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r507185153
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
Thinking further, `DMA_REQ_MUX_REG` applies to channel pairs, so forcing `cidx` to be even in the macro would make it work for single and pair cases.
I do think the macro should remain local and `da1469x_dma_acquire_xxx()` and `da1469x_dma_release_channel()` should be used instead of accessing `DMA_REQ_MUX_REG` directly (e.g. crypto driver should use these calls for channel 7 with the macro fixed).
----------------------------------------------------------------
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
[GitHub] [mynewt-core] andrzej-kaczmarek commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r507747562
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
> I do think the macro should remain local and `da1469x_dma_acquire_xxx()` and `da1469x_dma_release_channel()` should be used instead of accessing `DMA_REQ_MUX_REG` directly (e.g. crypto driver should use these calls for channel 7 with the macro fixed).
agree, acquire/release should be used since it also keeps track of used channels internally. if some pieces of code manually modify mux register, things may not works as expected... so if there's something that acquire/release does not do it should be fixed there
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-713106657
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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
[GitHub] [mynewt-core] benmccrea commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
benmccrea commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r508880669
##########
File path: hw/mcu/dialog/da1469x/src/da1469x_dma.c
##########
@@ -116,17 +122,19 @@ da1469x_dma_init(void)
struct da1469x_dma_regs *
da1469x_dma_acquire_single(int cidx)
{
- struct da1469x_dma_regs *chan;
+ struct da1469x_dma_regs *chan = NULL;
+ int sr;
assert(cidx < MCU_DMA_CHAN_MAX);
+ OS_ENTER_CRITICAL(sr);
if (cidx < 0) {
cidx = find_free_single();
if (cidx < 0) {
- return NULL;
+ goto end_single;
Review comment:
@nkaje Ahh.. I missed it. Thanks, it looks good.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] benmccrea commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
benmccrea commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r507182931
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
@agross-korg
It might be better to have separate macros for single-channel and channel-pair (peripheral) use, where `da1469x_dma_acquire_single()` would use the single-channel version and `da1469x_dma_acquire_periph()` would use the channel-pair version.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] nkaje commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
nkaje commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r508839546
##########
File path: hw/mcu/dialog/da1469x/src/da1469x_dma.c
##########
@@ -116,17 +122,19 @@ da1469x_dma_init(void)
struct da1469x_dma_regs *
da1469x_dma_acquire_single(int cidx)
{
- struct da1469x_dma_regs *chan;
+ struct da1469x_dma_regs *chan = NULL;
+ int sr;
assert(cidx < MCU_DMA_CHAN_MAX);
+ OS_ENTER_CRITICAL(sr);
if (cidx < 0) {
cidx = find_free_single();
if (cidx < 0) {
- return NULL;
+ goto end_single;
Review comment:
`chan` is initialized to NULL, which gets set by `MCU_DMA_CIDX2CHAN(cidx)` down below. If we end up taking the `goto` path to `end_single`, `chan` set to `NULL` is returned.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] benmccrea commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
benmccrea commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r507185153
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
Thinking further, `DMA_REQ_MUX_REG` applies to channel pairs, so forcing `cidx` to be even in the macro would make it work for single and pair cases.
I do think the macro should remain local and `da1469x_dma_acquire_xxx()`/`da1469x_dma_release_channel()` should be used instead of accessing `DMA_REQ_MUX_REG` directly (e.g. crypto driver should use these calls for channel 7 with the macro fixed).
----------------------------------------------------------------
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
[GitHub] [mynewt-core] agross-korg commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
agross-korg commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r506711850
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
the macro as it is today is wrong then, so that needs fixing. I'd levy the additional requirement of bogging down pairs of channels due to the inability to use one channel for one thing and the other for something else, unless of course this is memory to memory (which is what the secure is doing).
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-713612757
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-713663020
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-713612757
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-713048559
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
<details>
```diff
@@ -79,9 +79,9 @@
dma_regs->DMA_A_START_REG = (uint32_t)key;
dma_regs->DMA_B_START_REG = (uint32_t)&AES_HASH->CRYPTO_KEYS_START;
dma_regs->DMA_CTRL_REG = DMA_DMA7_CTRL_REG_AINC_Msk |
- DMA_DMA7_CTRL_REG_BINC_Msk |
- (MCU_DMA_BUS_WIDTH_4B << DMA_DMA7_CTRL_REG_BW_Pos) |
- DMA_DMA7_CTRL_REG_DMA_ON_Msk;
+ DMA_DMA7_CTRL_REG_BINC_Msk |
+ (MCU_DMA_BUS_WIDTH_4B << DMA_DMA7_CTRL_REG_BW_Pos) |
+ DMA_DMA7_CTRL_REG_DMA_ON_Msk;
while (dma_regs->DMA_IDX_REG != keylen);
da1469x_dma_release_channel(dma_regs);
```
</details>
#### hw/mcu/dialog/da1469x/src/da1469x_dma.c
<details>
```diff
@@ -39,12 +39,12 @@
*/
#define MCU_DMA_GET_MUX(_cidx) \
- ((DMA->DMA_REQ_MUX_REG >> DA1469X_DMA_MUX_SHIFT(_cidx)) & 0xf)
+ ((DMA->DMA_REQ_MUX_REG >> DA1469X_DMA_MUX_SHIFT(_cidx)) & 0xf)
#define MCU_DMA_SET_MUX(_cidx, _periph) \
- DMA->DMA_REQ_MUX_REG = \
+ DMA->DMA_REQ_MUX_REG = \
((DMA->DMA_REQ_MUX_REG & ~(0xf << DA1469X_DMA_MUX_SHIFT(_cidx))) \
- | ((_periph) & 0xf) << DA1469X_DMA_MUX_SHIFT(_cidx)); \
+ | ((_periph) & 0xf) << DA1469X_DMA_MUX_SHIFT(_cidx)); \
struct da1469x_dma_interrupt_cfg {
```
</details>
----------------------------------------------------------------
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
[GitHub] [mynewt-core] benmccrea commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
benmccrea commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r507182931
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
@agross-korg I agree, the macro needs fixed. It might be better to have separate macros for single-channel and channel-pair (peripheral) use, where `da1469x_dma_acquire_single()` would use the single-channel version and `da1469x_dma_acquire_periph()` would use the channel-pair version.
BTW - the driver does support channels 8-15 by setting MCU_DMA_CHAN_MAX > 8, but only channels 0-7 are defined (in hardware) by `DMA_REQ_MUX_REG`.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] agross-korg commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
agross-korg commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r506712358
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
never mind ignoring the fact that we only have 8 channels, we dont have protections on the peripheral being 0-15, or the channel even being valid. What happens if someone asks for 9.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot commented on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-710100847
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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
[GitHub] [mynewt-core] nkaje commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
nkaje commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r509389037
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -64,26 +64,27 @@ da1469x_has_support(struct crypto_dev *crypto, uint8_t op, uint16_t algo,
static void
do_dma_key_tx(const void *key, uint16_t keylen)
{
- DMA_Type *dma_regs = DMA;
+ struct da1469x_dma_regs *dma_regs;
/* DMA len: keylen in bits, bus width is 4 */
keylen >>= 5;
/* enable OTP clock and set in read mode */
da1469x_clock_amba_enable(CRG_TOP_CLK_AMBA_REG_OTP_ENABLE_Msk);
da1469x_otp_set_mode(OTPC_MODE_READ);
- /* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
- dma_regs->DMA7_LEN_REG = keylen;
+ /* securely DMA hardware key using channel #7 from secret storage to QSPI decrypt engine */
+ dma_regs = da1469x_dma_acquire_single(7);
Review comment:
addressed.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] nkaje commented on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
nkaje commented on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-713601533
Rebased and tested on top of today's master:HEAD https://github.com/apache/mynewt-core/commit/50b2ca98149b9ed316b15bc97c99286758f70fdd
----------------------------------------------------------------
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
[GitHub] [mynewt-core] agross-korg commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
agross-korg commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r509360840
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -64,26 +64,27 @@ da1469x_has_support(struct crypto_dev *crypto, uint8_t op, uint16_t algo,
static void
do_dma_key_tx(const void *key, uint16_t keylen)
{
- DMA_Type *dma_regs = DMA;
+ struct da1469x_dma_regs *dma_regs;
/* DMA len: keylen in bits, bus width is 4 */
keylen >>= 5;
/* enable OTP clock and set in read mode */
da1469x_clock_amba_enable(CRG_TOP_CLK_AMBA_REG_OTP_ENABLE_Msk);
da1469x_otp_set_mode(OTPC_MODE_READ);
- /* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
- dma_regs->DMA7_LEN_REG = keylen;
+ /* securely DMA hardware key using channel #7 from secret storage to QSPI decrypt engine */
+ dma_regs = da1469x_dma_acquire_single(7);
Review comment:
acquire single can return NULL.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] apache-mynewt-bot removed a comment on pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#issuecomment-713106657
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
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
[GitHub] [mynewt-core] benmccrea commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
benmccrea commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r506617502
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
@nkaje I don't believe this will work properly. I think the MCU_DMA_SET_MUX macro expects to work on channel-pair requests, where the lower channel index of the pair is passed as cidx.
DMA_REQ_MUX_REG is a 16-bit register. If you pass cidx=7 to this macro, the resulting bit mask and bit set values will be shifted beyond bit 15 in the register:
Example: cidx=7, periph=MCU_DMA_PERIPH_NONE=0xf
(_periph) << ((_cidx) * 2) --> left-shift 0xf by 14 bits = b111100000000000000 = 0x3c000
I think the `MCU_DMA_SET_MUX` macro was intended to be used privately in da1469x_dma.c by `da1469x_dma_acquire_single()` and `da1469x_dma_acquire_periph()`.
Take a look at this code in `da1469x_dma_acquire_single()`:
```
/*
* DMA_REQ_MUX_REG applies only to channels < 8
*/
if (cidx < 8) {
MCU_DMA_SET_MUX(cidx, MCU_DMA_PERIPH_NONE);
}
```
Note that this code will not work properly for odd-numbered channels. Even though it is a single channel request, the lower channel # of a channel pair must be passed for it to work properly.
I think we need to do both of the following to really fix this issue:
1. Fix `da1469x_dma_acquire_single()` so it properly works for both even and odd single-channel requests.
2. Use `da1469x_dma_acquire_single()` and `da1469x_dma_release_channel()` in the crypto code for channel 7, so that the DMA driver can function properly when channel 7 is in use.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] agross-korg commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
agross-korg commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r508747207
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
I think we should merge this change which fixes a known, serious issue. Then we decide what to do for the other, which is an annoying problem that impacts battery life.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] andrzej-kaczmarek commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r508342638
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
we should not acquire once since DMA controller resides in PD_SYS which means you need to keep PD_SYS always enabled so M33 is not powered off during deep sleep. and this brings us to two issues here:
- DMA should keep PD_SYS up once at least channel is acquired (https://github.com/apache/mynewt-core/pull/2394) as otherwise all DMA configuration is gone after deep sleep
- crpyto driver should also acquire PD_SYS since it resited in the same power domain (similar to what e.g. GPADC driver does)
all that also means existing code should always use acquire/release helper to reserve DMA channel and if we want to have PD_SYS disabled in deep sleep, any code that uses DMA and/or crypto driver would need to open/close/acquire/release/configure/whatever channel and/or dev only when needed.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] agross-korg commented on a change in pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
agross-korg commented on a change in pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392#discussion_r508589109
##########
File path: hw/drivers/crypto/crypto_da1469x/src/crypto_da1469x.c
##########
@@ -73,7 +74,10 @@ do_dma_key_tx(const void *key, uint16_t keylen)
da1469x_otp_set_mode(OTPC_MODE_READ);
/* securely DMA hardware key from secret storage to QSPI decrypt engine */
- dma_regs->DMA_REQ_MUX_REG |= 0xf000;
+ OS_ENTER_CRITICAL(sr);
+ /* Use the secure channel #7 */
+ MCU_DMA_SET_MUX(7, MCU_DMA_PERIPH_NONE);
Review comment:
The acquire or release shouldn't have anything to do with power management. It's silly to be acquiring and releasing the same channels for the same stuff all the time. It's unnecessary and wasteful and means we'll be leaving on power domains that we could have been turning off if the power management stuff would wait for devices to quiesce.
----------------------------------------------------------------
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
[GitHub] [mynewt-core] nkaje merged pull request #2392: da1469x: Fix DMA Multiplexer register access
Posted by GitBox <gi...@apache.org>.
nkaje merged pull request #2392:
URL: https://github.com/apache/mynewt-core/pull/2392
----------------------------------------------------------------
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