-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Add support for Raspberry Pi Pico 2 flash controller. #89182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add support for Raspberry Pi Pico 2 flash controller. #89182
Conversation
@hanan619 it looks like you've copied code directly from the Pico SDK. This is not permitted: the Pico SDK is under the BSD 3-Clause, and Zephyr is Apache 2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have some clarification about the provenance of the changes before reviewing further.
I’d like to point out that the existing RP2040 (Pico1) flash implementation in Zephyr also follows the Pico SDK and includes direct references to its source. My implementation for RP2350 (Pico2) continues this approach for consistency across Raspberry Pi platforms. |
ad66107
to
8cc9df8
Compare
This https://github.com/zephyrproject-rtos/zephyr/pull/89182/files#diff-5b63ca534e31287da5b9512b307ebf0a52703e12edb4bece40956c0c2bef0b48R2-R3 certainly is a little concerning: Raspberry Pi haven't offered up anything under Apache 2.0 . Re-labelling Raspberry Pi's BSD 3-Clause as Apache 2.0 is not permitted. I'll raise this concern in an issue, and edit this message with a link to it here.
I don't ascribe to a mindset of "everyone else is misbehaving, so I'll misbehave too." I wasn't a reviewer of the previous code, so my focus is brought to this here.
That list of conditions (and/or the reference to the code being released under BSD 3 Clause.
Thanks, I think it's best for some of the more experienced reviewers to comment on this. @henrikbrixandersen , I think you've contirbuted feedback on similar licencing terms, would you be able to comment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://docs.zephyrproject.org/latest/contribute/guidelines.html#licensing:
Importing code into the Zephyr OS from other projects that use a license other than the Apache 2.0 license needs to be fully understood in context and approved by the Zephyr governing board.
Can't your contribution just be (your) Zephyr glue code around code being pulled from the pico-sdk zephyr module?
Partial writes are not supported in pico-sdk. Therefore someone has added partial_write implementation in zephyr flash for rp2040. |
It seems that the check was missed in the original code, so we need to consider how to deal with it. Here, the request is to move the HAL-derived sources to https://github.com/zephyrproject-rtos/hal_rpi_pico rather than https://github.com/raspberrypi/pico-sdk so there is no problem with putting the zephyr-specific modification, and no required upstreaming. I think it is possible to move the HAL-derived sources and their modifications from here to https://github.com/zephyrproject-rtos/hal_rpi_pico/tree/zephyr/src/rp2_common/hardware_flash. |
8cc9df8
to
840defa
Compare
I have created a PR in hal_rpi_pico for the desired changes. |
The Raspberry Pi Pico 2 uses a QMI flash controller, which differs from the SSI controller used in the original Pico. Zephyr already has support for the SSI controller, but lacked support for QMI. This change adds the QMI flash controller implementation in the flash_rpi_pico.c driver file. Additionally, the RP2350 SoC devicetree file (rp2350.dtsi) has been updated to enable and describe the flash controller for Pico 2. Signed-off-by: Hanan Arshad <[email protected]>
840defa
to
58c17d3
Compare
…iables Cleaned up the flash_rpi_pico driver to improve code readability and compliance with Zephyr coding standards. Fixed inconsistent indentation across the file and removed variables that were declared but not used. A few improvements are credited to Manu3l0us. Signed-off-by: Hanan Arshad <[email protected]>
…_pico This commit links the hal_rpi_pico PR required for the flash driver. west.yml is updated Signed-off-by: Hanan Arshad <[email protected]>
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
I have updated the PR. |
|
Please assign someone to PR zephyrproject-rtos/hal_rpi_pico#11 |
The original Pico uses an SSI flash controller, which already has support
in Zephyr. The Pico 2 uses a different QMI flash controller. This PR adds
QMI controller support to Zephyr:
flash_rpi_pico.c
rp2350.dtsi