On Sun, Dec 11, 2022 at 10:41 AM Marek Vasut <marex@xxxxxxx> wrote:
> For SAI to generate MCLK on external SoC pad, the transmitter must be
> enabled as well. With transmitter disabled, no clock are generated.
> Enable the transmitter using the TERE bit.
>
> Rather than doing ad-hoc enablement of the TERE bit, convert the mclk
> handling into a clock provider and adjust the SAI driver points which
> modify the TERE bit to instead enable and disable the clock. Register
> two clock, mclk_tx and mclk_rx, each representing TCSR and RCSR TERE
> bit respectively. Expose only the mclk_tx, since it does not really
> matter which TERE bit is enabled for the generation of output MCLK,
> but it does matter which TERE bit is enabled for internal operation
> of the SAI.
>
> As a nice bonus, this simplifies the MCLK handling in the driver itself.
>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> ---
> Cc: Fabio Estevam <festevam@xxxxxxxxx>
> Cc: Jaroslav Kysela <perex@xxxxxxxx>
> Cc: Liam Girdwood <lgirdwood@xxxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
> Cc: Shengjiu Wang <shengjiu.wang@xxxxxxxxx>
> Cc: Takashi Iwai <tiwai@xxxxxxxx>
> Cc: Xiubo Li <Xiubo.Lee@xxxxxxxxx>
> To: alsa-devel@xxxxxxxxxxxxxxxx
> ---
> sound/soc/fsl/fsl_sai.c | 226 ++++++++++++++++++++++++++++++----------
> sound/soc/fsl/fsl_sai.h | 3 +
> 2 files changed, 176 insertions(+), 53 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index ade5a6b44b66a..944450ba19b62 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -185,34 +185,9 @@ static int fsl_sai_set_dai_bclk_ratio(struct
> snd_soc_dai *dai,
> return 0;
> }
>
> -static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai,
> - int clk_id, unsigned int freq, bool tx)
> +static int fsl_sai_set_dai_sysclk_tr(struct fsl_sai *sai, int clk_id,
> bool tx)
> {
> - struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> - unsigned int ofs = sai->soc_data->reg_offset;
> - u32 val_cr2 = 0;
> -
> - switch (clk_id) {
> - case FSL_SAI_CLK_BUS:
> - val_cr2 |= FSL_SAI_CR2_MSEL_BUS;
> - break;
> - case FSL_SAI_CLK_MAST1:
> - val_cr2 |= FSL_SAI_CR2_MSEL_MCLK1;
> - break;
> - case FSL_SAI_CLK_MAST2:
> - val_cr2 |= FSL_SAI_CR2_MSEL_MCLK2;
> - break;
> - case FSL_SAI_CLK_MAST3:
> - val_cr2 |= FSL_SAI_CR2_MSEL_MCLK3;
> - break;
> - default:
> - return -EINVAL;
> - }
> -
> - regmap_update_bits(sai->regmap, FSL_SAI_xCR2(tx, ofs),
> - FSL_SAI_CR2_MSEL_MASK, val_cr2);
> -
> - return 0;
> + return clk_set_parent(sai->mclk_clk_out[tx],
> sai->mclk_clk[clk_id]);
> }
>
> static int fsl_sai_set_mclk_rate(struct snd_soc_dai *dai, int clk_id,
> unsigned int freq)
> @@ -257,13 +232,13 @@ static int fsl_sai_set_dai_sysclk(struct snd_soc_dai
> *cpu_dai,
> }
> }
>
> - ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq, true);
> + ret = fsl_sai_set_dai_sysclk_tr(sai, clk_id, true);
> if (ret) {
> dev_err(cpu_dai->dev, "Cannot set tx sysclk: %d\n", ret);
> return ret;
> }
>
> - ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq, false);
> + ret = fsl_sai_set_dai_sysclk_tr(sai, clk_id, false);
> if (ret)
> dev_err(cpu_dai->dev, "Cannot set rx sysclk: %d\n", ret);
>
> @@ -487,8 +462,7 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai,
> bool tx, u32 freq)
> else
> return 0;
>
> - regmap_update_bits(sai->regmap, reg, FSL_SAI_CR2_MSEL_MASK,
> - FSL_SAI_CR2_MSEL(sai->mclk_id[tx]));
> + fsl_sai_set_dai_sysclk_tr(sai, sai->mclk_id[tx], tx);
>
> if (savediv == 1)
> regmap_update_bits(sai->regmap, reg,
> @@ -707,32 +681,41 @@ static int fsl_sai_hw_free(struct snd_pcm_substream
> *substream,
> static void fsl_sai_reset(struct fsl_sai *sai, bool rx, bool tx)
> {
> unsigned int ofs = sai->soc_data->reg_offset;
> + bool tx_keep_tere = false, rx_keep_tere = false;
> + u32 tcsr = 0, rcsr = 0;
> +
> + if (sai->mclk_clk_out[0])
> + rx_keep_tere = clk_hw_is_enabled(&(sai->mclk_clk_hw[0]));
> + if (sai->mclk_clk_out[1])
> + tx_keep_tere = clk_hw_is_enabled(&(sai->mclk_clk_hw[1]));
> +
> + if (rx_keep_tere) {
> + regmap_read(sai->regmap, FSL_SAI_RCSR(ofs), &rcsr);
> + rcsr &= FSL_SAI_CSR_TERE;
> + }
> +
> + if (tx_keep_tere) {
> + regmap_read(sai->regmap, FSL_SAI_TCSR(ofs), &tcsr);
> + tcsr &= FSL_SAI_CSR_TERE;
> + }
>
> if (tx)
> - regmap_write(sai->regmap, FSL_SAI_TCSR(ofs),
> FSL_SAI_CSR_SR);
> + regmap_write(sai->regmap, FSL_SAI_TCSR(ofs),
> FSL_SAI_CSR_SR | tcsr);
> if (rx)
> - regmap_write(sai->regmap, FSL_SAI_RCSR(ofs),
> FSL_SAI_CSR_SR);
> + regmap_write(sai->regmap, FSL_SAI_RCSR(ofs),
> FSL_SAI_CSR_SR | rcsr);
> usleep_range(1000, 2000);
>
Not sure if you have test your patch.
there is kernel dump for "scheduling while atomic"
[ 248.778590] BUG: scheduling while atomic: swapper/0/0/0x00010003
[ 248.784618] Modules linked in:
[ 248.787677] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
6.1.0-rc6-00014-gf2b129b4c056-dirty #136
[ 248.796292] Hardware name: NXP i.MX8MNano EVK board (DT)
[ 248.801604] Call trace:
[ 248.804051] dump_backtrace.part.0+0xdc/0xf0
[ 248.808337] show_stack+0x18/0x40
[ 248.811660] dump_stack_lvl+0x64/0x80
[ 248.815327] dump_stack+0x18/0x34
[ 248.818645] __schedule_bug+0x60/0x80
[ 248.822311] __schedule+0x630/0x710
[ 248.825808] schedule+0x5c/0xd0
[ 248.828955] schedule_hrtimeout_range_clock+0xf4/0x134
[ 248.834096] schedule_hrtimeout_range+0x14/0x20
[ 248.838629] usleep_range_state+0x74/0xb0
[ 248.842641] fsl_sai_reset+0x13c/0x1cc
[ 248.846397] fsl_sai_config_disable+0x94/0xb0
[ 248.850760] fsl_sai_trigger+0x208/0x29c
[ 248.854688] snd_soc_pcm_dai_trigger+0xcc/0x26c
[ 248.859223] soc_pcm_trigger+0x164/0x1f4
[ 248.863154] snd_pcm_do_stop+0x68/0x90
> if (tx)
> - regmap_write(sai->regmap, FSL_SAI_TCSR(ofs), 0);
> + regmap_write(sai->regmap, FSL_SAI_TCSR(ofs), tcsr);
> if (rx)
> - regmap_write(sai->regmap, FSL_SAI_RCSR(ofs), 0);
> + regmap_write(sai->regmap, FSL_SAI_RCSR(ofs), rcsr);
> }
>
> static void fsl_sai_config_disable(struct fsl_sai *sai, int dir)
> {
> unsigned int ofs = sai->soc_data->reg_offset;
> bool tx = dir == TX;
> - u32 xcsr, count = 100;
>
> - regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> - FSL_SAI_CSR_TERE, 0);
> -
> - /* TERE will remain set till the end of current frame */
> - do {
> - udelay(10);
> - regmap_read(sai->regmap, FSL_SAI_xCSR(tx, ofs), &xcsr);
> - } while (--count && xcsr & FSL_SAI_CSR_TERE);
> + clk_disable_unprepare(sai->mclk_clk_out[tx]);
>
> regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> FSL_SAI_CSR_FR, FSL_SAI_CSR_FR);
> @@ -780,8 +763,7 @@ static int fsl_sai_trigger(struct snd_pcm_substream
> *substream, int cmd,
> regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
>
> - regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> - FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> + clk_prepare_enable(sai->mclk_clk_out[tx]);
> /*
> * Enable the opposite direction for synchronous mode
> * 1. Tx sync with Rx: only set RE for Rx; set TE & RE for
> Tx
> @@ -794,8 +776,7 @@ static int fsl_sai_trigger(struct snd_pcm_substream
> *substream, int cmd,
> * be safe to do, judging by years of testing results.
> */
> if (fsl_sai_dir_is_synced(sai, adir))
> - regmap_update_bits(sai->regmap,
> FSL_SAI_xCSR((!tx), ofs),
> - FSL_SAI_CSR_TERE,
> FSL_SAI_CSR_TERE);
> + clk_prepare_enable(sai->mclk_clk_out[!tx]);
>
> regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> FSL_SAI_CSR_xIE_MASK, FSL_SAI_FLAGS);
> @@ -1281,6 +1262,86 @@ static int fsl_sai_read_dlcfg(struct fsl_sai *sai)
> return 0;
> }
>
> +static struct fsl_sai *mclk_get_clk_hw_to_sai(struct clk_hw *hw, bool *tx)
> +{
> + *tx = !strcmp(clk_hw_get_name(hw), "mclk_tx");
> +
> + return container_of(hw, struct fsl_sai, mclk_clk_hw[*tx]);
> +}
> +
> +static int mclk_gate_enable(struct clk_hw *hw)
> +{
> + bool tx;
> + struct fsl_sai *sai = mclk_get_clk_hw_to_sai(hw, &tx);
> + unsigned int ofs = sai->soc_data->reg_offset;
> +
> + pm_runtime_get_sync(&sai->pdev->dev);
> +
> + /* Transmitter must be enabled to generate MCLK on pad */
> + regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> + FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
> +
> + return 0;
> +}
> +
> +static void mclk_gate_disable(struct clk_hw *hw)
> +{
> + bool tx;
> + struct fsl_sai *sai = mclk_get_clk_hw_to_sai(hw, &tx);
> + unsigned int ofs = sai->soc_data->reg_offset;
> + u32 xcsr, count = 100;
> +
> + regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> FSL_SAI_CSR_TERE, 0);
> +
> + /* TERE will remain set till the end of current frame */
> + do {
> + udelay(10);
> + regmap_read(sai->regmap, FSL_SAI_xCSR(tx, ofs), &xcsr);
> + } while (--count && xcsr & FSL_SAI_CSR_TERE);
> +
> + pm_runtime_put_sync(&sai->pdev->dev);
> +}
> +
> +static int mclk_gate_is_enabled(struct clk_hw *hw)
> +{
> + bool tx;
> + struct fsl_sai *sai = mclk_get_clk_hw_to_sai(hw, &tx);
> + unsigned int ofs = sai->soc_data->reg_offset;
> + u32 xcsr;
> +
> + regmap_read(sai->regmap, FSL_SAI_xCSR(tx, ofs), &xcsr);
> +
> + return !!(xcsr & FSL_SAI_CSR_TERE);
> +}
> +
> +static u8 mclk_gate_get_parent(struct clk_hw *hw)
> +{
> + bool tx;
> + struct fsl_sai *sai = mclk_get_clk_hw_to_sai(hw, &tx);
> +
> + return sai->mclk_id[tx];
> +}
> +
> +static int mclk_gate_set_parent(struct clk_hw *hw, u8 index)
> +{
> + bool tx;
> + struct fsl_sai *sai = mclk_get_clk_hw_to_sai(hw, &tx);
> + unsigned int ofs = sai->soc_data->reg_offset;
> +
> + regmap_update_bits(sai->regmap, FSL_SAI_xCR2(tx, ofs),
> + FSL_SAI_CR2_MSEL_MASK, FSL_SAI_CR2_MSEL(index));
> +
> + return 0;
> +}
> +
> +const struct clk_ops mclk_gate_ops = {
> + .enable = mclk_gate_enable,
> + .disable = mclk_gate_disable,
> + .is_enabled = mclk_gate_is_enabled,
> + .get_parent = mclk_gate_get_parent,
> + .set_parent = mclk_gate_set_parent,
> +};
> +
> static int fsl_sai_runtime_suspend(struct device *dev);
> static int fsl_sai_runtime_resume(struct device *dev);
>
> @@ -1295,6 +1356,13 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
> int irq, ret, i;
> int index;
> u32 dmas[4];
> + const char *mclk_parent_names[FSL_SAI_MCLK_MAX];
> + const char *mclk_rxtx_names[2] = { "mclk_rx", "mclk_tx" };
>
please consider multi sai instance case, this name will duplicate
that cause sai probe issue.
best regards
wang shengjiu
+ struct clk_init_data mclk_init = {
> + .ops = &mclk_gate_ops,
> + .parent_names = mclk_parent_names,
> + .num_parents = FSL_SAI_MCLK_MAX,
> + };
>
> sai = devm_kzalloc(dev, sizeof(*sai), GFP_KERNEL);
> if (!sai)
> @@ -1341,12 +1409,14 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
> i, PTR_ERR(sai->mclk_clk[i]));
> sai->mclk_clk[i] = NULL;
> }
> + mclk_parent_names[i] = __clk_get_name(sai->mclk_clk[i]);
> }
>
> if (sai->soc_data->mclk0_is_mclk1)
> sai->mclk_clk[0] = sai->mclk_clk[1];
> else
> sai->mclk_clk[0] = sai->bus_clk;
> + mclk_parent_names[0] = __clk_get_name(sai->mclk_clk[0]);
>
> fsl_asoc_get_pll_clocks(&pdev->dev, &sai->pll8k_clk,
> &sai->pll11k_clk);
> @@ -1443,8 +1513,34 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
>
> /* Get sai version */
> ret = fsl_sai_check_version(dev);
> - if (ret < 0)
> + if (ret < 0) {
> dev_warn(dev, "Error reading SAI version: %d\n", ret);
> + goto err_pm_get_sync;
> + }
> +
> + mclk_init.name = mclk_rxtx_names[0];
> + sai->mclk_clk_hw[0].init = &mclk_init;
> + ret = of_clk_hw_register(np, &sai->mclk_clk_hw[0]);
> + if (ret)
> + goto err_pm_get_sync;
> +
> + mclk_init.name = mclk_rxtx_names[1];
> + sai->mclk_clk_hw[1].init = &mclk_init;
> + ret = of_clk_hw_register(np, &sai->mclk_clk_hw[1]);
> + if (ret)
> + goto err_clk_unregister_rx;
> +
> + sai->mclk_clk_out[0] = clk_hw_get_clk(&sai->mclk_clk_hw[0], NULL);
> + if (IS_ERR(sai->mclk_clk_out[0])) {
> + ret = PTR_ERR(sai->mclk_clk_out[0]);
> + goto err_clk_unregister_tx;
> + }
> +
> + sai->mclk_clk_out[1] = clk_hw_get_clk(&sai->mclk_clk_hw[1], NULL);
> + if (IS_ERR(sai->mclk_clk_out[1])) {
> + ret = PTR_ERR(sai->mclk_clk_out[1]);
> + goto err_clk_put_rx;
> + }
>
> /* Select MCLK direction */
> if (of_find_property(np, "fsl,sai-mclk-direction-output", NULL) &&
> @@ -1455,7 +1551,7 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
>
> ret = pm_runtime_put_sync(dev);
> if (ret < 0 && ret != -ENOSYS)
> - goto err_pm_get_sync;
> + goto err_clk_put_tx;
>
> /*
> * Register platform component before registering cpu dai for there
> @@ -1466,21 +1562,39 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
> if (ret) {
> if (!IS_ENABLED(CONFIG_SND_SOC_IMX_PCM_DMA))
> dev_err(dev, "Error: You must enable the
> imx-pcm-dma support!\n");
> - goto err_pm_get_sync;
> + goto err_clk_put_tx;
> }
> } else {
> ret = devm_snd_dmaengine_pcm_register(dev, NULL, 0);
> if (ret)
> - goto err_pm_get_sync;
> + goto err_clk_put_tx;
> }
>
> ret = devm_snd_soc_register_component(dev, &fsl_component,
> &sai->cpu_dai_drv, 1);
> if (ret)
> - goto err_pm_get_sync;
> + goto err_clk_put_tx;
> +
> + /* Expose only the TX clock */
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> + &sai->mclk_clk_hw[1]);
> + if (ret)
> + goto err_clk_put_tx;
>
> return ret;
>
> +err_clk_put_tx:
> + clk_put(sai->mclk_clk_out[1]);
> +
> +err_clk_put_rx:
> + clk_put(sai->mclk_clk_out[0]);
> +
> +err_clk_unregister_tx:
> + clk_hw_unregister(&sai->mclk_clk_hw[1]);
> +
> +err_clk_unregister_rx:
> + clk_hw_unregister(&sai->mclk_clk_hw[0]);
> +
> err_pm_get_sync:
> if (!pm_runtime_status_suspended(dev))
> fsl_sai_runtime_suspend(dev);
> @@ -1492,6 +1606,12 @@ static int fsl_sai_probe(struct platform_device
> *pdev)
>
> static int fsl_sai_remove(struct platform_device *pdev)
> {
> + struct fsl_sai *sai = dev_get_drvdata(&pdev->dev);
> +
> + clk_put(sai->mclk_clk_out[1]);
> + clk_put(sai->mclk_clk_out[0]);
> + clk_hw_unregister(&sai->mclk_clk_hw[1]);
> + clk_hw_unregister(&sai->mclk_clk_hw[0]);
> pm_runtime_disable(&pdev->dev);
> if (!pm_runtime_status_suspended(&pdev->dev))
> fsl_sai_runtime_suspend(&pdev->dev);
> diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
> index 197748a888d5f..f2abf2d6de218 100644
> --- a/sound/soc/fsl/fsl_sai.h
> +++ b/sound/soc/fsl/fsl_sai.h
> @@ -6,6 +6,7 @@
> #ifndef __FSL_SAI_H
> #define __FSL_SAI_H
>
> +#include <linux/clk-provider.h>
> #include <linux/dma/imx-dma.h>
> #include <sound/dmaengine_pcm.h>
>
> @@ -277,6 +278,8 @@ struct fsl_sai {
> struct clk *mclk_clk[FSL_SAI_MCLK_MAX];
> struct clk *pll8k_clk;
> struct clk *pll11k_clk;
> + struct clk_hw mclk_clk_hw[2]; /* MCLK provider */
> + struct clk *mclk_clk_out[2]; /* MCLK output */
> struct resource *res;
>
> bool is_consumer_mode;
> --
> 2.35.1
>
>
