Re: [PATCH v1] dt-bindings: clock: convert rockchip,rk3128-cru.txt to YAML

[Johan Jonker]

On 9/12/22 12:51, Krzysztof Kozlowski wrote:
> On 11/09/2022 23:20, Johan Jonker wrote:
>> Convert rockchip,rk3128-cru.txt to YAML.
>>
>> Signed-off-by: Johan Jonker <jbx6244@xxxxxxxxx>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3128-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3128-cru.yaml
>> new file mode 100644
>> index 000000000..03e5d7f0e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3128-cru.yaml
>> @@ -0,0 +1,73 @@
>> +# SPDX-License-Identifier: GPL-2.0
> 

> Can't it be Dual licensed?

That depends on Heiko and Rockchip.
I can produce the patch for it, but I'm not in control whether they reply or not. 

===============

dt-bindings: add bindings for px30 clock controller
@mmind
Elaine Zhang authored and mmind committed on Jul 3, 2018 

dt-bindings: add documentation for rk1108 cru
@shawn1221
@mmind
shawn1221 authored and mmind committed on Nov 16, 2016 

dt-bindings: add documentation of rk3036 clock controller
@acgzx
@mmind
acgzx authored and mmind committed on Nov 23, 2015 

dt-bindings: add documentation for rk3188 clock and reset unit
@mmind
mmind authored and Mike Turquette committed on Jul 13, 2014 

dt-bindings: add documentation of rk3228 clock controller
@JeffyCN
@mmind
JeffyCN authored and mmind committed on Dec 12, 2015 

dt-bindings: add documentation for rk3288 cru
@mmind
mmind authored and Mike Turquette committed on Jul 13, 2014 

dt-bindings: add documentation of rk3668 clock controller
@mmind
@bebarino
mmind authored and bebarino committed on Jul 7, 2015 

dt-bindings: Add bindings for rk3308 clock controller
@finley1226
@mmind
finley1226 authored and mmind committed on Sep 5, 2019 

dt-bindings: add bindings for rk3328 clock controller
@mmind
Elaine Zhang authored and mmind committed on Jan 5, 2017 

dt-bindings: add documentation of rk3668 clock controller
@mmind
@bebarino
mmind authored and bebarino committed on Jul 7, 2015 

dt-bindings: add bindings for rk3399 clock controller
@acgzx
@mmind
acgzx authored and mmind committed on Mar 28, 2016 


> 
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/rockchip,rk3128-cru.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Rockchip RK3126/RK3128 Clock and Reset Unit (CRU)
>> +
>> +maintainers:
>> +  - Elaine Zhang <zhangqing@xxxxxxxxxxxxxx>
>> +  - Heiko Stuebner <heiko@xxxxxxxxx>
>> +
>> +description: |
>> +  The RK3126/RK3128 clock controller generates and supplies clock 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/rk3128-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
>> +    - "ext_i2s"    - external I2S clock  - optional
>> +    - "gmac_clkin" - external GMAC clock - optional
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - rockchip,rk3126-cru
>> +      - rockchip,rk3128-cru
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#clock-cells":
>> +    const: 1
>> +
>> +  "#reset-cells":
>> +    const: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    const: xin24m
> 

> More clocks were mentioned in old binding.

We go for a slow approach.
The man with the hammer syndrome thinks that every hardware needs to described with the DT method.
For clocks we do that in the clock driver with macros and structures.
This works fine for Rockchip. If the need arises further proposals will be submitted.
So thanks, but no thanks!


In U-boot we can't filter clocks with exception for the cru node, so DTOC adds them to dt-plat.c with tpl size increase.
When syncing DT changes must be made all over the place.

static struct dtd_fixed_clock dtv_oscillator = {
	.clock_frequency	= 0x16e3600,
	.clock_output_names	= "xin24m",
};
U_BOOT_DRVINFO(oscillator) = {
	.name		= "fixed_clock",
	.plat		= &dtv_oscillator,
	.plat_size	= sizeof(dtv_oscillator),
	.parent_idx	= -1,
};

static struct dtd_rockchip_rk3066a_cru dtv_clock_controller_at_20000000 = {
	.clocks			= {
			{3, {}},},
	.reg			= {0x20000000, 0x1000},
	.rockchip_grf		= 0x9,
};
U_BOOT_DRVINFO(clock_controller_at_20000000) = {
	.name		= "rockchip_rk3066a_cru",
	.plat		= &dtv_clock_controller_at_20000000,
	.plat_size	= sizeof(dtv_clock_controller_at_20000000),
	.parent_idx	= -1,
};

Comment by Heiko:

In the discussion Stephen had comments about the optional clocks, that
have a circular dependency (xin24m -> cru -> i2c -> rtc -> xin32k -> cru) .
After everyone put in their argument, the discussion itself just stopped.

I've picked up the conversion patches anyway, as this is the status-quo
in terms of modelling clocks on Rockchip, so the yaml conversion doesn't
change anything in either direction and just transforms what is in the
kernel right now into the yaml format which will improve devicetree checks
a lot and also reduce the number of warnings emitted by the checker.

https://lore.kernel.org/linux-rockchip/5781991.QJadu78ljV@phil/

> 
>> +
>> +  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: clock-controller@20000000 {
>> +      compatible = "rockchip,rk3128-cru";
>> +      reg = <0x20000000 0x1000>;
>> +      rockchip,grf = <&grf>;
>> +      #clock-cells = <1>;
>> +      #reset-cells = <1>;
>> +    };
> 
> 
> Best regards,
> Krzysztof