[PATCH v3 0/6] ASoC: Xilinx fixes

[PATCH v3 0/6] 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.

Changes since v2:
-drop patches already merged
-added constraint to simple-card to allow enforcing valid sample rates

Changes since v1:
-formatting fixes
-renamed last_sysclk variables to sysclk
-require exact match for clock divisor rather than rounding to nearest
-broke out driver data structure change in xlnx_i2s to separate patch
-added constraints for sample rate based on sysclk to xlnx_i2s
-switched to separate function for DAI parsing for platforms in simple_card

Robert Hancock (6):
  ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting
  ASoC: xilinx: xlnx_i2s: create drvdata structure
  ASoC: xilinx: xlnx_i2s: Handle sysclk setting
  ASoC: simple-card-utils: Set sysclk on all components
  ASoC: dt-bindings: simple-card: document new system-clock-fixed flag
  ASoC: simple-card-utils: Add new system-clock-fixed flag

 .../bindings/sound/simple-card.yaml           |  11 ++
 include/sound/simple_card_utils.h             |   1 +
 sound/soc/generic/simple-card-utils.c         |  86 ++++++++--
 sound/soc/xilinx/xlnx_formatter_pcm.c         |  25 +++
 sound/soc/xilinx/xlnx_i2s.c                   | 147 +++++++++++++-----
 5 files changed, 223 insertions(+), 47 deletions(-)

-- 
2.31.1

[PATCH v3 1/6] 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 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/sound/soc/xilinx/xlnx_formatter_pcm.c b/sound/soc/xilinx/xlnx_formatter_pcm.c
index ce19a6058b27..5c4158069a5a 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 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->sysclk = freq;
+	return 0;
+}
+
 static int xlnx_formatter_pcm_open(struct snd_soc_component *component,
 				   struct snd_pcm_substream *substream)
 {
@@ -450,11 +460,25 @@ 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->sysclk) {
+		unsigned int mclk_fs = adata->sysclk / params_rate(params);
+
+		if (adata->sysclk % params_rate(params) != 0) {
+			dev_warn(component->dev, "sysclk %u not divisible by rate %u\n",
+				 adata->sysclk, params_rate(params));
+			return -EINVAL;
+		}
+
+		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);
@@ -552,6 +576,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 v3 2/6] ASoC: xilinx: xlnx_i2s: create drvdata structure

[Robert Hancock]

An upcoming change will require storing additional driver data other
than the memory base address. Create a drvdata structure and use that
rather than storing the raw base address pointer.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 sound/soc/xilinx/xlnx_i2s.c | 66 ++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
index cc641e582c82..3bafa34b789a 100644
--- a/sound/soc/xilinx/xlnx_i2s.c
+++ b/sound/soc/xilinx/xlnx_i2s.c
@@ -22,15 +22,20 @@
 #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;
+};
+
 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;
 }
@@ -40,13 +45,13 @@ 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);
 
 	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 +61,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(1, 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;
@@ -95,20 +100,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;
 	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) {
@@ -134,35 +138,35 @@ 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;
 	}
 
-	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

Re: [PATCH v3 2/6] ASoC: xilinx: xlnx_i2s: create drvdata structure

[Amadeusz Sławiński]

On 1/20/2022 8:58 PM, Robert Hancock wrote:
> An upcoming change will require storing additional driver data other
> than the memory base address. Create a drvdata structure and use that
> rather than storing the raw base address pointer.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>   sound/soc/xilinx/xlnx_i2s.c | 66 ++++++++++++++++++++-----------------
>   1 file changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
> index cc641e582c82..3bafa34b789a 100644
> --- a/sound/soc/xilinx/xlnx_i2s.c
> +++ b/sound/soc/xilinx/xlnx_i2s.c
> @@ -22,15 +22,20 @@
>   #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;
> +};
> +
>   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;
>   }
> @@ -40,13 +45,13 @@ 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);
>   
>   	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 +61,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(1, 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;
> @@ -95,20 +100,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;
>   	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) {
> @@ -134,35 +138,35 @@ 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;
>   	}
>   
> -	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;
>   }

I don't think this patch is needed, snd_soc_dai, already has pointer to 
its snd_soc_dai_driver, so there is no need to keep it additionally in 
drvdata?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/sound/soc-dai.h?h=v5.16#n431

Re: [PATCH v3 2/6] ASoC: xilinx: xlnx_i2s: create drvdata structure

[Robert Hancock]

On Fri, 2022-01-21 at 10:06 +0100, Amadeusz Sławiński wrote:
> On 1/20/2022 8:58 PM, Robert Hancock wrote:
> > An upcoming change will require storing additional driver data other
> > than the memory base address. Create a drvdata structure and use that
> > rather than storing the raw base address pointer.
> > 
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > ---
> >   sound/soc/xilinx/xlnx_i2s.c | 66 ++++++++++++++++++++-----------------
> >   1 file changed, 35 insertions(+), 31 deletions(-)
> > 
> > diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
> > index cc641e582c82..3bafa34b789a 100644
> > --- a/sound/soc/xilinx/xlnx_i2s.c
> > +++ b/sound/soc/xilinx/xlnx_i2s.c
> > @@ -22,15 +22,20 @@
> >   #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;
> > +};
> > +
> >   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;
> >   }
> > @@ -40,13 +45,13 @@ 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);
> >   
> >   	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 +61,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(1, 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;
> > @@ -95,20 +100,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;
> >   	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) {
> > @@ -134,35 +138,35 @@ 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;
> >   	}
> >   
> > -	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;
> >   }
> 
> I don't think this patch is needed, snd_soc_dai, already has pointer to 
> its snd_soc_dai_driver, so there is no need to keep it additionally in 
> drvdata?
> 
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/sound/soc-dai.h?h=v5.16*n431__;Iw!!IOGos0k!wK-VYGvndh29eBo3CZIPn7xG_X7ib4R8-hEVEyJc8aXkGoYORJ8cLH25u-K31eZAYe4$ 
> 
> 

It's not a pointer to the struct snd_soc_dai_driver that's in the drvdata
structure, snd_soc_dai_driver is actually part of the drvdata structure.
Previously it was allocating snd_soc_dai_driver by itself, and stuffing the
base address into the drvdata pointer. Now it's allocating one
xlnx_i2s_drv_data structure which contains both (and more attributes to come).

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

[PATCH v3 3/6] ASoC: xilinx: xlnx_i2s: 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

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 sound/soc/xilinx/xlnx_i2s.c | 91 +++++++++++++++++++++++++++++++++----
 1 file changed, 81 insertions(+), 10 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
index 3bafa34b789a..4cc6ee7c81a3 100644
--- a/sound/soc/xilinx/xlnx_i2s.c
+++ b/sound/soc/xilinx/xlnx_i2s.c
@@ -18,6 +18,8 @@
 #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)
@@ -25,6 +27,12 @@
 struct xlnx_i2s_drv_data {
 	struct snd_soc_dai_driver dai_drv;
 	void __iomem *base;
+	unsigned int sysclk;
+	u32 data_width;
+	u32 channels;
+	bool is_32bit_lrclk;
+	struct snd_ratnum ratnum;
+	struct snd_pcm_hw_constraint_ratnums rate_constraints;
 };
 
 static int xlnx_i2s_set_sclkout_div(struct snd_soc_dai *cpu_dai,
@@ -35,11 +43,50 @@ static int xlnx_i2s_set_sclkout_div(struct snd_soc_dai *cpu_dai,
 	if (!div || (div & ~I2S_I2STIM_VALID_MASK))
 		return -EINVAL;
 
+	drv_data->sysclk = 0;
+
 	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->sysclk = freq;
+	if (freq) {
+		unsigned int bits_per_sample;
+
+		if (drv_data->is_32bit_lrclk)
+			bits_per_sample = 32;
+		else
+			bits_per_sample = drv_data->data_width;
+
+		drv_data->ratnum.num = freq / (bits_per_sample * drv_data->channels) / 2;
+		drv_data->ratnum.den_step = 1;
+		drv_data->ratnum.den_min = 1;
+		drv_data->ratnum.den_max = 255;
+		drv_data->rate_constraints.rats = &drv_data->ratnum;
+		drv_data->rate_constraints.nrats = 1;
+	}
+	return 0;
+}
+
+static int xlnx_i2s_startup(struct snd_pcm_substream *substream,
+			    struct snd_soc_dai *dai)
+{
+	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(dai);
+
+	if (drv_data->sysclk)
+		return snd_pcm_hw_constraint_ratnums(substream->runtime, 0,
+						     SNDRV_PCM_HW_PARAM_RATE,
+						     &drv_data->rate_constraints);
+
+	return 0;
+}
+
 static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream,
 			      struct snd_pcm_hw_params *params,
 			      struct snd_soc_dai *i2s_dai)
@@ -47,6 +94,26 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream,
 	u32 reg_off, chan_id;
 	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
 
+	if (drv_data->sysclk) {
+		unsigned int bits_per_sample, sclk, sclk_div;
+
+		if (drv_data->is_32bit_lrclk)
+			bits_per_sample = 32;
+		else
+			bits_per_sample = drv_data->data_width;
+
+		sclk = params_rate(params) * bits_per_sample * params_channels(params);
+		sclk_div = drv_data->sysclk / sclk / 2;
+
+		if ((drv_data->sysclk % sclk != 0) ||
+		    !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->sysclk, sclk);
+			return -EINVAL;
+		}
+		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
+	}
+
 	chan_id = params_channels(params) / 2;
 
 	while (chan_id > 0) {
@@ -67,7 +134,7 @@ static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		writel(1, drv_data->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:
@@ -83,7 +150,9 @@ 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,
+	.startup = xlnx_i2s_startup,
 	.hw_params = xlnx_i2s_hw_params
 };
 
@@ -102,7 +171,7 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
 {
 	struct xlnx_i2s_drv_data *drv_data;
 	int ret;
-	u32 ch, format, data_width;
+	u32 format;
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 
@@ -114,19 +183,19 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
 	if (IS_ERR(drv_data->base))
 		return PTR_ERR(drv_data->base);
 
-	ret = of_property_read_u32(node, "xlnx,num-channels", &ch);
+	ret = of_property_read_u32(node, "xlnx,num-channels", &drv_data->channels);
 	if (ret < 0) {
 		dev_err(dev, "cannot get supported channels\n");
 		return ret;
 	}
-	ch = ch * 2;
+	drv_data->channels *= 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;
@@ -141,21 +210,23 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
 		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.channels_min = drv_data->channels;
+		drv_data->dai_drv.playback.channels_max = drv_data->channels;
 		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")) {
 		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.channels_min = drv_data->channels;
+		drv_data->dai_drv.capture.channels_max = drv_data->channels;
 		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, drv_data);
 
-- 
2.31.1

[PATCH v3 4/6] 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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index a81323d1691d..9736102e6808 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) {
@@ -282,16 +283,30 @@ int asoc_simple_hw_params(struct snd_pcm_substream *substream,
 			if (ret < 0)
 				return ret;
 		}
+
 		for_each_prop_dai_cpu(props, i, pdai) {
 			ret = asoc_simple_set_clk_rate(pdai, mclk);
 			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)
 				return ret;
 		}
+
 		for_each_rtd_cpu_dais(rtd, i, sdai) {
 			ret = snd_soc_dai_set_sysclk(sdai, 0, mclk, SND_SOC_CLOCK_OUT);
 			if (ret && ret != -ENOTSUPP)
-- 
2.31.1

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

[Kuninori Morimoto]

Hi

> 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>
> ---

Reviewed-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Thank you for your help !!

Best regards
---
Kuninori Morimoto

[PATCH v3 5/6] ASoC: dt-bindings: simple-card: document new system-clock-fixed flag

[Robert Hancock]

Document the new system-clock-fixed flag, which can be used to specify
that the driver cannot or should not allow the clock frequency of the
mapped clock to be modified.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 .../devicetree/bindings/sound/simple-card.yaml        | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml
index 45fd9fd9eb54..00597dc4f396 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.yaml
+++ b/Documentation/devicetree/bindings/sound/simple-card.yaml
@@ -48,6 +48,15 @@ definitions:
       It is useful for some aCPUs with fixed clocks.
     $ref: /schemas/types.yaml#/definitions/flag
 
+  system-clock-fixed:
+    description: |
+      Specifies that the clock frequency should not be modified.
+      Implied when system-clock-frequency is specified, but can be used when
+      a clock is mapped to the device whose frequency cannot or should not be
+      changed. When mclk-fs is also specified, this restricts the device to a
+      single fixed sampling rate.
+    $ref: /schemas/types.yaml#/definitions/flag
+
   mclk-fs:
     description: |
       Multiplication factor between stream rate and codec mclk.
@@ -134,6 +143,8 @@ definitions:
         $ref: "#/definitions/system-clock-frequency"
       system-clock-direction-out:
         $ref: "#/definitions/system-clock-direction-out"
+      system-clock-fixed:
+        $ref: "#/definitions/system-clock-fixed"
     required:
       - sound-dai
 
-- 
2.31.1

[PATCH v3 6/6] ASoC: simple-card-utils: Add new system-clock-fixed flag

[Robert Hancock]

Add a new system-clock-fixed flag, which can be used to specify that the
driver cannot or should not allow the clock frequency of the mapped clock
to be modified. This behavior is also implied if the system-clock-frequency
parameter is set explicitly - the flag is meant for cases where a clock is
mapped to the DAI but which is, or should be treated as, fixed.

When mclk-fs is also specified, this causes a PCM constraint to be added
which enforces that only the corresponding valid sample rate can be used.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 include/sound/simple_card_utils.h     |  1 +
 sound/soc/generic/simple-card-utils.c | 71 ++++++++++++++++++++++-----
 2 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h
index df430f1c2a10..5ee269c59aac 100644
--- a/include/sound/simple_card_utils.h
+++ b/include/sound/simple_card_utils.h
@@ -25,6 +25,7 @@ struct asoc_simple_dai {
 	unsigned int tx_slot_mask;
 	unsigned int rx_slot_mask;
 	struct clk *clk;
+	bool clk_fixed;
 };
 
 struct asoc_simple_data {
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 9736102e6808..a4babfb63175 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -165,12 +165,15 @@ int asoc_simple_parse_clk(struct device *dev,
 	 *  or device's module clock.
 	 */
 	clk = devm_get_clk_from_child(dev, node, NULL);
+	simple_dai->clk_fixed = of_property_read_bool(
+		node, "system-clock-fixed");
 	if (!IS_ERR(clk)) {
 		simple_dai->sysclk = clk_get_rate(clk);
 
 		simple_dai->clk = clk;
 	} else if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
 		simple_dai->sysclk = val;
+		simple_dai->clk_fixed = true;
 	} else {
 		clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
 		if (!IS_ERR(clk))
@@ -184,12 +187,29 @@ int asoc_simple_parse_clk(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(asoc_simple_parse_clk);
 
+static int asoc_simple_check_fixed_sysclk(struct device *dev,
+					  struct asoc_simple_dai *dai,
+					  unsigned int *fixed_sysclk)
+{
+	if (dai->clk_fixed) {
+		if (*fixed_sysclk && *fixed_sysclk != dai->sysclk) {
+			dev_err(dev, "inconsistent fixed sysclk rates (%u vs %u)\n",
+				*fixed_sysclk, dai->sysclk);
+			return -EINVAL;
+		}
+		*fixed_sysclk = dai->sysclk;
+	}
+
+	return 0;
+}
+
 int asoc_simple_startup(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(rtd->card);
 	struct simple_dai_props *props = simple_priv_to_props(priv, rtd->num);
 	struct asoc_simple_dai *dai;
+	unsigned int fixed_sysclk = 0;
 	int i1, i2, i;
 	int ret;
 
@@ -197,12 +217,32 @@ int asoc_simple_startup(struct snd_pcm_substream *substream)
 		ret = asoc_simple_clk_enable(dai);
 		if (ret)
 			goto cpu_err;
+		ret = asoc_simple_check_fixed_sysclk(rtd->dev, dai, &fixed_sysclk);
+		if (ret)
+			goto cpu_err;
 	}
 
 	for_each_prop_dai_codec(props, i2, dai) {
 		ret = asoc_simple_clk_enable(dai);
 		if (ret)
 			goto codec_err;
+		ret = asoc_simple_check_fixed_sysclk(rtd->dev, dai, &fixed_sysclk);
+		if (ret)
+			goto codec_err;
+	}
+
+	if (fixed_sysclk && props->mclk_fs) {
+		unsigned int fixed_rate = fixed_sysclk / props->mclk_fs;
+
+		if (fixed_sysclk % props->mclk_fs) {
+			dev_err(rtd->dev, "fixed sysclk %u not divisible by mclk_fs %u\n",
+				fixed_sysclk, props->mclk_fs);
+			return -EINVAL;
+		}
+		ret = snd_pcm_hw_constraint_minmax(substream->runtime, SNDRV_PCM_HW_PARAM_RATE,
+			fixed_rate, fixed_rate);
+		if (ret)
+			goto codec_err;
 	}
 
 	return 0;
@@ -226,31 +266,40 @@ EXPORT_SYMBOL_GPL(asoc_simple_startup);
 void asoc_simple_shutdown(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
-	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 	struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(rtd->card);
 	struct simple_dai_props *props = simple_priv_to_props(priv, rtd->num);
 	struct asoc_simple_dai *dai;
 	int i;
 
-	if (props->mclk_fs) {
-		snd_soc_dai_set_sysclk(codec_dai, 0, 0, SND_SOC_CLOCK_IN);
-		snd_soc_dai_set_sysclk(cpu_dai, 0, 0, SND_SOC_CLOCK_OUT);
-	}
+	for_each_prop_dai_cpu(props, i, dai) {
+		if (props->mclk_fs && !dai->clk_fixed)
+			snd_soc_dai_set_sysclk(asoc_rtd_to_cpu(rtd, i),
+					       0, 0, SND_SOC_CLOCK_IN);
 
-	for_each_prop_dai_cpu(props, i, dai)
 		asoc_simple_clk_disable(dai);
-	for_each_prop_dai_codec(props, i, dai)
+	}
+	for_each_prop_dai_codec(props, i, dai) {
+		if (props->mclk_fs && !dai->clk_fixed)
+			snd_soc_dai_set_sysclk(asoc_rtd_to_codec(rtd, i),
+					       0, 0, SND_SOC_CLOCK_IN);
+
 		asoc_simple_clk_disable(dai);
+	}
 }
 EXPORT_SYMBOL_GPL(asoc_simple_shutdown);
 
-static int asoc_simple_set_clk_rate(struct asoc_simple_dai *simple_dai,
+static int asoc_simple_set_clk_rate(struct device *dev,
+				    struct asoc_simple_dai *simple_dai,
 				    unsigned long rate)
 {
 	if (!simple_dai)
 		return 0;
 
+	if (simple_dai->clk_fixed && rate != simple_dai->sysclk) {
+		dev_err(dev, "dai %s invalid clock rate %lu\n", simple_dai->name, rate);
+		return -EINVAL;
+	}
+
 	if (!simple_dai->clk)
 		return 0;
 
@@ -279,13 +328,13 @@ int asoc_simple_hw_params(struct snd_pcm_substream *substream,
 		mclk = params_rate(params) * mclk_fs;
 
 		for_each_prop_dai_codec(props, i, pdai) {
-			ret = asoc_simple_set_clk_rate(pdai, mclk);
+			ret = asoc_simple_set_clk_rate(rtd->dev, pdai, mclk);
 			if (ret < 0)
 				return ret;
 		}
 
 		for_each_prop_dai_cpu(props, i, pdai) {
-			ret = asoc_simple_set_clk_rate(pdai, mclk);
+			ret = asoc_simple_set_clk_rate(rtd->dev, pdai, mclk);
 			if (ret < 0)
 				return ret;
 		}
-- 
2.31.1

Re: [PATCH v3 0/6] ASoC: Xilinx fixes

[Mark Brown]

On Thu, 20 Jan 2022 13:58:26 -0600, Robert Hancock wrote:
> 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.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/6] ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting
      commit: 1c5091fbe7e0d0804158200b7feac5123f7b4fbd
[2/6] ASoC: xilinx: xlnx_i2s: create drvdata structure
      commit: 5e46c63ca22278fe363dfd9f5360c2e2ad082087
[3/6] ASoC: xilinx: xlnx_i2s: Handle sysclk setting
      commit: c47aef899c1bb0cbda48808356e7c040d95ca612
[4/6] ASoC: simple-card-utils: Set sysclk on all components
      commit: ce2f7b8d4290c22e462e465d1da38a1c113ae66a
[5/6] ASoC: dt-bindings: simple-card: document new system-clock-fixed flag
      commit: e9fed03aebacb8873dee8e2edfbce96f27f6c730
[6/6] ASoC: simple-card-utils: Add new system-clock-fixed flag
      commit: 5ca2ab4598179a2690a38420f3fde9f2ad79d55c

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark