gh-raspberrypi/pico-sdk
Watch
1
0
Fork
You've already forked pico-sdk
 0
Code Issues 262 Pull requests 63 Releases Activity

Update docs, and add methods for new RP2350 DMA features #2588

Merged
kilograham merged 6 commits from dma_extras into develop 2025-07-25 09:14:59 -05:00
Conversation 31 Commits 6 Files changed 1 +254 -40
kilograham commented 2025-07-23 15:09:45 -05:00 (Migrated from github.com)
  • self-trigering/endless transfers
  • reversed/double-increment DMA

fixes #2059
fixes #2444

* self-trigering/endless transfers * reversed/double-increment DMA fixes #2059 fixes #2444
Wren6991 (Migrated from github.com) reviewed 2025-07-23 15:09:45 -05:00
will-v-pi (Migrated from github.com) reviewed 2025-07-23 15:09:45 -05:00
lurch (Migrated from github.com) reviewed 2025-07-23 17:51:58 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -144,3 +143,4 @@
typedef enum dma_channel_transfer_size {
DMA_SIZE_8 = 0, ///< Byte transfer (8 bits)
DMA_SIZE_16 = 1, ///< Half word transfer (16 bits)
DMA_SIZE_32 = 2 ///< Word transfer (32 bits)
lurch (Migrated from github.com) commented 2025-07-23 17:51:57 -05:00

Given that this function is named channel_config_set_write_address_update_type, was this supposed to say:

 * \param update_type The type of adjustment to make to the write address after each transfer.

?

Given that this function is named `channel_config_set_write_address_update_type`, was this supposed to say: ```suggestion * \param update_type The type of adjustment to make to the write address after each transfer. ``` ?
lurch (Migrated from github.com) reviewed 2025-07-23 17:52:42 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -144,3 +143,4 @@
typedef enum dma_channel_transfer_size {
DMA_SIZE_8 = 0, ///< Byte transfer (8 bits)
DMA_SIZE_16 = 1, ///< Half word transfer (16 bits)
DMA_SIZE_32 = 2 ///< Word transfer (32 bits)
lurch (Migrated from github.com) commented 2025-07-23 17:52:42 -05:00

The above line had a copy'n'paste error, so maybe this line is wrong too? (Perhaps it was supposed to say "...for memory to peripheral transfers")

The above line had a copy'n'paste error, so maybe this line is wrong too? (_Perhaps_ it was supposed to say "...for memory to peripheral transfers")
lurch (Migrated from github.com) reviewed 2025-07-23 18:07:53 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-23 18:07:53 -05:00

Would this flag a transfer_count of 0 as an invalid_param? The doxygen comment above implies that 0 is in the valid range? Also, I think that perhaps this should be using < rather than & (with the comparison-value obviously modified as appropriate), because otherwise a value above 2^28 that has some of the lower bits also set, would still count as "valid"? 🤔

Would this flag a `transfer_count` of 0 as an invalid_param? The doxygen comment above implies that 0 is in the valid range? Also, I _think_ that perhaps this should be using `<` rather than `&` (with the comparison-value obviously modified as appropriate), because otherwise a value above 2^28 that has some of the lower bits also set, would still count as "valid"? :thinking:
lurch (Migrated from github.com) reviewed 2025-07-23 18:10:20 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-23 18:10:19 -05:00

If the valid range for RP2040 is 0 -> 2^32 - 1, should this be

static inline uint32_t dma_encode_transfer_count(uint32_t transfer_count) {

?

If the valid range for RP2040 is 0 -> 2^32 - 1, should this be ```suggestion static inline uint32_t dma_encode_transfer_count(uint32_t transfer_count) { ``` ?
lurch (Migrated from github.com) reviewed 2025-07-23 18:11:08 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-23 18:11:08 -05:00 
 * On RP2040 self-triggering DMA is not supported, so this method must not be used
```suggestion * On RP2040 self-triggering DMA is not supported, so this method must not be used ```
lurch (Migrated from github.com) reviewed 2025-07-23 18:17:53 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-23 18:17:53 -05:00 
 * On RP2040 endless DMA transfers are not supported, so this method must not be used
```suggestion * On RP2040 endless DMA transfers are not supported, so this method must not be used ```
lurch (Migrated from github.com) reviewed 2025-07-23 18:23:52 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-23 18:23:51 -05:00

Perhaps it's worth naming this function dma_channel_set_encoded_transfer_count just to make it more obvious?

_Perhaps_ it's worth naming this function `dma_channel_set_encoded_transfer_count` just to make it more obvious?
lurch (Migrated from github.com) reviewed 2025-07-23 18:28:22 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-23 18:28:21 -05:00

It appears in multiple places, but I wonder if "non-zero values of the top 4 bits are used to" could be shortened to "the top 4 bits are used to", since you'd never indicate anything with a zero-value?

It appears in multiple places, but I wonder if "non-zero values of the top 4 bits are used to" could be shortened to "the top 4 bits are used to", since you'd never indicate anything with a zero-value?
lurch (Migrated from github.com) reviewed 2025-07-23 18:40:56 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -154,0 +184,4 @@
#else
#define DMA_ADDRESS_UPDATE_TYPE_TO_DMA_CH_CTRL_WRITE_BITS(u) \
((((u)&1) << DMA_CH0_CTRL_TRIG_INCR_WRITE_LSB) | \
(((u)&2) << (DMA_CH0_CTRL_TRIG_INCR_WRITE_REV_LSB - 1)))
lurch (Migrated from github.com) commented 2025-07-23 18:40:55 -05:00

Bit-twiddling isn't my strong point, but (DMA_CH0_CTRL_TRIG_INCR_READ_REV_LSB - 1) == DMA_CH0_CTRL_TRIG_INCR_READ_LSB, so does that mean that this is equivalent to:

#define DMA_ADDRESS_UPDATE_TYPE_TO_DMA_CH_CTRL_READ_BITS(u) (((u)&3) << DMA_CH0_CTRL_TRIG_INCR_READ_LSB)
Bit-twiddling isn't my strong point, but `(DMA_CH0_CTRL_TRIG_INCR_READ_REV_LSB - 1) == DMA_CH0_CTRL_TRIG_INCR_READ_LSB`, so does that mean that this is equivalent to: ```c #define DMA_ADDRESS_UPDATE_TYPE_TO_DMA_CH_CTRL_READ_BITS(u) (((u)&3) << DMA_CH0_CTRL_TRIG_INCR_READ_LSB) ```
Wren6991 (Migrated from github.com) reviewed 2025-07-23 18:50:27 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -154,0 +184,4 @@
#else
#define DMA_ADDRESS_UPDATE_TYPE_TO_DMA_CH_CTRL_WRITE_BITS(u) \
((((u)&1) << DMA_CH0_CTRL_TRIG_INCR_WRITE_LSB) | \
(((u)&2) << (DMA_CH0_CTRL_TRIG_INCR_WRITE_REV_LSB - 1)))
Wren6991 (Migrated from github.com) commented 2025-07-23 18:50:27 -05:00

Yeah, I deliberately put the bits next to each other so you could do that. Graham's code is better because it doesn't rely on that assumption. We usually expect these to be used with constants so there is no additional cost

Yeah, I deliberately put the bits next to each other so you could do that. Graham's code is better because it doesn't rely on that assumption. We usually expect these to be used with constants so there is no additional cost
Wren6991 (Migrated from github.com) reviewed 2025-07-23 19:05:30 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -144,3 +143,4 @@
typedef enum dma_channel_transfer_size {
DMA_SIZE_8 = 0, ///< Byte transfer (8 bits)
DMA_SIZE_16 = 1, ///< Half word transfer (16 bits)
DMA_SIZE_32 = 2 ///< Word transfer (32 bits)
Wren6991 (Migrated from github.com) commented 2025-07-23 18:51:10 -05:00

Do we usually put a typedef on enums?

Do we usually put a typedef on enums?
Wren6991 (Migrated from github.com) commented 2025-07-23 18:53:59 -05:00

yes this is also swapped, should say memory to peripheral

yes this is also swapped, should say `memory to peripheral`
Wren6991 (Migrated from github.com) commented 2025-07-23 18:55:06 -05:00

writee address -> write address

`writee address` -> `write address`
src/rp2_common/hardware_dma/include/hardware/dma.h
Wren6991 (Migrated from github.com) commented 2025-07-23 18:56:40 -05:00

That would be more explicit. We expect 32-bit ints on both Arm and RISC-V though.

That would be more explicit. We expect 32-bit ints on both Arm and RISC-V though.
src/rp2_common/hardware_dma/include/hardware/dma.h
Wren6991 (Migrated from github.com) commented 2025-07-23 19:01:00 -05:00

The test is correct. invalid_params_if is a negative assert, so it flags inputs which have any of the top 4 bits set. MODE_BITS is 0xf << 28

The test is correct. `invalid_params_if` is a negative assert, so it flags inputs which have any of the top 4 bits set. MODE_BITS is `0xf << 28`
lurch (Migrated from github.com) reviewed 2025-07-23 19:26:01 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-23 19:26:01 -05:00

Ahhh! I thought that it was positive-comparing against COUNT_BITS, I hadn't twigged that it was negative-comparing against MODE_BITS. In which case please ignore my earlier comment 😂

Ahhh! I thought that it was positive-comparing against COUNT_BITS, I hadn't twigged that it was negative-comparing against MODE_BITS. In which case please ignore my earlier comment :joy:
kilograham (Migrated from github.com) reviewed 2025-07-24 09:33:56 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
kilograham (Migrated from github.com) commented 2025-07-24 09:33:56 -05:00

yeah; i went with uint here vs uint32_t for the encoded, as the encoded is a register value whereas this is a "user value" - seemed like a distinction in my head, but all a bit hypothetical

yeah; i went with `uint` here vs `uint32_t` for the encoded, as the encoded is a register value whereas this is a "user value" - seemed like a distinction in my head, but all a bit hypothetical
kilograham (Migrated from github.com) reviewed 2025-07-24 09:34:41 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
kilograham (Migrated from github.com) commented 2025-07-24 09:34:41 -05:00

good suggestion, but i think i'll leave it

good suggestion, but i think i'll leave it
kilograham (Migrated from github.com) reviewed 2025-07-24 09:35:53 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
kilograham (Migrated from github.com) commented 2025-07-24 09:35:52 -05:00

what is the distinction here - FYI you can still use it, it doesn't work

what is the distinction here - FYI you can still use it, it doesn't work
kilograham (Migrated from github.com) reviewed 2025-07-24 09:37:32 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -154,0 +184,4 @@
#else
#define DMA_ADDRESS_UPDATE_TYPE_TO_DMA_CH_CTRL_WRITE_BITS(u) \
((((u)&1) << DMA_CH0_CTRL_TRIG_INCR_WRITE_LSB) | \
(((u)&2) << (DMA_CH0_CTRL_TRIG_INCR_WRITE_REV_LSB - 1)))
kilograham (Migrated from github.com) commented 2025-07-24 09:37:32 -05:00

yeah, i think i got squirly after getting bitten by the READ/WRITE being different distances apart on RP2040 vs RP2350 (i had a shift between them in there at first but it broke on RP2040)

yeah, i think i got squirly after getting bitten by the READ/WRITE being different distances apart on RP2040 vs RP2350 (i had a shift between them in there at first but it broke on RP2040)
lurch (Migrated from github.com) reviewed 2025-07-24 09:49:33 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-24 09:49:33 -05:00

Okay, perhaps "should" then? IMHO "should" feels more active (i.e. something the user does) whereas "may" feels more passive (i.e. something the SDK does) ? 🤷‍♂️

Okay, perhaps "should" then? IMHO "should" feels more active (i.e. something the user does) whereas "may" feels more passive (i.e. something the SDK does) ? :man_shrugging:
lurch (Migrated from github.com) reviewed 2025-07-24 09:56:28 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -162,3 +247,4 @@
channel_config_set_read_address_update_type(c, incr ? DMA_ADDRESS_UPDATE_INCREMENT : DMA_ADDRESS_UPDATE_NONE);
}
/*! \brief Set DMA channel write increment in a channel configuration object
lurch (Migrated from github.com) commented 2025-07-24 09:56:28 -05:00

You've changed writee address increments to address increments - did you mean to change it to write address increments ? (which would also be consistent with the channel_config_set_read_increment docstring that says read address increments rather than address increments)

You've changed `writee address increments` to ` address increments` - did you mean to change it to `write address increments` ? (which would also be consistent with the `channel_config_set_read_increment` docstring that says `read address increments` rather than ` address increments`)
kilograham (Migrated from github.com) reviewed 2025-07-24 10:00:01 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -144,3 +143,4 @@
typedef enum dma_channel_transfer_size {
DMA_SIZE_8 = 0, ///< Byte transfer (8 bits)
DMA_SIZE_16 = 1, ///< Half word transfer (16 bits)
DMA_SIZE_32 = 2 ///< Word transfer (32 bits)
kilograham (Migrated from github.com) commented 2025-07-24 10:00:01 -05:00

yes, i have added. I also made dma_channel_config_t the preferred name for dma_channel_config

note, there are a few straggler un-typedef-ed enums elsewhere outside of dma.h, but I'm leaving those for now, as i may do some more rationalization of where things are typedef-ed in future releases

yes, i have added. I also made `dma_channel_config_t` the preferred name for `dma_channel_config` note, there are a few straggler un-typedef-ed enums elsewhere outside of dma.h, but I'm leaving those for now, as i may do some more rationalization of where things are typedef-ed in future releases
lurch (Migrated from github.com) reviewed 2025-07-24 10:03:53 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -140,36 +140,128 @@ bool dma_channel_is_claimed(uint channel);
*
lurch (Migrated from github.com) commented 2025-07-24 10:03:53 -05:00

Small nitpick, but this is probably more readable as

* \param incr True to enable read address increments, whereby the read address increments by the transfer size with each transfer; false to perform each read from the same address.

or

* \param incr True to enable read address increments, whereby the read address increments by the transfer size with each transfer. False to perform each read from the same address.
Small nitpick, but this is probably more readable as ```suggestion * \param incr True to enable read address increments, whereby the read address increments by the transfer size with each transfer; false to perform each read from the same address. ``` or ```suggestion * \param incr True to enable read address increments, whereby the read address increments by the transfer size with each transfer. False to perform each read from the same address. ```
kilograham (Migrated from github.com) reviewed 2025-07-24 10:40:02 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -162,3 +247,4 @@
channel_config_set_read_address_update_type(c, incr ? DMA_ADDRESS_UPDATE_INCREMENT : DMA_ADDRESS_UPDATE_NONE);
}
/*! \brief Set DMA channel write increment in a channel configuration object
kilograham (Migrated from github.com) commented 2025-07-24 10:40:02 -05:00

i did not mean to can you suggest fix; easier flow for me

i did not mean to can you suggest fix; easier flow for me
lurch (Migrated from github.com) reviewed 2025-07-24 10:44:45 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -140,36 +140,128 @@ bool dma_channel_is_claimed(uint channel);
*
lurch (Migrated from github.com) commented 2025-07-24 10:44:44 -05:00 
 * \param incr True to enable write address increments, whereby the write address increments by the transfer size with each transfer, false to perform each write to the same address.
```suggestion * \param incr True to enable write address increments, whereby the write address increments by the transfer size with each transfer, false to perform each write to the same address. ```
lurch (Migrated from github.com) reviewed 2025-07-24 10:46:23 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
lurch (Migrated from github.com) commented 2025-07-24 10:46:23 -05:00

I'm going to "resolve" this as it's a duplicate of another suggestion (which has now evolved into a discussion).

I'm going to "resolve" this as it's a duplicate of another suggestion (which has now evolved into a discussion).
kilograham (Migrated from github.com) reviewed 2025-07-24 12:52:24 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
kilograham (Migrated from github.com) commented 2025-07-24 12:52:24 -05:00

i dont think i care enough to spend more time

i dont think i care enough to spend more time
kilograham (Migrated from github.com) reviewed 2025-07-24 12:53:43 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
kilograham (Migrated from github.com) commented 2025-07-24 12:53:42 -05:00

if you make "should" as a suggestion, i'll incorporate it tho

if you make "should" as a suggestion, i'll incorporate it tho
lurch (Migrated from github.com) reviewed 2025-07-24 14:43:22 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -140,36 +140,128 @@ bool dma_channel_is_claimed(uint channel);
*
lurch (Migrated from github.com) commented 2025-07-24 14:43:22 -05:00 
 * \param incr True to enable write address increments, whereby the write address increments by the transfer size with each transfer. False to perform each write to the same address.
```suggestion * \param incr True to enable write address increments, whereby the write address increments by the transfer size with each transfer. False to perform each write to the same address. ```
lurch (Migrated from github.com) reviewed 2025-07-24 14:44:36 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -419,21 +511,110 @@ static inline void dma_channel_set_write_addr(uint channel, volatile void *write
}
lurch (Migrated from github.com) commented 2025-07-24 14:44:35 -05:00 
 * On RP2040 self-triggering DMA is not supported, so this method should not be used
```suggestion * On RP2040 self-triggering DMA is not supported, so this method should not be used ```
lurch (Migrated from github.com) reviewed 2025-07-24 14:45:01 -05:00
src/rp2_common/hardware_dma/include/hardware/dma.h
@ -419,21 +511,110 @@ static inline void dma_channel_set_write_addr(uint channel, volatile void *write
}
lurch (Migrated from github.com) commented 2025-07-24 14:45:01 -05:00 
 * On RP2040 endless DMA transfers are not supported, so this method should not be used
```suggestion * On RP2040 endless DMA transfers are not supported, so this method should not be used ```
peterharperuk (Migrated from github.com) approved these changes 2025-07-25 04:38:56 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
Wren6991
will-v-pi
peterharperuk
Labels
Clear labels   
bazel

   
bluetooth

   
board settings

   
boot_stage2

   
bug
Something isn't working

  
build

   
c++

   
can't reproduce
Can not reproduce the bug

  
clang

   
clib-interface

   
cmsis

   
code cleanliness

   
code size

   
compiler version issues

   
cyw43_driver

   
debug

   
discussion

   
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
dynamic_clock

   
elf2uf2

   
enhancement
New feature or request

  
errata

   
filesystem

   
flash layout

   
floating point

   
FreeRTOS

   
hardware_adc

   
hardware_claim

   
hardware_clocks

   
hardware_dma

   
hardware_exception

   
hardware_flash

   
hardware_gpio

   
hardware_i2c

   
hardware_irq

   
hardware_pio

   
hardware_pwm

   
hardware_rcp

   
hardware_rtc

   
hardware_spi

   
hardware_sync

   
hardware_uart

   
hardware_vreg

   
hardware_watchdog

   
hardware_xosc

   
help wanted
Extra attention is needed

  
host

   
IAR

   
installation

   
invalid
This doesn't seem right

  
linker scripts

   
lto

   
lwip

   
mbedtls

   
missing API

   
needs reporter detail

   
networking

   
newlib

   
newlib-multicore

   
non-c

   
non-cmake

   
non-gcc

   
openocd

   
orphan PR

   
OS integration

   
pico_async_context

   
pico_base

   
pico_binary_info

   
pico_flash

   
picolibc

   
pico_lwip

   
pico_malloc

   
pico_multicore

   
pico_printf

   
pico_runtime

   
pico_stdio

   
pico_stdio_usb

   
pico_sync

   
pico_time

   
pico_util

   
pioasm

   
port
issues or discussions of porting code/libraries to rp2040

  
question
Further information is requested

  
review

   
review2

   
rtos interop
Consideration for FreeRTOS co-existence

  
runtime init

   
static library

   
svd

   
test

   
tinyusb

   
vscode

   
wifi integration

   
wontfix
This will not be worked on

No labels
bazel
bluetooth
board settings
boot_stage2
bug
build
c++
can't reproduce
clang
clib-interface
cmsis
code cleanliness
code size
compiler version issues
cyw43_driver
debug
discussion
documentation
duplicate
dynamic_clock
elf2uf2
enhancement
errata
filesystem
flash layout
floating point
FreeRTOS
hardware_adc
hardware_claim
hardware_clocks
hardware_dma
hardware_exception
hardware_flash
hardware_gpio
hardware_i2c
hardware_irq
hardware_pio
hardware_pwm
hardware_rcp
hardware_rtc
hardware_spi
hardware_sync
hardware_uart
hardware_vreg
hardware_watchdog
hardware_xosc
help wanted
host
IAR
installation
invalid
linker scripts
lto
lwip
mbedtls
missing API
needs reporter detail
networking
newlib
newlib-multicore
non-c
non-cmake
non-gcc
openocd
orphan PR
OS integration
pico_async_context
pico_base
pico_binary_info
pico_flash
picolibc
pico_lwip
pico_malloc
pico_multicore
pico_printf
pico_runtime
pico_stdio
pico_stdio_usb
pico_sync
pico_time
pico_util
pioasm
port
question
review
review2
rtos interop
runtime init
static library
svd
test
tinyusb
vscode
wifi integration
wontfix
Milestone
Clear milestone
No items
No milestone
2.2.0
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-raspberrypi/pico-sdk!2588
No description provided.