Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller

From: Pierre-Louis Bossart
Date: Fri Aug 20 2021 - 15:02:24 EST

Next message: Utkarsh Verma: "[PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions"
Previous message: Anirudh Rayabharam: "[PATCH v4 2/2] usbip: clean up code in vhci_device_unlink_cleanup"
In reply to: Nicolas Frattaroli: "[PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller"
Next in thread: Nicolas Frattaroli: "Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.c b/sound/soc/rockchip/rockchip_i2s_tdm.c
> new file mode 100644
> index 000000000000..c02b66f3c913
> --- /dev/null
> +++ b/sound/soc/rockchip/rockchip_i2s_tdm.c
> @@ -0,0 +1,1737 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ALSA SoC Audio Layer - Rockchip I2S/TDM Controller driver
> + *
> + * Copyright (c) 2018 Rockchip Electronics Co. Ltd.
> + * Author: Sugar Zhang <sugar.zhang@xxxxxxxxxxxxxx>
> + * Author: Nicolas Frattaroli <frattaroli.nicolas@xxxxxxxxx>
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/delay.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/spinlock.h>
> +#include <sound/pcm_params.h>
> +#include <sound/dmaengine_pcm.h>

alphabetical order?

> +static void rockchip_snd_xfer_clear(struct rk_i2s_tdm_dev *i2s_tdm,
> + unsigned int clr)
> +{
> + unsigned int xfer_mask;
> + unsigned int xfer_val;
> + unsigned int val;
> + int retry = 10;
> + bool tx = clr & I2S_CLR_TXC;
> + bool rx = clr & I2S_CLR_RXC;
> +
> + if (!(rx || tx))
> + return;
> +
> + xfer_mask = (tx ? I2S_XFER_TXS_START : 0) |
> + (rx ? I2S_XFER_RXS_START : 0);
> + xfer_val = (tx ? I2S_XFER_TXS_STOP : 0) |
> + (rx ? I2S_XFER_RXS_STOP : 0);
> +
> + regmap_update_bits(i2s_tdm->regmap, I2S_XFER, xfer_mask, xfer_val);
> + udelay(150);
> + regmap_update_bits(i2s_tdm->regmap, I2S_CLR, clr, clr);
> +
> + regmap_read(i2s_tdm->regmap, I2S_CLR, &val);
> + /* Wait on the clear operation to finish */
> + while (val) {

delay needed here?

> + regmap_read(i2s_tdm->regmap, I2S_CLR, &val);
> + retry--;
> + if (!retry) {
> + dev_warn(i2s_tdm->dev, "clear failed, reset %s%s\n",
> + tx ? "tx" : "", rx ? "rx" : "");
> + if (rx && tx)
> + rockchip_snd_xfer_sync_reset(i2s_tdm);
> + else if (tx)
> + rockchip_snd_reset(i2s_tdm->tx_reset);
> + else if (rx)
> + rockchip_snd_reset(i2s_tdm->rx_reset);
> + break;
> + }
> + }
> +}

> +static int rockchip_i2s_tdm_set_fmt(struct snd_soc_dai *cpu_dai,
> + unsigned int fmt)
> +{
> + struct rk_i2s_tdm_dev *i2s_tdm = to_info(cpu_dai);
> + unsigned int mask = 0, val = 0, tdm_val = 0, txcr_val = 0, rxcr_val = 0;

not all initializations are required. Run e.g. cppcheck.

> + int ret = 0;
> + bool is_tdm = i2s_tdm->tdm_mode;
> +
> + pm_runtime_get_sync(cpu_dai->dev);

test if this worked?

Also if this fails you need to call pm_runtime_put_noidle()

this is what we use.

ret = pm_runtime_get_sync(dev);
if (ret < 0 && ret != -EACCES) {
pm_runtime_put_noidle(dev);
return ret;
}

> + mask = I2S_CKR_MSS_MASK;
> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> + case SND_SOC_DAIFMT_CBS_CFS:
> + val = I2S_CKR_MSS_MASTER;
> + i2s_tdm->is_master_mode = true;
> + break;
> + case SND_SOC_DAIFMT_CBM_CFM:
> + val = I2S_CKR_MSS_SLAVE;
> + i2s_tdm->is_master_mode = false;
> + break;
> + default:
> + ret = -EINVAL;
> + goto err_pm_put;
> + }
> +
> + regmap_update_bits(i2s_tdm->regmap, I2S_CKR, mask, val);
> +
> + mask = I2S_CKR_CKP_MASK | I2S_CKR_TLP_MASK | I2S_CKR_RLP_MASK;
> + switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> + case SND_SOC_DAIFMT_NB_NF:
> + val = I2S_CKR_CKP_NORMAL |
> + I2S_CKR_TLP_NORMAL |
> + I2S_CKR_RLP_NORMAL;
> + break;
> + case SND_SOC_DAIFMT_NB_IF:
> + val = I2S_CKR_CKP_NORMAL |
> + I2S_CKR_TLP_INVERTED |
> + I2S_CKR_RLP_INVERTED;
> + break;
> + case SND_SOC_DAIFMT_IB_NF:
> + val = I2S_CKR_CKP_INVERTED |
> + I2S_CKR_TLP_NORMAL |
> + I2S_CKR_RLP_NORMAL;
> + break;
> + case SND_SOC_DAIFMT_IB_IF:
> + val = I2S_CKR_CKP_INVERTED |
> + I2S_CKR_TLP_INVERTED |
> + I2S_CKR_RLP_INVERTED;
> + break;
> + default:
> + ret = -EINVAL;
> + goto err_pm_put;
> + }
> +
> + regmap_update_bits(i2s_tdm->regmap, I2S_CKR, mask, val);
> +
> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> + case SND_SOC_DAIFMT_RIGHT_J:
> + txcr_val = I2S_TXCR_IBM_RSJM;
> + rxcr_val = I2S_RXCR_IBM_RSJM;
> + break;
> + case SND_SOC_DAIFMT_LEFT_J:
> + txcr_val = I2S_TXCR_IBM_LSJM;
> + rxcr_val = I2S_RXCR_IBM_LSJM;
> + break;
> + case SND_SOC_DAIFMT_I2S:
> + txcr_val = I2S_TXCR_IBM_NORMAL;
> + rxcr_val = I2S_RXCR_IBM_NORMAL;
> + break;
> + case SND_SOC_DAIFMT_DSP_A: /* PCM no delay mode */
> + txcr_val = I2S_TXCR_TFS_PCM;
> + rxcr_val = I2S_RXCR_TFS_PCM;
> + break;
> + case SND_SOC_DAIFMT_DSP_B: /* PCM delay 1 mode */
> + txcr_val = I2S_TXCR_TFS_PCM | I2S_TXCR_PBM_MODE(1);
> + rxcr_val = I2S_RXCR_TFS_PCM | I2S_RXCR_PBM_MODE(1);
> + break;
> + default:
> + ret = -EINVAL;
> + goto err_pm_put;
> + }
> +
> + mask = I2S_TXCR_IBM_MASK | I2S_TXCR_TFS_MASK | I2S_TXCR_PBM_MASK;
> + regmap_update_bits(i2s_tdm->regmap, I2S_TXCR, mask, txcr_val);
> +
> + mask = I2S_RXCR_IBM_MASK | I2S_RXCR_TFS_MASK | I2S_RXCR_PBM_MASK;
> + regmap_update_bits(i2s_tdm->regmap, I2S_RXCR, mask, rxcr_val);
> +
> + if (is_tdm) {
> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> + case SND_SOC_DAIFMT_RIGHT_J:
> + val = I2S_TXCR_TFS_TDM_I2S;
> + tdm_val = TDM_SHIFT_CTRL(2);
> + break;
> + case SND_SOC_DAIFMT_LEFT_J:
> + val = I2S_TXCR_TFS_TDM_I2S;
> + tdm_val = TDM_SHIFT_CTRL(1);
> + break;
> + case SND_SOC_DAIFMT_I2S:
> + val = I2S_TXCR_TFS_TDM_I2S;
> + tdm_val = TDM_SHIFT_CTRL(0);
> + break;
> + case SND_SOC_DAIFMT_DSP_A:
> + val = I2S_TXCR_TFS_TDM_PCM;
> + tdm_val = TDM_SHIFT_CTRL(0);
> + break;
> + case SND_SOC_DAIFMT_DSP_B:
> + val = I2S_TXCR_TFS_TDM_PCM;
> + tdm_val = TDM_SHIFT_CTRL(2);
> + break;
> + default:
> + ret = -EINVAL;
> + goto err_pm_put;
> + }
> +
> + tdm_val |= TDM_FSYNC_WIDTH_SEL1(1);
> + if (i2s_tdm->tdm_fsync_half_frame)
> + tdm_val |= TDM_FSYNC_WIDTH_HALF_FRAME;
> + else
> + tdm_val |= TDM_FSYNC_WIDTH_ONE_FRAME;
> +
> + mask = I2S_TXCR_TFS_MASK;
> + regmap_update_bits(i2s_tdm->regmap, I2S_TXCR, mask, val);
> + regmap_update_bits(i2s_tdm->regmap, I2S_RXCR, mask, val);
> +
> + mask = TDM_FSYNC_WIDTH_SEL1_MSK | TDM_FSYNC_WIDTH_SEL0_MSK |
> + TDM_SHIFT_CTRL_MSK;
> + regmap_update_bits(i2s_tdm->regmap, I2S_TDM_TXCR,
> + mask, tdm_val);
> + regmap_update_bits(i2s_tdm->regmap, I2S_TDM_RXCR,
> + mask, tdm_val);
> + }
> +
> +err_pm_put:
> + pm_runtime_put(cpu_dai->dev);
> +
> + return ret;
> +}
> +

> +static int rockchip_i2s_tdm_clk_set_rate(struct rk_i2s_tdm_dev *i2s_tdm,
> + struct clk *clk, unsigned long rate,
> + int ppm)
> +{
> + unsigned long rate_target;
> + int delta, ret;
> +
> + if (ppm == i2s_tdm->clk_ppm)
> + return 0;
> +
> + delta = (ppm < 0) ? -1 : 1;
> + delta *= (int)div64_u64((u64)rate * (u64)abs(ppm) + 500000,
> + 1000000);

formula looks odd? looks like you are implementing a round to nearest
operation, but that shouldn't require this multiplication?

> +
> + rate_target = rate + delta;
> +
> + if (!rate_target)
> + return -EINVAL;
> +
> + ret = clk_set_rate(clk, rate_target);
> + if (ret)
> + return ret;
> +
> + i2s_tdm->clk_ppm = ppm;
> +
> + return ret;
> +}
> +
> +static int rockchip_i2s_tdm_calibrate_mclk(struct rk_i2s_tdm_dev *i2s_tdm,
> + struct snd_pcm_substream *substream,
> + unsigned int lrck_freq)
> +{
> + struct clk *mclk_root;
> + struct clk *mclk_parent;
> + unsigned int mclk_root_freq;
> + unsigned int mclk_root_initial_freq;
> + unsigned int mclk_parent_freq;
> + unsigned int div, delta;
> + u64 ppm;
> + int ret;
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + mclk_parent = i2s_tdm->mclk_tx_src;
> + else
> + mclk_parent = i2s_tdm->mclk_rx_src;
> +
> + switch (lrck_freq) {
> + case 8000:
> + case 16000:
> + case 24000:
> + case 32000:
> + case 48000:
> + case 64000:
> + case 96000:
> + case 192000:
> + mclk_root = i2s_tdm->mclk_root0;
> + mclk_root_freq = i2s_tdm->mclk_root0_freq;
> + mclk_root_initial_freq = i2s_tdm->mclk_root0_initial_freq;
> + mclk_parent_freq = DEFAULT_MCLK_FS * 192000;
> + break;
> + case 11025:
> + case 22050:
> + case 44100:
> + case 88200:
> + case 176400:
> + mclk_root = i2s_tdm->mclk_root1;
> + mclk_root_freq = i2s_tdm->mclk_root1_freq;
> + mclk_root_initial_freq = i2s_tdm->mclk_root1_initial_freq;
> + mclk_parent_freq = DEFAULT_MCLK_FS * 176400;
> + break;
> + default:
> + dev_err(i2s_tdm->dev, "Invalid LRCK freq: %u Hz\n",
> + lrck_freq);
> + return -EINVAL;
> + }
> +
> + ret = clk_set_parent(mclk_parent, mclk_root);
> + if (ret)
> + return ret;
> +
> + ret = rockchip_i2s_tdm_clk_set_rate(i2s_tdm, mclk_root,
> + mclk_root_freq, 0);
> + if (ret)
> + return ret;
> +
> + delta = abs(mclk_root_freq % mclk_parent_freq - mclk_parent_freq);
> + ppm = div64_u64((uint64_t)delta * 1000000, (uint64_t)mclk_root_freq);
> +
> + if (ppm) {
> + div = DIV_ROUND_CLOSEST(mclk_root_initial_freq, mclk_parent_freq);
> + if (!div)
> + return -EINVAL;
> +
> + mclk_root_freq = mclk_parent_freq * round_up(div, 2);
> +
> + ret = clk_set_rate(mclk_root, mclk_root_freq);
> + if (ret)
> + return ret;
> +
> + i2s_tdm->mclk_root0_freq = clk_get_rate(i2s_tdm->mclk_root0);
> + i2s_tdm->mclk_root1_freq = clk_get_rate(i2s_tdm->mclk_root1);
> + }
> +
> + ret = clk_set_rate(mclk_parent, mclk_parent_freq);
> +
> + return ret;

return clk_set_rate(mclk_parent, mclk_parent_freq); ?

> +}
> +
> +static int rockchip_i2s_tdm_set_mclk(struct rk_i2s_tdm_dev *i2s_tdm,
> + struct snd_pcm_substream *substream,
> + struct clk **mclk)
> +{
> + unsigned int mclk_freq;
> + int ret;
> +
> + if (i2s_tdm->clk_trcm) {
> + if (i2s_tdm->mclk_tx_freq != i2s_tdm->mclk_rx_freq) {
> + dev_err(i2s_tdm->dev,
> + "clk_trcm, tx: %d and rx: %d should be same\n",
> + i2s_tdm->mclk_tx_freq,
> + i2s_tdm->mclk_rx_freq);
> + ret = -EINVAL;
> + return ret;

return -EINVAL?

> + }
> +
> + ret = clk_set_rate(i2s_tdm->mclk_tx, i2s_tdm->mclk_tx_freq);
> + if (ret)
> + return ret;
> +
> + ret = clk_set_rate(i2s_tdm->mclk_rx, i2s_tdm->mclk_rx_freq);
> + if (ret)
> + return ret;
> +
> + /* mclk_rx is also ok. */
> + *mclk = i2s_tdm->mclk_tx;
> + } else {
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + *mclk = i2s_tdm->mclk_tx;
> + mclk_freq = i2s_tdm->mclk_tx_freq;
> + } else {
> + *mclk = i2s_tdm->mclk_rx;
> + mclk_freq = i2s_tdm->mclk_rx_freq;
> + }
> +
> + ret = clk_set_rate(*mclk, mclk_freq);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +

> + pm_runtime_enable(&pdev->dev);
> + if (!pm_runtime_enabled(&pdev->dev)) {
> + ret = i2s_tdm_runtime_resume(&pdev->dev);

that looks like dead code? you've just enabled pm_runtime, why would
this fail?

> + if (ret)
> + goto err_pm_disable;
> + }
> +
> + regmap_update_bits(i2s_tdm->regmap, I2S_DMACR, I2S_DMACR_TDL_MASK,
> + I2S_DMACR_TDL(16));
> + regmap_update_bits(i2s_tdm->regmap, I2S_DMACR, I2S_DMACR_RDL_MASK,
> + I2S_DMACR_RDL(16));
> + regmap_update_bits(i2s_tdm->regmap, I2S_CKR, I2S_CKR_TRCM_MASK,
> + i2s_tdm->clk_trcm << I2S_CKR_TRCM_SHIFT);
> +
> + if (i2s_tdm->soc_data && i2s_tdm->soc_data->init)
> + i2s_tdm->soc_data->init(&pdev->dev, res->start);
> +
> + ret = devm_snd_soc_register_component(&pdev->dev,
> + &rockchip_i2s_tdm_component,
> + &i2s_tdm_dai, 1);
> +
> + if (ret) {
> + dev_err(&pdev->dev, "Could not register DAI\n");
> + goto err_suspend;
> + }
> +
> + ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> + if (ret) {
> + dev_err(&pdev->dev, "Could not register PCM\n");
> + return ret;
> + }
> +
> + return 0;
> +
> +err_suspend:
> + if (!pm_runtime_status_suspended(&pdev->dev))
> + i2s_tdm_runtime_suspend(&pdev->dev);

why is this necessary?

> +err_pm_disable:
> + pm_runtime_disable(&pdev->dev);
> +
> + return ret;
> +}
> +
> +static int rockchip_i2s_tdm_remove(struct platform_device *pdev)
> +{
> + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(&pdev->dev);
> +
> + pm_runtime_disable(&pdev->dev);
> + if (!pm_runtime_status_suspended(&pdev->dev))
> + i2s_tdm_runtime_suspend(&pdev->dev);

this looks backwards, if you disable pm_runtime first what is the
expectation for the rest.
> +
> + if (!IS_ERR(i2s_tdm->mclk_tx))
> + clk_prepare_enable(i2s_tdm->mclk_tx);
> + if (!IS_ERR(i2s_tdm->mclk_rx))
> + clk_prepare_enable(i2s_tdm->mclk_rx);
> + if (!IS_ERR(i2s_tdm->hclk))
> + clk_disable_unprepare(i2s_tdm->hclk);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP

use __maybe_unused

> +static int rockchip_i2s_tdm_suspend(struct device *dev)
> +{
> + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev);
> +
> + regcache_mark_dirty(i2s_tdm->regmap);
> +
> + return 0;
> +}
> +
> +static int rockchip_i2s_tdm_resume(struct device *dev)
> +{
> + struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0)
> + return ret;
> + ret = regcache_sync(i2s_tdm->regmap);
> + pm_runtime_put(dev);
> +
> + return ret;
> +}
> +#endif

Next message: Utkarsh Verma: "[PATCH] USB: serial: iuu_phoenix: Replace symbolic permissions by octal permissions"
Previous message: Anirudh Rayabharam: "[PATCH v4 2/2] usbip: clean up code in vhci_device_unlink_cleanup"
In reply to: Nicolas Frattaroli: "[PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller"
Next in thread: Nicolas Frattaroli: "Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]