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

Dmitry Rokosov
2024-05-13T09:18:10Z

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