| Message ID | 5fd6e6c463c2e8d141cf4b485f585f007a71442e.1442469788.git.shengjiu.wang@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 17, 2015 at 02:09:24PM +0800, Shengjiu Wang wrote: > + "dma" The spba clock is needed when sdma share peripheral > + script is used. It sounds like this clock is used by the DMA controller - shouldn't the DMA controller driver be responsible for enabling it?
On Thu, Sep 17, 2015 at 12:01:54PM +0100, Mark Brown wrote: > On Thu, Sep 17, 2015 at 02:09:24PM +0800, Shengjiu Wang wrote: > > > + "dma" The spba clock is needed when sdma share peripheral > > + script is used. > It sounds like this clock is used by the DMA controller - shouldn't the > DMA controller driver be responsible for enabling it? I agree but it doesn't seems to be easy to enable and disable it dynamically along with ASoC open() and close() functions as the current clocks of SDMA get enabled when its channels are allocated during the system boot-up via generic DMA bindings. Maybe we can put in the issue_pending() and terminate_all() which are regularly called along with ASoC trigger(), or just open it permanently if the penalty of power efficiency is ignorable.
On Thu, Sep 17, 2015 at 10:54:08AM -0700, Nicolin Chen wrote: > On Thu, Sep 17, 2015 at 12:01:54PM +0100, Mark Brown wrote: > > On Thu, Sep 17, 2015 at 02:09:24PM +0800, Shengjiu Wang wrote: > > > + "dma" The spba clock is needed when sdma share peripheral > > > + script is used. > > It sounds like this clock is used by the DMA controller - shouldn't the > > DMA controller driver be responsible for enabling it? > I agree but it doesn't seems to be easy to enable and disable it > dynamically along with ASoC open() and close() functions as the > current clocks of SDMA get enabled when its channels are allocated > during the system boot-up via generic DMA bindings. Do we need to do things along with open() and close() or wouldn't doing things along with the DMA operatiosn suffice? > Maybe we can put in the issue_pending() and terminate_all() which > are regularly called along with ASoC trigger(), or just open it > permanently if the penalty of power efficiency is ignorable. Or some kind of runtime PM usage within the DMA driver (possibly at those points)? It does seem like something we should solve in the DMA code rather than in generic code.
On Sat, Sep 19, 2015 at 06:50:40AM -0700, Mark Brown wrote: > On Thu, Sep 17, 2015 at 10:54:08AM -0700, Nicolin Chen wrote: > > On Thu, Sep 17, 2015 at 12:01:54PM +0100, Mark Brown wrote: > > > On Thu, Sep 17, 2015 at 02:09:24PM +0800, Shengjiu Wang wrote: > > > > > + "dma" The spba clock is needed when sdma share peripheral > > > > + script is used. > > > > It sounds like this clock is used by the DMA controller - shouldn't the > > > DMA controller driver be responsible for enabling it? > > > I agree but it doesn't seems to be easy to enable and disable it > > dynamically along with ASoC open() and close() functions as the > > current clocks of SDMA get enabled when its channels are allocated > > during the system boot-up via generic DMA bindings. > > Do we need to do things along with open() and close() or wouldn't doing > things along with the DMA operatiosn suffice? > > > Maybe we can put in the issue_pending() and terminate_all() which > > are regularly called along with ASoC trigger(), or just open it > > permanently if the penalty of power efficiency is ignorable. > > Or some kind of runtime PM usage within the DMA driver (possibly at > those points)? It does seem like something we should solve in the DMA > code rather than in generic code. > I think spba clock should be used by the audio IP not DMA, because in same situation the FSL_SSI doesn't need this clock. best regards wang shengjiu
On Sat, Oct 10, 2015 at 7:26 AM, Shengjiu Wang <shengjiu.wang@freescale.com> wrote: > I think spba clock should be used by the audio IP not DMA, because in same > situation the FSL_SSI doesn't need this clock. According to this commit from FSL BSP, the SSI2 port on MX53 is connected to SPBA and needs the SPBA clock: http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/mach-mx5/clock.c?h=imx_2.6.35_maintain&id=2438f48431b7aff00a522502d5a6ea217ea133d9
On Sat, Oct 10, 2015 at 05:03:07PM -0300, Fabio Estevam wrote: > On Sat, Oct 10, 2015 at 7:26 AM, Shengjiu Wang > <shengjiu.wang@freescale.com> wrote: > > > I think spba clock should be used by the audio IP not DMA, because in same > > situation the FSL_SSI doesn't need this clock. > > According to this commit from FSL BSP, the SSI2 port on MX53 is > connected to SPBA and needs the SPBA clock: > > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/mach-mx5/clock.c?h=imx_2.6.35_maintain&id=2438f48431b7aff00a522502d5a6ea217ea133d9 Hmm, the detail of an SPBA issue is sometimes complicated due to different fabric designs between SoCs. We cannot simply say that a DMA access for an IP under the Shared Peripheral Bus requires SPBA clock or not unless the hardware team clearly demands that. However, as an arbiter, the SPBA must be outside any of those IPs and tied to the SDMA in order to balancing memory access between the SDMA and a co-existing CPU. The question is whether we treat it as a part of an IP wrapper or a part of the SDMA. Therefore, unless a CPU access would fail even *without any SDMA access being launched*, it sounds fair to me to put it into the DMA code.
On Sat, Oct 10, 2015 at 01:35:57PM -0700, Nicolin Chen wrote: > On Sat, Oct 10, 2015 at 05:03:07PM -0300, Fabio Estevam wrote: > > On Sat, Oct 10, 2015 at 7:26 AM, Shengjiu Wang > > <shengjiu.wang@freescale.com> wrote: > > > > > I think spba clock should be used by the audio IP not DMA, because in same > > > situation the FSL_SSI doesn't need this clock. > > > > According to this commit from FSL BSP, the SSI2 port on MX53 is > > connected to SPBA and needs the SPBA clock: > > > > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/mach-mx5/clock.c?h=imx_2.6.35_maintain&id=2438f48431b7aff00a522502d5a6ea217ea133d9 > > Hmm, the detail of an SPBA issue is sometimes complicated due to > different fabric designs between SoCs. We cannot simply say that > a DMA access for an IP under the Shared Peripheral Bus requires > SPBA clock or not unless the hardware team clearly demands that. > > However, as an arbiter, the SPBA must be outside any of those IPs > and tied to the SDMA in order to balancing memory access between > the SDMA and a co-existing CPU. The question is whether we treat > it as a part of an IP wrapper or a part of the SDMA. > > Therefore, unless a CPU access would fail even *without any SDMA > access being launched*, it sounds fair to me to put it into the > DMA code. I discussed this issue with IC team. The SPBA bus has 3 master port, one is for SDMA, one is for MCU (ARM core), the third one is for DSP ( currently there is no connection for DSP). Any two master port are used simultaneously, there will be arbitration issue when SPBA clock is not enabled. So I think it is better put SPBA clock in Audio IP, because if there is DSP master port + MCU master used, there will be issue too. (DSP master port is not used in i.MX chip). Anyway I think put it in SDMA driver is not good choice, if SDMA is removed, there will be still issue. Put it in Audio IP maybe more safe.
On Wed, Nov 04, 2015 at 04:34:44PM +0800, Shengjiu Wang wrote: > I discussed this issue with IC team. The SPBA bus has 3 master port, > one is for SDMA, one is for MCU (ARM core), the third one is for DSP ( > currently there is no connection for DSP). Any two master port are used > simultaneously, there will be arbitration issue when SPBA clock is not > enabled. > > So I think it is better put SPBA clock in Audio IP, because if there is > DSP master port + MCU master used, there will be issue too. (DSP master > port is not used in i.MX chip). Anyway I think put it in SDMA driver is > not good choice, if SDMA is removed, there will be still issue. Put it > in Audio IP maybe more safe. It's really ambiguous here as it's not easy to say which side is the best. I'm personally okay if you insist so. However, you need to refine the patch as I suggested in my first reply and document it well: Apparently, "dma" is not a good name for this clock. And it should be an optional property so as not to break the old DTBs. And only the platforms (check DT compatible) that use the arbiter can fetch the clock or get a warning when not present in the DTB. Thanks Nicolin
diff --git a/Documentation/devicetree/bindings/sound/fsl,spdif.txt b/Documentation/devicetree/bindings/sound/fsl,spdif.txt index b5ee32e..9113dc0 100644 --- a/Documentation/devicetree/bindings/sound/fsl,spdif.txt +++ b/Documentation/devicetree/bindings/sound/fsl,spdif.txt @@ -27,6 +27,8 @@ Required properties: Transceiver Clock Diagram" of SoC reference manual. It can also be referred to TxClk_Source bit of register SPDIF_STC. + "dma" The spba clock is needed when sdma share peripheral + script is used. - big-endian : If this property is absent, the native endian mode will be in use as default, or the big endian mode diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c index ab729f2..2f4cb37 100644 --- a/sound/soc/fsl/fsl_spdif.c +++ b/sound/soc/fsl/fsl_spdif.c @@ -106,6 +106,7 @@ struct fsl_spdif_priv { struct clk *rxclk; struct clk *coreclk; struct clk *sysclk; + struct clk *dmaclk; struct snd_dmaengine_dai_dma_data dma_params_tx; struct snd_dmaengine_dai_dma_data dma_params_rx; }; @@ -466,6 +467,12 @@ static int fsl_spdif_startup(struct snd_pcm_substream *substream, return ret; } + ret = clk_prepare_enable(spdif_priv->dmaclk); + if (ret) { + dev_err(&pdev->dev, "failed to enable dma clock\n"); + return ret; + } + ret = spdif_softreset(spdif_priv); if (ret) { dev_err(&pdev->dev, "failed to soft reset\n"); @@ -540,6 +547,7 @@ static void fsl_spdif_shutdown(struct snd_pcm_substream *substream, spdif_intr_status_clear(spdif_priv); regmap_update_bits(regmap, REG_SPDIF_SCR, SCR_LOW_POWER, SCR_LOW_POWER); + clk_disable_unprepare(spdif_priv->dmaclk); clk_disable_unprepare(spdif_priv->coreclk); } } @@ -1221,6 +1229,13 @@ static int fsl_spdif_probe(struct platform_device *pdev) return PTR_ERR(spdif_priv->coreclk); } + /* Get dma clock for dma script operation */ + spdif_priv->dmaclk = devm_clk_get(&pdev->dev, "dma"); + if (IS_ERR(spdif_priv->dmaclk)) { + dev_err(&pdev->dev, "no dma clock in devicetree\n"); + return PTR_ERR(spdif_priv->dmaclk); + } + /* Select clock source for rx/tx clock */ spdif_priv->rxclk = devm_clk_get(&pdev->dev, "rxtx1"); if (IS_ERR(spdif_priv->rxclk)) {
spdif need to enable the spba clock, when sdma is using share peripheral script. If don't enable it, may cause the read/write wrong data from/to registers. Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com> --- Documentation/devicetree/bindings/sound/fsl,spdif.txt | 2 ++ sound/soc/fsl/fsl_spdif.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+)