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
