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.

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

Update lower VCO frequency limit according to datasheet update #684

Closed
wants to merge 1 commit into from
+17 −10
Closed

Update lower VCO frequency limit according to datasheet update #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions rp2040-hal/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

## Fixed

- Fixed minimum PLL's VCO frequency according to updated datasheet - @ithinuel

## [0.9.0]

### MSRV
Expand Down
21 changes: 11 additions & 10 deletions rp2040-hal/src/pll.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,10 @@ pub mod common_configs {

/// Default, nominal configuration for PLL_USB.
pub const PLL_USB_48MHZ: PLLConfig = PLLConfig {
vco_freq: HertzU32::MHz(480),
vco_freq: HertzU32::MHz(960),
Copy link
Member

Choose a reason for hiding this comment

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

I like that you didn't take the value from https://github.com/raspberrypi/pico-sdk/pull/869/files, but kept it closer to the previous value.
Another possible combination would be a vco_freq of 768 MHz with post_div1: 4 and post_div2: 4.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the multiple of 48 between 750 and 1600 only these can be divided by valid post_div1/2 value:

multiple divisor
768 4*4
864 6*3
960 5*4
1008 7*3
1152 6*4
1200 5*5
1344 7*4
1440 6*5

The doc recommends that when the two post_div have different value to preferably use the higher one first.
But it does not say anything about preferring same-divisor vs different-divisors 🤷

I read in the datasheet for another chip that higher internal PLL frequency was better (I can't remember if it was for power consumption or frequency stability).

The pico SDK has used 1200 with same-divisor 5*5.
I doubled the frequency because it required the least mental calculation 💇

Copy link
Member

Choose a reason for hiding this comment

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

As I understand it, higher frequency of the VCO means the resulting output frequency is more stable. At the cost of higher power consumption. So it's a tradeoff.
I don't know anything about the stability requirements of the USB clock, but if it worked reasonably well with the 400MHz VCO in the past, I'd guess that 768 or 960 MHz will be more than enough and 1200MHz would not result in any useful improvement.
But that's all guesswork.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I just found this in the datasheet:

Jitter is minimised by running the VCO at the highest possible frequency, so that higher post-divide values can be used.
For example, 1500MHz VCO / 6 / 2 = 125MHz. To reduce power consumption, the VCO frequency should be as low as
possible. For example: 750MHz VCO / 6 / 1 = 125MHz.

Copy link
Member

Choose a reason for hiding this comment

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

I tried to measure the jitter depending on the VCO frequency.

The measurement setup is just an oscilloscope which measures the signal frequency at the clock output on gpio21, and calculates a standard deviation from those values. The validity of these measurements is a questionable because the (nominally) 100MHz oscilloscope (Siglent SDS 1104X-E) is probably at its limit for measuring this 48MHz signal. And of course the measurement method is not really suitable to characterize signal jitter.

Anyway, I measured the following standard deviations of the frequency:

  • at 480MHz VCO: 22kHz
  • at 768MHz VCO: 20kHz
  • at 960MHz VCO: 18kHz
  • at 1200MHz VCO: 19kHz
  • at 1440MHz VCO: 18kHz

My impression is that the differences between 960 and 1400 MHz are just statistical fluctuations. I only measured 1000 cycles, so the values were not yet stable enough to tell it with more precision.
But the increase of the standard deviation at lower VCO frequencies was obvious.

Of course, it might be that the actual values are much lower and the measurements are dominated by errors caused by my measurement setup.

In conclusion, I'd say we should use a VCO frequency of at least 960 MHz. Above that value, this measurement doesn't deliver any meaningful result.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest that we first apply the minimal change, #688, to use the same VCO value as the C SDK.
This doesn't invalidate your other changes (updating the valid range & code improvements), but by appying the minimal update first, we can do a bugfix release without breaking changes, first.

refdiv: 1,
post_div1: 5,
post_div2: 2,
post_div2: 4,
};
}

Expand All @@ -141,18 +141,22 @@ impl<D: PhaseLockedLoopDevice> PhaseLockedLoop<Disabled, D> {
xosc_frequency: HertzU32,
config: PLLConfig,
) -> Result<PhaseLockedLoop<Disabled, D>, Error> {
const VCO_FREQ_RANGE: RangeInclusive<HertzU32> = HertzU32::MHz(400)..=HertzU32::MHz(1_600);
const VCO_FREQ_RANGE: RangeInclusive<HertzU32> = HertzU32::MHz(750)..=HertzU32::MHz(1_600);
Copy link
Member

Choose a reason for hiding this comment

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

So you don't think increasing the lower limit here should be considered a semver breaking change?

Most users would use the default values anyway, so updating PLL_USB_48MHZ would be enough to avoid an out-of-spec setting.

So the question is how to handle the few users which use a custom clock spec outside the new limits. Does the risk of a somehow unstable clock justify the breaking change in a bugfix version?
And is a runtime panic at startup a sufficiently better alternative? Unfortunately, I don't see a way to catch this at build time.

I'd prefer to leave this limit at 400MHz.

Copy link
Member Author

@ithinuel ithinuel Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So you don't think increasing the lower limit here should be considered a semver breaking change?

I'm not too sure to be honest. Should we update the semver on the first PR that requires it to be updated or right before a release when we review the changelog ?

Does the risk of a somehow unstable clock justify the breaking change in a bugfix version?

I may have missed some discussions around having a bug fix version.

Copy link
Member

Choose a reason for hiding this comment

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

I may have missed some discussions around having a bug fix version.

No there was no such discussion. It's just that we could do compatible releases right now without much effort as the last release is only a few days ago, so checking for breaking changes is still easy.

Once we start merging breaking changes, we'd need to manually backport fixes to a bugfix branch if we want to do such a release.

(Bugfix is not the best term here, as we are pre 1.0, there are no separate numbers for minor and patch releases. Let's call it compatible vs. incompatible, using the terms from https://doc.rust-lang.org/cargo/reference/resolver.html#semver-compatibility.)

const POSTDIV_RANGE: Range<u8> = 1..7;
const FBDIV_RANGE: Range<u16> = 16..320;

let vco_freq = config.vco_freq;
let PLLConfig {
vco_freq,
refdiv,
post_div1,
post_div2,
} = config;

if !VCO_FREQ_RANGE.contains(&vco_freq) {
return Err(Error::VcoFreqOutOfRange);
}

if !POSTDIV_RANGE.contains(&config.post_div1) || !POSTDIV_RANGE.contains(&config.post_div2)
{
if !POSTDIV_RANGE.contains(&post_div1) || !POSTDIV_RANGE.contains(&post_div2) {
return Err(Error::PostDivOutOfRage);
}

Expand All @@ -161,7 +165,7 @@ impl<D: PhaseLockedLoopDevice> PhaseLockedLoop<Disabled, D> {

let ref_freq_hz: HertzU32 = xosc_frequency
.to_Hz()
.checked_div(u32::from(config.refdiv))
.checked_div(u32::from(refdiv))
.ok_or(Error::BadArgument)?
.Hz();

Expand All @@ -180,9 +184,6 @@ impl<D: PhaseLockedLoopDevice> PhaseLockedLoop<Disabled, D> {
return Err(Error::FeedbackDivOutOfRange);
}

let refdiv = config.refdiv;
let post_div1 = config.post_div1;
let post_div2 = config.post_div2;
let frequency: HertzU32 = ((ref_freq_hz / u32::from(refdiv)) * u32::from(fbdiv))
/ (u32::from(post_div1) * u32::from(post_div2));

Expand Down