[PATCH v2 0/6] ASoC: Xilinx fixes

[PATCH v2 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 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: Make buffer bytes multiple of period
    bytes
  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: fix probe failure on platform component
  ASoC: simple-card-utils: Set sysclk on all components

 sound/soc/generic/simple-card-utils.c |  15 +++
 sound/soc/generic/simple-card.c       |  26 ++++-
 sound/soc/xilinx/xlnx_formatter_pcm.c |  52 ++++++++-
 sound/soc/xilinx/xlnx_i2s.c           | 147 +++++++++++++++++++-------
 4 files changed, 200 insertions(+), 40 deletions(-)

-- 
2.31.1

[PATCH v2 1/6] 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 | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_formatter_pcm.c b/sound/soc/xilinx/xlnx_formatter_pcm.c
index 91afea9d5de6..ce19a6058b27 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,32 @@ 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 v2 2/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

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

[Mark Brown]

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

On Fri, Jan 07, 2022 at 03:47:07PM -0600, Robert Hancock wrote:
> 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.

The driver should be setting a constraint for this if the sysclk is
configured, we shouldn't end up in a situation where userspace is trying
to start a playback that will fail.

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

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

[Robert Hancock]

On Mon, 2022-01-10 at 14:46 +0000, Mark Brown wrote:
> On Fri, Jan 07, 2022 at 03:47:07PM -0600, Robert Hancock wrote:
> > 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.
> 
> The driver should be setting a constraint for this if the sysclk is
> configured, we shouldn't end up in a situation where userspace is trying
> to start a playback that will fail.

So as far as I can tell, the current situation is:

-On initialization for simple-card, if a clock frequency is specified in device
tree, set_sysclk is called on the DAI by asoc_simple_init_dai (called by
asoc_simple_dai_init). However, it doesn't appear that it is called on the
platform (xlnx_formatter_pcm in this case) at this point.

-startup gets called on the DAI from pcm_open, so xlnx_i2s should be able to
add its rate constraints properly at that point. However, xlnx_formatter_pcm
has no sysclk set at this point, so it couldn't currently enforce any
constraint based on that.

-when the top-level hw_params call is made with simple-card, set_sysclk gets
called on everything by asoc_simple_hw_params prior to hw_params calls on all
of the components. The sysclk there is based on the device tree mclk-fs and the
sample rate which might wipe out the original clock frequency if the
constraints don't prevent setting an invalid rate.

-In the case of xlnx_formatter_pcm and simple-card, since sysclk is determined
by sample rate times mclk-fs, there's no way for userspace to violate the
constraint that the sample rate divides evenly into sysclk.

-When the PCM is closed, asoc_simple_shutdown sets the sysclk to 0 on the DAIs,
which ends up wiping out the fixed value that xlnx_i2s had stored from
initialization.

From that I conclude:

-in order to add any constraints on sample rate based on sysclk in the
formatter driver, it would need to get set_sysclk called during initialization
which doesn't currently happen. But with simple-card, there's no way those
constraints could be violated other than a kernel bug.

-xlnx_i2s needs some way to avoid its stored sysclk being wiped out during PCM
close so that the constraints are handled properly during subsequent opens.

Any thoughts on how those should be handled?

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

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

[Mark Brown]

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

On Mon, Jan 10, 2022 at 06:28:36PM +0000, Robert Hancock wrote:

> -On initialization for simple-card, if a clock frequency is specified in device
> tree, set_sysclk is called on the DAI by asoc_simple_init_dai (called by
> asoc_simple_dai_init). However, it doesn't appear that it is called on the
> platform (xlnx_formatter_pcm in this case) at this point.

> -startup gets called on the DAI from pcm_open, so xlnx_i2s should be able to
> add its rate constraints properly at that point. However, xlnx_formatter_pcm
> has no sysclk set at this point, so it couldn't currently enforce any
> constraint based on that.

I thought the later patches in your series were intended to fix things
so that the set_sysclk() does get called?

> -when the top-level hw_params call is made with simple-card, set_sysclk gets
> called on everything by asoc_simple_hw_params prior to hw_params calls on all
> of the components. The sysclk there is based on the device tree mclk-fs and the
> sample rate which might wipe out the original clock frequency if the
> constraints don't prevent setting an invalid rate.

If the device is using mclk-fs then either there's a fixed sample rate
(in which case simple-card probably ought to force it without the driver
worrying) or the sysclk will vary in which case simple-card should be
setting the sysclk to 0 when the card goes idle to clear any
constraints (which as you say later it does).

> -In the case of xlnx_formatter_pcm and simple-card, since sysclk is determined
> by sample rate times mclk-fs, there's no way for userspace to violate the
> constraint that the sample rate divides evenly into sysclk.

Only on a system where the sysclk can vary - this is a generic card so
someone could set a fixed sysclk, and of course the driver could be used
with other cards.

> -in order to add any constraints on sample rate based on sysclk in the
> formatter driver, it would need to get set_sysclk called during initialization
> which doesn't currently happen. But with simple-card, there's no way those
> constraints could be violated other than a kernel bug.

You shouldn't be making assumptions about the machine driver in the DMA
driver, especially for something like this used in a FPGA product which
has even more flexibility than most things.

> -xlnx_i2s needs some way to avoid its stored sysclk being wiped out during PCM
> close so that the constraints are handled properly during subsequent opens.

Depending on how flexible the system is clearing the sysclk stored in
the I2S driver may be desirable - if the sysclk rate can be changed then
you usually don't want to force constraints based on what it was during
the last stream, you want to relax such constraints so that a new sysclk
can be chosen where appropriate.

If that's not possible in this system then it sounds like it should be
setting system-clock-frequency and simple-card should then not be
clearing any setting in the components when the stream closes down, it
should be setting the clock up once at init.  Broadly speaking the
machine driver is responsible for ensuring that the overall system
configuration is sensible and coherent (that's what it's there for).

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

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

[Robert Hancock]

On Mon, 2022-01-10 at 20:31 +0000, Mark Brown wrote:
> On Mon, Jan 10, 2022 at 06:28:36PM +0000, Robert Hancock wrote:
> 
> > -On initialization for simple-card, if a clock frequency is specified in
> > device
> > tree, set_sysclk is called on the DAI by asoc_simple_init_dai (called by
> > asoc_simple_dai_init). However, it doesn't appear that it is called on the
> > platform (xlnx_formatter_pcm in this case) at this point.
> > -startup gets called on the DAI from pcm_open, so xlnx_i2s should be able
> > to
> > add its rate constraints properly at that point. However,
> > xlnx_formatter_pcm
> > has no sysclk set at this point, so it couldn't currently enforce any
> > constraint based on that.
> 
> I thought the later patches in your series were intended to fix things
> so that the set_sysclk() does get called?

It does get called during hw_params now, but not during initialization as it is
with the DAI components. I'm not sure that really matters if we don't try to
add constraints in that driver though (see below) and it just needs to know
what sysclk and sample rate are being used.

> 
> > -when the top-level hw_params call is made with simple-card, set_sysclk
> > gets
> > called on everything by asoc_simple_hw_params prior to hw_params calls on
> > all
> > of the components. The sysclk there is based on the device tree mclk-fs and
> > the
> > sample rate which might wipe out the original clock frequency if the
> > constraints don't prevent setting an invalid rate.
> 
> If the device is using mclk-fs then either there's a fixed sample rate
> (in which case simple-card probably ought to force it without the driver
> worrying) or the sysclk will vary in which case simple-card should be
> setting the sysclk to 0 when the card goes idle to clear any
> constraints (which as you say later it does).

It sounds like to fix that, simple-card needs to keep track of whether the DAI
has a fixed clock rate or not. If it does, then it shouldn't be zeroing out the
sysclk when closing the device as that's wiping out information that needs to
be persistent. I guess if the frequency was read from a system-clock-frequency
property then we know it is fixed. There are other cases where it could be a
fixed rate, such as if the clock is connected to a fixed-clock. Maybe we need
an explicit DT flag "system-clock-fixed" or something for those cases?

Then at least in the case where mclk-fs is set and one or more of the DAIs have
a fixed rate, simple-card can add a constraint to restrict the sample rate to
the fixed clock divided by mclk-fs?

> 
> > -In the case of xlnx_formatter_pcm and simple-card, since sysclk is
> > determined
> > by sample rate times mclk-fs, there's no way for userspace to violate the
> > constraint that the sample rate divides evenly into sysclk.
> 
> Only on a system where the sysclk can vary - this is a generic card so
> someone could set a fixed sysclk, and of course the driver could be used
> with other cards.
> 
> > -in order to add any constraints on sample rate based on sysclk in the
> > formatter driver, it would need to get set_sysclk called during
> > initialization
> > which doesn't currently happen. But with simple-card, there's no way those
> > constraints could be violated other than a kernel bug.
> 
> You shouldn't be making assumptions about the machine driver in the DMA
> driver, especially for something like this used in a FPGA product which
> has even more flexibility than most things.

That's true, but the constraint of "sample rate must divide into sysclk" in the
formatter seems like one that should always be the case, and shouldn't really
be the responsibility of the low-level driver level to verify but should be
handled by simple-card or whatever other top-level driver.

> 
> > -xlnx_i2s needs some way to avoid its stored sysclk being wiped out during
> > PCM
> > close so that the constraints are handled properly during subsequent opens.
> 
> Depending on how flexible the system is clearing the sysclk stored in
> the I2S driver may be desirable - if the sysclk rate can be changed then
> you usually don't want to force constraints based on what it was during
> the last stream, you want to relax such constraints so that a new sysclk
> can be chosen where appropriate.
> 
> If that's not possible in this system then it sounds like it should be
> setting system-clock-frequency and simple-card should then not be
> clearing any setting in the components when the stream closes down, it
> should be setting the clock up once at init.  Broadly speaking the
> machine driver is responsible for ensuring that the overall system
> configuration is sensible and coherent (that's what it's there for).
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

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

[Mark Brown]

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

On Mon, Jan 10, 2022 at 09:24:38PM +0000, Robert Hancock wrote:
> On Mon, 2022-01-10 at 20:31 +0000, Mark Brown wrote:

> > If the device is using mclk-fs then either there's a fixed sample rate
> > (in which case simple-card probably ought to force it without the driver
> > worrying) or the sysclk will vary in which case simple-card should be
> > setting the sysclk to 0 when the card goes idle to clear any
> > constraints (which as you say later it does).

> It sounds like to fix that, simple-card needs to keep track of whether the DAI
> has a fixed clock rate or not. If it does, then it shouldn't be zeroing out the
> sysclk when closing the device as that's wiping out information that needs to
> be persistent. I guess if the frequency was read from a system-clock-frequency
> property then we know it is fixed. There are other cases where it could be a
> fixed rate, such as if the clock is connected to a fixed-clock. Maybe we need
> an explicit DT flag "system-clock-fixed" or something for those cases?

If the clock is fixed in the clock API we should arrange to be able to
enumerate that from the clock API - we shouldn't be requiring redundant
properties for something like that.

> Then at least in the case where mclk-fs is set and one or more of the DAIs have
> a fixed rate, simple-card can add a constraint to restrict the sample rate to
> the fixed clock divided by mclk-fs?

Yes, however we figure out that sysclk is fixed.

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

[PATCH v2 3/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

[PATCH v2 4/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 v2 5/6] 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 | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index a89d1cfdda32..78419e18717d 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -28,6 +28,30 @@ static const struct snd_soc_ops simple_ops = {
 	.hw_params	= asoc_simple_hw_params,
 };
 
+static int asoc_simple_parse_platform(struct device_node *node,
+				      struct snd_soc_dai_link_component *dlc)
+{
+	struct of_phandle_args args;
+	int ret;
+
+	if (!node)
+		return 0;
+
+	/*
+	 * Get node via "sound-dai = <&phandle port>"
+	 * it will be used as xxx_of_node on soc_bind_dai_link()
+	 */
+	ret = of_parse_phandle_with_args(node, DAI, CELL, 0, &args);
+	if (ret)
+		return ret;
+
+	/* dai_name is not required and may not exist for plat component */
+
+	dlc->of_node = args.np;
+
+	return 0;
+}
+
 static int asoc_simple_parse_dai(struct device_node *node,
 				 struct snd_soc_dai_link_component *dlc,
 				 int *is_single_link)
@@ -289,7 +313,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_platform(plat, platforms);
 	if (ret < 0)
 		goto dai_link_of_err;
 
-- 
2.31.1

[PATCH v2 6/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: (subset) [PATCH v2 0/6] ASoC: Xilinx fixes

[Mark Brown]

On Fri, 7 Jan 2022 15:47:05 -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-linus

Thanks!

[1/6] ASoC: xilinx: xlnx_formatter_pcm: Make buffer bytes multiple of period bytes
      commit: e958b5884725dac86d36c1e7afe5a55f31feb0b2

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

Re: (subset) [PATCH v2 0/6] ASoC: Xilinx fixes

[Mark Brown]

On Fri, 7 Jan 2022 15:47:05 -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-linus

Thanks!

[1/6] ASoC: xilinx: xlnx_formatter_pcm: Make buffer bytes multiple of period bytes
      commit: e958b5884725dac86d36c1e7afe5a55f31feb0b2
[5/6] ASoC: simple-card: fix probe failure on platform component
      commit: a64067f4cecaaa4deed8e33d3266bc0bcc189142

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