bcm2835-i2s: add support for set_tdm_slot and 384kHz sample rates #1982

HiassofT · 2017-04-30T22:20:11Z

set_tdm_slot support allows us to use codecs with fixed bclks per sample with simple-card. This means we will be able to remove a bunch of downstream card drivers and replace them with DT overlays using the simple-card driver. All that's needed then is to set the tdm slot config in the cpu node of simple-card. eg:

cpu_dai: simple-audio-card,cpu {
        sound-dai = <&i2s>;
        dai-tdm-slot-num = <2>;
        dai-tdm-slot-width = <32>;
};

The old set_bclk_ratio method is still supported, so there's no pressing need to change the current downstream drivers.

384kHz support has been requested for quite a long time. Testing with an iqaudio-dac (pcm5122 codec) went fine so I think it is safe to finally add that.

ping @pelwell @clivem

clivem · 2017-04-30T23:16:39Z

384kHz support has been requested for quite a long time. Testing with an iqaudio-dac (pcm5122 codec) went fine so I think it is safe to finally add that.

OK, cool! It's safe to finally add that, now that it has finally been tested! ;)

pelwell · 2017-05-01T06:40:10Z

sound/soc/bcm/bcm2835-i2s.c

@@ -1,3 +1,4 @@
+#define DEBUG


Remove this.

pelwell · 2017-05-01T06:40:54Z

sound/soc/bcm/bcm2835-i2s.c

+
+	/*
+	 * bcm2835-i2s only supports stereo configurations, so also
+	 * check that excactly 2 bits are set in the mask.


Typo "excactly"

pelwell · 2017-05-01T06:42:41Z

sound/soc/bcm/bcm2835-i2s.c

-	default:
-		return -EINVAL;
-	}
+	data_length = params_width(params);


Have we lost an error for unsupported formats?

pelwell · 2017-05-01T06:47:00Z

sound/soc/bcm/bcm2835-i2s.c

-	default:
-		return -EINVAL;
-	}
+	format = BCM2835_I2S_CH1(format) | BCM2835_I2S_CH2(format);


Do we care that there is no error case here?

@flatmax How do these changes work for you?

HiassofT · 2017-05-01T08:58:50Z

@pelwell thanks a lot for your feedback! I've removed the debug leftover and fixed the typo.

As for the removed checks: hw_params is already guarded by the (stream) constraints setup by the driver in snd_soc_dai_driver. ALSA/ checks for these constraints and requests for other channels/samplerates/formats won't make it to hw_params. So these checks in hw_params were redundant.

But I noticed another missing check, hw_params missed errors from clk_set_rate. I've added that check to the samplerate patch, using SNDRV_PCM_RATE_CONTINUOUS should now really be safe.

As for Matt's ugly hack: messing with the constraints setup by a driver by directly modifying it's private data structures was never allowed. ALSA drivers should only use the snd_pcm_hw_constraint* methods to add additional constrains. Other than that I'm pretty sure that this hack still works as the checks in hw_params are now removed... But I'd really like to see his driver implemented properly, as Mark Brown and I told him on the ALSA mailing list.

flatmax · 2017-05-01T09:26:51Z

On 01/05/17 16:47, Phil Elwell wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In sound/soc/bcm/bcm2835-i2s.c <#1982 (comment)>: > @@ -1,3 +1,4 @@ +#define DEBUG Remove this. ------------------------------------------------------------------------ In sound/soc/bcm/bcm2835-i2s.c <#1982 (comment)>: > @@ -221,25 +231,86 @@ static int bcm2835_i2s_set_dai_fmt(struct snd_soc_dai *dai, return 0; } +static int bcm2835_i2s_set_channel_pos(unsigned int mask, int slots, int width, + unsigned int* chpos) +{ + int i, slot; + + /* mask out unavailable slots */ + mask &= GENMASK(slots - 1, 0); + + /* + * bcm2835-i2s only supports stereo configurations, so also + * check that excactly 2 bits are set in the mask. Typo "excactly" ------------------------------------------------------------------------ In sound/soc/bcm/bcm2835-i2s.c <#1982 (comment)>: > - * does not apply. - */ - switch (params_format(params)) { - case SNDRV_PCM_FORMAT_S16_LE: - data_length = 16; - break; - case SNDRV_PCM_FORMAT_S24_LE: - data_length = 24; - break; - case SNDRV_PCM_FORMAT_S32_LE: - data_length = 32; - break; - default: - return -EINVAL; - } + data_length = params_width(params); Have we lost an error for unsupported formats? ------------------------------------------------------------------------ In sound/soc/bcm/bcm2835-i2s.c <#1982 (comment)>: > @@ -308,28 +369,24 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream, return -EINVAL; } - ch1pos = data_delay; - ch2pos = bclk_ratio / 2 + data_delay; - - switch (params_channels(params)) { - case 2: - case 8: - format = BCM2835_I2S_CH1(format) | BCM2835_I2S_CH2(format); - format |= BCM2835_I2S_CH1(BCM2835_I2S_CHPOS(ch1pos)); - format |= BCM2835_I2S_CH2(BCM2835_I2S_CHPOS(ch2pos)); - break; - default: - return -EINVAL; - } + format = BCM2835_I2S_CH1(format) | BCM2835_I2S_CH2(format); Do we care that there is no error case here? @flatmax <https://github.com/FlatMax> How do these changes work for you?

People have received their Octo 8 channel sound cards and they are reporting good stability and great functioning. I want to check that this patch doesn't break some features in both the stereo card and the 8 channel card. In the case of the stereo card it depends heavily on the bcm2835_i2s_set_dai_bclk_ratio function to setup the clocking offsets correctly for the wm8731 chip. I want to check that this doesn't effect wm8731 based cards. In the case of the Octo 8 channel card, it relies heavily on the DMA subsystem of ALSA. I would want to check that using the TDM ALSA subsystem doesn't effect the DMA mapping. thanks Matt

flatmax · 2017-05-01T09:31:52Z

On 01/05/17 18:59, Matthias Reichl wrote: @pelwell <https://github.com/pelwell> thanks a lot for your feedback! I've removed the debug leftover and fixed the typo. As for the removed checks: hw_params is already guarded by the (stream) constraints setup by the driver in snd_soc_dai_driver. ALSA/ checks for these constraints and requests for other channels/samplerates/formats won't make it to hw_params. So these checks in hw_params were redundant. But I noticed another missing check, hw_params missed errors from clk_set_rate. I've added that check to the samplerate patch, using SNDRV_PCM_RATE_CONTINUOUS should now really be safe. As for Matt's ugly hack: messing with the constraints setup by a driver by directly modifying it's private data structures was never allowed. ALSA drivers should only use the snd_pcm_hw_constraint* methods to add additional constrains. Other than that I'm pretty sure that this hack still works as the checks in hw_params are now removed... But I'd really like to see his driver implemented properly, as Mark Brown and I told him on the ALSA mailing list.

I understand that this patch is trying to pretend that the BCM chip is a time division multiple access chip (TDM) which is used in multichannel sound cards. I can't imagine it would float with ALSA devs, because the BCM chipset is strictly stereo, not multichannel TDM. If you we want to keep the bcm i2s driver closely in line with the upstream system, then this very big modification would swing us far away from their current point. I am flagging that it may make future cherry picks from the mainline kernel difficult to integrate. thanks Matt

HiassofT · 2017-05-01T09:58:27Z

@flatmax I plan to upstream these patches, just wanted to add it here first to gain some feedback and testing.

bcm2835 will work perfectly well in TDM setups and can be extended to DSP mode as well. I didn't have the time yet to implement and test DSP mode clocking but once I finished that it'll be possible to eg use bcm2835 as a DSP mode slave in an 8 slot setup. The only limitation is that bcm2835 is restricted to exactly 2 active slots.

That limitation isn't too bad, the whole idea behind TDM is to hook up multiple (slave) devices, each handling a different part of the audio data stream - eg have 4 stereo DACs connected to a DSP with 8-channel (DSP mode) output.

TDM support for bcm2835 means you'll be able to do this with RPis as well, eg connect 4 RPis tp an 8-channel source.

flatmax · 2017-05-01T13:40:06Z

On 01/05/17 19:58, Matthias Reichl wrote: @flatmax <https://github.com/flatmax> I plan to upstream these patches, just wanted to add it here first to gain some feedback and testing. bcm2835 will work perfectly well in TDM setups and can be extended to DSP mode as well. I didn't have the time yet to implement and test DSP mode clocking but once I finished that it'll be possible to eg use bcm2835 as a DSP mode slave in an 8 slot setup. The only limitation is that bcm2835 is restricted to exactly 2 active slots. That limitation isn't too bad, the whole idea behind TDM is to hook up multiple (slave) devices, each handling a different part of the audio data stream - eg have 4 stereo DACs connected to a DSP with 8-channel (DSP mode) output. TDM support for bcm2835 means you'll be able to do this with RPis as well, eg connect 4 RPis tp an 8-channel source.

This is a good idea. Have you confirmed that these patches don't effect the bit clock offsets for L and R channels ? I would like to make sure that the wm8731 machine driver with a 12 MHz crystal doesn't get effected. You can see that it heavily relies on setting bclk ratios in the snd_audioinjector_pi_soundcard_hw_params functionhere : https://github.com/raspberrypi/linux/blob/rpi-4.9.y/sound/soc/bcm/audioinjector-pi-soundcard.c Secondly, I would like to do a test of this code against the audio injector Octo system which relies on DMA mapping consistency to make sure the 8 channels of audio don't jump around. thanks Matt

HiassofT · 2017-05-01T17:34:50Z

@flatmax thanks a lot for your feedback! I'd missed that there are actually codecs out there that require an odd bclk ratio. I've updated the commit so that these will now work exactly as before. Please retest with the updated PR.

flatmax · 2017-05-02T04:54:25Z

On 02/05/17 03:35, Matthias Reichl wrote: @flatmax <https://github.com/flatmax> thanks a lot for your feedback! I'd missed that there are actually codecs out there that require an odd bclk ratio. I've updated the commit so that these will now work exactly as before. Please retest with the updated PR.

I am overseas at the moment, but will be back in action and able to test these patches next week. Matt

HiassofT · 2017-05-05T14:32:07Z

Just noticed that for supporting right-justified mode I'll have to store the original slot/channel lengths. I'll refactor my code and push a new version when I'm ready. Please wait with merging until then.

bcm2835 supports arbitrary positioning of channel data within a frame and thus is capable of supporting TDM modes. Since the driver is limited to 2-channel operations only TDM setups with exactly 2 active slots are supported. Logical TDM slot numbering follows the usual convention: For I2S-like modes, with a 50% duty-cycle frame clock, slots 0, 2, ... are transmitted in the first half of a frame, slots 1, 3, ... are transmitted in the second half. For DSP modes slot numbering is ascending: 0, 1, 2, 3, ... Channel position calculation has been refactored to use TDM info and moved out of hw_params. set_tdm_slot, set_bclk_ratio and hw_params now check more strictly if the configuration is valid. Illegal configurations like odd number of slots in I2S mode, data lengths exceeding slot width or frame sizes larger than the hardware limit of 1024 are rejected. Also hw_params now properly checks for errors from clk_set_rate. Allowed PCM formats are already guarded by stream constraints, thus the formats check in hw_params has been removed and data_length is now retrieved via params_width(). Also standard functions like snd_soc_params_to_bclk are now being used instead of manual calculations to make the code more readable. Special care has been taken to ensure that set_bclk_ratio works as before. The bclk ratio is mapped to a 2-channel TDM config with a slot width of half the ratio. In order to support odd ratios, which can't be expressed via a TDM config, the ratio (frame length) is stored and used by hw_params. Signed-off-by: Matthias Reichl <hias@horus.com>

DSP modes and left/right justified modes can be supported on bcm2835 by configuring the frame sync polarity and frame sync length registers and by adjusting the channel data position registers. Clock and frame sync polarity handling in hw_params has been refactored to make the interaction between logical rising/falling edge frame start and physical configuration (changed by normal/inverted polarity modes) clearer. Modes where the first active data bit is transmitted immediately after frame start (eg DSP mode B with slot 0 active) only work reliable if bcm2835 is configured as frame master. In frame slave mode channel swap (or shift, this isn't quite clear yet) can occur. Currently the driver only warns if an unstable configuration is detected but doensn't prevent using them. Signed-off-by: Matthias Reichl <hias@horus.com>

Sample rates are only restricted by the capabilities of the clock driver, so use SNDRV_PCM_RATE_CONTINUOUS instead of SNDRV_PCM_RATE_8000_192000. Tests (eg with pcm5122) have shown that bcm2835 works fine in 384kHz/32bit stereo mode, so change the maximum allowed rate from 192kHz to 384kHz. Signed-off-by: Matthias Reichl <hias@horus.com>

bcm2835's configuration registers can't be changed when a stream is running, which means asymmetric configurations aren't supported. Channel and rate symmetry are already enforced by constraints but samplebits had been missed. As hw_params doesn't check for symmetry constraints by itself and just returns success if a stream is running this led to situations where asymmetric configurations were seeming to succeed but of course didn't work because the hardware wasn't configured at all. Fix this by adding the missing samplerate symmetry constraint. Signed-off-by: Matthias Reichl <hias@horus.com>

HiassofT · 2017-05-07T16:21:44Z

I've completely refactored my TDM code, position calculation is now done in hw_params as before (with the use of helper functions).

This was necessary so that logical TDM slot numbering could be mapped in the expected way (0 1 2 3... for DSP modes, 0 2 ... 1 3 ... for I2S and left/right justified modes).

I've also implemented support for left/right justified and DSP A and B modes.

I found out why I sometimes got channel swap (or shift) before: this happens when bcm2835 is configured as a frame slave and data has to be transmitted immediately after frame start (eg in DSP mode B with active slot 0). The driver detects such configurations and warns about them.

BTW: it's not unusual for this configuration to fail, wm5102 datasheet also mentions that this is unstable and the driver errors out in such conditions.

Finally, I added a patch that adds the missing symmetry constraint. Looks like I forgot to PR/upstream it before.

I've tested TDM mode (4 slots of 32 bit each) in left-justified, I2S, DSP-A and DSP-B modes, in master and slave configs (but not all combinations of these) with the Cirrus logic audio card and it worked as expected.

I've also included debug printks in the driver and verified that channel positioning and calling set_bclk_ratio with odd numbers works as before.

Please review and test again.

flatmax · 2017-05-10T10:29:20Z

Hi there, I have begun the test process for this. Cloned your repo Matthias, checked out the branch : git clone --depth=1 https://github.com/HiassofT/rpi-linux.git -b i2s-tdm-samplerate-4.9 Matt

…

On 08/05/17 02:21, Matthias Reichl wrote: I've completely refactored my TDM code, position calculation is now done in hw_params as before (with the use of helper functions). This was necessary so that logical TDM slot numbering could be mapped in the expected way (0 1 2 3... for DSP modes, 0 2 ... 1 3 ... for I2S and left/right justified modes). I've also implemented support for left/right justified and DSP A and B modes. I found out why I sometimes got channel swap (or shift) before: this happens when bcm2835 is configured as a frame slave and data has to be transmitted immediately after frame start (eg in DSP mode B with active slot 0). The driver detects such configurations and warns about them. BTW: it's not unusual for this configuration to fail, wm5102 datasheet also mentions that this is unstable and the driver errors out in such conditions. Finally, I added a patch that adds the missing symmetry constraint. Looks like I forgot to PR/upstream it before. I've tested TDM mode (4 slots of 32 bit each) in left-justified, I2S, DSP-A and DSP-B modes, in master and slave configs (but not all combinations of these) with the Cirrus logic audio card and it worked as expected. I've also included debug printks in the driver and verified that channel positioning and calling set_bclk_ratio with odd numbers works as before. Please review and test again. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1982 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq6B6pu2Emcx6S-d0BxgxA5-b2nIGTsks5r3e-hgaJpZM4NMtfw>.

flatmax · 2017-05-12T12:52:12Z

I have done a listening test with the WM8731 codec - which has strange bit offsets - and it sounds good. I would like to also test the Octo multichannel sound card (early next week or on the weekend) and do a second sanity check. But so far so good. Matt

…

On 10/05/17 20:29, Matt Flax wrote: Hi there, I have begun the test process for this. Cloned your repo Matthias, checked out the branch : git clone --depth=1 https://github.com/HiassofT/rpi-linux.git -b i2s-tdm-samplerate-4.9 Matt On 08/05/17 02:21, Matthias Reichl wrote: > > I've completely refactored my TDM code, position calculation is now > done in hw_params as before (with the use of helper functions). > > This was necessary so that logical TDM slot numbering could be mapped > in the expected way (0 1 2 3... for DSP modes, 0 2 ... 1 3 ... for > I2S and left/right justified modes). > > I've also implemented support for left/right justified and DSP A and > B modes. > > I found out why I sometimes got channel swap (or shift) before: this > happens when bcm2835 is configured as a frame slave and data has to > be transmitted immediately after frame start (eg in DSP mode B with > active slot 0). The driver detects such configurations and warns > about them. > > BTW: it's not unusual for this configuration to fail, wm5102 > datasheet also mentions that this is unstable and the driver errors > out in such conditions. > > Finally, I added a patch that adds the missing symmetry constraint. > Looks like I forgot to PR/upstream it before. > > I've tested TDM mode (4 slots of 32 bit each) in left-justified, I2S, > DSP-A and DSP-B modes, in master and slave configs (but not all > combinations of these) with the Cirrus logic audio card and it worked > as expected. > > I've also included debug printks in the driver and verified that > channel positioning and calling set_bclk_ratio with odd numbers works > as before. > > Please review and test again. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1982 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAq6B6pu2Emcx6S-d0BxgxA5-b2nIGTsks5r3e-hgaJpZM4NMtfw>. >

pelwell · 2017-05-12T13:07:13Z

When we have enough positive feedback we'll push the patch to 4.11 to get some testing from Millhouse's LibreElec builds.

@clivem @hifiberry @iqaudio @msperl Any comments?

hifiberry · 2017-05-13T18:50:10Z

Having a generic I2S driver that can be activated using device trees is a really good thing. It would be cool to have a method to add ALSA controls that can run in user space. That would simplify development of drivers that might just add a simple volume control and otherwise run completely using the generic driver. Does anybody know if this has been done before?

HiassofT · 2017-05-14T12:33:23Z

@hifiberry when using simple-card all codec controls (eg "Digital Playback Volume" on pcm5122) are available in userspace - like with separate machine drivers.

If you need some special setup with your card (eg switching between separate clocks for 44.1/48kHz rate families) or need/want to add controls that are not present in codec drivers (eg "IEC958 Playback Default" for controlling the AES bits on wm8804) this has to be done in a machine driver - simple-card can't handle that.

Software volume control, iec958 pcms etc are added via alsa card configuration files. These are looked up by driver name (with a fallback to ASoC card name if driver_name isn't explicitly set) in /usr/share/alsa/cards so that eg all intel HDA based cards automatically get the softvol+dmix plugins activated for their default PCM.

Similarly it would make sense to enable softvol for the default pcm plus the iec958 pcm eg on all wm8804 based cards.

And this is currently a bit of a mess: instead of having single machine drivers for each of the card families (like in upstream code where eg the intel hda driver handles all intel hda variants) separate ones had been added - of course all with different card and driver names and a lot of duplicated code.

This needs to be refactored, so that there's a single driver for all pcm5122 based cards, another one for all wm8804 based cards etc. Then it's easy to just add a single ALSA card config for the (eg) rpi-wm8804 machine driver and all current and future wm8804 based cards can benefit from that default config.

With simple-card unifying the config is a little bit trickier. As it only has a single name property this means you have to set this to the common card (driver) name if you want to have some default card config applied.

If you want to choose this name freely you either have to add a separate card config (matching the name) for it or add an alias for it to aliases.conf in the ALSA cards directory.

Pillar1989 · 2017-05-24T07:34:28Z

@HiassofT I still not understand how your PR works ? can you share your iqaudio-dac (pcm5122 codec) DT source code ?

Pillar1989 · 2017-05-24T10:26:01Z

Refer to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8b5e2e396b589119bcc9c6a382a999e0202bae18.
You can set channel_max > 2, why your code still keep 2 ?

HiassofT · 2017-05-24T12:42:40Z

@Pillar1989 here's the pcm5122 overlay I used for testing set_tdm_slot
https://gist.github.com/HiassofT/80fdb744f77dfc58c4cb2db876bb0e9a

As for channels: no, we can't set channels_max to > 2. The bcm2835 hardware only has 2 channel position registers so we can't support more than that. The blackfin hardware you referred to supports 8 channels, so this is quite different.

Pillar1989 · 2017-05-24T23:50:37Z

So we cannot set dai-tdm-slot-num > 2 ？

HiassofT · 2017-05-25T11:16:09Z

You can set dai-tdm-slot-num in DT to values greater than 2 but then you also have to set dai-tdm-slot-tx-mask and dai-tdm-slot-rx-mask to exactly 2 active channels - this is what the hardware can support.

eg:

dai-tdm-slot-num = <4>;
dai-tdm-slot-width = <32>;
dai-tdm-slot-tx-mask = <0 0 1 1>;
dai-tdm-slot-rx-mask = <1 0 0 1>;

Using more than 2 tdm slots and selecting which slots should be active will be more useful if you setup the config in a machine driver. Then you can implement runtime slot selection via alsa controls calling snd_soc_dai_set_tdm_slot.

Pillar1989 · 2017-05-26T14:22:47Z

dai-tdm-slot-num and dai-tdm-slot-width are easy to understand. What's the dai-tdm-slot-tx-mask means ? how to set the value ??

HiassofT · 2017-05-28T14:50:35Z

These are bitmasks that configure the time-slot allocation for the device - 1 bit per slot, bit 0 is slot 0. eg if you have a setup with 4 time slots and 2 stereo devices and want to configure one of that devices to use slots 0+1 and the other one to use slots 2+3 you set mask of the first device to 0011b/0x03 and the mask of the second one to 1100b/0x0c

Pillar1989 · 2017-06-07T09:05:20Z

@HiassofT I have two 4 channel ADC codec(support I2S TDM), I want to record 8 channel data. But I see your code:

		/*
		 * The driver is limited to 2-channel setups.
		 * Check that exactly 2 bits are set in the masks.
		 */
		if (hweight_long((unsigned long) rx_mask) != 2
		    || hweight_long((unsigned long) tx_mask) != 2)
			return -EINVAL;

How should I set the dai-tdm-slot-rx-mask ??

Pillar1989 · 2017-06-07T09:54:09Z

My DT setting

                dailink0_slave: seeed-voicecard,cpu {
                    sound-dai = <&i2s>;
                    dai-tdm-slot-num = <8>;
                    dai-tdm-slot-width = <16>;
                    dai-tdm-slot-tx-mask = <0 0 1 1 0 0 0 0>;
                    dai-tdm-slot-rx-mask = <0 0 0 0 0 0 1 1>;                    
                };
                codec_dai: seeed-voicecard,codec {
                    sound-dai = <&ac108_a>;
                    clocks =  <&ac108_mclk>;
                    clock-id = <1>;
                };

How can I use arecord tool record 8 channel data together ??
You can see:
pi@raspberrypi:~/seeed-voicecard $ arecord --dump-hw-params
Recording WAVE 'stdin' : Unsigned 8 bit, Rate 8000 Hz, Mono
HW Params of device "default":
ACCESS: MMAP_INTERLEAVED RW_INTERLEAVED
FORMAT: S16_LE S24_LE S32_LE
SUBFORMAT: STD
SAMPLE_BITS: [16 32]
FRAME_BITS: [32 64]
CHANNELS: 2
RATE: [8000 96000]
PERIOD_TIME: (333 8192000]
PERIOD_SIZE: [32 65536]
PERIOD_BYTES: [256 524288]
PERIODS: [2 4096]
BUFFER_TIME: (666 16384000]
BUFFER_SIZE: [64 131072]
BUFFER_BYTES: [256 524288]
TICK_TIME: ALL
arecord: set_params:1233: Sample format non available
Available formats:

S16_LE
S24_LE
S32_LE

Pillar1989 · 2017-06-07T11:48:06Z

Is it possible to relax the ch2 register setting for 8 channels?? @HiassofT @flatmax

HiassofT · 2017-06-16T10:48:01Z

@Pillar1989 relaxing channel constraints is not the purpose of this PR. The purpose of this PR is to implement modes and functions that are documented in the BCM2835 datasheet and have been verified to work correctly.

I've been thinking a while if and if yes how the channel constraint could be relaxed (which @flatmax is currently doing with a hack). But I haven't fully decided about this yet as there are several issues to take care of:

First of all I didn't have the time yet to do in-depth tests with more than 2 active channels - I only did a few quick tests some 2 or 3 months ago.
It looks like despite what's written in the datasheet the frame length register is used in frame slave mode as well and bcm2835 keeps transmitting additional data instead of waiting for the next frame sync edge. So, basically, Matt's implementation seems to rely on undocumented behaviour.
Matt's implementation also needs special hardware and software (to get the clocking right), so just hooking up some multi-channel codec won't work.
Simply relaxing the channel constraint is going to break several implementations as non-working multi-channel-capability will be exposed to userspace for all sound cards that use a multi-channel-capable codec. So we'd have to make the relaxation strictly opt-in, eg by installing specific constraints in .startup depending on tdm slot numbers and explicitly checking for allowed/supported configurations.

As these are rather severe issues and not easy to solve the best solution IMHO for now is to make sure Matt's hack keeps working (which it still should with this PR). If you want multi-channel support and can accept the cost/restrictions they come at, use the same approach as Matt.

HiassofT · 2017-06-16T10:52:36Z

@flatmax did you have the time yet to test this PR with your octo card? I think the channel restriction hack in your machine driver should still work but it would be good if you could verify that.

flatmax · 2017-06-16T12:23:39Z

Yes I agree. I will make time this coming week. Matt

…

On 16/06/17 20:52, Matthias Reichl wrote: @flatmax <https://github.com/flatmax> did you have the time yet to test this PR with your octo card? I /think/ the channel restriction hack in your machine driver should still work but it would be good if you could verify that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1982 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq6B4x8OPjUj2mTDs8y22fsm8vgJtiIks5sEl6AgaJpZM4NMtfw>.

Pillar1989 · 2017-06-16T12:27:24Z

@flatmax what's your special hardware and software (to get the clocking right). Maybe my hardware clocking is same as yours

flatmax · 2017-07-04T10:06:47Z

@HiassofT just tested tonight, looking good !
I played/recorded a tune in loopback with 8 channels and it sounded good.

Matt

HiassofT · 2017-07-04T10:43:23Z

@flatmax thanks a lot for testting!

@pelwell if you prefer, pick the changes into 4.11 and 4.12 before merging to 4.9 to get some additional test coverage via milhouse LibreELEC builds. I'm rather confident the changes won't break anything so merging to 4.9/11/12 now should be safe as well.

pelwell · 2017-07-04T16:30:21Z

These commits are now in rpi-4.11.y and rpi-4.12.y, so they'll be picked up by Milhouse's nightly builds.

HiassofT · 2017-07-28T07:21:24Z

Testing in Milhouse builds worked fine, no issues reported during the last 3.5 weeks.

@pelwell care to merge?

pelwell · 2017-07-28T08:24:31Z

Thanks, @HiassofT.

kernel: bcm2835-i2s: add support for set_tdm_slot and 384kHz sample rates See: raspberrypi/linux#1982

HiassofT · 2017-11-10T22:46:15Z

@pelwell @popcornmix just got the notification that all changes were accepted upstream, so bcm2835-i2s in 4.15 should be on par with our downstream version
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/127115.html
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/bcm/bcm2835-i2s.c?h=for-next

git history is slightly different, as 8a6923d removes the code block added by 8911a63 I've dropped the latter that in the upstream commit. If git complains when rebasing, just drop all local commits and use the upstream version

pelwell · 2017-11-11T14:40:38Z

Am I right in thinking that the upstream code won't support the 8-channel usage permitted by 8911a63? If so, we'll need to find a workaround because this was a useful feature.

HiassofT · 2017-11-11T14:57:28Z

The 8-channel hack will still work, upstream bcm2835-i2s.c will be 100% identical to our current downstream version

In case the hack should break some time in the future I'd like to see a proper implementation as I outlined here https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=193550&start=25#p1219382

pelwell · 2017-11-11T15:43:00Z

That's good to know - thanks.

HiassofT mentioned this pull request Apr 30, 2017

New driver for the G2 labs TDA1543 v2 DAC #1973

Closed

pelwell reviewed May 1, 2017

View reviewed changes

HiassofT force-pushed the i2s-tdm-samplerate-4.9 branch from ac8c7e1 to 7d660dc Compare May 1, 2017 08:17

HiassofT force-pushed the i2s-tdm-samplerate-4.9 branch from 7d660dc to f4c6e7a Compare May 1, 2017 17:32

HiassofT added 4 commits May 7, 2017 17:10

HiassofT force-pushed the i2s-tdm-samplerate-4.9 branch from f4c6e7a to 50d3f8f Compare May 7, 2017 16:05

pelwell merged commit b9f2f5b into raspberrypi:rpi-4.9.y Jul 28, 2017

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Jul 30, 2017

kernel: Bump to 4.9.40

92a16f1

kernel: bcm2835-i2s: add support for set_tdm_slot and 384kHz sample rates See: raspberrypi/linux#1982

popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Jul 30, 2017

kernel: Bump to 4.9.40

3202f1b

kernel: bcm2835-i2s: add support for set_tdm_slot and 384kHz sample rates See: raspberrypi/linux#1982

HiassofT deleted the i2s-tdm-samplerate-4.9 branch September 5, 2017 09:59

bcm2835-i2s: add support for set_tdm_slot and 384kHz sample rates #1982

bcm2835-i2s: add support for set_tdm_slot and 384kHz sample rates #1982

Uh oh!

Conversation

HiassofT commented Apr 30, 2017

Uh oh!

clivem commented Apr 30, 2017

Uh oh!

pelwell May 1, 2017

Choose a reason for hiding this comment

Uh oh!

pelwell May 1, 2017

Choose a reason for hiding this comment

Uh oh!

pelwell May 1, 2017

Choose a reason for hiding this comment

Uh oh!

pelwell May 1, 2017

Choose a reason for hiding this comment

Uh oh!

HiassofT commented May 1, 2017

Uh oh!

flatmax commented May 1, 2017 via email

Uh oh!

flatmax commented May 1, 2017 via email

Uh oh!

HiassofT commented May 1, 2017

Uh oh!

flatmax commented May 1, 2017 via email

Uh oh!

HiassofT commented May 1, 2017

Uh oh!

flatmax commented May 2, 2017 via email

Uh oh!

HiassofT commented May 5, 2017

Uh oh!

HiassofT commented May 7, 2017

Uh oh!

flatmax commented May 10, 2017 via email

Uh oh!

flatmax commented May 12, 2017 via email

Uh oh!

pelwell commented May 12, 2017

Uh oh!

hifiberry commented May 13, 2017

Uh oh!

HiassofT commented May 14, 2017

Uh oh!

Pillar1989 commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pillar1989 commented May 24, 2017

Uh oh!

HiassofT commented May 24, 2017

Uh oh!

Pillar1989 commented May 24, 2017

Uh oh!

HiassofT commented May 25, 2017

Uh oh!

Pillar1989 commented May 26, 2017

Uh oh!

HiassofT commented May 28, 2017 via email

Uh oh!

Pillar1989 commented Jun 7, 2017

Uh oh!

Pillar1989 commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pillar1989 commented Jun 7, 2017

Uh oh!

HiassofT commented Jun 16, 2017

Uh oh!

HiassofT commented Jun 16, 2017

Uh oh!

flatmax commented Jun 16, 2017 via email

Uh oh!

Pillar1989 commented Jun 16, 2017

Uh oh!

flatmax commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pillar1989 commented May 24, 2017 •

edited

Loading

Pillar1989 commented Jun 7, 2017 •

edited

Loading

flatmax commented Jul 4, 2017 •

edited

Loading