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@xxxxxxxxxxxxxxxxx> > > --- > > .../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
