Re: [PATCH V4 01/14] dt-bindings: cpufreq: mediatek: Add MediaTek CCI property

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 25/04/2022 08:19, Rex-BC Chen wrote:
> On Fri, 2022-04-22 at 19:34 +0200, Krzysztof Kozlowski wrote:
>> On 22/04/2022 19:26, Krzysztof Kozlowski wrote:
>>> On 22/04/2022 09:52, Rex-BC Chen wrote:
>>>> MediaTek Cache Coherent Interconnect (CCI) uses software devfreq
>>>> module
>>>> for scaling clock frequency and adjust voltage.
>>>> The phandle could be linked between CPU and MediaTek CCI for some
>>>> MediaTek SoCs, like MT8183 and MT8186.
>>>> Therefore, we add this property in cpufreq-mediatek.txt.
>>>>
>>>> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
>>>> ---
>>>>  .../devicetree/bindings/cpufreq/cpufreq-mediatek.txt         | 5
>>>> +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>>> mediatek.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>>> mediatek.txt
>>>> index b8233ec91d3d..3387e1e2a2df 100644
>>>> --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>>> mediatek.txt
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
>>>> mediatek.txt
>>>> @@ -20,6 +20,11 @@ Optional properties:
>>>>  	       Vsram to fit SoC specific needs. When absent, the
>>>> voltage scaling
>>>>  	       flow is handled by hardware, hence no software "voltage
>>>> tracking" is
>>>>  	       needed.
>>>> +- mediatek,cci:
>>>> +	MediaTek Cache Coherent Interconnect (CCI) uses the software
>>>> devfreq module to
>>>> +	scale the clock frequency and adjust the voltage.
>>>
>>> Devfreq is a SW mechanism, it should not be part of bindings
>>> description.
> 
> Hello Krzysztof,
> 
> The reason we want to get the "mediatek,cci":
> We need to check the mediatek cci is ready and probed done.
> Because cpufreq and mediatek cci are sharing the same regulator in
> little core cpus.
> Therefore, to prevent high frequency low voltage issue, we need to make
> sure the mediatek cci is ready.
> 
> If mediatek cci is ready, cpufreq and mediatek cci will register the
> same regulator and from regulator's implementation, if there are two
> device using the same regulator, the framwork will make sure it's using
> the max voltage.

Thanks for explanation. The property should be described with what you
said here. The property and description should match hardware, so there
is no place for devfreq. Instead mention that power rail is shared or
voltage regulators are common.

However I am not sure if you solved your problem... see below:

> For example:
> mediatek cci set 1.2V originally. When cpufreq want to adjust lower
> frequency adn set voltage to 1.0V.
> The framework will remain using 1.2V to prevent crash of mediatek cci.

No, regulator_set_voltage() for proc_reg says:
"NOTE: If the regulator is shared between several devices then the lowest
 request voltage that meets the system constraints will be used."

Not the highest. So when your devfreq and cpufreq boots, calling
regulator_set_voltage will still cause high frequency and low voltage.

> 
> Therefore, we need to confirm the mediatek cci is ready and register
> the regulator.
>
>>>
>>>> +	For details, please refer to
>>>> +	Documentation/devicetree/bindings/interconnect/mediatek,cci.yam
>>>> l
>>>
>>> Since the file does not exist, I have troubles reviewing it. First
>>> of
>>> all, you already have "mediatek,cci-control" property in DT, so why
>>> using different name?
> 
> I am not sure where is "mediatek,cci-control". I think this name is not
> used before.
> 

Documentation/devicetree/bindings/net/mediatek-net.txt

>>>
>>> Second, it looks like you want to put devfreq into bindings instead
>>> of
>>> using proper interconnect bindings.
>>
>> Actually judging by the driver this looks like some
>> device-boot-time-ordering, so I wonder whether this is a proper way
>> to
>> express it.
> 
> Yes, we need to get the mediatek cci node and let cpufreq and mediatek
> cci link succefully. In that case, we can know the mediatek cci is
> ready. And we can set the voltage using the regulator framwork.
> 
> [1]: 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20220422075239.16437-11-rex-bc.chen@mediatek.com/

Yes, I see the use case. I am not convinced yet whether this is proper
approach...


Best regards,
Krzysztof