[alsa-devel] [PATCH v2] ASoC: topology: Fix bclk and fsync inversion in set_link_hw_format()
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Wed Mar 14 19:37:20 CET 2018
On 03/14/2018 04:42 AM, Pan, Xiuli wrote:
>
>
> On 3/14/2018 03:52, Kirill Marinushkin wrote:
>> 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:
>
> Yes, but what I suggest is that if we can not modify the kernel part.
> But just use the value in alsatplg to fix this problem. We actually
> not passed any string in the tplg file, but only some parsed value.
> Could we align the string & value mapping to make this more easier.
>
> Though I know that the value for bclk_master ans fsync_master is very
> confusing.
Xiuli, I was not able to understand any of your points...
Rule #1 is we don't break userspace. If people generated a .tplg file
with an old version of alsatplg or an inversion in the .conf file, then
we have to provide a backwards-compatible solution.
>
>> ~~~~
>> #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.
>
> What I worry about is that we have make hw_cfg->bclk_master usage
> different in kernel part.
> I think we should keep this the same now, the same 0 have different
> meanings.
> Otherwise, why should we check codec_master?
>>>>> @@ -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.
>
> OK, we may add this support later.
>
> Thanks
> Xiuli
>
>> Best Regards,
>> Kirill
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
More information about the Alsa-devel
mailing list