[PATCH 0/5] ASoC: Xilinx fixes
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(-)
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; }
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.
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,
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.
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.
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.
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; }
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);
writel(0, base + I2S_CORE_CTRL_OFFSET);
writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
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.
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);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.
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.
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;
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().
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)
participants (2)
-
Mark Brown -
Robert Hancock