Re: [PATCH v3 6/7] dt-bindings: clock: meson: add A1 CPU clock controller bindings

Jerome Brunet
2024-06-10T11:47:09Z

On Mon 10 Jun 2024 at 14:18, Dmitry Rokosov <ddrokosov@salutedevices.com> 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@salutedevices.com> 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@salutedevices.com>
>> > Reviewed-by: Rob Herring <robh@kernel.org>
>> > ---
>> > .../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@linaro.org>
>> > + - Jerome Brunet <jbrunet@baylibre.com>
>> > + - Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > +
>> > +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