Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bf62ad40-6bcf-62ae-f56a-cdc8d17456ec@gmail.com>
Date:   Thu, 24 Mar 2022 20:51:36 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3188-cru.txt to
 YAML
Content-Language: en-US
To:     Krzysztof Kozlowski <krzk@kernel.org>, heiko@sntech.de
Cc:     robh+dt@kernel.org, krzk+dt@kernel.org, mturquette@baylibre.com,
        sboyd@kernel.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20220324133229.24035-1-jbx6244@gmail.com>
 <f7493d93-6c8a-efa9-1f2c-a0003a6d43b2@kernel.org>
From:   Johan Jonker <jbx6244@gmail.com>
In-Reply-To: <f7493d93-6c8a-efa9-1f2c-a0003a6d43b2@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Heiko, Krzysztof,

Question for the Rockchip clock maintainer:
What clock should be used here and other SoCs with several clock parents
in the tree?

The clock.yaml produces a lot off notifications like:

/arch/arm/boot/dts/rk3036-evb.dtb: clock-controller@20000000: 'clocks'
is a dependency of 'assigned-clocks'

Same for power-controller:

/arch/arm/boot/dts/rk3288-evb-rk808.dtb: power-controller: 'clocks' is a
dependency of 'assigned-clocks'

Just followed rk3399.yaml that got approved recent.
The current CRU nodes are in use for a long time this way.
Converting a simple .txt comes with a lot of extra work each time. :\
Please advise.

Johan

===
dependencies:
  assigned-clocks: [clocks]

ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make dtbs_check
DT_SCHEMA_FILES=~/.local/lib/python3.7/site-packages/dtschema/schemas/clock/clock.yaml

===

See /Documentation/devicetree/bindings/clock/ grep for "assigned":

From cirrus,lochnagar.yaml:

  clocks: true
  assigned-clocks: true
  assigned-clock-parents: true

Is this a possible solution?
So we we don't have to be specific.


On 3/24/22 15:28, Krzysztof Kozlowski wrote:
> On 24/03/2022 14:32, Johan Jonker wrote:
>> Current dts files with RK3188/RK3066 'cru' nodes are manually verified.
>> In order to automate this process rockchip,rk3188-cru.txt has to be
>> converted to YAML.
>>
>> Changed:
>>   Add properties to fix notifications by clocks.yaml for example:
>>     clocks
>>     assigned-clock-rates
>>     assigned-clocks
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>  .../bindings/clock/rockchip,rk3188-cru.txt    | 61 --------------
>>  .../bindings/clock/rockchip,rk3188-cru.yaml   | 81 +++++++++++++++++++
>>  2 files changed, 81 insertions(+), 61 deletions(-)
>>  delete mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
>>  create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
>> deleted file mode 100644
>> index 7f368530a..000000000
>> --- a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.txt
>> +++ /dev/null
>> @@ -1,61 +0,0 @@
>> -* Rockchip RK3188/RK3066 Clock and Reset Unit
>> -
>> -The RK3188/RK3066 clock controller generates and supplies clock to various
>> -controllers within the SoC and also implements a reset controller for SoC
>> -peripherals.
>> -
>> -Required Properties:
>> -
>> -- compatible: should be "rockchip,rk3188-cru", "rockchip,rk3188a-cru" or
>> -			"rockchip,rk3066a-cru"
>> -- reg: physical base address of the controller and length of memory mapped
>> -  region.
>> -- #clock-cells: should be 1.
>> -- #reset-cells: should be 1.
>> -
>> -Optional Properties:
>> -
>> -- rockchip,grf: phandle to the syscon managing the "general register files"
>> -  If missing pll rates are not changeable, due to the missing pll lock status.
>> -
>> -Each clock is assigned an identifier and client nodes can use this identifier
>> -to specify the clock which they consume. All available clocks are defined as
>> -preprocessor macros in the dt-bindings/clock/rk3188-cru.h and
>> -dt-bindings/clock/rk3066-cru.h headers and can be used in device tree sources.
>> -Similar macros exist for the reset sources in these files.
>> -
>> -External clocks:
>> -
>> -There are several clocks that are generated outside the SoC. It is expected
>> -that they are defined using standard clock bindings with following
>> -clock-output-names:
>> - - "xin24m" - crystal input - required,
>> - - "xin32k" - rtc clock - optional,
>> - - "xin27m" - 27mhz crystal input on rk3066 - optional,
>> - - "ext_hsadc" - external HSADC clock - optional,
>> - - "ext_cif0" - external camera clock - optional,
>> - - "ext_rmii" - external RMII clock - optional,
>> - - "ext_jtag" - externalJTAG clock - optional
>> -
>> -Example: Clock controller node:
>> -
>> -	cru: cru@20000000 {
>> -		compatible = "rockchip,rk3188-cru";
>> -		reg = <0x20000000 0x1000>;
>> -		rockchip,grf = <&grf>;
>> -
>> -		#clock-cells = <1>;
>> -		#reset-cells = <1>;
>> -	};
>> -
>> -Example: UART controller node that consumes the clock generated by the clock
>> -  controller:
>> -
>> -	uart0: serial@10124000 {
>> -		compatible = "snps,dw-apb-uart";
>> -		reg = <0x10124000 0x400>;
>> -		interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
>> -		reg-shift = <2>;
>> -		reg-io-width = <1>;
>> -		clocks = <&cru SCLK_UART0>;
>> -	};
>> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
>> new file mode 100644
>> index 000000000..136a9771e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml
>> @@ -0,0 +1,81 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/rockchip,rk3188-cru.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Rockchip RK3188/RK3066 Clock and Reset Unit (CRU)
>> +
>> +maintainers:
>> +  - Heiko Stuebner <heiko@sntech.de>
>> +
>> +description: |
>> +  The RK3188/RK3066 clock controller generates and supplies clocks to various
>> +  controllers within the SoC and also implements a reset controller for SoC
>> +  peripherals.
>> +  Each clock is assigned an identifier and client nodes can use this identifier
>> +  to specify the clock which they consume. All available clocks are defined as
>> +  preprocessor macros in the dt-bindings/clock/rk3188-cru.h and
>> +  dt-bindings/clock/rk3066-cru.h headers and can be used in device tree sources.
>> +  Similar macros exist for the reset sources in these files.
>> +  There are several clocks that are generated outside the SoC. It is expected
>> +  that they are defined using standard clock bindings with following
>> +  clock-output-names:
>> +    - "xin24m"    - crystal input                 - required
>> +    - "xin32k"    - RTC clock                     - optional
>> +    - "xin27m"    - 27mhz crystal input on RK3066 - optional
>> +    - "ext_hsadc" - external HSADC clock          - optional
>> +    - "ext_cif0"  - external camera clock         - optional
>> +    - "ext_rmii"  - external RMII clock           - optional
>> +    - "ext_jtag"  - external JTAG clock           - optional
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - rockchip,rk3066a-cru
>> +      - rockchip,rk3188-cru
>> +      - rockchip,rk3188a-cru
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#clock-cells":
>> +    const: 1
>> +
>> +  "#reset-cells":
>> +    const: 1
>> +
>> +  clocks:
>> +    minItems: 1
> 

> You need maxItems. Would be also nice to describe what should be here as
> input

Just followed rockchip,rk3399-cru.yaml that got rob+dt's ack.

Is this dependency only for simple clock consumers suitable for complex
clock trees with pll's, mux, etc? Shouldn't that be relaxed a bit?

> 
> 
>> +
>> +  assigned-clock-rates:
>> +    minItems: 1
>> +    maxItems: 64
>> +
>> +  assigned-clocks:
>> +    minItems: 1
>> +    maxItems: 64
> 
> Both assigned-xxx should not be necessary. Did you try validate the dtbs
> without these?

That's how it's currently done for Rockchip cru's.


ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/rockchip,rk3188-cru.yaml

> 
> I guess you added "clocks" above because of these, so you need to
> correct the DTS because asigned-xxx depend on clocks property.

See comment above.

> 
>> +
>> +  rockchip,grf:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to the syscon managing the "general register files" (GRF),
>> +      if missing pll rates are not changeable, due to the missing pll lock status.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#clock-cells"
>> +  - "#reset-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    cru: cru@20000000 {
> 
> Node name: clock-controller

Which schema?

> 
>> +      compatible = "rockchip,rk3188-cru";
>> +      reg = <0x20000000 0x1000>;
>> +      rockchip,grf = <&grf>;
>> +      #clock-cells = <1>;
>> +      #reset-cells = <1>;
>> +    };
> 
> 
> Best regards,
> Krzysztof