[alsa-devel] [PATCH 0/6] ASoC: davinci-mcbsp: add binding for McBSP
This series of patches adds devicetree support for the davinci McBSP audio interface.
Add devicetree binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx MultiChannel Buffered Serial Port (McBSP)
The optional register range "dat" is not implemented at the moment. The current driver supports only DMA into RX/TX registers but no FIFO. Once the FIFO is implemented in the driver the "dat" range will be used.
Signed-off-by: Petr Kulhavy petr@barix.com --- .../bindings/sound/davinci-mcbsp-audio.txt | 57 ++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt new file mode 100644 index 000000000000..f60fceb927dd --- /dev/null +++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt @@ -0,0 +1,57 @@ +Texas Instruments DaVinci McBSP module +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This binding describes the "Multi-channel Buffered Serial Port" (McBSP) +audio interface found in some TI DaVinci processors like e.g. the DA850, +DM6446, DA355. + + +Required properties: +~~~~~~~~~~~~~~~~~~~~ +- compatible : "ti,da850-mcbsp-audio" + +- reg : physical base address and length of the controller memory mapped + region(s). +- reg-names : Should contain: + * "mpu" for the main registers (required). For compatibility with + existing software, it is recommended this is the first entry. + * "dat" for the data FIFO (optional). + +- dmas: two element list of DMA controller phandles and DMA request line + ordered pairs. +- dma-names: identifier string for each DMA request line in the dmas property. + These strings correspond 1:1 with the ordered pairs in dmas. The dma + identifiers must be "rx" and "tx". + +Optional properties: +~~~~~~~~~~~~~~~~~~~~ +- interrupts : Interrupt numbers for McBSP +- interrupt-names : Known interrupt names are "rx" and "tx" + +- pinctrl-0: Should specify pin control group used for this controller. +- pinctrl-names: Should contain only one value - "default", for more details + please refer to pinctrl-bindings.txt + +- channel-combine : boolean. If present L and R channels are combined into one + DMA transfer, however the labelling of the channels is swapped. + Therefore this option should be used only if the channels can + be swapped back at the codec side again. + +Example (AM1808): +~~~~~~~~~~~~~~~~~ + +mcbsp0: mcbsp@1d10000 { + compatible = "ti,davinci-mcbsp-audio"; + pinctrl-names = "default"; + pinctrl-0 = <&mcbsp0_pins>; + + reg = <0x00110000 0x1000>, + <0x00310000 0x1000>; + reg-names = "mpu", "dat"; + interrupts = <97 98>; + interrupts-names = "rx", "tx"; + dmas = <&edma0 3 + &edma0 2>; + dma-names = "tx", "rx"; + status = "okay"; +};
On 04/06/16 16:21, Petr Kulhavy wrote:
Add devicetree binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx MultiChannel Buffered Serial Port (McBSP)
The optional register range "dat" is not implemented at the moment. The current driver supports only DMA into RX/TX registers but no FIFO. Once the FIFO is implemented in the driver the "dat" range will be used.
Signed-off-by: Petr Kulhavy petr@barix.com
.../bindings/sound/davinci-mcbsp-audio.txt | 57 ++++++++++++++++++++++
I would drop the -audio postfix. I know the McASP introduced this and it is annoying. Let's not repeat it again...
1 file changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt new file mode 100644 index 000000000000..f60fceb927dd --- /dev/null +++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt @@ -0,0 +1,57 @@ +Texas Instruments DaVinci McBSP module +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+This binding describes the "Multi-channel Buffered Serial Port" (McBSP) +audio interface found in some TI DaVinci processors like e.g. the DA850, +DM6446, DA355.
Given that the driver is actually a driver for daVinci ASP and it completely ignores registers introduced when the IP is renamed from ASP to McBSP, should we say something about this? That the ASP is compatible or subset of McBSP (w/o the multichannel support) and these bindings could be used for ASP, with adding new compatible? Or just leave that out and bother with it when we have such a device booting with DT?
+Required properties: +~~~~~~~~~~~~~~~~~~~~ +- compatible : "ti,da850-mcbsp-audio"
+- reg : physical base address and length of the controller memory mapped
region(s).+- reg-names : Should contain:
* "mpu" for the main registers (required). For compatibility withexisting software, it is recommended this is the first entry.* "dat" for the data FIFO (optional).+- dmas: two element list of DMA controller phandles and DMA request line
ordered pairs.+- dma-names: identifier string for each DMA request line in the dmas property.
These strings correspond 1:1 with the ordered pairs in dmas. The dmaidentifiers must be "rx" and "tx".+Optional properties: +~~~~~~~~~~~~~~~~~~~~ +- interrupts : Interrupt numbers for McBSP +- interrupt-names : Known interrupt names are "rx" and "tx"
+- pinctrl-0: Should specify pin control group used for this controller. +- pinctrl-names: Should contain only one value - "default", for more details
please refer to pinctrl-bindings.txt+- channel-combine : boolean. If present L and R channels are combined into one
DMA transfer, however the labelling of the channels is swapped.Therefore this option should be used only if the channels canbe swapped back at the codec side again.+Example (AM1808): +~~~~~~~~~~~~~~~~~
+mcbsp0: mcbsp@1d10000 {
- compatible = "ti,davinci-mcbsp-audio";
- pinctrl-names = "default";
- pinctrl-0 = <&mcbsp0_pins>;
- reg = <0x00110000 0x1000>,
<0x00310000 0x1000>;- reg-names = "mpu", "dat";
- interrupts = <97 98>;
- interrupts-names = "rx", "tx";
- dmas = <&edma0 3
&edma0 2>;- dma-names = "tx", "rx";
- status = "okay";
+};
On 07.04.2016 15:33, Peter Ujfalusi wrote:
On 04/06/16 16:21, Petr Kulhavy wrote:
Add devicetree binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx MultiChannel Buffered Serial Port (McBSP)
The optional register range "dat" is not implemented at the moment. The current driver supports only DMA into RX/TX registers but no FIFO. Once the FIFO is implemented in the driver the "dat" range will be used.
Signed-off-by: Petr Kulhavy petr@barix.com
.../bindings/sound/davinci-mcbsp-audio.txt | 57 ++++++++++++++++++++++
I would drop the -audio postfix. I know the McASP introduced this and it is annoying. Let's not repeat it again...
I'm going to correct it, thanks.
1 file changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt new file mode 100644 index 000000000000..f60fceb927dd --- /dev/null +++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt @@ -0,0 +1,57 @@ +Texas Instruments DaVinci McBSP module +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+This binding describes the "Multi-channel Buffered Serial Port" (McBSP) +audio interface found in some TI DaVinci processors like e.g. the DA850, +DM6446, DA355.
Given that the driver is actually a driver for daVinci ASP and it completely ignores registers introduced when the IP is renamed from ASP to McBSP, should we say something about this? That the ASP is compatible or subset of McBSP (w/o the multichannel support) and these bindings could be used for ASP, with adding new compatible? Or just leave that out and bother with it when we have such a device booting with DT?
It would be nice to mention both but if there is only one compatible string then it doesn't make much sense. And saying "you can add your own compatible string" is the binding not the right place to say this. But could we add another compatible string for the older ASP?
Petr
On 07.04.2016 15:33, Peter Ujfalusi wrote:
Given that the driver is actually a driver for daVinci ASP and it completely ignores registers introduced when the IP is renamed from ASP to McBSP, should we say something about this? That the ASP is compatible or subset of McBSP (w/o the multichannel support) and these bindings could be used for ASP, with adding new compatible? Or just leave that out and bother with it when we have such a device booting with DT?
Hi Peter,
what do you think about the following formulation?
-- This binding describes the "Multi-channel Buffered Serial Port" (McBSP) audio interface found in some TI DaVinci processors like the OMAP-L138 or AM180x.
Some older DaVinci processors like the DM6446 or DA355 use a simpler version of the digital audio interface, called ASP. This driver can support them as well if the appropriate compatible string is added. --
Thanks Petr
On 04/08/16 12:24, Petr Kulhavy wrote:
On 07.04.2016 15:33, Peter Ujfalusi wrote:
Given that the driver is actually a driver for daVinci ASP and it completely ignores registers introduced when the IP is renamed from ASP to McBSP, should we say something about this? That the ASP is compatible or subset of McBSP (w/o the multichannel support) and these bindings could be used for ASP, with adding new compatible? Or just leave that out and bother with it when we have such a device booting with DT?
Hi Peter,
what do you think about the following formulation?
-- This binding describes the "Multi-channel Buffered Serial Port" (McBSP) audio interface found in some TI DaVinci processors like the OMAP-L138 or AM180x.
Some older DaVinci processors like the DM6446 or DA355 use a simpler version of the digital audio interface, called ASP. This driver can support them as well if the appropriate compatible string is added.
Hrm, I don't think we should mention the ASP, it looks out of context. The ASP will have compatible of ti,dm644x-asp or something similar. We should worry about this when the time comes.
On 11.04.2016 10:11, Peter Ujfalusi wrote:
On 04/08/16 12:24, Petr Kulhavy wrote:
Hi Peter,
what do you think about the following formulation?
-- This binding describes the "Multi-channel Buffered Serial Port" (McBSP) audio interface found in some TI DaVinci processors like the OMAP-L138 or AM180x.
Some older DaVinci processors like the DM6446 or DA355 use a simpler version of the digital audio interface, called ASP. This driver can support them as well if the appropriate compatible string is added.
Hrm, I don't think we should mention the ASP, it looks out of context. The ASP will have compatible of ti,dm644x-asp or something similar. We should worry about this when the time comes.
OK, I will keep just the first paragraph.
Thanks! Petr
This adds DT support for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx McBSP driver.
Signed-off-by: Petr Kulhavy petr@barix.com --- sound/soc/davinci/Kconfig | 6 ++- sound/soc/davinci/davinci-i2s.c | 106 ++++++++++++++++++++++++++++++++++------ 2 files changed, 96 insertions(+), 16 deletions(-)
diff --git a/sound/soc/davinci/Kconfig b/sound/soc/davinci/Kconfig index 50ca291cc225..6b732d8e5896 100644 --- a/sound/soc/davinci/Kconfig +++ b/sound/soc/davinci/Kconfig @@ -16,7 +16,11 @@ config SND_EDMA_SOC - DRA7xx family
config SND_DAVINCI_SOC_I2S - tristate + tristate "DaVinci Multichannel Buffered Serial Port (McBSP) support" + depends on SND_EDMA_SOC + help + Say Y or M here if you want to have support for McBSP IP found in + Texas Instruments DaVinci DA850 SoCs.
config SND_DAVINCI_SOC_MCASP tristate "Multichannel Audio Serial Port (McASP) support" diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c index ec98548a5fc9..2ebfe86dd584 100644 --- a/sound/soc/davinci/davinci-i2s.c +++ b/sound/soc/davinci/davinci-i2s.c @@ -4,9 +4,15 @@ * Author: Vladimir Barinov, vbarinov@embeddedalley.com * Copyright: (C) 2007 MontaVista Software, Inc., source@mvista.com * + * DT support (c) 2016 Petr Kulhavy, Barix AG petr@barix.com + * based on davinci-mcasp.c DT support + * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. + * + * TODO: + * on DA850 implement HW FIFOs instead of DMA into DXR and DRR registers */
#include <linux/init.h> @@ -648,15 +654,82 @@ static const struct snd_soc_component_driver davinci_i2s_component = { .name = "davinci-i2s", };
+static struct snd_platform_data* +davinci_i2s_set_pdata_from_of(struct platform_device *pdev) +{ + struct snd_platform_data *pdata = NULL; + struct device_node *np; + struct of_phandle_args dma_spec; + int ret; + + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node) + return dev_get_platdata(&pdev->dev); + + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return ERR_PTR(-ENOMEM); + + np = pdev->dev.of_node; + + ret = of_property_match_string(np, "dma-names", "tx"); + if (ret >= 0) { + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, + &dma_spec); + if (ret >= 0) + pdata->tx_dma_channel = dma_spec.args[0]; + } + + ret = of_property_match_string(np, "dma-names", "rx"); + if (ret >= 0) { + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, + &dma_spec); + if (ret >= 0) + pdata->rx_dma_channel = dma_spec.args[0]; + } + + /* optional parameters */ + + pdata->enable_channel_combine = + of_property_read_bool(np, "channel-combine"); + + /* + * pdata->clk_input_pin is deliberately not exported to DT + * and the default value of the clk_input_pin is MCBSP_CLKR. + * The value MCBSP_CLKS makes no sense as it turns the CPU + * to a bit clock master in the SND_SOC_DAIFMT_CBM_CFS mode + * where it should be bit clock slave! + */ + + return pdata; +} + static int davinci_i2s_probe(struct platform_device *pdev) { + struct snd_platform_data *pdata; struct davinci_mcbsp_dev *dev; struct resource *mem, *res; void __iomem *io_base; int *dma; int ret;
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + pdata = davinci_i2s_set_pdata_from_of(pdev); + if (IS_ERR(pdata)) { + dev_err(&pdev->dev, "Error populating platform data, err %ld\n", + PTR_ERR(pdata)); + return PTR_ERR(pdata); + } + + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu"); + if (!mem) { + dev_warn(&pdev->dev, + ""mpu" mem resource not found, using index 0\n"); + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(&pdev->dev, "no mem resource?\n"); + return -ENODEV; + } + } + io_base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(io_base)) return PTR_ERR(io_base); @@ -680,25 +753,21 @@ static int davinci_i2s_probe(struct platform_device *pdev) (dma_addr_t)(mem->start + DAVINCI_MCBSP_DRR_REG);
/* first TX, then RX */ - res = platform_get_resource(pdev, IORESOURCE_DMA, 0); - if (!res) { - dev_err(&pdev->dev, "no DMA resource\n"); - ret = -ENXIO; - goto err_release_clk; - } dma = &dev->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; - *dma = res->start; dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = dma; + res = platform_get_resource(pdev, IORESOURCE_DMA, 0); + if (res) + *dma = res->start; + else + *dma = pdata->tx_dma_channel;
- res = platform_get_resource(pdev, IORESOURCE_DMA, 1); - if (!res) { - dev_err(&pdev->dev, "no DMA resource\n"); - ret = -ENXIO; - goto err_release_clk; - } dma = &dev->dma_request[SNDRV_PCM_STREAM_CAPTURE]; - *dma = res->start; dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].filter_data = dma; + res = platform_get_resource(pdev, IORESOURCE_DMA, 1); + if (res) + *dma = res->start; + else + *dma = pdata->rx_dma_channel;
dev->dev = &pdev->dev; dev_set_drvdata(&pdev->dev, dev); @@ -737,11 +806,18 @@ static int davinci_i2s_remove(struct platform_device *pdev) return 0; }
+static const struct of_device_id davinci_mcbsp_match[] = { + { .compatible = "ti,da850-mcbsp-audio" }, + {}, +}; +MODULE_DEVICE_TABLE(of, davinci_mcbsp_match); + static struct platform_driver davinci_mcbsp_driver = { .probe = davinci_i2s_probe, .remove = davinci_i2s_remove, .driver = { .name = "davinci-mcbsp", + .of_match_table = of_match_ptr(davinci_mcbsp_match), }, };
On 04/06/16 16:21, Petr Kulhavy wrote:
This adds DT support for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx McBSP driver.
Signed-off-by: Petr Kulhavy petr@barix.com
sound/soc/davinci/Kconfig | 6 ++- sound/soc/davinci/davinci-i2s.c | 106 ++++++++++++++++++++++++++++++++++------ 2 files changed, 96 insertions(+), 16 deletions(-)
diff --git a/sound/soc/davinci/Kconfig b/sound/soc/davinci/Kconfig index 50ca291cc225..6b732d8e5896 100644 --- a/sound/soc/davinci/Kconfig +++ b/sound/soc/davinci/Kconfig @@ -16,7 +16,11 @@ config SND_EDMA_SOC - DRA7xx family
config SND_DAVINCI_SOC_I2S
- tristate
- tristate "DaVinci Multichannel Buffered Serial Port (McBSP) support"
- depends on SND_EDMA_SOC
- help
Say Y or M here if you want to have support for McBSP IP found inTexas Instruments DaVinci DA850 SoCs.config SND_DAVINCI_SOC_MCASP tristate "Multichannel Audio Serial Port (McASP) support" diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c index ec98548a5fc9..2ebfe86dd584 100644 --- a/sound/soc/davinci/davinci-i2s.c +++ b/sound/soc/davinci/davinci-i2s.c @@ -4,9 +4,15 @@
- Author: Vladimir Barinov, vbarinov@embeddedalley.com
- Copyright: (C) 2007 MontaVista Software, Inc., source@mvista.com
- DT support (c) 2016 Petr Kulhavy, Barix AG petr@barix.com
based on davinci-mcasp.c DT support
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- TODO:
- on DA850 implement HW FIFOs instead of DMA into DXR and DRR registers
The BFIFO looks identical to AFIFO for McASP...
*/
#include <linux/init.h> @@ -648,15 +654,82 @@ static const struct snd_soc_component_driver davinci_i2s_component = { .name = "davinci-i2s", };
+static struct snd_platform_data* +davinci_i2s_set_pdata_from_of(struct platform_device *pdev) +{
- struct snd_platform_data *pdata = NULL;
- struct device_node *np;
- struct of_phandle_args dma_spec;
- int ret;
- if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
return dev_get_platdata(&pdev->dev);- pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
- if (!pdata)
return ERR_PTR(-ENOMEM);- np = pdev->dev.of_node;
- ret = of_property_match_string(np, "dma-names", "tx");
- if (ret >= 0) {
ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret,&dma_spec);if (ret >= 0)pdata->tx_dma_channel = dma_spec.args[0];- }
- ret = of_property_match_string(np, "dma-names", "rx");
- if (ret >= 0) {
ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret,&dma_spec);if (ret >= 0)pdata->rx_dma_channel = dma_spec.args[0];- }
Why would you do this? If we booted with DT the only thing needed by the edma-pcm (dmaengine) is the name of the DMA channel...
- /* optional parameters */
- pdata->enable_channel_combine =
of_property_read_bool(np, "channel-combine");
This can only be done when the McBSP is used in DSP_x mode since this will make the McBSP to transmit the stereo audio as mono on the bus.
After a quick look at the relevant parts in the driver, I think most of the things are pretty much broken or at least they are not totally correct. McBSP do support real I2S (SRGR:FWID), and the phase setup is wrong in the code. IMO. I don't have HW to test the daVinci-MCBSP or the ASP so I can not comment on how well things are working.
- /*
* pdata->clk_input_pin is deliberately not exported to DT* and the default value of the clk_input_pin is MCBSP_CLKR.* The value MCBSP_CLKS makes no sense as it turns the CPU* to a bit clock master in the SND_SOC_DAIFMT_CBM_CFS mode* where it should be bit clock slave!*/
Interesting. As a whole the clock selection part is mostly broken in the driver... It would be better to add davinci_i2s_set_sysclk() to deal with the system clock configuration.
- return pdata;
+}
static int davinci_i2s_probe(struct platform_device *pdev) {
- struct snd_platform_data *pdata; struct davinci_mcbsp_dev *dev; struct resource *mem, *res; void __iomem *io_base; int *dma; int ret;
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- pdata = davinci_i2s_set_pdata_from_of(pdev);
- if (IS_ERR(pdata)) {
dev_err(&pdev->dev, "Error populating platform data, err %ld\n",PTR_ERR(pdata));return PTR_ERR(pdata);- }
- mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu");
- if (!mem) {
dev_warn(&pdev->dev,"\"mpu\" mem resource not found, using index 0\n");mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);if (!mem) {dev_err(&pdev->dev, "no mem resource?\n");return -ENODEV;}- }
- io_base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(io_base)) return PTR_ERR(io_base);
@@ -680,25 +753,21 @@ static int davinci_i2s_probe(struct platform_device *pdev) (dma_addr_t)(mem->start + DAVINCI_MCBSP_DRR_REG);
/* first TX, then RX */
- res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
- if (!res) {
dev_err(&pdev->dev, "no DMA resource\n");ret = -ENXIO;goto err_release_clk;- } dma = &dev->dma_request[SNDRV_PCM_STREAM_PLAYBACK];
- *dma = res->start; dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = dma;
- res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
- if (res)
*dma = res->start;- else
*dma = pdata->tx_dma_channel;
Please follow the way davinci-mcasp does it right now: dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; ... dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; res = platform_get_resource(pdev, IORESOURCE_DMA, 0); if (res) *dma = res->start; else *dma = pdata->tx_dma_channel;
/* dmaengine filter data for DT and non-DT boot */ if (pdev->dev.of_node) dma_data->filter_data = "tx"; else dma_data->filter_data = dma;
while we are here, I think the tx/rx_dma_channel is something we should just get rid of. It is not used as far as I can tell. Something like this would do I think: dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; ... res = platform_get_resource(pdev, IORESOURCE_DMA, 0); if (res) { dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; *dma = res->start; dma_data->filter_data = dma; } else if (pdev->dev.of_node) { dma_data->filter_data = "tx"; } else { dev_err(&pdev->dev, "Missing DMA tx resource\n"); return -ENODEV; }
- res = platform_get_resource(pdev, IORESOURCE_DMA, 1);
- if (!res) {
dev_err(&pdev->dev, "no DMA resource\n");ret = -ENXIO;goto err_release_clk;- } dma = &dev->dma_request[SNDRV_PCM_STREAM_CAPTURE];
- *dma = res->start; dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].filter_data = dma;
- res = platform_get_resource(pdev, IORESOURCE_DMA, 1);
- if (res)
*dma = res->start;- else
*dma = pdata->rx_dma_channel;
same comment as for the TX dma part.
dev->dev = &pdev->dev; dev_set_drvdata(&pdev->dev, dev); @@ -737,11 +806,18 @@ static int davinci_i2s_remove(struct platform_device *pdev) return 0; }
+static const struct of_device_id davinci_mcbsp_match[] = {
- { .compatible = "ti,da850-mcbsp-audio" },
I would drop the "-audio" for the binding...
- {},
+}; +MODULE_DEVICE_TABLE(of, davinci_mcbsp_match);
static struct platform_driver davinci_mcbsp_driver = { .probe = davinci_i2s_probe, .remove = davinci_i2s_remove, .driver = { .name = "davinci-mcbsp",
.of_match_table = of_match_ptr(davinci_mcbsp_match),
The driver calls itself davinci-i2s.c and the prefixes internally used are davinci_i2s_*. The driver name is "davinci-mcbsp". And it is basically a driver for daVinci ASP (the McBSP additions are not configured at all).
I still think we should keep the davinci_i2s_* when adding new code, or rename the whole driver and it's prefixes to something more sensible?
}, };
On 07.04.2016 14:42, Peter Ujfalusi wrote:
On 04/06/16 16:21, Petr Kulhavy wrote:
*/
#include <linux/init.h> @@ -648,15 +654,82 @@ static const struct snd_soc_component_driver davinci_i2s_component = { .name = "davinci-i2s", };
+static struct snd_platform_data* +davinci_i2s_set_pdata_from_of(struct platform_device *pdev) +{
- struct snd_platform_data *pdata = NULL;
- struct device_node *np;
- struct of_phandle_args dma_spec;
- int ret;
- if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
return dev_get_platdata(&pdev->dev);- pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
- if (!pdata)
return ERR_PTR(-ENOMEM);- np = pdev->dev.of_node;
- ret = of_property_match_string(np, "dma-names", "tx");
- if (ret >= 0) {
ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret,&dma_spec);if (ret >= 0)pdata->tx_dma_channel = dma_spec.args[0];- }
- ret = of_property_match_string(np, "dma-names", "rx");
- if (ret >= 0) {
ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret,&dma_spec);if (ret >= 0)pdata->rx_dma_channel = dma_spec.args[0];- }
Why would you do this? If we booted with DT the only thing needed by the edma-pcm (dmaengine) is the name of the DMA channel...
I don't like this code either. But I have tried just to set the dma_filter to "rx" or "tx" and it didn't work. Perhaps because the edma pcm filter function filters by channel number and not by name? Or do you see an easier way of getting the channel number from the name?
- /* optional parameters */
- pdata->enable_channel_combine =
of_property_read_bool(np, "channel-combine");This can only be done when the McBSP is used in DSP_x mode since this will make the McBSP to transmit the stereo audio as mono on the bus.
I have actually never used this option and don't see a benefit of it. Is it just some workaround that should not be included in the binding?
After a quick look at the relevant parts in the driver, I think most of the things are pretty much broken or at least they are not totally correct. McBSP do support real I2S (SRGR:FWID), and the phase setup is wrong in the code. IMO. I don't have HW to test the daVinci-MCBSP or the ASP so I can not comment on how well things are working.
The I2S worked for me if the frame size equalled the PCM data size. E.g. 16-bit audio, 2 channels, 16 bit frame size -> 32 clock ticks per sample in total. However with bigger frame sizes it didn't work because it cannot insert "blanks" between the channels. E.g. with 32-bit wide slots and 16-bit audio it sends both left and right data in the first 32 clock ticks (i.e. still the first channel) and then the during the other channel the data line goes to high impedance which results in effectively playing only the left channel (and noise in the other channel).
That's probably what the comment about broken I2S is trying to say. I couldn't find any help in the datasheet either.
On some codecs the frame size is configurable and there it works, but codecs that require 32-bit frames don't work with 16-bit audio or need to use DSP_x format.
- return pdata;
+}
- static int davinci_i2s_probe(struct platform_device *pdev) {
- struct snd_platform_data *pdata; struct davinci_mcbsp_dev *dev; struct resource *mem, *res; void __iomem *io_base; int *dma; int ret;
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- pdata = davinci_i2s_set_pdata_from_of(pdev);
- if (IS_ERR(pdata)) {
dev_err(&pdev->dev, "Error populating platform data, err %ld\n",PTR_ERR(pdata));return PTR_ERR(pdata);- }
- mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu");
- if (!mem) {
dev_warn(&pdev->dev,"\"mpu\" mem resource not found, using index 0\n");mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);if (!mem) {dev_err(&pdev->dev, "no mem resource?\n");return -ENODEV;}- }
- io_base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(io_base)) return PTR_ERR(io_base);
@@ -680,25 +753,21 @@ static int davinci_i2s_probe(struct platform_device *pdev) (dma_addr_t)(mem->start + DAVINCI_MCBSP_DRR_REG);
/* first TX, then RX */
- res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
- if (!res) {
dev_err(&pdev->dev, "no DMA resource\n");ret = -ENXIO;goto err_release_clk;- } dma = &dev->dma_request[SNDRV_PCM_STREAM_PLAYBACK];
- *dma = res->start; dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = dma;
- res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
- if (res)
*dma = res->start;- else
*dma = pdata->tx_dma_channel;Please follow the way davinci-mcasp does it right now: dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; ... dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; res = platform_get_resource(pdev, IORESOURCE_DMA, 0); if (res) *dma = res->start; else *dma = pdata->tx_dma_channel;
/* dmaengine filter data for DT and non-DT boot */ if (pdev->dev.of_node) dma_data->filter_data = "tx"; else dma_data->filter_data = dma;
while we are here, I think the tx/rx_dma_channel is something we should just get rid of. It is not used as far as I can tell. Something like this would do I think: dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; ... res = platform_get_resource(pdev, IORESOURCE_DMA, 0); if (res) { dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; *dma = res->start; dma_data->filter_data = dma; } else if (pdev->dev.of_node) { dma_data->filter_data = "tx"; } else { dev_err(&pdev->dev, "Missing DMA tx resource\n"); return -ENODEV; }
Is the edma pcm filter function able to filter in once case by the dma number and in the other case by the dma name? See my above comment as well...
dev->dev = &pdev->dev; dev_set_drvdata(&pdev->dev, dev); @@ -737,11 +806,18 @@ static int davinci_i2s_remove(struct platform_device *pdev) return 0; }
+static const struct of_device_id davinci_mcbsp_match[] = {
- { .compatible = "ti,da850-mcbsp-audio" },
I would drop the "-audio" for the binding...
I was trying to stay consistent with the mcasp. But I don't mind keeping the name shorter :-)
- {},
+}; +MODULE_DEVICE_TABLE(of, davinci_mcbsp_match);
- static struct platform_driver davinci_mcbsp_driver = { .probe = davinci_i2s_probe, .remove = davinci_i2s_remove, .driver = { .name = "davinci-mcbsp",
.of_match_table = of_match_ptr(davinci_mcbsp_match),The driver calls itself davinci-i2s.c and the prefixes internally used are davinci_i2s_*. The driver name is "davinci-mcbsp". And it is basically a driver for daVinci ASP (the McBSP additions are not configured at all). I still think we should keep the davinci_i2s_* when adding new code, or rename the whole driver and it's prefixes to something more sensible?
Good point, thanks! I will rename the table to davinci_i2s_match to stay consistent.
Petr
On 04/07/16 16:32, Petr Kulhavy wrote:
On 07.04.2016 14:42, Peter Ujfalusi wrote:
On 04/06/16 16:21, Petr Kulhavy wrote:
*/ #include <linux/init.h> @@ -648,15 +654,82 @@ static const struct snd_soc_component_driver davinci_i2s_component = { .name = "davinci-i2s", }; +static struct snd_platform_data* +davinci_i2s_set_pdata_from_of(struct platform_device *pdev) +{
- struct snd_platform_data *pdata = NULL;
- struct device_node *np;
- struct of_phandle_args dma_spec;
- int ret;
- if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
return dev_get_platdata(&pdev->dev);- pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
- if (!pdata)
return ERR_PTR(-ENOMEM);- np = pdev->dev.of_node;
- ret = of_property_match_string(np, "dma-names", "tx");
- if (ret >= 0) {
ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret,&dma_spec);if (ret >= 0)pdata->tx_dma_channel = dma_spec.args[0];- }
- ret = of_property_match_string(np, "dma-names", "rx");
- if (ret >= 0) {
ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret,&dma_spec);if (ret >= 0)pdata->rx_dma_channel = dma_spec.args[0];- }
Why would you do this? If we booted with DT the only thing needed by the edma-pcm (dmaengine) is the name of the DMA channel...
I don't like this code either. But I have tried just to set the dma_filter to "rx" or "tx" and it didn't work. Perhaps because the edma pcm filter function filters by channel number and not by name?
the filter_fn is only used in legacy boot case not in DT boot.
Or do you see an easier way of getting the channel number from the name?
It was failing because you were using wrong dt bindings for the eDMA, if you update that it will work.
- /* optional parameters */
- pdata->enable_channel_combine =
of_property_read_bool(np, "channel-combine");This can only be done when the McBSP is used in DSP_x mode since this will make the McBSP to transmit the stereo audio as mono on the bus.
I have actually never used this option and don't see a benefit of it. Is it just some workaround that should not be included in the binding?
It is to have more time for the DMA to write, read the audio data as McBSP does not have FIFO. This is a SW parameter, so it does not belong to the DT if we take things by the letter. Probably it is better to leave it out for now and add it later if there is a need?
After a quick look at the relevant parts in the driver, I think most of the things are pretty much broken or at least they are not totally correct. McBSP do support real I2S (SRGR:FWID), and the phase setup is wrong in the code. IMO. I don't have HW to test the daVinci-MCBSP or the ASP so I can not comment on how well things are working.
The I2S worked for me if the frame size equalled the PCM data size. E.g. 16-bit audio, 2 channels, 16 bit frame size -> 32 clock ticks per sample in total. However with bigger frame sizes it didn't work because it cannot insert "blanks" between the channels. E.g. with 32-bit wide slots and 16-bit audio it sends both left and right data in the first 32 clock ticks (i.e. still the first channel) and then the during the other channel the data line goes to high impedance which results in effectively playing only the left channel (and noise in the other channel).
Yeah, this is not working on OMAP-McBSP either.
That's probably what the comment about broken I2S is trying to say. I couldn't find any help in the datasheet either.
With exact bclk the I2S is working fine, with most codecs at least we are using that with OMAPs.
On some codecs the frame size is configurable and there it works, but codecs that require 32-bit frames don't work with 16-bit audio or need to use DSP_x format.
- return pdata;
+}
- static int davinci_i2s_probe(struct platform_device *pdev) {
- struct snd_platform_data *pdata; struct davinci_mcbsp_dev *dev; struct resource *mem, *res; void __iomem *io_base; int *dma; int ret;
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- pdata = davinci_i2s_set_pdata_from_of(pdev);
- if (IS_ERR(pdata)) {
dev_err(&pdev->dev, "Error populating platform data, err %ld\n",PTR_ERR(pdata));return PTR_ERR(pdata);- }
- mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu");
- if (!mem) {
dev_warn(&pdev->dev,"\"mpu\" mem resource not found, using index 0\n");mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);if (!mem) {dev_err(&pdev->dev, "no mem resource?\n");return -ENODEV;}- }
io_base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(io_base)) return PTR_ERR(io_base);@@ -680,25 +753,21 @@ static int davinci_i2s_probe(struct platform_device *pdev) (dma_addr_t)(mem->start + DAVINCI_MCBSP_DRR_REG); /* first TX, then RX */
- res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
- if (!res) {
dev_err(&pdev->dev, "no DMA resource\n");ret = -ENXIO;goto err_release_clk;- } dma = &dev->dma_request[SNDRV_PCM_STREAM_PLAYBACK];
- *dma = res->start; dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = dma;
- res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
- if (res)
*dma = res->start;- else
*dma = pdata->tx_dma_channel;Please follow the way davinci-mcasp does it right now: dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; ... dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; res = platform_get_resource(pdev, IORESOURCE_DMA, 0); if (res) *dma = res->start; else *dma = pdata->tx_dma_channel;
/* dmaengine filter data for DT and non-DT boot */ if (pdev->dev.of_node) dma_data->filter_data = "tx"; else dma_data->filter_data = dma;while we are here, I think the tx/rx_dma_channel is something we should just get rid of. It is not used as far as I can tell. Something like this would do I think: dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; ... res = platform_get_resource(pdev, IORESOURCE_DMA, 0); if (res) { dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; *dma = res->start; dma_data->filter_data = dma; } else if (pdev->dev.of_node) { dma_data->filter_data = "tx"; } else { dev_err(&pdev->dev, "Missing DMA tx resource\n"); return -ENODEV; }
Is the edma pcm filter function able to filter in once case by the dma number and in the other case by the dma name? See my above comment as well...
The filter_fn is for the legacy boot, DT does not use filter, it looks up the channel. The reason for the failure in your case was that the dma bindings in mcbsps were not correct, if you update them they will work.
dev->dev = &pdev->dev; dev_set_drvdata(&pdev->dev, dev);@@ -737,11 +806,18 @@ static int davinci_i2s_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id davinci_mcbsp_match[] = {
- { .compatible = "ti,da850-mcbsp-audio" },
I would drop the "-audio" for the binding...
I was trying to stay consistent with the mcasp. But I don't mind keeping the name shorter :-)
Not just shorter, but the -audio postfix is just stupid. IMHO.
- {},
+}; +MODULE_DEVICE_TABLE(of, davinci_mcbsp_match);
- static struct platform_driver davinci_mcbsp_driver = { .probe = davinci_i2s_probe, .remove = davinci_i2s_remove, .driver = { .name = "davinci-mcbsp",
.of_match_table = of_match_ptr(davinci_mcbsp_match),The driver calls itself davinci-i2s.c and the prefixes internally used are davinci_i2s_*. The driver name is "davinci-mcbsp". And it is basically a driver for daVinci ASP (the McBSP additions are not configured at all). I still think we should keep the davinci_i2s_* when adding new code, or rename the whole driver and it's prefixes to something more sensible?
Good point, thanks! I will rename the table to davinci_i2s_match to stay consistent.
Petr
Add clock definitions "davinci-mcbsp.0" and "davinci-mcbsp.1" in order to make McBSP driver work on the DA850 platform.
The McBSP 0 and 1 interfaces were not defined for the DA850 platform. Neither were the related clocks. In order to make the use of McBSP via devicetree the clocks need to be defined.
Signed-off-by: Petr Kulhavy petr@barix.com --- arch/arm/mach-davinci/da850.c | 16 ++++++++++++++++ arch/arm/mach-davinci/psc.h | 2 ++ 2 files changed, 18 insertions(+)
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c index 97d8779a9a65..2b9d972e30ac 100644 --- a/arch/arm/mach-davinci/da850.c +++ b/arch/arm/mach-davinci/da850.c @@ -306,6 +306,20 @@ static struct clk mcasp_clk = { .flags = DA850_CLK_ASYNC3, };
+static struct clk mcbsp0_clk = { + .name = "mcbsp0", + .parent = &pll1_sysclk2, + .lpsc = DA850_LPSC1_McBSP0, + .gpsc = 1, +}; + +static struct clk mcbsp1_clk = { + .name = "mcbsp1", + .parent = &pll1_sysclk2, + .lpsc = DA850_LPSC1_McBSP1, + .gpsc = 1, +}; + static struct clk lcdc_clk = { .name = "lcdc", .parent = &pll0_sysclk2, @@ -464,6 +478,8 @@ static struct clk_lookup da850_clks[] = { CLK("davinci_emac.1", NULL, &emac_clk), CLK("davinci_mdio.0", "fck", &emac_clk), CLK("davinci-mcasp.0", NULL, &mcasp_clk), + CLK("davinci-mcbsp.0", NULL, &mcbsp0_clk), + CLK("davinci-mcbsp.1", NULL, &mcbsp1_clk), CLK("da8xx_lcdc.0", "fck", &lcdc_clk), CLK("da830-mmc.0", NULL, &mmcsd0_clk), CLK("da830-mmc.1", NULL, &mmcsd1_clk), diff --git a/arch/arm/mach-davinci/psc.h b/arch/arm/mach-davinci/psc.h index 99d47cfa301f..8af9f09fc10c 100644 --- a/arch/arm/mach-davinci/psc.h +++ b/arch/arm/mach-davinci/psc.h @@ -171,6 +171,8 @@ #define DA8XX_LPSC1_I2C 11 #define DA8XX_LPSC1_UART1 12 #define DA8XX_LPSC1_UART2 13 +#define DA850_LPSC1_McBSP0 14 +#define DA850_LPSC1_McBSP1 15 #define DA8XX_LPSC1_LCDC 16 #define DA8XX_LPSC1_PWM 17 #define DA850_LPSC1_MMC_SD1 18
Add OF_DEV_AUXDATA for mcbsp to be able to use clocks.
Signed-off-by: Petr Kulhavy petr@barix.com --- arch/arm/mach-davinci/da8xx-dt.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c index c4b5808ca7c1..fd9ac0bcf7a6 100644 --- a/arch/arm/mach-davinci/da8xx-dt.c +++ b/arch/arm/mach-davinci/da8xx-dt.c @@ -45,8 +45,13 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = { OF_DEV_AUXDATA("ns16550a", 0x01d0d000, "serial8250.2", NULL), OF_DEV_AUXDATA("ti,davinci_mdio", 0x01e24000, "davinci_mdio.0", NULL), OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1", - NULL), - OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0", NULL), + NULL), + OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0", + NULL), + OF_DEV_AUXDATA("ti,da850-mcbsp-audio", 0x01d10000, "davinci-mcbsp.0", + NULL), + OF_DEV_AUXDATA("ti,da850-mcbsp-audio", 0x01d11000, "davinci-mcbsp.1", + NULL), {} };
The DA850 supports interrupts 0 to 100. The number of interrupts property "ti,intc-size" was wrongly set to 100. Corrected to 101.
Signed-off-by: Petr Kulhavy petr@barix.com --- arch/arm/boot/dts/da850.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi index 226cda76e77c..5996e765e59c 100644 --- a/arch/arm/boot/dts/da850.dtsi +++ b/arch/arm/boot/dts/da850.dtsi @@ -19,7 +19,7 @@ compatible = "ti,cp-intc"; interrupt-controller; #interrupt-cells = <1>; - ti,intc-size = <100>; + ti,intc-size = <101>; reg = <0xfffee000 0x2000>; }; };
Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux configurations.
Signed-off-by: Petr Kulhavy petr@barix.com --- arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi index 5996e765e59c..9e2b1e97377c 100644 --- a/arch/arm/boot/dts/da850.dtsi +++ b/arch/arm/boot/dts/da850.dtsi @@ -148,7 +148,24 @@ 0xc 0x88888888 0xffffffff >; }; - + mcbsp0_pins: pinmux_mcbsp0_pins { + pinctrl-single,bits = < + /* PINMUX2: + * CLKS0, DX0, DR0, FSX0 + * FSR0, CLKX0, CLKR0 + */ + 0x8 0x02222220 0xfffffff0 + >; + }; + mcbsp1_pins: pinmux_mcbsp1_pins { + pinctrl-single,bits = < + /* PINMUX1: + * CLKS1, DX1, DR1, FSX1, + * FSR1, CLKX1, CLKR1 + */ + 0x4 0x22222220 0xfffffff0 + >; + }; }; edma0: edma@01c00000 { compatible = "ti,edma3-tpcc"; @@ -335,6 +352,32 @@ <&edma0 0 1>; dma-names = "tx", "rx"; }; + + mcbsp0: mcbsp@1d10000 { + compatible = "ti,da850-mcbsp-audio"; + reg = <0x00110000 0x1000>, + <0x00310000 0x1000>; + reg-names = "mpu", "dat"; + interrupts = <97 98>; + interrupts-names = "rx", "tx"; + dmas = <&edma0 3 + &edma0 2>; + dma-names = "tx", "rx"; + status = "disabled"; + }; + mcbsp1: mcbsp@1d11000 { + compatible = "ti,da850-mcbsp-audio"; + reg = <0x00111000 0x1000>, + <0x00311000 0x1000>; + reg-names = "mpu", "dat"; + interrupts = <99 100>; + interrupts-names = "rx", "tx"; + dmas = <&edma0 5 + &edma0 4>; + dma-names = "tx", "rx"; + status = "disabled"; + }; + }; nand_cs3@62000000 { compatible = "ti,davinci-nand";
On 04/06/16 16:21, Petr Kulhavy wrote:
Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux configurations.
Signed-off-by: Petr Kulhavy petr@barix.com
arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi index 5996e765e59c..9e2b1e97377c 100644 --- a/arch/arm/boot/dts/da850.dtsi +++ b/arch/arm/boot/dts/da850.dtsi @@ -148,7 +148,24 @@ 0xc 0x88888888 0xffffffff >; };
mcbsp0_pins: pinmux_mcbsp0_pins {pinctrl-single,bits = </* PINMUX2:* CLKS0, DX0, DR0, FSX0* FSR0, CLKX0, CLKR0*/0x8 0x02222220 0xfffffff0>;};mcbsp1_pins: pinmux_mcbsp1_pins {pinctrl-single,bits = </* PINMUX1:* CLKS1, DX1, DR1, FSX1,* FSR1, CLKX1, CLKR1*/0x4 0x22222220 0xfffffff0
This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is used by the board for audio. When I say conflict, I mean that audio will be completely broken on the board.
>;};@@ -335,6 +352,32 @@ <&edma0 0 1>; dma-names = "tx", "rx"; };
mcbsp0: mcbsp@1d10000 {compatible = "ti,da850-mcbsp-audio";reg = <0x00110000 0x1000>,<0x00310000 0x1000>;reg-names = "mpu", "dat";interrupts = <97 98>;interrupts-names = "rx", "tx";dmas = <&edma0 3&edma0 2>;
This will not work since the eDMA now has the new binding in use, you need to have: dmas = <&edma0 3 1>, <&edma0 2 1>;
McBSP should also select the higher priority TPTC as the McASP does.
dma-names = "tx", "rx";status = "disabled";};mcbsp1: mcbsp@1d11000 {compatible = "ti,da850-mcbsp-audio";reg = <0x00111000 0x1000>,<0x00311000 0x1000>;reg-names = "mpu", "dat";interrupts = <99 100>;interrupts-names = "rx", "tx";dmas = <&edma0 5&edma0 4>;
Same here.
dma-names = "tx", "rx";status = "disabled";};- }; nand_cs3@62000000 { compatible = "ti,davinci-nand";
On 07.04.2016 13:34, Peter Ujfalusi wrote:
On 04/06/16 16:21, Petr Kulhavy wrote:
Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux configurations.
Signed-off-by: Petr Kulhavy petr@barix.com
arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi index 5996e765e59c..9e2b1e97377c 100644 --- a/arch/arm/boot/dts/da850.dtsi +++ b/arch/arm/boot/dts/da850.dtsi @@ -148,7 +148,24 @@ 0xc 0x88888888 0xffffffff >; };
mcbsp0_pins: pinmux_mcbsp0_pins {pinctrl-single,bits = </* PINMUX2:* CLKS0, DX0, DR0, FSX0* FSR0, CLKX0, CLKR0*/0x8 0x02222220 0xfffffff0>;};mcbsp1_pins: pinmux_mcbsp1_pins {pinctrl-single,bits = </* PINMUX1:* CLKS1, DX1, DR1, FSX1,* FSR1, CLKX1, CLKR1*/0x4 0x22222220 0xfffffff0This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is used by the board for audio. When I say conflict, I mean that audio will be completely broken on the board.
I agree with you, the EVM uses the pins for other peripherals. However I understand that the da850.dtsi is a generic description of the DA850 platform. Other DA850 based designs that use the McBSP will not have conflicts. For instance my two AM1808 based boards don't use the McASP. Of course the board's DTS must enable only non-conflicting peripherals/pinmux configurations. But having the pinmuxes defined does not break anything and actually helps creating the DTS file. Or did I miss something?
>;};@@ -335,6 +352,32 @@ <&edma0 0 1>; dma-names = "tx", "rx"; };
mcbsp0: mcbsp@1d10000 {compatible = "ti,da850-mcbsp-audio";reg = <0x00110000 0x1000>,<0x00310000 0x1000>;reg-names = "mpu", "dat";interrupts = <97 98>;interrupts-names = "rx", "tx";dmas = <&edma0 3&edma0 2>;This will not work since the eDMA now has the new binding in use, you need to have: dmas = <&edma0 3 1>, <&edma0 2 1>;
McBSP should also select the higher priority TPTC as the McASP does.
Absolutely, you are right! I still use the old DMA phandles. I will correct that.
Petr
On 04/07/16 15:16, Petr Kulhavy wrote:
On 07.04.2016 13:34, Peter Ujfalusi wrote:
On 04/06/16 16:21, Petr Kulhavy wrote:
Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux configurations.
Signed-off-by: Petr Kulhavy petr@barix.com
arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi index 5996e765e59c..9e2b1e97377c 100644 --- a/arch/arm/boot/dts/da850.dtsi +++ b/arch/arm/boot/dts/da850.dtsi @@ -148,7 +148,24 @@ 0xc 0x88888888 0xffffffff >; };
mcbsp0_pins: pinmux_mcbsp0_pins {pinctrl-single,bits = </* PINMUX2:* CLKS0, DX0, DR0, FSX0* FSR0, CLKX0, CLKR0*/0x8 0x02222220 0xfffffff0>;};mcbsp1_pins: pinmux_mcbsp1_pins {pinctrl-single,bits = </* PINMUX1:* CLKS1, DX1, DR1, FSX1,* FSR1, CLKX1, CLKR1*/0x4 0x22222220 0xfffffff0This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is used by the board for audio. When I say conflict, I mean that audio will be completely broken on the board.
I agree with you, the EVM uses the pins for other peripherals. However I understand that the da850.dtsi is a generic description of the DA850 platform. Other DA850 based designs that use the McBSP will not have conflicts. For instance my two AM1808 based boards don't use the McASP. Of course the board's DTS must enable only non-conflicting peripherals/pinmux configurations. But having the pinmuxes defined does not break anything and actually helps creating the DTS file.
I think what the da850.dtsi does is wrong. The dtsi file should not set any pinmux, those need to be set by the board .dts files If one board uses McASP0, it will set up the pins for that and leave McBSP pins as they were, but other board might use McBSP1 and not use McASP0, there you will have pincontrol for McBSP1.
Or did I miss something?
>;}; }; edma0: edma@01c00000 { compatible = "ti,edma3-tpcc";@@ -335,6 +352,32 @@ <&edma0 0 1>; dma-names = "tx", "rx"; };
mcbsp0: mcbsp@1d10000 {compatible = "ti,da850-mcbsp-audio";reg = <0x00110000 0x1000>,<0x00310000 0x1000>;reg-names = "mpu", "dat";interrupts = <97 98>;interrupts-names = "rx", "tx";dmas = <&edma0 3&edma0 2>;This will not work since the eDMA now has the new binding in use, you need to have: dmas = <&edma0 3 1>, <&edma0 2 1>;
McBSP should also select the higher priority TPTC as the McASP does.
Absolutely, you are right! I still use the old DMA phandles. I will correct that.
Petr
On 07.04.2016 14:45, Peter Ujfalusi wrote:
On 04/07/16 15:16, Petr Kulhavy wrote:
On 07.04.2016 13:34, Peter Ujfalusi wrote:
On 04/06/16 16:21, Petr Kulhavy wrote:
Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux configurations.
Signed-off-by: Petr Kulhavy petr@barix.com
arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi index 5996e765e59c..9e2b1e97377c 100644 --- a/arch/arm/boot/dts/da850.dtsi +++ b/arch/arm/boot/dts/da850.dtsi @@ -148,7 +148,24 @@ 0xc 0x88888888 0xffffffff >; };
mcbsp0_pins: pinmux_mcbsp0_pins {pinctrl-single,bits = </* PINMUX2:* CLKS0, DX0, DR0, FSX0* FSR0, CLKX0, CLKR0*/0x8 0x02222220 0xfffffff0>;};mcbsp1_pins: pinmux_mcbsp1_pins {pinctrl-single,bits = </* PINMUX1:* CLKS1, DX1, DR1, FSX1,* FSR1, CLKX1, CLKR1*/0x4 0x22222220 0xfffffff0This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is used by the board for audio. When I say conflict, I mean that audio will be completely broken on the board.
I agree with you, the EVM uses the pins for other peripherals. However I understand that the da850.dtsi is a generic description of the DA850 platform. Other DA850 based designs that use the McBSP will not have conflicts. For instance my two AM1808 based boards don't use the McASP. Of course the board's DTS must enable only non-conflicting peripherals/pinmux configurations. But having the pinmuxes defined does not break anything and actually helps creating the DTS file.
I think what the da850.dtsi does is wrong. The dtsi file should not set any pinmux, those need to be set by the board .dts files If one board uses McASP0, it will set up the pins for that and leave McBSP pins as they were, but other board might use McBSP1 and not use McASP0, there you will have pincontrol for McBSP1.
It does not set the pinmux, it just defines the configurations. As far as I understand the pin configuration is applied when a node includes these two lines:
pinctrl-names = "default"; pinctrl-0 = <&mcbspc0_pins>;
Petr
On 04/07/16 15:55, Petr Kulhavy wrote:
On 07.04.2016 14:45, Peter Ujfalusi wrote:
On 04/07/16 15:16, Petr Kulhavy wrote:
On 07.04.2016 13:34, Peter Ujfalusi wrote:
On 04/06/16 16:21, Petr Kulhavy wrote:
Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux configurations.
Signed-off-by: Petr Kulhavy petr@barix.com
arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi index 5996e765e59c..9e2b1e97377c 100644 --- a/arch/arm/boot/dts/da850.dtsi +++ b/arch/arm/boot/dts/da850.dtsi @@ -148,7 +148,24 @@ 0xc 0x88888888 0xffffffff >; };
mcbsp0_pins: pinmux_mcbsp0_pins {pinctrl-single,bits = </* PINMUX2:* CLKS0, DX0, DR0, FSX0* FSR0, CLKX0, CLKR0*/0x8 0x02222220 0xfffffff0>;};mcbsp1_pins: pinmux_mcbsp1_pins {pinctrl-single,bits = </* PINMUX1:* CLKS1, DX1, DR1, FSX1,* FSR1, CLKX1, CLKR1*/0x4 0x22222220 0xfffffff0This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is used by the board for audio. When I say conflict, I mean that audio will be completely broken on the board.
I agree with you, the EVM uses the pins for other peripherals. However I understand that the da850.dtsi is a generic description of the DA850 platform. Other DA850 based designs that use the McBSP will not have conflicts. For instance my two AM1808 based boards don't use the McASP. Of course the board's DTS must enable only non-conflicting peripherals/pinmux configurations. But having the pinmuxes defined does not break anything and actually helps creating the DTS file.
I think what the da850.dtsi does is wrong. The dtsi file should not set any pinmux, those need to be set by the board .dts files If one board uses McASP0, it will set up the pins for that and leave McBSP pins as they were, but other board might use McBSP1 and not use McASP0, there you will have pincontrol for McBSP1.
It does not set the pinmux, it just defines the configurations. As far as I understand the pin configuration is applied when a node includes these two lines:
pinctrl-names = "default"; pinctrl-0 = <&mcbspc0_pins>;
Yeah, true. just ignore my comment for the pinctrl part...
Petr
On Thursday 07 April 2016 06:15 PM, Peter Ujfalusi wrote:
I think what the da850.dtsi does is wrong. The dtsi file should not set any pinmux, those need to be set by the board .dts files If one board uses McASP0, it will set up the pins for that and leave McBSP pins as they were, but other board might use McBSP1 and not use McASP0, there you will have pincontrol for McBSP1.
Most functions on da850 are available only on one physical pin. So every board that needs that functionality needs to setup the pinmux the same way. This was the idea behind adding pinmux to da850.dtsi rather than repeating it for every board.
Even though the pinctrl entries are there in da850.dtsi, the pinmux registers wont be touched unless the status is set to "okay" for that device in board .dts file.
The only care that needs to be taken is that the pins listed in da850.dtsi need to be the minimum required for achieving that functionality. If a board needs more than what is populated in da850.dtsi then it can define its own pinctrl entries overriding that coming from da850.dtsi
Thanks, Sekhar
participants (3)
-
Peter Ujfalusi -
Petr Kulhavy -
Sekhar Nori