Received-SPF: pass (google.com: domain of linux-kernel+bounces-177398-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 13 May 2024 12:18:02 +0300
From: Dmitry Rokosov <ddrokosov@salutedevices.com>
To: Conor Dooley <conor@kernel.org>
CC: <neil.armstrong@linaro.org>, <jbrunet@baylibre.com>,
	<mturquette@baylibre.com>, <sboyd@kernel.org>, <robh+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <khilman@baylibre.com>,
	<martin.blumenstingl@googlemail.com>, <jian.hu@amlogic.com>,
	<kernel@sberdevices.ru>, <rockosov@gmail.com>,
	<linux-amlogic@lists.infradead.org>, <linux-clk@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 2/7] dt-bindings: clock: meson: a1: pll: introduce new
 syspll bindings
Message-ID: <20240513091735.l3uewwzfrdd6qwbl@CAB-WSD-L081021>
References: <20240510090933.19464-1-ddrokosov@salutedevices.com>
 <20240510090933.19464-3-ddrokosov@salutedevices.com>
 <20240511-secret-barcode-e25c722ddf1d@spud>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240511-secret-barcode-e25c722ddf1d@spud>
User-Agent: NeoMutt/20220415

Hello Conor,

Thank you for quick review!

On Sat, May 11, 2024 at 02:08:03PM +0100, Conor Dooley wrote:
> On Fri, May 10, 2024 at 12:08:54PM +0300, Dmitry Rokosov wrote:
> > The 'syspll' PLL is a general-purpose PLL designed specifically for the
> > CPU clock. It is capable of producing output frequencies within the
> > range of 768MHz to 1536MHz.
> > 
> > The clock source sys_pll_div16, being one of the GEN clock parents,
> > plays a crucial role and cannot be tagged as "optional". Unfortunately,
> > it was not implemented earlier due to the cpu clock ctrl driver's
> > pending status on the TODO list.
> 
> It's fine to not mark it optional in the binding, but it should be
> optional in the driver as otherwise backwards compatibility will be
> broken. Given this is an integral clock driver, sounds like it would
> quite likely break booting on these devices if the driver doesn't treat
> syspll_in as optional.
> A lesson perhaps in describing the hardware entirely, even if the
> drivers don't make use of all the information yet?

Yes, it's definitely the right lesson for me. However, without syspll or
syspll_in, we cannot utilize CPU power management at all. I will attempt
to make it an optional feature on the driver side, but it might
necessitate additional conditions to disable CPU clock handling when
syspll is unavailable.

> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  .../devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml     | 7 +++++--
> >  include/dt-bindings/clock/amlogic,a1-pll-clkc.h            | 2 ++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > index a59b188a8bf5..fbba57031278 100644
> > --- a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > @@ -26,11 +26,13 @@ properties:
> >      items:
> >        - description: input fixpll_in
> >        - description: input hifipll_in
> > +      - description: input syspll_in
> >  
> >    clock-names:
> >      items:
> >        - const: fixpll_in
> >        - const: hifipll_in
> > +      - const: syspll_in
> >  
> >  required:
> >    - compatible
> > @@ -53,7 +55,8 @@ examples:
> >              reg = <0 0x7c80 0 0x18c>;
> >              #clock-cells = <1>;
> >              clocks = <&clkc_periphs CLKID_FIXPLL_IN>,
> > -                     <&clkc_periphs CLKID_HIFIPLL_IN>;
> > -            clock-names = "fixpll_in", "hifipll_in";
> > +                     <&clkc_periphs CLKID_HIFIPLL_IN>,
> > +                     <&clkc_periphs CLKID_SYSPLL_IN>;
> > +            clock-names = "fixpll_in", "hifipll_in", "syspll_in";
> >          };
> >      };
> > diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> > index 2b660c0f2c9f..a702d610589c 100644
> > --- a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> > +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> > @@ -21,5 +21,7 @@
> >  #define CLKID_FCLK_DIV5		8
> >  #define CLKID_FCLK_DIV7		9
> >  #define CLKID_HIFI_PLL		10
> > +#define CLKID_SYS_PLL		11
> > +#define CLKID_SYS_PLL_DIV16	12
> >  
> >  #endif /* __A1_PLL_CLKC_H */
> > -- 
> > 2.43.0
> > 
> > 


-- 
Thank you,
Dmitry