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.

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

RP1 384kHz I2S audio support #5999

Merged
merged 4 commits into from Mar 7, 2024
Merged

RP1 384kHz I2S audio support #5999

merged 4 commits into from Mar 7, 2024

Conversation

pelwell
Copy link
Contributor

@pelwell pelwell commented Feb 29, 2024

Two separate problems made it impossible to get 384kHz I2S audio on RP1 (i.e. Pi 5). The first is that if a BCLK ratio other than sample size * 2 is used, the result is the wrong BCLK clock rate. The second is that the choice of PLL frequencies meant that the required BCLK rate for 384kHz 32-bit stereo (*), which is 24.576MHz, is unobtainable, which caused the clock driver to choose the oscillator as a source and resulted in a 25MHz clock and 390.625kHz audio - close, but no cigar.

See #5743 (comment).

(*) Or 16-bit stereo with bclk_ratio of 64, i.e. 16-bits of padding per sample per channel.

@pelwell
Copy link
Contributor Author

pelwell commented Feb 29, 2024

@P33M As discussed.

@pelwell
Copy link
Contributor Author

pelwell commented Feb 29, 2024

As usual, sudo rpi-update pulls/5999 will install a trial/beta kernel with this patch.

@P33M
Copy link
Contributor

P33M commented Feb 29, 2024

The new PLL channel output frequencies should still allow the cohabitation of audio_* and pwm clocks.
lgtm

@P33M
Copy link
Contributor

P33M commented Feb 29, 2024

Random fact: pll_audio has a ternary divider with the same ratio ranges as the secondary divider. Can't remember why we added that - for some reason only the adc, gpclk4 and gpclk5 can select it as a source.

@pelwell
Copy link
Contributor Author

pelwell commented Feb 29, 2024

Is that helpful?

@j-schambacher
Copy link
Contributor

Using the 5999-kernel it seems something is still wrong in the ratio settings (dev->ccr?).
I get the following outputs using the plain hifiberry-dac-overlay generating 1kHz sine wave on either channel:
48/16 OK (1kHz) 32clks/frame, LRCLK=48kHz
48/24 -> output 2kHz and ratio 32clks/frame, LRCLK=96kHz
48/32 -> output 2kHz and ratio 32clks/frame, LRCLK=96kHz

As the standard DAC does not use 'fixed_bclk_ratio' it should run through your new calculation.

The DAC8X shows still the 3/4 LRCLK behaviour with 24bits format while using with 'fixed_bclk_ratio=64'.

@pelwell Please let me know, if I can do some more tests.

@P33M
Copy link
Contributor

P33M commented Feb 29, 2024

No, since the adc is happy to be driven from xosc. I vaguely remember discussions about 44.1khz use-cases, and I think the ultimate constraint is that the vco needs to change to accommodate these (which prevents simultaneous use of anything 48kHz-related).

@pelwell
Copy link
Contributor Author

pelwell commented Feb 29, 2024

I can get what look like sensible waveforms, but only after removing the explicit set_blk_ratio call in rpi-simple-soundcard.c - it looks as though snd_pcm_format_physical_width must be returning 32 for S24_LE. I don't think it is wrong to make that change, but it shouldn't be necessary, so I'd like to understand the behaviour before pushing an update.

@pelwell
Copy link
Contributor Author

pelwell commented Feb 29, 2024

That explains it:

	[SNDRV_PCM_FORMAT_S24_LE] = {
		.width = 24, .phys = 32, .le = 1, .signd = 1,
		.silence = {},
	},

I suspect that the driver should be using snd_pcm_format_width instead, but will confirm.

@pelwell pelwell force-pushed the i2s384 branch 3 times, most recently from e917853 to 1470ec2 Compare March 1, 2024 16:11
@pelwell
Copy link
Contributor Author

pelwell commented Mar 1, 2024

I was right about snd_pcm_format_width - that's what the soundcards should be using, but fixing it revealed another problem: 2 x 24 bits at 48kHz gives a BCLK of 2304000, which isn't an integer divisor of the core audio PLL frequency. Fortunately there's another PLL frequency that gives access to that as well - 1.8432GHz.

This still doesn't give accurate clocks for multiples of 44100 - for that we have to add a mechanism to change the audio PLL on demand. The hardware is capable of that, but I don't think it fits into the Linux clock framework in a clean way - it would have to be a special case within the RP1 clock driver.

See: raspberrypi#5743 (comment)

Signed-off-by: Phil Elwell <phil@raspberrypi.com>
ALSA's concept of the physical width of a sample is how much memory it
occupies, including any padding. This not the same as the count of bits
of actual sample content. In particular, S24_LE has a width of 24 bits
but a physical width of 32 bits because there is a byte of padding with
each sample.

When calculating bclk_ratio, etc., it is width that matters, not
physical width. Correct the error that has been replicated across the
drivers for many Raspberry Pi-compatible soundcards.

Signed-off-by: Phil Elwell <phil@raspberrypi.com>
@pelwell
Copy link
Contributor Author

pelwell commented Mar 6, 2024

I've updated this PR with two new patches. The first removes pll_audio* as potential sources for all clocks except clk_i2s. The second detects attempts to set the rate for clk_i2s and triggers a recalculation of the parent PLLs.

With these changes I get perfect clocks for all the standard audio rates.

@pelwell pelwell force-pushed the i2s384 branch 2 times, most recently from b420527 to 775e554 Compare March 6, 2024 15:55
Prevent all clocks except clk_i2s from using the audio PLLs as sources,
so that clk_i2s may be allowed to change them as needed.

Signed-off-by: Phil Elwell <phil@raspberrypi.com>
Add dedicated code allowing the audio PLLs to be changed, enabling
perfect I2S clock generation. The slowest legal pll_audio_core and
pll_audio will be selected that leads to the required clk_i2s rate.

Signed-off-by: Phil Elwell <phil@raspberrypi.com>
@P33M
Copy link
Contributor

P33M commented Mar 7, 2024

There's no good way of switching a PLL VCO without either a) driving already-configured downstream clocks out of spec or b) notifying all downstream clocks that they should pause while the VCO is reconfigured. This unavoidably upsets use-cases where consumers expect continuous clocking at a certain rate. At design-time, the intent was to choose at start-of-day whether or not you wanted either 44.1kHz or 48kHz clocks - on-the-fly switching wasn't anticipated as software resampling should be fine - until it isn't (DSD over I2S).

Removing pll_audio from other peripheral clocks that can drive audio frequencies means that I2S becomes king-of-the-hill regardless of the downstream hardware configuration. That prompted me to ask - why does the driver store information about how the hardware is both designed and configured? This information should be in the DT - and overlays can then manipulate it, with users deciding which tradeoffs to take around which sets of peripherals play well with each other. One such example is that you can use pll_video as an alternate source for audio and pwm peris - but then this precludes DSI/DPI/VEC use-cases.

But adding such flexibility is a substantial rewrite - and right now I2S is king of the hill as there are no drivers for the audio_* peris and PWM should be happy with xosc.

};
const unsigned long xosc_rate = clk_hw_get_rate(clk_xosc);
const unsigned long core_max = 2400000000;
const unsigned long core_min = xosc_rate * 16;
Copy link
Contributor

Choose a reason for hiding this comment

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

The datasheet VCO minimum here is a static 600MHz, but the VCO-to-refclk ratio is more restrictive. RP1 probably won't ever be fitted with a different (slower) crystal.

@pelwell pelwell merged commit 6f16847 into raspberrypi:rpi-6.6.y Mar 7, 2024
11 of 12 checks passed
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Mar 7, 2024
See: raspberrypi/linux#6008

kernel: Add GPCLK support on RP1
See: raspberrypi/linux#6013

kernel: RP1 384kHz I2S audio support
See: raspberrypi/linux#5999
popcornmix added a commit to raspberrypi/rpi-firmware that referenced this pull request Mar 7, 2024
See: raspberrypi/linux#6008

kernel: Add GPCLK support on RP1
See: raspberrypi/linux#6013

kernel: RP1 384kHz I2S audio support
See: raspberrypi/linux#5999
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants