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. 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. > > + > > + 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. [...] -- Thank you, Dmitry
