Message-ID: <359bf92f-2e67-28b2-8412-9e47bb24ffdb@nvidia.com>
Date:   Mon, 6 Jan 2020 09:51:41 +0530
From:   Sameer Pujar <spujar@...dia.com>
To:     Sowjanya Komatineni <skomatineni@...dia.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Mark Brown <broonie@...nel.org>
CC:     <thierry.reding@...il.com>, <jonathanh@...dia.com>,
        <lgirdwood@...il.com>, <perex@...ex.cz>, <tiwai@...e.com>,
        <mperttunen@...dia.com>, <gregkh@...uxfoundation.org>,
        <sboyd@...nel.org>, <robh+dt@...nel.org>, <mark.rutland@....com>,
        <pdeschrijver@...dia.com>, <pgaikwad@...dia.com>,
        <josephl@...dia.com>, <daniel.lezcano@...aro.org>,
        <mmaddireddy@...dia.com>, <markz@...dia.com>,
        <devicetree@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 12/19] ASoC: tegra: Add initial parent configuration
 for audio mclk


On 1/5/2020 10:33 AM, Sowjanya Komatineni wrote:
>
> 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.

This is true.

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

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)

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

Powered by blists - more mailing lists