Skip to content

Conversation

@soburi
Copy link
Member

@soburi soburi commented Jan 18, 2023

Adding RaspberryPi Pico DMA driver.

Signed-off-by: TOKITA Hiroshi [email protected]


The SPI client PR is #54195.
It pass the tests/spi/spi_loopback test.
And work fine samples/subsys/fs/fat_fs sample with my configuration.

@soburi soburi added the platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) label Jan 18, 2023
@teburd teburd self-assigned this Jan 18, 2023
@teburd teburd added the area: DMA Direct Memory Access label Jan 18, 2023
@soburi soburi force-pushed the pico_dma branch 8 times, most recently from ef700a0 to 1fd9ffc Compare January 21, 2023 09:06
@soburi soburi force-pushed the pico_dma branch 10 times, most recently from bb96b0b to ad1dfa7 Compare January 28, 2023 08:49
@soburi soburi marked this pull request as ready for review January 28, 2023 10:32
@soburi soburi force-pushed the pico_dma branch 2 times, most recently from 9bc5bc1 to af56e30 Compare March 5, 2023 06:54
@soburi soburi requested a review from teburd March 5, 2023 08:43
Copy link
Contributor

@yonsch yonsch left a comment

Choose a reason for hiding this comment

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

Thanks, a few comments/questions

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these functions/file needed? And why is the extern needed?

Copy link
Member Author

@soburi soburi Mar 9, 2023

Choose a reason for hiding this comment

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

It defines rpi_pico_dma specific APIs.
This is not fit in zephyr's common API features.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a channel_priority field in struct dma_config which could almost certainly be used for this

Copy link
Member Author

@soburi soburi Mar 18, 2023

Choose a reason for hiding this comment

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

Thank you for reviewing, and sorry for my late response.

I recently updated the rpi_pico's HAL module. This includes the DMA priority API.
I updated this PR to call that API with refer to the channel_priority value.
The rpi_pico specific function is no needed.
I removed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

what is ch_cfg in this case? If we need to expand the dma_config struct to cover this, that seems more sensible that adding rpi specific functions if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it is not necessary for basic operation, I deleted it.
For functions that may be able to common (For example, rpi_pico has a function to change the order of 2 bytes and 4 bytes), I will consider to create as a separately PR if necessary.

@yonsch
Copy link
Contributor

yonsch commented Mar 10, 2023

Pico parts LGTM. I still think it's better to extend the DMA API rather than adding a pico specific API, as much as it's possible. But that I'll leave to @teburd to decide

@soburi soburi force-pushed the pico_dma branch 2 times, most recently from 9a263fb to 5a835fc Compare March 12, 2023 09:52
yonsch and others added 2 commits March 15, 2023 07:08
Some pico-sdk drivers call a panic function, originally implemented
as part of the Pico's C runtime. This commit adds a Zephyr compatible
implementation of panic, so that those drivers could be compiled with
Zephyr.

Signed-off-by: Yonatan Schachter <[email protected]>
Enable DMA driver.
Add the path of the DMA driver header into include paths.
`hardware_claim` is depends by DMA driver, also enable it.

Signed-off-by: TOKITA Hiroshi <[email protected]>
@soburi soburi force-pushed the pico_dma branch 2 times, most recently from cc4a437 to f975dff Compare March 14, 2023 23:42
@yonsch
Copy link
Contributor

yonsch commented Mar 15, 2023

@soburi please rebase so that the CI will test this with 1.5.0
Oops, you already did

Copy link
Contributor

@teburd teburd left a comment

Choose a reason for hiding this comment

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

Many LOG_DBG's should be LOG_ERR's

The driver, binding, and the rest look ok to me

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a possible runtime error condition, LOG_ERR.

If this is a programming error __ASSERT()

Copy link
Member Author

Choose a reason for hiding this comment

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

The function caller can give the NULL as filter_pattern args.
It is need to handle as the runtime error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOG_ERR

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOG_ERR

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOG_ERR

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOG_ERR

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor

Choose a reason for hiding this comment

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

All LOG_DBG's here should be LOG_ERR

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto.

soburi added 2 commits March 18, 2023 13:58
Adding RaspberryPi Pico DMA driver.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add overlay file for rpi_pico target.

Signed-off-by: TOKITA Hiroshi <[email protected]>
@carlescufi carlescufi merged commit 774affe into zephyrproject-rtos:main Mar 22, 2023
@soburi soburi deleted the pico_dma branch March 22, 2023 10:10
soburi added a commit to soburi/zephyr that referenced this pull request Aug 2, 2023
Add myself as codeowner of previously committed driver.

- zephyrproject-rtos#53892

Signed-off-by: TOKITA Hiroshi <[email protected]>
fabiobaltieri pushed a commit that referenced this pull request Aug 3, 2023
Add myself as codeowner of previously committed driver.

- #53892

Signed-off-by: TOKITA Hiroshi <[email protected]>
kunoh pushed a commit to kunoh/zephyr that referenced this pull request Aug 7, 2023
Add myself as codeowner of previously committed driver.

- zephyrproject-rtos#53892

Signed-off-by: TOKITA Hiroshi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Devicetree Binding PR modifies or adds a Device Tree binding area: DMA Direct Memory Access manifest-hal_rpi_pico platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants