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@xxxxxxxxxxxx>
>>> +
>>> +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
