[PATCH 0/2] Graph fixes for using multiple endpoints per port

[PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

Hi all,

Here are two fixes that allow me to have multiple endpoints defined in
the dts file for audio-graph-card. To do that, we need to fix up few
issues as the graph binding Documentation/devicetree/bindings/graph.txt
allows multiple endpoints per port. This allows configuring TDM codecs
for I2S for example.

Kuninori-san, can you please check if this makes sense to you and
compare against the graph binding?

Cheers,

Tony


Tony Lindgren (2):
  ASoC: simple-card-utils: revert port changes to follow graph binding
  ASoC: audio-graph-card: Fix parsing of multiple endpoints

 sound/soc/generic/audio-graph-card.c  | 41 +++++++++++++++++++++++----
 sound/soc/generic/simple-card-utils.c | 22 ++++++++++++--
 2 files changed, 55 insertions(+), 8 deletions(-)

-- 
2.19.2

[PATCH 1/2] ASoC: simple-card-utils: revert port changes to follow graph binding

[Tony Lindgren]

Commit b6f3fc005a2c ("ASoC: simple-card-utils: fixup
asoc_simple_card_get_dai_id() counting") changed endpoint parsing for
asoc_simple_card_get_dai_id(), but it seems the old code is correct.

This code should follow the generic binding documentation for
Documentation/devicetree/bindings/graph.txt that allows multiple
endpoints for each port.

Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 sound/soc/generic/simple-card-utils.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -269,19 +269,35 @@ EXPORT_SYMBOL_GPL(asoc_simple_card_parse_dai);
 
 static int asoc_simple_card_get_dai_id(struct device_node *ep)
 {
-	struct of_endpoint info;
+	struct device_node *node;
+	struct device_node *endpoint;
+	int i, id;
 	int ret;
 
 	ret = snd_soc_get_dai_id(ep);
 	if (ret != -ENOTSUPP)
 		return ret;
 
+	node = of_graph_get_port_parent(ep);
+
 	/*
 	 * Non HDMI sound case, counting port/endpoint on its DT
 	 * is enough. Let's count it.
 	 */
-	of_graph_parse_endpoint(ep, &info);
-	return info.port;
+	i = 0;
+	id = -1;
+	for_each_endpoint_of_node(node, endpoint) {
+		if (endpoint == ep)
+			id = i;
+		i++;
+	}
+
+	of_node_put(node);
+
+	if (id < 0)
+		return -ENODEV;
+
+	return id;
 }
 
 int asoc_simple_card_parse_graph_dai(struct device_node *ep,
-- 
2.19.2

[PATCH 2/2] ASoC: audio-graph-card: Fix parsing of multiple endpoints

[Tony Lindgren]

We currently use only the first endpoint even if multiple endpoints
are configured for a port.

We should follow what's specified in the device graph binding
document Documentation/devicetree/bindings/graph.txt in paragraph
"Organisation of ports and endpoints":

"If a single port is connected to more than one remote device, an
 'endpoint' child node must be provided for each link."

This is the case for example for I2S where multiple devices can be
connected to a single I2S port sharing it using TDM (Time-Division
Multiplexing).

To fix this, we need to iterate over each endpoint for a port.

Note that the dts "dais" property should still contain a single
property for each port, and not for each endpoint.

Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 sound/soc/generic/audio-graph-card.c | 41 ++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c
--- a/sound/soc/generic/audio-graph-card.c
+++ b/sound/soc/generic/audio-graph-card.c
@@ -156,17 +156,17 @@ static int asoc_graph_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
-static int asoc_graph_card_dai_link_of(struct device_node *cpu_port,
+static int asoc_graph_card_ep_of(struct device_node *cpu_port,
+					struct device_node *cpu_ep,
 					struct graph_card_data *priv,
 					int *dai_idx, int link_idx)
 {
 	struct device *dev = graph_priv_to_dev(priv);
 	struct snd_soc_dai_link *dai_link = graph_priv_to_link(priv, link_idx);
 	struct graph_dai_props *dai_props = graph_priv_to_props(priv, link_idx);
+	struct device_node *codec_ep = of_graph_get_remote_endpoint(cpu_ep);
 	struct asoc_simple_dai *cpu_dai;
 	struct asoc_simple_dai *codec_dai;
-	struct device_node *cpu_ep    = of_get_next_child(cpu_port, NULL);
-	struct device_node *codec_ep = of_graph_get_remote_endpoint(cpu_ep);
 	int ret;
 
 	cpu_dai			=
@@ -230,6 +230,31 @@ static int asoc_graph_card_dai_link_of(struct device_node *cpu_port,
 	return ret;
 }
 
+static int asoc_graph_card_dai_link_of(struct device_node *cpu_port,
+					struct graph_card_data *priv,
+					int *dai_idx, int link_idx)
+{
+	struct device *dev = graph_priv_to_dev(priv);
+	struct device_node *ep = NULL;
+	int num_ep, ep_idx, ret;
+
+	num_ep = of_get_child_count(cpu_port);
+	if (!num_ep) {
+		dev_err(dev, "no cpu endpoints found for %pOf\n", cpu_port);
+		return -ENODEV;
+	}
+
+	for (ep_idx = 0; ep_idx < num_ep; ep_idx++) {
+		ep = of_get_next_child(cpu_port, ep);
+		ret = asoc_graph_card_ep_of(cpu_port, ep, priv,
+					    dai_idx, link_idx + ep_idx);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
 static int asoc_graph_card_parse_of(struct graph_card_data *priv)
 {
 	struct of_phandle_iterator it;
@@ -272,8 +297,14 @@ static int asoc_graph_get_dais_count(struct device *dev)
 	int count = 0;
 	int rc;
 
-	of_for_each_phandle(&it, rc, node, "dais", NULL, 0)
-		count++;
+	of_for_each_phandle(&it, rc, node, "dais", NULL, 0) {
+		int ep_count = of_get_child_count(it.node);
+
+		if (ep_count)
+			count += ep_count;
+		else
+			count++;
+	}
 
 	return count;
 }
-- 
2.19.2

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony

> Here are two fixes that allow me to have multiple endpoints defined in
> the dts file for audio-graph-card. To do that, we need to fix up few
> issues as the graph binding Documentation/devicetree/bindings/graph.txt
> allows multiple endpoints per port. This allows configuring TDM codecs
> for I2S for example.
> 
> Kuninori-san, can you please check if this makes sense to you and
> compare against the graph binding?

This looks a little bit strange for me.
Can you show me your DT for it ?

Best regards
---
Kuninori Morimoto

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181211 03:31]:
> 
> Hi Tony
> 
> > Here are two fixes that allow me to have multiple endpoints defined in
> > the dts file for audio-graph-card. To do that, we need to fix up few
> > issues as the graph binding Documentation/devicetree/bindings/graph.txt
> > allows multiple endpoints per port. This allows configuring TDM codecs
> > for I2S for example.
> > 
> > Kuninori-san, can you please check if this makes sense to you and
> > compare against the graph binding?
> 
> This looks a little bit strange for me.
> Can you show me your DT for it ?

Sure, adding also Sebastian to Cc. Here's what I currently have for droid 4
dts with two codecs on I2S. Please just ignore the GNSS parts there..

The TDM configuration is all done in the cpcap_audio_codec via set_tdm_slot().
The modem voice call codec is a serdev driver :) I'll need some more time to
be able to post patches but it's basically working for voice calls.

Regards,

Tony

8< ---------
diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
--- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
+++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
@@ -653,6 +653,28 @@
 	pinctrl-0 = <&uart1_pins>;
 	interrupts-extended = <&wakeupgen GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH
 			       &omap4_pmx_core 0xfc>;
+
+	modem {
+		compatible = "motorola,mapphone-mdm6600-serdev";
+		phys = <&fsusb1_phy>;
+		phy-names = "usb";
+
+		mot_mdm6600_audio: audio-codec {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			#sound-dai-cells = <1>;
+
+			port@0 {
+				mot_mdm6600_audio_codec0: endpoint {
+					remote-endpoint = <&cpu_dai_mdm>;
+				};
+			};
+		};
+
+		gnss {
+			compatible = "motorola,mapphone-mdm6600-gnss";
+		};
+	};
 };
 
 &uart3 {
@@ -746,12 +768,18 @@
 	status = "okay";
 
 	mcbsp3_port: port {
-		cpu_dai3: endpoint {
+		cpu_dai3: endpoint@0 {
 			dai-format = "dsp_a";
 			frame-master = <&cpcap_audio_codec1>;
 			bitclock-master = <&cpcap_audio_codec1>;
 			remote-endpoint = <&cpcap_audio_codec1>;
 		};
+		cpu_dai_mdm: endpoint@1 {
+			dai-format = "dsp_a";
+			frame-master = <&cpcap_audio_codec1>;
+			bitclock-master = <&cpcap_audio_codec1>;
+			remote-endpoint = <&mot_mdm6600_audio_codec0>;
+		};
 	};
 };
 

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony

> > This looks a little bit strange for me.
> > Can you show me your DT for it ?
> 
> Sure, adding also Sebastian to Cc. Here's what I currently have for droid 4
> dts with two codecs on I2S. Please just ignore the GNSS parts there..
> 
> The TDM configuration is all done in the cpcap_audio_codec via set_tdm_slot().
> The modem voice call codec is a serdev driver :) I'll need some more time to
> be able to post patches but it's basically working for voice calls.

Hmm... it seems strange...
I guess you are using "ti,omap4-mcbsp" driver (= linux/sound/soc/omap/omap-mcbsp.c)
Is this correct ?

If so, your driver is registering component as

	ret = devm_snd_soc_register_component(&pdev->dev,
					      &omap_mcbsp_component,
					      &omap_mcbsp_dai, 1);

Your driver has only 1 DAI.

>  	mcbsp3_port: port {
> -		cpu_dai3: endpoint {
> +		cpu_dai3: endpoint@0 {
>  			dai-format = "dsp_a";
>  			frame-master = <&cpcap_audio_codec1>;
>  			bitclock-master = <&cpcap_audio_codec1>;
>  			remote-endpoint = <&cpcap_audio_codec1>;
>  		};
> +		cpu_dai_mdm: endpoint@1 {
> +			dai-format = "dsp_a";
> +			frame-master = <&cpcap_audio_codec1>;
> +			bitclock-master = <&cpcap_audio_codec1>;
> +			remote-endpoint = <&mot_mdm6600_audio_codec0>;
> +		};

And here, you have 1 port, 2 endpoint.
Then, asoc_simple_card_get_dai_id() should return 1.

And, your [2/2] patch,
I guess you are misunderstanding about "port" vs "endpoint",
or omap-mcbsp driver side need to update ?

Best regards
---
Kuninori Morimoto

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony, again

> >  	mcbsp3_port: port {
> > -		cpu_dai3: endpoint {
> > +		cpu_dai3: endpoint@0 {
> >  			dai-format = "dsp_a";
> >  			frame-master = <&cpcap_audio_codec1>;
> >  			bitclock-master = <&cpcap_audio_codec1>;
> >  			remote-endpoint = <&cpcap_audio_codec1>;
> >  		};
> > +		cpu_dai_mdm: endpoint@1 {
> > +			dai-format = "dsp_a";
> > +			frame-master = <&cpcap_audio_codec1>;
> > +			bitclock-master = <&cpcap_audio_codec1>;
> > +			remote-endpoint = <&mot_mdm6600_audio_codec0>;
> > +		};

audio-graph-scu-card and my posting merged audio-graph-card
are assuming multi-endpoint will be used for DPCM purpose.

But, above endpoint connection seems you want to is
not DPCM (?), I'm not sure.

Best regards
---
Kuninori Morimoto

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181211 05:30]:
> 
> Hi Tony, again
> 
> > >  	mcbsp3_port: port {
> > > -		cpu_dai3: endpoint {
> > > +		cpu_dai3: endpoint@0 {
> > >  			dai-format = "dsp_a";
> > >  			frame-master = <&cpcap_audio_codec1>;
> > >  			bitclock-master = <&cpcap_audio_codec1>;
> > >  			remote-endpoint = <&cpcap_audio_codec1>;
> > >  		};
> > > +		cpu_dai_mdm: endpoint@1 {
> > > +			dai-format = "dsp_a";
> > > +			frame-master = <&cpcap_audio_codec1>;
> > > +			bitclock-master = <&cpcap_audio_codec1>;
> > > +			remote-endpoint = <&mot_mdm6600_audio_codec0>;
> > > +		};
> 
> audio-graph-scu-card and my posting merged audio-graph-card
> are assuming multi-endpoint will be used for DPCM purpose.
> 
> But, above endpoint connection seems you want to is
> not DPCM (?), I'm not sure.

Yes DPCM should work nicely here :)

So to recap, Sebastian already added the cpcap codec a while back,
please see arch/arm/boot/dts/omap4-droid4-xt894.dts for the soundcard
node. Then see the link below for an earlier email describing how the
various components are wired for TDM [0].

Hope that clarifies things a bit more,

Tony

[0] https://lkml.org/lkml/2018/3/28/405

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181211 05:16]:
> 
> Hi Tony
> 
> > > This looks a little bit strange for me.
> > > Can you show me your DT for it ?
> > 
> > Sure, adding also Sebastian to Cc. Here's what I currently have for droid 4
> > dts with two codecs on I2S. Please just ignore the GNSS parts there..
> > 
> > The TDM configuration is all done in the cpcap_audio_codec via set_tdm_slot().
> > The modem voice call codec is a serdev driver :) I'll need some more time to
> > be able to post patches but it's basically working for voice calls.
> 
> Hmm... it seems strange...
> I guess you are using "ti,omap4-mcbsp" driver (= linux/sound/soc/omap/omap-mcbsp.c)
> Is this correct ?

Yes.

> If so, your driver is registering component as
> 
> 	ret = devm_snd_soc_register_component(&pdev->dev,
> 					      &omap_mcbsp_component,
> 					      &omap_mcbsp_dai, 1);
> 
> Your driver has only 1 DAI.

Yes I have a patch for omap-mcbsp.c too :)

> >  	mcbsp3_port: port {
> > -		cpu_dai3: endpoint {
> > +		cpu_dai3: endpoint@0 {
> >  			dai-format = "dsp_a";
> >  			frame-master = <&cpcap_audio_codec1>;
> >  			bitclock-master = <&cpcap_audio_codec1>;
> >  			remote-endpoint = <&cpcap_audio_codec1>;
> >  		};
> > +		cpu_dai_mdm: endpoint@1 {
> > +			dai-format = "dsp_a";
> > +			frame-master = <&cpcap_audio_codec1>;
> > +			bitclock-master = <&cpcap_audio_codec1>;
> > +			remote-endpoint = <&mot_mdm6600_audio_codec0>;
> > +		};
> 
> And here, you have 1 port, 2 endpoint.
> Then, asoc_simple_card_get_dai_id() should return 1.
> 
> And, your [2/2] patch,
> I guess you are misunderstanding about "port" vs "endpoint",
> or omap-mcbsp driver side need to update ?

Yes omap-mcbsp driver needs to be updated for multiple endpoints.

Adding Jarkko and Peter also to Cc, below is the WIP patch that I'm
currently using for omap-mcbsp to add more DAIs.

So far nothing else to do in the omap-mcbsp as it's the cpcap hardware
that configures the TDM timeslots. And I'm currently assuming the
first instance is the master, I guess that should be parsed from the
the frame-master dts property instead.

Regards,

Tony

8< ----------------------
diff --git a/sound/soc/omap/omap-mcbsp-priv.h b/sound/soc/omap/omap-mcbsp-priv.h
--- a/sound/soc/omap/omap-mcbsp-priv.h
+++ b/sound/soc/omap/omap-mcbsp-priv.h
@@ -262,6 +262,8 @@ struct omap_mcbsp {
 	struct omap_mcbsp_platform_data *pdata;
 	struct omap_mcbsp_st_data *st_data;
 	struct omap_mcbsp_reg_cfg cfg_regs;
+	struct snd_soc_dai_driver *dais;
+	int dai_count;
 	struct snd_dmaengine_dai_dma_data dma_data[2];
 	unsigned int dma_req[2];
 	int dma_op_mode;
diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -28,6 +28,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_graph.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -1318,23 +1319,53 @@ static int omap_mcbsp_remove(struct snd_soc_dai *dai)
 	return 0;
 }
 
-static struct snd_soc_dai_driver omap_mcbsp_dai = {
-	.probe = omap_mcbsp_probe,
-	.remove = omap_mcbsp_remove,
-	.playback = {
-		.channels_min = 1,
-		.channels_max = 16,
-		.rates = OMAP_MCBSP_RATES,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
-	},
-	.capture = {
-		.channels_min = 1,
-		.channels_max = 16,
-		.rates = OMAP_MCBSP_RATES,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
-	},
-	.ops = &mcbsp_dai_ops,
-};
+static int omap_mcbsp_init_dais(struct omap_mcbsp *mcbsp)
+{
+	struct device_node *np = mcbsp->dev->of_node;
+	int i;
+
+	if (np)
+		mcbsp->dai_count = of_graph_get_endpoint_count(np);
+
+	if (!mcbsp->dai_count)
+		mcbsp->dai_count = 1;
+
+	mcbsp->dais = devm_kcalloc(mcbsp->dev, mcbsp->dai_count,
+				   sizeof(*mcbsp->dais), GFP_KERNEL);
+	if (!mcbsp->dais)
+		return -ENOMEM;
+
+	for (i = 0; i < mcbsp->dai_count; i++) {
+		struct snd_soc_dai_driver *dai = &mcbsp->dais[i];
+
+		dai->name = devm_kasprintf(mcbsp->dev, GFP_KERNEL, "%s-dai%i",
+					   dev_name(mcbsp->dev), i);
+
+		if (i == 0) {
+			dai->probe = omap_mcbsp_probe;
+			dai->remove = omap_mcbsp_remove;
+			dai->ops = &mcbsp_dai_ops;
+		}
+		dai->playback.channels_min = 1;
+		dai->playback.channels_max = 16;
+		dai->playback.rates = OMAP_MCBSP_RATES;
+		if (mcbsp->pdata->reg_size == 2)
+			dai->playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
+		else
+			dai->playback.formats = SNDRV_PCM_FMTBIT_S16_LE |
+						SNDRV_PCM_FMTBIT_S32_LE;
+		dai->capture.channels_min = 1;
+		dai->capture.channels_max = 16;
+		dai->capture.rates = OMAP_MCBSP_RATES;
+		if (mcbsp->pdata->reg_size == 2)
+			dai->capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
+		else
+			dai->capture.formats = SNDRV_PCM_FMTBIT_S16_LE |
+					       SNDRV_PCM_FMTBIT_S32_LE;
+	}
+
+	return 0;
+}
 
 static const struct snd_soc_component_driver omap_mcbsp_component = {
 	.name		= "omap-mcbsp",
@@ -1423,18 +1454,17 @@ static int asoc_mcbsp_probe(struct platform_device *pdev)
 	mcbsp->dev = &pdev->dev;
 	platform_set_drvdata(pdev, mcbsp);
 
-	ret = omap_mcbsp_init(pdev);
+	ret = omap_mcbsp_init_dais(mcbsp);
 	if (ret)
 		return ret;
 
-	if (mcbsp->pdata->reg_size == 2) {
-		omap_mcbsp_dai.playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
-		omap_mcbsp_dai.capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
-	}
+	ret = omap_mcbsp_init(pdev);
+	if (ret)
+		return ret;
 
 	ret = devm_snd_soc_register_component(&pdev->dev,
 					      &omap_mcbsp_component,
-					      &omap_mcbsp_dai, 1);
+					      mcbsp->dais, mcbsp->dai_count);
 	if (ret)
 		return ret;
 

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony

> > And, your [2/2] patch,
> > I guess you are misunderstanding about "port" vs "endpoint",
> > or omap-mcbsp driver side need to update ?
> 
> Yes omap-mcbsp driver needs to be updated for multiple endpoints.
> 
> Adding Jarkko and Peter also to Cc, below is the WIP patch that I'm
> currently using for omap-mcbsp to add more DAIs.
> 
> So far nothing else to do in the omap-mcbsp as it's the cpcap hardware
> that configures the TDM timeslots. And I'm currently assuming the
> first instance is the master, I guess that should be parsed from the
> the frame-master dts property instead.
(snip)
> +	if (np)
> +		mcbsp->dai_count = of_graph_get_endpoint_count(np);

OK, you have multi DAI.
Then, you need to count is "port", not "endpoint".

So, your DT will be below.
You want to have is *1 for asoc_simple_card_get_dai_id().
You want to double check is *2 (maybe).

	ports {
		mcbsp3_port0: port@0 {
*1			reg = <0>;
			cpu_dai3: endpoint {
	 			dai-format = "dsp_a";
 				frame-master = <&cpcap_audio_codec1>;
 				bitclock-master = <&cpcap_audio_codec1>;
 				remote-endpoint = <&cpcap_audio_codec1>;
	 		};
		};
		mcbsp3_port1: port@1 {
*1			reg = <1>;
			cpu_dai_mdm: endpoint {
				dai-format = "dsp_a";
*2				frame-master = <&cpcap_audio_codec1>;
*2				bitclock-master = <&cpcap_audio_codec1>;
				remote-endpoint = <&mot_mdm6600_audio_codec0>;
			};
	 	};
	};


Best regards
---
Kuninori Morimoto

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

Hi,

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181211 06:14]:
> 
> Hi Tony
> 
> > > And, your [2/2] patch,
> > > I guess you are misunderstanding about "port" vs "endpoint",
> > > or omap-mcbsp driver side need to update ?
> > 
> > Yes omap-mcbsp driver needs to be updated for multiple endpoints.
> > 
> > Adding Jarkko and Peter also to Cc, below is the WIP patch that I'm
> > currently using for omap-mcbsp to add more DAIs.
> > 
> > So far nothing else to do in the omap-mcbsp as it's the cpcap hardware
> > that configures the TDM timeslots. And I'm currently assuming the
> > first instance is the master, I guess that should be parsed from the
> > the frame-master dts property instead.
> (snip)
> > +	if (np)
> > +		mcbsp->dai_count = of_graph_get_endpoint_count(np);
> 
> OK, you have multi DAI.
> Then, you need to count is "port", not "endpoint".

The issue I have with that it does not then follow the binding doc :)

See this part in Documentation/devicetree/bindings/graph.txt:

 "If a single port is connected to more than one remote device, an
 'endpoint' child node must be provided for each link."

Isn't the I2C TDM case the same as "single port connecected to
more than one remote device" rather than multiple ports?

To me it seems we're currently only handling the multiple ports
case, and not multiple endpoints for a port. Other than fixing
that, things should work just as earlier with my two patches.
That is unless I accidentally broke something.

So just trying to correct the binding usage. Or am I missing
something?

Regards,

Tony

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony

> The issue I have with that it does not then follow the binding doc :)
> 
> See this part in Documentation/devicetree/bindings/graph.txt:
> 
>  "If a single port is connected to more than one remote device, an
>  'endpoint' child node must be provided for each link."
> 
> Isn't the I2C TDM case the same as "single port connecected to
> more than one remote device" rather than multiple ports?
> 
> To me it seems we're currently only handling the multiple ports
> case, and not multiple endpoints for a port. Other than fixing
> that, things should work just as earlier with my two patches.
> That is unless I accidentally broke something.
> 
> So just trying to correct the binding usage. Or am I missing
> something?

I'm not 100% sure your "I2C TDM case", but you can check
multi-endpoint sample on "Example: Multi DAI with DPCM" below.
"pcm3168a" is using multi-endpoint.
Does this help you ?

	https://patchwork.kernel.org/patch/10712877/


Best regards
---
Kuninori Morimoto

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony, again

> > The issue I have with that it does not then follow the binding doc :)
> > 
> > See this part in Documentation/devicetree/bindings/graph.txt:
> > 
> >  "If a single port is connected to more than one remote device, an
> >  'endpoint' child node must be provided for each link."

My understanding is that 1 "port" is for 1 "physical interface".
In sound case, it is 1 "DAI".
And, 1 "endpoint" is for 1 "connection".

	"If a single port is connected to more than one remote device, an
	'endpoint' child node must be provided for each link."

This meanns, "If 1 DAI (*1) is connected to multiple remote DAIs(*2),
this connection is indicated by multiple "endpoint"" or something like that.

(*2)
DAIA-endpoint---endpoint--\
DAIB-endpoint---endpoint-----DAI (*1)
DAIC-endpoint---endpoint--/

> > Isn't the I2C TDM case the same as "single port connecected to
> > more than one remote device" rather than multiple ports?

I re-checked https://lkml.org/lkml/2018/3/28/405.
Are MDM6600 / OMAP4 CPU portion, and are CPCAP / WL1285 Codec portion ?
Then, it is not yet supported (on ALSA SoC level?).
If my memory was correct, Lars-Peter had some idea for Mux,
But, not yet implemented I think.

audio-graph[-scu] / simple-card[-scu] are supporting DPCM,
but it is for multiple CPU - single Codec.

Best regards
---
Kuninori Morimoto

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181212 00:12]:
> 
> Hi Tony, again
> 
> > > The issue I have with that it does not then follow the binding doc :)
> > > 
> > > See this part in Documentation/devicetree/bindings/graph.txt:
> > > 
> > >  "If a single port is connected to more than one remote device, an
> > >  'endpoint' child node must be provided for each link."
> 
> My understanding is that 1 "port" is for 1 "physical interface".

Yes I agree.

> In sound case, it is 1 "DAI".
> And, 1 "endpoint" is for 1 "connection".

Yes. So I have 1 physical port (mcbsp) TDM split between
two codecs (cpcap and mdm).

> 	"If a single port is connected to more than one remote device, an
> 	'endpoint' child node must be provided for each link."
> 
> This meanns, "If 1 DAI (*1) is connected to multiple remote DAIs(*2),
> this connection is indicated by multiple "endpoint"" or something like that.
> 
> (*2)
> DAIA-endpoint---endpoint--\
> DAIB-endpoint---endpoint-----DAI (*1)
> DAIC-endpoint---endpoint--/

Yeah. So the only thing missing is parsing multiple endpoints
at the DAI end :) And that's why the two patches I posted.

> > > Isn't the I2C TDM case the same as "single port connecected to
> > > more than one remote device" rather than multiple ports?
> 
> I re-checked https://lkml.org/lkml/2018/3/28/405.
> Are MDM6600 / OMAP4 CPU portion, and are CPCAP / WL1285 Codec portion ?

MDM6600 is a Qualcomm modem running Motorola custom firmware
CPCAP is a Motorola custom PMIC for omap4430 SoC
WL128 is a WLAN and Bluetooth chip

So following your drawing above:

There are two separate McBSP instances, here's the one
in question:

MDM6600 modem-----endpoint--\
CPCAP PMIC codec2-endpoint----McBSP3 on SoC
WL1285 Bluetooth--endpoint--/

The other McBSP instance is dedicated for SoC audio:

CPCAP PMIC codec1-endpoint---McBSP3 on SoC

> Then, it is not yet supported (on ALSA SoC level?).

Only the cpcap codec is in the mainline currently, the mdm
codec driver has not yet been posted as it depends on some
ts 27.010 serdev patches.

> If my memory was correct, Lars-Peter had some idea for Mux,
> But, not yet implemented I think.

Hmm well I don't think much else is needed currently, we
already have everything needed at the ASoC level. See yet
another WIP patch configuring the TDM for mdm codec voice
call by the existing cpcap codec driver below just by
implementing .set_tdm_slot function.

> audio-graph[-scu] / simple-card[-scu] are supporting DPCM,
> but it is for multiple CPU - single Codec.

Well with my patches I certainly have the above configuration
working just fine with two audio-graph-card instances connected
to a single physical McBSP port.

Regards,

Tony

8< --------------------
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
@@ -251,6 +259,8 @@ struct cpcap_audio {
 	int codec_clk_id;
 	int codec_freq;
 	int codec_format;
+
+	unsigned int voice_call:1;
 };
 
 static int cpcap_st_workaround(struct snd_soc_dapm_widget *w,
@@ -1370,6 +1380,114 @@ static int cpcap_voice_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+/*
+ * Configure voice call if cpcap->voice_call is set.
+ *
+ * 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 piceses as CPU is not involved in the voice call.
+ */
+static int cpcap_voice_call(struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct cpcap_audio *cpcap = snd_soc_component_get_drvdata(component);
+	int mask, err;
+
+	/* Maybe enable modem to codec VAUDIO_MODE1? */
+	mask = BIT(CPCAP_BIT_VAUDIO_MODE1);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_VAUDIOC,
+				 mask, cpcap->voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Maybe clear MIC1_MUX? */
+	mask = BIT(CPCAP_BIT_MIC1_MUX);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_TXI,
+				 mask, cpcap->voice_call ? 0 : mask);
+	if (err)
+		return err;
+
+	/* Maybe set MIC2_MUX? */
+	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, cpcap->voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Maybe enable LDSP? */
+	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, cpcap->voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Maybe 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, cpcap->voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Maybe unmute voice? */
+	err = snd_soc_dai_digital_mute(dai, !cpcap->voice_call,
+				       SNDRV_PCM_STREAM_PLAYBACK);
+	if (err)
+		return err;
+
+	/* Maybe enable modem to codec mic CDC and HPF? */
+	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, cpcap->voice_call ? mask : 0);
+	if (err)
+		return err;
+
+	/* Maybe enable modem to codec CDC? */
+	mask = BIT(CPCAP_BIT_CDC_CLK_EN);
+	err = regmap_update_bits(cpcap->regmap, CPCAP_REG_CDI,
+				 mask, cpcap->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;
+
+	/* Modem to codec audio with no CPU involved? */
+	if (tx_mask == 0 && rx_mask == 1 && slot_width == 8)
+		cpcap->voice_call = true;
+	else
+		cpcap->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(dai);
+	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 +1509,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,
 };
 

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [181212 00:43]:
> The other McBSP instance is dedicated for SoC audio:
> 
> CPCAP PMIC codec1-endpoint---McBSP3 on SoC

Sorry this should be McBSP2, not McBSP3 above for
the SoC dedicated audio port.

Tony

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181211 23:16]:
> 
> Hi Tony
> 
> > The issue I have with that it does not then follow the binding doc :)
> > 
> > See this part in Documentation/devicetree/bindings/graph.txt:
> > 
> >  "If a single port is connected to more than one remote device, an
> >  'endpoint' child node must be provided for each link."
> > 
> > Isn't the I2C TDM case the same as "single port connecected to
> > more than one remote device" rather than multiple ports?
> > 
> > To me it seems we're currently only handling the multiple ports
> > case, and not multiple endpoints for a port. Other than fixing
> > that, things should work just as earlier with my two patches.
> > That is unless I accidentally broke something.
> > 
> > So just trying to correct the binding usage. Or am I missing
> > something?
> 
> I'm not 100% sure your "I2C TDM case", but you can check
> multi-endpoint sample on "Example: Multi DAI with DPCM" below.
> "pcm3168a" is using multi-endpoint.
> Does this help you ?
> 
> 	https://patchwork.kernel.org/patch/10712877/

Hmm, so do you have multiple separate ports at the "&sound" node
hardware? If so then yeah multiple ports make sense.

But if you only a single physical (I2S?) port at the
"&sound" node hardware, then IMO you should only have one
port and multiple endpoints there according to the graph.txt
binding doc.

In my McBSP case there is only a single physical I2S port
that can be TDM split into timeslots.

Regards,

Tony

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony

> > 	https://patchwork.kernel.org/patch/10712877/
> 
> Hmm, so do you have multiple separate ports at the "&sound" node
> hardware? If so then yeah multiple ports make sense.
>
> But if you only a single physical (I2S?) port at the
> "&sound" node hardware, then IMO you should only have one
> port and multiple endpoints there according to the graph.txt
> binding doc.
> 
> In my McBSP case there is only a single physical I2S port
> that can be TDM split into timeslots.

Mine has 4 DAIs. Each DAI can output 2ch.
These will be merged and wil be 8ch TDM and goes to Codec.
But hmm.. it is 4 DAIs, but 1 "physical" interface...

So, your patch seems correct, but will breaks DPCM...
I will confirm it.

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony, again

> > > 	https://patchwork.kernel.org/patch/10712877/
> > 
> > Hmm, so do you have multiple separate ports at the "&sound" node
> > hardware? If so then yeah multiple ports make sense.
> >
> > But if you only a single physical (I2S?) port at the
> > "&sound" node hardware, then IMO you should only have one
> > port and multiple endpoints there according to the graph.txt
> > binding doc.
> > 
> > In my McBSP case there is only a single physical I2S port
> > that can be TDM split into timeslots.
> 
> Mine has 4 DAIs. Each DAI can output 2ch.
> These will be merged and wil be 8ch TDM and goes to Codec.
> But hmm.. it is 4 DAIs, but 1 "physical" interface...
> 
> So, your patch seems correct, but will breaks DPCM...
> I will confirm it.

I thought "port" = "DAI", but yeah, "port" = "physical interface".
Then, my issue is that we can't judge DAI size from DT.
For example, MIXer case, 2 CPU DAIs are connected to 1 Codec.

	DAI0 --- CPU --- Codec
	DAI1 /

In this case, CPU side needs 2 DAIs,
Codec side needs 1 DAI only.
For both CPU/Codec case, OF graph will be like below,
and we can't judge DAIs size from this.

	port {
		ep0: endpint@0 {
			remote-endpoint = <xxx>;
		};
		ep1: endpint@1 {
			remote-endpoint = <xxx>;
		};
	}

To solve this issue, we need to use "reg" for it.
Then, we can get correct DAI ID.

	/* CPU has 2 DAIs */
	CPU {
		port {
			ep0: endpint@0 {
				reg = <0>;
				remote-endpoint = <xxx>;
			};
			ep1: endpint@1 {
				reg = <1>;
				remote-endpoint = <xxx>;
			};
		};
	}

	/* Codec has 1 DAI */
	Codec {
		port {
			reg = <0>;
			ep0: endpint@0 {
				remote-endpoint = <xxx>;
			};
			ep1: endpint@1 {
				remote-endpoint = <xxx>;
			};
		};
	}


Can you agree this ? we need extra patch,
but it can solve your / my problem.

Now I'm posting patches to merging
"audio-graph-card" and "DPCM ver audio-graph-card".
If you are OK, I will include above solution patch
to this patch-set.

Current audio-graph doesn't expect your use-case,
and I want to avoid conflict.

So, "merged" audio-graph should solve your use-case.
If you can agree about this, I will post patch-set.

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181212 06:52]:
> 
> Hi Tony, again
> 
> > > > 	https://patchwork.kernel.org/patch/10712877/
> > > 
> > > Hmm, so do you have multiple separate ports at the "&sound" node
> > > hardware? If so then yeah multiple ports make sense.
> > >
> > > But if you only a single physical (I2S?) port at the
> > > "&sound" node hardware, then IMO you should only have one
> > > port and multiple endpoints there according to the graph.txt
> > > binding doc.
> > > 
> > > In my McBSP case there is only a single physical I2S port
> > > that can be TDM split into timeslots.
> > 
> > Mine has 4 DAIs. Each DAI can output 2ch.
> > These will be merged and wil be 8ch TDM and goes to Codec.
> > But hmm.. it is 4 DAIs, but 1 "physical" interface...
> > 
> > So, your patch seems correct, but will breaks DPCM...
> > I will confirm it.
> 
> I thought "port" = "DAI", but yeah, "port" = "physical interface".

OK good to hear :)

> Then, my issue is that we can't judge DAI size from DT.
> For example, MIXer case, 2 CPU DAIs are connected to 1 Codec.
> 
> 	DAI0 --- CPU --- Codec
> 	DAI1 /
> 
> In this case, CPU side needs 2 DAIs,
> Codec side needs 1 DAI only.

Oh so the other way around compared to my use case. Hmm.

> For both CPU/Codec case, OF graph will be like below,
> and we can't judge DAIs size from this.
> 
> 	port {
> 		ep0: endpint@0 {
> 			remote-endpoint = <xxx>;
> 		};
> 		ep1: endpint@1 {
> 			remote-endpoint = <xxx>;
> 		};
> 	}

Hmm I too need to add secondary DAIs for McBSP in addition to the
primary DAI controlling the McBSP hardware resources.

> To solve this issue, we need to use "reg" for it.
> Then, we can get correct DAI ID.

Hmm yeah maybe. Just to think of other options, maybe also the
#size-cells could be used?

As the binding allows adding #address-cells and #size-cells to
the port node.. Usually if you refer a subnode of a device you
just use #address=cells = <2> where the second field would be for
the offset. So maybe this could be used for 1 DAI this way:

 	/* Codec has 1 DAI */
 	Codec {
 		port {
			#address-cells = <2>;
			#size-cells = <1>;

 			ep: endpoint {
 				remote-endpoint = <xxx>;
 			};
 		};
 	}

Where this codec would then need to be referenced with just an
additional instance number:

foo = <&ep 0>;
bar = <&ep 1>;
...

And then for a codec with 2 DAIs the usual #size-cells = <1>
would be used with numbered endpoints for each DAI the way
you already described:

 	port {
 		ep0: endpint@0 {
 			remote-endpoint = <xxx>;
 		};
 		ep1: endpint@1 {
 			remote-endpoint = <xxx>;
 		};
 	}

Do you think that would work?

> Can you agree this ? we need extra patch,
> but it can solve your / my problem.

Yes it's starting to make sense :)

> Now I'm posting patches to merging
> "audio-graph-card" and "DPCM ver audio-graph-card".
> If you are OK, I will include above solution patch
> to this patch-set.

Sure, maybe please first check if the #size-cells = <2>
option would work though?

> Current audio-graph doesn't expect your use-case,
> and I want to avoid conflict.
> 
> So, "merged" audio-graph should solve your use-case.
> If you can agree about this, I will post patch-set.

Yeah I agree, just still wondering what might be the best
way to represent 1 DAI vs 2 DAIs.

Regards,

Tony

Re: [alsa-devel] [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony

>  	/* Codec has 1 DAI */
>  	Codec {
>  		port {
> 			#address-cells = <2>;
> 			#size-cells = <1>;
> 
>  			ep: endpoint {
>  				remote-endpoint = <xxx>;
>  			};
>  		};
>  	}
(snip)
> foo = <&ep 0>;
> bar = <&ep 1>;

Ahh, it looks nice idea !
but hmm..., it seems dtc doesn't allow us to use #address-cells = <2>
for "remote-endpoint".

--- ${LINUX}/scripts/dtc/checks.c ---
static struct node *get_remote_endpoint(struct check *c, struct dt_info *dti,
					struct node *endpoint)
{
	...
	prop = get_property(endpoint, "remote-endpoint");
	if (!prop)
		return NULL;

=>	phandle = propval_cell(prop);
	/* Give up if this is an overlay with external references */
	...
}

I'm not good at dtc, but propval_cell() is assuming it is single address cell.
We will have many assert warning on

	remote-endpoint = <&xxx 0>;

Can you please double check it ?

And, I'm wonder remote-endpoint need to cross pointing, right ?
one way looks nice by address-cells <2>, but other way is ?

	port {
		cpu-ep0: endpoint@0 {
			remote-endpoint = <&codec-ep 0>;
		};
		cpu-ep1: endpoint@1 {
			remote-endpoint = <&codec-ep 1>;
		};
	};

	port {
		#address-cells = <2>;
		#size-cells = <1>;
		codec-ep: endpoint {
(*1)			remote-endpoint = <&cpu-ep0>;
		};
	};

This case, cpu-epX -> codec-ep are OK.
But, codec-ep -> cpu-epX case can point only 1 endpoint (*1).
I'm not sure, but maybe this is not a OF graph policy (?)

> > Can you agree this ? we need extra patch,
> > but it can solve your / my problem.
> 
> Yes it's starting to make sense :)

Thanks

> > Now I'm posting patches to merging
> > "audio-graph-card" and "DPCM ver audio-graph-card".
> > If you are OK, I will include above solution patch
> > to this patch-set.
> 
> Sure, maybe please first check if the #size-cells = <2>
> option would work though?

maybe #address-cells, instead of #size-cells ;P
But, yeah, please double check it too.

> > Current audio-graph doesn't expect your use-case,
> > and I want to avoid conflict.
> > 
> > So, "merged" audio-graph should solve your use-case.
> > If you can agree about this, I will post patch-set.
> 
> Yeah I agree, just still wondering what might be the best
> way to represent 1 DAI vs 2 DAIs.

According to OF graph maintainer, he said that
counting port / endpoint are not guaranteed,
and we need to use "reg".
(It is the reason of b6f3fc005a2c8b425d7a0973b43bef05890bf479)

If you could double checked, and we could agreed
that "reg" is the solution for us.
I will post patches.

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181213 00:24]:
> (snip)
> > foo = <&ep 0>;
> > bar = <&ep 1>;
> 
> Ahh, it looks nice idea !
> but hmm..., it seems dtc doesn't allow us to use #address-cells = <2>
> for "remote-endpoint".

OK. Yeah let's scrap that then, it does not suit for
endpoints it seems. Thanks for checking.

> According to OF graph maintainer, he said that
> counting port / endpoint are not guaranteed,
> and we need to use "reg".
> (It is the reason of b6f3fc005a2c8b425d7a0973b43bef05890bf479)
> 
> If you could double checked, and we could agreed
> that "reg" is the solution for us.
> I will post patches.

OK yes your "reg" proposal works for me.

Regards,

Tony

Re: [alsa-devel] [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Kuninori Morimoto]

Hi Tony

> > According to OF graph maintainer, he said that
> > counting port / endpoint are not guaranteed,
> > and we need to use "reg".
> > (It is the reason of b6f3fc005a2c8b425d7a0973b43bef05890bf479)
> > 
> > If you could double checked, and we could agreed
> > that "reg" is the solution for us.
> > I will post patches.
> 
> OK yes your "reg" proposal works for me.

OK, thanks.
I will post patch-set, soon.

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181213 01:06]:
> 
> Hi Tony
> 
> > > According to OF graph maintainer, he said that
> > > counting port / endpoint are not guaranteed,
> > > and we need to use "reg".
> > > (It is the reason of b6f3fc005a2c8b425d7a0973b43bef05890bf479)
> > > 
> > > If you could double checked, and we could agreed
> > > that "reg" is the solution for us.
> > > I will post patches.
> > 
> > OK yes your "reg" proposal works for me.
> 
> OK, thanks.
> I will post patch-set, soon.

OK sounds good to me.

Thanks,

Tony

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Peter Ujfalusi]

On 12/12/2018 2.19, Tony Lindgren wrote:
> * Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181211 23:16]:
>>
>> Hi Tony
>>
>>> The issue I have with that it does not then follow the binding doc :)
>>>
>>> See this part in Documentation/devicetree/bindings/graph.txt:
>>>
>>>  "If a single port is connected to more than one remote device, an
>>>  'endpoint' child node must be provided for each link."
>>>
>>> Isn't the I2C TDM case the same as "single port connecected to
>>> more than one remote device" rather than multiple ports?
>>>
>>> To me it seems we're currently only handling the multiple ports
>>> case, and not multiple endpoints for a port. Other than fixing
>>> that, things should work just as earlier with my two patches.
>>> That is unless I accidentally broke something.
>>>
>>> So just trying to correct the binding usage. Or am I missing
>>> something?
>>
>> I'm not 100% sure your "I2C TDM case", but you can check
>> multi-endpoint sample on "Example: Multi DAI with DPCM" below.
>> "pcm3168a" is using multi-endpoint.
>> Does this help you ?
>>
>> 	https://patchwork.kernel.org/patch/10712877/
> 
> Hmm, so do you have multiple separate ports at the "&sound" node
> hardware? If so then yeah multiple ports make sense.
> 
> But if you only a single physical (I2S?) port at the
> "&sound" node hardware, then IMO you should only have one
> port and multiple endpoints there according to the graph.txt
> binding doc.
> 
> In my McBSP case there is only a single physical I2S port
> that can be TDM split into timeslots.

So what is missing from the McBSP driver is to configure the TDM. We
never had a hardware which would require it so it is _not_ implemented.

imho the 'only' thing is to implement the set_tdm_slot callback for the
McBSP DAI. In DT you would have single card with two dai_link section
and each section would set different tdm slots to use for the codecs
listening on different slots.

There is one issue for sure with this setup: the two PCM can not be used
at the same time. But we have one DMA channel so if you would open both
the PCM stream need to be set up in a way to match with the HW or create
a asound.conf file to do some mapping.

> 
> Regards,
> 
> Tony
> 

- Péter

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

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [181212 13:03]:
> On 12/12/2018 2.19, Tony Lindgren wrote:
> > In my McBSP case there is only a single physical I2S port
> > that can be TDM split into timeslots.
> 
> So what is missing from the McBSP driver is to configure the TDM. We
> never had a hardware which would require it so it is _not_ implemented.

Curiously.. Nothing needs to be done in the McBSP driver for the droid
4 TDM configuration AFAIK.

The CPCAP PMIC is the clock master, and only the PMIC registers need to
be configured in this case for the timeslot to switch between codecs
connected to McBSP3.

> imho the 'only' thing is to implement the set_tdm_slot callback for the
> McBSP DAI. In DT you would have single card with two dai_link section
> and each section would set different tdm slots to use for the codecs
> listening on different slots.
> 
> There is one issue for sure with this setup: the two PCM can not be used
> at the same time. But we have one DMA channel so if you would open both
> the PCM stream need to be set up in a way to match with the HW or create
> a asound.conf file to do some mapping.

Yes in the droid 4 TDM case only one device can be used at a time
and all that configuration is done in the PMIC codec .set_tdm_slot
function.

I think it's possible to do more complex configurations where McBSP
is the master and would implement a .set_tdm_slot function. But I
don't know anything about that and I'm not aware of any such use
cases in the mainline kernel.

Regards,

Tony

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Peter Ujfalusi]

On 12/12/2018 16.50, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [181212 13:03]:
>> On 12/12/2018 2.19, Tony Lindgren wrote:
>>> In my McBSP case there is only a single physical I2S port
>>> that can be TDM split into timeslots.
>>
>> So what is missing from the McBSP driver is to configure the TDM. We
>> never had a hardware which would require it so it is _not_ implemented.
> 
> Curiously.. Nothing needs to be done in the McBSP driver for the droid
> 4 TDM configuration AFAIK.

So you always have 4 timeslot and that's it?

> The CPCAP PMIC is the clock master, and only the PMIC registers need to
> be configured in this case for the timeslot to switch between codecs
> connected to McBSP3.

The McBSP TDM configuration is not master only.
You basically tell McBSP on which timeslot to transmit/receive.
Let's say you have two codecs connected to a single McBSP.
codec1 is configured to listen for timeslot 0/1
codec2 is configured to listen for timeslot 2/3

If you open a stereo stream to codec1 then you tell McBSP to
send/receive the data under timeslot 0/1 and ignore any other timeslots.

If you open a stereo stream to codec2 then you tell McBSP to
send/receive the data under timeslot 2/3 and ignore any other timeslots.

For codec1 you don't really need anything regarding to TDM configuration
as McBSP will send/receive right after the start condition on the FS,
but for codec2 you need to configure the TDM mode of McBSP to ignore
timeslot 0/1

>> imho the 'only' thing is to implement the set_tdm_slot callback for the
>> McBSP DAI. In DT you would have single card with two dai_link section
>> and each section would set different tdm slots to use for the codecs
>> listening on different slots.
>>
>> There is one issue for sure with this setup: the two PCM can not be used
>> at the same time. But we have one DMA channel so if you would open both
>> the PCM stream need to be set up in a way to match with the HW or create
>> a asound.conf file to do some mapping.
> 
> Yes in the droid 4 TDM case only one device can be used at a time
> and all that configuration is done in the PMIC codec .set_tdm_slot
> function.

Hrm, do you have two DAIs on the PMIC side or different timeslots from
the TDM stream is routed to different outputs, similarly to twl6040
where timeslot 0/1 is Headset, timeslot 2/3 is Handsfree and timeslot 5
is to drive a vibra?

> I think it's possible to do more complex configurations where McBSP
> is the master and would implement a .set_tdm_slot function. But I
> don't know anything about that and I'm not aware of any such use
> cases in the mainline kernel.

No, the set_tdm_slot is applicable for both master and slave mode of McBSP.

> 
> Regards,
> 
> Tony
> 

- Péter

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

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [181213 06:52]:
> On 12/12/2018 16.50, Tony Lindgren wrote:
> > * Peter Ujfalusi <peter.ujfalusi@ti.com> [181212 13:03]:
> >> On 12/12/2018 2.19, Tony Lindgren wrote:
> >>> In my McBSP case there is only a single physical I2S port
> >>> that can be TDM split into timeslots.
> >>
> >> So what is missing from the McBSP driver is to configure the TDM. We
> >> never had a hardware which would require it so it is _not_ implemented.
> > 
> > Curiously.. Nothing needs to be done in the McBSP driver for the droid
> > 4 TDM configuration AFAIK.
> 
> So you always have 4 timeslot and that's it?

No.. "droid 4", not "4 timeslot" above :)

See the McBSP3 parts of the diagram at [0]. The PMIC has
register bits for timeslots 0 to 2:

$ git grep CPCAP | grep TIMESLOT

> > The CPCAP PMIC is the clock master, and only the PMIC registers need to
> > be configured in this case for the timeslot to switch between codecs
> > connected to McBSP3.
> 
> The McBSP TDM configuration is not master only.
> You basically tell McBSP on which timeslot to transmit/receive.
> Let's say you have two codecs connected to a single McBSP.
> codec1 is configured to listen for timeslot 0/1
> codec2 is configured to listen for timeslot 2/3

OK. Right now I'm only configuring things at the PMIC
end as the clocking is different compared to using CPCAP
PMIC voice codec and when routing data from the MDM voice
call codec to the CPCAP PMIC.

> If you open a stereo stream to codec1 then you tell McBSP to
> send/receive the data under timeslot 0/1 and ignore any other timeslots.
> 
> If you open a stereo stream to codec2 then you tell McBSP to
> send/receive the data under timeslot 2/3 and ignore any other timeslots.
> 
> For codec1 you don't really need anything regarding to TDM configuration
> as McBSP will send/receive right after the start condition on the FS,
> but for codec2 you need to configure the TDM mode of McBSP to ignore
> timeslot 0/1

OK. Sounds like what you're describing could be used to route the
MDM voice codec audio for capture to a file via McBSP etc.

> >> imho the 'only' thing is to implement the set_tdm_slot callback for the
> >> McBSP DAI. In DT you would have single card with two dai_link section
> >> and each section would set different tdm slots to use for the codecs
> >> listening on different slots.
> >>
> >> There is one issue for sure with this setup: the two PCM can not be used
> >> at the same time. But we have one DMA channel so if you would open both
> >> the PCM stream need to be set up in a way to match with the HW or create
> >> a asound.conf file to do some mapping.
> > 
> > Yes in the droid 4 TDM case only one device can be used at a time
> > and all that configuration is done in the PMIC codec .set_tdm_slot
> > function.
> 
> Hrm, do you have two DAIs on the PMIC side or different timeslots from
> the TDM stream is routed to different outputs, similarly to twl6040
> where timeslot 0/1 is Headset, timeslot 2/3 is Handsfree and timeslot 5
> is to drive a vibra?

Two DAIs on the PMIC, one at McBSP2 and one shared with MDM codec
at McBSP3.

> > I think it's possible to do more complex configurations where McBSP
> > is the master and would implement a .set_tdm_slot function. But I
> > don't know anything about that and I'm not aware of any such use
> > cases in the mainline kernel.
> 
> No, the set_tdm_slot is applicable for both master and slave mode of McBSP.

OK sure.

Regards,

Tony

[0] https://lkml.org/lkml/2018/3/28/405

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Peter Ujfalusi]

On 11/12/2018 7.35, Tony Lindgren wrote:
> * Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> [181211 05:16]:
>>
>> Hi Tony
>>
>>>> This looks a little bit strange for me.
>>>> Can you show me your DT for it ?
>>>
>>> Sure, adding also Sebastian to Cc. Here's what I currently have for droid 4
>>> dts with two codecs on I2S. Please just ignore the GNSS parts there..
>>>
>>> The TDM configuration is all done in the cpcap_audio_codec via set_tdm_slot().
>>> The modem voice call codec is a serdev driver :) I'll need some more time to
>>> be able to post patches but it's basically working for voice calls.
>>
>> Hmm... it seems strange...
>> I guess you are using "ti,omap4-mcbsp" driver (= linux/sound/soc/omap/omap-mcbsp.c)
>> Is this correct ?
> 
> Yes.
> 
>> If so, your driver is registering component as
>>
>> 	ret = devm_snd_soc_register_component(&pdev->dev,
>> 					      &omap_mcbsp_component,
>> 					      &omap_mcbsp_dai, 1);
>>
>> Your driver has only 1 DAI.
> 
> Yes I have a patch for omap-mcbsp.c too :)
> 
>>>  	mcbsp3_port: port {
>>> -		cpu_dai3: endpoint {
>>> +		cpu_dai3: endpoint@0 {
>>>  			dai-format = "dsp_a";
>>>  			frame-master = <&cpcap_audio_codec1>;
>>>  			bitclock-master = <&cpcap_audio_codec1>;
>>>  			remote-endpoint = <&cpcap_audio_codec1>;
>>>  		};
>>> +		cpu_dai_mdm: endpoint@1 {
>>> +			dai-format = "dsp_a";
>>> +			frame-master = <&cpcap_audio_codec1>;
>>> +			bitclock-master = <&cpcap_audio_codec1>;
>>> +			remote-endpoint = <&mot_mdm6600_audio_codec0>;
>>> +		};
>>
>> And here, you have 1 port, 2 endpoint.
>> Then, asoc_simple_card_get_dai_id() should return 1.
>>
>> And, your [2/2] patch,
>> I guess you are misunderstanding about "port" vs "endpoint",
>> or omap-mcbsp driver side need to update ?
> 
> Yes omap-mcbsp driver needs to be updated for multiple endpoints.
> 
> Adding Jarkko and Peter also to Cc, below is the WIP patch that I'm
> currently using for omap-mcbsp to add more DAIs.

Hrm.
McBSP have single DX and single DR pin. Iow each McBSP have single DAI.

> 
> So far nothing else to do in the omap-mcbsp as it's the cpcap hardware
> that configures the TDM timeslots. And I'm currently assuming the
> first instance is the master, I guess that should be parsed from the
> the frame-master dts property instead.
> 
> Regards,
> 
> Tony
> 
> 8< ----------------------
> diff --git a/sound/soc/omap/omap-mcbsp-priv.h b/sound/soc/omap/omap-mcbsp-priv.h
> --- a/sound/soc/omap/omap-mcbsp-priv.h
> +++ b/sound/soc/omap/omap-mcbsp-priv.h
> @@ -262,6 +262,8 @@ struct omap_mcbsp {
>  	struct omap_mcbsp_platform_data *pdata;
>  	struct omap_mcbsp_st_data *st_data;
>  	struct omap_mcbsp_reg_cfg cfg_regs;
> +	struct snd_soc_dai_driver *dais;
> +	int dai_count;
>  	struct snd_dmaengine_dai_dma_data dma_data[2];
>  	unsigned int dma_req[2];
>  	int dma_op_mode;
> diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
> --- a/sound/soc/omap/omap-mcbsp.c
> +++ b/sound/soc/omap/omap-mcbsp.c
> @@ -28,6 +28,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/of_graph.h>
>  #include <sound/core.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
> @@ -1318,23 +1319,53 @@ static int omap_mcbsp_remove(struct snd_soc_dai *dai)
>  	return 0;
>  }
>  
> -static struct snd_soc_dai_driver omap_mcbsp_dai = {
> -	.probe = omap_mcbsp_probe,
> -	.remove = omap_mcbsp_remove,
> -	.playback = {
> -		.channels_min = 1,
> -		.channels_max = 16,
> -		.rates = OMAP_MCBSP_RATES,
> -		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
> -	},
> -	.capture = {
> -		.channels_min = 1,
> -		.channels_max = 16,
> -		.rates = OMAP_MCBSP_RATES,
> -		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE,
> -	},
> -	.ops = &mcbsp_dai_ops,
> -};
> +static int omap_mcbsp_init_dais(struct omap_mcbsp *mcbsp)
> +{
> +	struct device_node *np = mcbsp->dev->of_node;
> +	int i;
> +
> +	if (np)
> +		mcbsp->dai_count = of_graph_get_endpoint_count(np);
> +
> +	if (!mcbsp->dai_count)
> +		mcbsp->dai_count = 1;
> +
> +	mcbsp->dais = devm_kcalloc(mcbsp->dev, mcbsp->dai_count,
> +				   sizeof(*mcbsp->dais), GFP_KERNEL);
> +	if (!mcbsp->dais)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < mcbsp->dai_count; i++) {
> +		struct snd_soc_dai_driver *dai = &mcbsp->dais[i];
> +
> +		dai->name = devm_kasprintf(mcbsp->dev, GFP_KERNEL, "%s-dai%i",
> +					   dev_name(mcbsp->dev), i);
> +
> +		if (i == 0) {
> +			dai->probe = omap_mcbsp_probe;
> +			dai->remove = omap_mcbsp_remove;
> +			dai->ops = &mcbsp_dai_ops;
> +		}
> +		dai->playback.channels_min = 1;
> +		dai->playback.channels_max = 16;
> +		dai->playback.rates = OMAP_MCBSP_RATES;
> +		if (mcbsp->pdata->reg_size == 2)
> +			dai->playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
> +		else
> +			dai->playback.formats = SNDRV_PCM_FMTBIT_S16_LE |
> +						SNDRV_PCM_FMTBIT_S32_LE;
> +		dai->capture.channels_min = 1;
> +		dai->capture.channels_max = 16;
> +		dai->capture.rates = OMAP_MCBSP_RATES;
> +		if (mcbsp->pdata->reg_size == 2)
> +			dai->capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
> +		else
> +			dai->capture.formats = SNDRV_PCM_FMTBIT_S16_LE |
> +					       SNDRV_PCM_FMTBIT_S32_LE;

I prefer to have the 'if (mcbsp->pdata->reg_size == 2)' grouped for
playback and capture in one place.

> +	}
> +
> +	return 0;
> +}
>  
>  static const struct snd_soc_component_driver omap_mcbsp_component = {
>  	.name		= "omap-mcbsp",
> @@ -1423,18 +1454,17 @@ static int asoc_mcbsp_probe(struct platform_device *pdev)
>  	mcbsp->dev = &pdev->dev;
>  	platform_set_drvdata(pdev, mcbsp);
>  
> -	ret = omap_mcbsp_init(pdev);
> +	ret = omap_mcbsp_init_dais(mcbsp);
>  	if (ret)
>  		return ret;
>  
> -	if (mcbsp->pdata->reg_size == 2) {
> -		omap_mcbsp_dai.playback.formats = SNDRV_PCM_FMTBIT_S16_LE;
> -		omap_mcbsp_dai.capture.formats = SNDRV_PCM_FMTBIT_S16_LE;
> -	}
> +	ret = omap_mcbsp_init(pdev);
> +	if (ret)
> +		return ret;
>  
>  	ret = devm_snd_soc_register_component(&pdev->dev,
>  					      &omap_mcbsp_component,
> -					      &omap_mcbsp_dai, 1);
> +					      mcbsp->dais, mcbsp->dai_count);
>  	if (ret)
>  		return ret;
>  
> 

- Péter

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

Re: [PATCH 0/2] Graph fixes for using multiple endpoints per port

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [181212 12:47]:
> McBSP have single DX and single DR pin. Iow each McBSP have single DAI.

Yeah it's single physical port for sure. But at least currently we need
additional secondary DAIs for each TDM channel in addition to the primary
DAI managing the McBSP registers.

Currently there's nothing to do for the secondary DAIs, but if McBSP was
also the clock master then the secondary DAI functions would need to take
care of the multiplexing McBSP hardware resources. But that can be done
later if we actually ever see some McBSP being TDM master use case..

Regards,

Tony