Move imx6q clock lookup over to device tree
Subsystems (6)
cover Move imx6q clock lookup over to device tree
The series adds a generic of_clk_src_onecell_get() function into clock framework, and then moves imx6q clock lookup over to device tree, so that any new clock lookup to be added at later time does not need to patch kerenel.
Shawn Guo (2):
clk: add of_clk_src_onecell_get() support
ARM: imx6q: replace clk_register_clkdev with clock DT lookup
.../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 - drivers/clk/clk.c | 15 ++ include/linux/clk-provider.h | 1 + 7 files changed, 493 insertions(+), 50 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/imx6q-clock.txt
Signature · 1 line
1.7.5.4
PATCH 1/2 clk: add of_clk_src_onecell_get() support
For those SoCs that have hundreds of clock outputs, their clock DT bindings could reasonably define #clock-cells as 1 and require the client device specify the index of the clock it consumes in the cell of its "clocks" phandle.
Add a generic of_clk_src_onecell_get() function for this purpose.
--- drivers/clk/clk.c | 15 +++++++++++++++ include/linux/clk-provider.h | 1 + 2 files changed, 16 insertions(+), 0 deletions(-)
drivers/clk/clk.c +15 -0
@@ -1582,6 +1582,21 @@ struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec, }EXPORT_SYMBOL_GPL (of_clk_src_simple_get); +struct clk *of_clk_src_onecell_get (struct of_phandle_args *clkspec,void *data) +{ +const char *clk_name; +int idx = clkspec->args[0 ]; +int ret; + + ret =of_property_read_string_index (clkspec->np,"clock-output-names" , + idx, &clk_name); +if (ret <0 ) +return ERR_PTR (ret); + +return __clk_lookup (clk_name); +} +EXPORT_SYMBOL_GPL (of_clk_src_onecell_get); +/** * of_clk_add_provider() - Register a clock provider for a node * @np: Device node pointer associated with clock provider
include/linux/clk-provider.h +1 -0
@@ -360,6 +360,7 @@ int of_clk_add_provider(struct device_node *np,void of_clk_del_provider (struct device_node *np);struct clk *of_clk_src_simple_get (struct of_phandle_args *clkspec,void *data); +struct clk *of_clk_src_onecell_get (struct of_phandle_args *clkspec,void *data);const char *of_clk_get_parent_name (struct device_node *np,int index);void of_clk_init (const struct of_device_id *matches);
Signature · 1 line
1.7.5.4
PATCH 1/2 clk: add of_clk_src_onecell_get() support
Quoted 5 lines · Shawn Guo
For those SoCs that have hundreds of clock outputs, their clock DT bindings could reasonably define #clock-cells as 1 and require the client device specify the index of the clock it consumes in the cell of its "clocks" phandle.
Quoted 8 lines
Add a generic
of_clk_src_onecell_get()function for this purpose.Signed-off-by Shawn Guo <[email protected]>--- drivers/clk/clk.c | 15 +++++++++++++++ include/linux/clk-provider.h | 1 + 2 files changed, 16 insertions(+), 0 deletions(-)
Mike,
Can you please apply this patch on clk tree if it looks good to you? I plan to ask arm-soc people pull in clk tree as dependency and then have the other 3 patches in the series to go via arm-soc tree.
Regards,
Shawn
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
In reply to Shawn GuoIt 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.
--- .../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
Documentation/devicetree/bindings/clock/imx6q-clock.txt new file +223 -0
@@ -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 at 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 at 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"; +};
arch/arm/boot/dts/imx6q-sabrelite.dts +1 -0
@@ -111,6 +111,7 @@ codec: sgtl5000 at 0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; + clocks = <&clks 169>; VDDA-supply = <®_2p5v>; VDDIO-supply = <®_3p3v>; };
arch/arm/boot/dts/imx6q.dtsi +251 -10
456 lines not on this page · raw message
arch/arm/mach-imx/clk-imx6q.c +2 -39
50 lines not on this page · raw message
arch/arm/mach-imx/mach-imx6q.c +0 -1
@@ -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);
Signature · 1 line
1.7.5.4
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 10 lines · Shawn Guo
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 callclk_register_clkdevfor 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 <[email protected]>
Looks good. For both patches:
Quoted 808 lines
--- .../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.txtDocumentation/devicetree/bindings/clock/imx6q-clock.txt new file +223
-0@@ -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 at 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 at 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"; +};arch/arm/boot/dts/imx6q-sabrelite.dts +1
-0@@ -111,6 +111,7 @@ codec: sgtl5000 at 0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; + clocks = <&clks 169>; VDDA-supply = <®_2p5v>; VDDIO-supply = <®_3p3v>; };arch/arm/boot/dts/imx6q.dtsi +251
-10@@ -93,18 +93,23 @@ dma-apbh at 00110000 { compatible = "fsl,imx6q-dma-apbh", "fsl,imx28-dma-apbh"; reg = <0x00110000 0x2000>; + clocks = <&clks 106>; }; gpmi-nand at 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 at 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 at 020c4000 { + clks: ccm at 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 at 020c8000 {@@ -468,12 +679,14 @@ compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy"; reg = <0x020c9000 0x1000>; interrupts = <0 44 0x04>; + clocks = <&clks 182>; }; usbphy2: usbphy at 020ca000 { compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy"; reg = <0x020ca000 0x1000>; interrupts = <0 45 0x04>; + clocks = <&clks 183>; }; snvs at 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"; }; };arch/arm/mach-imx/clk-imx6q.c +2
-39@@ -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 );arch/arm/mach-imx/mach-imx6q.c +0
-1@@ -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);
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
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 at 020c4000 {
...
my_clock: clock at 0 {
name = "my_clock_name"; }
}
uart at nnnnnnnn {
...
clocks = <&my_clock>;
}
?
Signature · 2 lines
Matt Sealey <[email protected]> Product Development Analyst, Genesi USA, Inc.
Quoted 829 lines · Rob Herring
Quoted 10 lines · Shawn Guo
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 callclk_register_clkdevfor 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 <[email protected]>Looks good. For both patches:
Reviewed-by Rob Herring <[email protected]>Quoted 801 lines
--- .../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.txtDocumentation/devicetree/bindings/clock/imx6q-clock.txt new file +223
-0@@ -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 at 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 at 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"; +};arch/arm/boot/dts/imx6q-sabrelite.dts +1
-0@@ -111,6 +111,7 @@ codec: sgtl5000 at 0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; + clocks = <&clks 169>; VDDA-supply = <®_2p5v>; VDDIO-supply = <®_3p3v>; };arch/arm/boot/dts/imx6q.dtsi +251
-10@@ -93,18 +93,23 @@ dma-apbh at 00110000 { compatible = "fsl,imx6q-dma-apbh", "fsl,imx28-dma-apbh"; reg = <0x00110000 0x2000>; + clocks = <&clks 106>; }; gpmi-nand at 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 at 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 at 020c4000 { + clks: ccm at 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 at 020c8000 {@@ -468,12 +679,14 @@ compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy"; reg = <0x020c9000 0x1000>; interrupts = <0 44 0x04>; + clocks = <&clks 182>; }; usbphy2: usbphy at 020ca000 { compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy"; reg = <0x020ca000 0x1000>; interrupts = <0 45 0x04>; + clocks = <&clks 183>; }; snvs at 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"; }; };arch/arm/mach-imx/clk-imx6q.c +2
-39@@ -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 );arch/arm/mach-imx/mach-imx6q.c +0
-0arch/arm/mach-imx/mach-imx6q.c static void __init imx6q_sabrelite_cko1_setup(void) · line 99
Quoted 7 lines
arch/arm/mach-imx/mach-imx6q.c +0
-1@@ -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);_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 3 lines · Matt Sealey
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.
Quoted 14 lines
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 at 020c4000 { ... my_clock: clock at 0 { name = "my_clock_name"; } }
Quoted 7 lines
uart at 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.
Signature · 2 lines
Regards, Shawn
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 29 lines · Shawn Guo
Quoted 3 lines · Matt Sealey
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_initinimx53-dt.c, you will find that lookup for sgtl5000 is really board specific and should go to device tree.Quoted 19 lines
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 at 020c4000 { ... my_clock: clock at 0 { name = "my_clock_name"; } }uart at nnnnnnnn { ... clocks = <&my_clock>; }
Quoted 4 lines
Quoted 2 lines
?
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?
Signature · 2 lines
Matt Sealey <[email protected]> Product Development Analyst, Genesi USA, Inc.
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 7 lines · Matt Sealey
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.
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Matt Sealey
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?
Quoted 6 lines
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.
Quoted 5 lines
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.
Quoted 39 lines
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 */ }
Quoted 4 lines
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
Quoted 5 lines
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.
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 9 lines · Rob Herring
Quoted 4 lines · Russell King - ARM Linux
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.)
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 11 lines · Russell King - ARM Linux
Quoted 9 lines · Rob Herring
Quoted 4 lines · Russell King - ARM Linux
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.
Quoted 9 lines
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().
Ok, if I got your point correctly, it's about amending the following changes, and then we can get rid of that big clock-output-names list from dts.
Regards,
Shawn
--8<---
arch/arm/mach-imx/clk-imx6q.c +1 -1
@@ -387,7 +387,7 @@ 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 ); +of_clk_add_provider (np, of_clk_src_onecell_get, clk);clk_register_clkdev (clk[gpt_ipg],"ipg" ,"imx-gpt.0" );clk_register_clkdev (clk[gpt_ipg_per],"per" ,"imx-gpt.0" );
drivers/clk/clk.c +2 -8
@@ -1584,16 +1584,10 @@ EXPORT_SYMBOL_GPL(of_clk_src_simple_get);struct clk *of_clk_src_onecell_get (struct of_phandle_args *clkspec,void *data) { -const char *clk_name; +struct clk **clks = data;int idx = clkspec->args[0 ]; -int ret; - - ret =of_property_read_string_index (clkspec->np,"clock-output-names" , - idx, &clk_name); -if (ret <0 ) -return ERR_PTR (ret); -return __clk_lookup (clk_name); +return clks[idx]; }EXPORT_SYMBOL_GPL (of_clk_src_onecell_get);
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
In reply to Matt SealeyQuoted 32 lines · Matt Sealey
Quoted 31 lines · Shawn Guo
Quoted 3 lines · Matt Sealey
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_initinimx53-dt.c, you will find that lookup for sgtl5000 is really board specific and should go to device tree.Quoted 21 lines
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 at 020c4000 { ... my_clock: clock at 0 { name = "my_clock_name"; } }uart at nnnnnnnn { ... clocks = <&my_clock>; }?
Quoted 7 lines
Quoted 2 lines
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
Quoted 9 lines
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?
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 7 lines · Rob Herring
Quoted 3 lines
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 at foo {
clock-name = "uart";
fsl,clock-type = "ipg";
...
};uart at 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.
Quoted 3 lines
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.
Signature · 2 lines
Matt Sealey <[email protected]> Product Development Analyst, Genesi USA, Inc.
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
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.
Signature · 2 lines
Matt Sealey <[email protected]> Product Development Analyst, Genesi USA, Inc.
Quoted 141 lines · Matt Sealey
Quoted 7 lines · Rob Herring
Quoted 3 lines
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_serialcannot 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 at foo { clock-name = "uart"; fsl,clock-type = "ipg"; ... };uart at 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.
Quoted 3 lines
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.cto 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.--
Matt Sealey <[email protected]>
Product Development Analyst, Genesi USA, Inc.
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
Quoted 3 lines · Matt Sealey
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.
Quoted 4 lines
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.
Quoted 20 lines
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_muxetc.) 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.cfiles which are sharing common functionality into one mega-abstraction which works across the whole family?
Quoted 13 lines
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.
Quoted 8 lines
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
Quoted 8 lines
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.
Quoted 5 lines
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
PATCH 2/2 ARM: imx6q: replace clk_register_clkdev with clock DT lookup
In reply to Matt SealeyQuoted 2 lines · Matt Sealey
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.
Quoted 11 lines
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.
Signature · 2 lines
Regards, Shawn
PATCH 3/4 clk: mxs: replace imx28 clk_register_clkdev with clock DT lookup
In reply to Shawn GuoIt really becomes a maintenance 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 imx28 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.
--- .../devicetree/bindings/clock/imx28-clock.txt | 100 ++++++++++++++++++ arch/arm/boot/dts/imx28.dtsi | 100 ++++++++++++++++++- drivers/clk/mxs/clk-imx28.c | 109 +------------------ 3 files changed, 204 insertions(+), 105 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/imx28-clock.txt
Documentation/devicetree/bindings/clock/imx28-clock.txt new file +100 -0
@@ -0,0 +1,100 @@ +* Clock bindings for Freescale i.MX28 + +Required properties: +- compatible: Should be "fsl,imx28-clkctrl" +- reg: Address and length of the register set +- #clock-cells: Should be <1> +- clock-output-names: A list of clock output names that i.MX28 CLKCTRL + provides. The full list of all valid clock names and IDs is below. + + Name ID + ------------------ + ref_xtal 0 + pll0 1 + pll1 2 + pll2 3 + ref_cpu 4 + ref_emi 5 + ref_io0 6 + ref_io1 7 + ref_pix 8 + ref_hsadc 9 + ref_gpmi 10 + saif0_sel 11 + saif1_sel 12 + gpmi_sel 13 + ssp0_sel 14 + ssp1_sel 15 + ssp2_sel 16 + ssp3_sel 17 + emi_sel 18 + etm_sel 19 + lcdif_sel 20 + cpu 21 + ptp_sel 22 + cpu_pll 23 + cpu_xtal 24 + hbus 25 + xbus 26 + ssp0_div 27 + ssp1_div 28 + ssp2_div 29 + ssp3_div 30 + gpmi_div 31 + emi_pll 32 + emi_xtal 33 + lcdif_div 34 + etm_div 35 + ptp 36 + saif0_div 37 + saif1_div 38 + clk32k_div 39 + rtc 40 + lradc 41 + spdif_div 42 + clk32k 43 + pwm 44 + uart 45 + ssp0 46 + ssp1 47 + ssp2 48 + ssp3 49 + gpmi 50 + spdif 51 + emi 52 + saif0 53 + saif1 54 + lcdif 55 + etm 56 + fec 57 + can0 58 + can1 59 + usb0 60 + usb1 61 + usb0_pwr 62 + usb1_pwr 63 + enet_out 64 + + The ID will be used by clock consumer in the first cell of "clocks" + phandle to specify the desired clock. + +Examples: + +clks: clkctrl at 80040000 { + compatible = "fsl,imx28-clkctrl"; + reg = <0x80040000 0x2000>; + #clock-cells = <1>; + clock-output-names = + ... + "uart", /* 45 */ + ... + "end_of_list"; +}; + +auart0: serial at 8006a000 { + compatible = "fsl,imx28-auart", "fsl,imx23-auart"; + reg = <0x8006a000 0x2000>; + interrupts = <112 70 71>; + clocks = <&clks 45>; + status = "disabled"; +};
arch/arm/boot/dts/imx28.dtsi +98 -2
257 lines not on this page · raw message
drivers/clk/mxs/clk-imx28.c +6 -103
135 lines not on this page · raw message
Signature · 1 line
1.7.5.4
PATCH 4/4 clk: mxs: replace imx23 clk_register_clkdev with clock DT lookup
In reply to Shawn GuoIt really becomes a maintenance 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 imx23 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.
--- .../devicetree/bindings/clock/imx23-clock.txt | 77 ++++++++++++++++++++ arch/arm/boot/dts/imx23.dtsi | 60 +++++++++++++++- drivers/clk/mxs/clk-imx23.c | 51 ++------------ 3 files changed, 141 insertions(+), 47 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/imx23-clock.txt
Documentation/devicetree/bindings/clock/imx23-clock.txt new file +77 -0
@@ -0,0 +1,77 @@ +* Clock bindings for Freescale i.MX23 + +Required properties: +- compatible: Should be "fsl,imx23-clkctrl" +- reg: Address and length of the register set +- #clock-cells: Should be <1> +- clock-output-names: A list of clock output names that i.MX23 CLKCTRL + provides. The full list of all valid clock names and IDs is below. + + Name ID + ------------------ + ref_xtal 0 + pll 1 + ref_cpu 2 + ref_emi 3 + ref_pix 4 + ref_io 5 + saif_sel 6 + lcdif_sel 7 + gpmi_sel 8 + ssp_sel 9 + emi_sel 10 + cpu 11 + etm_sel 12 + cpu_pll 13 + cpu_xtal 14 + hbus 15 + xbus 16 + lcdif_div 17 + ssp_div 18 + gpmi_div 19 + emi_pll 20 + emi_xtal 21 + etm_div 22 + saif_div 23 + clk32k_div 24 + rtc 25 + adc 26 + spdif_div 27 + clk32k 28 + dri 29 + pwm 30 + filt 31 + uart 32 + ssp 33 + gpmi 34 + spdif 35 + emi 36 + saif 37 + lcdif 38 + etm 39 + usb 40 + usb_pwr 41 + + The ID will be used by clock consumer in the first cell of "clocks" + phandle to specify the desired clock. + +Examples: + +clks: clkctrl at 80040000 { + compatible = "fsl,imx23-clkctrl"; + reg = <0x80040000 0x2000>; + #clock-cells = <1>; + clock-output-names = + ... + "uart", /* 32 */ + ... + "end_of_list"; +}; + +auart0: serial at 8006c000 { + compatible = "fsl,imx23-auart"; + reg = <0x8006c000 0x2000>; + interrupts = <24 25 23>; + clocks = <&clks 32>; + status = "disabled"; +};
arch/arm/boot/dts/imx23.dtsi +58 -2
127 lines not on this page · raw message
drivers/clk/mxs/clk-imx23.c +6 -45
76 lines not on this page · raw message
Signature · 1 line
1.7.5.4