| Message ID | 20220105225146.3517039-3-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Xilinx fixes | expand |
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.
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,
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(+)