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

[Jia-Wei Chang]

On Thu, 2022-03-24 at 13:44 +0100, Krzysztof Kozlowski wrote:
> 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.

Sure, I will remove the word "bindings" from title.

> 
> 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. :)

Appreciate your comments.
It's a bit hard to do upstream at first time, thank you for
understanding.

> 
> > 
> > > 
> > > > +
> > > > +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?

Yes, you're correct.
"opp-table: true" should be removed.
I messed it up.

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