Skip to content

Conversation

vbrzeski
Copy link
Contributor

Improve the Audio PLL clock frequency calculations to provide a more accurate clock frequency.

These formulas were derived with a Wolfram Alpha query using the long-form formula in the source comments.

Improve the Audio PLL clock frequency calculations to provide a more
accurate clock frequency.

Signed-off-by: Victor Brzeski <[email protected]>
@sonarqubecloud
Copy link

@vbrzeski
Copy link
Contributor Author

I'm actually unsure if this formula in the source code is correct. The standard div value overflows for 48k

case 8:
return AUDIOPLL_DIV_8;
case 12:
return AUDIOPLL_DIV_12;
Copy link
Contributor

@bjarki-andreasen bjarki-andreasen Oct 17, 2025

Choose a reason for hiding this comment

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

Prescaler div 12 was the only used and valid prescaler when this driver was written, has that changed?

@carlescufi
Copy link
Member

I'm actually unsure if this formula in the source code is correct. The standard div value overflows for 48k

@tmon-nordic any input?

@tmon-nordic
Copy link
Contributor

tmon-nordic commented Oct 23, 2025

I'm actually unsure if this formula in the source code is correct. The standard div value overflows for 48k

@tmon-nordic any input?

Where do you see overflow? Code uses uint64_t and I don't quite see any possibility for overflow.

I run over the whole possible frequency range with divider 12 (intended for AudioPLL) and for most values the both old and new formulas match. However, when the derived fraction differs (and they all do differ only by 1), the new formula has significantly higher absolute error (> 40 Hz) than the old formula (<1 Hz). Edit: My test code did integer division and not DIV_ROUND_CLOSEST(), so the conclusions are invalid.

It is important to note that while the comment frequency = ((4 + (freq_fraction * 2^-16)) * 32000000) / 12 looks like integer arithmetic, the generated frequency is not constrained to integers. However, when frequency is input to a function it indeed is integer constrained (1 Hz resolution should not be a problem), the actual calculations are also on integers and the output is integer (must be due to hardware only accepting fraction as integer value).

Comment on lines +38 to +52
switch (divider) {
case 0:
return AUDIOPLL_DIV_DISABLED;
case 6:
return AUDIOPLL_DIV_6;
case 8:
return AUDIOPLL_DIV_8;
case 12:
return AUDIOPLL_DIV_12;
case 16:
return AUDIOPLL_DIV_16;
default:
__ASSERT(divider <= 4, "invalid audiopll divider");
return (enum audiopll_prescaler_div)divider;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The values seem to originate from nrfs header, but I have no idea why nrfs makes them available. I would prefer if AudioPLL just continues to use hardcoded AUDIOPLL_DIV_12 which is known to work.

@tmon-nordic
Copy link
Contributor

With corrected comparison script, the results are as follows:

  • new formula has lower absolute error for all frequencies that output different fraction value
  • the difference between new and old formula can only be 1 (when output between new and old formula differs, new is always 1 lower than old)
  • on differing outputs, new formula always results in frequency lower than requested and old formula always results in frequency higher than requested
  • absolute error for new formula is on average 0.655 Hz lower than using old formula (min 0.0026 Hz, max 1.33 Hz, stddev 0.378 Hz)

Therefore new formula is slightly better on accuracy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants