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. > 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
