Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open

[Chen-Yu Tsai]

On Mon, Oct 5, 2020 at 8:01 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> On Wed, Sep 30, 2020 at 09:11:43PM -0500, Samuel Holland wrote:
> > The codec's clock input is shared among all AIFs, and shared with other
> > audio-related hardware in the SoC, including I2S and SPDIF controllers.
> > To ensure sample rates selected by userspace or by codec2codec DAI links
> > are maintained, the clock rate must be protected while it is in use.
> >
> > Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> > ---
> >  sound/soc/sunxi/sun8i-codec.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> > index 501af64d43a0..86065bee7cd3 100644
> > --- a/sound/soc/sunxi/sun8i-codec.c
> > +++ b/sound/soc/sunxi/sun8i-codec.c
> > @@ -416,27 +416,32 @@ static int sun8i_codec_get_lrck_div_order(unsigned int slots,
> >       unsigned int div = slots * slot_width;
> >
> >       if (div < 16 || div > 256)
> >               return -EINVAL;
> >
> >       return order_base_2(div);
> >  }
> >
> > +static unsigned int sun8i_codec_get_sysclk_rate(unsigned int sample_rate)
> > +{
> > +     return sample_rate % 4000 ? 22579200 : 24576000;
> > +}
> > +
> >  static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
> >                                struct snd_pcm_hw_params *params,
> >                                struct snd_soc_dai *dai)
> >  {
> >       struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
> >       struct sun8i_codec_aif *aif = &scodec->aifs[dai->id];
> >       unsigned int sample_rate = params_rate(params);
> >       unsigned int slots = aif->slots ?: params_channels(params);
> >       unsigned int slot_width = aif->slot_width ?: params_width(params);
> > -     unsigned int sysclk_rate = clk_get_rate(scodec->clk_module);
> > -     int lrck_div_order, word_size;
> > +     unsigned int sysclk_rate = sun8i_codec_get_sysclk_rate(sample_rate);
> > +     int lrck_div_order, ret, word_size;
> >       u8 bclk_div;
> >
> >       /* word size */
> >       switch (params_width(params)) {
> >       case 8:
> >               word_size = 0x0;
> >               break;
> >       case 16:
> > @@ -466,17 +471,30 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
> >                          (lrck_div_order - 4) << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
> >
> >       /* BCLK divider (SYSCLK/BCLK ratio) */
> >       bclk_div = sun8i_codec_get_bclk_div(sysclk_rate, lrck_div_order, sample_rate);
> >       regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> >                          SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
> >                          bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
> >
> > -     if (!aif->open_streams) {
> > +     /* SYSCLK rate */
> > +     if (aif->open_streams) {
> > +             ret = clk_set_rate(scodec->clk_module, sysclk_rate);
> > +             if (ret < 0)
> > +                     return ret;
> > +     } else {
> > +             ret = clk_set_rate_exclusive(scodec->clk_module, sysclk_rate);
>
> It's not really clear to me why we wouldn't want to always protect the
> clock rate here?

I believe the intention is to allow a window, i.e. when no audio
blocks are running,
when it is possible to switch between sample rate families?

ChenYu

> > +             if (ret == -EBUSY)
> > +                     dev_err(dai->dev, "%s: clock is busy! Sample rate %u Hz "
> > +                             "conflicts with other audio streams.\n",
>
> This string creates a checkpatch warning.
>
> Maxime