On Mon, Oct 5, 2020 at 8:01 PM Maxime Ripard <[email protected]> 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 <[email protected]>
> > ---
> > 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

2020-10-05 18:19:48

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 00/25] ASoC: sun8i-codec: support for AIF2 and AIF3

On Wed, 30 Sep 2020 21:11:23 -0500, Samuel Holland wrote:
> This series adds support the other two AIFs present in the sun8i codec,
> which can be used for codec2codec DAI links.
>
> This series first cleans up the DAPM component driver so there is an
> organized place to put the new widgets. Then it fills out the DAI
> driver, removing assumptions that were made for AIF1 (16 bits, 2
> channels, certain clock inversions). Some new logic is required to
> handle 3 DAIs and the ADC/DAC sharing the same clock. Finally, it adds
> the new DAIs, and hooks them up with DAPM widgets and routes per the
> hardware topology.
>
> [...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/8] ASoC: sun8i-codec: Set up clock tree at probe time
commit: d8f006825ac57e34f7ed5e63a1e16d889dc1508e
[2/8] ASoC: sun8i-codec: Swap module clock/reset dependencies
commit: ed3caa3bd44c9ae1cebeb32d787adc5ed35e29fa
[3/8] ASoC: sun8i-codec: Sort DAPM controls, widgets, and routes
commit: d58b7247087900414aa3e988e70ecba85e06f412
[4/8] ASoC: sun8i-codec: Consistently name DAPM widgets and routes
commit: 7b51f3c7029fab706e7d9ac99f67cbcf8f29beca
[5/8] ASoC: sun8i-codec: Correct DAPM widget types
commit: fc5668f62d089ba69b343f0e80146f5a3bc6fa71
[6/8] ASoC: sun8i-codec: Fix AIF widget channel references
commit: 4ab60cef3149d57fe56add8c60ee7e6d45816f27
[7/8] ASoC: sun8i-codec: Enable AIF mono/stereo control
commit: 18ebd62c30f0380da11d6c86e20b56c771ac1e18
[8/8] ASoC: sun8i-codec: Use snd_soc_dai_get_drvdata
commit: a886990c9525e83146829c7711ce444ff652c98a

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

2020-10-06 04:45:24

by Samuel Holland

[permalink] [raw]

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

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 <[email protected]>
>> ---
>> 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.

>> + 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.

> Maxime

Cheers,
Samuel

2020-10-06 04:47:30

by Samuel Holland

[permalink] [raw]

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

On 10/5/20 8:15 AM, Chen-Yu Tsai wrote:
> On Mon, Oct 5, 2020 at 8:01 PM Maxime Ripard <[email protected]> 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 <[email protected]>
>>> ---
>>> 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

2020-10-08 13:05:30

by Maxime Ripard

[permalink] [raw]

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

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 <[email protected]>
> >> ---
> >> 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

Attachments:

(No filename) (3.96 kB)
signature.asc (235.00 B)
Download all attachments