Update lower VCO frequency limit according to datasheet update

[ithinuel]

Fixes #682

[jannic]

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.

[ithinuel]

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.

[jannic]

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.)

[jannic]

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.

[ithinuel]

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 💇

[jannic]

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.

[ithinuel]

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.

[jannic]

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.

[jannic]

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.

[jannic]

You can find a rebased branch here: https://github.com/jannic/rp-hal/tree/update-pll-vco-min-freq

@ithinuel, I didn't want to force-push into your branch, even if I may have the necessary permissions.
One open question is if we want a default VCO frequency of 1200 MHz or 960 MHz. I don't think it makes a meaningful difference, so for me both choices would be fine.

[ithinuel]

To be honest I don’t even think this PR is still necesarry, the only change it bring (excluding the PLL config which has been corrected in #688 already) is the destructuring assignment for the PLLConfig.
I’ll close this PR.

[jannic]

This PR added correct validation of the VCO frequency. That was not part of #688 because it is a breaking change, but now that we are preparing a 0.10 release, we can include it.

[ithinuel]

Ok, that makes sense, I created #773 with that change only (and proper description).