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