Re: [PATCH] ASoC: bcm2835-i2s: substream alignment now independent in hwparams

[Matthias Reichl <hias@horus.com>]

On Fri, Mar 27, 2020 at 11:30:50AM +1100, Matt Flax wrote:
> 
> On 27/3/20 10:56 am, Matt Flax wrote:
> > 
> > Should this patch be handled through the ALSA team the R. Pi team or the
> > BCM team ?
> > 
> 
> Resending again with reduced recipients.
> 
> 
> > 
> > thanks
> > 
> > Matt
> > 
> > On 24/3/20 8:08 pm, Matt Flax wrote:
> > > Substream sample alignment was being set in hwparams for both
> > > substreams at the same time. This became a problem when    the Audio
> > > Injector isolated sound card needed to offset sample alignment
> > > for high sample    rates. The latency difference between playback
> > > and capture occurs because of the digital isolation chip
> > > propagation time, particularly when the codec is master and
> > > the DAC return is twice delayed.
> > > 
> > > This patch sets sample alignment registers  based on the substream
> > > direction in hwparams. This gives the machine driver more control
> > > over sample alignment in the bcm2835 i2s driver.
> > > 
> > > Signed-off-by: Matt Flax <flatmax@flatmax.org>
> > > ---
> > >   sound/soc/bcm/bcm2835-i2s.c | 36 +++++++++++++++++++-----------------
> > >   1 file changed, 19 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/sound/soc/bcm/bcm2835-i2s.c b/sound/soc/bcm/bcm2835-i2s.c
> > > index e6a12e271b07..9db542699a13 100644
> > > --- a/sound/soc/bcm/bcm2835-i2s.c
> > > +++ b/sound/soc/bcm/bcm2835-i2s.c
> > > @@ -493,11 +493,6 @@ static int bcm2835_i2s_hw_params(struct
> > > snd_pcm_substream *substream,
> > >           return -EINVAL;
> > >       }
> > >   -    bcm2835_i2s_calc_channel_pos(&rx_ch1_pos, &rx_ch2_pos,
> > > -        rx_mask, slot_width, data_delay, odd_slot_offset);
> > > -    bcm2835_i2s_calc_channel_pos(&tx_ch1_pos, &tx_ch2_pos,
> > > -        tx_mask, slot_width, data_delay, odd_slot_offset);
> > > -
> > >       /*
> > >        * Transmitting data immediately after frame start, eg
> > >        * in left-justified or DSP mode A, only works stable
> > > @@ -508,19 +503,26 @@ static int bcm2835_i2s_hw_params(struct
> > > snd_pcm_substream *substream,
> > >               "Unstable slave config detected, L/R may be swapped");
> > >         /*
> > > -     * Set format for both streams.
> > > -     * We cannot set another frame length
> > > -     * (and therefore word length) anyway,
> > > -     * so the format will be the same.
> > > +     * Set format on a per stream basis.
> > > +     * The alignment format can be different depending on direction.
> > >        */
> > > -    regmap_write(dev->i2s_regmap, BCM2835_I2S_RXC_A_REG,
> > > -          format
> > > -        | BCM2835_I2S_CH1_POS(rx_ch1_pos)
> > > -        | BCM2835_I2S_CH2_POS(rx_ch2_pos));
> > > -    regmap_write(dev->i2s_regmap, BCM2835_I2S_TXC_A_REG,
> > > -          format
> > > -        | BCM2835_I2S_CH1_POS(tx_ch1_pos)
> > > -        | BCM2835_I2S_CH2_POS(tx_ch2_pos));
> > > +    if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > > +        bcm2835_i2s_calc_channel_pos(&rx_ch1_pos, &rx_ch2_pos,
> > > +            rx_mask, slot_width, data_delay, odd_slot_offset);
> > > +        regmap_write(dev->i2s_regmap, BCM2835_I2S_RXC_A_REG,
> > > +              format
> > > +            | BCM2835_I2S_CH1_POS(rx_ch1_pos)
> > > +            | BCM2835_I2S_CH2_POS(rx_ch2_pos));
> > > +    }
> > > +
> > > +    if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > > +        bcm2835_i2s_calc_channel_pos(&tx_ch1_pos, &tx_ch2_pos,
> > > +            tx_mask, slot_width, data_delay, odd_slot_offset);
> > > +        regmap_write(dev->i2s_regmap, BCM2835_I2S_TXC_A_REG,
> > > +              format
> > > +            | BCM2835_I2S_CH1_POS(tx_ch1_pos)
> > > +            | BCM2835_I2S_CH2_POS(tx_ch2_pos));
> > > +    }
> > >         /* Setup the I2S mode */

This will break duplex operation if a second stream is opened when
a stream is already running as the channel position registers for
the second stream haven't been set up.

Note this code at the very beginning of hw_params:

        /*
         * If a stream is already enabled,
         * the registers are already set properly.
         */
        regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);

        if (csreg & (BCM2835_I2S_TXON | BCM2835_I2S_RXON))
                return 0;

The reason for this check is that we can't change bcm2835 I2S registers
after I2S RX/TX has been enabled - the reason why is explained in the
datasheet:

> The PCM interface runs asynchronously at the PCM_CLK rate and
> automatically transfers transmit and receive data across to the
> internal APB clock domain. The control registers are NOT
> synchronised and should be programmed before the device is enabled
> and should NOT be changed whilst the interface is running.
> 
> Only the EN, RXON and TXON bits of the PCMCS register are synchronised
> across the PCM - APB clock domain and are allowed to be changed whilst
> the interface is running.

Therefore we need to set up channel masks for both RX and TX before
any stream is started.

so long,

Hias