[alsa-devel] [PATCH 0/2] Graph fixes for using multiple endpoints per port
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(-)
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,
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; }
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
* 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>; + }; }; };
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
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
* 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
* 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;
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
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
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
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
* 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, };
* 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
* 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 ?
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
Hi Tony
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
Hi Tony, again
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
* Kuninori Morimoto kuninori.morimoto.gx@renesas.com [181212 06:52]:
Hi Tony, again
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
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
* 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
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
* 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
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 ?
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
* 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
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
* 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
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;elsedai->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;elsedai->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);
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
* 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
participants (3)
-
Kuninori Morimoto -
Peter Ujfalusi -
Tony Lindgren