| Message ID | 66bdda0c0b75d737d649260b9ec10f9f870c20cd.1379590036.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Thu, Sep 26, 2013 at 08:18:28PM +0100, Jyri Sarha wrote: > This patch adds DMA register location to mcasp DT bindings. On am33xx > SoCs the McASP registers are mapped trough L4 interconnect, which is > not accessible by the DMA controller, so McASP data port is mapped > trough L3 to a different location. > > Signed-off-by: Hebbar, Gururaja <gururaja.hebbar@ti.com> > Signed-off-by: Darren Etheridge <detheridge@ti.com> > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > .../bindings/sound/davinci-mcasp-audio.txt | 8 ++- > sound/soc/davinci/davinci-mcasp.c | 59 +++++++++++++------- > 2 files changed, 46 insertions(+), 21 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt > index 374e145..63b67ae 100644 > --- a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt > +++ b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt > @@ -6,7 +6,11 @@ Required properties: > "ti,da830-mcasp-audio" : for both DA830 & DA850 platforms > "ti,omap2-mcasp-audio" : for OMAP2 platforms (TI81xx, AM33xx) > > -- reg : Should contain McASP registers offset and length > +- reg : Should contain McASP registers address and length for mpu and > + optionally for dma controller access. > +- reg-names : The mandatory reg-range must be named "mpu" and the optional DMA > + reg-range must be named "dma". For backward compatibility it is > + good to keep "mpu" first in the list. I've never heard the term "reg-range" before. The should probably be something like "reg entry". How about something like the following instead: - reg: Should contain reg specifiers for the entries in the reg-names property. - reg-names: Should contain: * "mpu" for the main registers (required). For compatibility with existing software, it is recommended this is the first entry. * "dma" for the DMA registers (optional). That way we don't end up describing each reg entry twice. I have some questions however. I took a look at the McASP (TMS320C6000) reference guide, and the registers appeared to all be in one contiguous bank, and "mpu" and "dma" don't appear to be names of particular registers or names of banks of particular registers. Am I looking in the wrong place? Is there up-to-date documentation I can look at? Why are these split across two reg entries, and which particular registers do they actually cover? I have some concern about the description of other properties too. If we're going to amend the binding, they should be fixed up too. > - interrupts : Interrupt number for McASP The device also seems to be able to generate multiple interrupts -- which interrupt does this actually cover? The driver doesn't seem to use it (by a grep of irq|interrupt). Have I missed something? > - op-mode : I2S/DIT ops mode. What type is this? What are valid values? > - tdm-slots : Slots for TDM operation. Here too. This description is completely opaque. Taking a look at the version in kernel sources this appears to be a list of values, in groups of four? What is the format of this property? > @@ -15,7 +19,6 @@ Required properties: > to "num-serializer" parameter. Each entry is a number indication > serializer pin direction. (0 - INACTIVE, 1 - TX, 2 - RX) > > - > Optional properties: > > - ti,hwmods : Must be "mcasp<n>", n is controller instance starting 0 > @@ -31,6 +34,7 @@ mcasp0: mcasp0@1d00000 { > #address-cells = <1>; > #size-cells = <0>; Why does this have #address-cells and #size-cells? There are no children in the example. > reg = <0x100000 0x3000>; > + reg-names "mpu"; > interrupts = <82 83>; > op-mode = <0>; /* MCASP_IIS_MODE */ > tdm-slots = <2>; A few questions from a brief skim of the documentation: There seem to be external clocks (AHCLKR and ACLKR), but they aren't described. Are we always able to use an internal clock instead? Is no external reference clock needed? The err_release_clk label in the error path confuses me -- which clock(s) does the preceding pm_runtime_get ensure remains active? One internal to the McASP? It looks like some pins can be used as GPIO -- is there not a need for something like pinctrl for configuring this? Cheers, Mark. > diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c > index 32ddb7f..a056fc5 100644 > --- a/sound/soc/davinci/davinci-mcasp.c > +++ b/sound/soc/davinci/davinci-mcasp.c > @@ -1001,18 +1001,40 @@ static const struct snd_soc_component_driver davinci_mcasp_component = { > .name = "davinci-mcasp", > }; > > +/* Some HW specific values and defaults. The rest is filled in from DT. */ > +static struct snd_platform_data dm646x_mcasp_pdata = { > + .tx_dma_offset = 0x400, > + .rx_dma_offset = 0x400, > + .asp_chan_q = EVENTQ_0, > + .version = MCASP_VERSION_1, > +}; > + > +static struct snd_platform_data da830_mcasp_pdata = { > + .tx_dma_offset = 0x2000, > + .rx_dma_offset = 0x2000, > + .asp_chan_q = EVENTQ_0, > + .version = MCASP_VERSION_2, > +}; > + > +static struct snd_platform_data omap2_mcasp_pdata = { > + .tx_dma_offset = 0, > + .rx_dma_offset = 0, > + .asp_chan_q = EVENTQ_0, > + .version = MCASP_VERSION_3, > +}; > + > static const struct of_device_id mcasp_dt_ids[] = { > { > .compatible = "ti,dm646x-mcasp-audio", > - .data = (void *)MCASP_VERSION_1, > + .data = &dm646x_mcasp_pdata, > }, > { > .compatible = "ti,da830-mcasp-audio", > - .data = (void *)MCASP_VERSION_2, > + .data = &da830_mcasp_pdata, > }, > { > .compatible = "ti,omap2-mcasp-audio", > - .data = (void *)MCASP_VERSION_3, > + .data = &omap2_mcasp_pdata, > }, > { /* sentinel */ } > }; > @@ -1035,20 +1057,13 @@ static struct snd_platform_data *davinci_mcasp_set_pdata_from_of( > pdata = pdev->dev.platform_data; > return pdata; > } else if (match) { > - pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > - if (!pdata) { > - ret = -ENOMEM; > - goto nodata; > - } > + pdata = (struct snd_platform_data *) match->data; > } else { > /* control shouldn't reach here. something is wrong */ > ret = -EINVAL; > goto nodata; > } > > - if (match->data) > - pdata->version = (u8)((int)match->data); > - > ret = of_property_read_u32(np, "op-mode", &val); > if (ret >= 0) > pdata->op_mode = val; > @@ -1145,10 +1160,15 @@ static int davinci_mcasp_probe(struct platform_device *pdev) > return -EINVAL; > } > > - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu"); > if (!mem) { > - dev_err(&pdev->dev, "no mem resource?\n"); > - return -ENODEV; > + dev_warn(dev->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; > + } > } > > ioarea = devm_request_mem_region(&pdev->dev, mem->start, > @@ -1182,13 +1202,16 @@ static int davinci_mcasp_probe(struct platform_device *pdev) > dev->rxnumevt = pdata->rxnumevt; > dev->dev = &pdev->dev; > > + dma = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma"); > + if (!dma) > + dma = mem; > + > dma_data = &dev->dma_params[SNDRV_PCM_STREAM_PLAYBACK]; > dma_data->asp_chan_q = pdata->asp_chan_q; > dma_data->ram_chan_q = pdata->ram_chan_q; > dma_data->sram_pool = pdata->sram_pool; > dma_data->sram_size = pdata->sram_size_playback; > - dma_data->dma_addr = (dma_addr_t) (pdata->tx_dma_offset + > - mem->start); > + dma_data->dma_addr = dma->start + pdata->tx_dma_offset; > > /* first TX, then RX */ > res = platform_get_resource(pdev, IORESOURCE_DMA, 0); > @@ -1205,8 +1228,7 @@ static int davinci_mcasp_probe(struct platform_device *pdev) > dma_data->ram_chan_q = pdata->ram_chan_q; > dma_data->sram_pool = pdata->sram_pool; > dma_data->sram_size = pdata->sram_size_capture; > - dma_data->dma_addr = (dma_addr_t)(pdata->rx_dma_offset + > - mem->start); > + dma_data->dma_addr = dma->start + pdata->rx_dma_offset; > > res = platform_get_resource(pdev, IORESOURCE_DMA, 1); > if (!res) { > @@ -1266,4 +1288,3 @@ module_platform_driver(davinci_mcasp_driver); > MODULE_AUTHOR("Steve Chen"); > MODULE_DESCRIPTION("TI DAVINCI McASP SoC Interface"); > MODULE_LICENSE("GPL"); > - > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 07, 2013 at 10:47:18PM +0100, Mark Rutland wrote: > On Thu, Sep 26, 2013 at 08:18:28PM +0100, Jyri Sarha wrote: > > - interrupts : Interrupt number for McASP > The device also seems to be able to generate multiple interrupts -- which > interrupt does this actually cover? > The driver doesn't seem to use it (by a grep of irq|interrupt). Have I missed > something? No, they're not actually of much practical use to us at the minute but it was generally felt better to include the information and not use it so that if someone does come up with a use for them then the trees for deployed systems already have the information.
On 10/08/2013 12:47 AM, Mark Rutland wrote: > Hello, Hi! I am not the author of this driver so my knowledge may have gaps, when it comes to details I have not touched yet. Anyway, I do my best to address your comments. ... >> -- reg : Should contain McASP registers offset and length >> +- reg : Should contain McASP registers address and length for mpu and >> + optionally for dma controller access. >> +- reg-names : The mandatory reg-range must be named "mpu" and the optional DMA >> + reg-range must be named "dma". For backward compatibility it is >> + good to keep "mpu" first in the list. > > I've never heard the term "reg-range" before. The should probably be something > like "reg entry". How about something like the following instead: > Well, an address and length describes a "range", but if it is not commonly used term, there is no need to use it here either. > - reg: Should contain reg specifiers for the entries in the reg-names property. > > - reg-names: Should contain: > * "mpu" for the main registers (required). For compatibility with > existing software, it is recommended this is the first entry. > * "dma" for the DMA registers (optional). > > That way we don't end up describing each reg entry twice. The both contain the same information, but your version is certainly easier to read. I'll take it. Thanks! > > I have some questions however. I took a look at the McASP (TMS320C6000) > reference guide, and the registers appeared to all be in one contiguous bank, > and "mpu" and "dma" don't appear to be names of particular registers or names > of banks of particular registers. Am I looking in the wrong place? Is there > up-to-date documentation I can look at? > > Why are these split across two reg entries, and which particular registers do > they actually cover? The need for two different register properties does not come from McASP IP, but from SoC design. On AM33XX the McASP registers are mapped for MPU through L4 bus, which is no accessible to the DMA controller. The McASP data port registers are mapped trough L3 buss to a different address for DMA controller. Maybe the second register property could be called to "dat" instead of "dma" since only data port registers are mapped trough that address. > > I have some concern about the description of other properties too. If we're > going to amend the binding, they should be fixed up too. > >> - interrupts : Interrupt number for McASP > > The device also seems to be able to generate multiple interrupts -- which > interrupt does this actually cover? > > The driver doesn't seem to use it (by a grep of irq|interrupt). Have I missed > something? > No you have not. A later patch in this set make the interrupt property optional and adds "tx" and "rx" interrupt-names: http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066732.html >> - op-mode : I2S/DIT ops mode. > > What type is this? That property selects whether McASP is working in I2S mode or in Integrated Digital audio interface Transmitter (DIT) mode for S/PDIF, IEC60958-1, AES-3 formats. > > What are valid values? 0 and 1. I'll document those. Thanks. >> - tdm-slots : Slots for TDM operation. > > Here too. This description is completely opaque. > I am not absolutely sure here. I'll check the details and try to come up with a better description. Talking about num-serializer and serial-dir property here... > Taking a look at the version in kernel sources this appears to be a list of > values, in groups of four? > > What is the format of this property? > >> @@ -15,7 +19,6 @@ Required properties: >> to "num-serializer" parameter. Each entry is a number indication >> serializer pin direction. (0 - INACTIVE, 1 - TX, 2 - RX) >> >> - The McASP can have up to 16 serializers for selecting data pin usage. The numbers have been grouped into 4x4 matrix only for better readability. BTW, the num-serializers property is redundant and it will be removed in the next version of the patch series. >> Optional properties: >> >> - ti,hwmods : Must be "mcasp<n>", n is controller instance starting 0 >> @@ -31,6 +34,7 @@ mcasp0: mcasp0@1d00000 { >> #address-cells = <1>; >> #size-cells = <0>; > > Why does this have #address-cells and #size-cells? There are no children in the > example. > I removed those from the actual dts file, but forgot to do so here. I'll fix that. Thanks! >> reg = <0x100000 0x3000>; >> + reg-names "mpu"; >> interrupts = <82 83>; >> op-mode = <0>; /* MCASP_IIS_MODE */ >> tdm-slots = <2>; > > A few questions from a brief skim of the documentation: > > There seem to be external clocks (AHCLKR and ACLKR), but they aren't described. > Are we always able to use an internal clock instead? Is no external reference > clock needed? As far as I understand the choice is to use either McASP internal clock or codec provided clock. The selection is made by the machine/platform driver by calling snd_soc_dai_set_fmt() and applying the required bits in SND_SOC_DAIFMT_MASTER_MASK. I guess the choice may not always be HW specific. > > The err_release_clk label in the error path confuses me -- which clock(s) does > the preceding pm_runtime_get ensure remains active? One internal to the McASP? > I guess that is really up to machine/platform driver that configures dapm. > It looks like some pins can be used as GPIO -- is there not a need for something > like pinctrl for configuring this? > That is true, but since driver does not support it and there is no example of GPIO usage, no guess work has been done about the required DT properties. > Cheers, > Mark. > >> diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c >> index 32ddb7f..a056fc5 100644 >> --- a/sound/soc/davinci/davinci-mcasp.c >> +++ b/sound/soc/davinci/davinci-mcasp.c >> @@ -1001,18 +1001,40 @@ static const struct snd_soc_component_driver davinci_mcasp_component = { >> .name = "davinci-mcasp", >> }; >> >> +/* Some HW specific values and defaults. The rest is filled in from DT. */ >> +static struct snd_platform_data dm646x_mcasp_pdata = { >> + .tx_dma_offset = 0x400, >> + .rx_dma_offset = 0x400, >> + .asp_chan_q = EVENTQ_0, >> + .version = MCASP_VERSION_1, >> +}; >> + >> +static struct snd_platform_data da830_mcasp_pdata = { >> + .tx_dma_offset = 0x2000, >> + .rx_dma_offset = 0x2000, >> + .asp_chan_q = EVENTQ_0, >> + .version = MCASP_VERSION_2, >> +}; >> + >> +static struct snd_platform_data omap2_mcasp_pdata = { >> + .tx_dma_offset = 0, >> + .rx_dma_offset = 0, >> + .asp_chan_q = EVENTQ_0, >> + .version = MCASP_VERSION_3, >> +}; >> + >> static const struct of_device_id mcasp_dt_ids[] = { >> { >> .compatible = "ti,dm646x-mcasp-audio", >> - .data = (void *)MCASP_VERSION_1, >> + .data = &dm646x_mcasp_pdata, >> }, >> { >> .compatible = "ti,da830-mcasp-audio", >> - .data = (void *)MCASP_VERSION_2, >> + .data = &da830_mcasp_pdata, >> }, >> { >> .compatible = "ti,omap2-mcasp-audio", >> - .data = (void *)MCASP_VERSION_3, >> + .data = &omap2_mcasp_pdata, >> }, >> { /* sentinel */ } >> }; >> @@ -1035,20 +1057,13 @@ static struct snd_platform_data *davinci_mcasp_set_pdata_from_of( >> pdata = pdev->dev.platform_data; >> return pdata; >> } else if (match) { >> - pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> - if (!pdata) { >> - ret = -ENOMEM; >> - goto nodata; >> - } >> + pdata = (struct snd_platform_data *) match->data; >> } else { >> /* control shouldn't reach here. something is wrong */ >> ret = -EINVAL; >> goto nodata; >> } >> >> - if (match->data) >> - pdata->version = (u8)((int)match->data); >> - >> ret = of_property_read_u32(np, "op-mode", &val); >> if (ret >= 0) >> pdata->op_mode = val; >> @@ -1145,10 +1160,15 @@ static int davinci_mcasp_probe(struct platform_device *pdev) >> return -EINVAL; >> } >> >> - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu"); >> if (!mem) { >> - dev_err(&pdev->dev, "no mem resource?\n"); >> - return -ENODEV; >> + dev_warn(dev->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; >> + } >> } >> >> ioarea = devm_request_mem_region(&pdev->dev, mem->start, >> @@ -1182,13 +1202,16 @@ static int davinci_mcasp_probe(struct platform_device *pdev) >> dev->rxnumevt = pdata->rxnumevt; >> dev->dev = &pdev->dev; >> >> + dma = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma"); >> + if (!dma) >> + dma = mem; >> + >> dma_data = &dev->dma_params[SNDRV_PCM_STREAM_PLAYBACK]; >> dma_data->asp_chan_q = pdata->asp_chan_q; >> dma_data->ram_chan_q = pdata->ram_chan_q; >> dma_data->sram_pool = pdata->sram_pool; >> dma_data->sram_size = pdata->sram_size_playback; >> - dma_data->dma_addr = (dma_addr_t) (pdata->tx_dma_offset + >> - mem->start); >> + dma_data->dma_addr = dma->start + pdata->tx_dma_offset; >> >> /* first TX, then RX */ >> res = platform_get_resource(pdev, IORESOURCE_DMA, 0); >> @@ -1205,8 +1228,7 @@ static int davinci_mcasp_probe(struct platform_device *pdev) >> dma_data->ram_chan_q = pdata->ram_chan_q; >> dma_data->sram_pool = pdata->sram_pool; >> dma_data->sram_size = pdata->sram_size_capture; >> - dma_data->dma_addr = (dma_addr_t)(pdata->rx_dma_offset + >> - mem->start); >> + dma_data->dma_addr = dma->start + pdata->rx_dma_offset; >> >> res = platform_get_resource(pdev, IORESOURCE_DMA, 1); >> if (!res) { >> @@ -1266,4 +1288,3 @@ module_platform_driver(davinci_mcasp_driver); >> MODULE_AUTHOR("Steve Chen"); >> MODULE_DESCRIPTION("TI DAVINCI McASP SoC Interface"); >> MODULE_LICENSE("GPL"); >> - >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10/08/2013 12:13 PM, Jyri Sarha wrote: >> I have some questions however. I took a look at the McASP (TMS320C6000) >> reference guide, and the registers appeared to all be in one contiguous >> bank, and "mpu" and "dma" don't appear to be names of particular >> registers or names of banks of particular registers. Am I looking in the >> wrong place? Is there up-to-date documentation I can look at? >> >> Why are these split across two reg entries, and which particular >> registers do they actually cover? > > The need for two different register properties does not come from McASP > IP, but from SoC design. On AM33XX the McASP registers are mapped for MPU > through L4 bus, which is no accessible to the DMA controller. The McASP > data port registers are mapped trough L3 buss to a different address for > DMA controller. Maybe the second register property could be called to "dat" > instead of "dma" since only data port registers are mapped trough that > address. We have discussed this with Jyri and we are going to use 'dat' for the data port address. The driver can configure the IP via the 'mpu' area but it is preferred to use the 'dat' or data port of McASP when it comes to data. It is possible to use the transmit/receive buffer registers in the 'mpu' area but it get's complicated when more than one serializer is used. >>> - op-mode : I2S/DIT ops mode. >> >> What type is this? > > That property selects whether McASP is working in I2S mode or in > Integrated Digital audio interface Transmitter (DIT) mode for S/PDIF, > IEC60958-1, AES-3 formats. Not sure but we might be able to handle this via standard ASoC way, via the da_fmt so the machine driver will tell the mcasp driver what is the format it needs to send the data. Adding SND_SOC_DAIFMT_SPDIF/IEC60958/AES3 to core will be needed. > >> >> What are valid values? > > 0 and 1. I'll document those. Thanks. > >>> - tdm-slots : Slots for TDM operation. >> >> Here too. This description is completely opaque. >> > > I am not absolutely sure here. I'll check the details and try to come up > with a better description. Not sure about this either, my interpretation of what it means: number of channels to be sent via one serializer. as an example: We have 3 stereo codecs in the design all of them are connected to the same McASP instance but to be fed via different serializer: AXR0 - codec0 AXR1 - codec1 AXR2 - codec2 Since the codecs are stereo -> tdm-slots = <2>; two channels/serializer If the stream is stereo we only ouput to codec0 via AXR0. If the stream is 4 channel, the first 2 channels will be via AXR0 to codec0 the next 2 channles will be sent to codec1 via AXR1. and so on. I think it would be possible to extract this information from the serial-dir property + machine driver configured DAI_FMT and from the coming stream. I2S, L/R justified is stereo, in DSP modes we can have more channels. In the serial-dir array we could count the AXR directions. Basically all the information which is needed to make this redundant are there. But we have the legacy davinci users at the same time and we do not want to break eisting users. I think Daniel Mack (CCd) can clarify my understanding of the tdm-slots. >> It looks like some pins can be used as GPIO -- is there not a need for >> something like pinctrl for configuring this? >> > > That is true, but since driver does not support it and there is no example > of GPIO usage, no guess work has been done about the required DT > properties. This is kind of complicated issue. Any of the McASP pins (CLK, FS, AXR, etc) can be configured to be used as McASP pins or GPIO pins. This configuration is McASP IP specific, need to be done within McASP. pinctrl is overkill for this IMHO but we would need a gpio driver for McASP. If some driver requests a gpio in McASP we can just switch that pin to GPIO mode, however this brings up quite a bit of questions: - embedd the gpio driver into the dai driver? - if the system does not have audio -> no GPIO driver for the rest? - separate driver for gpio functionality? - Sanity cheks all over the place on which pin is GPIO or McASP. and so forth.
On Tue, Oct 08, 2013 at 01:46:41AM +0100, Mark Brown wrote: > On Mon, Oct 07, 2013 at 10:47:18PM +0100, Mark Rutland wrote: > > On Thu, Sep 26, 2013 at 08:18:28PM +0100, Jyri Sarha wrote: > > > > - interrupts : Interrupt number for McASP > > > The device also seems to be able to generate multiple interrupts -- which > > interrupt does this actually cover? > > > The driver doesn't seem to use it (by a grep of irq|interrupt). Have I missed > > something? > > No, they're not actually of much practical use to us at the minute but > it was generally felt better to include the information and not use it > so that if someone does come up with a use for them then the trees for > deployed systems already have the information. Sure, but if this device can generate multiple interrupts, we should make it possible to describe all of them, unambiguously. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/10/2013 07:59 PM, Mark Rutland wrote: >> No, they're not actually of much practical use to us at the minute but >> it was generally felt better to include the information and not use it >> so that if someone does come up with a use for them then the trees for >> deployed systems already have the information. > > Sure, but if this device can generate multiple interrupts, we should > make it possible to describe all of them, unambiguously. This is why Jyri added them to the DT. They are not used by the Linux driver, but the HW have interrupt lines (two of them: TX and RX).
On Thu, Oct 10, 2013 at 06:29:59PM +0100, Peter Ujfalusi wrote: > On 10/10/2013 07:59 PM, Mark Rutland wrote: > >> No, they're not actually of much practical use to us at the minute but > >> it was generally felt better to include the information and not use it > >> so that if someone does come up with a use for them then the trees for > >> deployed systems already have the information. > > > > Sure, but if this device can generate multiple interrupts, we should > > make it possible to describe all of them, unambiguously. > > This is why Jyri added them to the DT. They are not used by the Linux driver, > but the HW have interrupt lines (two of them: TX and RX). The binding only describes a single interrupt, and even if multiple interrupts were listed, there's no way to disambiguate them (e.g. interrupt-names). It would be nice to remedy this. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/16/2013 06:04 PM, Mark Rutland wrote: > On Thu, Oct 10, 2013 at 06:29:59PM +0100, Peter Ujfalusi wrote: >> On 10/10/2013 07:59 PM, Mark Rutland wrote: >>>> No, they're not actually of much practical use to us at the minute but >>>> it was generally felt better to include the information and not use it >>>> so that if someone does come up with a use for them then the trees for >>>> deployed systems already have the information. >>> >>> Sure, but if this device can generate multiple interrupts, we should >>> make it possible to describe all of them, unambiguously. >> >> This is why Jyri added them to the DT. They are not used by the Linux driver, >> but the HW have interrupt lines (two of them: TX and RX). > > The binding only describes a single interrupt, and even if multiple > interrupts were listed, there's no way to disambiguate them (e.g. > interrupt-names). > > It would be nice to remedy this. > This has been fixed in a later patch in the same series here: [RESEND PATCH v3 05/11] ASoC: davinci-mcasp: Interrupts property to optional and add interrupt-names http://mailman.alsa-project.org/pipermail/alsa-devel/2013-September/066732.html However the v3 series is already obsolete. The latest posted series is v4. There the patch patch bellow inculdes interrupt names, plus some other improvements based on your comments earlier: [PATCH v4 05/10] ASoC: davinci-mcasp: Improve DT bindings document http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067100.html There is already couple of updates even to v4 series of the patches, here: [PATCH v4.1 08/10] ARM/dts: am33xx: mcasp: Add location for data port registers to reg-property http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067125.html [PATCH v4.2 09/10] ARM/dts: am335x-evm: Add audio support for am335x-evm.dts http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067133.html [PATCH v4.2 10/10] ARM/dts: am335x-evmsk: Audio support http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067140.html Best regards, Jyri -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt index 374e145..63b67ae 100644 --- a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt +++ b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt @@ -6,7 +6,11 @@ Required properties: "ti,da830-mcasp-audio" : for both DA830 & DA850 platforms "ti,omap2-mcasp-audio" : for OMAP2 platforms (TI81xx, AM33xx) -- reg : Should contain McASP registers offset and length +- reg : Should contain McASP registers address and length for mpu and + optionally for dma controller access. +- reg-names : The mandatory reg-range must be named "mpu" and the optional DMA + reg-range must be named "dma". For backward compatibility it is + good to keep "mpu" first in the list. - interrupts : Interrupt number for McASP - op-mode : I2S/DIT ops mode. - tdm-slots : Slots for TDM operation. @@ -15,7 +19,6 @@ Required properties: to "num-serializer" parameter. Each entry is a number indication serializer pin direction. (0 - INACTIVE, 1 - TX, 2 - RX) - Optional properties: - ti,hwmods : Must be "mcasp<n>", n is controller instance starting 0 @@ -31,6 +34,7 @@ mcasp0: mcasp0@1d00000 { #address-cells = <1>; #size-cells = <0>; reg = <0x100000 0x3000>; + reg-names "mpu"; interrupts = <82 83>; op-mode = <0>; /* MCASP_IIS_MODE */ tdm-slots = <2>; diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 32ddb7f..a056fc5 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -1001,18 +1001,40 @@ static const struct snd_soc_component_driver davinci_mcasp_component = { .name = "davinci-mcasp", }; +/* Some HW specific values and defaults. The rest is filled in from DT. */ +static struct snd_platform_data dm646x_mcasp_pdata = { + .tx_dma_offset = 0x400, + .rx_dma_offset = 0x400, + .asp_chan_q = EVENTQ_0, + .version = MCASP_VERSION_1, +}; + +static struct snd_platform_data da830_mcasp_pdata = { + .tx_dma_offset = 0x2000, + .rx_dma_offset = 0x2000, + .asp_chan_q = EVENTQ_0, + .version = MCASP_VERSION_2, +}; + +static struct snd_platform_data omap2_mcasp_pdata = { + .tx_dma_offset = 0, + .rx_dma_offset = 0, + .asp_chan_q = EVENTQ_0, + .version = MCASP_VERSION_3, +}; + static const struct of_device_id mcasp_dt_ids[] = { { .compatible = "ti,dm646x-mcasp-audio", - .data = (void *)MCASP_VERSION_1, + .data = &dm646x_mcasp_pdata, }, { .compatible = "ti,da830-mcasp-audio", - .data = (void *)MCASP_VERSION_2, + .data = &da830_mcasp_pdata, }, { .compatible = "ti,omap2-mcasp-audio", - .data = (void *)MCASP_VERSION_3, + .data = &omap2_mcasp_pdata, }, { /* sentinel */ } }; @@ -1035,20 +1057,13 @@ static struct snd_platform_data *davinci_mcasp_set_pdata_from_of( pdata = pdev->dev.platform_data; return pdata; } else if (match) { - pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); - if (!pdata) { - ret = -ENOMEM; - goto nodata; - } + pdata = (struct snd_platform_data *) match->data; } else { /* control shouldn't reach here. something is wrong */ ret = -EINVAL; goto nodata; } - if (match->data) - pdata->version = (u8)((int)match->data); - ret = of_property_read_u32(np, "op-mode", &val); if (ret >= 0) pdata->op_mode = val; @@ -1145,10 +1160,15 @@ static int davinci_mcasp_probe(struct platform_device *pdev) return -EINVAL; } - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu"); if (!mem) { - dev_err(&pdev->dev, "no mem resource?\n"); - return -ENODEV; + dev_warn(dev->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; + } } ioarea = devm_request_mem_region(&pdev->dev, mem->start, @@ -1182,13 +1202,16 @@ static int davinci_mcasp_probe(struct platform_device *pdev) dev->rxnumevt = pdata->rxnumevt; dev->dev = &pdev->dev; + dma = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma"); + if (!dma) + dma = mem; + dma_data = &dev->dma_params[SNDRV_PCM_STREAM_PLAYBACK]; dma_data->asp_chan_q = pdata->asp_chan_q; dma_data->ram_chan_q = pdata->ram_chan_q; dma_data->sram_pool = pdata->sram_pool; dma_data->sram_size = pdata->sram_size_playback; - dma_data->dma_addr = (dma_addr_t) (pdata->tx_dma_offset + - mem->start); + dma_data->dma_addr = dma->start + pdata->tx_dma_offset; /* first TX, then RX */ res = platform_get_resource(pdev, IORESOURCE_DMA, 0); @@ -1205,8 +1228,7 @@ static int davinci_mcasp_probe(struct platform_device *pdev) dma_data->ram_chan_q = pdata->ram_chan_q; dma_data->sram_pool = pdata->sram_pool; dma_data->sram_size = pdata->sram_size_capture; - dma_data->dma_addr = (dma_addr_t)(pdata->rx_dma_offset + - mem->start); + dma_data->dma_addr = dma->start + pdata->rx_dma_offset; res = platform_get_resource(pdev, IORESOURCE_DMA, 1); if (!res) { @@ -1266,4 +1288,3 @@ module_platform_driver(davinci_mcasp_driver); MODULE_AUTHOR("Steve Chen"); MODULE_DESCRIPTION("TI DAVINCI McASP SoC Interface"); MODULE_LICENSE("GPL"); -