[PATCH 0/5] ASoC: Xilinx fixes

[PATCH 0/5] ASoC: Xilinx fixes

[Robert Hancock]

There are drivers in mainline for the Xilinx Audio Formatter and Xilinx
I2S IP cores. However, because of a few issues, these were only really
usable with Xilinx's xlnx_pl_snd_card top-level driver, which is not in
mainline (and not suitable for mainline).

The fixes in this patchset, for the simple-card layer as well as the 
Xilinx drivers, now allow these drivers to be properly used with
simple-card without any out-of-tree support code.

Robert Hancock (5):
  ASoC: xilinx: xlnx_formatter_pcm: Make buffer bytes multiple of period
    bytes
  ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting
  ASoC: xilinx: xlnx_i2s.c: Handle sysclk setting
  ASoC: simple-card: fix probe failure on platform component
  ASoC: simple-card-utils: Set sysclk on all components

 sound/soc/generic/simple-card-utils.c |  11 +++
 sound/soc/generic/simple-card.c       |  52 +++++++------
 sound/soc/xilinx/xlnx_formatter_pcm.c |  44 ++++++++++-
 sound/soc/xilinx/xlnx_i2s.c           | 104 +++++++++++++++++---------
 4 files changed, 150 insertions(+), 61 deletions(-)

-- 
2.31.1

[PATCH 1/5] ASoC: xilinx: xlnx_formatter_pcm: Make buffer bytes multiple of period bytes

[Robert Hancock]

This patch is based on one in the Xilinx kernel tree, "ASoc: xlnx: Make
buffer bytes multiple of period bytes" by Devarsh Thakkar. The same
issue exists in the mainline version of the driver. The original
patch description is as follows:

"The Xilinx Audio Formatter IP has a constraint on period
bytes to be multiple of 64. This leads to driver changing
the period size to suitable frames such that period bytes
are multiple of 64.

Now since period bytes and period size are updated but not
the buffer bytes, this may make the buffer bytes unaligned
and not multiple of period bytes.

When this happens we hear popping noise as while DMA is being
done the buffer bytes are not enough to complete DMA access
for last period of frame within the application buffer boundary.

To avoid this, align buffer bytes too as multiple of 64, and
set another constraint to always enforce number of periods as
integer. Now since, there is already a rule in alsa core
to enforce Buffer size = Number of Periods * Period Size
this automatically aligns buffer bytes as multiple of period
bytes."

Fixes: 6f6c3c36f091 ("ASoC: xlnx: add pcm formatter platform driver")
Cc: Devarsh Thakkar <devarsh.thakkar@xilinx.com>
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 sound/soc/xilinx/xlnx_formatter_pcm.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_formatter_pcm.c b/sound/soc/xilinx/xlnx_formatter_pcm.c
index 91afea9d5de6..db22e25cf3f8 100644
--- a/sound/soc/xilinx/xlnx_formatter_pcm.c
+++ b/sound/soc/xilinx/xlnx_formatter_pcm.c
@@ -37,6 +37,7 @@
 #define XLNX_AUD_XFER_COUNT	0x28
 #define XLNX_AUD_CH_STS_START	0x2C
 #define XLNX_BYTES_PER_CH	0x44
+#define XLNX_AUD_ALIGN_BYTES	64
 
 #define AUD_STS_IOC_IRQ_MASK	BIT(31)
 #define AUD_STS_CH_STS_MASK	BIT(29)
@@ -368,12 +369,30 @@ static int xlnx_formatter_pcm_open(struct snd_soc_component *component,
 	snd_soc_set_runtime_hwparams(substream, &xlnx_pcm_hardware);
 	runtime->private_data = stream_data;
 
-	/* Resize the period size divisible by 64 */
+	/* Resize the period bytes as divisible by 64 */
 	err = snd_pcm_hw_constraint_step(runtime, 0,
-					 SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64);
+					 SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
+					 XLNX_AUD_ALIGN_BYTES);
 	if (err) {
 		dev_err(component->dev,
-			"unable to set constraint on period bytes\n");
+			"Unable to set constraint on period bytes\n");
+		return err;
+	}
+	/* Resize the buffer bytes as divisible by 64 */
+	err = snd_pcm_hw_constraint_step(runtime, 0,
+					 SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
+					 XLNX_AUD_ALIGN_BYTES);
+	if (err) {
+		dev_err(component->dev,
+			"Unable to set constraint on buffer bytes\n");
+		return err;
+	}
+	/* Set periods as integer multiple */
+	err = snd_pcm_hw_constraint_integer(runtime,
+					    SNDRV_PCM_HW_PARAM_PERIODS);
+	if (err < 0) {
+		dev_err(component->dev,
+			"Unable to set constraint on periods to be integer\n");
 		return err;
 	}
 
-- 
2.31.1

[PATCH 2/5] ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting

[Robert Hancock]

This driver did not set the MM2S Fs Multiplier Register to the proper
value for playback streams. This needs to be set to the sample rate to
MCLK multiplier, or random stream underflows can occur on the downstream
I2S transmitter.

Store the sysclk value provided via the set_sysclk callback and use that
in conjunction with the sample rate in the hw_params callback to calculate
the proper value to set for this register.

Fixes: 6f6c3c36f091 ("ASoC: xlnx: add pcm formatter platform driver")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 sound/soc/xilinx/xlnx_formatter_pcm.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/sound/soc/xilinx/xlnx_formatter_pcm.c b/sound/soc/xilinx/xlnx_formatter_pcm.c
index db22e25cf3f8..d35838cf5302 100644
--- a/sound/soc/xilinx/xlnx_formatter_pcm.c
+++ b/sound/soc/xilinx/xlnx_formatter_pcm.c
@@ -84,6 +84,7 @@ struct xlnx_pcm_drv_data {
 	struct snd_pcm_substream *play_stream;
 	struct snd_pcm_substream *capture_stream;
 	struct clk *axi_clk;
+	unsigned int last_sysclk;
 };
 
 /*
@@ -314,6 +315,15 @@ static irqreturn_t xlnx_s2mm_irq_handler(int irq, void *arg)
 	return IRQ_NONE;
 }
 
+static int xlnx_formatter_set_sysclk(struct snd_soc_component *component,
+				     int clk_id, int source, unsigned int freq, int dir)
+{
+	struct xlnx_pcm_drv_data *adata = dev_get_drvdata(component->dev);
+
+	adata->last_sysclk = freq;
+	return 0;
+}
+
 static int xlnx_formatter_pcm_open(struct snd_soc_component *component,
 				   struct snd_pcm_substream *substream)
 {
@@ -448,11 +458,19 @@ static int xlnx_formatter_pcm_hw_params(struct snd_soc_component *component,
 	u64 size;
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct xlnx_pcm_stream_param *stream_data = runtime->private_data;
+	struct xlnx_pcm_drv_data *adata = dev_get_drvdata(component->dev);
 
 	active_ch = params_channels(params);
 	if (active_ch > stream_data->ch_limit)
 		return -EINVAL;
 
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
+	    adata->last_sysclk) {
+		unsigned int mclk_fs = DIV_ROUND_CLOSEST(adata->last_sysclk, params_rate(params));
+
+		writel(mclk_fs, stream_data->mmio + XLNX_AUD_FS_MULTIPLIER);
+	}
+
 	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE &&
 	    stream_data->xfer_mode == AES_TO_PCM) {
 		val = readl(stream_data->mmio + XLNX_AUD_STS);
@@ -550,6 +568,7 @@ static int xlnx_formatter_pcm_new(struct snd_soc_component *component,
 
 static const struct snd_soc_component_driver xlnx_asoc_component = {
 	.name		= DRV_NAME,
+	.set_sysclk	= xlnx_formatter_set_sysclk,
 	.open		= xlnx_formatter_pcm_open,
 	.close		= xlnx_formatter_pcm_close,
 	.hw_params	= xlnx_formatter_pcm_hw_params,
-- 
2.31.1

[PATCH 3/5] ASoC: xilinx: xlnx_i2s.c: Handle sysclk setting

[Robert Hancock]

This driver previously only handled the set_clkdiv divider callback when
setting the SCLK Out Divider field in the I2S Timing Control register.
However, when using the simple-audio-card driver, the set_sysclk function
is called but not set_clkdiv. This caused the divider not to be set,
leaving it at an invalid value of 0 and resulting in a very low SCLK
output rate.

Handle set_clkdiv and store the sysclk (MCLK) value for later use in
hw_params to set the SCLK Out Divider such that:
MCLK/SCLK = divider * 2

Fixes: 112a8900d4b0 ("ASoC: xlnx: Add i2s driver")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 sound/soc/xilinx/xlnx_i2s.c | 104 ++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 34 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
index cc641e582c82..1abe28821916 100644
--- a/sound/soc/xilinx/xlnx_i2s.c
+++ b/sound/soc/xilinx/xlnx_i2s.c
@@ -18,20 +18,39 @@
 #define DRV_NAME "xlnx_i2s"
 
 #define I2S_CORE_CTRL_OFFSET		0x08
+#define I2S_CORE_CTRL_32BIT_LRCLK	BIT(3)
+#define I2S_CORE_CTRL_ENABLE		BIT(0)
 #define I2S_I2STIM_OFFSET		0x20
 #define I2S_CH0_OFFSET			0x30
 #define I2S_I2STIM_VALID_MASK		GENMASK(7, 0)
 
+struct xlnx_i2s_drv_data {
+	struct snd_soc_dai_driver dai_drv;
+	void __iomem *base;
+	unsigned int last_sysclk;
+	u32 data_width;
+	bool is_32bit_lrclk;
+};
+
 static int xlnx_i2s_set_sclkout_div(struct snd_soc_dai *cpu_dai,
 				    int div_id, int div)
 {
-	void __iomem *base = snd_soc_dai_get_drvdata(cpu_dai);
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(cpu_dai);
 
 	if (!div || (div & ~I2S_I2STIM_VALID_MASK))
 		return -EINVAL;
 
-	writel(div, base + I2S_I2STIM_OFFSET);
+	writel(div, drv_data->base + I2S_I2STIM_OFFSET);
+
+	return 0;
+}
+
+static int xlnx_i2s_set_sysclk(struct snd_soc_dai *dai,
+			       int clk_id, unsigned int freq, int dir)
+{
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(dai);
 
+	drv_data->last_sysclk = freq;
 	return 0;
 }
 
@@ -40,13 +59,28 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream,
 			      struct snd_soc_dai *i2s_dai)
 {
 	u32 reg_off, chan_id;
-	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
+
+	if (drv_data->last_sysclk) {
+		unsigned int bits_per_sample = drv_data->is_32bit_lrclk ?
+					       32 : drv_data->data_width;
+		unsigned int sclk = params_rate(params) * bits_per_sample *
+				    params_channels(params);
+		unsigned int sclk_div = DIV_ROUND_CLOSEST(drv_data->last_sysclk, sclk) / 2;
+
+		if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {
+			dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk %u and sclk %u\n",
+				 drv_data->last_sysclk, sclk);
+			return -EINVAL;
+		}
+		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
+	}
 
 	chan_id = params_channels(params) / 2;
 
 	while (chan_id > 0) {
 		reg_off = I2S_CH0_OFFSET + ((chan_id - 1) * 4);
-		writel(chan_id, base + reg_off);
+		writel(chan_id, drv_data->base + reg_off);
 		chan_id--;
 	}
 
@@ -56,18 +90,18 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream,
 static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 			    struct snd_soc_dai *i2s_dai)
 {
-	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		writel(1, base + I2S_CORE_CTRL_OFFSET);
+		writel(I2S_CORE_CTRL_ENABLE, drv_data->base + I2S_CORE_CTRL_OFFSET);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		writel(0, base + I2S_CORE_CTRL_OFFSET);
+		writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
 		break;
 	default:
 		return -EINVAL;
@@ -78,6 +112,7 @@ static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 
 static const struct snd_soc_dai_ops xlnx_i2s_dai_ops = {
 	.trigger = xlnx_i2s_trigger,
+	.set_sysclk = xlnx_i2s_set_sysclk,
 	.set_clkdiv = xlnx_i2s_set_sclkout_div,
 	.hw_params = xlnx_i2s_hw_params
 };
@@ -95,20 +130,19 @@ MODULE_DEVICE_TABLE(of, xlnx_i2s_of_match);
 
 static int xlnx_i2s_probe(struct platform_device *pdev)
 {
-	void __iomem *base;
-	struct snd_soc_dai_driver *dai_drv;
+	struct xlnx_i2s_drv_data *drv_data;
 	int ret;
-	u32 ch, format, data_width;
+	u32 ch, format;
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 
-	dai_drv = devm_kzalloc(&pdev->dev, sizeof(*dai_drv), GFP_KERNEL);
-	if (!dai_drv)
+	drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
+	if (!drv_data)
 		return -ENOMEM;
 
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
+	drv_data->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(drv_data->base))
+		return PTR_ERR(drv_data->base);
 
 	ret = of_property_read_u32(node, "xlnx,num-channels", &ch);
 	if (ret < 0) {
@@ -117,12 +151,12 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
 	}
 	ch = ch * 2;
 
-	ret = of_property_read_u32(node, "xlnx,dwidth", &data_width);
+	ret = of_property_read_u32(node, "xlnx,dwidth", &drv_data->data_width);
 	if (ret < 0) {
 		dev_err(dev, "cannot get data width\n");
 		return ret;
 	}
-	switch (data_width) {
+	switch (drv_data->data_width) {
 	case 16:
 		format = SNDRV_PCM_FMTBIT_S16_LE;
 		break;
@@ -134,35 +168,37 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
 	}
 
 	if (of_device_is_compatible(node, "xlnx,i2s-transmitter-1.0")) {
-		dai_drv->name = "xlnx_i2s_playback";
-		dai_drv->playback.stream_name = "Playback";
-		dai_drv->playback.formats = format;
-		dai_drv->playback.channels_min = ch;
-		dai_drv->playback.channels_max = ch;
-		dai_drv->playback.rates	= SNDRV_PCM_RATE_8000_192000;
-		dai_drv->ops = &xlnx_i2s_dai_ops;
+		drv_data->dai_drv.name = "xlnx_i2s_playback";
+		drv_data->dai_drv.playback.stream_name = "Playback";
+		drv_data->dai_drv.playback.formats = format;
+		drv_data->dai_drv.playback.channels_min = ch;
+		drv_data->dai_drv.playback.channels_max = ch;
+		drv_data->dai_drv.playback.rates	= SNDRV_PCM_RATE_8000_192000;
+		drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
 	} else if (of_device_is_compatible(node, "xlnx,i2s-receiver-1.0")) {
-		dai_drv->name = "xlnx_i2s_capture";
-		dai_drv->capture.stream_name = "Capture";
-		dai_drv->capture.formats = format;
-		dai_drv->capture.channels_min = ch;
-		dai_drv->capture.channels_max = ch;
-		dai_drv->capture.rates = SNDRV_PCM_RATE_8000_192000;
-		dai_drv->ops = &xlnx_i2s_dai_ops;
+		drv_data->dai_drv.name = "xlnx_i2s_capture";
+		drv_data->dai_drv.capture.stream_name = "Capture";
+		drv_data->dai_drv.capture.formats = format;
+		drv_data->dai_drv.capture.channels_min = ch;
+		drv_data->dai_drv.capture.channels_max = ch;
+		drv_data->dai_drv.capture.rates = SNDRV_PCM_RATE_8000_192000;
+		drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
 	} else {
 		return -ENODEV;
 	}
+	drv_data->is_32bit_lrclk = readl(drv_data->base + I2S_CORE_CTRL_OFFSET) &
+				   I2S_CORE_CTRL_32BIT_LRCLK;
 
-	dev_set_drvdata(&pdev->dev, base);
+	dev_set_drvdata(&pdev->dev, drv_data);
 
 	ret = devm_snd_soc_register_component(&pdev->dev, &xlnx_i2s_component,
-					      dai_drv, 1);
+					      &drv_data->dai_drv, 1);
 	if (ret) {
 		dev_err(&pdev->dev, "i2s component registration failed\n");
 		return ret;
 	}
 
-	dev_info(&pdev->dev, "%s DAI registered\n", dai_drv->name);
+	dev_info(&pdev->dev, "%s DAI registered\n", drv_data->dai_drv.name);
 
 	return ret;
 }
-- 
2.31.1

[PATCH 4/5] ASoC: simple-card: fix probe failure on platform component

[Robert Hancock]

A previous change to simple-card resulted in asoc_simple_parse_dai
attempting to retrieve the dai_name for platform components, which are
unlikely to have a valid DAI name. This caused simple-card to fail to
probe when using the xlnx_formatter_pcm as the platform component, since
it does not register any DAI components.

Since the dai_name is not used for platform components, just skip trying
to retrieve it for those.

Fixes: f107294c6422 ("ASoC: simple-card: support snd_soc_dai_link_component style for cpu")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 sound/soc/generic/simple-card.c | 52 ++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index a89d1cfdda32..1295836e04f4 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -30,6 +30,7 @@ static const struct snd_soc_ops simple_ops = {
 
 static int asoc_simple_parse_dai(struct device_node *node,
 				 struct snd_soc_dai_link_component *dlc,
+				 bool is_plat,
 				 int *is_single_link)
 {
 	struct of_phandle_args args;
@@ -46,28 +47,31 @@ static int asoc_simple_parse_dai(struct device_node *node,
 	if (ret)
 		return ret;
 
-	/*
-	 * FIXME
-	 *
-	 * Here, dlc->dai_name is pointer to CPU/Codec DAI name.
-	 * If user unbinded CPU or Codec driver, but not for Sound Card,
-	 * dlc->dai_name is keeping unbinded CPU or Codec
-	 * driver's pointer.
-	 *
-	 * If user re-bind CPU or Codec driver again, ALSA SoC will try
-	 * to rebind Card via snd_soc_try_rebind_card(), but because of
-	 * above reason, it might can't bind Sound Card.
-	 * Because Sound Card is pointing to released dai_name pointer.
-	 *
-	 * To avoid this rebind Card issue,
-	 * 1) It needs to alloc memory to keep dai_name eventhough
-	 *    CPU or Codec driver was unbinded, or
-	 * 2) user need to rebind Sound Card everytime
-	 *    if he unbinded CPU or Codec.
-	 */
-	ret = snd_soc_of_get_dai_name(node, &dlc->dai_name);
-	if (ret < 0)
-		return ret;
+	/* dai_name is not required and may not exist for plat component */
+	if (!is_plat) {
+		/*
+		 * FIXME
+		 *
+		 * Here, dlc->dai_name is pointer to CPU/Codec DAI name.
+		 * If user unbinded CPU or Codec driver, but not for Sound Card,
+		 * dlc->dai_name is keeping unbinded CPU or Codec
+		 * driver's pointer.
+		 *
+		 * If user re-bind CPU or Codec driver again, ALSA SoC will try
+		 * to rebind Card via snd_soc_try_rebind_card(), but because of
+		 * above reason, it might can't bind Sound Card.
+		 * Because Sound Card is pointing to released dai_name pointer.
+		 *
+		 * To avoid this rebind Card issue,
+		 * 1) It needs to alloc memory to keep dai_name eventhough
+		 *    CPU or Codec driver was unbinded, or
+		 * 2) user need to rebind Sound Card everytime
+		 *    if he unbinded CPU or Codec.
+		 */
+		ret = snd_soc_of_get_dai_name(node, &dlc->dai_name);
+		if (ret < 0)
+			return ret;
+	}
 
 	dlc->of_node = args.np;
 
@@ -134,7 +138,7 @@ static int simple_parse_node(struct asoc_simple_priv *priv,
 
 	simple_parse_mclk_fs(top, np, dai_props, prefix);
 
-	ret = asoc_simple_parse_dai(np, dlc, cpu);
+	ret = asoc_simple_parse_dai(np, dlc, false, cpu);
 	if (ret)
 		return ret;
 
@@ -289,7 +293,7 @@ static int simple_dai_link_of(struct asoc_simple_priv *priv,
 	if (ret < 0)
 		goto dai_link_of_err;
 
-	ret = asoc_simple_parse_dai(plat, platforms, NULL);
+	ret = asoc_simple_parse_dai(plat, platforms, true, NULL);
 	if (ret < 0)
 		goto dai_link_of_err;
 
-- 
2.31.1

[PATCH 5/5] ASoC: simple-card-utils: Set sysclk on all components

[Robert Hancock]

If an mclk-fs value was provided in the device tree configuration, the
calculated MCLK was fed into the downstream codec DAI and CPU DAI,
however set_sysclk was not being called on the platform device. Some
platform devices such as the Xilinx Audio Formatter need to know the MCLK
as well.

Call snd_soc_component_set_sysclk on each component in the stream to set
the proper sysclk value in addition to the existing call of
snd_soc_dai_set_sysclk on the codec DAI and CPU DAI. This may end up
resulting in redundant calls if one of the snd_soc_dai_set_sysclk calls
ends up calling snd_soc_component_set_sysclk itself, but that isn't
expected to cause any significant harm.

Fixes: f48dcbb6d47d ("ASoC: simple-card-utils: share asoc_simple_hw_param()")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 sound/soc/generic/simple-card-utils.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index a81323d1691d..8345a750b183 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -275,6 +275,7 @@ int asoc_simple_hw_params(struct snd_pcm_substream *substream,
 		mclk_fs = props->mclk_fs;
 
 	if (mclk_fs) {
+		struct snd_soc_component *component;
 		mclk = params_rate(params) * mclk_fs;
 
 		for_each_prop_dai_codec(props, i, pdai) {
@@ -287,6 +288,16 @@ int asoc_simple_hw_params(struct snd_pcm_substream *substream,
 			if (ret < 0)
 				return ret;
 		}
+		/* Ensure sysclk is set on all components in case any
+		 * (such as platform components) are missed by calls to
+		 * snd_soc_dai_set_sysclk.
+		 */
+		for_each_rtd_components(rtd, i, component) {
+			ret = snd_soc_component_set_sysclk(component, 0, 0,
+							   mclk, SND_SOC_CLOCK_IN);
+			if (ret && ret != -ENOTSUPP)
+				return ret;
+		}
 		for_each_rtd_codec_dais(rtd, i, sdai) {
 			ret = snd_soc_dai_set_sysclk(sdai, 0, mclk, SND_SOC_CLOCK_IN);
 			if (ret && ret != -ENOTSUPP)
-- 
2.31.1

Re: [PATCH 1/5] ASoC: xilinx: xlnx_formatter_pcm: Make buffer bytes multiple of period bytes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

On Wed, Jan 05, 2022 at 04:51:42PM -0600, Robert Hancock wrote:

> +		return err;
> +	}
> +	/* Resize the buffer bytes as divisible by 64 */

> +	}
> +	/* Set periods as integer multiple */
> +	err = snd_pcm_hw_constraint_integer(runtime,

Missing blank lines between blocks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/5] ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

On Wed, Jan 05, 2022 at 04:51:43PM -0600, Robert Hancock wrote:

>  	struct clk *axi_clk;
> +	unsigned int last_sysclk;

Typically this would just be called sysclk - calling it last_sysclk
makes things a bit confusing.  It's being used as though it were the
current sysclk and that's what set_sysclk() is supposed to be for.

> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
> +	    adata->last_sysclk) {
> +		unsigned int mclk_fs = DIV_ROUND_CLOSEST(adata->last_sysclk, params_rate(params));
> +
> +		writel(mclk_fs, stream_data->mmio + XLNX_AUD_FS_MULTIPLIER);
> +	}
> +

Does the IP actually cope properly with inexact ratios, especially if
the actual clock rate is lower than mclk_fs would suggest?  It's more
common to be able to tolerate a higher clock than specified.

It's interesting that this is only for playback.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/5] ASoC: xilinx: xlnx_i2s.c: Handle sysclk setting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

On Wed, Jan 05, 2022 at 04:51:44PM -0600, Robert Hancock wrote:

> +	unsigned int last_sysclk;

Same naming issue.

> +	if (drv_data->last_sysclk) {
> +		unsigned int bits_per_sample = drv_data->is_32bit_lrclk ?
> +					       32 : drv_data->data_width;

Please write normal conditional statements, it improves legibility.

> +		unsigned int sclk = params_rate(params) * bits_per_sample *
> +				    params_channels(params);

snd_soc_params_to_bclk().

> +		unsigned int sclk_div = DIV_ROUND_CLOSEST(drv_data->last_sysclk, sclk) / 2;

Same issue with _ROUND_CLOSEST()

> +
> +		if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {
> +			dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk %u and sclk %u\n",
> +				 drv_data->last_sysclk, sclk);
> +			return -EINVAL;
> +		}

This indicates that we should be setting some constraints on sample rate
based on sysclk.

> +		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
> +	}

Does the device actually support operation without knowing the sysclk?

> @@ -56,18 +90,18 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream,
>  static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>  			    struct snd_soc_dai *i2s_dai)
>  {
> -	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
> +	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> -		writel(1, base + I2S_CORE_CTRL_OFFSET);
> +		writel(I2S_CORE_CTRL_ENABLE, drv_data->base + I2S_CORE_CTRL_OFFSET);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> -		writel(0, base + I2S_CORE_CTRL_OFFSET);
> +		writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
>  		break;

Please split the change to have a struct for driver data out into a
separate change, it's a large reformatting and is big enough to justify
it - more of the diff is that than the change described in the changelog
which doesn't mention this at all.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 5/5] ASoC: simple-card-utils: Set sysclk on all components

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 199 bytes --]

On Wed, Jan 05, 2022 at 04:51:46PM -0600, Robert Hancock wrote:

> +			if (ret && ret != -ENOTSUPP)
> +				return ret;
> +		}
>  		for_each_rtd_codec_dais(rtd, i, sdai) {

Again, missing blank line.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/5] ASoC: simple-card: fix probe failure on platform component

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

On Wed, Jan 05, 2022 at 04:51:45PM -0600, Robert Hancock wrote:

>  static int asoc_simple_parse_dai(struct device_node *node,
>  				 struct snd_soc_dai_link_component *dlc,
> +				 bool is_plat,
>  				 int *is_single_link)

Why not just make a function for platforms given that your change causes
such a huge part of the function to get skipped?  This interface is
pretty confusing, if the thing isn't a DAI we probably just shouldn't be
using a function called _parse_dai().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/5] ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting

[Robert Hancock]

On Thu, 2022-01-06 at 12:26 +0000, Mark Brown wrote:
> On Wed, Jan 05, 2022 at 04:51:43PM -0600, Robert Hancock wrote:
> 
> >  	struct clk *axi_clk;
> > +	unsigned int last_sysclk;
> 
> Typically this would just be called sysclk - calling it last_sysclk
> makes things a bit confusing.  It's being used as though it were the
> current sysclk and that's what set_sysclk() is supposed to be for.

Will switch to just sysclk.

> 
> > +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
> > +	    adata->last_sysclk) {
> > +		unsigned int mclk_fs = DIV_ROUND_CLOSEST(adata->last_sysclk,
> > params_rate(params));
> > +
> > +		writel(mclk_fs, stream_data->mmio + XLNX_AUD_FS_MULTIPLIER);
> > +	}
> > +
> 
> Does the IP actually cope properly with inexact ratios, especially if
> the actual clock rate is lower than mclk_fs would suggest?  It's more
> common to be able to tolerate a higher clock than specified.

Unknown at this point - the test setup I have has a fixed sample rate so I
can't really test it. The documentation is unclear on exactly why this register
exists and what it's used for, it just indicates it should be set for the
sample rate to MCLK multiplier. All I really know for sure is that with it left
set to the default of 384 when the actual multiplier is 256, it doesn't work
properly.

> 
> It's interesting that this is only for playback.


-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH 2/5] ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

On Thu, Jan 06, 2022 at 05:38:42PM +0000, Robert Hancock wrote:
> On Thu, 2022-01-06 at 12:26 +0000, Mark Brown wrote:

> > Does the IP actually cope properly with inexact ratios, especially if
> > the actual clock rate is lower than mclk_fs would suggest?  It's more
> > common to be able to tolerate a higher clock than specified.

> Unknown at this point - the test setup I have has a fixed sample rate so I
> can't really test it. The documentation is unclear on exactly why this register
> exists and what it's used for, it just indicates it should be set for the
> sample rate to MCLK multiplier. All I really know for sure is that with it left
> set to the default of 384 when the actual multiplier is 256, it doesn't work
> properly.

Generally the IP will have to do more work than can be done in a single
clock cycle for each bit and needs everything to be done with
synchronous clocks to avoid data corruption.  Based on your report there
it seems like it definitely doesn't tolerate being underclocked well so
DIV_ROUND_CLOSEST is not what's wanted.  Requiring an exact match would
be safest.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/5] ASoC: xilinx: xlnx_i2s.c: Handle sysclk setting

[Robert Hancock]

On Thu, 2022-01-06 at 12:42 +0000, Mark Brown wrote:
> On Wed, Jan 05, 2022 at 04:51:44PM -0600, Robert Hancock wrote:
> 
> > +	unsigned int last_sysclk;
> 
> Same naming issue.

Will switch to sysclk here as well.

> 
> > +	if (drv_data->last_sysclk) {
> > +		unsigned int bits_per_sample = drv_data->is_32bit_lrclk ?
> > +					       32 : drv_data->data_width;
> 
> Please write normal conditional statements, it improves legibility.

Will do.

> 
> > +		unsigned int sclk = params_rate(params) * bits_per_sample *
> > +				    params_channels(params);
> 
> snd_soc_params_to_bclk().

I don't think that works here since that depends on the result of
snd_soc_params_to_frame_size, which doesn't account for the bits per sample
being forced to 32 when the 32bit_lrclk mode is active?

> 
> > +		unsigned int sclk_div = DIV_ROUND_CLOSEST(drv_data-
> > >last_sysclk, sclk) / 2;
> 
> Same issue with _ROUND_CLOSEST()

Will update to require exact divisibility.

> 
> > +
> > +		if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {
> > +			dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk
> > %u and sclk %u\n",
> > +				 drv_data->last_sysclk, sclk);
> > +			return -EINVAL;
> > +		}
> 
> This indicates that we should be setting some constraints on sample rate
> based on sysclk.

Is there a way to do this at this level given that it can only be enforced
after set_sysclk is called? Most of the other drivers that enforce rate
constraints seem to be more of a fixed list..

> 
> > +		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
> > +	}
> 
> Does the device actually support operation without knowing the sysclk?

It could work if set_clkdiv is called to directly set the divider, rather than
set_sysclk. simple-card doesn't do that, but possibly some other setup which
uses this does?

> 
> > @@ -56,18 +90,18 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream
> > *substream,
> >  static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> >  			    struct snd_soc_dai *i2s_dai)
> >  {
> > -	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
> > +	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
> >  
> >  	switch (cmd) {
> >  	case SNDRV_PCM_TRIGGER_START:
> >  	case SNDRV_PCM_TRIGGER_RESUME:
> >  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > -		writel(1, base + I2S_CORE_CTRL_OFFSET);
> > +		writel(I2S_CORE_CTRL_ENABLE, drv_data->base +
> > I2S_CORE_CTRL_OFFSET);
> >  		break;
> >  	case SNDRV_PCM_TRIGGER_STOP:
> >  	case SNDRV_PCM_TRIGGER_SUSPEND:
> >  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> > -		writel(0, base + I2S_CORE_CTRL_OFFSET);
> > +		writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
> >  		break;
> 
> Please split the change to have a struct for driver data out into a
> separate change, it's a large reformatting and is big enough to justify
> it - more of the diff is that than the change described in the changelog
> which doesn't mention this at all.

Will do.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH 3/5] ASoC: xilinx: xlnx_i2s.c: Handle sysclk setting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

On Thu, Jan 06, 2022 at 11:25:28PM +0000, Robert Hancock wrote:
> On Thu, 2022-01-06 at 12:42 +0000, Mark Brown wrote:
> > On Wed, Jan 05, 2022 at 04:51:44PM -0600, Robert Hancock wrote:

> > snd_soc_params_to_bclk().

> I don't think that works here since that depends on the result of
> snd_soc_params_to_frame_size, which doesn't account for the bits per sample
> being forced to 32 when the 32bit_lrclk mode is active?

OK.

> > > +		if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {
> > > +			dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk
> > > %u and sclk %u\n",
> > > +				 drv_data->last_sysclk, sclk);
> > > +			return -EINVAL;
> > > +		}

> > This indicates that we should be setting some constraints on sample rate
> > based on sysclk.

> Is there a way to do this at this level given that it can only be enforced
> after set_sysclk is called? Most of the other drivers that enforce rate
> constraints seem to be more of a fixed list..

	if (drvdata->sysclk) {
		/* set constraints */
	}


like a bunch of other drivers do (eg, wm8731).

> > > +		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
> > > +	}

> > Does the device actually support operation without knowing the sysclk?

> It could work if set_clkdiv is called to directly set the divider, rather than
> set_sysclk. simple-card doesn't do that, but possibly some other setup which
> uses this does?

We should be migrating away from set_clkdiv() anyway, it'll be fine to
require that such drivers be updated.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]