Re: [alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support
On 20-06-18, 10:53, Srinivas Kandagatla wrote: > On 20/06/18 10:31, Vinod wrote: > > > Here clock will be enabled twice but disable will be called only once when > > > count = 0. > > > > > > This will make the clock always enabled. So, I think we should keep either > > > mutex lock or atomic variable to synchronize this. > > we are using DPCM here right? > > We should probably model this clk as DAPM widget so we do not need to handle > this in machine code. Sure that would be even better, but my point was that we need not worry about .startup racing in case of DPCM as it holds a card mutex before it calls PCM ops.. so it is already serialized.. -- ~Vinod
Re: [alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support
On 20/06/18 10:31, Vinod wrote: Here clock will be enabled twice but disable will be called only once when count = 0. This will make the clock always enabled. So, I think we should keep either mutex lock or atomic variable to synchronize this. we are using DPCM here right? We should probably model this clk as DAPM widget so we do not need to handle this in machine code. --srini
Re: [alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support
Hi Rohit,
On 20-06-18, 13:07, Rohit Kumar wrote:
> > On 19-06-18, 19:20, Rohit Kumar wrote:
> > > On 6/19/2018 10:35 AM, Vinod wrote:
> > > > On 18-06-18, 16:46, Rohit kumar wrote:
> > > >
> > > > > +struct sdm845_snd_data {
> > > > > + struct snd_soc_card *card;
> > > > > + struct regulator *vdd_supply;
> > > > > + struct snd_soc_dai_link dai_link[];
> > > > > +};
> > > > > +
> > > > > +static struct mutex pri_mi2s_res_lock;
> > > > > +static struct mutex quat_tdm_res_lock;
> > > > any reason why the locks can't be part of sdm845_snd_data?
> > > > Also why do we need two locks ?
> > > No specific reason, I will move it to sdm845_snd_data.
> > > These locks are used to protect enable/disable of bit clocks. We have
> > > Primary MI2S RX/TX
> > > and Quaternary TDM RX/TX interfaces. For primary mi2s rx/tx, we have
> > > single
> > > clock which is
> > > synchronized with pri_mi2s_res_lock. For Quat TDM RX/TX, we are using
> > > quat_tdm_res_lock.
> > > We need two locks as we are protecting two different resources.
> > I think bigger question is why do you need any locks? What is the race
> > scenario you envision which needs protection
> >
>
> Below is one of the race condition:
>
> Thread1 | Thread2
> --
> startup() |
> count++; | startup()
> read count (count = 1) |
> enable_clock() | count++; //count = 2
> shutdown() |
> count--;// count = 1 |
> | read count (count = 1)
> | enable_clock()
>
> Here clock will be enabled twice but disable will be called only once when
> count = 0.
>
> This will make the clock always enabled. So, I think we should keep either
> mutex lock or atomic variable to synchronize this.
we are using DPCM here right?
--
~Vinod
Re: [alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support
Hi Rohit,
On 19-06-18, 19:20, Rohit Kumar wrote:
> On 6/19/2018 10:35 AM, Vinod wrote:
> > On 18-06-18, 16:46, Rohit kumar wrote:
> >
> > > +struct sdm845_snd_data {
> > > + struct snd_soc_card *card;
> > > + struct regulator *vdd_supply;
> > > + struct snd_soc_dai_link dai_link[];
> > > +};
> > > +
> > > +static struct mutex pri_mi2s_res_lock;
> > > +static struct mutex quat_tdm_res_lock;
> > any reason why the locks can't be part of sdm845_snd_data?
> > Also why do we need two locks ?
> No specific reason, I will move it to sdm845_snd_data.
> These locks are used to protect enable/disable of bit clocks. We have
> Primary MI2S RX/TX
> and Quaternary TDM RX/TX interfaces. For primary mi2s rx/tx, we have single
> clock which is
> synchronized with pri_mi2s_res_lock. For Quat TDM RX/TX, we are using
> quat_tdm_res_lock.
> We need two locks as we are protecting two different resources.
I think bigger question is why do you need any locks? What is the race
scenario you envision which needs protection
--
~Vinod
Re: [alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support
Thanks Srinivas for reviewing.
On 6/19/2018 2:16 PM, Srinivas Kandagatla wrote:
Thanks Rohit for the patch!
On 18/06/18 12:16, Rohit kumar wrote:
This patch adds sdm845 audio machine driver support.
Signed-off-by: Rohit kumar
---
.../devicetree/bindings/sound/qcom,sdm845.txt | 87
sound/soc/qcom/Kconfig | 9 +
sound/soc/qcom/Makefile | 2 +
sound/soc/qcom/sdm845.c | 539
+
4 files changed, 637 insertions(+)
create mode 100644
Documentation/devicetree/bindings/sound/qcom,sdm845.txt
Split the bindings into a separate patch!
Sure, will do it in next spin.
create mode 100644 sound/soc/qcom/sdm845.c
diff --git a/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
new file mode 100644
index 000..fc0e98c
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
@@ -0,0 +1,87 @@
+* Qualcomm Technologies Inc. SDM845 ASoC sound card driver
+
+This binding describes the SDM845 sound card, which uses qdsp for
audio.
[..]
+
+- codec:
+ Usage: required
+ Value type:
+ Definition: codec dai sub-node
+
+- platform:
+ Usage: opional
Optional
okay
+ Value type:
+ Definition: platform dai sub-node
+
+- sound-dai:
+ Usage: required
+ Value type:
+ Definition: dai phandle/s and port of CPU/CODEC/PLATFORM node.
+
+Example:
+
+audio {
+ compatible = "qcom,sdm845-sndcard";
+ qcom,model = "sdm845-snd-card";
+ pinctrl-names = "pri_active", "pri_sleep";
+ pinctrl-0 = <&pri_mi2s_active &pri_mi2s_ws_active>;
+ pinctrl-1 = <&pri_mi2s_sleep &pri_mi2s_ws_sleep>;
+
+ qcom,audio-routing = "PRI_MI2S_RX Audio Mixer", "Pri-mi2s-gpio";
+
+ cdc-vdd-supply = <&pm8998_l14>;
+
+ fe@1 {
Lets not use fe or be reference here, its just a link.
okay
+ link-name = "MultiMedia1";
+ cpu {
+ sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>;
+ };
+ platform {
+ sound-dai = <&q6asmdai>;
+ };
asmdai has sound-cell specifier as 1, so this will dtc will throw
warning for this.
have a look at how 8996 is done.
ok, sure
+ };
+
+ be@1 {
[..]
diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
index 206945b..ac9609e 100644
--- a/sound/soc/qcom/Makefile
+++ b/sound/soc/qcom/Makefile
@@ -14,10 +14,12 @@ obj-$(CONFIG_SND_SOC_LPASS_APQ8016) +=
snd-soc-lpass-apq8016.o
snd-soc-storm-objs := storm.o
snd-soc-apq8016-sbc-objs := apq8016_sbc.o
snd-soc-apq8096-objs := apq8096.o
+snd-soc-sdm845-objs := sdm845.o
obj-$(CONFIG_SND_SOC_STORM) += snd-soc-storm.o
obj-$(CONFIG_SND_SOC_APQ8016_SBC) += snd-soc-apq8016-sbc.o
obj-$(CONFIG_SND_SOC_MSM8996) += snd-soc-apq8096.o
++obj-$(CONFIG_SND_SOC_SDM845) += snd-soc-sdm845.o
?? looks like typo here.
Right. Will update.
#DSP lib
obj-$(CONFIG_SND_SOC_QDSP6) += qdsp6/
diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
new file mode 100644
index 000..70d2611
--- /dev/null
+++ b/sound/soc/qcom/sdm845.c
@@ -0,0 +1,539 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include
+#include
[..]
+}
+
+static int sdm845_snd_startup(struct snd_pcm_substream *substream)
+{
+ unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+
+ pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
+ switch (cpu_dai->id) {
+ case PRIMARY_MI2S_RX:
+ case PRIMARY_MI2S_TX:
+ mutex_lock(&pri_mi2s_res_lock);
Mutex and atomic variables looks redundant here.
Can you explain why do you need both?
Right. Only mutex is required here. I will make count as non-atomic.
...
+
+static int sdm845_sbc_parse_of(struct snd_soc_card *card)
+{
+ struct device *dev = card->dev;
+ struct snd_soc_dai_link *link;
+ struct device_node *np, *codec, *platform, *cpu, *node;
+ int ret, num_links;
+ struct sdm845_snd_data *data;
+
+ ret = snd_soc_of_parse_card_name(card, "qcom,model");
+ if (ret) {
+ dev_err(dev, "Error parsing card name: %d\n", ret);
+ return ret;
+ }
+
+ node = dev->of_node;
+
+ /* DAPM routes */
+ if (of_property_read_bool(node, "qcom,audio-routing")) {
+ ret = snd_soc_of_parse_audio_routing(card,
+ "qcom,audio-routing");
+ if (ret)
+ return ret;
+ }
+
+ /* Populate links */
+ num_links = of_get_child_count(node);
+
+ dev_info(dev, "Found %d child audio dai links..\n", num_links);
Looks unnessary!
Ok . Will remove it in next patchset.
+
+ link->platform_of_node = of_parse_phandle(platform,
+ "sound-dai", 0);
+ if (!link->platform_of_node) {
+ dev_err(card->dev, "error getting platform phandle\n");
+
Re: [alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support
Thanks Vinod for reviewing.
On 6/19/2018 10:35 AM, Vinod wrote:
On 18-06-18, 16:46, Rohit kumar wrote:
+struct sdm845_snd_data {
+ struct snd_soc_card *card;
+ struct regulator *vdd_supply;
+ struct snd_soc_dai_link dai_link[];
+};
+
+static struct mutex pri_mi2s_res_lock;
+static struct mutex quat_tdm_res_lock;
any reason why the locks can't be part of sdm845_snd_data?
Also why do we need two locks ?
No specific reason, I will move it to sdm845_snd_data.
These locks are used to protect enable/disable of bit clocks. We have
Primary MI2S RX/TX
and Quaternary TDM RX/TX interfaces. For primary mi2s rx/tx, we have
single clock which is
synchronized with pri_mi2s_res_lock. For Quat TDM RX/TX, we are using
quat_tdm_res_lock.
We need two locks as we are protecting two different resources.
+static atomic_t pri_mi2s_clk_count;
+static atomic_t quat_tdm_clk_count;
Any specific reason for using atomic variables?
Nothing as such. As we are using mutex to synchronize, we can make it
non- atomic.
Will do it in next-spin.
+static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28};
+
+static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+ int ret = 0;
+ int channels, slot_width;
+
+ channels = params_channels(params);
+ if (channels < 1 || channels > 8) {
I though ch = 0 would be caught by framework and IIRC ASoC doesn't
support more than 8 channels
OK. Will check and remove.
+ pr_err("%s: invalid param channels %d\n",
+ __func__, channels);
+ return -EINVAL;
+ }
+
+ switch (params_format(params)) {
+ case SNDRV_PCM_FORMAT_S32_LE:
+ case SNDRV_PCM_FORMAT_S24_LE:
+ case SNDRV_PCM_FORMAT_S16_LE:
+ slot_width = 32;
+ break;
+ default:
+ pr_err("%s: invalid param format 0x%x\n",
+ __func__, params_format(params));
why not use dev_err, bonus you get device name printer with the logs :)
Sure. Will change it.
+static int sdm845_snd_startup(struct snd_pcm_substream *substream)
+{
+ unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+
+ pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
It is good for debug but not very useful here, so removing it would be
good
OK
+ switch (cpu_dai->id) {
+ case PRIMARY_MI2S_RX:
+ case PRIMARY_MI2S_TX:
+ mutex_lock(&pri_mi2s_res_lock);
+ if (atomic_inc_return(&pri_mi2s_clk_count) == 1) {
+ snd_soc_dai_set_sysclk(cpu_dai,
+ Q6AFE_LPASS_CLK_ID_MCLK_1,
+ DEFAULT_MCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
+ snd_soc_dai_set_sysclk(cpu_dai,
+ Q6AFE_LPASS_CLK_ID_PRI_MI2S_IBIT,
+ DEFAULT_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
+ }
+ mutex_unlock(&pri_mi2s_res_lock);
why do we need locking here? Can you please explain that.
So, we can have two usecases: one with primary mi2s rx and other with
primary mi2s tx.
Lock is required to increment pri_mi2s_clk_count and enable clock so
that disable of one
usecase does not disable the clock.
+ snd_soc_dai_set_fmt(cpu_dai, fmt);
+ break;
empty line after break helps in readability
Sure. Will add that change.
+static int sdm845_sbc_parse_of(struct snd_soc_card *card)
+{
+ struct device *dev = card->dev;
+ struct snd_soc_dai_link *link;
+ struct device_node *np, *codec, *platform, *cpu, *node;
+ int ret, num_links;
+ struct sdm845_snd_data *data;
+
+ ret = snd_soc_of_parse_card_name(card, "qcom,model");
+ if (ret) {
+ dev_err(dev, "Error parsing card name: %d\n", ret);
+ return ret;
+ }
+
+ node = dev->of_node;
+
+ /* DAPM routes */
+ if (of_property_read_bool(node, "qcom,audio-routing")) {
+ ret = snd_soc_of_parse_audio_routing(card,
+ "qcom,audio-routing");
+ if (ret)
+ return ret;
+ }
so if we dont find audio-routing, then? we seems to continue..
Right. Its not mandatory to have qcom,audio-routing in device tree.
Regards,
Rohit
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support
On 18-06-18, 16:46, Rohit kumar wrote:
> +struct sdm845_snd_data {
> + struct snd_soc_card *card;
> + struct regulator *vdd_supply;
> + struct snd_soc_dai_link dai_link[];
> +};
> +
> +static struct mutex pri_mi2s_res_lock;
> +static struct mutex quat_tdm_res_lock;
any reason why the locks can't be part of sdm845_snd_data?
Also why do we need two locks ?
> +static atomic_t pri_mi2s_clk_count;
> +static atomic_t quat_tdm_clk_count;
Any specific reason for using atomic variables?
> +static unsigned int tdm_slot_offset[8] = {0, 4, 8, 12, 16, 20, 24, 28};
> +
> +static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> + int ret = 0;
> + int channels, slot_width;
> +
> + channels = params_channels(params);
> + if (channels < 1 || channels > 8) {
I though ch = 0 would be caught by framework and IIRC ASoC doesn't
support more than 8 channels
> + pr_err("%s: invalid param channels %d\n",
> + __func__, channels);
> + return -EINVAL;
> + }
> +
> + switch (params_format(params)) {
> + case SNDRV_PCM_FORMAT_S32_LE:
> + case SNDRV_PCM_FORMAT_S24_LE:
> + case SNDRV_PCM_FORMAT_S16_LE:
> + slot_width = 32;
> + break;
> + default:
> + pr_err("%s: invalid param format 0x%x\n",
> + __func__, params_format(params));
why not use dev_err, bonus you get device name printer with the logs :)
> +static int sdm845_snd_startup(struct snd_pcm_substream *substream)
> +{
> + unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +
> + pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
It is good for debug but not very useful here, so removing it would be
good
> + switch (cpu_dai->id) {
> + case PRIMARY_MI2S_RX:
> + case PRIMARY_MI2S_TX:
> + mutex_lock(&pri_mi2s_res_lock);
> + if (atomic_inc_return(&pri_mi2s_clk_count) == 1) {
> + snd_soc_dai_set_sysclk(cpu_dai,
> + Q6AFE_LPASS_CLK_ID_MCLK_1,
> + DEFAULT_MCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> + snd_soc_dai_set_sysclk(cpu_dai,
> + Q6AFE_LPASS_CLK_ID_PRI_MI2S_IBIT,
> + DEFAULT_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> + }
> + mutex_unlock(&pri_mi2s_res_lock);
why do we need locking here? Can you please explain that.
> + snd_soc_dai_set_fmt(cpu_dai, fmt);
> + break;
empty line after break helps in readability
> +static int sdm845_sbc_parse_of(struct snd_soc_card *card)
> +{
> + struct device *dev = card->dev;
> + struct snd_soc_dai_link *link;
> + struct device_node *np, *codec, *platform, *cpu, *node;
> + int ret, num_links;
> + struct sdm845_snd_data *data;
> +
> + ret = snd_soc_of_parse_card_name(card, "qcom,model");
> + if (ret) {
> + dev_err(dev, "Error parsing card name: %d\n", ret);
> + return ret;
> + }
> +
> + node = dev->of_node;
> +
> + /* DAPM routes */
> + if (of_property_read_bool(node, "qcom,audio-routing")) {
> + ret = snd_soc_of_parse_audio_routing(card,
> + "qcom,audio-routing");
> + if (ret)
> + return ret;
> + }
so if we dont find audio-routing, then? we seems to continue..
--
~Vinod
