| Message ID | 1365362721-3731-6-git-send-email-mpa@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Markus Pargmann wrote: > Add support for non-dma pcm for imx platforms with imx-pcm-fiq support. > Instead of imx-pcm-audio, in this case imx-pcm-fiq-audio device is added > and the SIER flags are set differently. So just to be clear, this is interrupt-driven SSI audio? So you're generating an interrupt every time the transmit FIFO goes below the threshold? I wonder if it makes sense to enable both FIFOs, so that you take half as many interrupts per second. > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > --- > sound/soc/fsl/fsl_ssi.c | 70 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 58 insertions(+), 12 deletions(-) > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c > index 7decbd9..afb5a23 100644 > --- a/sound/soc/fsl/fsl_ssi.c > +++ b/sound/soc/fsl/fsl_ssi.c > @@ -120,10 +120,12 @@ struct fsl_ssi_private { > > bool new_binding; > bool ssi_on_imx; > + bool dma; Can you rename this to "use_dma" or something like that? > struct clk *clk; > struct platform_device *imx_pcm_pdev; > struct imx_pcm_dma_params dma_params_tx; > struct imx_pcm_dma_params dma_params_rx; > + struct imx_pcm_fiq_params fiq_params; > > struct { > unsigned int rfrc; > @@ -353,7 +355,8 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, > */ > > /* Enable the interrupts and DMA requests */ > - write_ssi(SIER_FLAGS, &ssi->sier); > + if (ssi_private->dma) > + write_ssi(SIER_FLAGS, &ssi->sier); > > /* > * Set the watermark for transmit FIFI 0 and receive FIFO 0. We > @@ -520,6 +523,18 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, > return -EINVAL; > } > > + if (!ssi_private->dma) { > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > + write_ssi(CCSR_SSI_SOR_TX_CLR, &ssi->sor); > + write_ssi(CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN, > + &ssi->sier); > + } else { > + write_ssi(CCSR_SSI_SOR_RX_CLR, &ssi->sor); > + write_ssi(CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN, > + &ssi->sier); > + } > + } > + > return 0; > } > > @@ -680,6 +695,8 @@ static int fsl_ssi_probe(struct platform_device *pdev) > sizeof(fsl_ssi_dai_template)); > ssi_private->cpu_dai_drv.name = ssi_private->name; > > + ssi_private->dma = !of_property_read_bool(np, "fsl,imx-fiq"); Instead of looking for the FIQ property, maybe you should just look for the absence of a DMA property/node, and then default to interrupts if there is no DMA. That would make it more generic, and even work on non-IMX systems.
On Sun, Apr 07, 2013 at 07:18:50PM -0500, Timur Tabi wrote: > Markus Pargmann wrote: > >Add support for non-dma pcm for imx platforms with imx-pcm-fiq support. > >Instead of imx-pcm-audio, in this case imx-pcm-fiq-audio device is added > >and the SIER flags are set differently. > > So just to be clear, this is interrupt-driven SSI audio? So you're > generating an interrupt every time the transmit FIFO goes below the > threshold? Yes > I wonder if it makes sense to enable both FIFOs, so that you take > half as many interrupts per second. It looks like imx-pcm-fiq and ssi-fiq.s do not support transmitting through two FIFOs at the moment. > > > > >Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > >--- > > sound/soc/fsl/fsl_ssi.c | 70 ++++++++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 58 insertions(+), 12 deletions(-) > > > >diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c > >index 7decbd9..afb5a23 100644 > >--- a/sound/soc/fsl/fsl_ssi.c > >+++ b/sound/soc/fsl/fsl_ssi.c > >@@ -120,10 +120,12 @@ struct fsl_ssi_private { > > > > bool new_binding; > > bool ssi_on_imx; > >+ bool dma; > > Can you rename this to "use_dma" or something like that? Done. > > > struct clk *clk; > > struct platform_device *imx_pcm_pdev; > > struct imx_pcm_dma_params dma_params_tx; > > struct imx_pcm_dma_params dma_params_rx; > >+ struct imx_pcm_fiq_params fiq_params; > > > > struct { > > unsigned int rfrc; > >@@ -353,7 +355,8 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, > > */ > > > > /* Enable the interrupts and DMA requests */ > >- write_ssi(SIER_FLAGS, &ssi->sier); > >+ if (ssi_private->dma) > >+ write_ssi(SIER_FLAGS, &ssi->sier); > > > > /* > > * Set the watermark for transmit FIFI 0 and receive FIFO 0. We > >@@ -520,6 +523,18 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, > > return -EINVAL; > > } > > > >+ if (!ssi_private->dma) { > >+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > >+ write_ssi(CCSR_SSI_SOR_TX_CLR, &ssi->sor); > >+ write_ssi(CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN, > >+ &ssi->sier); > >+ } else { > >+ write_ssi(CCSR_SSI_SOR_RX_CLR, &ssi->sor); > >+ write_ssi(CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN, > >+ &ssi->sier); > >+ } > >+ } > >+ > > return 0; > > } > > > >@@ -680,6 +695,8 @@ static int fsl_ssi_probe(struct platform_device *pdev) > > sizeof(fsl_ssi_dai_template)); > > ssi_private->cpu_dai_drv.name = ssi_private->name; > > > >+ ssi_private->dma = !of_property_read_bool(np, "fsl,imx-fiq"); > > Instead of looking for the FIQ property, maybe you should just look > for the absence of a DMA property/node, and then default to > interrupts if there is no DMA. That would make it more generic, and > even work on non-IMX systems. I do not think it is possible. For example imx27 ssi does support DMA but for specific boards we have to use fiq instead (phycore-ac97). So I would prefer to define the DMA in the chip dtsi file and choose fiq, if necessary, in the board dts. Regards, Markus
Markus Pargmann wrote: > I do not think it is possible. For example imx27 ssi does support > DMA but for specific boards we have to use fiq instead (phycore-ac97). > So I would prefer to define the DMA in the chip dtsi file and choose > fiq, if necessary, in the board dts. I'm not completely familiar with the way i.MX SSI bindings work, but isn't there some property that says, "this SSI needs to use this DMA channel"? So if that property is missing, then it means that there's no link between the SSI and a DMA channel, and so you need to use FIQ. For example, for PowerPC bindings, we have this: ssi@16000 { compatible = "fsl,mpc8610-ssi"; cell-index = <0>; reg = <0x16000 0x100>; interrupt-parent = <&mpic>; interrupts = <62 2>; fsl,mode = "i2s-slave"; codec-handle = <&cs4270>; --> fsl,playback-dma = <&dma00>; --> fsl,capture-dma = <&dma01>; So on PowerPC, if these two properties are missing, then we would use interrupt mode.
On Sun, Apr 14, 2013 at 08:37:21AM -0500, Timur Tabi wrote: > Markus Pargmann wrote: > >I do not think it is possible. For example imx27 ssi does support > >DMA but for specific boards we have to use fiq instead (phycore-ac97). > >So I would prefer to define the DMA in the chip dtsi file and choose > >fiq, if necessary, in the board dts. > > I'm not completely familiar with the way i.MX SSI bindings work, but > isn't there some property that says, "this SSI needs to use this DMA > channel"? So if that property is missing, then it means that there's > no link between the SSI and a DMA channel, and so you need to use > FIQ. > > For example, for PowerPC bindings, we have this: > > ssi@16000 { > compatible = "fsl,mpc8610-ssi"; > cell-index = <0>; > reg = <0x16000 0x100>; > interrupt-parent = <&mpic>; > interrupts = <62 2>; > fsl,mode = "i2s-slave"; > codec-handle = <&cs4270>; > --> fsl,playback-dma = <&dma00>; > --> fsl,capture-dma = <&dma01>; > > So on PowerPC, if these two properties are missing, then we would > use interrupt mode. In general, imx27 does support SSI with DMA, for example MACH_IMX27_VISSTRIM_M10 does use SSI with DMA. But there are special boards like Eukrea TLV320 and phycore-ac97 that have to use FIQ instead. All of them have a imx27 chip. The imx27 SSI node (not posted yet) using the generic DMA bindings: ssi1: ssi@10010000 { compatible = "fsl,imx21-ssi"; reg = <0x10010000 0x1000>; interrupts = <14>; dmas = <&dma 13 &dma 12 &dma 15 &dma 14>; dma-names = "tx", "rx", "tx1", "rx1"; clocks = <&clks 26>; clock-names = "ipg"; fsl,mode = "i2s-slave"; status = "disabled"; }; So instead of defining the dma channels in imx27.dtsi, we could define them only in board dts files. But that would duplicate the DMA property for every board using DMA. The alternative is the "fsl,imx-fiq" property as introduced in this patch. Regards, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Markus Pargmann wrote: > In general, imx27 does support SSI with DMA, for example > MACH_IMX27_VISSTRIM_M10 does use SSI with DMA. But there are special > boards like Eukrea TLV320 and phycore-ac97 that have to use FIQ instead. > All of them have a imx27 chip. Why is the decision to use FIQ board-specific? All of the hardware is inside the SoC itself. How could a board make using DMA impossible?
On Sun, Apr 14, 2013 at 09:53:57AM -0500, Timur Tabi wrote: > Markus Pargmann wrote: > >In general, imx27 does support SSI with DMA, for example > >MACH_IMX27_VISSTRIM_M10 does use SSI with DMA. But there are special > >boards like Eukrea TLV320 and phycore-ac97 that have to use FIQ instead. > >All of them have a imx27 chip. > > Why is the decision to use FIQ board-specific? All of the hardware > is inside the SoC itself. How could a board make using DMA > impossible? I only ported the imx-ssi ac97 driver code and tested twice using DMA without success. But there is a good description of the problem in imx-ssi: " The WM9712 with which this driver was developed with always sends GPIO status data in slot 12 which we receive in our (PCM-) data stream. The only chance we have is to manually skip this data in the FIQ handler." Regards, Markus
Markus Pargmann wrote: > I only ported the imx-ssi ac97 driver code and tested twice using DMA > without success. But there is a good description of the problem in > imx-ssi: > > " The WM9712 with which this driver > was developed with always sends GPIO status data in slot 12 which > we receive in our (PCM-) data stream. The only chance we have is to > manually skip this data in the FIQ handler." Ah, so this is a work-around for a bug, where the codec is incompatible with the SoC? Mark, are you familiar with this issue? Is there a way to reconfigure the codec so that it works better with the SoC? It seems ridiculous to add all this code just because an incompatible codec was chosen for some board. Either way, the board DTS needs to clearly state *why* we're enabling FIQ mode when DMA mode should work. And the patch that adds FIQ support needs to say that it's for boards with broken DMA support.
On Sun, Apr 14, 2013 at 10:25:49AM -0500, Timur Tabi wrote: > Markus Pargmann wrote: > >I only ported the imx-ssi ac97 driver code and tested twice using DMA > >without success. But there is a good description of the problem in > >imx-ssi: > > > >" The WM9712 with which this driver > >was developed with always sends GPIO status data in slot 12 which > >we receive in our (PCM-) data stream. The only chance we have is to > >manually skip this data in the FIQ handler." > > Ah, so this is a work-around for a bug, where the codec is > incompatible with the SoC? Mark, are you familiar with this issue? > Is there a way to reconfigure the codec so that it works better with > the SoC? It seems ridiculous to add all this code just because an > incompatible codec was chosen for some board. > > Either way, the board DTS needs to clearly state *why* we're > enabling FIQ mode when DMA mode should work. And the patch that > adds FIQ support needs to say that it's for boards with broken DMA > support. Okay, I will add those comments to the commit message, binding documentation and add parts of the imx-ssi problem description to fsl-ssi. Regards, Markus
On Sun, Apr 14, 2013 at 10:25:49AM -0500, Timur Tabi wrote: > Ah, so this is a work-around for a bug, where the codec is > incompatible with the SoC? Mark, are you familiar with this issue? > Is there a way to reconfigure the codec so that it works better with > the SoC? It seems ridiculous to add all this code just because an > incompatible codec was chosen for some board. It's a limitation in the SoC - it sort of tries to bodge AC'97 in but doesn't do a great job of it and can't handle the idea that there's multiple streams of data on the bus. This isn't something that can be configured in the CODEC.
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 7decbd9..afb5a23 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -120,10 +120,12 @@ struct fsl_ssi_private { bool new_binding; bool ssi_on_imx; + bool dma; struct clk *clk; struct platform_device *imx_pcm_pdev; struct imx_pcm_dma_params dma_params_tx; struct imx_pcm_dma_params dma_params_rx; + struct imx_pcm_fiq_params fiq_params; struct { unsigned int rfrc; @@ -353,7 +355,8 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, */ /* Enable the interrupts and DMA requests */ - write_ssi(SIER_FLAGS, &ssi->sier); + if (ssi_private->dma) + write_ssi(SIER_FLAGS, &ssi->sier); /* * Set the watermark for transmit FIFI 0 and receive FIFO 0. We @@ -520,6 +523,18 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, return -EINVAL; } + if (!ssi_private->dma) { + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + write_ssi(CCSR_SSI_SOR_TX_CLR, &ssi->sor); + write_ssi(CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN, + &ssi->sier); + } else { + write_ssi(CCSR_SSI_SOR_RX_CLR, &ssi->sor); + write_ssi(CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN, + &ssi->sier); + } + } + return 0; } @@ -680,6 +695,8 @@ static int fsl_ssi_probe(struct platform_device *pdev) sizeof(fsl_ssi_dai_template)); ssi_private->cpu_dai_drv.name = ssi_private->name; + ssi_private->dma = !of_property_read_bool(np, "fsl,imx-fiq"); + /* Get the addresses and IRQ */ ret = of_address_to_resource(np, 0, &res); if (ret) { @@ -701,12 +718,15 @@ static int fsl_ssi_probe(struct platform_device *pdev) goto error_iomap; } - /* The 'name' should not have any slashes in it. */ - ret = request_irq(ssi_private->irq, fsl_ssi_isr, 0, ssi_private->name, - ssi_private); - if (ret < 0) { - dev_err(&pdev->dev, "could not claim irq %u\n", ssi_private->irq); - goto error_irqmap; + if (ssi_private->dma) { + /* The 'name' should not have any slashes in it. */ + ret = request_irq(ssi_private->irq, fsl_ssi_isr, 0, + ssi_private->name, ssi_private); + if (ret < 0) { + dev_err(&pdev->dev, "could not claim irq %u\n", + ssi_private->irq); + goto error_irqmap; + } } /* Are the RX and the TX clocks locked? */ @@ -789,12 +809,38 @@ static int fsl_ssi_probe(struct platform_device *pdev) } if (ssi_private->ssi_on_imx) { - ssi_private->imx_pcm_pdev = - platform_device_register_simple("imx-pcm-audio", + if (!ssi_private->dma) { + ssi_private->imx_pcm_pdev = platform_device_alloc( + "imx-fiq-pcm-audio", + pdev->id); + if (!ssi_private->imx_pcm_pdev) { + ret = -ENOMEM; + goto error_dev; + } + + ssi_private->fiq_params.irq = ssi_private->irq; + ssi_private->fiq_params.base = ssi_private->ssi; + ssi_private->fiq_params.dma_params_rx = + &ssi_private->dma_params_rx; + ssi_private->fiq_params.dma_params_tx = + &ssi_private->dma_params_tx; + + platform_set_drvdata(ssi_private->imx_pcm_pdev, + &ssi_private->fiq_params); + ret = platform_device_add(ssi_private->imx_pcm_pdev); + if (ret) { + dev_err(&pdev->dev, "Failed to add imx-fiq-pcm-audio device\n"); + platform_device_put(ssi_private->imx_pcm_pdev); + goto error_dev; + } + } else { + ssi_private->imx_pcm_pdev = + platform_device_register_simple("imx-pcm-audio", -1, NULL, 0); - if (IS_ERR(ssi_private->imx_pcm_pdev)) { - ret = PTR_ERR(ssi_private->imx_pcm_pdev); - goto error_dev; + if (IS_ERR(ssi_private->imx_pcm_pdev)) { + ret = PTR_ERR(ssi_private->imx_pcm_pdev); + goto error_dev; + } } }
Add support for non-dma pcm for imx platforms with imx-pcm-fiq support. Instead of imx-pcm-audio, in this case imx-pcm-fiq-audio device is added and the SIER flags are set differently. Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- sound/soc/fsl/fsl_ssi.c | 70 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 12 deletions(-)