8000 arch/arm/rp23xx: update USB PLL/VCO/FBDIV by shtirlic · Pull Request #16300 · apache/nuttx · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

arch/arm/rp23xx: update USB PLL/VCO/FBDIV #16300

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 8, 2025
Merged

arch/arm/rp23xx: update USB PLL/VCO/FBDIV #16300

merged 1 commit into from
May 8, 2025

Conversation

shtirlic
Copy link
Contributor
@shtirlic shtirlic commented May 2, 2025 •
edited
Loading

Summary

image

RP2350 datasheet and upstream pico-sdk suggests higher VCO freq to
better output stability.
Increase the VCO and update PLL divs according to specs and calculation.
Higher VCO freq is more stable, lower VCO freq is more power friendly.
Also it's possible to use 120/6/5 with VCO 1440MHz for USB PLL to even
higher output stability.
USB setup changes from rp2040
Must clear the MAIN_CTRL.PHY_ISO bit at startup and after power down
events.

Impact

More stable USB operations

Testing

[280432.642035] usb 1-1: new full-speed USB device number 118 using xhci_hcd
[280432.792214] usb 1-1: New USB device found, idVendor=0525, idProduct=a4a7, bcdDevice= 1.01
[280432.792240] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[280432.792243] usb 1-1: Product: CDC/ACM Serial
[280432.792246] usb 1-1: Manufacturer: NuttX
[280432.792251] usb 1-1: SerialNumber: 0
[280432.838272] cdc_acm 1-1:1.0: ttyACM2: USB ACM device

---- Opened the serial port /dev/ttyACM2 ----

NuttShell (NSH) NuttX-12.9.0
nsh> ls /dev/
/dev:
 console
 null
 ttyACM0
 zero

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: XS The size of the change in this PR is very small labels May 2, 2025
@shtirlic shtirlic changed the title arch/arm/rp23xx: Update USB pll/VCO arch/arm/rp23xx: Update USB PLL/VCO/FBDIV May 2, 2025
@shtirlic shtirlic changed the title arch/arm/rp23xx: Update USB PLL/VCO/FBDIV arch/arm/rp23xx: update USB PLL/VCO/FBDIV May 2, 2025
@shtirlic
Copy link
Contributor Author
shtirlic commented May 2, 2025

@keever50 if I am not mistaken you had some USB issues, could you test this changes to see any improvements.

@keever50
Copy link
Contributor
keever50 commented May 2, 2025

@shtirlic Interesting! Good catch. Ill look into this later.
Have you confirmed this yourself as well?
Do you think instability can cause USB to drop out or system hangs?

Yesterday I was looking at the clocks of RP2040. I have a theory that incorrect clocking or incorrect voltages cause overall instability.
Is it possible that some of the clocking is a direct port between RP2040 and RP23xx?

I hope this solves some of the USB CDC ACM serial issues. They are extremely unreliable on RP2040. (After filling the memory with enough things, more than just the nsh)
I have seen the same happen in hardware UART, so is there a possibility that the same mistakes have been made there?

I will test this later today, thank you for experimenting!

@shtirlic
Copy link
Contributor Author
shtirlic commented May 2, 2025

@keever50 in pico-sdk they use the same config for USB pll for rp2040 and 2350, so you could try to change it, even set it higher for tests.

@shtirlic
Copy link
Contributor Author
shtirlic commented May 2, 2025 •
edited
Loading

@keever50 I am in the process of reviewing all peripherals code for rp2350 and found some of them are broken from the start due to some changes, see my pwm and watchdog current pull . Also there are some erratas and changes upstream.

@shtirlic shtirlic force-pushed the usbupdate branch 2 times, most recently from 24eba77 to 7525f1c Compare May 3, 2025 15:05
@shtirlic
arch/arm/rp23xx: update USB PLL/VCO/FBDIV and usb setup
0097a23 
RP2350 datasheet and upstream pico-sdk suggests higher VCO freq to
better output stability.
Increase the VCO and update PLL divs according to specs and calculation.
Higher VCO freq is more stable, lower VCO freq is more power friendly.
Also it's possible to use 120/6/5 with VCO 1440MHz for USB PLL to even
higher output stability.
USB setup changes from rp2040
Must clear the MAIN_CTRL.PHY_ISO bit at startup and after power down
events.

Signed-off-by: Serg Podtynnyi <serg@podtynnyi.com>
@shtirlic shtirlic marked this pull request as ready for review May 7, 2025 03:38
@xiaoxiang781216
Copy link
Contributor

@cederom what's problem make the merge button gray?

@shtirlic shtirlic requested a review from acassis May 7, 2025 06:45
@shtirlic
Copy link
Contributor Author
shtirlic commented May 7, 2025

@xiaoxiang781216 maybe because of this, I asked @acassis to re-approve to see if it helps

Last push must be approved (new changes require re-approval)
At least 2 approving reviews required

@xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 maybe because of this, I asked @acassis to re-approve to see if it helps

Last push must be approved (new changes require re-approval)
At least 2 approving reviews required

the problem now change to build/check never finish.

@raiden00pl
Copy link
Member

@shtirlic try rebase to last master

cederom
cederom approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor
@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @shtirlic :-)

@cederom
Copy link
Contributor
cederom commented May 7, 2025

Sorry for the problems we are updating PR merge rules and in a short window of testing we will update all settings as needed :-)

@acassis
Copy link
Contributor
acassis commented May 7, 2025

@cederom what does it mean "Waiting for status to be reported" ?

@cederom
Copy link
Contributor
cederom commented May 7, 2025

@acassis: @cederom what does it mean "Waiting for status to be reported" ?

Sorry this is unrelated to this patch, comes from asf.yaml update we are fixing right now :-)

@cederom
Copy link
Contributor
cederom commented May 8, 2025

Allright the PR processing is now fixed, merging :-)

@cederom cederom merged commit 7c203a9 into apache:master May 8, 2025
25 checks passed
@shtirlic shtirlic deleted the usbupdate branch May 8, 2025 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acassis acassis acassis approved these changes

@cederom cederom cederom approved these changes

@raiden00pl raiden00pl raiden00pl approved these changes

@jerpelea jerpelea jerpelea approved these changes

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 approved these changes

Assignees
No one assigned
Labels
Arch: arm Issues related to ARM (32-bit) architecture Size: XS The size of the change in this PR is very small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@shtirlic @keever50 @xiaoxiang781216 @raiden00pl @cederom @acassis @jerpelea
0