Re: [PATCH v3 4/4] ASoC: cdns: Add Cadence I2S-MC controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] sound: cdns: clock refcount leak on late probe error and driver unbind
- [High] sound: cdns: inverted clock master/slave mode evaluation in hw_params
- [High] sound: cdns: inverted BCLK data delay for I2S and Left-Justified formats
- [High] sound: cdns: read-modify-write race on volatile interrupt status register
- [Medium] sound: cdns: synchronizing unit resets dropped due to incomplete regmap mask
- [Medium] sound: cdns: unprotected shared clock parent modification across DAIs
- [Medium] cdns: premature FIFO reset deassertion over APB bus
--

commit ba70a70c0f6376ecabc6aa01d7c62b130cb6e3a9
Author: Joakim Zhang <joakim.zhang@cixtech.com>

ASoC: cdns: Add Cadence I2S-MC controller driver

Add support for the Cadence I2S-MC multi-channel controller found in
the CIX SKY1 audio subsystem.

> diff --git a/sound/soc/cdns/cdns-i2s-mc.c b/sound/soc/cdns/cdns-i2s-mc.c
> new file mode 100644
> index 0000000000000..e9a0880a06167
> --- /dev/null
> +++ b/sound/soc/cdns/cdns-i2s-mc.c

[ ... ]

> +static irqreturn_t cdns_i2s_mc_isr(int irq, void *devid)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = (struct cdns_i2s_mc_priv *)devid;
> +	struct device *dev = i2s_mc_priv->dev;
> +	u32 stat;
> +
> +	if (regmap_read(i2s_mc_priv->regmap, I2S_INTR_STAT, &stat))
> +		return IRQ_NONE;
> +
> +	stat &= I2S_INTR_STAT_TDATA_UNDERR | I2S_INTR_STAT_RDATA_OVERR;
> +	if (!stat)
> +		return IRQ_NONE;
> +
> +	/* Clear the status */
> +	regmap_update_bits(i2s_mc_priv->regmap, I2S_INTR_STAT, stat, 0);

[Severity: High]
Does this read-modify-write cycle introduce a hardware race condition on the
volatile interrupt status register?

The I2S_INTR_STAT register is asynchronously updated by the hardware to flag
events. When regmap_update_bits() performs its read-modify-write cycle, any
new hardware interrupts that assert between the read and the write could
be inadvertently cleared without being handled (if W1C) or overwritten with
0 and lost (if W0C or R/W).

Would it be safer to use a direct regmap_write() here?

[ ... ]

> +static int cdns_i2s_mc_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	struct device *dev = i2s_mc_priv->dev;
> +	u32 ctrl = 0, dev_conf = 0;

[ ... ]

> +		/* DAI hardware audio formats */
> +		switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +		case SND_SOC_DAIFMT_I2S:
> +			dev_conf |= I2S_DEV_CONF_TRAN_I2S_ALIGN_LR;
> +			dev_conf &= ~(I2S_DEV_CONF_TRAN_APB_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_DATA_WS_DEL |
> +				      I2S_DEV_CONF_TRAN_WS_DSP_MODE);
> +			break;
> +		case SND_SOC_DAIFMT_RIGHT_J:
> +			dev_conf |= I2S_DEV_CONF_TRAN_DATA_WS_DEL;
> +			dev_conf &= ~(I2S_DEV_CONF_TRAN_I2S_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_APB_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_WS_DSP_MODE);
> +			break;
> +		case SND_SOC_DAIFMT_LEFT_J:
> +			dev_conf |= I2S_DEV_CONF_TRAN_I2S_ALIGN_LR | I2S_DEV_CONF_TRAN_DATA_WS_DEL;
> +			dev_conf &= ~(I2S_DEV_CONF_TRAN_APB_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_WS_DSP_MODE);
> +			break;

[Severity: High]
Are the BCLK data delays inverted for I2S and Left-Justified formats?

Standard I2S requires a 1 BCLK delay for the data relative to the WS signal,
while Left-Justified requires a 0 BCLK delay.

The I2S_DEV_CONF_TRAN_DATA_WS_DEL bit seems to configure this 1 BCLK delay
(as it is explicitly set for DSP_A and cleared for DSP_B).

However, this code clears this bit for SND_SOC_DAIFMT_I2S (removing the
required delay) and sets it for SND_SOC_DAIFMT_LEFT_J (adding an incorrect
delay), which might cause 1-bit audio misalignment.

[ ... ]

> +static int cdns_i2s_mc_prepare(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *cpu_dai)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	u32 ctrl = 0, fifo = 0;

[ ... ]

> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		/* Transmitter clock enable */
> +		regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> +				   I2S_CID_CTRL_STROBE_TS, 0);
> +
> +		/*
> +		 * Transmitter FIFO reset
> +		 * When '0', transmit FIFO pointers are reset to zero. Threshold level for
> +		 * this FIFO is unchanged. This bit is automatically set to '1' after one
> +		 * clock cycle if TX FIFO reset has been acknowledged.
> +		 * Deassert then assert this bit here, since I2S_CTRL register is not
> +		 * volatile, would not read from hardware any longer. If not, it would
> +		 * clear tx fifo every time when write this register.
> +		 */
> +		ctrl &= ~I2S_CTRL_TFIFO_RST;
> +		regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> +				   I2S_CTRL_TFIFO_RST, ctrl);
> +		ctrl |= I2S_CTRL_TFIFO_RST;
> +		regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> +				   I2S_CTRL_TFIFO_RST, ctrl);

[ ... ]

> +	} else {
> +		/* Receiver clock enable */
> +		regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> +				   I2S_CID_CTRL_STROBE_RS, 0);
> +
> +		/*
> +		 * Receiver FIFO reset
> +		 * When '0', receive FIFO pointers are reset to zero. Threshold level for
> +		 * this FIFO is unchanged. This bit is automatically set to '1' after one
> +		 * clock cycle if RX FIFO reset has been acknowledged.
> +		 * Deassert then assert this bit here, since I2S_CTRL register is not
> +		 * volatile, would not read from hardware any longer. If not, it would
> +		 * clear rx fifo every time when write this register.
> +		 */
> +		ctrl &= ~I2S_CTRL_RFIFO_RST;
> +		regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> +				   I2S_CTRL_RFIFO_RST, ctrl);
> +		ctrl |= I2S_CTRL_RFIFO_RST;
> +		regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> +				   I2S_CTRL_RFIFO_RST, ctrl);

[Severity: Medium]
Will these back-to-back writes deassert the FIFO resets too quickly?

The hardware requires the reset bit to be held for at least 1 I2S functional
clock cycle before it auto-acknowledges and sets it to 1. Since these writes
go over the fast APB bus, they might deassert the reset in tens of nanoseconds,
and the slower clock domain crossing synchronizer could miss this pulse,
aborting the reset.

Should there be a delay here, or could the cache be manipulated exclusively
using regcache_cache_only() instead?

[ ... ]

> +static int cdns_i2s_mc_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *cpu_dai)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	struct device *dev = i2s_mc_priv->dev;
> +	u32 rate, sample_rate = 0;
> +	u32 slots, slot_width, resolution, ctrl;
> +	unsigned long i2s_clk_rate;
> +	u8 pin_tx_num, pin_rx_num;
> +	struct clk *clk_parent;
> +	bool is_master_mode;
> +	int ret;
> +
> +	rate = params_rate(params);
> +	slot_width = i2s_mc_priv->devtype_data->data_width;
> +	dev_dbg(dev, "rate = %d, slot_width = %d\n", rate, slot_width);
> +
> +	/* switch clk mux to select the appropriate clk parent */
> +	if (rate % 8000 == 0) {
> +		/* Sampling rate is a multiple of 8KHz, select "audio-clk0" */
> +		clk_parent = i2s_mc_priv->clks[AUDIO_CLK0];
> +
> +		ret = clk_set_parent(i2s_mc_priv->clk_i2s, clk_parent);
> +	} else if (rate % 11025 == 0) {
> +		/* Sampling rate is a multiple of 11.025KHz, select "audio-clk2" */
> +		clk_parent = i2s_mc_priv->clks[AUDIO_CLK2];
> +
> +		ret = clk_set_parent(i2s_mc_priv->clk_i2s, clk_parent);

[Severity: Medium]
Does this unconditionally change the shared clock parent without protecting
against active streams on the other DAI?

Since there are two independent DAIs (I2S_MC_AIF1 and I2S_MC_AIF2), starting
a new stream on one DAI with a different sample rate will silently change
the parent of the shared clk_i2s clock.

Could this break the clock rate and corrupt the audio of a concurrently
running stream on the other DAI?

> +	} else {
> +		dev_err(i2s_mc_priv->dev, "Invalid sample rate\n");
> +		return -EINVAL;
> +	}
> +	if (ret) {
> +		dev_err(i2s_mc_priv->dev, "Failed to set i2s clock parent\n");
> +		return ret;
> +	}
> +
> +	i2s_clk_rate = clk_get_rate(i2s_mc_priv->clk_i2s);
> +	dev_dbg(dev, "i2s clk rate = %lu\n", i2s_clk_rate);
> +
> +	regmap_read(i2s_mc_priv->regmap, I2S_CTRL, &ctrl);
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		is_master_mode = ctrl & I2S_CTRL_T_MS;
> +	else
> +		is_master_mode = ctrl & I2S_CTRL_R_MS;
> +
> +	if (is_master_mode) {

[Severity: High]
Is the clock master/slave mode evaluation inverted here?

In cdns_i2s_mc_set_fmt(), I2S_CTRL_T_MS is cleared when the CPU is master
(SND_SOC_DAIFMT_CBC_CFC) and set when the CPU is slave (SND_SOC_DAIFMT_CBP_CFP).

However, the logic here evaluates is_master_mode as true when the CPU is
a slave, and false when the CPU is a master.

This causes the driver to incorrectly calculate and program the clock divider
when acting as a slave, and skip computing the divider entirely when acting
as a master.

[ ... ]

> +static int cdns_i2s_mc_dai_probe(struct snd_soc_dai *cpu_dai)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	u32 ctrl = 0, cid_ctrl = 0;
> +
> +	/*
> +	 * Transceiver disable
> +	 * Transceiver clock disable
> +	 * All interrupts masked
> +	 * Reset for transmitter synchronizing unit
> +	 * Reset for receiver synchronizing unit
> +	 */
> +	ctrl |= FIELD_PREP(I2S_CTRL_I2S_EN, 0);
> +	ctrl &= ~(I2S_CTRL_TSYNC_RST | I2S_CTRL_RSYNC_RST);
> +	cid_ctrl |= I2S_CID_CTRL_STROBE_TS | I2S_CID_CTRL_STROBE_RS;
> +	cid_ctrl &= ~I2S_CID_CTRL_INTREQ_MASK;
> +
> +	regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL, I2S_CTRL_I2S_EN, ctrl);

[Severity: Medium]
Will this restrictive mask prevent the synchronizing unit resets from reaching
the hardware?

The driver clears I2S_CTRL_TSYNC_RST and I2S_CTRL_RSYNC_RST (bits 25 and 26)
in the ctrl variable, but issues the write using only I2S_CTRL_I2S_EN
(bits 0-7) as the mask. The intended hardware modifications to bits 25 and 26
will be silently masked out.

[ ... ]

> +static int cdns_i2s_mc_probe(struct platform_device *pdev)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv;

[ ... ]

> +	/*
> +	 * Let pm_runtime_put_sync() disable the clocks, and take regcache effect.
> +	 * If CONFIG_PM is not enabled, the clock will stay powered.
> +	 */
> +	pm_runtime_put_sync(&pdev->dev);
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev, &cdns_i2s_mc_component,
> +					      cdns_i2s_mc_dai, ARRAY_SIZE(cdns_i2s_mc_dai));
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to register soc component\n");

[Severity: High]
Does returning directly here bypass the err_disable_clks cleanup path?

The clocks clk_hst and clk_i2s are manually enabled earlier in this function
via cdns_i2s_mc_clks_enable(). Returning directly on failure will leak the
clock prepare/enable reference counts.

Should this jump to err_disable_clks instead?

[ ... ]

> +static const struct of_device_id cdns_i2s_mc_of_match[] = {
> +	{ .compatible = "cix,sky1-i2s-mc", .data = &sky1_devtype_data },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, cdns_i2s_mc_of_match);
> +
> +static struct platform_driver cdns_i2s_mc_driver = {
> +	.probe = cdns_i2s_mc_probe,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.pm = pm_ptr(&cdns_i2s_mc_pm_ops),
> +		.of_match_table = cdns_i2s_mc_of_match,
> +	},
> +};

[Severity: High]
Does this driver lack a .remove callback to clean up the clocks?

Because the clocks are manually enabled in probe and there is no devm action
registered to disable them, the clock references will be permanently leaked
when the module is unbound or removed.

Could this leave the hardware clocks physically powered if CONFIG_PM is
disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009092552.2377960-1-joakim.zhang@cixtech.com?part=4