| Message ID | 20201001021148.15852-21-samuel@sholland.org |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: sun8i-codec: support for AIF2 and AIF3 | expand |
Hi Samuel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on asoc/for-next]
[also build test ERROR on next-20200930]
[cannot apply to sunxi/sunxi/for-next v5.9-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Samuel-Holland/ASoC-sun8i-codec-support-for-AIF2-and-AIF3/20201001-101451
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6a43c578b796aed3cd013c065ef444fa82b24fce
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Samuel-Holland/ASoC-sun8i-codec-support-for-AIF2-and-AIF3/20201001-101451
git checkout 6a43c578b796aed3cd013c065ef444fa82b24fce
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "clk_set_rate_exclusive" [sound/soc/sunxi/sun8i-codec.ko] undefined!
>> ERROR: modpost: "clk_rate_exclusive_put" [sound/soc/sunxi/sun8i-codec.ko] undefined!
ERROR: modpost: "of_clk_add_hw_provider" [sound/soc/qcom/qdsp6/q6afe-clocks.ko] undefined!
ERROR: modpost: "devm_clk_hw_register" [sound/soc/qcom/qdsp6/q6afe-clocks.ko] undefined!
ERROR: modpost: "__delay" [drivers/net/phy/mdio-cavium.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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@sholland.org> > --- > 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? > + 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
On Mon, Oct 5, 2020 at 8:01 PM Maxime Ripard <maxime@cerno.tech> 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@sholland.org> > > --- > > 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
On 10/5/20 7:01 AM, Maxime Ripard 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@sholland.org> >> --- >> 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?
On 10/5/20 8:15 AM, Chen-Yu Tsai wrote: > On Mon, Oct 5, 2020 at 8:01 PM Maxime Ripard <maxime@cerno.tech> 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@sholland.org> >>> --- >>> 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 ... >>> @@ -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? Yes, this is an advantage now. It would not be the case if sun4i-i2s did something similar. It has the same problem that multiple separate sound cards compete for one PLL. > ChenYu Cheers, Samuel
On Mon, Oct 05, 2020 at 11:43:51PM -0500, Samuel Holland wrote: > On 10/5/20 7:01 AM, Maxime Ripard 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@sholland.org> > >> --- > >> 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? > > From Documentation/sound/kernel-api/writing-an-alsa-driver.rst: > > hw_params callback > ... > Note that this and ``prepare`` callbacks may be called multiple > times per initialization. For example, the OSS emulation may call > these callbacks at each change via its ioctl. > > Clock rate protection is reference counted, so we must only take one > reference (or at least a known number of references) per stream. Ah, right. Can you add a comment to make that more obvious? > >> + 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. > > I will put it on one line, though >100 columns is also a checkpatch warning. Yeah, but in general having an error on a single line is more important. That way you can then grep for that error message Maxime
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); + if (ret == -EBUSY) + dev_err(dai->dev, "%s: clock is busy! Sample rate %u Hz " + "conflicts with other audio streams.\n", + dai->name, sample_rate); + if (ret < 0) + return ret; + scodec->sysclk_rate = sysclk_rate; scodec->sysclk_refcnt++; } aif->sample_rate = sample_rate; aif->open_streams |= BIT(substream->stream); return sun8i_codec_update_sample_rate(scodec); @@ -487,16 +505,17 @@ static int sun8i_codec_hw_free(struct snd_pcm_substream *substream, { struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai); struct sun8i_codec_aif *aif = &scodec->aifs[dai->id]; if (aif->open_streams != BIT(substream->stream)) goto done; scodec->sysclk_refcnt--; + clk_rate_exclusive_put(scodec->clk_module); aif->sample_rate = 0; done: aif->open_streams &= ~BIT(substream->stream); return 0; }
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@sholland.org> --- sound/soc/sunxi/sun8i-codec.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)