[PATCH v3 0/6] 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.
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(-)
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,
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; }
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);
chan_id--; }writel(chan_id, drv_data->base + reg_off);
@@ -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);
break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:writel(1, drv_data->base + I2S_CORE_CTRL_OFFSET);
writel(0, base + I2S_CORE_CTRL_OFFSET);
break; default: return -EINVAL;writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
@@ -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;
} else if (of_device_is_compatible(node, "xlnx,i2s-receiver-1.0")) {drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
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;
} else { return -ENODEV; }drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
- 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);
if (ret) { dev_err(&pdev->dev, "i2s component registration failed\n"); return ret; }&drv_data->dai_drv, 1);
- 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/incl...
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);
chan_id--; }writel(chan_id, drv_data->base + reg_off);
@@ -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);
break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:writel(1, drv_data->base + I2S_CORE_CTRL_OFFSET);
writel(0, base + I2S_CORE_CTRL_OFFSET);
break; default: return -EINVAL;writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);
@@ -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;
} else if (of_device_is_compatible(node, "xlnx,i2s-receiver-1.0")) {drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
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;
} else { return -ENODEV; }drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
- 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);
if (ret) { dev_err(&pdev->dev, "i2s component registration failed\n"); return ret; }&drv_data->dai_drv, 1);
- 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/...
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).
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);
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)
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
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
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; }
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
participants (4)
-
Amadeusz Sławiński
-
Kuninori Morimoto
-
Mark Brown
-
Robert Hancock