Re: [PATCH] arm64: dts: add zx296718's topcrm node

[Baoyou Xie <baoyou.xie@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 5219 bytes --]

On 30 November 2016 at 16:26, Jun Nie <jun.nie@linaro.org> wrote:

> 2016-11-30 15:33 GMT+08:00 Baoyou Xie <baoyou.xie@linaro.org>:
> > Enable topcrm clock node for zx296718, which is used for
> > CPU's frequency change.
>
> Please follow general rule, such as
> arm64: dts: zx: brief title of your changes
>
> Sounds good, though some patches didn't do so.


> >
> > Furthermore, this patch adds the CPU clock phandle in CPU's node
> > and uses operating-points-v2 to register operating points.
> >
> > So it can be used by cpufreq-dt driver.
>
> Suggest to split clock nodes and cpu-freq nodes changes into different
> patches.
>
> >
> > Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
> > ---
> >  arch/arm64/boot/dts/zte/zx296718.dtsi | 48
> +++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/zte/zx296718.dtsi
> b/arch/arm64/boot/dts/zte/zx296718.dtsi
> > index 6b239a3..f9eb37d 100644
> > --- a/arch/arm64/boot/dts/zte/zx296718.dtsi
> > +++ b/arch/arm64/boot/dts/zte/zx296718.dtsi
> > @@ -44,6 +44,7 @@
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/clock/zx296718-clock.h>
> >
> >  / {
> >         compatible = "zte,zx296718";
> > @@ -81,6 +82,8 @@
> >                         compatible = "arm,cortex-a53","arm,armv8";
> >                         reg = <0x0 0x0>;
> >                         enable-method = "psci";
> > +                       clocks = <&topcrm A53_GATE>;
> > +                       operating-points-v2 = <&cluster0_opp>;
> >                 };
> >
> >                 cpu1: cpu@1 {
> > @@ -88,6 +91,7 @@
> >                         compatible = "arm,cortex-a53","arm,armv8";
> >                         reg = <0x0 0x1>;
> >                         enable-method = "psci";
> > +                       operating-points-v2 = <&cluster0_opp>;
> >                 };
> >
> >                 cpu2: cpu@2 {
> > @@ -95,6 +99,7 @@
> >                         compatible = "arm,cortex-a53","arm,armv8";
> >                         reg = <0x0 0x2>;
> >                         enable-method = "psci";
> > +                       operating-points-v2 = <&cluster0_opp>;
> >                 };
> >
> >                 cpu3: cpu@3 {
> > @@ -102,6 +107,43 @@
> >                         compatible = "arm,cortex-a53","arm,armv8";
> >                         reg = <0x0 0x3>;
> >                         enable-method = "psci";
> > +                       operating-points-v2 = <&cluster0_opp>;
> > +               };
> > +       };
> > +
> > +       cluster0_opp: opp_table0 {
> > +               compatible = "operating-points-v2";
> > +               opp-shared;
> > +
> > +               opp@1000000000 {
> > +                       opp-hz = /bits/ 64 <500000000>;
>
> Why frequency in opp name differ with opp-hz value?
>
> Do we must keep them same? if don't so, just leave it, OK?


> > +                       opp-microvolt = <857000>;
> > +                       clock-latency-ns = <500000>;
> > +               };
> > +               opp@1100000000 {
> > +                       opp-hz = /bits/ 64 <648000000>;
> > +                       opp-microvolt = <857000>;
> > +                       clock-latency-ns = <500000>;
> > +               };
> > +               opp@1200000000 {
> > +                       opp-hz = /bits/ 64 <800000000>;
> > +                       opp-microvolt = <882000>;
> > +                       clock-latency-ns = <500000>;
> > +               };
> > +               opp@1300000000 {
> > +                       opp-hz = /bits/ 64 <1000000000>;
> > +                       opp-microvolt = <892000>;
> > +                       clock-latency-ns = <500000>;
> > +               };
> > +               opp@1400000000 {
> > +                       opp-hz = /bits/ 64 <1188000000>;
> > +                       opp-microvolt = <1009000>;
> > +                       clock-latency-ns = <500000>;
> > +               };
> > +               opp@1500000000 {
> > +                       opp-hz = /bits/ 64 <1312000000>;
> > +                       opp-microvolt = <1052000>;
> > +                       clock-latency-ns = <500000>;
> >                 };
>
> I did not see frequency 1500000000 or 1312000000 in clock drivers for
> A53. Please confirm whether clock driver need update or your need
> revise your patch.
>
> Oh, it's my mistake. In fact, frequency 1500000000 or 1312000000 can be
removed. These two cases are trial, but we don't suggest using them in
products.


> >         };
> >
> > @@ -279,6 +321,12 @@
> >                         dma-requests = <32>;
> >                 };
> >
> > +               topcrm: clock-controller@01461000 {
>
> Removing heading 0 in address of name.
>
> > +                       compatible = "zte,zx296718-topcrm";
> > +                       reg = <0x01461000 0x1000>;
> > +                       #clock-cells = <1>;
> > +               };
> > +
> >                 sysctrl: sysctrl@1463000 {
> >                         compatible = "zte,zx296718-sysctrl", "syscon";
> >                         reg = <0x1463000 0x1000>;
> > --
> > 2.7.4
> >
>

[-- Attachment #2: Type: text/html, Size: 8346 bytes --]