Skip to content
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

Initial support for Pico 2 (squashed version of PR 77368) #83343

Merged
merged 19 commits into from
Dec 23, 2024

Conversation

kartben
Copy link
Collaborator

@kartben kartben commented Dec 23, 2024

In the interest of getting the work from PR #77368 finally merged into upstream main, this PR proposes the exact same change set with the only difference being:

The following commits have been squashed together. As per our contribution guidelines, sign-off entry of the original author has been kept intact, and I've added mine as well:

SoC:

  • Relevant part of "rp2350: Define and implement a cpucluster of Cortex-M33s" squashed into:
  • soc: rp2350: Add initial support for the Raspberry Pi RP2350

Board:

  • Relevant part of "rp2350: Define and implement a cpucluster of Cortex-M33s" and
  • "doc: rpi_pico2: Document Raspberry Pi Pico 2 and related changes" squashed into:
  • "boards: Add initial support for the Raspberry Pi Pico 2"

Add support for SoC-specific clock ids and update the initialization
function to support the existing RP2040 and add support for the RP2350.

clock_control_rpi_pico.c uses numerical values for clock ids taken from
rpi_pico_clock.h which are the "clock generator". For the RP2350 these
values are different for some of the same logical clock sources, as well
as the RP2040 and RP2350 having different clock sources available.

Signed-off-by: Andrew Featherstone <[email protected]>
kartben and others added 18 commits December 23, 2024 22:55
RP2350 is Raspberry Pi's newest SoC. From the datasheet:

"RP2350 is a new family of microcontrollers from Raspberry Pi that
offers significant enhancements over RP2040. Key features include:
• Dual Cortex-M33 or Hazard3 processors at 150 MHz
• 520 kB on-chip SRAM, in 10 independent banks
• 8 kB of one-time-programmable storage (OTP)
• Up to 16 MB of external QSPI flash/PSRAM via dedicated QSPI bus
...
"

This commit introduces some changes to support the existing RP2040 and
what is describe by Raspberry Pi as the "RP2350 family". Currently there
are 4 published products in the family: RP2350A, RP2350B, RP2354A, and
RP2354A. Within Zephyr's taxonomy, split the configuration as follows:
Family: Raspberry Pi Pico. This contains all RP2XXX SoCs,
SoC Series: RP2040 and RP2350.
SoC: RP2040 and, for now, just the RP2350A, which is present on the Pico
2, where the A suffix indicates  QFN-60 package type. This structure is
reflected in `soc/raspberrypi/soc.yml`, and somewhat assumes that there
won't be a RP2050, for example, as a RP2040 with more RAM.

This is foundation work ahead of introducing support for Raspberry Pi's
Pico 2 board, which is fitted with a RP2350A and 4MB of flash.

Signed-off-by: Andrew Featherstone <[email protected]>
Signed-off-by: Benjamin Cabé <[email protected]>
Extend the existing driver to add some initial support for the new SoC,
whilst maintaining compatibility with the RP2040.

Signed-off-by: Andrew Featherstone <[email protected]>
Unlike the RP2040, the RP2350 has multiple tick generators that need to
be started. Start TIMER0 and TIMER1 tick generators during
clock_control_init.

Signed-off-by: Andrew Featherstone <[email protected]>
The watchdog register configuration of RP2350 differs from that
of RP2040, so we make fit that.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Signed-off-by: Andrew Featherstone <[email protected]>
The RP2350 SoC series contain two timer peripherals. Extend the driver
to support using the second timer (`TIMER1`).

N.b. this requires a fix from the Pico SDK to be patched into
hal_rpi_pico. See raspberrypi/pico-sdk#1949 .

Signed-off-by: Andrew Featherstone <[email protected]>
A significant amount of the pin muxing is duplicated between the RP2040,
the RP2350A, and RP2350B. Reflect this in the file structure, with a
`-common` suffix used to to indicate this.

Macros are defined in ascending order of the function index in the
relevant table in the datasheet. SoC/SoC-series specific macros are
defined in their respective tables. Functions that are not currently
used (e.g. the new HSTX) are intentionally not defined here as they do
not (currently) have any use in the Zephyr tree (i.e. there's no drivers
that make use of this functionality).

clang-format has been run over the existing definitions to reduce the
noise generated by CI. These are cosmetic changes; I've tried to retain
attribution to the relevant authors where applicable.

Signed-off-by: Andrew Featherstone <[email protected]>
On RP2350, the alt function value can be up to 0x1F, so store as 5 bits.

Signed-off-by: Peter Johanson <[email protected]>
Signed-off-by: Andrew Featherstone <[email protected]>
The Raspberry Pi Pico 2 is Raspberry Pi's first board fitted with their
RP2350A SoC.

This adds a minimal board definition, sufficient to build and run
`samples/hello_world` and `samples/basic/blinky` on the board. Images
can be run on the target using OpenOCD. Raspberry Pi's `picotool` can
create a UF2 binary, which ensures that errata RP2350-E10 is avoided
e.g.

```
> picotool uf2 convert build\rpi_pico2\hello_world\zephyr\zephyr.elf \
    build\rpi_pico2\hello_world\zephyr\zephyr.uf2 \
    --family rp2350-arm-s --abs-block`
```

Raspberry Pi Pico 2 is a low-cost, high-performance microcontroller
board with flexible digital interfaces. Key features include:

- RP2350A microcontroller chip designed by Raspberry Pi in the United
  Kingdom
- Dual Cortex-M33 or Hazard3 processors at up to 150MHz
- 520KB of SRAM, and 4MB of on-board flash memory
- USB 1.1 with device and host support
- Low-power sleep and dormant modes
- Drag-and-drop programming using mass storage over USB
- 26x multi-function GPIO pins including 3 that can be used for ADC
- 2x SPI, 2x I2C, 2x UART, 3x 12-bit 500ksps Analogue to Digital
  Converter (ADC), 24x controllable PWM channels
- 2x Timer with 4 alarms, 1x AON Timer
- Temperature sensor
- 3x Programmable IO (PIO) blocks, 12 state machines total for custom
  peripheral support
    - Flexible, user-programmable high-speed IO
    - Can emulate interfaces such as SD Card and VGA

The Raspberry Pi Pico 2 comes as a castellated module which allows
soldering direct to carrier boards.

Only enable timer 0 for now. Timer 1 won't work correctly until the
rpi_pico HAL has picked up the fix for `hardware_alarm_irq_handler`. See
raspberrypi/pico-sdk#1949 .

Added some documentation for the board itself (mostly aiming to refer to
canonical sources of information rather duplicate). Add entries in the
release notes where applicable.

boards/raspberrypi/rpi_pico2/doc/img/rpi_pico2.webp is a cropped and
compressed version of https://www.raspberrypi.com/documentation/microcontrollers/images/pico-2.png
which is released under the CC-BY-SA-4.0 license. See https://github.com/raspberrypi/documentation/blob/develop/LICENSE.md

Signed-off-by: Andrew Featherstone <[email protected]>
Signed-off-by: Benjamin Cabé <[email protected]>
Add UF2 Family ID for Raspberry Pi 2350 and build
UF2 image by default for Pico 2 board

Signed-off-by: Ryan Grachek <[email protected]>
Signed-off-by: Andrew Featherstone <[email protected]>
The Raspberry Pi Pico 2's device is compatible with the existing Pico 1.
The build system requires a `<board>.overlay` file, but these use the
pre-processing to #include the sibling rpi_pico.overlay files rather
than duplicating the contents as an attempt to keep things DRY.

Tested locally.

Signed-off-by: Andrew Featherstone <[email protected]>
For these tests' needs, the RP2350 on the Pico 2 is compatible with the
RP2040 on the Pico 1. #include the latter's overlay in preference to
duplicating the content.

Signed-off-by: Andrew Featherstone <[email protected]>
Add OpenOCD debugger support.
For now we will need Raspberry Pi'a forked version of OpenOCD from
https://github.com/raspberrypi/openocd .

The default adapter speed is set to match Raspberry Pi's documentation.

Signed-off-by: Andrew Featherstone <[email protected]>
Extend gpio_api_1pin so that tests can require a test fixture to provide
an external pulldown resistor to the board under test. Use the new
test-gpio-external-pulldown device tree binding to define where that
GPIO is, and, finally, add a device tree overlay for the Raspberry Pi
Pico 2 board that defines where the pulldown provided by the fixture
will be.

Tested locally using `--fixture gpio_external_pull_down` when running
Twister on the command line, or by creating and using a Hardware Map
file, in combination with a modified Pico 2.

Signed-off-by: Andrew Featherstone <[email protected]>
Add initial support for the RP2350's PIO peripherals, extend the
existing example under samples/boards/raspberrypi/rpi_pico/uart_pio to
demonstrate this on the Raspberry Pi Pico 2, and update the board's
documentation.

Signed-off-by: Andrew Featherstone <[email protected]>
Signed-off-by: Manuel Aebischer <[email protected]>
Add initial support for the RP2350's DMA peripheral, allow tests
under drivers/dma/loop_transfer to run on on the Raspberry Pi Pico 2,
and update the board's documentation.

Signed-off-by: Manuel Aebischer <[email protected]>
Signed-off-by: Andrew Featherstone <[email protected]>
Avoid referring to Pico 2 (the name of a board). In this context,
RPI_PICO is used to refer to the (Zephyr) `SOC_FAMILY` rather than the
Pico 1 board. This clarifies common numerical values between the RP2040
and RP2350 SoC series, and enables existing DTS files to be used with
RP2350-based boards with fewer changes.

Remove the use of Zehpyr's `CONFIG_` macros from the device tree files,
and replace them with `SOC_SERIES`-specific files. Update the driver
implementation to conditionally include the correct file. Update
documentation and samples to match.

Signed-off-by: Andrew Featherstone <[email protected]>
Increase test coverage for Raspberry Pi's SoCs. Use the `socs` folder
rather than `boards` to enable these tests to run on any boards with the
same SoCs.

Signed-off-by: Andrew Featherstone <[email protected]>
The RP2350's PWM peripheral is largely unchanged from the RP2040's, but
the higher clock frequency means the long blink delay must be lower.

Signed-off-by: Andrew Featherstone <[email protected]>
@kartben kartben force-pushed the add-rpi-pico-2-support-squash branch from efd8b27 to 4456adb Compare December 23, 2024 21:58
@nordicjm nordicjm requested a review from soburi December 23, 2024 22:12
@kartben kartben marked this pull request as ready for review December 23, 2024 22:12
@kartben kartben merged commit 6a47f72 into zephyrproject-rtos:main Dec 23, 2024
39 checks passed
@ajf58
Copy link
Contributor

ajf58 commented Dec 23, 2024

5c39bb2 looks like it's both authored by and committed by kartben.

This, maintainers (@soburi @carlescufi @nordicjm), is why I really really really tried so many times to discourage futzing around and rebasing code. It's such an easy mistake to fall into exactly this, where as far as git's history will record (and gitlens et al will show when people are reviewing this), all of the work in that commit was done by @kartben .

It's really late in the evening in my timezone. I understand and am grateful kartben has put the effort it and actually did something in front of a keyboard., but man am I pretty blue that the finale of this was a rush-job approval that ended up with this artefact.

@soburi
Copy link
Member

soburi commented Dec 23, 2024

That's why the repeated requests were made "in good faith of reviewers."

It was clear from the beginning that the terms you agreed to when you committed this code allowed you to do this. As explained before, editing by committers is a common practice in OSS, including Linux development, and there is nothing to prevent this.

When the vote result was announced, you had no choice but to fix it yourself or accept it if someone else fixes it. However, by not following the instructions of the vote, you refused to reach a consensus. And by refusing even yesterday's update by the maintainer, you also refused the last chance to merge in your name.

Escalation is provided for when consensus cannot be reached, and refusing to accept the result of the vote can only be interpreted as a refusal to reach a consensus with this project and the community.

If consensus cannot be reached, it is clear that the only option is to proceed based on the terms and written agreement. That is what the terms and agreement are for.

@henrikbrixandersen henrikbrixandersen removed their request for review December 23, 2024 23:47
@ajf58
Copy link
Contributor

ajf58 commented Dec 24, 2024

the last chance to merge in your name.

@soburi , that's not what I'm talking about. I am talking about the difference between a committer and an author within git. Please take this as constructive feedback: quite often when I try and raise a question about one topic you confidently reply about something else, often at great length. This makes it quite challenging to move the conversation forward.

Please go and look at 5c39bb2 . In the process of squashing, both the author (which should be me), and the committer have changed. IMHO former is not correct, the latter is. I really don't think kartben meant to do that: they didn't do it in their previous version of the PR, but post-review feedback they did.

Edit:
I don't think there's any practical[1] way to change this after the fact. I do feel it's reasonable to recognise (for my self) and state (to others) my thoughts and feelings about this.

[1] No one is going to force-push to main to change this, for many reasons, and I don't expect them to.

@kartben
Copy link
Collaborator Author

kartben commented Dec 24, 2024

Hi @ajf58

Please go and look at 5c39bb2 . In the process of squashing, both the author (which should be me), and the committer have changed. IMHO former is not correct, the latter is. I really don't think kartben meant to do that: they didn't do it in their previous version of the PR, but post-review feedback they did.

Let me try to explain the thought process here.

You initially asked me to drop your sign-off from the squashed commits as a way to express your discontent regarding how this was in your opinion not needed. So initially, when I pushed to your branch, squashed commit was "author: Benjamin, sign-off: Benjamin, Co-authored-by: Andrew". The fact your sign-off had been dropped was an OK thing to do as per my interpretation of the DCO, in particular clause (b). However, our guidelines require require to not remove existing sign-off.

So the second iteration, in this PR and what ended up being merged, I tried to convey the same thing you'd asked me to, by also taking into account your sign-off had to be there. So I put myself as author of the commit (arguably I did some, albeit a very very small portion, work on it, so it's hopefully not incorrect) as a way to not be in a situation where you'd be both author and sign-off (as you probably know, our compliance checks require that the git author appears in the list of sign-offs), as you'd explicitly told me you didn't want that.
So in the end it's "Author: Benjamin, Sign-off: Benjamin, Sign-off: Andrew". What's arguably missing is Co-authored-by: Andrew but 1/ like you mentioned, it's not really a git "native" thing, and in Zephyr sign-off typically implies (co-)authorship and 2/ it doesn't look like tools like Gitlens care about Co-authored-by anyway, although apparently GitHub does e58dfac.

So yes you're right, perhaps it was rushed in that you would have in fact been happy with both being git author and sign-off entry in the squashed commit in the end (I'd reached to you about that, and I should have probably waited longer to hear back from you, as you'd maybe change your mind, so I'm sorry I didn't), or that I should have added Co-authored-by in addition to Sign-off even if it felt redundant to me -- looks like maybe it wasn't.

Edit: I don't think there's any practical[1] way to change this after the fact. I do feel it's reasonable to recognise (for my self) and state (to others) my thoughts and feelings about this.

Yep, that's perfectly reasonable and understandable.

@kartben
Copy link
Collaborator Author

kartben commented Dec 24, 2024

I really don't think kartben meant to do that: they didn't do it in their previous version of the PR, but post-review feedback they did.

And to be extra clear: yes, I of course had no intention to come across as claiming authorship in any way, and would have been happy for my name to not even appear on the commits at all had it been an option :)

@carlescufi
Copy link
Member

This, maintainers (@soburi @carlescufi @nordicjm), is why I really really really tried so many times to discourage futzing around and rebasing code. It's such an easy mistake to fall into exactly this, where as far as git's history will record (and gitlens et al will show when people are reviewing this), all of the work in that commit was done by @kartben .

@ajf58 just a quick note here: the rebasing/squashing that was originally requested in your PR was to be done and signed off entirely by you, so at that point I don't think there was any risk of mismatching authorship in commits. Of course, the moment a second person got involved then this "issue" suddenly appeared and creative usage of Git commit author, signed-off-by and co-authored-by was required in order to try and reflect the reality of things in the commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants