Re: [PATCH v5 12/19] ASoC: tegra: Add initial parent configuration for audio mclk

06.01.2020 19:30, Sowjanya Komatineni пишет:

On 1/6/20 8:09 AM, Sowjanya Komatineni wrote:

quoted

On 1/5/20 8:21 PM, Sameer Pujar wrote:

quoted

On 1/5/2020 10:33 AM, Sowjanya Komatineni wrote:

quoted

On 1/4/20 5:05 PM, Dmitry Osipenko wrote:

quoted

04.01.2020 08:49, Sowjanya Komatineni пишет:

quoted

On 1/2/20 8:12 AM, Dmitry Osipenko wrote:

quoted

02.01.2020 10:03, Sowjanya Komatineni пишет:

quoted

On 12/27/19 1:19 PM, Sowjanya Komatineni wrote:

quoted

On 12/27/19 6:56 AM, Dmitry Osipenko wrote:

quoted

25.12.2019 20:57, Mark Brown пишет:

quoted

On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote:

quoted

21.12.2019 01:26, Sowjanya Komatineni пишет:

quoted

Tegra PMC clock clk_out_1 is dedicated for audio mclk from
Tegra30
through Tegra210 and currently Tegra clock driver does
initial parent
configuration for audio mclk "clk_out_1" and enables them
by default.

Please delete unneeded context from mails when replying. 
Doing this
makes it much easier to find your reply in the message,
helping ensure
it won't be missed by people scrolling through the irrelevant
quoted
material.

Ok

quoted

quoted

quoted

- clk_disable_unprepare(data->clk_cdev1);
- clk_disable_unprepare(data->clk_pll_a_out0);
- clk_disable_unprepare(data->clk_pll_a);
+    if (__clk_is_enabled(data->clk_cdev1))
+ clk_disable_unprepare(data->clk_cdev1);

The root of the problem is that you removed clocks enabling
from
tegra_asoc_utils_init().

currently, audio mclk and its parent clocks enabling are from
clock
driver init and not from tegra_asoc_utils_init.

quoted

quoted

quoted

I'm not sure why clocks should be disabled during the
rate-changing,
probably this action is not really needed.

I know nothing about this particular device but this is not that
unusual a restriction for audio hardware, you often can't
robustly reconfigure the clocking for a device while it's active
due to issues in the hardware.  You often see issues with FIFOs
glitching or state machines getting stuck. This may not be an
issue here but if it's something that's documented as a
requirement it's probably good to pay attention.

I don't know details about that hardware either, maybe it is
simply not
safe to change PLL_A rate dynamically and then
CLK_SET_RATE_GATE could
be used.

If nobody knows for sure, then will be better to keep
tegra_asoc_utils_set_rate() unchanged.

plla rate change through tegra_asoc_utils_set_rate() happens
only when
there is not active playback or record corresponding to this sound
device.

So, I don't see reason for disabling clock during rate change
and not
sure why we had this from the beginning.

Thierry/Sameer,

Can you please comment?

Yes, we can use CLK_SET_RATE_GATE for PLLA and remove clock
disabling
before rate change.

PLLA is used for both I2S controller clock and also for audio
mclk. I2S
driver suspend resume implementations takes care of enabling and
disabling I2S clock but audio mclk will be enabled during that
time and
PLLA disable might not happen. So using CLK_SET_RATE_GATE
prevents rate
change to happen and as rate change happens only when there is
no active
audio record/playback, we can perform rate change without
disable/enable
during rate change.

So probably below changes should be good.

   * remove asoc_utils_set_rate call from asoc_utils_init as
set_rate
     happens during existing hw_params callback implementations
in sound
     drivers and there is no need to do rate change during
asoc_utils_init.
   * remove disable/enable clocks during rate change in
asoc_utils_set_rate.
   * add startup and shutdown snd_soc_ops callbacks to do enable
and
     disable audio mclk.

Sounds good, thanks. I'll be happy to review and test it.

Regarding disabling audio mclk during PLLA rate change, no need to
explicitly disable PLLA on asoc utils as clock driver takes care
of it
properly during pll rate change.

But the downstream clock divider hardware can malfunction without
recovery when subject to unstable PLL output during locking, unless
clock is gated.

So it is recommended to disable downstream clocks during PLL rate
change.

PLLA downstream clocks are I2S and audio mclk (cdev1/clk1 and extern1
clocks) and I2S clock is disabled in I2S driver by PM runtime ops.

The I2S driver uses asynchronous pm_runtime_put() and thus there is no
guarantee that I2S clock is disabled at the time of changing PLLA
rate.
Could this be a problem?

Looking into soc_pcm_hw_params, I see dai_link hw_params ops happens
prior to  platform snd_soc_dai_driver hw_params ops.

This is true.

quoted

So, PLL rate change thru asoc_utils_set_rate happens during sample
rate config of dai_link hw_params ops and during this time I2S will
always be in idle state.

This is probably not the case, since runtime resume for I2S would
have already enabled the clock for I2S and in turn PLLA. The
hw_param() call would happen later.
We could have used a fixed clock rate for PLLA, but the reason why we
are setting the rate at runtime is, we support sample rates (and
multuples) of 8kHz and 11.025kHz.
Both of these require a different PLLA base rate for downstream clock
dividers to work properly. That is why, I think we have two base
rates for PLLA.

Even if we want to enable the clocks (for i2s) in hw_param(), this
still may not help.
For example there could be multiple I2S instances, which can use the
same PLLA source. ALSA playback/capture on different I2S can run
independently.
Hence we are not sure if clk_disable_unprepare() in
tegra_asoc_utils.c would actually disable PLLA. Hence I think the
problem exists in current code too.

clk_disable_unprepare in aosc_utils_set_rate will not disable PLLA as
it will be in use by other consumer (I2S).

But clock driver it self takes care of disabling pll output and
keeping it in bypass state so its not really like PLLA output is off.

So regarding PLLA rate change, we dont have to explicitly disable in
audio driver as pll clock driver takes care during rate update,

But consumer clocks of PLLA need to be disabled during rate update so
existing dividers doesn't cause any malfunction with new rate update.

audio mclk can will be in disabled state during rate update thru
hw_params with addition of shutdown snd_soc_ops callbacks to disable
of audio mclk.

But issue is for I2S clock where clock might be in enabled state and
this is issue with current I2S driver already.

Sameer/Dmitry,

Making sure I2S clock is in disabled state during rate update is
something need to work thru for proper fix and this is not related to
this patch series as this issue exists with current upstream.

So, can we take care of this as separate patch out of this series so I
can get this series out as this PMC clock changes are needed for
upcoming camera drivers.

Based on internal discussion with sameer, proper I2S clock fix will be
take care separately and this is not something introduced with this
patch series.

So, will move once with audio MCLK fix in next version of patch series.

  * remove asoc_utils_set_rate call from asoc_utils_init as set_rate
    happens during hw_params callback based on existing driver
  * add shutdown snd_soc_ops callbacks to disable of audio mclk
  * remove disable clocks prior to rate change in asoc_utils_set_rate
    (as audio mclk will already be in disabled state by the time
    hw_param callback gets executed) and keep audio mclk enable after
    rate change in asoc_utils_set_rate

quoted

quoted

We really want to allow user to run any sample rate in the supported
range. However the sample rate is known during hw_param() callback.

Looking at current discussion, we may have to provide an aternate way
of switching PLLA base rate (may be not in ALSA callbacks)

quoted

Sameer, Please confirm.

quoted

quoted

For audio mclk, need to make sure mclk are disabled during rate
change.

So below are the changes to audio clocks that will be in next
version.

   * remove tegra_asoc_utils_set_rate call from
tegra_asoc_utils_init as
     tegra_asoc_utils_set_rate happens during hw_params callback.
   * add shutdown snd_soc_ops callbacks to disable of audio mclk.
   * remove disable audio mclk (cdev1) and plla clocks prior to rate
     change in tegra_asoc_utils_set_rate (as audio mclk will
always be in
     disabled state every time hw_param callback gets executed)
and keep
     audio mclk enable after the rate change in
tegra_asoc_utils_set_rate.

Indeed, it should be better to factor out all changes that are not
directly related to the PMC patches into separate patchset.