[PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Tony Lindgren]

For using cpcap for voice calls, we need to route audio directly from
the modem to cpcap for TDM (Time Division Multiplexing). The voice call
is direct data between the modem and cpcap with no CPU involvment. In
this mode, the cpcap related audio mixer controls work for the speaker
selection and volume though.

To do this, we need to implement standard snd_soc_dai_set_tdm_slot()
for cpcap. Then the modem codec driver can use snd_soc_dai_set_sysclk(),
snd_soc_dai_set_fmt(), and snd_soc_dai_set_tdm_slot() to configure a
voice call.

Let's add cpcap_voice_set_tdm_slot() for this, and cpcap_voice_call()
helper to configure the additional registers needed for voice call.

Let's also clear CPCAP_REG_VAUDIOC on init in case we have the bit for
CPCAP_BIT_VAUDIO_MODE0 set on init.

Cc: Arthur D. <spinal.by@gmail.com>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 sound/soc/codecs/cpcap.c | 123 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/sound/soc/codecs/cpcap.c b/sound/soc/codecs/cpcap.c
--- a/sound/soc/codecs/cpcap.c
+++ b/sound/soc/codecs/cpcap.c
@@ -16,6 +16,14 @@
 #include <sound/soc.h>
 #include <sound/tlv.h>
 
+/* Register 512 CPCAP_REG_VAUDIOC --- Audio Regulator and Bias Voltage */
+#define CPCAP_BIT_AUDIO_LOW_PWR           6
+#define CPCAP_BIT_AUD_LOWPWR_SPEED        5
+#define CPCAP_BIT_VAUDIOPRISTBY           4
+#define CPCAP_BIT_VAUDIO_MODE1            2
+#define CPCAP_BIT_VAUDIO_MODE0            1
+#define CPCAP_BIT_V_AUDIO_EN              0
+
 /* Register 513 CPCAP_REG_CC     --- CODEC */
 #define CPCAP_BIT_CDC_CLK2                15
 #define CPCAP_BIT_CDC_CLK1                14
@@ -221,6 +229,7 @@ struct cpcap_reg_info {
 };
 
 static const struct cpcap_reg_info cpcap_default_regs[] = {
+	{ CPCAP_REG_VAUDIOC, 0x003F, 0x0000 },
 	{ CPCAP_REG_CC, 0xFFFF, 0x0000 },
 	{ CPCAP_REG_CC, 0xFFFF, 0x0000 },
 	{ CPCAP_REG_CDI, 0xBFFF, 0x0000 },
@@ -1370,6 +1379,119 @@ static int cpcap_voice_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+/*
+ * Configure codec for voice call if requested.
+ *
+ * We can configure most with snd_soc_dai_set_sysclk(), snd_soc_dai_set_fmt()
+ * and snd_soc_dai_set_tdm_slot(). This function configures the rest of the
+ * cpcap related hardware as CPU is not involved in the voice call.
+ */
+static int cpcap_voice_call(struct cpcap_audio *cpcap, struct snd_soc_dai *dai,
+			    bool voice_call)
+{
+	int mask, err;
+
+	/* Modem to codec VAUDIO_MODE1 */
+	mask = BIT(CPCAP_BIT_VAUDIO_MODE1);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_VAUDIOC,
+				 mask, voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Clear MIC1_MUX for call */
+	mask = BIT(CPCAP_BIT_MIC1_MUX);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_TXI,
+				 mask, voice_call ? 0 : mask);
+	if (err)
+		return err;
+
+	/* Set MIC2_MUX for call */
+	mask = BIT(CPCAP_BIT_MB_ON1L) | BIT(CPCAP_BIT_MB_ON1R) |
+		BIT(CPCAP_BIT_MIC2_MUX) | BIT(CPCAP_BIT_MIC2_PGA_EN);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_TXI,
+				 mask, voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Enable LDSP for call */
+	mask = BIT(CPCAP_BIT_A2_LDSP_L_EN) | BIT(CPCAP_BIT_A2_LDSP_R_EN);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_RXOA,
+				 mask, voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Enable CPCAP_BIT_PGA_CDC_EN for call */
+	mask = BIT(CPCAP_BIT_PGA_CDC_EN);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_RXCOA,
+				 mask, voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Unmute voice for call */
+	if (dai) {
+		err = snd_soc_dai_digital_mute(dai, !voice_call,
+					       SNDRV_PCM_STREAM_PLAYBACK);
+		if (err)
+			return err;
+	}
+
+	/* Set modem to codec mic CDC and HPF for call */
+	mask = BIT(CPCAP_BIT_MIC2_CDC_EN) | BIT(CPCAP_BIT_CDC_EN_RX) |
+	       BIT(CPCAP_BIT_AUDOHPF_1) | BIT(CPCAP_BIT_AUDOHPF_0) |
+	       BIT(CPCAP_BIT_AUDIHPF_1) | BIT(CPCAP_BIT_AUDIHPF_0);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CC,
+				 mask, voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Enable modem to codec CDC for call*/
+	mask = BIT(CPCAP_BIT_CDC_CLK_EN);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CDI,
+				 mask, voice_call ? mask : 0);
+
+	return err;
+}
+
+static int cpcap_voice_set_tdm_slot(struct snd_soc_dai *dai,
+				    unsigned int tx_mask, unsigned int rx_mask,
+				    int slots, int slot_width)
+{
+	struct snd_soc_component *component = dai->component;
+	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
+	int err, ts_mask, mask;
+	bool voice_call;
+
+	/*
+	 * Primitive test for voice call, probably needs more checks
+	 * later on for 16-bit calls detected, Bluetooth headset etc.
+	 */
+	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
+		voice_call = true;
+	else
+		voice_call = false;
+
+	ts_mask = 0x7 << CPCAP_BIT_MIC2_TIMESLOT0;
+	ts_mask |= 0x7 << CPCAP_BIT_MIC1_RX_TIMESLOT0;
+
+	mask = (tx_mask & 0x7) << CPCAP_BIT_MIC2_TIMESLOT0;
+	mask |= (rx_mask & 0x7) << CPCAP_BIT_MIC1_RX_TIMESLOT0;
+
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CDI,
+				 ts_mask, mask);
+	if (err)
+		return err;
+
+	err = cpcap_set_samprate(cpcap, CPCAP_DAI_VOICE, slot_width * 1000);
+	if (err)
+		return err;
+
+	err = cpcap_voice_call(cpcap, dai, voice_call);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static int cpcap_voice_set_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_component *component = dai->component;
@@ -1391,6 +1513,7 @@ static const struct snd_soc_dai_ops cpcap_dai_voice_ops = {
 	.hw_params	= cpcap_voice_hw_params,
 	.set_sysclk	= cpcap_voice_set_dai_sysclk,
 	.set_fmt	= cpcap_voice_set_dai_fmt,
+	.set_tdm_slot	= cpcap_voice_set_tdm_slot,
 	.digital_mute	= cpcap_voice_set_mute,
 };
 
-- 
2.25.0

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Peter Ujfalusi]

On 11/02/2020 20.10, Tony Lindgren wrote:
> For using cpcap for voice calls, we need to route audio directly from
> the modem to cpcap for TDM (Time Division Multiplexing). The voice call
> is direct data between the modem and cpcap with no CPU involvment. In
> this mode, the cpcap related audio mixer controls work for the speaker
> selection and volume though.
> 
> To do this, we need to implement standard snd_soc_dai_set_tdm_slot()
> for cpcap. Then the modem codec driver can use snd_soc_dai_set_sysclk(),
> snd_soc_dai_set_fmt(), and snd_soc_dai_set_tdm_slot() to configure a
> voice call.
> 
> Let's add cpcap_voice_set_tdm_slot() for this, and cpcap_voice_call()
> helper to configure the additional registers needed for voice call.
> 
> Let's also clear CPCAP_REG_VAUDIOC on init in case we have the bit for
> CPCAP_BIT_VAUDIO_MODE0 set on init.
> 
> Cc: Arthur D. <spinal.by@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  sound/soc/codecs/cpcap.c | 123 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/sound/soc/codecs/cpcap.c b/sound/soc/codecs/cpcap.c
> --- a/sound/soc/codecs/cpcap.c
> +++ b/sound/soc/codecs/cpcap.c
> @@ -16,6 +16,14 @@
>  #include <sound/soc.h>
>  #include <sound/tlv.h>
>  
> +/* Register 512 CPCAP_REG_VAUDIOC --- Audio Regulator and Bias Voltage */
> +#define CPCAP_BIT_AUDIO_LOW_PWR           6
> +#define CPCAP_BIT_AUD_LOWPWR_SPEED        5
> +#define CPCAP_BIT_VAUDIOPRISTBY           4
> +#define CPCAP_BIT_VAUDIO_MODE1            2
> +#define CPCAP_BIT_VAUDIO_MODE0            1
> +#define CPCAP_BIT_V_AUDIO_EN              0
> +
>  /* Register 513 CPCAP_REG_CC     --- CODEC */
>  #define CPCAP_BIT_CDC_CLK2                15
>  #define CPCAP_BIT_CDC_CLK1                14
> @@ -221,6 +229,7 @@ struct cpcap_reg_info {
>  };
>  
>  static const struct cpcap_reg_info cpcap_default_regs[] = {
> +	{ CPCAP_REG_VAUDIOC, 0x003F, 0x0000 },
>  	{ CPCAP_REG_CC, 0xFFFF, 0x0000 },
>  	{ CPCAP_REG_CC, 0xFFFF, 0x0000 },
>  	{ CPCAP_REG_CDI, 0xBFFF, 0x0000 },
> @@ -1370,6 +1379,119 @@ static int cpcap_voice_set_dai_fmt(struct snd_soc_dai *codec_dai,
>  	return 0;
>  }
>  
> +/*
> + * Configure codec for voice call if requested.
> + *
> + * We can configure most with snd_soc_dai_set_sysclk(), snd_soc_dai_set_fmt()
> + * and snd_soc_dai_set_tdm_slot(). This function configures the rest of the
> + * cpcap related hardware as CPU is not involved in the voice call.
> + */
> +static int cpcap_voice_call(struct cpcap_audio *cpcap, struct snd_soc_dai *dai,
> +			    bool voice_call)
> +{
> +	int mask, err;
> +
> +	/* Modem to codec VAUDIO_MODE1 */
> +	mask = BIT(CPCAP_BIT_VAUDIO_MODE1);
> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_VAUDIOC,
> +				 mask, voice_call ? mask : 0);
> +	if (err)
> +		return err;
> +
> +	/* Clear MIC1_MUX for call */
> +	mask = BIT(CPCAP_BIT_MIC1_MUX);
> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_TXI,
> +				 mask, voice_call ? 0 : mask);
> +	if (err)
> +		return err;
> +
> +	/* Set MIC2_MUX for call */
> +	mask = BIT(CPCAP_BIT_MB_ON1L) | BIT(CPCAP_BIT_MB_ON1R) |
> +		BIT(CPCAP_BIT_MIC2_MUX) | BIT(CPCAP_BIT_MIC2_PGA_EN);
> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_TXI,
> +				 mask, voice_call ? mask : 0);
> +	if (err)
> +		return err;
> +
> +	/* Enable LDSP for call */
> +	mask = BIT(CPCAP_BIT_A2_LDSP_L_EN) | BIT(CPCAP_BIT_A2_LDSP_R_EN);
> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_RXOA,
> +				 mask, voice_call ? mask : 0);
> +	if (err)
> +		return err;
> +
> +	/* Enable CPCAP_BIT_PGA_CDC_EN for call */
> +	mask = BIT(CPCAP_BIT_PGA_CDC_EN);
> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_RXCOA,
> +				 mask, voice_call ? mask : 0);
> +	if (err)
> +		return err;
> +
> +	/* Unmute voice for call */
> +	if (dai) {
> +		err = snd_soc_dai_digital_mute(dai, !voice_call,
> +					       SNDRV_PCM_STREAM_PLAYBACK);
> +		if (err)
> +			return err;
> +	}
> +
> +	/* Set modem to codec mic CDC and HPF for call */
> +	mask = BIT(CPCAP_BIT_MIC2_CDC_EN) | BIT(CPCAP_BIT_CDC_EN_RX) |
> +	       BIT(CPCAP_BIT_AUDOHPF_1) | BIT(CPCAP_BIT_AUDOHPF_0) |
> +	       BIT(CPCAP_BIT_AUDIHPF_1) | BIT(CPCAP_BIT_AUDIHPF_0);
> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CC,
> +				 mask, voice_call ? mask : 0);
> +	if (err)
> +		return err;
> +
> +	/* Enable modem to codec CDC for call*/
> +	mask = BIT(CPCAP_BIT_CDC_CLK_EN);
> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CDI,
> +				 mask, voice_call ? mask : 0);
> +
> +	return err;
> +}
> +
> +static int cpcap_voice_set_tdm_slot(struct snd_soc_dai *dai,
> +				    unsigned int tx_mask, unsigned int rx_mask,
> +				    int slots, int slot_width)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
> +	int err, ts_mask, mask;
> +	bool voice_call;
> +
> +	/*
> +	 * Primitive test for voice call, probably needs more checks
> +	 * later on for 16-bit calls detected, Bluetooth headset etc.
> +	 */
> +	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
> +		voice_call = true;
> +	else
> +		voice_call = false;

You only have voice call if only rx slot0 is in use?
If you record mono on the voice DAI, then rx_mask is also 1, no?

> +
> +	ts_mask = 0x7 << CPCAP_BIT_MIC2_TIMESLOT0;
> +	ts_mask |= 0x7 << CPCAP_BIT_MIC1_RX_TIMESLOT0;
> +
> +	mask = (tx_mask & 0x7) << CPCAP_BIT_MIC2_TIMESLOT0;
> +	mask |= (rx_mask & 0x7) << CPCAP_BIT_MIC1_RX_TIMESLOT0;
> +
> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CDI,
> +				 ts_mask, mask);
> +	if (err)
> +		return err;
> +
> +	err = cpcap_set_samprate(cpcap, CPCAP_DAI_VOICE, slot_width * 1000);
> +	if (err)
> +		return err;

You will also set the sampling rate for voice in
cpcap_voice_hw_params(), but that is for normal playback/capture, right?

> +
> +	err = cpcap_voice_call(cpcap, dai, voice_call);
> +	if (err)
> +		return err;

It feels like that these should be done via DAPM with codec to codec route?

> +
> +	return 0;
> +}
> +
>  static int cpcap_voice_set_mute(struct snd_soc_dai *dai, int mute)
>  {
>  	struct snd_soc_component *component = dai->component;
> @@ -1391,6 +1513,7 @@ static const struct snd_soc_dai_ops cpcap_dai_voice_ops = {
>  	.hw_params	= cpcap_voice_hw_params,
>  	.set_sysclk	= cpcap_voice_set_dai_sysclk,
>  	.set_fmt	= cpcap_voice_set_dai_fmt,
> +	.set_tdm_slot	= cpcap_voice_set_tdm_slot,
>  	.digital_mute	= cpcap_voice_set_mute,
>  };
>  
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [200212 09:18]:
> On 11/02/2020 20.10, Tony Lindgren wrote:
> > +static int cpcap_voice_set_tdm_slot(struct snd_soc_dai *dai,
> > +				    unsigned int tx_mask, unsigned int rx_mask,
> > +				    int slots, int slot_width)
> > +{
> > +	struct snd_soc_component *component = dai->component;
> > +	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
> > +	int err, ts_mask, mask;
> > +	bool voice_call;
> > +
> > +	/*
> > +	 * Primitive test for voice call, probably needs more checks
> > +	 * later on for 16-bit calls detected, Bluetooth headset etc.
> > +	 */
> > +	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
> > +		voice_call = true;
> > +	else
> > +		voice_call = false;
> 
> You only have voice call if only rx slot0 is in use?

Yeah so it seems. Then there's the modem to wlcore bluetooth path that
I have not looked at. But presumably that's again just configuring some
tdm slot on the PMIC.

> If you record mono on the voice DAI, then rx_mask is also 1, no?

It is above :) But maybe I don't follow what you're asking here and
maybe you have some better check in mind.

I have no idea where we would implement recording voice calls for
example, I guess mcbsp could do that somewhere to dump out a tdm slot
specific traffic.

> > +
> > +	ts_mask = 0x7 << CPCAP_BIT_MIC2_TIMESLOT0;
> > +	ts_mask |= 0x7 << CPCAP_BIT_MIC1_RX_TIMESLOT0;
> > +
> > +	mask = (tx_mask & 0x7) << CPCAP_BIT_MIC2_TIMESLOT0;
> > +	mask |= (rx_mask & 0x7) << CPCAP_BIT_MIC1_RX_TIMESLOT0;
> > +
> > +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CDI,
> > +				 ts_mask, mask);
> > +	if (err)
> > +		return err;
> > +
> > +	err = cpcap_set_samprate(cpcap, CPCAP_DAI_VOICE, slot_width * 1000);
> > +	if (err)
> > +		return err;
> 
> You will also set the sampling rate for voice in
> cpcap_voice_hw_params(), but that is for normal playback/capture, right?

Yeah so normal playback/capture is already working with cpcap codec driver
with mainline Linux. The voice call needs to set rate to 8000.

> > +
> > +	err = cpcap_voice_call(cpcap, dai, voice_call);
> > +	if (err)
> > +		return err;
> 
> It feels like that these should be done via DAPM with codec to codec route?

Sure if you have some better way of doing it :) Do you have an example to
point me to?

Regards,

Tony

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Peter Ujfalusi]

Hi Tony,

On 12/02/2020 16.46, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [200212 09:18]:
>> On 11/02/2020 20.10, Tony Lindgren wrote:
>>> +static int cpcap_voice_set_tdm_slot(struct snd_soc_dai *dai,
>>> +				    unsigned int tx_mask, unsigned int rx_mask,
>>> +				    int slots, int slot_width)
>>> +{
>>> +	struct snd_soc_component *component = dai->component;
>>> +	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
>>> +	int err, ts_mask, mask;
>>> +	bool voice_call;
>>> +
>>> +	/*
>>> +	 * Primitive test for voice call, probably needs more checks
>>> +	 * later on for 16-bit calls detected, Bluetooth headset etc.
>>> +	 */
>>> +	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
>>> +		voice_call = true;
>>> +	else
>>> +		voice_call = false;
>>
>> You only have voice call if only rx slot0 is in use?
> 
> Yeah so it seems. Then there's the modem to wlcore bluetooth path that
> I have not looked at. But presumably that's again just configuring some
> tdm slot on the PMIC.
> 
>> If you record mono on the voice DAI, then rx_mask is also 1, no?
> 
> It is above :) But maybe I don't follow what you're asking here

If you arecrod -Dvoice_pcm -c1 -fS8 > /dev/null
then it is reasonable that the machine driver will set rx_mask = 1

> and maybe you have some better check in mind.

Not sure, but relying on set_tdm_slots to decide if we are in a call
case does not sound right.

> I have no idea where we would implement recording voice calls for
> example, I guess mcbsp could do that somewhere to dump out a tdm slot
> specific traffic.

Need to check how things are wired and how they expected to work ;)

>>> +
>>> +	ts_mask = 0x7 << CPCAP_BIT_MIC2_TIMESLOT0;
>>> +	ts_mask |= 0x7 << CPCAP_BIT_MIC1_RX_TIMESLOT0;
>>> +
>>> +	mask = (tx_mask & 0x7) << CPCAP_BIT_MIC2_TIMESLOT0;
>>> +	mask |= (rx_mask & 0x7) << CPCAP_BIT_MIC1_RX_TIMESLOT0;
>>> +
>>> +	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CDI,
>>> +				 ts_mask, mask);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	err = cpcap_set_samprate(cpcap, CPCAP_DAI_VOICE, slot_width * 1000);
>>> +	if (err)
>>> +		return err;
>>
>> You will also set the sampling rate for voice in
>> cpcap_voice_hw_params(), but that is for normal playback/capture, right?
> 
> Yeah so normal playback/capture is already working with cpcap codec driver
> with mainline Linux. The voice call needs to set rate to 8000.

But if you have a voice call initiated should not the rate be set by the
set_sysclk()?


>>> +
>>> +	err = cpcap_voice_call(cpcap, dai, voice_call);
>>> +	if (err)
>>> +		return err;
>>
>> It feels like that these should be done via DAPM with codec to codec route?
> 
> Sure if you have some better way of doing it :) Do you have an example to
> point me to?

Something along the lines of:
https://mailman.alsa-project.org/pipermail/alsa-devel/2020-February/162915.html

The it is a matter of building and connecting the DAPM routes between
the two codec and with a flip of the switch you would have audio flowing
between them.

> 
> Regards,
> 
> Tony
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [200214 13:30]:
> Hi Tony,
> 
> On 12/02/2020 16.46, Tony Lindgren wrote:
> > * Peter Ujfalusi <peter.ujfalusi@ti.com> [200212 09:18]:
> >> On 11/02/2020 20.10, Tony Lindgren wrote:
> >>> +static int cpcap_voice_set_tdm_slot(struct snd_soc_dai *dai,
> >>> +				    unsigned int tx_mask, unsigned int rx_mask,
> >>> +				    int slots, int slot_width)
> >>> +{
> >>> +	struct snd_soc_component *component = dai->component;
> >>> +	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
> >>> +	int err, ts_mask, mask;
> >>> +	bool voice_call;
> >>> +
> >>> +	/*
> >>> +	 * Primitive test for voice call, probably needs more checks
> >>> +	 * later on for 16-bit calls detected, Bluetooth headset etc.
> >>> +	 */
> >>> +	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
> >>> +		voice_call = true;
> >>> +	else
> >>> +		voice_call = false;
> >>
> >> You only have voice call if only rx slot0 is in use?
> > 
> > Yeah so it seems. Then there's the modem to wlcore bluetooth path that
> > I have not looked at. But presumably that's again just configuring some
> > tdm slot on the PMIC.
> > 
> >> If you record mono on the voice DAI, then rx_mask is also 1, no?
> > 
> > It is above :) But maybe I don't follow what you're asking here
> 
> If you arecrod -Dvoice_pcm -c1 -fS8 > /dev/null
> then it is reasonable that the machine driver will set rx_mask = 1
> 
> > and maybe you have some better check in mind.
> 
> Not sure, but relying on set_tdm_slots to decide if we are in a call
> case does not sound right.

OK yeah seems at least bluetooth would need to be also handled
in the set_tdm_slots.

> >> You will also set the sampling rate for voice in
> >> cpcap_voice_hw_params(), but that is for normal playback/capture, right?
> > 
> > Yeah so normal playback/capture is already working with cpcap codec driver
> > with mainline Linux. The voice call needs to set rate to 8000.
> 
> But if you have a voice call initiated should not the rate be set by the
> set_sysclk()?

Hmm does set_sysclk called from modem codec know that cpcap codec
is the clock master based on bitclock-master and set the rate
for cpcap codec?

> >> It feels like that these should be done via DAPM with codec to codec route?
> > 
> > Sure if you have some better way of doing it :) Do you have an example to
> > point me to?
> 
> Something along the lines of:
> https://mailman.alsa-project.org/pipermail/alsa-devel/2020-February/162915.html
> 
> The it is a matter of building and connecting the DAPM routes between
> the two codec and with a flip of the switch you would have audio flowing
> between them.

Sounds good to me.

Tony

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Peter Ujfalusi]

Hi Tony,

On 18/02/2020 1.23, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [200214 13:30]:
>> Hi Tony,
>>
>> On 12/02/2020 16.46, Tony Lindgren wrote:
>>> * Peter Ujfalusi <peter.ujfalusi@ti.com> [200212 09:18]:
>>>> On 11/02/2020 20.10, Tony Lindgren wrote:
>>>>> +static int cpcap_voice_set_tdm_slot(struct snd_soc_dai *dai,
>>>>> +				    unsigned int tx_mask, unsigned int rx_mask,
>>>>> +				    int slots, int slot_width)
>>>>> +{
>>>>> +	struct snd_soc_component *component = dai->component;
>>>>> +	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
>>>>> +	int err, ts_mask, mask;
>>>>> +	bool voice_call;
>>>>> +
>>>>> +	/*
>>>>> +	 * Primitive test for voice call, probably needs more checks
>>>>> +	 * later on for 16-bit calls detected, Bluetooth headset etc.
>>>>> +	 */
>>>>> +	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
>>>>> +		voice_call = true;
>>>>> +	else
>>>>> +		voice_call = false;
>>>>
>>>> You only have voice call if only rx slot0 is in use?
>>>
>>> Yeah so it seems. Then there's the modem to wlcore bluetooth path that
>>> I have not looked at. But presumably that's again just configuring some
>>> tdm slot on the PMIC.
>>>
>>>> If you record mono on the voice DAI, then rx_mask is also 1, no?
>>>
>>> It is above :) But maybe I don't follow what you're asking here
>>
>> If you arecrod -Dvoice_pcm -c1 -fS8 > /dev/null
>> then it is reasonable that the machine driver will set rx_mask = 1
>>
>>> and maybe you have some better check in mind.
>>
>> Not sure, but relying on set_tdm_slots to decide if we are in a call
>> case does not sound right.
> 
> OK yeah seems at least bluetooth would need to be also handled
> in the set_tdm_slots.

set_tdm_slots() is for setting how the TDM slots supposed to be used by
the component and not really for things to configure different operating
modes.

If you hardwire things in set_tdm_slots() for the droid4 then how the
codec driver can be reused in other setups?

>>>> You will also set the sampling rate for voice in
>>>> cpcap_voice_hw_params(), but that is for normal playback/capture, right?
>>>
>>> Yeah so normal playback/capture is already working with cpcap codec driver
>>> with mainline Linux. The voice call needs to set rate to 8000.
>>
>> But if you have a voice call initiated should not the rate be set by the
>> set_sysclk()?
> 
> Hmm does set_sysclk called from modem codec know that cpcap codec
> is the clock master based on bitclock-master and set the rate
> for cpcap codec?

Neither component should call set_sysclk, set_tdm_slots. The machine
driver should as it is the only one who know how things are wired...

> 
>>>> It feels like that these should be done via DAPM with codec to codec route?
>>>
>>> Sure if you have some better way of doing it :) Do you have an example to
>>> point me to?
>>
>> Something along the lines of:
>> https://mailman.alsa-project.org/pipermail/alsa-devel/2020-February/162915.html
>>
>> The it is a matter of building and connecting the DAPM routes between
>> the two codec and with a flip of the switch you would have audio flowing
>> between them.
> 
> Sounds good to me.
> 
> Tony
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [200218 15:16]:
> On 18/02/2020 1.23, Tony Lindgren wrote:
> > * Peter Ujfalusi <peter.ujfalusi@ti.com> [200214 13:30]:
> >> Hi Tony,
> >>
> >> On 12/02/2020 16.46, Tony Lindgren wrote:
> >>> * Peter Ujfalusi <peter.ujfalusi@ti.com> [200212 09:18]:
> >>>> On 11/02/2020 20.10, Tony Lindgren wrote:
> >>>>> +static int cpcap_voice_set_tdm_slot(struct snd_soc_dai *dai,
> >>>>> +				    unsigned int tx_mask, unsigned int rx_mask,
> >>>>> +				    int slots, int slot_width)
> >>>>> +{
> >>>>> +	struct snd_soc_component *component = dai->component;
> >>>>> +	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
> >>>>> +	int err, ts_mask, mask;
> >>>>> +	bool voice_call;
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * Primitive test for voice call, probably needs more checks
> >>>>> +	 * later on for 16-bit calls detected, Bluetooth headset etc.
> >>>>> +	 */
> >>>>> +	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
> >>>>> +		voice_call = true;
> >>>>> +	else
> >>>>> +		voice_call = false;
> >>>>
> >>>> You only have voice call if only rx slot0 is in use?
> >>>
> >>> Yeah so it seems. Then there's the modem to wlcore bluetooth path that
> >>> I have not looked at. But presumably that's again just configuring some
> >>> tdm slot on the PMIC.
> >>>
> >>>> If you record mono on the voice DAI, then rx_mask is also 1, no?
> >>>
> >>> It is above :) But maybe I don't follow what you're asking here
> >>
> >> If you arecrod -Dvoice_pcm -c1 -fS8 > /dev/null
> >> then it is reasonable that the machine driver will set rx_mask = 1
> >>
> >>> and maybe you have some better check in mind.
> >>
> >> Not sure, but relying on set_tdm_slots to decide if we are in a call
> >> case does not sound right.
> > 
> > OK yeah seems at least bluetooth would need to be also handled
> > in the set_tdm_slots.
> 
> set_tdm_slots() is for setting how the TDM slots supposed to be used by
> the component and not really for things to configure different operating
> modes.
> 
> If you hardwire things in set_tdm_slots() for the droid4 then how the
> codec driver can be reused in other setups?

Right, I'm all go for better solutions :)

> >>>> You will also set the sampling rate for voice in
> >>>> cpcap_voice_hw_params(), but that is for normal playback/capture, right?
> >>>
> >>> Yeah so normal playback/capture is already working with cpcap codec driver
> >>> with mainline Linux. The voice call needs to set rate to 8000.
> >>
> >> But if you have a voice call initiated should not the rate be set by the
> >> set_sysclk()?
> > 
> > Hmm does set_sysclk called from modem codec know that cpcap codec
> > is the clock master based on bitclock-master and set the rate
> > for cpcap codec?
> 
> Neither component should call set_sysclk, set_tdm_slots. The machine
> driver should as it is the only one who know how things are wired...

OK, but so what's the machine driver part in this case?

Regards,

Tony

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Tue, Feb 18, 2020 at 07:32:11AM -0800, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [200218 15:16]:

> > > Hmm does set_sysclk called from modem codec know that cpcap codec
> > > is the clock master based on bitclock-master and set the rate
> > > for cpcap codec?

> > Neither component should call set_sysclk, set_tdm_slots. The machine
> > driver should as it is the only one who know how things are wired...

> OK, but so what's the machine driver part in this case?

The machine driver is responsible for saying how everything is glued
together, both where the wires run and any policy decisions about how
the clocking tree should be arranged or TDM used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4062 bytes --]

Hi,

On Tue, Feb 18, 2020 at 07:32:11AM -0800, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [200218 15:16]:
> > On 18/02/2020 1.23, Tony Lindgren wrote:
> > > * Peter Ujfalusi <peter.ujfalusi@ti.com> [200214 13:30]:
> > >> Hi Tony,
> > >>
> > >> On 12/02/2020 16.46, Tony Lindgren wrote:
> > >>> * Peter Ujfalusi <peter.ujfalusi@ti.com> [200212 09:18]:
> > >>>> On 11/02/2020 20.10, Tony Lindgren wrote:
> > >>>>> +static int cpcap_voice_set_tdm_slot(struct snd_soc_dai *dai,
> > >>>>> +				    unsigned int tx_mask, unsigned int rx_mask,
> > >>>>> +				    int slots, int slot_width)
> > >>>>> +{
> > >>>>> +	struct snd_soc_component *component = dai->component;
> > >>>>> +	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
> > >>>>> +	int err, ts_mask, mask;
> > >>>>> +	bool voice_call;
> > >>>>> +
> > >>>>> +	/*
> > >>>>> +	 * Primitive test for voice call, probably needs more checks
> > >>>>> +	 * later on for 16-bit calls detected, Bluetooth headset etc.
> > >>>>> +	 */
> > >>>>> +	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
> > >>>>> +		voice_call = true;
> > >>>>> +	else
> > >>>>> +		voice_call = false;
> > >>>>
> > >>>> You only have voice call if only rx slot0 is in use?
> > >>>
> > >>> Yeah so it seems. Then there's the modem to wlcore bluetooth path that
> > >>> I have not looked at. But presumably that's again just configuring some
> > >>> tdm slot on the PMIC.
> > >>>
> > >>>> If you record mono on the voice DAI, then rx_mask is also 1, no?
> > >>>
> > >>> It is above :) But maybe I don't follow what you're asking here
> > >>
> > >> If you arecrod -Dvoice_pcm -c1 -fS8 > /dev/null
> > >> then it is reasonable that the machine driver will set rx_mask = 1
> > >>
> > >>> and maybe you have some better check in mind.
> > >>
> > >> Not sure, but relying on set_tdm_slots to decide if we are in a call
> > >> case does not sound right.
> > > 
> > > OK yeah seems at least bluetooth would need to be also handled
> > > in the set_tdm_slots.
> > 
> > set_tdm_slots() is for setting how the TDM slots supposed to be used by
> > the component and not really for things to configure different operating
> > modes.
> > 
> > If you hardwire things in set_tdm_slots() for the droid4 then how the
> > codec driver can be reused in other setups?
> 
> Right, I'm all go for better solutions :)
> 
> > >>>> You will also set the sampling rate for voice in
> > >>>> cpcap_voice_hw_params(), but that is for normal playback/capture, right?
> > >>>
> > >>> Yeah so normal playback/capture is already working with cpcap codec driver
> > >>> with mainline Linux. The voice call needs to set rate to 8000.
> > >>
> > >> But if you have a voice call initiated should not the rate be set by the
> > >> set_sysclk()?
> > > 
> > > Hmm does set_sysclk called from modem codec know that cpcap codec
> > > is the clock master based on bitclock-master and set the rate
> > > for cpcap codec?
> > 
> > Neither component should call set_sysclk, set_tdm_slots. The machine
> > driver should as it is the only one who know how things are wired...
> 
> OK, but so what's the machine driver part in this case?

simple-graph-card is the current machine driver. We might have to
introduce a Droid 4 specific driver instead. I used simple(-graph)-card
instead of introducing a new driver, since the setup was simple enough
without modem and bluetooth. The simple card was perfect to test the CPCAP
codec driver. The TDM things might be complex enough to create
a new machine driver (as I mentioned in the original patchset
adding CPCAP codec support).

Note: Don't use Motorola's tree to learn about ASoC. Their soundcard
and cpcap codec drivers are full of weird hacks. I'm pretty sure the
author(s) did not really understand how ASoC works. From my
experience you should only use their code to understand the
hardware wiring. You might also want to look into the MC13783
datasheet for the keyword "network mode".

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

On Tue, Feb 18, 2020 at 06:06:28PM +0100, Sebastian Reichel wrote:

> simple-graph-card is the current machine driver. We might have to
> introduce a Droid 4 specific driver instead. I used simple(-graph)-card
> instead of introducing a new driver, since the setup was simple enough
> without modem and bluetooth. The simple card was perfect to test the CPCAP
> codec driver. The TDM things might be complex enough to create
> a new machine driver (as I mentioned in the original patchset
> adding CPCAP codec support).

I tend to agree here, phones are generally one of the most complicated
classes of system for clocking and interconnects and the CODECs they use
often the most complex too so they're really stretching the generic
cards.  It'd be nice to be able to handle things with generic cards but
it's likely you'll run into issues that it'd be unreasonable to force
you to address for system enablement.  OTOH if you manage to get one of
the generic cards working well that'd be excellent!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [200218 17:43]:
> On Tue, Feb 18, 2020 at 06:06:28PM +0100, Sebastian Reichel wrote:
> 
> > simple-graph-card is the current machine driver. We might have to
> > introduce a Droid 4 specific driver instead. I used simple(-graph)-card
> > instead of introducing a new driver, since the setup was simple enough
> > without modem and bluetooth. The simple card was perfect to test the CPCAP
> > codec driver. The TDM things might be complex enough to create
> > a new machine driver (as I mentioned in the original patchset
> > adding CPCAP codec support).
> 
> I tend to agree here, phones are generally one of the most complicated
> classes of system for clocking and interconnects and the CODECs they use
> often the most complex too so they're really stretching the generic
> cards.  It'd be nice to be able to handle things with generic cards but
> it's likely you'll run into issues that it'd be unreasonable to force
> you to address for system enablement.  OTOH if you manage to get one of
> the generic cards working well that'd be excellent!

Well to me it seems that we just already have all the data needed with
the graph binding and snd-soc-audio-graph-card + codec2codec support.

I don't think we have cases where the cpcap codec is not the master,
so as long as the cpcap codec knows what's going on then there
may not be a need for machine driver.

I guess the the bluetooth to modem path is the one to check to see
what provides the clocks..

Regards,

Tony

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On Wed, Feb 19, 2020 at 09:39:02AM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [200218 17:43]:

> > you to address for system enablement.  OTOH if you manage to get one of
> > the generic cards working well that'd be excellent!

> Well to me it seems that we just already have all the data needed with
> the graph binding and snd-soc-audio-graph-card + codec2codec support.

> I don't think we have cases where the cpcap codec is not the master,
> so as long as the cpcap codec knows what's going on then there
> may not be a need for machine driver.

> I guess the the bluetooth to modem path is the one to check to see
> what provides the clocks..

Usually in telephony cases it's the modem that's the clock master FWIW.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [200219 17:46]:
> On Wed, Feb 19, 2020 at 09:39:02AM -0800, Tony Lindgren wrote:
> > * Mark Brown <broonie@kernel.org> [200218 17:43]:
> 
> > > you to address for system enablement.  OTOH if you manage to get one of
> > > the generic cards working well that'd be excellent!
> 
> > Well to me it seems that we just already have all the data needed with
> > the graph binding and snd-soc-audio-graph-card + codec2codec support.
> 
> > I don't think we have cases where the cpcap codec is not the master,
> > so as long as the cpcap codec knows what's going on then there
> > may not be a need for machine driver.
> 
> > I guess the the bluetooth to modem path is the one to check to see
> > what provides the clocks..
> 
> Usually in telephony cases it's the modem that's the clock master FWIW.

Well at least the samplerate needs to be configured in the cpcap
codec driver for voice calls, and we're setting CPCAP_BIT_CDC_CLK_EN
bit for voice call which is the "Voice DAI Clock". It's also set when
just playing audio using the voice channel is used. And we also have
a similar bit for CPCAP_BIT_ST_CLK_EN for "HiFi DAI Clock" for the
hifi channel. So these would seem to hit that it is really the cpcap
that's the clock master for voice calls in this case.

But I guess the test to do there would be to just clear the bit
for CPCAP_BIT_CDC_CLK_EN during a voice call and see if it still
works.

Regards,

Tony

Re: [PATCH] ASoC: cpcap: Implement set_tdm_slot for voice call support

[Tony Lindgren]

* Sebastian Reichel <sebastian.reichel@collabora.com> [200218 17:07]:
> On Tue, Feb 18, 2020 at 07:32:11AM -0800, Tony Lindgren wrote:
> > OK, but so what's the machine driver part in this case?
> 
> simple-graph-card is the current machine driver. We might have to
> introduce a Droid 4 specific driver instead. I used simple(-graph)-card
> instead of introducing a new driver, since the setup was simple enough
> without modem and bluetooth. The simple card was perfect to test the CPCAP
> codec driver. The TDM things might be complex enough to create
> a new machine driver (as I mentioned in the original patchset
> adding CPCAP codec support).

Well we do have the .set_tdm_slot to configure things. If it turns
out we only need to track the machine audio state in cpcap.c, then
we don't need a separate machine driver.

However, if it turns out that cpcap is not always active for
some audio paths, then yeah it seems that we need a custom
machine driver to keep track of the machine audio state.

> Note: Don't use Motorola's tree to learn about ASoC. Their soundcard
> and cpcap codec drivers are full of weird hacks. I'm pretty sure the
> author(s) did not really understand how ASoC works. From my
> experience you should only use their code to understand the
> hardware wiring. You might also want to look into the MC13783
> datasheet for the keyword "network mode".

Yeah nope. And just dumping out the cpcap registers in Android
seems to provide enough information to get the missing features
working.

Regards,

Tony