Re: [PATCH 1/3] dt-bindings: devfreq: mediatek: add mtk cci devfreq dt-bindings

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/03/2022 13:11, Jia-Wei Chang wrote:
>>
>> Remove "driver Device Tree Bindings". "Devfreq" is Linuxism, so this
>> maybe "bus frequency scaling"? Although later you call the device
>> node
>> as cci.
> 
> Should I use "Binding for MediaTek's Cache Coherent Interconnect (CCI)
> frequency and voltage scaling" as new title?

I just suggested to remove word "bindings" so do not add it again. This
should be a title for hardware.

Now what exactly is it - you should know better than me. :)
"MediaTek's Cache Coherent Interconnect (CCI) frequency and voltage
scaling" sounds good to me, assuming that this is the hardware we talk
here about. :)

> 
>>
>>> +
>>> +maintainers:
>>> +  - Jia-Wei Chang <jia-wei.chang@mediatek.com>
>>> +
>>> +description: |
>>> +  This module is used to create CCI DEVFREQ.
>>> +  The performance will depend on both CCI frequency and CPU
>>> frequency.
>>> +  For MT8186, CCI co-buck with Little core.
>>> +  Contain CCI opp table for voltage and frequency scaling.
>>
>> Half of this description (first and last sentence) does not describe
>> the
>> actual hardware. Please describe hardware, not driver.
> 
> Sure, I will fix it in the next version.
> 
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: "mediatek,mt8186-cci"
>>
>> No need for quotes.
> 
> Sure, I will fix it in the next version.
> 
>>
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description:
>>> +          The first one is the multiplexer for clock input of CPU
>>> cluster.
>>> +      - description:
>>> +          The other is used as an intermediate clock source when
>>> the original
>>> +          CPU is under transition and not stable yet.
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: "cci"
>>> +      - const: "intermediate"
>>
>> No need for quotes.
> 
> Sure, I will fix it in the next version.
> 
>>
>>> +
>>> +  operating-points-v2:
>>> +    description:
>>> +      For details, please refer to
>>> +      Documentation/devicetree/bindings/opp/opp-v2.yaml
>>> +
>>> +  opp-table: true
>>
>> Same comments as your CPU freq bindings apply.
> 
> mtk-cci-devfreq is a new driver and its arch is same as mediatek-
> cpufreq so that the properties of mtk-cci are refer to mediatek-cpufreq 
> bindings.
> operating-point-v2 is used to determine the voltage and frequency of
> dvfs which is further utilized by mtk-cci-devfreq.

"operating-point-v2" is understood, but the same as in cpufreq bindings,
I am questioning why do you have "opp-table: true". It's a bit
confusing, so maybe I miss something?

> 
>>
>>> +
>>> +  proc-supply:
>>> +    description:
>>> +      Phandle of the regulator for CCI that provides the supply
>>> voltage.
>>> +
>>> +  sram-supply:
>>> +    description:
>>> +      Phandle of the regulator for sram of CCI that provides the
>>> supply
>>> +      voltage. When present, the cci devfreq driver needs to do
>>> +      "voltage tracking" to step by step scale up/down Vproc and
>>> Vsram to fit
>>> +      SoC specific needs. When absent, the voltage scaling flow is
>>> handled by
>>> +      hardware, hence no software "voltage tracking" is needed.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - clocks
>>> +  - clock-names
>>> +  - operating-points-v2
>>> +  - proc-supply
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/mt8186-clk.h>
>>> +    cci: cci {
>>
>> Node names should be generic and describe type of device. Are you
>> sure
>> this is a CCI? Maybe "interconnect" suits it better?
> 
> Yes, this is a CCI and it is generic type of device like CPU in my
> opinion.
> If my understanding is correct, CCI is more suitable.

OK.

> 
>>
>>> +      compatible = "mediatek,mt8186-cci";
>>> +      clocks = <&mcusys CLK_MCU_ARMPLL_BUS_SEL>, <&apmixedsys
>>> CLK_APMIXED_MAINPLL>;
>>> +      clock-names = "cci", "intermediate";
>>> +      operating-points-v2 = <&cci_opp>;
>>> +      proc-supply = <&mt6358_vproc12_reg>;
>>> +      sram-supply = <&mt6358_vsram_proc12_reg>;
>>> +    };
>>
>>
>> Best regards,
>> Krzysztof
> 


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel