Re: [PATCH v4 13/19] ASoC: tegra: Add fallback implementation for audio mclk

[Dmitry Osipenko]

18.12.2019 19:32, Sowjanya Komatineni ÐÐÑÐÑ:
> 
> On 12/18/19 8:29 AM, Dmitry Osipenko wrote:
>> 18.12.2019 18:43, Sowjanya Komatineni ÐÐÑÐÑ:
>>> On 12/17/19 11:31 PM, Dmitry Osipenko wrote:
>>>> 18.12.2019 10:22, Dmitry Osipenko ÐÐÑÐÑ:
>>>>> 18.12.2019 10:14, Sowjanya Komatineni ÐÐÑÐÑ:
>>>>>> On 12/17/19 11:01 PM, Dmitry Osipenko wrote:
>>>>>>> 18.12.2019 09:59, Dmitry Osipenko ÐÐÑÐÑ:
>>>>>>>> 17.12.2019 23:04, Sowjanya Komatineni ÐÐÑÐÑ:
>>>>>>>>> mclk is from clk_out_1 which is part of Tegra PMC block and pmc
>>>>>>>>> clocks
>>>>>>>>> are moved to Tegra PMC driver with pmc as clock provider and
>>>>>>>>> using pmc
>>>>>>>>> clock ids.
>>>>>>>>>
>>>>>>>>> New device tree uses clk_out_1 from pmc clock provider.
>>>>>>>>>
>>>>>>>>> So, this patch adds implementation for mclk fallback to extern1
>>>>>>>>> when
>>>>>>>>> retrieving mclk returns -ENOENT to be backward compatible of new
>>>>>>>>> device
>>>>>>>>> tree with older kernels.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
>>>>>>>>> ---
>>>>>>>>> ÂÂÂ sound/soc/tegra/tegra_asoc_utils.c | 11 ++++++++++-
>>>>>>>>> ÂÂÂ 1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/sound/soc/tegra/tegra_asoc_utils.c
>>>>>>>>> b/sound/soc/tegra/tegra_asoc_utils.c
>>>>>>>>> index fe9ca8acd0fb..1b88c6043082 100644
>>>>>>>>> --- a/sound/soc/tegra/tegra_asoc_utils.c
>>>>>>>>> +++ b/sound/soc/tegra/tegra_asoc_utils.c
>>>>>>>>> @@ -191,7 +191,16 @@ int tegra_asoc_utils_init(struct
>>>>>>>>> tegra_asoc_utils_data *data,
>>>>>>>>> ÂÂÂÂÂÂÂ data->clk_cdev1 = devm_clk_get(dev, "mclk");
>>>>>>>>> ÂÂÂÂÂÂÂ if (IS_ERR(data->clk_cdev1)) {
>>>>>>>>> ÂÂÂÂÂÂÂÂÂÂÂ dev_err(data->dev, "Can't retrieve clk cdev1\n");
>>>>>>>>> -ÂÂÂÂÂÂÂ return PTR_ERR(data->clk_cdev1);
>>>>>>>>> +ÂÂÂÂÂÂÂ if (PTR_ERR(data->clk_cdev1) != -ENOENT)
>>>>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂ return PTR_ERR(data->clk_cdev1);
>>>>>>>>> +ÂÂÂÂÂÂÂ /* Fall back to extern1 */
>>>>>>>>> +ÂÂÂÂÂÂÂ data->clk_cdev1 = devm_clk_get(dev, "extern1");
>>>>>>>>> +ÂÂÂÂÂÂÂ if (IS_ERR(data->clk_cdev1)) {
>>>>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(data->dev, "Can't retrieve clk extern1\n");
>>>>>>>>> +ÂÂÂÂÂÂÂÂÂÂÂ return PTR_ERR(data->clk_cdev1);
>>>>>>>>> +ÂÂÂÂÂÂÂ }
>>>>>>>>> +
>>>>>>>>> +ÂÂÂÂÂÂÂ dev_err(data->dev, "Falling back to extern1\n");
>>>>>>>>> ÂÂÂÂÂÂÂ }
>>>>>>>>> ÂÂÂ ÂÂÂÂÂ /*
>>>>>>>>>
>>>>>>>> [ÂÂÂ 1.769091] ------------[ cut here ]------------
>>>>>>>> [ÂÂÂ 1.769249] WARNING: CPU: 2 PID: 1 at drivers/clk/clk.c:954
>>>>>>>> clk_core_disable+0xa5/0x1d4
>>>>>>>> [ÂÂÂ 1.769330] clk_out_1 already disabled
>>>>>>>> [ÂÂÂ 1.769459] Modules linked in:
>>>>>>>> [ÂÂÂ 1.769541] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
>>>>>>>> 5.5.0-rc1-next-20191213-00167-g6b9fbcdac8f3-dirty #266
>>>>>>>> [ÂÂÂ 1.769676] Hardware name: NVIDIA Tegra SoC (Flattened Device
>>>>>>>> Tree)
>>>>>>>> [ÂÂÂ 1.769775] [<c010e4bd>] (unwind_backtrace) from [<c010a0fd>]
>>>>>>>> (show_stack+0x11/0x14)
>>>>>>>> [ÂÂÂ 1.769918] [<c010a0fd>] (show_stack) from [<c09a37b1>]
>>>>>>>> (dump_stack+0x85/0x94)
>>>>>>>> [ÂÂÂ 1.770061] [<c09a37b1>] (dump_stack) from [<c011f3d1>]
>>>>>>>> (__warn+0xc1/0xc4)
>>>>>>>> [ÂÂÂ 1.770144] [<c011f3d1>] (__warn) from [<c011f691>]
>>>>>>>> (warn_slowpath_fmt+0x61/0x78)
>>>>>>>> [ÂÂÂ 1.770285] [<c011f691>] (warn_slowpath_fmt) from [<c04a0e7d>]
>>>>>>>> (clk_core_disable+0xa5/0x1d4)
>>>>>>>> [ÂÂÂ 1.770427] [<c04a0e7d>] (clk_core_disable) from [<c04a0fc3>]
>>>>>>>> (clk_core_disable_lock+0x17/0x20)
>>>>>>>> [ÂÂÂ 1.770516] [<c04a0fc3>] (clk_core_disable_lock) from
>>>>>>>> [<c07792bb>]
>>>>>>>> (tegra_asoc_utils_set_rate+0x53/0x208)
>>>>>>>> [ÂÂÂ 1.770662] [<c07792bb>] (tegra_asoc_utils_set_rate) from
>>>>>>>> [<c077b8c5>] (tegra_rt5640_probe+0xd5/0x128)
>>>>>>>> [ÂÂÂ 1.770808] [<c077b8c5>] (tegra_rt5640_probe) from [<c0555eb7>]
>>>>>>>> (platform_drv_probe+0x33/0x68)
>>>>>>>> [ÂÂÂ 1.770958] [<c0555eb7>] (platform_drv_probe) from [<c055471d>]
>>>>>>>> (really_probe+0x14d/0x240)
>>>>>>>> [ÂÂÂ 1.771099] [<c055471d>] (really_probe) from [<c055493f>]
>>>>>>>> (driver_probe_device+0x43/0x11c)
>>>>>>>> [ÂÂÂ 1.771187] [<c055493f>] (driver_probe_device) from [<c0554b25>]
>>>>>>>> (device_driver_attach+0x3d/0x40)
>>>>>>>> [ÂÂÂ 1.771328] [<c0554b25>] (device_driver_attach) from
>>>>>>>> [<c0554b5f>]
>>>>>>>> (__driver_attach+0x37/0x78)
>>>>>>>> [ÂÂÂ 1.771469] [<c0554b5f>] (__driver_attach) from [<c05532fb>]
>>>>>>>> (bus_for_each_dev+0x43/0x6c)
>>>>>>>> [ÂÂÂ 1.771609] [<c05532fb>] (bus_for_each_dev) from [<c0553e0f>]
>>>>>>>> (bus_add_driver+0xe3/0x148)
>>>>>>>> [ÂÂÂ 1.771692] [<c0553e0f>] (bus_add_driver) from [<c055531d>]
>>>>>>>> (driver_register+0x39/0xa0)
>>>>>>>> [ÂÂÂ 1.771833] [<c055531d>] (driver_register) from [<c0102c2f>]
>>>>>>>> (do_one_initcall+0x43/0x1bc)
>>>>>>>> [ÂÂÂ 1.771979] [<c0102c2f>] (do_one_initcall) from [<c1000ce5>]
>>>>>>>> (kernel_init_freeable+0x121/0x194)
>>>>>>>> [ÂÂÂ 1.772129] [<c1000ce5>] (kernel_init_freeable) from
>>>>>>>> [<c09b40e9>]
>>>>>>>> (kernel_init+0xd/0xd0)
>>>>>>>> [ÂÂÂ 1.772215] [<c09b40e9>] (kernel_init) from [<c01010bd>]
>>>>>>>> (ret_from_fork+0x11/0x34)
>>>>>>>> [ÂÂÂ 1.772349] Exception stack(0xde907fb0 to 0xde907ff8)
>>>>>>>>
>>>>>>> Although, that's probably related to the "ASoC: tegra: Add initial
>>>>>>> parent configuration for audio mclk".
>>>>>>>
>>>>>> Actually I see these warnings of already unprepared and already
>>>>>> disabled
>>>>>> for pll_a, pll_a_out0, and clk_out_1 even without this whole patch
>>>>>> series as well.
>>>>>>
>>>>>> I think its from tegra_asoc_utils_set_rate() doing
>>>>>> clk_disable_unprepare
>>>>>> and these clocks are already unprepared and disabled so its just
>>>>>> warning
>>>>>> from clk_core_unprepare and clk_core_disable.
>>>>> Doesn't happen for me without this series.
>>> I looked at wrong log, right earlier clock driver keeps them enabled so
>>> asoc_utils_set_rate() disables the clock fine but now enabling audio
>>> clock should be done in asoc_utils_init() to let the
>>> clk_disable_unprepare from asoc_utils_set_rate not to show this warning.
>>>
>>> But actually we don't need to have clock enabled in asoc_utils_init
>>> prior to invoking asoc_utils_set_rate from utils_init and its just warns
>>> during sound driver probe because clock is already in disabled state. At
>>> same time it doesn't harm to have it kept enabled in utils_init. So will
>>> keep it enabled in asoc_utils_init to prevent this warning to show up.
>>>
>>>> But sound works with both old/new device-trees.
>> The rule of thumb: don't enable anything when it shouldn't be enabled.
>> If clocks are disabled at the time of drivers probe, then drivers
>> shouldn't disable the clocks.
> Will fix asoc_utils_set_rate to disable only when its enabled.

BTW, you should CC ALSA maintainers for the audio patches. Perhaps it's
not worth to CC ALSA's ML since it requires registration and spams on
every reply.