[2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup

It really becomes an issue that every time a device needs to look
up (clk_get) a clock we have to patch kernel clock file to call
clk_register_clkdev for that clock.

Since clock DT support which is meant to resolve clock lookup in device
tree is in place, the patch moves imx6q client devices' clock lookup
over to device tree, so that any new lookup to be added at later time
can just get done in DT instead of kernel.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../devicetree/bindings/clock/imx6q-clock.txt      |  223 +++++++++++++++++
 arch/arm/boot/dts/imx6q-sabrelite.dts              |    1 +
 arch/arm/boot/dts/imx6q.dtsi                       |  261 +++++++++++++++++++-
 arch/arm/mach-imx/clk-imx6q.c                      |   41 +---
 arch/arm/mach-imx/mach-imx6q.c                     |    1 -
 5 files changed, 477 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/imx6q-clock.txt

On 08/16/2012 03:08 AM, Shawn Guo wrote:
> It really becomes an issue that every time a device needs to look
> up (clk_get) a clock we have to patch kernel clock file to call
> clk_register_clkdev for that clock.
> 
> Since clock DT support which is meant to resolve clock lookup in device
> tree is in place, the patch moves imx6q client devices' clock lookup
> over to device tree, so that any new lookup to be added at later time
> can just get done in DT instead of kernel.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

Looks good. For both patches:

Reviewed-by: Rob Herring <rob.herring@calxeda.com>

> ---
>  .../devicetree/bindings/clock/imx6q-clock.txt      |  223 +++++++++++++++++
>  arch/arm/boot/dts/imx6q-sabrelite.dts              |    1 +
>  arch/arm/boot/dts/imx6q.dtsi                       |  261 +++++++++++++++++++-
>  arch/arm/mach-imx/clk-imx6q.c                      |   41 +---
>  arch/arm/mach-imx/mach-imx6q.c                     |    1 -
>  5 files changed, 477 insertions(+), 50 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/imx6q-clock.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> new file mode 100644
> index 0000000..19d8126
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> @@ -0,0 +1,223 @@
> +* Clock bindings for Freescale i.MX6 Quad
> +
> +Required properties:
> +- compatible: Should be "fsl,imx6q-ccm"
> +- reg: Address and length of the register set
> +- interrupts: Should contain CCM interrupt
> +- #clock-cells: Should be <1>
> +- clock-output-names: A list of clock output names that CCM provides.
> +  The full list of all valid clock names and IDs is below.
> +
> +	Name			ID
> +	---------------------------
> +	dummy			0
> +	ckil			1
> +	ckih			2
> +	osc			3
> +	pll2_pfd0_352m		4
> +	pll2_pfd1_594m		5
> +	pll2_pfd2_396m		6
> +	pll3_pfd0_720m		7
> +	pll3_pfd1_540m		8
> +	pll3_pfd2_508m		9
> +	pll3_pfd3_454m		10
> +	pll2_198m		11
> +	pll3_120m		12
> +	pll3_80m		13
> +	pll3_60m		14
> +	twd			15
> +	step			16
> +	pll1_sw			17
> +	periph_pre		18
> +	periph2_pre		19
> +	periph_clk2_sel		20
> +	periph2_clk2_sel	21
> +	axi_sel			22
> +	esai_sel		23
> +	asrc_sel		24
> +	spdif_sel		25
> +	gpu2d_axi		26
> +	gpu3d_axi		27
> +	gpu2d_core_sel		28
> +	gpu3d_core_sel		29
> +	gpu3d_shader_sel	30
> +	ipu1_sel		31
> +	ipu2_sel		32
> +	ldb_di0_sel		33
> +	ldb_di1_sel		34
> +	ipu1_di0_pre_sel	35
> +	ipu1_di1_pre_sel	36
> +	ipu2_di0_pre_sel	37
> +	ipu2_di1_pre_sel	38
> +	ipu1_di0_sel		39
> +	ipu1_di1_sel		40
> +	ipu2_di0_sel		41
> +	ipu2_di1_sel		42
> +	hsi_tx_sel		43
> +	pcie_axi_sel		44
> +	ssi1_sel		45
> +	ssi2_sel		46
> +	ssi3_sel		47
> +	usdhc1_sel		48
> +	usdhc2_sel		49
> +	usdhc3_sel		50
> +	usdhc4_sel		51
> +	enfc_sel		52
> +	emi_sel			53
> +	emi_slow_sel		54
> +	vdo_axi_sel		55
> +	vpu_axi_sel		56
> +	cko1_sel		57
> +	periph			58
> +	periph2			59
> +	periph_clk2		60
> +	periph2_clk2		61
> +	ipg			62
> +	ipg_per			63
> +	esai_pred		64
> +	esai_podf		65
> +	asrc_pred		66
> +	asrc_podf		67
> +	spdif_pred		68
> +	spdif_podf		69
> +	can_root		70
> +	ecspi_root		71
> +	gpu2d_core_podf		72
> +	gpu3d_core_podf		73
> +	gpu3d_shader		74
> +	ipu1_podf		75
> +	ipu2_podf		76
> +	ldb_di0_podf		77
> +	ldb_di1_podf		78
> +	ipu1_di0_pre		79
> +	ipu1_di1_pre		80
> +	ipu2_di0_pre		81
> +	ipu2_di1_pre		82
> +	hsi_tx_podf		83
> +	ssi1_pred		84
> +	ssi1_podf		85
> +	ssi2_pred		86
> +	ssi2_podf		87
> +	ssi3_pred		88
> +	ssi3_podf		89
> +	uart_serial_podf	90
> +	usdhc1_podf		91
> +	usdhc2_podf		92
> +	usdhc3_podf		93
> +	usdhc4_podf		94
> +	enfc_pred		95
> +	enfc_podf		96
> +	emi_podf		97
> +	emi_slow_podf		98
> +	vpu_axi_podf		99
> +	cko1_podf		100
> +	axi			101
> +	mmdc_ch0_axi_podf	102
> +	mmdc_ch1_axi_podf	103
> +	arm			104
> +	ahb			105
> +	apbh_dma		106
> +	asrc			107
> +	can1_ipg		108
> +	can1_serial		109
> +	can2_ipg		110
> +	can2_serial		111
> +	ecspi1			112
> +	ecspi2			113
> +	ecspi3			114
> +	ecspi4			115
> +	ecspi5			116
> +	enet			117
> +	esai			118
> +	gpt_ipg			119
> +	gpt_ipg_per		120
> +	gpu2d_core		121
> +	gpu3d_core		122
> +	hdmi_iahb		123
> +	hdmi_isfr		124
> +	i2c1			125
> +	i2c2			126
> +	i2c3			127
> +	iim			128
> +	enfc			129
> +	ipu1			130
> +	ipu1_di0		131
> +	ipu1_di1		132
> +	ipu2			133
> +	ipu2_di0		134
> +	ldb_di0			135
> +	ldb_di1			136
> +	ipu2_di1		137
> +	hsi_tx			138
> +	mlb			139
> +	mmdc_ch0_axi		140
> +	mmdc_ch1_axi		141
> +	ocram			142
> +	openvg_axi		143
> +	pcie_axi		144
> +	pwm1			145
> +	pwm2			146
> +	pwm3			147
> +	pwm4			148
> +	per1_bch		149
> +	gpmi_bch_apb		150
> +	gpmi_bch		151
> +	gpmi_io			152
> +	gpmi_apb		153
> +	sata			154
> +	sdma			155
> +	spba			156
> +	ssi1			157
> +	ssi2			158
> +	ssi3			159
> +	uart_ipg		160
> +	uart_serial		161
> +	usboh3			162
> +	usdhc1			163
> +	usdhc2			164
> +	usdhc3			165
> +	usdhc4			166
> +	vdo_axi			167
> +	vpu_axi			168
> +	cko1			169
> +	pll1_sys		170
> +	pll2_bus		171
> +	pll3_usb_otg		172
> +	pll4_audio		173
> +	pll5_video		174
> +	pll6_mlb		175
> +	pll7_usb_host		176
> +	pll8_enet		177
> +	ssi1_ipg		178
> +	ssi2_ipg		179
> +	ssi3_ipg		180
> +	rom			181
> +	usbphy1			182
> +	usbphy2			183
> +	ldb_di0_div_3_5		184
> +	ldb_di1_div_3_5		185
> +
> +  The ID will be used by clock consumer in the first cell of "clocks"
> +  phandle to specify the desired clock.
> +
> +Examples:
> +
> +clks: ccm@020c4000 {
> +	compatible = "fsl,imx6q-ccm";
> +	reg = <0x020c4000 0x4000>;
> +	interrupts = <0 87 0x04 0 88 0x04>;
> +	#clock-cells = <1>;
> +	clock-output-names = ...
> +			     "uart_ipg",
> +			     "uart_serial",
> +			     ...;
> +};
> +
> +uart1: serial@02020000 {
> +	compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
> +	reg = <0x02020000 0x4000>;
> +	interrupts = <0 26 0x04>;
> +	clocks = <&clks 160>, <&clks 161>;
> +	clock-names = "ipg", "per";
> +	status = "disabled";
> +};
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 72f30f3..cfdbe53 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -111,6 +111,7 @@
>  				codec: sgtl5000@0a {
>  					compatible = "fsl,sgtl5000";
>  					reg = <0x0a>;
> +					clocks = <&clks 169>;
>  					VDDA-supply = <&reg_2p5v>;
>  					VDDIO-supply = <&reg_3p3v>;
>  				};
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index 0052fe7..acff2dc 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -93,18 +93,23 @@
>  		dma-apbh@00110000 {
>  			compatible = "fsl,imx6q-dma-apbh", "fsl,imx28-dma-apbh";
>  			reg = <0x00110000 0x2000>;
> +			clocks = <&clks 106>;
>  		};
>  
>  		gpmi-nand@00112000 {
> -		       compatible = "fsl,imx6q-gpmi-nand";
> -		       #address-cells = <1>;
> -		       #size-cells = <1>;
> -		       reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
> -		       reg-names = "gpmi-nand", "bch";
> -		       interrupts = <0 13 0x04>, <0 15 0x04>;
> -		       interrupt-names = "gpmi-dma", "bch";
> -		       fsl,gpmi-dma-channel = <0>;
> -		       status = "disabled";
> +			compatible = "fsl,imx6q-gpmi-nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
> +			reg-names = "gpmi-nand", "bch";
> +			interrupts = <0 13 0x04>, <0 15 0x04>;
> +			interrupt-names = "gpmi-dma", "bch";
> +			clocks = <&clks 152>, <&clks 153>, <&clks 151>,
> +				 <&clks 150>, <&clks 149>;
> +			clock-names = "gpmi_io", "gpmi_apb", "gpmi_bch",
> +				      "gpmi_bch_apb", "per1_bch";
> +			fsl,gpmi-dma-channel = <0>;
> +			status = "disabled";
>  		};
>  
>  		timer@00a00600 {
> @@ -146,6 +151,8 @@
>  					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>  					reg = <0x02008000 0x4000>;
>  					interrupts = <0 31 0x04>;
> +					clocks = <&clks 112>, <&clks 112>;
> +					clock-names = "ipg", "per";
>  					status = "disabled";
>  				};
>  
> @@ -155,6 +162,8 @@
>  					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>  					reg = <0x0200c000 0x4000>;
>  					interrupts = <0 32 0x04>;
> +					clocks = <&clks 113>, <&clks 113>;
> +					clock-names = "ipg", "per";
>  					status = "disabled";
>  				};
>  
> @@ -164,6 +173,8 @@
>  					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>  					reg = <0x02010000 0x4000>;
>  					interrupts = <0 33 0x04>;
> +					clocks = <&clks 114>, <&clks 114>;
> +					clock-names = "ipg", "per";
>  					status = "disabled";
>  				};
>  
> @@ -173,6 +184,8 @@
>  					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>  					reg = <0x02014000 0x4000>;
>  					interrupts = <0 34 0x04>;
> +					clocks = <&clks 115>, <&clks 115>;
> +					clock-names = "ipg", "per";
>  					status = "disabled";
>  				};
>  
> @@ -182,6 +195,8 @@
>  					compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>  					reg = <0x02018000 0x4000>;
>  					interrupts = <0 35 0x04>;
> +					clocks = <&clks 116>, <&clks 116>;
> +					clock-names = "ipg", "per";
>  					status = "disabled";
>  				};
>  
> @@ -189,6 +204,8 @@
>  					compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>  					reg = <0x02020000 0x4000>;
>  					interrupts = <0 26 0x04>;
> +					clocks = <&clks 160>, <&clks 161>;
> +					clock-names = "ipg", "per";
>  					status = "disabled";
>  				};
>  
> @@ -201,6 +218,7 @@
>  					compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
>  					reg = <0x02028000 0x4000>;
>  					interrupts = <0 46 0x04>;
> +					clocks = <&clks 178>;
>  					fsl,fifo-depth = <15>;
>  					fsl,ssi-dma-events = <38 37>;
>  					status = "disabled";
> @@ -210,6 +228,7 @@
>  					compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
>  					reg = <0x0202c000 0x4000>;
>  					interrupts = <0 47 0x04>;
> +					clocks = <&clks 179>;
>  					fsl,fifo-depth = <15>;
>  					fsl,ssi-dma-events = <42 41>;
>  					status = "disabled";
> @@ -219,6 +238,7 @@
>  					compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
>  					reg = <0x02030000 0x4000>;
>  					interrupts = <0 48 0x04>;
> +					clocks = <&clks 180>;
>  					fsl,fifo-depth = <15>;
>  					fsl,ssi-dma-events = <46 45>;
>  					status = "disabled";
> @@ -358,6 +378,7 @@
>  				compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>  				reg = <0x020bc000 0x4000>;
>  				interrupts = <0 80 0x04>;
> +				clocks = <&clks 0>;
>  				status = "disabled";
>  			};
>  
> @@ -365,13 +386,203 @@
>  				compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>  				reg = <0x020c0000 0x4000>;
>  				interrupts = <0 81 0x04>;
> +				clocks = <&clks 0>;
>  				status = "disabled";
>  			};
>  
> -			ccm@020c4000 {
> +			clks: ccm@020c4000 {
>  				compatible = "fsl,imx6q-ccm";
>  				reg = <0x020c4000 0x4000>;
>  				interrupts = <0 87 0x04 0 88 0x04>;
> +				#clock-cells = <1>;
> +				clock-output-names =
> +					"dummy",		/* 0 */
> +					"ckil",			/* 1 */
> +					"ckih",			/* 2 */
> +					"osc",			/* 3 */
> +					"pll2_pfd0_352m",	/* 4 */
> +					"pll2_pfd1_594m",	/* 5 */
> +					"pll2_pfd2_396m",	/* 6 */
> +					"pll3_pfd0_720m",	/* 7 */
> +					"pll3_pfd1_540m",	/* 8 */
> +					"pll3_pfd2_508m",	/* 9 */
> +					"pll3_pfd3_454m",	/* 10 */
> +					"pll2_198m",		/* 11 */
> +					"pll3_120m",		/* 12 */
> +					"pll3_80m",		/* 13 */
> +					"pll3_60m",		/* 14 */
> +					"twd",			/* 15 */
> +					"step",			/* 16 */
> +					"pll1_sw",		/* 17 */
> +					"periph_pre",		/* 18 */
> +					"periph2_pre",		/* 19 */
> +					"periph_clk2_sel",	/* 20 */
> +					"periph2_clk2_sel",	/* 21 */
> +					"axi_sel",		/* 22 */
> +					"esai_sel",		/* 23 */
> +					"asrc_sel",		/* 24 */
> +					"spdif_sel",		/* 25 */
> +					"gpu2d_axi",		/* 26 */
> +					"gpu3d_axi",		/* 27 */
> +					"gpu2d_core_sel",	/* 28 */
> +					"gpu3d_core_sel",	/* 29 */
> +					"gpu3d_shader_sel",	/* 30 */
> +					"ipu1_sel",		/* 31 */
> +					"ipu2_sel",		/* 32 */
> +					"ldb_di0_sel",		/* 33 */
> +					"ldb_di1_sel",		/* 34 */
> +					"ipu1_di0_pre_sel",	/* 35 */
> +					"ipu1_di1_pre_sel",	/* 36 */
> +					"ipu2_di0_pre_sel",	/* 37 */
> +					"ipu2_di1_pre_sel",	/* 38 */
> +					"ipu1_di0_sel",		/* 39 */
> +					"ipu1_di1_sel",		/* 40 */
> +					"ipu2_di0_sel",		/* 41 */
> +					"ipu2_di1_sel",		/* 42 */
> +					"hsi_tx_sel",		/* 43 */
> +					"pcie_axi_sel",		/* 44 */
> +					"ssi1_sel",		/* 45 */
> +					"ssi2_sel",		/* 46 */
> +					"ssi3_sel",		/* 47 */
> +					"usdhc1_sel",		/* 48 */
> +					"usdhc2_sel",		/* 49 */
> +					"usdhc3_sel",		/* 50 */
> +					"usdhc4_sel",		/* 51 */
> +					"enfc_sel",		/* 52 */
> +					"emi_sel",		/* 53 */
> +					"emi_slow_sel",		/* 54 */
> +					"vdo_axi_sel",		/* 55 */
> +					"vpu_axi_sel",		/* 56 */
> +					"cko1_sel",		/* 57 */
> +					"periph",		/* 58 */
> +					"periph2",		/* 59 */
> +					"periph_clk2",		/* 60 */
> +					"periph2_clk2",		/* 61 */
> +					"ipg",			/* 62 */
> +					"ipg_per",		/* 63 */
> +					"esai_pred",		/* 64 */
> +					"esai_podf",		/* 65 */
> +					"asrc_pred",		/* 66 */
> +					"asrc_podf",		/* 67 */
> +					"spdif_pred",		/* 68 */
> +					"spdif_podf",		/* 69 */
> +					"can_root",		/* 70 */
> +					"ecspi_root",		/* 71 */
> +					"gpu2d_core_podf",	/* 72 */
> +					"gpu3d_core_podf",	/* 73 */
> +					"gpu3d_shader",		/* 74 */
> +					"ipu1_podf",		/* 75 */
> +					"ipu2_podf",		/* 76 */
> +					"ldb_di0_podf",		/* 77 */
> +					"ldb_di1_podf",		/* 78 */
> +					"ipu1_di0_pre",		/* 79 */
> +					"ipu1_di1_pre",		/* 80 */
> +					"ipu2_di0_pre",		/* 81 */
> +					"ipu2_di1_pre",		/* 82 */
> +					"hsi_tx_podf",		/* 83 */
> +					"ssi1_pred",		/* 84 */
> +					"ssi1_podf",		/* 85 */
> +					"ssi2_pred",		/* 86 */
> +					"ssi2_podf",		/* 87 */
> +					"ssi3_pred",		/* 88 */
> +					"ssi3_podf",		/* 89 */
> +					"uart_serial_podf",	/* 90 */
> +					"usdhc1_podf",		/* 91 */
> +					"usdhc2_podf",		/* 92 */
> +					"usdhc3_podf",		/* 93 */
> +					"usdhc4_podf",		/* 94 */
> +					"enfc_pred",		/* 95 */
> +					"enfc_podf",		/* 96 */
> +					"emi_podf",		/* 97 */
> +					"emi_slow_podf",	/* 98 */
> +					"vpu_axi_podf",		/* 99 */
> +					"cko1_podf",		/* 100 */
> +					"axi",			/* 101 */
> +					"mmdc_ch0_axi_podf",	/* 102 */
> +					"mmdc_ch1_axi_podf",	/* 103 */
> +					"arm",			/* 104 */
> +					"ahb",			/* 105 */
> +					"apbh_dma",		/* 106 */
> +					"asrc",			/* 107 */
> +					"can1_ipg",		/* 108 */
> +					"can1_serial",		/* 109 */
> +					"can2_ipg",		/* 110 */
> +					"can2_serial",		/* 111 */
> +					"ecspi1",		/* 112 */
> +					"ecspi2",		/* 113 */
> +					"ecspi3",		/* 114 */
> +					"ecspi4",		/* 115 */
> +					"ecspi5",		/* 116 */
> +					"enet",			/* 117 */
> +					"esai",			/* 118 */
> +					"gpt_ipg",		/* 119 */
> +					"gpt_ipg_per",		/* 120 */
> +					"gpu2d_core",		/* 121 */
> +					"gpu3d_core",		/* 122 */
> +					"hdmi_iahb",		/* 123 */
> +					"hdmi_isfr",		/* 124 */
> +					"i2c1",			/* 125 */
> +					"i2c2",			/* 126 */
> +					"i2c3",			/* 127 */
> +					"iim",			/* 128 */
> +					"enfc",			/* 129 */
> +					"ipu1",			/* 130 */
> +					"ipu1_di0",		/* 131 */
> +					"ipu1_di1",		/* 132 */
> +					"ipu2",			/* 133 */
> +					"ipu2_di0",		/* 134 */
> +					"ldb_di0",		/* 135 */
> +					"ldb_di1",		/* 136 */
> +					"ipu2_di1",		/* 137 */
> +					"hsi_tx",		/* 138 */
> +					"mlb",			/* 139 */
> +					"mmdc_ch0_axi",		/* 140 */
> +					"mmdc_ch1_axi",		/* 141 */
> +					"ocram",		/* 142 */
> +					"openvg_axi",		/* 143 */
> +					"pcie_axi",		/* 144 */
> +					"pwm1",			/* 145 */
> +					"pwm2",			/* 146 */
> +					"pwm3",			/* 147 */
> +					"pwm4",			/* 148 */
> +					"per1_bch",		/* 149 */
> +					"gpmi_bch_apb",		/* 150 */
> +					"gpmi_bch",		/* 151 */
> +					"gpmi_io",		/* 152 */
> +					"gpmi_apb",		/* 153 */
> +					"sata",			/* 154 */
> +					"sdma",			/* 155 */
> +					"spba",			/* 156 */
> +					"ssi1",			/* 157 */
> +					"ssi2",			/* 158 */
> +					"ssi3",			/* 159 */
> +					"uart_ipg",		/* 160 */
> +					"uart_serial",		/* 161 */
> +					"usboh3",		/* 162 */
> +					"usdhc1",		/* 163 */
> +					"usdhc2",		/* 164 */
> +					"usdhc3",		/* 165 */
> +					"usdhc4",		/* 166 */
> +					"vdo_axi",		/* 167 */
> +					"vpu_axi",		/* 168 */
> +					"cko1",			/* 169 */
> +					"pll1_sys",		/* 170 */
> +					"pll2_bus",		/* 171 */
> +					"pll3_usb_otg",		/* 172 */
> +					"pll4_audio",		/* 173 */
> +					"pll5_video",		/* 174 */
> +					"pll6_mlb",		/* 175 */
> +					"pll7_usb_host",	/* 176 */
> +					"pll8_enet",		/* 177 */
> +					"ssi1_ipg",		/* 178 */
> +					"ssi2_ipg",		/* 179 */
> +					"ssi3_ipg",		/* 180 */
> +					"rom",			/* 181 */
> +					"usbphy1",		/* 182 */
> +					"usbphy2",		/* 183 */
> +					"ldb_di0_div_3_5",	/* 184 */
> +					"ldb_di1_div_3_5",	/* 185 */
> +					"end_of_list";
>  			};
>  
>  			anatop@020c8000 {
> @@ -468,12 +679,14 @@
>  				compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
>  				reg = <0x020c9000 0x1000>;
>  				interrupts = <0 44 0x04>;
> +				clocks = <&clks 182>;
>  			};
>  
>  			usbphy2: usbphy@020ca000 {
>  				compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
>  				reg = <0x020ca000 0x1000>;
>  				interrupts = <0 45 0x04>;
> +				clocks = <&clks 183>;
>  			};
>  
>  			snvs@020cc000 {
> @@ -608,6 +821,9 @@
>  				compatible = "fsl,imx6q-sdma", "fsl,imx35-sdma";
>  				reg = <0x020ec000 0x4000>;
>  				interrupts = <0 2 0x04>;
> +				clocks = <&clks 155>, <&clks 155>;
> +				clock-names = "ipg", "ahb";
> +				fsl,sdma-ram-script-name = "imx/sdma/sdma-imx6q-to1.bin";
>  			};
>  		};
>  
> @@ -631,6 +847,7 @@
>  				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>  				reg = <0x02184000 0x200>;
>  				interrupts = <0 43 0x04>;
> +				clocks = <&clks 162>;
>  				fsl,usbphy = <&usbphy1>;
>  				status = "disabled";
>  			};
> @@ -639,6 +856,7 @@
>  				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>  				reg = <0x02184200 0x200>;
>  				interrupts = <0 40 0x04>;
> +				clocks = <&clks 162>;
>  				fsl,usbphy = <&usbphy2>;
>  				status = "disabled";
>  			};
> @@ -647,6 +865,7 @@
>  				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>  				reg = <0x02184400 0x200>;
>  				interrupts = <0 41 0x04>;
> +				clocks = <&clks 162>;
>  				status = "disabled";
>  			};
>  
> @@ -654,6 +873,7 @@
>  				compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>  				reg = <0x02184600 0x200>;
>  				interrupts = <0 42 0x04>;
> +				clocks = <&clks 162>;
>  				status = "disabled";
>  			};
>  
> @@ -661,6 +881,8 @@
>  				compatible = "fsl,imx6q-fec";
>  				reg = <0x02188000 0x4000>;
>  				interrupts = <0 118 0x04 0 119 0x04>;
> +				clocks = <&clks 117>, <&clks 117>;
> +				clock-names = "ipg", "ahb";
>  				status = "disabled";
>  			};
>  
> @@ -673,6 +895,8 @@
>  				compatible = "fsl,imx6q-usdhc";
>  				reg = <0x02190000 0x4000>;
>  				interrupts = <0 22 0x04>;
> +				clocks = <&clks 163>, <&clks 163>, <&clks 163>;
> +				clock-names = "ipg", "ahb", "per";
>  				status = "disabled";
>  			};
>  
> @@ -680,6 +904,8 @@
>  				compatible = "fsl,imx6q-usdhc";
>  				reg = <0x02194000 0x4000>;
>  				interrupts = <0 23 0x04>;
> +				clocks = <&clks 164>, <&clks 164>, <&clks 164>;
> +				clock-names = "ipg", "ahb", "per";
>  				status = "disabled";
>  			};
>  
> @@ -687,6 +913,8 @@
>  				compatible = "fsl,imx6q-usdhc";
>  				reg = <0x02198000 0x4000>;
>  				interrupts = <0 24 0x04>;
> +				clocks = <&clks 165>, <&clks 165>, <&clks 165>;
> +				clock-names = "ipg", "ahb", "per";
>  				status = "disabled";
>  			};
>  
> @@ -694,6 +922,8 @@
>  				compatible = "fsl,imx6q-usdhc";
>  				reg = <0x0219c000 0x4000>;
>  				interrupts = <0 25 0x04>;
> +				clocks = <&clks 166>, <&clks 166>, <&clks 166>;
> +				clock-names = "ipg", "ahb", "per";
>  				status = "disabled";
>  			};
>  
> @@ -703,6 +933,7 @@
>  				compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
>  				reg = <0x021a0000 0x4000>;
>  				interrupts = <0 36 0x04>;
> +				clocks = <&clks 125>;
>  				status = "disabled";
>  			};
>  
> @@ -712,6 +943,7 @@
>  				compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
>  				reg = <0x021a4000 0x4000>;
>  				interrupts = <0 37 0x04>;
> +				clocks = <&clks 126>;
>  				status = "disabled";
>  			};
>  
> @@ -721,6 +953,7 @@
>  				compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
>  				reg = <0x021a8000 0x4000>;
>  				interrupts = <0 38 0x04>;
> +				clocks = <&clks 127>;
>  				status = "disabled";
>  			};
>  
> @@ -784,6 +1017,8 @@
>  				compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>  				reg = <0x021e8000 0x4000>;
>  				interrupts = <0 27 0x04>;
> +				clocks = <&clks 160>, <&clks 161>;
> +				clock-names = "ipg", "per";
>  				status = "disabled";
>  			};
>  
> @@ -791,6 +1026,8 @@
>  				compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>  				reg = <0x021ec000 0x4000>;
>  				interrupts = <0 28 0x04>;
> +				clocks = <&clks 160>, <&clks 161>;
> +				clock-names = "ipg", "per";
>  				status = "disabled";
>  			};
>  
> @@ -798,6 +1035,8 @@
>  				compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>  				reg = <0x021f0000 0x4000>;
>  				interrupts = <0 29 0x04>;
> +				clocks = <&clks 160>, <&clks 161>;
> +				clock-names = "ipg", "per";
>  				status = "disabled";
>  			};
>  
> @@ -805,6 +1044,8 @@
>  				compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>  				reg = <0x021f4000 0x4000>;
>  				interrupts = <0 30 0x04>;
> +				clocks = <&clks 160>, <&clks 161>;
> +				clock-names = "ipg", "per";
>  				status = "disabled";
>  			};
>  		};
> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> index 8e46407..433c683 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -387,48 +387,11 @@ int __init mx6q_clocks_init(void)
>  			pr_err("i.MX6q clk %d: register failed with %ld\n",
>  				i, PTR_ERR(clk[i]));
>  
> +	of_clk_add_provider(np, of_clk_src_onecell_get, NULL);
> +
>  	clk_register_clkdev(clk[gpt_ipg], "ipg", "imx-gpt.0");
>  	clk_register_clkdev(clk[gpt_ipg_per], "per", "imx-gpt.0");
>  	clk_register_clkdev(clk[twd], NULL, "smp_twd");
> -	clk_register_clkdev(clk[apbh_dma], NULL, "110000.dma-apbh");
> -	clk_register_clkdev(clk[per1_bch], "per1_bch", "112000.gpmi-nand");
> -	clk_register_clkdev(clk[gpmi_bch_apb], "gpmi_bch_apb", "112000.gpmi-nand");
> -	clk_register_clkdev(clk[gpmi_bch], "gpmi_bch", "112000.gpmi-nand");
> -	clk_register_clkdev(clk[gpmi_apb], "gpmi_apb", "112000.gpmi-nand");
> -	clk_register_clkdev(clk[gpmi_io], "gpmi_io", "112000.gpmi-nand");
> -	clk_register_clkdev(clk[usboh3], NULL, "2184000.usb");
> -	clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
> -	clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
> -	clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
> -	clk_register_clkdev(clk[usbphy1], NULL, "20c9000.usbphy");
> -	clk_register_clkdev(clk[usbphy2], NULL, "20ca000.usbphy");
> -	clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
> -	clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
> -	clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
> -	clk_register_clkdev(clk[uart_ipg], "ipg", "21e8000.serial");
> -	clk_register_clkdev(clk[uart_serial], "per", "21ec000.serial");
> -	clk_register_clkdev(clk[uart_ipg], "ipg", "21ec000.serial");
> -	clk_register_clkdev(clk[uart_serial], "per", "21f0000.serial");
> -	clk_register_clkdev(clk[uart_ipg], "ipg", "21f0000.serial");
> -	clk_register_clkdev(clk[uart_serial], "per", "21f4000.serial");
> -	clk_register_clkdev(clk[uart_ipg], "ipg", "21f4000.serial");
> -	clk_register_clkdev(clk[enet], NULL, "2188000.ethernet");
> -	clk_register_clkdev(clk[usdhc1], NULL, "2190000.usdhc");
> -	clk_register_clkdev(clk[usdhc2], NULL, "2194000.usdhc");
> -	clk_register_clkdev(clk[usdhc3], NULL, "2198000.usdhc");
> -	clk_register_clkdev(clk[usdhc4], NULL, "219c000.usdhc");
> -	clk_register_clkdev(clk[i2c1], NULL, "21a0000.i2c");
> -	clk_register_clkdev(clk[i2c2], NULL, "21a4000.i2c");
> -	clk_register_clkdev(clk[i2c3], NULL, "21a8000.i2c");
> -	clk_register_clkdev(clk[ecspi1], NULL, "2008000.ecspi");
> -	clk_register_clkdev(clk[ecspi2], NULL, "200c000.ecspi");
> -	clk_register_clkdev(clk[ecspi3], NULL, "2010000.ecspi");
> -	clk_register_clkdev(clk[ecspi4], NULL, "2014000.ecspi");
> -	clk_register_clkdev(clk[ecspi5], NULL, "2018000.ecspi");
> -	clk_register_clkdev(clk[sdma], NULL, "20ec000.sdma");
> -	clk_register_clkdev(clk[dummy], NULL, "20bc000.wdog");
> -	clk_register_clkdev(clk[dummy], NULL, "20c0000.wdog");
> -	clk_register_clkdev(clk[ssi1_ipg], NULL, "2028000.ssi");
>  	clk_register_clkdev(clk[cko1_sel], "cko1_sel", NULL);
>  	clk_register_clkdev(clk[ahb], "ahb", NULL);
>  	clk_register_clkdev(clk[cko1], "cko1", NULL);
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 6f9c23b..957f5fe 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -94,7 +94,6 @@ static void __init imx6q_sabrelite_cko1_setup(void)
>  	clk_set_parent(cko1_sel, ahb);
>  	rate = clk_round_rate(cko1, 16000000);
>  	clk_set_rate(cko1, rate);
> -	clk_register_clkdev(cko1, NULL, "0-000a");
>  put_clk:
>  	if (!IS_ERR(cko1_sel))
>  		clk_put(cko1_sel);
>

Yet another arbitrary array...

I'm not sure why you would move the registration lookup into the DT
and use an arbitrarily ordered array again. Sure, you're looking it up
entirely within the device tree here, but a better solution would be
to not name clocks twice, and structure your clock definition
properly.

What's wrong with;

ccm@020c4000 {
   ...
   my_clock: clock@0 {
       name = "my_clock_name";
   }
}

uart@nnnnnnnn {
   ...
   clocks = <&my_clock>;
}

?

On Mon, Aug 20, 2012 at 09:54:48AM -0500, Matt Sealey wrote:
> Yet another arbitrary array...
> 
> I'm not sure why you would move the registration lookup into the DT

Because I do not want to patch kernel every time I need a new lookup.
Furthermore, looking at function imx53_qsb_init in imx53-dt.c, you
will find that lookup for sgtl5000 is really board specific and should
go to device tree.

> and use an arbitrarily ordered array again. Sure, you're looking it up
> entirely within the device tree here, but a better solution would be
> to not name clocks twice, and structure your clock definition
> properly.
> 
> What's wrong with;
> 
> ccm@020c4000 {
>    ...
>    my_clock: clock@0 {
>        name = "my_clock_name";
>    }
> }
> 
> uart@nnnnnnnn {
>    ...
>    clocks = <&my_clock>;
> }
> 
> ?
> 
It turns a property into 185 nodes, which will just bloat the device
tree.  This issue has been discussed for a plenty of times.

On Mon, Aug 20, 2012 at 10:16 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Mon, Aug 20, 2012 at 09:54:48AM -0500, Matt Sealey wrote:
>> Yet another arbitrary array...
>>
>> I'm not sure why you would move the registration lookup into the DT
>
> Because I do not want to patch kernel every time I need a new lookup.
> Furthermore, looking at function imx53_qsb_init in imx53-dt.c, you
> will find that lookup for sgtl5000 is really board specific and should
> go to device tree.
>
>> and use an arbitrarily ordered array again. Sure, you're looking it up
>> entirely within the device tree here, but a better solution would be
>> to not name clocks twice, and structure your clock definition
>> properly.
>>
>> What's wrong with;
>>
>> ccm@020c4000 {
>>    ...
>>    my_clock: clock@0 {
>>        name = "my_clock_name";
>>    }
>> }
>>
>> uart@nnnnnnnn {
>>    ...
>>    clocks = <&my_clock>;
>> }
>>
>> ?
>>
> It turns a property into 185 nodes, which will just bloat the device
> tree.  This issue has been discussed for a plenty of times.

It's not bloat just because it is by its very definition "a big list", is it?

How do you explain duplicating the clock names in the array AND in the
device node as NOT being bloated?

You're going to have to define these clocks as a tree with parents and
leaf nodes anyway in the clock subsystem. Why not define these in the
device tree in total and reference them by handle when you build the
entire clock tree from the ground up? Or will it just be all the
clocks defined in Linux, but the lookups (which is what I see here)
moved into the DT? Why not form the lookups as part of the definition
of the clock tree?

On Mon, Aug 20, 2012 at 12:22:56PM -0500, Matt Sealey wrote:
> You're going to have to define these clocks as a tree with parents and
> leaf nodes anyway in the clock subsystem. Why not define these in the
> device tree in total and reference them by handle when you build the
> entire clock tree from the ground up? Or will it just be all the
> clocks defined in Linux, but the lookups (which is what I see here)
> moved into the DT? Why not form the lookups as part of the definition
> of the clock tree?

Well, IMHO the DT conversion of the clk lookup stuff has been done
completely wrong.

What should have been done is rather than invent a totally new bloody
lookup interface that drivers have to use instead of clk_get(), is to
embed the OF lookup _inside_ clk_get().

What you do is this:

1. Have property names in the device node like:

	clock_<connection-id> = <&provider-node output>

   In the case of a NULL connection id:

	clock = <&provider-node output>

   Remember that the connection ID is _supposed_ to be something that
   is described by the hardware (like - for the AACI primecell, the
   clock which runs the functional side is called "AACICLK" by the TRM,
   and for the MMCI primecell, it's "MMCICLK" - even though these two
   clocks may be fed by the same source in an implementation.)

2. clkdev's lookup is then modified to look at the struct device, and
   check for a DT node.  If there is a DT node, it formats a property
   string:

	if (dev->of_node) {
		char *propname, *clk_prop = NULL;
		struct property *p;

		if (conn_id) {
			clk_prop = kasprintf("clock_%s", conn_id);
			propname = clk_prop;
		} else {
			propname = "clock";
		}

		p = of_find_property(dev->of_node, propname, NULL);
		if (clk_prop)
			kfree(clk_prop);

		if (p) {
			clk = clk_get_from_of_property(p);
			if (clk)
				return clk;
		}

		/* Fallthrough to clkdev table lookup */
	}

So now, you're not dealing with inventing a whole load of names for clocks
on a platform, instead what you're doing is describing _where_ the clock
comes from in the system for a particular device by device node and index
into it - just like we do for interrupts.

This means there's no need for huge tables and such like of clock names.

I did mention this idea long ago but got ignored.

On 08/20/2012 12:22 PM, Matt Sealey wrote:
> On Mon, Aug 20, 2012 at 10:16 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> On Mon, Aug 20, 2012 at 09:54:48AM -0500, Matt Sealey wrote:
>>> Yet another arbitrary array...
>>>
>>> I'm not sure why you would move the registration lookup into the DT
>>
>> Because I do not want to patch kernel every time I need a new lookup.
>> Furthermore, looking at function imx53_qsb_init in imx53-dt.c, you
>> will find that lookup for sgtl5000 is really board specific and should
>> go to device tree.
>>
>>> and use an arbitrarily ordered array again. Sure, you're looking it up
>>> entirely within the device tree here, but a better solution would be
>>> to not name clocks twice, and structure your clock definition
>>> properly.
>>>
>>> What's wrong with;
>>>
>>> ccm@020c4000 {
>>>    ...
>>>    my_clock: clock@0 {
>>>        name = "my_clock_name";
>>>    }
>>> }
>>>
>>> uart@nnnnnnnn {
>>>    ...
>>>    clocks = <&my_clock>;
>>> }
>>>
>>> ?
>>>
>> It turns a property into 185 nodes, which will just bloat the device
>> tree.  This issue has been discussed for a plenty of times.
> 
> It's not bloat just because it is by its very definition "a big list", is it?
> 
> How do you explain duplicating the clock names in the array AND in the
> device node as NOT being bloated?

Read the clock binding doc. Names are optional and the 2 names are not
the same. One is the name of the output on the CCM and one is the name
on input to the module.

While generally optional, Shawn has chosen to require at least the
output names. In the case of defining a signal clock controller node
with lots of outputs, that is the right choice.

Rob

> 
> You're going to have to define these clocks as a tree with parents and
> leaf nodes anyway in the clock subsystem. Why not define these in the
> device tree in total and reference them by handle when you build the
> entire clock tree from the ground up? Or will it just be all the
> clocks defined in Linux, but the lookups (which is what I see here)
> moved into the DT? Why not form the lookups as part of the definition
> of the clock tree?
>

On 08/21/2012 07:27 AM, Russell King - ARM Linux wrote:
> On Mon, Aug 20, 2012 at 12:22:56PM -0500, Matt Sealey wrote:
>> You're going to have to define these clocks as a tree with parents and
>> leaf nodes anyway in the clock subsystem. Why not define these in the
>> device tree in total and reference them by handle when you build the
>> entire clock tree from the ground up? Or will it just be all the
>> clocks defined in Linux, but the lookups (which is what I see here)
>> moved into the DT? Why not form the lookups as part of the definition
>> of the clock tree?
> 
> Well, IMHO the DT conversion of the clk lookup stuff has been done
> completely wrong.
> 
> What should have been done is rather than invent a totally new bloody
> lookup interface that drivers have to use instead of clk_get(), is to
> embed the OF lookup _inside_ clk_get().

That is exactly what was done. Drivers only use clk_get. Only if you
don't have a struct device, then you can use of_clk_get.

Internally, you still need a conversion of clk provider node and cell to
a struct clk. It is up to each clk provider how to do this. The lookup
done here by Shawn is using the struct clk name and using the existing
clk framework lookup.

> What you do is this:
> 
> 1. Have property names in the device node like:
> 
> 	clock_<connection-id> = <&provider-node output>

The connection id is defined by the position in the list and
supplemented with "clock-names" property.

> 
>    In the case of a NULL connection id:
> 
> 	clock = <&provider-node output>
> 
>    Remember that the connection ID is _supposed_ to be something that
>    is described by the hardware (like - for the AACI primecell, the
>    clock which runs the functional side is called "AACICLK" by the TRM,
>    and for the MMCI primecell, it's "MMCICLK" - even though these two
>    clocks may be fed by the same source in an implementation.)
> 
> 2. clkdev's lookup is then modified to look at the struct device, and
>    check for a DT node.  If there is a DT node, it formats a property
>    string:
> 
> 	if (dev->of_node) {
> 		char *propname, *clk_prop = NULL;
> 		struct property *p;
> 
> 		if (conn_id) {
> 			clk_prop = kasprintf("clock_%s", conn_id);
> 			propname = clk_prop;
> 		} else {
> 			propname = "clock";
> 		}
> 
> 		p = of_find_property(dev->of_node, propname, NULL);
> 		if (clk_prop)
> 			kfree(clk_prop);
> 
> 		if (p) {
> 			clk = clk_get_from_of_property(p);
> 			if (clk)
> 				return clk;
> 		}
> 
> 		/* Fallthrough to clkdev table lookup */
> 	}
> 
> So now, you're not dealing with inventing a whole load of names for clocks
> on a platform, instead what you're doing is describing _where_ the clock
> comes from in the system for a particular device by device node and index
> into it - just like we do for interrupts.

That is what we're doing. The names are optional for DT, but happen to
be required for struct clk now. If we don't put something in DT, then
the clock names will have to be something generic like ccm-1..ccm-185.

Rob

> 
> This means there's no need for huge tables and such like of clock names.
> 
> I did mention this idea long ago but got ignored.
>

On Mon, Aug 20, 2012 at 12:22:56PM -0500, Matt Sealey wrote:
> It's not bloat just because it is by its very definition "a big list", is it?
> 
Grep for_each_node_by_*() and for_each_*_node() (include/linux/of.h)
in the tree to see how often these global device tree searching is
used, you may know how important not having so many nodes is.

> How do you explain duplicating the clock names in the array AND in the
> device node as NOT being bloated?
> 
> You're going to have to define these clocks as a tree with parents and
> leaf nodes anyway in the clock subsystem. Why not define these in the
> device tree in total and reference them by handle when you build the
> entire clock tree from the ground up? Or will it just be all the
> clocks defined in Linux, but the lookups (which is what I see here)
> moved into the DT? Why not form the lookups as part of the definition
> of the clock tree?
> 
This is something I had tried long time before, but it did not get
accepted because:

* It's unnecessary to encode the entire clock tree which is SoC
  specific in device tree.  Clock driver is a good place for that.

* Again, doing so will bloat device tree with hundreds of nodes.

On Tue, Aug 21, 2012 at 08:11:57AM -0500, Rob Herring wrote:
> On 08/21/2012 07:27 AM, Russell King - ARM Linux wrote:
> > So now, you're not dealing with inventing a whole load of names for clocks
> > on a platform, instead what you're doing is describing _where_ the clock
> > comes from in the system for a particular device by device node and index
> > into it - just like we do for interrupts.
> 
> That is what we're doing. The names are optional for DT, but happen to
> be required for struct clk now. If we don't put something in DT, then
> the clock names will have to be something generic like ccm-1..ccm-185.

And that's what's wrong.  Clocks themselves should _not_ be required to
be named.

If you use purely "producer node + index" then you don't need to name a
whole bunch of clocks, and you don't need to have an array of clock names
in the DT file.  This also gets rid of the time consuming strcmp against
every clock, which has already been raised as a problem with the existing
clk_get().

And, like it or not, the way they're being describing them in the DT file
at the top of this sub-thread, the matching _is_ done only by producer
name, which is TOTALLY the wrong way to go about this (that's how folk
tried to use the connection ID in the clk API and IT DOESN'T WORK for
reusable drivers.)

On Tue, Aug 21, 2012 at 7:53 AM, Rob Herring <rob.herring@calxeda.com> wrote:
>
>> How do you explain duplicating the clock names in the array AND in the
>> device node as NOT being bloated?
>
> Read the clock binding doc. Names are optional and the 2 names are not
> the same. One is the name of the output on the CCM and one is the name
> on input to the module.

My understanding of the i.MX clock tree naming in the docs, though, is
that the names in the DT don't match the ones in the docs, regardless.

I also don't understand how lots of nodes in a tree *is* lots of
bloat, by Shawn's definition, but lots of entirely Linux- and
common-clock-framework-specific names in a table *isn't* bloat even if
most of these clocks and names are not used for devices in the tree
anyway.

Device trees shouldn't encode data that is entirely specific to a
Linux driver. Even if the only user is Linux, you are not describing
the Linux driver, you are describing the hardware. The names match the
hardware specification in the CCM chapters of the manual, but just
barely. All Shawn has done here is make the lookup in the DT, which is
at the very least internally consistent (it's not referencing the
order of an array elsewhere than the device tree), but an index into
an array of strings is not the way we generally encode things in
device trees since the dawn of time, and certainly shouldn't be
acceptable behavior now.

I might agree somewhat with Shawn that encoding every clock for the
chip in the tree (some 190+ entries) and it's bits and entries is a
ridiculous amount, but most boards don't need all the clocks or
selects defined if they're not using those peripherals. There are ways
to make sure boards do not over-define their clock tree (and any
clocks not defined will not get enabled anyway).

That the clock tree is SoC-specific doesn't mean it should not be
encoded in the tree; the fact that Sascha could write a fairly
comprehensive common clock subsystem shows that for certain families
of SoC, certain groupings of clock mux, select and associations
between unit clock (for instance to write to registers) and peripheral
output clock (for instance to clock data to an MMC card or SPI bus or
whatever) are fairly "common" as it stands. What is not common is the
naming and the meaning of that naming, which in the end is only useful
to drivers as debugging information anyway.

What I don't get is why you'd implement a clock lookup as <&clk 160>
<&clk 161> and then futher give them names like "ipg" "per", even if
these were seperate clocks, it makes no sense to NAME them. If clock
160 is "uart_ipg" and 161 is "uart_serial", what are you gaining from
the clock-names property inside the actual device definition? You're
encoding the "ipg" root clock name twice, and "per" doesn't make any
sense here.

What's the difference with moving ALL the definitions of clock values
from the driver (such as arch/arm/mach-imx/clk-mx51-mx53.c) into the
DT and parsing it all out of the DT? Once it's registered then you
have to look it up by name, but if each DT driver node (uart@ for
example) references the clock node by phandle, it will be able to look
up that exact clock by that phandle, correct? What I'm confused about
right now is what the difference is between naming them "ipg" and
"per" vs. "donkey" and "shrek"? This is just a driver-specific naming
such that it can find these things and turn on and off the right
clocks at the right time, but it doesn't make any sense to refer to
the CCM node, then an index to a an array of names, then two more
names defining the clock definition. uart_serial cannot be used for
ANYTHING besides the uart "per" clock, so just encode this as a clock
type in the individual clock node. The clkdev subsystem can resolve
"per" to some magic value or the string in the tree...

uart_ipg_clk: clock@foo {
     clock-name = "uart";
     fsl,clock-type = "ipg";
     ...
};

uart@bar {
     clocks = <&uart_ipg_clk, &uart_per_clk>
};

I counted here and we're not "bloating" anything in terms of bytes
used to define the clocks - what we save in not doing an array lookup,
we lose in defining a node. Tthis is not going to end the world. What
you "waste" in the device tree, you get to automate in the Linux
driver anyway and remove a bunch of tables, which is what Shawn's
trying to do anyway. If the UART driver needs to do;

        sport->clk_ipg = devm_clk_get(&pdev->dev, "ipg");

Then it has all the information it needs here; one of the phandle
references points to a clock which has "ipg" as it's fsl,clock-type
property. Or something else.

It is going to be an SoC-specific binding, that's for sure, but since
there is a definition for a fixed clock, why can't there be a
definition for a gated clock (enable/disable), mux select defined as a
group and possible selections, and the right clocks being defined with
the right potential parents? The entire clock tree AND it's lookups
can be built from device tree, per documentation, per actual SoC
layout and the driver has to live with getting the information from
somewhere other than the heady mix of a table in DT, and a table in
Linux which don't necessarily depend on each other?

You only have to do these lookups once when the clock subsystem starts
on Linux, it's not like every time you want to go find a clock you
have to actually enter a firmware interface, parse the entire device
tree, come back out with a small snippet of data and then go off and
do the stuff; it's been put in a readily-accessible structure, and
gets done at device probe. Register them at boot, find them in the
clock subsystem, you're ready to go.

I would rather see every clock defined in the tree, as much "bloat" as
you would seem to think it is, it makes no sense to have a huge table
of clock definitions who's names and numbers are defined in the device
tree, yet register offsets and bit widths and values and defaults and
parents are defined in the driver itself along with lots of other
lists and tables. The table in the DT is making a huge assumption
about the subsystem needs to do the clock lookups and the way the
*Linux drivers* themself actually works; this is wrong, wrong, wrong
on so many levels.

> While generally optional, Shawn has chosen to require at least the
> output names. In the case of defining a signal clock controller node
> with lots of outputs, that is the right choice.

You only need to define the inputs and outputs you use, and in the end
I think referencing an array of strings by phandle and index into a
property contained in that handle, then looking up another string in
it's own device node to make sure which clock is which, is more
complex and takes longer than referencing a clock by phandle and
looking at two specific properties defining that clock. If we're going
this far, we should DEFINITELY be encoding the clock subsystems in the
SoC entirely in the device tree and automating clock setup. If the
common clock framework lives up to it's name, all you'd need is a
single arch/arm/mach-imx/clk.c to map the specific operation for every
i.MX SoC (i.e. bindings for i.MX) and not one for *every* i.MX SoC,
and all this data can be adequately encoded in the DT.

Ugh. Okay my blood sugar was super low. Can I just condense that into
I agree with Russell and the binding makes no sense once it gets past
the simple definition described in the binding?

If we take the pinctrl mess as an example, all the DT serves to do is
define an SoC-specific or family-specific binding into data the
pinctrl subsystem can use, via an SoC-specific abstraction in a driver
to make a generic subsystem in Linux work the way it should.

The common clock subsystem is no different - divider, fixed factor,
fixed rate, mux, gate, highbank (??) are defined in a way that
abstracts most of the actual SoC-specific stuff out to register
offsets, values, bit definitions and masks.

Right now for i.MX we have several abstraction interfaces
(imx_clk_XXX) which move values from tables around and call the
appropriate common clock function to register the clock. These
abstractions are identical for all the possible i.MX SoCs
(imx_clk_gate2, imx_clk_mux etc.) and move the values around so they
can be passed to clk_register_##same.

So if we can define a fixed-clock, why can't we define an
fsl,gated-clock or fsl,muxed-clock which defines an i.MX clock of that
type, and a much smaller subset of code that stuffs these values in
through a small abstraction and remove all these seperate
clk-imx51-imx53.c, clk-imx6q.c, clk-imx31.c files which are sharing
common functionality into one mega-abstraction which works across the
whole family?

The way I understand the binding right now (and I'm looking at this;
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob_plain;f=Documentation/devicetree/bindings/clock/clock-bindings.txt;hb=766e6a4ec602d0c107553b91b3434fe9c03474f4),
this is entirely what the clock binding is saying we should do. But I
don't understand why we're naming inputs and outputs, but not defining
how these signals are routed (possibly through gate bits in registers)
and leaving that down to some driver somewhere. Or why we have to name
clocks twice, once for the canonical name of the output in reference
to a higher level or root clock "uart_serial", vs. a driver-level name
for that input or output in the driver ("per"), since most clocks at
the driver level can't be re-used for other things anyway. If they
could, the whole prepare/unprepare and some explicit parenting/muxing
of clocks would handle that anyway so that the clock was enabled while
there was at least one user.

I do understand that in Shawn's patch and using UART as an example
again, "ipg" and "per" are definitely needed by the driver, and these
names are defined by the driver and implicit in the documentation to
enable the use of this unit, but at the end of the day the definition
at the *device* level (uart@) makes no sense except to perform a
lookup on a string, and in the end this string lookup only gets done
at the driver probe time anyway. With a standard definition of these
"ipg" and "per" strings per family of SoC or even in a generic fashion
across all possible SoCs (in this case, both are defined using
imx_clk_gate2()) then a node defining that clock and it's magical
driver-specific name would work just as well by phandle reference, and
it's parents are referenced by phandle, and all this stays within
SoC-specific abstraction of the common clocks and turns into normal
common clock structure stuff anyway (so you still get to do a string
lookup, but it's being stuffed by an i.MX driver and registered from
data it pulled from the DT).

Wouldn't we rather have a single device tree parser and clock
registration for i.MX than the current 7 which would get split into 7
clock registration drivers with some helpers that parsed half of it
via device tree? I don't really see what benefit you get out of going
for the halfway-defined model.

On 08/22/2012 07:08 PM, Matt Sealey wrote:
> Ugh. Okay my blood sugar was super low. Can I just condense that into
> I agree with Russell and the binding makes no sense once it gets past
> the simple definition described in the binding?

I can't speak for Russell, but I think his issue is addressed in v2.

> If we take the pinctrl mess as an example, all the DT serves to do is
> define an SoC-specific or family-specific binding into data the
> pinctrl subsystem can use, via an SoC-specific abstraction in a driver
> to make a generic subsystem in Linux work the way it should.

pinctrl changes much more board to board, so it's more valuable to have
in device tree. The changes from board to board in the clock tree are
much less.

> The common clock subsystem is no different - divider, fixed factor,
> fixed rate, mux, gate, highbank (??) are defined in a way that
> abstracts most of the actual SoC-specific stuff out to register
> offsets, values, bit definitions and masks.
> 
> Right now for i.MX we have several abstraction interfaces
> (imx_clk_XXX) which move values from tables around and call the
> appropriate common clock function to register the clock. These
> abstractions are identical for all the possible i.MX SoCs
> (imx_clk_gate2, imx_clk_mux etc.) and move the values around so they
> can be passed to clk_register_##same.
> 
> So if we can define a fixed-clock, why can't we define an
> fsl,gated-clock or fsl,muxed-clock which defines an i.MX clock of that
> type, and a much smaller subset of code that stuffs these values in
> through a small abstraction and remove all these seperate
> clk-imx51-imx53.c, clk-imx6q.c, clk-imx31.c files which are sharing
> common functionality into one mega-abstraction which works across the
> whole family?
>
> The way I understand the binding right now (and I'm looking at this;
> http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob_plain;f=Documentation/devicetree/bindings/clock/clock-bindings.txt;hb=766e6a4ec602d0c107553b91b3434fe9c03474f4),
> this is entirely what the clock binding is saying we should do. But I
> don't understand why we're naming inputs and outputs, but not defining
> how these signals are routed (possibly through gate bits in registers)
> and leaving that down to some driver somewhere. Or why we have to name
> clocks twice, once for the canonical name of the output in reference
> to a higher level or root clock "uart_serial", vs. a driver-level name
> for that input or output in the driver ("per"), since most clocks at
> the driver level can't be re-used for other things anyway. If they
> could, the whole prepare/unprepare and some explicit parenting/muxing
> of clocks would handle that anyway so that the clock was enabled while
> there was at least one user.

The binding defines connections on clocks between nodes. Whether this is
a single clock controller node with many outputs or a node per mux,
divider, gate, etc. is up to the implementer. I did the latter on
highbank because I've got about 8 clocks and half are the same (pll
outputs). Having worked on many generations of iMX and knowing all the
pitfalls of their clock controllers, I would do exactly what Shawn has
done for any part with complex clock trees. I don't think trying to
abstract things to completely generic code will be worth the effort or
be able to describe all the interactions between clocks.

The value in device tree is handling board differences as there are 10's
of boards per SOC.

> I do understand that in Shawn's patch and using UART as an example
> again, "ipg" and "per" are definitely needed by the driver, and these
> names are defined by the driver and implicit in the documentation to
> enable the use of this unit, but at the end of the day the definition
> at the *device* level (uart@) makes no sense except to perform a
> lookup on a string, and in the end this string lookup only gets done
> at the driver probe time anyway. With a standard definition of these
> "ipg" and "per" strings per family of SoC or even in a generic fashion
> across all possible SoCs (in this case, both are defined using
> imx_clk_gate2()) then a node defining that clock and it's magical
> driver-specific name would work just as well by phandle reference, and
> it's parents are referenced by phandle, and all this stays within
> SoC-specific abstraction of the common clocks and turns into normal
> common clock structure stuff anyway (so you still get to do a string
> lookup, but it's being stuffed by an i.MX driver and registered from
> data it pulled from the DT).

Why don't you read v2 which removes the strings.

> Wouldn't we rather have a single device tree parser and clock
> registration for i.MX than the current 7 which would get split into 7
> clock registration drivers with some helpers that parsed half of it
> via device tree? I don't really see what benefit you get out of going
> for the halfway-defined model.

All this has been discussed already over the 2 years of iterations of
common struct clock and clock bindings.

Rob