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

[Matthias Reichl <hias@horus.com>]

On Sat, Mar 28, 2020 at 08:50:52AM +1100, Matt Flax wrote:
> 
> On 28/3/20 12:23 am, Matthias Reichl wrote:
> > 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.
> 
> 
> I see what you mean. We can't change the registers once the system has
> started half duplex and then subsequently changed to full duplex.
> 
> There are cases however where playback and capture need to be set
> independently. In these cases the machine driver requires different format
> settings based on the stream direction.
> 
> What if we make a check for whether the system is already running and in
> that case return an error - forcing the user to use specify the same dai_fmt
> which is already in use before continuing ?

I'm not sure if I can follow you. dai_fmt, as the name implies, sets
the format of the DAI - you can't have different DAI formats for
playback/capture active at the same time.

This sounds a bit like you may be trying to work around some hardware
or codec configuration issue by creative use of the API.

> Would there be a better way to achieve different hwparams based on stream
> direction ?

If you really need different DAI formats for playback/capture it's
best to disallow full-duplex mode and set the DAI format based on
stream direction in the machine driver.

so long,

Hias

> 
> Matt
>