Received-SPF: pass (google.com: domain of linux-kernel+bounces-57851-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 8 Feb 2024 12:22:52 +0200
From: Tony Lindgren <tony@atomide.com>
To: Rob Herring <robh@kernel.org>
Cc: Andreas Kemnade <andreas@kemnade.info>, mturquette@baylibre.com,
	sboyd@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	conor+dt@kernel.org, kristo@kernel.org, linux-clk@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-omap@vger.kernel.org
Subject: Re: [PATCH] dt-bindings: clock: ti: Convert interface.txt to
 json-schema
Message-ID: <20240208102252.GB52537@atomide.com>
References: <20231127202359.145778-1-andreas@kemnade.info>
 <20231128171647.GA3343123-robh@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231128171647.GA3343123-robh@kernel.org>

Hi,

* Rob Herring <robh@kernel.org> [231128 17:16]:
> On Mon, Nov 27, 2023 at 09:23:59PM +0100, Andreas Kemnade wrote:
> > Convert the OMAP interface clock device tree binding to json-schema
> > and fix up reg property which is optional and taken from parent if
> > not specified.
> > Specify the creator of the original binding as a maintainer.
> 
> Great! This and other TI clocks are at the top of the list[1] of 
> occurrences of undocumented (by schemas) compatibles: 
> 
>    3763 ['ti,omap3-interface-clock']
>    3249 ['ti,divider-clock']
>    1764 ['ti,mux-clock']
>    1680 ['ti,gate-clock']
>    1522 ['ti,wait-gate-clock']
>    1459 ['ti,composite-clock']
>    1343 ['ti,composite-mux-clock']
>    1341 ['ti,clkctrl']
>    1296 ['fsl,imx6q-ssi', 'fsl,imx51-ssi']
>    1196 ['ti,composite-gate-clock']
>    1032 ['ti,clockdomain']
> 
> Of course, that's largely due to OMAP being early clock adopter and 
> trying to do fine-grained clocks in DT.

So related to dealing with the warnings above, and the numerous
warnings for unique_unit_address, I suggest we update the clksel clock
children for the standard reg property as we already discussed a bit
earlier.

The suggested patch for the am3 clksel children is below for reference.
I have at least one issue to sort out before I can post proper patches.

The issue I'm seeing is that updating omap3 clkcsel clocks in a similar
way adds a new error that gets multiplied by about 50 times as the
dss_tv_fck and dss_96m_fck both seem to really be gated by the same
bit..

I think the dss_tv_fck might be derived from the dss_96m_fck really,
and the documentation is wrong. If anybody has more info on this please
let me know, otherwise I guess I'll just leave the clock@e00 not updated
for now.

Regards,

Tony

> [1] https://gitlab.com/robherring/linux-dt/-/jobs/5620809910#L5618
>

8< ---------------------------
diff --git a/arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi b/arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi
--- a/arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am33xx-clocks.dtsi
@@ -108,30 +108,31 @@ clock@664 {
 		compatible = "ti,clksel";
 		reg = <0x664>;
 		#clock-cells = <2>;
-		#address-cells = <0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
 
-		ehrpwm0_tbclk: clock-ehrpwm0-tbclk {
+		ehrpwm0_tbclk: clock-ehrpwm0-tbclk@0 {
+			reg = <0>;
 			#clock-cells = <0>;
 			compatible = "ti,gate-clock";
 			clock-output-names = "ehrpwm0_tbclk";
 			clocks = <&l4ls_gclk>;
-			ti,bit-shift = <0>;
 		};
 
-		ehrpwm1_tbclk: clock-ehrpwm1-tbclk {
+		ehrpwm1_tbclk: clock-ehrpwm1-tbclk@1 {
+			reg = <1>;
 			#clock-cells = <0>;
 			compatible = "ti,gate-clock";
 			clock-output-names = "ehrpwm1_tbclk";
 			clocks = <&l4ls_gclk>;
-			ti,bit-shift = <1>;
 		};
 
-		ehrpwm2_tbclk: clock-ehrpwm2-tbclk {
+		ehrpwm2_tbclk: clock-ehrpwm2-tbclk@2 {
+			reg = <2>;
 			#clock-cells = <0>;
 			compatible = "ti,gate-clock";
 			clock-output-names = "ehrpwm2_tbclk";
 			clocks = <&l4ls_gclk>;
-			ti,bit-shift = <2>;
 		};
 	};
 };
@@ -566,17 +567,19 @@ clock@52c {
 		compatible = "ti,clksel";
 		reg = <0x52c>;
 		#clock-cells = <2>;
-		#address-cells = <0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
 
-		gfx_fclk_clksel_ck: clock-gfx-fclk-clksel {
+		gfx_fclk_clksel_ck: clock-gfx-fclk-clksel@1 {
+			reg = <1>;
 			#clock-cells = <0>;
 			compatible = "ti,mux-clock";
 			clock-output-names = "gfx_fclk_clksel_ck";
 			clocks = <&dpll_core_m4_ck>, <&dpll_per_m2_ck>;
-			ti,bit-shift = <1>;
 		};
 
-		gfx_fck_div_ck: clock-gfx-fck-div {
+		gfx_fck_div_ck: clock-gfx-fck-div@0 {
+			reg = <0>;
 			#clock-cells = <0>;
 			compatible = "ti,divider-clock";
 			clock-output-names = "gfx_fck_div_ck";
@@ -589,30 +592,32 @@ clock@700 {
 		compatible = "ti,clksel";
 		reg = <0x700>;
 		#clock-cells = <2>;
-		#address-cells = <0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
 
-		sysclkout_pre_ck: clock-sysclkout-pre {
+		sysclkout_pre_ck: clock-sysclkout-pre@0 {
+			reg = <0>;
 			#clock-cells = <0>;
 			compatible = "ti,mux-clock";
 			clock-output-names = "sysclkout_pre_ck";
 			clocks = <&clk_32768_ck>, <&l3_gclk>, <&dpll_ddr_m2_ck>, <&dpll_per_m2_ck>, <&lcd_gclk>;
 		};
 
-		clkout2_div_ck: clock-clkout2-div {
+		clkout2_div_ck: clock-clkout2-div@3 {
+			reg = <3>;
 			#clock-cells = <0>;
 			compatible = "ti,divider-clock";
 			clock-output-names = "clkout2_div_ck";
 			clocks = <&sysclkout_pre_ck>;
-			ti,bit-shift = <3>;
 			ti,max-div = <8>;
 		};
 
-		clkout2_ck: clock-clkout2 {
+		clkout2_ck: clock-clkout2@7 {
+			reg = <7>;
 			#clock-cells = <0>;
 			compatible = "ti,gate-clock";
 			clock-output-names = "clkout2_ck";
 			clocks = <&clkout2_div_ck>;
-			ti,bit-shift = <7>;
 		};
 	};
 };
-- 
2.43.0