[alsa-devel] [PATCH v2] ASoC: topology: Fix bclk and fsync inversion in set_link_hw_format()
Kirill Marinushkin
k.marinushkin at gmail.com
Tue Mar 13 20:52:35 CET 2018
On 03/13/18 07:57, Pan, Xiuli wrote:
>
>
> On 3/1/2018 14:53, Kirill Marinushkin wrote:
>> This patch is for alsa-lib. I forgot to mention it in the subject.
>>
>>
>> On 03/01/18 07:48, Kirill Marinushkin wrote:
>>> The values of bclk and fsync are inverted WRT the codec. But the existing
>>> solution already works for Broadwell, see the alsa-lib config:
>>>
>>> `alsa-lib/src/conf/topology/broadwell/broadwell.conf`
>>>
>>> This commit provides the backwards-compatible solution to fix this misuse.
>>> This commit goes in pair with the corresponding patch for linux.
>>>
>>> Signed-off-by: Kirill Marinushkin <k.marinushkin at gmail.com>
>>> Cc: Mark Brown <broonie at kernel.org>
>>> Cc: Takashi Iwai <tiwai at suse.de>
>>> Cc: Jaroslav Kysela <perex at perex.cz>
>>> Cc: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
>>> Cc: Pan Xiuli <xiuli.pan at linux.intel.com>
>>> Cc: Liam Girdwood <liam.r.girdwood at linux.intel.com>
>>> Cc: alsa-devel at alsa-project.org
>>> ---
>>> include/sound/asoc.h | 16 ++++++++++++++--
>>> include/topology.h | 4 ++--
>>> src/conf/topology/broadwell/broadwell.conf | 4 ++--
>>> src/topology/pcm.c | 30 ++++++++++++++++++++++++++----
>>> 4 files changed, 44 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/sound/asoc.h b/include/sound/asoc.h
>>> index 0f5d9f9a..89b00703 100644
>>> --- a/include/sound/asoc.h
>>> +++ b/include/sound/asoc.h
>>> @@ -156,6 +156,18 @@
>>> #define SND_SOC_TPLG_LNK_FLGBIT_SYMMETRIC_SAMPLEBITS (1 << 2)
>>> #define SND_SOC_TPLG_LNK_FLGBIT_VOICE_WAKEUP (1 << 3)
>>> +/* DAI topology BCLK parameter
>>> + * For the backwards capability, by default codec is bclk master
>>> + */
>>> +#define SND_SOC_TPLG_BCLK_CM 0 /* codec is bclk master */
>>> +#define SND_SOC_TPLG_BCLK_CS 1 /* codec is bclk slave */
>>> +
>>> +/* DAI topology FSYNC parameter
>>> + * For the backwards capability, by default codec is fsync master
>>> + */
>>> +#define SND_SOC_TPLG_FSYNC_CM 0 /* codec is fsync master */
>>> +#define SND_SOC_TPLG_FSYNC_CS 1 /* codec is fsync slave */
>>> +
>>> /*
>
> Could we keep this align with the old values.
>
> 1 for master of BCLK, 0 for slave
> 1 for master of FSYNC, 0 for slave
We cannot keep it aligned with the old *comment descriptions*, because we keep it aligned with the old *implementation*.
As you remember, the whole issue was with the reverted values.
We exactly keep our new solution aligned to the old solution by doing this:
~~~~
#define SND_SOC_TPLG_FSYNC_CM 0 /* codec is fsync master */
#define SND_SOC_TPLG_FSYNC_CS 1 /* codec is fsync slave */
~~~~
>
> This will save some change for SOF topology load parse to DSP.
> Now we need to modify a few codes too fit this changing.
>
See above why we can't follow this proposal.
> And to make things more easier. We can just keep the old values and only modify alsa-lib to let "master" == "codec_slave".
> Here we only need to change how we write tplg and no more kernel patches are need.
>
>>> * Block Header.
>>> * This header precedes all object and object arrays below.
>>> @@ -311,8 +323,8 @@ struct snd_soc_tplg_hw_config {
>>> __u8 clock_gated; /* 1 if clock can be gated to save power */
>>> __u8 invert_bclk; /* 1 for inverted BCLK, 0 for normal */
>>> __u8 invert_fsync; /* 1 for inverted frame clock, 0 for normal */
>>> - __u8 bclk_master; /* 1 for master of BCLK, 0 for slave */
>>> - __u8 fsync_master; /* 1 for master of FSYNC, 0 for slave */
>>> + __u8 bclk_master; /* SND_SOC_TPLG_BCLK_ value */
>>> + __u8 fsync_master; /* SND_SOC_TPLG_FSYNC_ value */
>>> __u8 mclk_direction; /* 0 for input, 1 for output */
>>> __le16 reserved; /* for 32bit alignment */
>>> __le32 mclk_rate; /* MCLK or SYSCLK freqency in Hz */
>>> diff --git a/include/topology.h b/include/topology.h
>>> index 8779da4d..5d7b46df 100644
>>> --- a/include/topology.h
>>> +++ b/include/topology.h
>>> @@ -1000,8 +1000,8 @@ struct snd_tplg_hw_config_template {
>>> unsigned char clock_gated; /* 1 if clock can be gated to save power */
>>> unsigned char invert_bclk; /* 1 for inverted BCLK, 0 for normal */
>>> unsigned char invert_fsync; /* 1 for inverted frame clock, 0 for normal */
>>> - unsigned char bclk_master; /* 1 for master of BCLK, 0 for slave */
>>> - unsigned char fsync_master; /* 1 for master of FSYNC, 0 for slave */
>>> + unsigned char bclk_master; /* SND_SOC_TPLG_BCLK_ value */
>>> + unsigned char fsync_master; /* SND_SOC_TPLG_FSYNC_ value */
>>> unsigned char mclk_direction; /* 0 for input, 1 for output */
>>> unsigned short reserved; /* for 32bit alignment */
>>> unsigned int mclk_rate; /* MCLK or SYSCLK freqency in Hz */
>>> diff --git a/src/conf/topology/broadwell/broadwell.conf b/src/conf/topology/broadwell/broadwell.conf
>>> index b8405d93..09fc4daa 100644
>>> --- a/src/conf/topology/broadwell/broadwell.conf
>>> +++ b/src/conf/topology/broadwell/broadwell.conf
>>> @@ -393,8 +393,8 @@ SectionGraph."dsp" {
>>> SectionHWConfig."CodecHWConfig" {
>>> id "1"
>>> format "I2S" # physical audio format.
>>> - bclk "master" # Platform is master of bit clock
>>> - fsync "master" # platform is master of fsync
>>> + bclk "codec_slave" # platform is master of bit clock (codec is slave)
>>> + fsync "codec_slave" # platform is master of fsync (codec is slave)
>>> }
>>> SectionLink."Codec" {
>>> diff --git a/src/topology/pcm.c b/src/topology/pcm.c
>>> index 58cee31d..bdab8eef 100644
>>> --- a/src/topology/pcm.c
>>> +++ b/src/topology/pcm.c
>>> @@ -1137,8 +1137,19 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
>>> if (snd_config_get_string(n, &val) < 0)
>>> return -EINVAL;
>>> - if (!strcmp(val, "master"))
>>> - hw_cfg->bclk_master = true;
>>> + if (!strcmp(val, "master")) {
>>> + /* For backwards capability,
>>> + * "master" == "codec is slave"
>>> + */
>>> + SNDERR("warning: deprecated bclk value '%s'\n",
>>> + val);
>>> +
>>> + hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CS;
>>> + } else if (!strcmp(val, "codec_slave")) {
>>> + hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CS;
>>> + } else if (!strcmp(val, "codec_master")) {
>>> + hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CM;
>>> + }
>>> continue;
>>> }
> What if some old tplg used "slave"? What should we do to those values.
>
Old behavior: "slave" => not modified (hw_cfg->bclk_master = 0)
New behavior: "slave" => not modified (hw_cfg->bclk_master = 0)
So, we keep this use-case unchanged.
>>> @@ -1163,8 +1174,19 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
>>> if (snd_config_get_string(n, &val) < 0)
>>> return -EINVAL;
>>> - if (!strcmp(val, "master"))
>>> - hw_cfg->fsync_master = true;
>>> + if (!strcmp(val, "master")) {
>>> + /* For backwards capability,
>>> + * "master" == "codec is slave"
>>> + */
>>> + SNDERR("warning: deprecated fsync value '%s'\n",
>>> + val);
>>> +
>>> + hw_cfg->fsync_master = SND_SOC_TPLG_FSYNC_CS;
>>> + } else if (!strcmp(val, "codec_slave")) {
>>> + hw_cfg->fsync_master = SND_SOC_TPLG_FSYNC_CS;
>>> + } else if (!strcmp(val, "codec_master")) {
>>> + hw_cfg->fsync_master = SND_SOC_TPLG_FSYNC_CM;
>>> + }
>>> continue;
>>> }
>>>
> Test with our SOF topology and found a missing part mclk.
> We also use mclk now, and I think these KEY WORDS should align with the new changes, too.
>
mclk is not related to this patch series. Feel free to add it in a separate patch.
Best Regards,
Kirill
More information about the Alsa-devel
mailing list