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

["Pan, Xiuli" <xiuli.pan@linux.intel.com>]

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