Date:   Thu, 1 Dec 2022 10:42:08 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Trevor Wu (吳文良) <Trevor.Wu@...iatek.com>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "broonie@...nel.org" <broonie@...nel.org>
Cc:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 03/12] ASoC: mediatek: mt8188: support audsys clock

Il 01/12/22 09:43, Trevor Wu (吳文良) ha scritto:
> On Wed, 2022-10-26 at 10:18 +0200, AngeloGioacchino Del Regno wrote:
>> Il 26/10/22 06:10, Trevor Wu (吳文良) ha scritto:
>>> On Tue, 2022-10-25 at 12:18 +0200, AngeloGioacchino Del Regno
>>> wrote:
>>>> Il 21/10/22 11:58, Trevor Wu (吳文良) ha scritto:
>>>>> On Fri, 2022-10-21 at 10:41 +0200, AngeloGioacchino Del Regno
>>>>> wrote:
>>>>>> Il 21/10/22 10:27, Trevor Wu ha scritto:
>>>>>>> Add mt8188 audio cg clock control. Audio clock gates are
>>>>>>> registered
>>>>>>> to CCF
>>>>>>> for reference count and clock parent management.
>>>>>>>
>>>>>>> Signed-off-by: Trevor Wu <trevor.wu@...iatek.com>
>>>>>>> ---
>>>>>>>      sound/soc/mediatek/mt8188/mt8188-audsys-clk.c | 206
>>>>>>> ++++++++++++++++++
>>>>>>>      sound/soc/mediatek/mt8188/mt8188-audsys-clk.h |  15 ++
>>>>>>>      .../soc/mediatek/mt8188/mt8188-audsys-clkid.h |  83
>>>>>>> +++++++
>>>>>>>      3 files changed, 304 insertions(+)
>>>>>>>      create mode 100644 sound/soc/mediatek/mt8188/mt8188-
>>>>>>> audsys-
>>>>>>> clk.c
>>>>>>>      create mode 100644 sound/soc/mediatek/mt8188/mt8188-
>>>>>>> audsys-
>>>>>>> clk.h
>>>>>>>      create mode 100644 sound/soc/mediatek/mt8188/mt8188-
>>>>>>> audsys-
>>>>>>> clkid.h
>>>>>>>
>>>>>>> diff --git a/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c
>>>>>>> b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..1f294231d4c2
>>>>>>> --- /dev/null
>>>>>>> +++ b/sound/soc/mediatek/mt8188/mt8188-audsys-clk.c
>>>>>>> @@ -0,0 +1,206 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>> +/*
>>>>>>> + * mt8188-audsys-clk.c  --  MediaTek 8188 audsys clock
>>>>>>> control
>>>>>>> + *
>>>>>>> + * Copyright (c) 2022 MediaTek Inc.
>>>>>>> + * Author: Chun-Chia Chiu <chun-chia.chiu@...iatek.com>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <linux/clk.h>
>>>>>>> +#include <linux/clk-provider.h>
>>>>>>> +#include <linux/clkdev.h>
>>>>>>> +#include "mt8188-afe-common.h"
>>>>>>> +#include "mt8188-audsys-clk.h"
>>>>>>> +#include "mt8188-audsys-clkid.h"
>>>>>>> +#include "mt8188-reg.h"
>>>>>>> +
>>>>>>> +struct afe_gate {
>>>>>>> +	int id;
>>>>>>> +	const char *name;
>>>>>>> +	const char *parent_name;
>>>>>>> +	int reg;
>>>>>>> +	u8 bit;
>>>>>>> +	const struct clk_ops *ops;
>>>>>>> +	unsigned long flags;
>>>>>>> +	u8 cg_flags;
>>>>>>> +};
>>>>>>> +
>>>>>>> +#define GATE_AFE_FLAGS(_id, _name, _parent, _reg, _bit,
>>>>>>> _flags,
>>>>>>> _cgflags) {\
>>>>>>> +		.id = _id,					
>>>>>>> \
>>>>>>> +		.name = _name,					
>>>>>>> \
>>>>>>> +		.parent_name = _parent,				
>>>>>>> \
>>>>>>> +		.reg = _reg,					
>>>>>>> \
>>>>>>> +		.bit = _bit,					
>>>>>>> \
>>>>>>> +		.flags = _flags,				
>>>>>>> \
>>>>>>> +		.cg_flags = _cgflags,				
>>>>>>> \
>>>>>>> +	}
>>>>>>> +
>>>>>>> +#define GATE_AFE(_id, _name, _parent, _reg, _bit)		
>>>>>>> \
>>>>>>> +	GATE_AFE_FLAGS(_id, _name, _parent, _reg, _bit,		
>>>>>>> \
>>>>>>> +		       CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>>>>>>> CLK_GATE_SET_TO_DISABLE)
>>>>>>
>>>>>> Can you please explain what's the reason for
>>>>>> CLK_IGNORE_UNUSED
>>>>>> here?
>>>>>> Maybe we can solve some issue that you're facing in a cleaner
>>>>>> way.
>>>>>>
>>>>>> Regards,
>>>>>> Angelo
>>>>>
>>>>> Hi Angelo,
>>>>>
>>>>> Because clk_disable_unused() calls clk_core_is_enabled(),
>>>>> register
>>>>> access happens in is_enabled() ops.
>>>>> At the moment, the power for register access is not enabled, so
>>>>> the
>>>>> register read results in CPU hang.
>>>>>
>>>>> That's why I added CLK_IGNORE_UNUSED here, but it can't resolve
>>>>> all
>>>>> issues. Actually, we met same problem when "cat
>>>>> /sys/kernel/debug/clk/clk_summary" is used. We are still
>>>>> suffering
>>>>> the
>>>>> problem.
>>>>>
>>>>> I'm not sure if I can implement clk ops by myself, and exclude
>>>>> the
>>>>> registration of is_enabled() ops.
>>>>>
>>>>
>>>> Is the power for register access enabled with a power domain?
>>>>
>>>> Check drivers/clk/clk.c, grep for core->rpm_enabled.
>>>>
>>>> If you enable runtime PM before registering the clocks, and you
>>>> register them
>>>> with the right struct device, the clock API will enable power for
>>>> you
>>>> before
>>>> trying to read the clock enable status.
>>>>
>>>> Regards,
>>>> Angelo
>>>>
>>>
>>> Hi Angelo,
>>>
>>> I tried the way in MT8195, but it caused circular lock problem.
>>>
>>> Because mtcmos depends on some clocks, clk_bulk_prepare_enable is
>>> also
>>> used in scpsys_power_on()[1].
>>> If the clock also depends on the power domain, this results in the
>>> circular lock problem.
>>> That's why I don't bind the power domain with these clocks.
>>>
>>
>> This is not supposed to happen... can you please give me a (MT8195)
>> patch to
>> reproduce the issue that you're seeing?
>>
>> I would like to investigate that to check if I can come up with a
>> good solution.
>>
>> Thanks,
>> Angelo
> 
> 
> Hi Angelo,
> 
> Sorry for replying late.
> The original implementation about clock depending on power domain was a
> cusotomized request, and it's not based on upstream code base. So I
> can't apply the implementation directly. I tried to implement the
> suggested solution in upstream code, but I can't reproduce the problem
> successfully.
> 
> At the same time, we reviewed the difference between MT8195 and MT8188.
> It's found that ADSP_INFRA should be kept ON to resolve the register
> r/w access limitation in MT8188, so we can match the hardware design in
> MT8195.
> 
> After discussing internally, we decided in favour of ADSP_INFRA
> soloution. Althought the lock problem can't be seen, the new lock
> relationship(prepare_lock -> genpd lock) is actually created.
> 
> In conclusion, ADSP_INFRA will be kept always on and I will remove
> CLK_IGNORE_UNUSED flag in V3.
> 

As far as the power consumption increase is ignorable (so, power consumption
is very little more), that's a good decision and I support that.

Regards,
Angelo

> Thanks,
> Trevor
> 
>>
>>> [1]
>>>
> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.1-rc2/source/drivers/soc/mediatek/mtk-pm-domains.c__;!!CTRNKA9wMg0ARbw!yVFCD-B4VZOxDXTGgDtpB0mJbVoY9tHODeICxthAC33lXMq6LRVTGS-4V-Dj129_cA$
>>>   
>>>
>>> Thanks,
>>> Trevor
>>>
>>>
>>
>>
>>

Powered by blists - more mailing lists