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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 1/4/20 5:05 PM, Dmitry Osipenko wrote:
> 04.01.2020 08:49, Sowjanya Komatineni пишет:
>> On 1/2/20 8:12 AM, Dmitry Osipenko wrote:
>>> 02.01.2020 10:03, Sowjanya Komatineni пишет:
>>>> On 12/27/19 1:19 PM, Sowjanya Komatineni wrote:
>>>>> On 12/27/19 6:56 AM, Dmitry Osipenko wrote:
>>>>>> 25.12.2019 20:57, Mark Brown пишет:
>>>>>>> On Mon, Dec 23, 2019 at 12:14:34AM +0300, Dmitry Osipenko wrote:
>>>>>>>> 21.12.2019 01:26, Sowjanya Komatineni пишет:
>>>>>>>>> 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
>>>>>>
>>>>>>>>> -    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.
>>>>>>>> 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.

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.

Sameer, Please confirm.

>> 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.
>>