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

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

[Kirill Marinushkin]

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 alsa-lib.

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: linux-kernel@vger.kernel.org
Cc: alsa-devel@alsa-project.org
---
 include/uapi/sound/asoc.h | 16 ++++++++++++++--
 sound/soc/soc-topology.c  | 12 +++++++-----
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index 69c37ecbff7e..f0e5e21efa54 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -160,6 +160,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 */
+
 /*
  * Block Header.
  * This header precedes all object and object arrays below.
@@ -315,8 +327,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/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 01a50413c66f..c5bdc673b195 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1994,13 +1994,15 @@ static void set_link_hw_format(struct snd_soc_dai_link *link,
 			link->dai_fmt |= SND_SOC_DAIFMT_IB_IF;
 
 		/* clock masters */
-		bclk_master = hw_config->bclk_master;
-		fsync_master = hw_config->fsync_master;
-		if (!bclk_master && !fsync_master)
+		bclk_master = (hw_config->bclk_master ==
+			       SND_SOC_TPLG_BCLK_CM);
+		fsync_master = (hw_config->fsync_master ==
+				SND_SOC_TPLG_FSYNC_CM);
+		if (bclk_master && fsync_master)
 			link->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
-		else if (bclk_master && !fsync_master)
-			link->dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
 		else if (!bclk_master && fsync_master)
+			link->dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
+		else if (bclk_master && !fsync_master)
 			link->dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
 		else
 			link->dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;
-- 
2.13.6

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

[Kirill Marinushkin]

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 */
+
 /*
  * 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;
 		}
 
@@ -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;
 		}
 
-- 
2.13.6

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

[Kirill Marinushkin]

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 */
> +
>  /*
>   * 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;
>  		}
>  
> @@ -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;
>  		}
>  

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

[Pan, Xiuli]

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
This will save some change for SOF topology load parse to DSP.
Now we need to modify a few codes too fit this changing.

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.

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

Thanks
Xiuli


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

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

[Kirill Marinushkin]

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:

~~~~
#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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Pan, Xiuli]

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.

> ~~~~
> #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

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

[Pierre-Louis Bossart]

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.

>
>> ~~~~
>> #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

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

[Pan, Xiuli]

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

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

[Kirill Marinushkin]

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

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

[Kirill Marinushkin]

Hello Xiuli, Pierre-Louis,

I want to resend this patch series. May I add "Reviewed-by:" tags with your names?

I think it will help us to have this patch series into upstream.

Best Regards,

Kirill


On 03/19/18 07:25, Kirill Marinushkin wrote:
> 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

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

[Pierre-Louis Bossart]

On 03/26/2018 01:51 PM, Kirill Marinushkin wrote:
> Hello Xiuli, Pierre-Louis,
>
> I want to resend this patch series. May I add "Reviewed-by:" tags with your names?
I am going to test it this week, was traveling on business and Xiuli is 
busy with a release.
Thanks for your patience.
>
> I think it will help us to have this patch series into upstream.
>
> Best Regards,
>
> Kirill
>
>
> On 03/19/18 07:25, Kirill Marinushkin wrote:
>> 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

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

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

[Pan, Xiuli]

On 3/27/2018 06:53, Pierre-Louis Bossart wrote:
>
>
> On 03/26/2018 01:51 PM, Kirill Marinushkin wrote:
>> Hello Xiuli, Pierre-Louis,
>>
>> I want to resend this patch series. May I add "Reviewed-by:" tags 
>> with your names?
> I am going to test it this week, was traveling on business and Xiuli 
> is busy with a release.
> Thanks for your patience.

Sure, we also have some tests with your patches in SOF now.
I did a test and make some changes in SOF about 2 weeks ago, and raised 
some comments to make sure my changes on SOF is necessary.
Now Pierre got the similar changes to SOF and test with your patches on 
one platform.
I just helped him to do testes on our other platforms.

You can also add "Test-by: " now.

Thanks
Xiuli


>>
>> I think it will help us to have this patch series into upstream.
>>
>> Best Regards,
>>
>> Kirill
>>
>>

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

[Pierre-Louis Bossart]

On 03/01/2018 12:53 AM, 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.
alsa-lib patch:
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>
>> 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 */
>> +
>>   /*
>>    * 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;
>>   		}
>>   
>> @@ -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;
>>   		}
>>   

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

[Pierre-Louis Bossart]

On 03/01/2018 12:45 AM, 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 alsa-lib.

kernel/ASoC patch:
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> 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: linux-kernel@vger.kernel.org
> Cc: alsa-devel@alsa-project.org
> ---
>   include/uapi/sound/asoc.h | 16 ++++++++++++++--
>   sound/soc/soc-topology.c  | 12 +++++++-----
>   2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
> index 69c37ecbff7e..f0e5e21efa54 100644
> --- a/include/uapi/sound/asoc.h
> +++ b/include/uapi/sound/asoc.h
> @@ -160,6 +160,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 */
> +
>   /*
>    * Block Header.
>    * This header precedes all object and object arrays below.
> @@ -315,8 +327,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/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 01a50413c66f..c5bdc673b195 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1994,13 +1994,15 @@ static void set_link_hw_format(struct snd_soc_dai_link *link,
>   			link->dai_fmt |= SND_SOC_DAIFMT_IB_IF;
>   
>   		/* clock masters */
> -		bclk_master = hw_config->bclk_master;
> -		fsync_master = hw_config->fsync_master;
> -		if (!bclk_master && !fsync_master)
> +		bclk_master = (hw_config->bclk_master ==
> +			       SND_SOC_TPLG_BCLK_CM);
> +		fsync_master = (hw_config->fsync_master ==
> +				SND_SOC_TPLG_FSYNC_CM);
> +		if (bclk_master && fsync_master)
>   			link->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
> -		else if (bclk_master && !fsync_master)
> -			link->dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
>   		else if (!bclk_master && fsync_master)
> +			link->dai_fmt |= SND_SOC_DAIFMT_CBS_CFM;
> +		else if (bclk_master && !fsync_master)
>   			link->dai_fmt |= SND_SOC_DAIFMT_CBM_CFS;
>   		else
>   			link->dai_fmt |= SND_SOC_DAIFMT_CBS_CFS;