Add initial support for the Raspberry Pi Pico 2

[ajf58]

https://builds.zephyrproject.io/zephyr/pr/77368/docs/boards/index.html#name=pico+2

This is a PR to add support for the Raspberry Pi Pico 2 board, and the RP2350 SoC. See also the discussion at #77329

This should not be merged currently, as it includes changes that will break the clock control driver for the RP2040.

Dependencies

1. west: hal_rpi_pico: Update Pico-SDK to 2.0.0 #76986
2. Updating hal_rpi_pico incorporate the change for raspberrypi/pico-sdk@0e5ef0f

cc: @yonsch @soburi

[zephyrbot]

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[soburi]

@ajf58
First, I've listed some points that caught my attention at a glance.
I haven't been able to check it thoroughly yet, so I'll look at it a bit more.

[soburi]

@ajf58

My suggestion.

https://github.com/soburi/zephyr/tree/pico2_soc

[nordicjm]

Updates are OK. Though migration guide entry needed for the SoC symbol change

[ajf58]

Updates are OK. Though migration guide entry needed for the SoC symbol change

@nordicjm where should this update go? Is under boards in doc/releases/migration-guide-4.0.rst correct?

[nordicjm]

Updates are OK. Though migration guide entry needed for the SoC symbol change

@nordicjm where should this update go? Is under boards in doc/releases/migration-guide-4.0.rst correct?

Yes

[ajf58]

File name fettling has been done. Please note the commit message did (and still does) highlight that in several cases this wasn't required.

Since this is now not going into v4.0, I've also modified the relevant commit so that the documentation updates target v4.1's release notes.

Please note my comment regarding the squashing of rp2350: Define and implement a cpucluster of Cortex-M33s. I'm not going to do that for the reasons I've already outlined above.

[ajf58]

Please can you mark this "conversation" as resolved? I don't think there's any more discussion to be had here.

[soburi]

LGTM

[nordicjm]

Please note my comment regarding the squashing of rp2350: Define and implement a cpucluster of Cortex-M33s. I'm not going to do that for the reasons I've already outlined above.

This is clearly defined in the zephyr contribution guidelines, the exact point of which you have been made aware of earlier https://docs.zephyrproject.org/latest/contribute/guidelines.html#contribution-workflow

If reviewers do request changes to your patch, you can interactively rebase commit(s) to fix review issues. In your development repo:

git rebase -i <offending-commit-id>^

In the interactive rebase editor, replace pick with edit to select a specific commit (if there’s more than one in your pull request), or remove the line to delete a commit entirely. Then edit files to fix the issues in the review.

As before, inspect and test your changes. When ready, continue the patch submission:

git add [file(s)]
git rebase --continue

Update commit comment if needed, and continue:

git push --force origin fix_comment_typo

History has nothing to do with it, you're trying to claim if someone submits a PR with C code changes that do not compile, instead of fixing them in the original commit, they should add a new commit which fixes them on top of it to preserve the history, this makes absolutely no sense.

[nordicjm]

Squash requested changes into original commits

[kartben]

... you're trying to claim if someone submits a PR with C code changes that do not compile

@nordicjm that's not at all what @ajf58 is saying (let alone "claiming"). Please, pretty please, refrain from telling people what they are saying "make no sense", especially if you're going into a straw man argument.

The commit that's left "unsquashed" is transitioning from a working state to another working state, AFAICT, and reflecting changes along the lines of "initial support" --> "slightly improved initial support". That last commit would have likely been a different, follow-up, PR in other circumstances.

[nordicjm]

... you're trying to claim if someone submits a PR with C code changes that do not compile

@nordicjm that's not at all what @ajf58 is saying (let alone "claiming"). Please, pretty please, refrain from telling people what they are saying "make no sense", especially if you're going into a straw man argument.

The C part is applying the same logic here to code instead of a dts file. See #80707 (review)

The commit that's left "unsquashed" is transitioning from a working state to another working state, AFAICT, and reflecting changes along the lines of "initial support" --> "slightly improved initial support". That last commit would have likely been a different, follow-up, PR in other circumstances.

Because we don't rebase history that has been merged, this has not been merged, you submit a clean PR which does what is needed, not add commits that you later fix up in the same PR, and this isn't even a zephyr this, this is a common sense thing with projects and git in general

[fabiobaltieri]

The commit that's left "unsquashed" is transitioning from a working state to another working state, AFAICT, and reflecting changes along the lines of "initial support" --> "slightly improved initial support". That last commit would have likely been a different, follow-up, PR in other circumstances.

That is not how code review works here, or in Linux (which the process is inspired from), or I imagine in any other project with a maintainer structure. Fixups go in the commit that introduced a file, sure in other circumstances it would be fine, but this is not that and the fixup commit makes the stack incredibly hard to review and the whole PR evolution very VERY hard to follow, leading to this being delayed further. There's a reason why that's the process documented in the contribution guidelines.

[soburi]

@ajf58

The commit content should not include the discussion process that led to this point, but rather revise it to a direct commit that reflects the final conclusion and commit it.
Also, please try to avoid making modifications to the same file within a PR as much as possible.

I have sorted it out. Please check it out. (Need to clone the repository)
https://github.com/soburi/zephyr/tree/pr77368_squash

There are some parts in the revision process that use my name,
so please use git format-patch and git am to revise and use it.

[ajf58]

Do Not Merge @soburi 's branch.

@kartben's comment is absolutely correct at #77368 (comment) .

When @fabiobaltieri says:

fixups go in the commit that introduced a file

There is no fixup commit.

Again, kartben is absolutely spot on. This PR could have been merged as-is without the last commit. I don't endorse it, but it has to have my sign off because the "signed off by" process mandates it.

I have sorted it out. Please check it out. (Need to clone the repository)
https://github.com/soburi/zephyr/tree/pr77368_squash

@soburi the branch you've created now really is wrong. At the very least:

1. because you've squashed at least one commit together, but failed to correctly change the commit message, the commit message is wrong.
2. You've messed up the committer and author on at least one commit.
3. You've created commits that will fail the Compliance Checks.

There are some parts in the revision process that use my name,
so please use git format-patch and git am to revise and use it.

No, absolutely not. You cannot take my work, break it, give it back to me, and expect me to fix it.

[nordicjm]

Had a quick look at @soburi's branch and looks ok.

because you've squashed at least one commit together, but failed to correctly change the commit message, the commit message is wrong.

He's correctly squashed requested changes into the base commits

You've messed up the committer and author on at least one commit.

Have you ever actually used git before? This is what happens when you rebase, even if you make no changes, go and rebase and git commit --amend --reset-author to fix.

You've created commits that will fail the Compliance Checks.

Where?

[soburi]

@ajf58

and expect me to fix it.

That's true.
That's why I'm not presenting this in a mergeable form.
But I think it's useful enough to show what I, @nordicjm and @fabiobaltieri expect.
Explaining this in words would be too long, so it's better to show an example, which is what I'm doing here.

[soburi]

@ajf58

Do Not Merge @soburi 's branch.

No one said my branch to merge.
In the first place, how can anyone merge something that isn't even a PR?

No, absolutely not. You cannot take my work, break it, give it back to me, and expect me to fix it.

Ok, of course, I'd welcome you to solve the problem your way.

[soburi]

I share @fabiobaltieri 's concern.
Even if policy changes as the PR branch evolves, the ultimately merged commits should be easy to read and organized based on the final policy.
Because the source code is usually "Write once, read many times."
As pointed out, the guideline's statement that we should rebase and force push also makes it undesirable to have "intermediate states" in a PR, I think.

[soburi]

@ajf58 @nordicjm @fabiobaltieri @kartben

Everyone,

Thank you for joining this lengthy discussion.

Now, the issue of rebasing commits has been raised,
and I think this is the last issue.
I don't think any implementation issues remain with the final code.

However, this "rebase and clean up the history before committing" is
a norm practiced by many people in Zephyr's development.
It is also explicitly stated in the guidelines, so I need to respect this from a maintainer's perspective.
In other words, the rules that everyone else follows must be followed.

@kartben 's comment is correct, but I think It needs stronger evidence against the clearly written rules.

So, my first hope is that this will be changed.
However, it is also true that @ajf58 is exhausted.
Looking at the long history so far, I also want to finish it soon.
I think we need to find a compromise.

1. Can it be approved in its current state?

It would be better to fix it from the perspective of the community's standards.
However, the negative impact would be limited and not a severe problem.

It would involve some compromise, but is this possible? > @nordicjm, @fabiobaltieri

2. If it needs to be fixed

If not, it needs to be fixed.

However, as already mentioned, @ajf58 is also quite exhausted.

Even if a fix is ​​necessary, we must consider how to make it as easy for him as possible.
We need to follow the community's rules, but I want to avoid asking for repeated fixes on this issue.

Can we proceed with the following process?

1. Present an acceptable fix policy (@nordicjm, @fabiobaltieri, @soburi, @kartben)
2. Submit the fix (@ajf58)
3. If a correction is needed, provide feedback with a directly applicable form (@nordicjm, @fabiobal, @soburi, @kartben)

It will be a burden for @ ajf58, but the rebase, force push, and clean-up of the history procedures are necessary if you commit to Zephyr, so I would like you to get used to it.

First of all, I would like to hear your opinion.

[nordicjm]

The rules are there in plain text for everyone to follow for the benefit of the project, no exceptions to these base rules (which are arguably rules for any git project in general, there's nothing zephyr specific about them)

[kartben]

AFAICT commit granularity also has a lot to do with maintainers' preferences (just like commit message labels, which are definitely not consistent across all areas), and as @soburi has now approved the PR here many times, I think it's safe to say that they, as the maintainer of this area, can probably live with the way things are currently split.

[fabiobaltieri]

Sure listen, it's no big deal, we merge off process changes that miss proper review all the time, this is fine. But the platform maintainer (@soburi) thinks it should be squashed, the buildsystem collab (@nordicjm, without whom the whole board infrastructure would be an unmanageable dumpster fire, which is arguably a bigger problem for the project) thinks it should be squashed, the process says it should be squashed. I'd be fine going off process if the maintainer(s) agreed not do, but it's not the case. So my suggestion is, pretty please, squash the commit and move on.

For the record, if this was Linux, that has a much better structured hierarchy pull request process, this would be a non-issue, the maintainer would have the option to take the patches in whatever form and massage them in shape in their working branch before opening a pull request themselves for integration, which is exactly what @soburi did. Also:

Explaining this in words would be too long, so it's better to show an example, which is what I'm doing here.

Amen.

[soburi]

@nordicjm @kartben @fabiobaltieri

Thank you for your opinion.

It's a tough decision, but I will proceed with the following process.

First of all, I would like to ask @ajf58 to deal with squash.
Of course, this is only a request, not an order.
However, I would be grateful if he would accept this,
and I think this would be the way to finish this PR most amicably.

If not, that is, if my request is not accepted, I will proceed with the approval as this PR is.
(In other words, I will remove the NACK by force.)
It may not be the best process, but as @kartben says, I think it is within the scope of discretion.
I also think that the log of this discussion is a sufficient record of what was confirmed to be okay
and what was compromised. It is possible to look back.

@ajf58 is probably very exhausted and may not be in a situation to respond.
It might be a little long, but I will wait until next week, and if there is no response by then,
I will exercise the above policy.
If any necessary responses or requests arise after that, I will handle them.

[fabiobaltieri]

If not, that is, if my request is not accepted, I will proceed with the approval as this PR is.
(In other words, I will remove the NACK by force.)

There's a fairly well defined process for review dismissal by the assignee (https://docs.zephyrproject.org/latest/project/project_roles.html#assignee), it eventually resolves up to the technical escalation. More discussions, more meetings, more arguing about what is ultimately a rather simple feedback about following the process. Or you could squash the commit @ajf58.

Anyway @soburi what I'm trying to say is that I appreciate and admire the strong stance to wrap this but I'd rather you not dismiss a non stale, non incorrect blocker as that may result into an endless series of process wg discussions about dmissial rules that I would have to endure. :-)