Re: [PATCH v2] ASoC: topology: Fix bclk and fsync inversion in set_link_hw_format()

[Kirill Marinushkin <k.marinushkin@gmail.com>]

Hello Mark, Takashi.

This patch series fixes an issue which causes troubles at Sound Open Firmware.
It is already reviewed and agreed by the SOF engineers
Pan, Xiuli <xiuli.pan@linux.intel.com> and Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Please have a look.

Notice: this patch series includes 1 patch for linux + 1 patch for alsa-lib.

Best Regards,
Kirill

On 03/15/18 04:01, Pan, Xiuli wrote:
>
>
> On 3/15/2018 02:37, Pierre-Louis Bossart wrote:
>>
>>
>> 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@gmail.com>
>>>>>>> Cc: Mark Brown <broonie@kernel.org>
>>>>>>> Cc: Takashi Iwai <tiwai@suse.de>
>>>>>>> Cc: Jaroslav Kysela <perex@perex.cz>
>>>>>>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>>>>> Cc: Pan Xiuli <xiuli.pan@linux.intel.com>
>>>>>>> Cc: Liam Girdwood <liam.r.girdwood@linux.intel.com>
>>>>>>> Cc: alsa-devel@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.
>>
>
> OK, I got the point. We must make enough changes for the old version files to be accepted.
> The SOF need some patches to work with these patches. I will send them once these are merged in.
>
> Thanks
> Xiuli
>
>>>
>>>> ~~~~
>>>> #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@alsa-project.org
>>>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>>
>>
>

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel