On Mon, Jun 10, 2024 at 01:47:06PM +0200, Jerome Brunet wrote: > On Mon 10 Jun 2024 at 14:18, Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxxxxx> wrote: > > > Hello Jerome, > > > > Thank you for the review! > > > > On Mon, Jun 10, 2024 at 12:04:09PM +0200, Jerome Brunet wrote: > >> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxxxxx> wrote: > >> > >> > Add the documentation and dt bindings for Amlogic A1 CPU clock > >> > controller. > >> > > >> > This controller consists of the general 'cpu_clk' and two main parents: > >> > 'cpu fixed clock' and 'syspll'. The 'cpu fixed clock' is an internal > >> > fixed clock, while the 'syspll' serves as an external input from the A1 > >> > PLL clock controller. > >> > > >> > Signed-off-by: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxxxxx> > >> > Reviewed-by: Rob Herring <robh@xxxxxxxxxx> > >> > --- > >> > .../bindings/clock/amlogic,a1-cpu-clkc.yaml | 64 +++++++++++++++++++ > >> > .../dt-bindings/clock/amlogic,a1-cpu-clkc.h | 19 ++++++ > >> > 2 files changed, 83 insertions(+) > >> > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml > >> > create mode 100644 include/dt-bindings/clock/amlogic,a1-cpu-clkc.h > >> > > >> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml > >> > new file mode 100644 > >> > index 000000000000..f4958b315ed4 > >> > --- /dev/null > >> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml > >> > @@ -0,0 +1,64 @@ > >> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > >> > +%YAML 1.2 > >> > +--- > >> > +$id: http://devicetree.org/schemas/clock/amlogic,a1-cpu-clkc.yaml# > >> > +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> > + > >> > +title: Amlogic A1 CPU Clock Control Unit > >> > + > >> > +maintainers: > >> > + - Neil Armstrong <neil.armstrong@xxxxxxxxxx> > >> > + - Jerome Brunet <jbrunet@xxxxxxxxxxxx> > >> > + - Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxxxxx> > >> > + > >> > +properties: > >> > + compatible: > >> > + const: amlogic,a1-cpu-clkc > >> > + > >> > + '#clock-cells': > >> > + const: 1 > >> > + > >> > + reg: > >> > + maxItems: 1 > >> > + > >> > + clocks: > >> > + items: > >> > + - description: input fixed pll div2 > >> > + - description: input fixed pll div3 > >> > + - description: input sys pll > >> > + - description: input oscillator (usually at 24MHz) > >> > >> According to the documentation, fdiv5 is also an input of the CPU clock > >> tree. > >> > >> That is typically the kind of things we'd prefer to get right from the > >> beginning to avoid modifying the bindings later. > >> > > > > Could you please share which documentation you are referencing? I have > > the A113L documentation, and there is no mention of the CPU clock IP. > > You should get in touch with Amlogic. > Okay, I will double check with Amlogic and back with accurate information. > > I retrieved below register map from the vendor's custom driver: > > > > === > > CPUCTRL_CLK_CTRL0 > > > > bits 1:0 - cpu_fsource_sel0 > > 0 - xtal > > 1 - fclk_div2 > > 2 - fclk_div3 > > > > bit 2 - cpu_fsel0 > > 0 - cpu_fsource_sel0 > > 1 - cpu_fsource_div0 > > > > bit 3 - UNKNONWN > > > > bits 9:4 - cpu_fsource_div0 > > Divider value > > > > bit 10 - cpu_fclk > > 0 - cpu_fsel0 > > 1 - cpu_fsel1 > > > > bit 11 - cpu_clk > > 0 - cpu_fclk > > 1 - sys_pll > > > > bits 15:12 - UNKNONWN > > > > bits 17:16 - cpu_fsource_sel1 > > 0 - xtal > > 1 - fclk_div2 > > 2 - fclk_div3 > > > > bit 18 - cpu_fsel1 > > 0 - cpu_fsource_sel1 > > 1 - cpu_fsource_div1 > > > > bit 19 - UNKNONWN > > > > bits 25:20 - cpu_fsource_div1 > > Divider value > > > > bits 31:26 - UNKNONWN > > === > > > > As you can see it doesn't have any other inputs except fclk_div2, > > fclk_div3, sys_pll and xtal. > > You might not know what to do with it yet, still it is part of the > documentation and should be part of the bindings too > > > > >> > + > >> > + clock-names: > >> > + items: > >> > + - const: fclk_div2 > >> > + - const: fclk_div3 > >> > + - const: sys_pll > >> > + - const: xtal > >> > + > >> > +required: > >> > + - compatible > >> > + - '#clock-cells' > >> > + - reg > >> > + - clocks > >> > + - clock-names > >> > + > >> > +additionalProperties: false > >> > + > >> > +examples: > >> > + - | > >> > + #include <dt-bindings/clock/amlogic,a1-pll-clkc.h> > >> > + apb { > >> > + #address-cells = <2>; > >> > + #size-cells = <2>; > >> > + > >> > + clock-controller@fd000000 { > >> > + compatible = "amlogic,a1-cpu-clkc"; > >> > + reg = <0 0xfd000080 0 0x8>; > >> > >> If reg is <0 0xfd000080 0 0x8> then node name should be clock-controller@fd000080 > >> > > > > Okay, I will fix that example in the next version. > > > > [...] > > -- > Jerome -- Thank you, Dmitry
