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
