[PATCH v2 2/6] ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting

[Robert Hancock]

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