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

[Mark Brown]

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).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20220110/7f2e1de2/attachment.sig>