[alsa-devel] [PATCH V1 0/3] fsl esai/spdif/asrc need spba clock
fsl esai/spdif/asrc need spba clock
Shengjiu Wang (3): ASoC: fsl_esai: spba clock is needed by esai device ASoC: fsl_spdif: spba clk is needed by spdif device ASoC: fsl_asrc: spba clock is needed by asrc device
Documentation/devicetree/bindings/sound/fsl,asrc.txt | 2 ++ Documentation/devicetree/bindings/sound/fsl,esai.txt | 2 ++ Documentation/devicetree/bindings/sound/fsl,spdif.txt | 2 ++ sound/soc/fsl/fsl_asrc.c | 12 ++++++++++++ sound/soc/fsl/fsl_asrc.h | 1 + sound/soc/fsl/fsl_esai.c | 13 +++++++++++++ sound/soc/fsl/fsl_spdif.c | 15 +++++++++++++++ 7 files changed, 47 insertions(+)
esai 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 register.
Signed-off-by: Shengjiu Wang shengjiu.wang@freescale.com --- Documentation/devicetree/bindings/sound/fsl,esai.txt | 2 ++ sound/soc/fsl/fsl_esai.c | 13 +++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/fsl,esai.txt b/Documentation/devicetree/bindings/sound/fsl,esai.txt index d3b6b5f..d677203 100644 --- a/Documentation/devicetree/bindings/sound/fsl,esai.txt +++ b/Documentation/devicetree/bindings/sound/fsl,esai.txt @@ -27,6 +27,8 @@ Required properties: derive HCK, SCK and FS. "fsys" The system clock derived from ahb clock used to derive HCK, SCK and FS. + "dma" The spba clock is needed when sdma share peripheral + script is used.
- fsl,fifo-depth : The number of elements in the transmit and receive FIFOs. This number is the maximum allowed value for diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index 837979e..4e99818 100644 --- a/sound/soc/fsl/fsl_esai.c +++ b/sound/soc/fsl/fsl_esai.c @@ -54,6 +54,7 @@ struct fsl_esai { struct clk *coreclk; struct clk *extalclk; struct clk *fsysclk; + struct clk *dmaclk; u32 fifo_depth; u32 slot_width; u32 slots; @@ -469,6 +470,9 @@ static int fsl_esai_startup(struct snd_pcm_substream *substream, ret = clk_prepare_enable(esai_priv->coreclk); if (ret) return ret; + ret = clk_prepare_enable(esai_priv->dmaclk); + if (ret) + return ret; if (!IS_ERR(esai_priv->extalclk)) { ret = clk_prepare_enable(esai_priv->extalclk); if (ret) @@ -499,6 +503,7 @@ err_fsysclk: if (!IS_ERR(esai_priv->extalclk)) clk_disable_unprepare(esai_priv->extalclk); err_extalck: + clk_disable_unprepare(esai_priv->dmaclk); clk_disable_unprepare(esai_priv->coreclk);
return ret; @@ -564,6 +569,7 @@ static void fsl_esai_shutdown(struct snd_pcm_substream *substream, clk_disable_unprepare(esai_priv->fsysclk); if (!IS_ERR(esai_priv->extalclk)) clk_disable_unprepare(esai_priv->extalclk); + clk_disable_unprepare(esai_priv->dmaclk); clk_disable_unprepare(esai_priv->coreclk); }
@@ -772,6 +778,13 @@ static int fsl_esai_probe(struct platform_device *pdev) dev_warn(&pdev->dev, "failed to get fsys clock: %ld\n", PTR_ERR(esai_priv->fsysclk));
+ esai_priv->dmaclk = devm_clk_get(&pdev->dev, "dma"); + if (IS_ERR(esai_priv->dmaclk)) { + dev_err(&pdev->dev, "Cannot get dma clock: %ld\n", + PTR_ERR(esai_priv->dmaclk)); + return PTR_ERR(esai_priv->dmaclk); + } + irq = platform_get_irq(pdev, 0); if (irq < 0) { dev_err(&pdev->dev, "no irq for node %s\n", pdev->name);
Hi,
On Thu, Sep 17, 2015 at 02:09:23PM +0800, Shengjiu Wang wrote:
esai 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 register.
It would be nicer to elaborate the SPBA as well, especially the reason why it "may cause the read/write wrong data from/to register".
Signed-off-by: Shengjiu Wang shengjiu.wang@freescale.com
diff --git a/Documentation/devicetree/bindings/sound/fsl,esai.txt b/Documentation/devicetree/bindings/sound/fsl,esai.txt index d3b6b5f..d677203 100644 --- a/Documentation/devicetree/bindings/sound/fsl,esai.txt +++ b/Documentation/devicetree/bindings/sound/fsl,esai.txt @@ -27,6 +27,8 @@ Required properties: derive HCK, SCK and FS. "fsys" The system clock derived from ahb clock used to derive HCK, SCK and FS.
- "dma" The spba clock is needed when sdma share peripheral
script is used.
It would be better to treat it as an optional property as ESAI itself doesn't require it to be enabled while a specific SDMA script does so as to access the registers successfully via the arbiter. There is one of existing SoCs that doesn't have SDMA and SPBA at all so they don't need it.
@@ -469,6 +470,9 @@ static int fsl_esai_startup(struct snd_pcm_substream *substream, ret = clk_prepare_enable(esai_priv->coreclk); if (ret) return ret;
- ret = clk_prepare_enable(esai_priv->dmaclk);
- if (ret)
return ret;
Needs a goto for a failed case in order to disable the coreclk.
@@ -772,6 +778,13 @@ static int fsl_esai_probe(struct platform_device *pdev) dev_warn(&pdev->dev, "failed to get fsys clock: %ld\n", PTR_ERR(esai_priv->fsysclk));
- esai_priv->dmaclk = devm_clk_get(&pdev->dev, "dma");
- if (IS_ERR(esai_priv->dmaclk)) {
dev_err(&pdev->dev, "Cannot get dma clock: %ld\n",PTR_ERR(esai_priv->dmaclk));return PTR_ERR(esai_priv->dmaclk);- }
This will break the old DTB -- would be solved by making the clock optional though. And it would be better to follow the pattern of previous error message.
Nicolin
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(+)
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)) {
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/...
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/...
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/...
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
asrc 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 register.
Signed-off-by: Shengjiu Wang shengjiu.wang@freescale.com --- Documentation/devicetree/bindings/sound/fsl,asrc.txt | 2 ++ sound/soc/fsl/fsl_asrc.c | 12 ++++++++++++ sound/soc/fsl/fsl_asrc.h | 1 + 3 files changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt b/Documentation/devicetree/bindings/sound/fsl,asrc.txt index b93362a..90a8cd6 100644 --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt @@ -25,6 +25,8 @@ Required properties: "mem" Peripheral access clock to access registers. "ipg" Peripheral clock to driver module. "asrck_<0-f>" Clock sources for input and output clock. + "dma" The spba clock is needed when sdma share peripheral + script is used.
- big-endian : If this property is absent, the little endian mode will be in use as default. Otherwise, the big endian diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index 9f087d4..5ec1eae 100644 --- a/sound/soc/fsl/fsl_asrc.c +++ b/sound/soc/fsl/fsl_asrc.c @@ -859,6 +859,12 @@ static int fsl_asrc_probe(struct platform_device *pdev) return PTR_ERR(asrc_priv->ipg_clk); }
+ asrc_priv->dma_clk = devm_clk_get(&pdev->dev, "dma"); + if (IS_ERR(asrc_priv->dma_clk)) { + dev_err(&pdev->dev, "failed to get dma script clock\n"); + return PTR_ERR(asrc_priv->dma_clk); + } + for (i = 0; i < ASRC_CLK_MAX_NUM; i++) { sprintf(tmp, "asrck_%x", i); asrc_priv->asrck_clk[i] = devm_clk_get(&pdev->dev, tmp); @@ -939,6 +945,9 @@ static int fsl_asrc_runtime_resume(struct device *dev) ret = clk_prepare_enable(asrc_priv->ipg_clk); if (ret) goto disable_mem_clk; + ret = clk_prepare_enable(asrc_priv->dma_clk); + if (ret) + goto disable_ipg_clk; for (i = 0; i < ASRC_CLK_MAX_NUM; i++) { ret = clk_prepare_enable(asrc_priv->asrck_clk[i]); if (ret) @@ -950,6 +959,8 @@ static int fsl_asrc_runtime_resume(struct device *dev) disable_asrck_clk: for (i--; i >= 0; i--) clk_disable_unprepare(asrc_priv->asrck_clk[i]); + clk_disable_unprepare(asrc_priv->dma_clk); +disable_ipg_clk: clk_disable_unprepare(asrc_priv->ipg_clk); disable_mem_clk: clk_disable_unprepare(asrc_priv->mem_clk); @@ -963,6 +974,7 @@ static int fsl_asrc_runtime_suspend(struct device *dev)
for (i = 0; i < ASRC_CLK_MAX_NUM; i++) clk_disable_unprepare(asrc_priv->asrck_clk[i]); + clk_disable_unprepare(asrc_priv->dma_clk); clk_disable_unprepare(asrc_priv->ipg_clk); clk_disable_unprepare(asrc_priv->mem_clk);
diff --git a/sound/soc/fsl/fsl_asrc.h b/sound/soc/fsl/fsl_asrc.h index 4aed63c..1bb13c2 100644 --- a/sound/soc/fsl/fsl_asrc.h +++ b/sound/soc/fsl/fsl_asrc.h @@ -442,6 +442,7 @@ struct fsl_asrc { unsigned long paddr; struct clk *mem_clk; struct clk *ipg_clk; + struct clk *dma_clk; struct clk *asrck_clk[ASRC_CLK_MAX_NUM]; spinlock_t lock;
participants (4)
-
Fabio Estevam -
Mark Brown -
Nicolin Chen -
Shengjiu Wang