Re: [PATCH v2 2/7] dt-bindings: clock: meson: a1: pll: introduce new syspll bindings

[Dmitry Rokosov <ddrokosov@salutedevices.com>]

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