fsl esai/spdif/asrc need spba clock
Subsystems (4)
cover 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(+)
Signature · 1 line
1.9.1
PATCH 1/3 ASoC: fsl_esai: spba clock is needed by esai device
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.
--- Documentation/devicetree/bindings/sound/fsl,esai.txt | 2 ++ sound/soc/fsl/fsl_esai.c | 13 +++++++++++++ 2 files changed, 15 insertions(+)
Documentation/devicetree/bindings/sound/fsl,esai.txt +2 -0
@@ -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
sound/soc/fsl/fsl_esai.c +13 -0
@@ -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);
Signature · 1 line
1.9.1
Nicolin Chen
reviewerRe: [PATCH V1 1/3] ASoC: fsl_esai: spba clock is needed by esai device
Hi,
Quoted 3 lines · Shengjiu Wang
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".
Documentation/devicetree/bindings/sound/fsl,esai.txt Required properties: · line 31
Quoted 12 lines
Signed-off-by Shengjiu Wang <[email protected]>Documentation/devicetree/bindings/sound/fsl,esai.txt +2
-0@@ -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.
Documentation/devicetree/bindings/sound/fsl,esai.txt static int fsl_esai_startup(struct snd_pcm_substream *substream, · line 475
Quoted 7 lines
Documentation/devicetree/bindings/sound/fsl,esai.txt +3
-0@@ -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.
Documentation/devicetree/bindings/sound/fsl,esai.txt static int fsl_esai_probe(struct platform_device *pdev) · line 786
Quoted 10 lines
Documentation/devicetree/bindings/sound/fsl,esai.txt +6
-0@@ -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
PATCH 2/3 ASoC: fsl_spdif: spba clk is needed by spdif device
In reply to Shengjiu Wangspdif 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.
--- Documentation/devicetree/bindings/sound/fsl,spdif.txt | 2 ++ sound/soc/fsl/fsl_spdif.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+)
Documentation/devicetree/bindings/sound/fsl,spdif.txt +2 -0
@@ -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
sound/soc/fsl/fsl_spdif.c +15 -0
@@ -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)) {
Signature · 1 line
1.9.1
Mark Brown
maintainerRe: [PATCH V1 2/3] ASoC: fsl_spdif: spba clk is needed by spdif device
Documentation/devicetree/bindings/sound/fsl,spdif.txt Required properties: · line 31
Quoted 2 lines · Shengjiu Wang
Documentation/devicetree/bindings/sound/fsl,spdif.txt +2
-0+ "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?
Nicolin Chen
reviewerRe: [PATCH V1 2/3] ASoC: fsl_spdif: spba clk is needed by spdif device
Quoted 7 lines · Mark Brown
Documentation/devicetree/bindings/sound/fsl,spdif.txt Required properties: · line 31
Quoted 2 lines · Shengjiu Wang
Documentation/devicetree/bindings/sound/fsl,spdif.txt +2
-0+ "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.
Mark Brown
maintainerRe: [PATCH V1 2/3] ASoC: fsl_spdif: spba clk is needed by spdif device
Quoted 9 lines · Nicolin Chen
Quoted 7 lines · Mark Brown
Documentation/devicetree/bindings/sound/fsl,spdif.txt Required properties: · line 31
Quoted 2 lines · Shengjiu Wang
Documentation/devicetree/bindings/sound/fsl,spdif.txt +2
-0+ "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?
Quoted 4 lines
I agree but it doesn't seems to be easy to enable and disable it dynamically along with ASoC
open()andclose()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?
Quoted 3 lines
Maybe we can put in the
issue_pending()andterminate_all()which are regularly called along with ASoCtrigger(), 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.
Re: [PATCH V1 2/3] ASoC: fsl_spdif: spba clk is needed by spdif device
Quoted 18 lines · Mark Brown
Quoted 9 lines · Nicolin Chen
Quoted 7 lines · Mark Brown
Documentation/devicetree/bindings/sound/fsl,spdif.txt Required properties: · line 31
Quoted 2 lines · Shengjiu Wang
Documentation/devicetree/bindings/sound/fsl,spdif.txt +2
-0+ "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?
Quoted 4 lines
I agree but it doesn't seems to be easy to enable and disable it dynamically along with ASoC
open()andclose()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()andclose()or wouldn't doing things along with the DMA operatiosn suffice?
Quoted 8 lines
Quoted 3 lines
Maybe we can put in the
issue_pending()andterminate_all()which are regularly called along with ASoCtrigger(), 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
Fabio Estevam
reviewerRe: [PATCH V1 2/3] ASoC: fsl_spdif: spba clk is needed by spdif device
Quoted 2 lines · Shengjiu Wang
I think spba clock should be used by the audio IP not DMA, because in same situation the
FSL_SSIdoesn'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:
Nicolin Chen
reviewerRe: [PATCH V1 2/3] ASoC: fsl_spdif: spba clk is needed by spdif device
Quoted 10 lines · Fabio Estevam
Quoted 2 lines · Shengjiu Wang
I think spba clock should be used by the audio IP not DMA, because in same situation the
FSL_SSIdoesn'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:
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.
Re: [PATCH V1 2/3] ASoC: fsl_spdif: spba clk is needed by spdif device
Quoted 17 lines · Nicolin Chen
Quoted 10 lines · Fabio Estevam
Quoted 2 lines · Shengjiu Wang
I think spba clock should be used by the audio IP not DMA, because in same situation the
FSL_SSIdoesn'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:
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.
Quoted 8 lines
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.
Nicolin Chen
reviewerRe: [PATCH V1 2/3] ASoC: fsl_spdif: spba clk is needed by spdif device
Quoted 11 lines · Shengjiu Wang
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
PATCH 3/3 ASoC: fsl_asrc: spba clock is needed by asrc device
In reply to Shengjiu Wangasrc 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.
--- 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(+)
Documentation/devicetree/bindings/sound/fsl,asrc.txt +2 -0
@@ -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
sound/soc/fsl/fsl_asrc.c +12 -0
@@ -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);
sound/soc/fsl/fsl_asrc.h +1 -0
@@ -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;
Signature · 1 line
1.9.1