* [PATCH 0/2] Move imx6q clock lookup over to device tree
@ 2012-08-16 8:08 Shawn Guo
[not found] ` <1345104526-14797-1-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2012-08-16 8:08 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette, Rob Herring
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
--
1.7.5.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] clk: add of_clk_src_onecell_get() support
[not found] ` <1345104526-14797-1-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2012-08-16 8:08 ` Shawn Guo
[not found] ` <1345104526-14797-2-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2012-08-16 8:08 ` [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup Shawn Guo
1 sibling, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2012-08-16 8:08 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette, Rob Herring
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.
Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
drivers/clk/clk.c | 15 +++++++++++++++
include/linux/clk-provider.h | 1 +
2 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index efdfd00..06bc0b5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 77335fa..2afcadf 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -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);
--
1.7.5.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <1345104526-14797-1-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2012-08-16 8:08 ` [PATCH 1/2] clk: add of_clk_src_onecell_get() support Shawn Guo
@ 2012-08-16 8:08 ` Shawn Guo
[not found] ` <1345104526-14797-3-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
1 sibling, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2012-08-16 8:08 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette, Rob Herring
It really becomes an issue that every time a device needs to look
up (clk_get) a clock we have to patch kernel clock file to call
clk_register_clkdev for that clock.
Since clock DT support which is meant to resolve clock lookup in device
tree is in place, the patch moves imx6q client devices' clock lookup
over to device tree, so that any new lookup to be added at later time
can just get done in DT instead of kernel.
Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
.../devicetree/bindings/clock/imx6q-clock.txt | 223 +++++++++++++++++
arch/arm/boot/dts/imx6q-sabrelite.dts | 1 +
arch/arm/boot/dts/imx6q.dtsi | 261 +++++++++++++++++++-
arch/arm/mach-imx/clk-imx6q.c | 41 +---
arch/arm/mach-imx/mach-imx6q.c | 1 -
5 files changed, 477 insertions(+), 50 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/imx6q-clock.txt
diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
new file mode 100644
index 0000000..19d8126
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
@@ -0,0 +1,223 @@
+* Clock bindings for Freescale i.MX6 Quad
+
+Required properties:
+- compatible: Should be "fsl,imx6q-ccm"
+- reg: Address and length of the register set
+- interrupts: Should contain CCM interrupt
+- #clock-cells: Should be <1>
+- clock-output-names: A list of clock output names that CCM provides.
+ The full list of all valid clock names and IDs is below.
+
+ Name ID
+ ---------------------------
+ dummy 0
+ ckil 1
+ ckih 2
+ osc 3
+ pll2_pfd0_352m 4
+ pll2_pfd1_594m 5
+ pll2_pfd2_396m 6
+ pll3_pfd0_720m 7
+ pll3_pfd1_540m 8
+ pll3_pfd2_508m 9
+ pll3_pfd3_454m 10
+ pll2_198m 11
+ pll3_120m 12
+ pll3_80m 13
+ pll3_60m 14
+ twd 15
+ step 16
+ pll1_sw 17
+ periph_pre 18
+ periph2_pre 19
+ periph_clk2_sel 20
+ periph2_clk2_sel 21
+ axi_sel 22
+ esai_sel 23
+ asrc_sel 24
+ spdif_sel 25
+ gpu2d_axi 26
+ gpu3d_axi 27
+ gpu2d_core_sel 28
+ gpu3d_core_sel 29
+ gpu3d_shader_sel 30
+ ipu1_sel 31
+ ipu2_sel 32
+ ldb_di0_sel 33
+ ldb_di1_sel 34
+ ipu1_di0_pre_sel 35
+ ipu1_di1_pre_sel 36
+ ipu2_di0_pre_sel 37
+ ipu2_di1_pre_sel 38
+ ipu1_di0_sel 39
+ ipu1_di1_sel 40
+ ipu2_di0_sel 41
+ ipu2_di1_sel 42
+ hsi_tx_sel 43
+ pcie_axi_sel 44
+ ssi1_sel 45
+ ssi2_sel 46
+ ssi3_sel 47
+ usdhc1_sel 48
+ usdhc2_sel 49
+ usdhc3_sel 50
+ usdhc4_sel 51
+ enfc_sel 52
+ emi_sel 53
+ emi_slow_sel 54
+ vdo_axi_sel 55
+ vpu_axi_sel 56
+ cko1_sel 57
+ periph 58
+ periph2 59
+ periph_clk2 60
+ periph2_clk2 61
+ ipg 62
+ ipg_per 63
+ esai_pred 64
+ esai_podf 65
+ asrc_pred 66
+ asrc_podf 67
+ spdif_pred 68
+ spdif_podf 69
+ can_root 70
+ ecspi_root 71
+ gpu2d_core_podf 72
+ gpu3d_core_podf 73
+ gpu3d_shader 74
+ ipu1_podf 75
+ ipu2_podf 76
+ ldb_di0_podf 77
+ ldb_di1_podf 78
+ ipu1_di0_pre 79
+ ipu1_di1_pre 80
+ ipu2_di0_pre 81
+ ipu2_di1_pre 82
+ hsi_tx_podf 83
+ ssi1_pred 84
+ ssi1_podf 85
+ ssi2_pred 86
+ ssi2_podf 87
+ ssi3_pred 88
+ ssi3_podf 89
+ uart_serial_podf 90
+ usdhc1_podf 91
+ usdhc2_podf 92
+ usdhc3_podf 93
+ usdhc4_podf 94
+ enfc_pred 95
+ enfc_podf 96
+ emi_podf 97
+ emi_slow_podf 98
+ vpu_axi_podf 99
+ cko1_podf 100
+ axi 101
+ mmdc_ch0_axi_podf 102
+ mmdc_ch1_axi_podf 103
+ arm 104
+ ahb 105
+ apbh_dma 106
+ asrc 107
+ can1_ipg 108
+ can1_serial 109
+ can2_ipg 110
+ can2_serial 111
+ ecspi1 112
+ ecspi2 113
+ ecspi3 114
+ ecspi4 115
+ ecspi5 116
+ enet 117
+ esai 118
+ gpt_ipg 119
+ gpt_ipg_per 120
+ gpu2d_core 121
+ gpu3d_core 122
+ hdmi_iahb 123
+ hdmi_isfr 124
+ i2c1 125
+ i2c2 126
+ i2c3 127
+ iim 128
+ enfc 129
+ ipu1 130
+ ipu1_di0 131
+ ipu1_di1 132
+ ipu2 133
+ ipu2_di0 134
+ ldb_di0 135
+ ldb_di1 136
+ ipu2_di1 137
+ hsi_tx 138
+ mlb 139
+ mmdc_ch0_axi 140
+ mmdc_ch1_axi 141
+ ocram 142
+ openvg_axi 143
+ pcie_axi 144
+ pwm1 145
+ pwm2 146
+ pwm3 147
+ pwm4 148
+ per1_bch 149
+ gpmi_bch_apb 150
+ gpmi_bch 151
+ gpmi_io 152
+ gpmi_apb 153
+ sata 154
+ sdma 155
+ spba 156
+ ssi1 157
+ ssi2 158
+ ssi3 159
+ uart_ipg 160
+ uart_serial 161
+ usboh3 162
+ usdhc1 163
+ usdhc2 164
+ usdhc3 165
+ usdhc4 166
+ vdo_axi 167
+ vpu_axi 168
+ cko1 169
+ pll1_sys 170
+ pll2_bus 171
+ pll3_usb_otg 172
+ pll4_audio 173
+ pll5_video 174
+ pll6_mlb 175
+ pll7_usb_host 176
+ pll8_enet 177
+ ssi1_ipg 178
+ ssi2_ipg 179
+ ssi3_ipg 180
+ rom 181
+ usbphy1 182
+ usbphy2 183
+ ldb_di0_div_3_5 184
+ ldb_di1_div_3_5 185
+
+ The ID will be used by clock consumer in the first cell of "clocks"
+ phandle to specify the desired clock.
+
+Examples:
+
+clks: ccm@020c4000 {
+ compatible = "fsl,imx6q-ccm";
+ reg = <0x020c4000 0x4000>;
+ interrupts = <0 87 0x04 0 88 0x04>;
+ #clock-cells = <1>;
+ clock-output-names = ...
+ "uart_ipg",
+ "uart_serial",
+ ...;
+};
+
+uart1: serial@02020000 {
+ compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
+ reg = <0x02020000 0x4000>;
+ interrupts = <0 26 0x04>;
+ clocks = <&clks 160>, <&clks 161>;
+ clock-names = "ipg", "per";
+ status = "disabled";
+};
diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
index 72f30f3..cfdbe53 100644
--- a/arch/arm/boot/dts/imx6q-sabrelite.dts
+++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
@@ -111,6 +111,7 @@
codec: sgtl5000@0a {
compatible = "fsl,sgtl5000";
reg = <0x0a>;
+ clocks = <&clks 169>;
VDDA-supply = <®_2p5v>;
VDDIO-supply = <®_3p3v>;
};
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 0052fe7..acff2dc 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -93,18 +93,23 @@
dma-apbh@00110000 {
compatible = "fsl,imx6q-dma-apbh", "fsl,imx28-dma-apbh";
reg = <0x00110000 0x2000>;
+ clocks = <&clks 106>;
};
gpmi-nand@00112000 {
- compatible = "fsl,imx6q-gpmi-nand";
- #address-cells = <1>;
- #size-cells = <1>;
- reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
- reg-names = "gpmi-nand", "bch";
- interrupts = <0 13 0x04>, <0 15 0x04>;
- interrupt-names = "gpmi-dma", "bch";
- fsl,gpmi-dma-channel = <0>;
- status = "disabled";
+ compatible = "fsl,imx6q-gpmi-nand";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
+ reg-names = "gpmi-nand", "bch";
+ interrupts = <0 13 0x04>, <0 15 0x04>;
+ interrupt-names = "gpmi-dma", "bch";
+ clocks = <&clks 152>, <&clks 153>, <&clks 151>,
+ <&clks 150>, <&clks 149>;
+ clock-names = "gpmi_io", "gpmi_apb", "gpmi_bch",
+ "gpmi_bch_apb", "per1_bch";
+ fsl,gpmi-dma-channel = <0>;
+ status = "disabled";
};
timer@00a00600 {
@@ -146,6 +151,8 @@
compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
reg = <0x02008000 0x4000>;
interrupts = <0 31 0x04>;
+ clocks = <&clks 112>, <&clks 112>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -155,6 +162,8 @@
compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
reg = <0x0200c000 0x4000>;
interrupts = <0 32 0x04>;
+ clocks = <&clks 113>, <&clks 113>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -164,6 +173,8 @@
compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
reg = <0x02010000 0x4000>;
interrupts = <0 33 0x04>;
+ clocks = <&clks 114>, <&clks 114>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -173,6 +184,8 @@
compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
reg = <0x02014000 0x4000>;
interrupts = <0 34 0x04>;
+ clocks = <&clks 115>, <&clks 115>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -182,6 +195,8 @@
compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
reg = <0x02018000 0x4000>;
interrupts = <0 35 0x04>;
+ clocks = <&clks 116>, <&clks 116>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -189,6 +204,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x02020000 0x4000>;
interrupts = <0 26 0x04>;
+ clocks = <&clks 160>, <&clks 161>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -201,6 +218,7 @@
compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
reg = <0x02028000 0x4000>;
interrupts = <0 46 0x04>;
+ clocks = <&clks 178>;
fsl,fifo-depth = <15>;
fsl,ssi-dma-events = <38 37>;
status = "disabled";
@@ -210,6 +228,7 @@
compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
reg = <0x0202c000 0x4000>;
interrupts = <0 47 0x04>;
+ clocks = <&clks 179>;
fsl,fifo-depth = <15>;
fsl,ssi-dma-events = <42 41>;
status = "disabled";
@@ -219,6 +238,7 @@
compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
reg = <0x02030000 0x4000>;
interrupts = <0 48 0x04>;
+ clocks = <&clks 180>;
fsl,fifo-depth = <15>;
fsl,ssi-dma-events = <46 45>;
status = "disabled";
@@ -358,6 +378,7 @@
compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
reg = <0x020bc000 0x4000>;
interrupts = <0 80 0x04>;
+ clocks = <&clks 0>;
status = "disabled";
};
@@ -365,13 +386,203 @@
compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
reg = <0x020c0000 0x4000>;
interrupts = <0 81 0x04>;
+ clocks = <&clks 0>;
status = "disabled";
};
- ccm@020c4000 {
+ clks: ccm@020c4000 {
compatible = "fsl,imx6q-ccm";
reg = <0x020c4000 0x4000>;
interrupts = <0 87 0x04 0 88 0x04>;
+ #clock-cells = <1>;
+ clock-output-names =
+ "dummy", /* 0 */
+ "ckil", /* 1 */
+ "ckih", /* 2 */
+ "osc", /* 3 */
+ "pll2_pfd0_352m", /* 4 */
+ "pll2_pfd1_594m", /* 5 */
+ "pll2_pfd2_396m", /* 6 */
+ "pll3_pfd0_720m", /* 7 */
+ "pll3_pfd1_540m", /* 8 */
+ "pll3_pfd2_508m", /* 9 */
+ "pll3_pfd3_454m", /* 10 */
+ "pll2_198m", /* 11 */
+ "pll3_120m", /* 12 */
+ "pll3_80m", /* 13 */
+ "pll3_60m", /* 14 */
+ "twd", /* 15 */
+ "step", /* 16 */
+ "pll1_sw", /* 17 */
+ "periph_pre", /* 18 */
+ "periph2_pre", /* 19 */
+ "periph_clk2_sel", /* 20 */
+ "periph2_clk2_sel", /* 21 */
+ "axi_sel", /* 22 */
+ "esai_sel", /* 23 */
+ "asrc_sel", /* 24 */
+ "spdif_sel", /* 25 */
+ "gpu2d_axi", /* 26 */
+ "gpu3d_axi", /* 27 */
+ "gpu2d_core_sel", /* 28 */
+ "gpu3d_core_sel", /* 29 */
+ "gpu3d_shader_sel", /* 30 */
+ "ipu1_sel", /* 31 */
+ "ipu2_sel", /* 32 */
+ "ldb_di0_sel", /* 33 */
+ "ldb_di1_sel", /* 34 */
+ "ipu1_di0_pre_sel", /* 35 */
+ "ipu1_di1_pre_sel", /* 36 */
+ "ipu2_di0_pre_sel", /* 37 */
+ "ipu2_di1_pre_sel", /* 38 */
+ "ipu1_di0_sel", /* 39 */
+ "ipu1_di1_sel", /* 40 */
+ "ipu2_di0_sel", /* 41 */
+ "ipu2_di1_sel", /* 42 */
+ "hsi_tx_sel", /* 43 */
+ "pcie_axi_sel", /* 44 */
+ "ssi1_sel", /* 45 */
+ "ssi2_sel", /* 46 */
+ "ssi3_sel", /* 47 */
+ "usdhc1_sel", /* 48 */
+ "usdhc2_sel", /* 49 */
+ "usdhc3_sel", /* 50 */
+ "usdhc4_sel", /* 51 */
+ "enfc_sel", /* 52 */
+ "emi_sel", /* 53 */
+ "emi_slow_sel", /* 54 */
+ "vdo_axi_sel", /* 55 */
+ "vpu_axi_sel", /* 56 */
+ "cko1_sel", /* 57 */
+ "periph", /* 58 */
+ "periph2", /* 59 */
+ "periph_clk2", /* 60 */
+ "periph2_clk2", /* 61 */
+ "ipg", /* 62 */
+ "ipg_per", /* 63 */
+ "esai_pred", /* 64 */
+ "esai_podf", /* 65 */
+ "asrc_pred", /* 66 */
+ "asrc_podf", /* 67 */
+ "spdif_pred", /* 68 */
+ "spdif_podf", /* 69 */
+ "can_root", /* 70 */
+ "ecspi_root", /* 71 */
+ "gpu2d_core_podf", /* 72 */
+ "gpu3d_core_podf", /* 73 */
+ "gpu3d_shader", /* 74 */
+ "ipu1_podf", /* 75 */
+ "ipu2_podf", /* 76 */
+ "ldb_di0_podf", /* 77 */
+ "ldb_di1_podf", /* 78 */
+ "ipu1_di0_pre", /* 79 */
+ "ipu1_di1_pre", /* 80 */
+ "ipu2_di0_pre", /* 81 */
+ "ipu2_di1_pre", /* 82 */
+ "hsi_tx_podf", /* 83 */
+ "ssi1_pred", /* 84 */
+ "ssi1_podf", /* 85 */
+ "ssi2_pred", /* 86 */
+ "ssi2_podf", /* 87 */
+ "ssi3_pred", /* 88 */
+ "ssi3_podf", /* 89 */
+ "uart_serial_podf", /* 90 */
+ "usdhc1_podf", /* 91 */
+ "usdhc2_podf", /* 92 */
+ "usdhc3_podf", /* 93 */
+ "usdhc4_podf", /* 94 */
+ "enfc_pred", /* 95 */
+ "enfc_podf", /* 96 */
+ "emi_podf", /* 97 */
+ "emi_slow_podf", /* 98 */
+ "vpu_axi_podf", /* 99 */
+ "cko1_podf", /* 100 */
+ "axi", /* 101 */
+ "mmdc_ch0_axi_podf", /* 102 */
+ "mmdc_ch1_axi_podf", /* 103 */
+ "arm", /* 104 */
+ "ahb", /* 105 */
+ "apbh_dma", /* 106 */
+ "asrc", /* 107 */
+ "can1_ipg", /* 108 */
+ "can1_serial", /* 109 */
+ "can2_ipg", /* 110 */
+ "can2_serial", /* 111 */
+ "ecspi1", /* 112 */
+ "ecspi2", /* 113 */
+ "ecspi3", /* 114 */
+ "ecspi4", /* 115 */
+ "ecspi5", /* 116 */
+ "enet", /* 117 */
+ "esai", /* 118 */
+ "gpt_ipg", /* 119 */
+ "gpt_ipg_per", /* 120 */
+ "gpu2d_core", /* 121 */
+ "gpu3d_core", /* 122 */
+ "hdmi_iahb", /* 123 */
+ "hdmi_isfr", /* 124 */
+ "i2c1", /* 125 */
+ "i2c2", /* 126 */
+ "i2c3", /* 127 */
+ "iim", /* 128 */
+ "enfc", /* 129 */
+ "ipu1", /* 130 */
+ "ipu1_di0", /* 131 */
+ "ipu1_di1", /* 132 */
+ "ipu2", /* 133 */
+ "ipu2_di0", /* 134 */
+ "ldb_di0", /* 135 */
+ "ldb_di1", /* 136 */
+ "ipu2_di1", /* 137 */
+ "hsi_tx", /* 138 */
+ "mlb", /* 139 */
+ "mmdc_ch0_axi", /* 140 */
+ "mmdc_ch1_axi", /* 141 */
+ "ocram", /* 142 */
+ "openvg_axi", /* 143 */
+ "pcie_axi", /* 144 */
+ "pwm1", /* 145 */
+ "pwm2", /* 146 */
+ "pwm3", /* 147 */
+ "pwm4", /* 148 */
+ "per1_bch", /* 149 */
+ "gpmi_bch_apb", /* 150 */
+ "gpmi_bch", /* 151 */
+ "gpmi_io", /* 152 */
+ "gpmi_apb", /* 153 */
+ "sata", /* 154 */
+ "sdma", /* 155 */
+ "spba", /* 156 */
+ "ssi1", /* 157 */
+ "ssi2", /* 158 */
+ "ssi3", /* 159 */
+ "uart_ipg", /* 160 */
+ "uart_serial", /* 161 */
+ "usboh3", /* 162 */
+ "usdhc1", /* 163 */
+ "usdhc2", /* 164 */
+ "usdhc3", /* 165 */
+ "usdhc4", /* 166 */
+ "vdo_axi", /* 167 */
+ "vpu_axi", /* 168 */
+ "cko1", /* 169 */
+ "pll1_sys", /* 170 */
+ "pll2_bus", /* 171 */
+ "pll3_usb_otg", /* 172 */
+ "pll4_audio", /* 173 */
+ "pll5_video", /* 174 */
+ "pll6_mlb", /* 175 */
+ "pll7_usb_host", /* 176 */
+ "pll8_enet", /* 177 */
+ "ssi1_ipg", /* 178 */
+ "ssi2_ipg", /* 179 */
+ "ssi3_ipg", /* 180 */
+ "rom", /* 181 */
+ "usbphy1", /* 182 */
+ "usbphy2", /* 183 */
+ "ldb_di0_div_3_5", /* 184 */
+ "ldb_di1_div_3_5", /* 185 */
+ "end_of_list";
};
anatop@020c8000 {
@@ -468,12 +679,14 @@
compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
reg = <0x020c9000 0x1000>;
interrupts = <0 44 0x04>;
+ clocks = <&clks 182>;
};
usbphy2: usbphy@020ca000 {
compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
reg = <0x020ca000 0x1000>;
interrupts = <0 45 0x04>;
+ clocks = <&clks 183>;
};
snvs@020cc000 {
@@ -608,6 +821,9 @@
compatible = "fsl,imx6q-sdma", "fsl,imx35-sdma";
reg = <0x020ec000 0x4000>;
interrupts = <0 2 0x04>;
+ clocks = <&clks 155>, <&clks 155>;
+ clock-names = "ipg", "ahb";
+ fsl,sdma-ram-script-name = "imx/sdma/sdma-imx6q-to1.bin";
};
};
@@ -631,6 +847,7 @@
compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
reg = <0x02184000 0x200>;
interrupts = <0 43 0x04>;
+ clocks = <&clks 162>;
fsl,usbphy = <&usbphy1>;
status = "disabled";
};
@@ -639,6 +856,7 @@
compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
reg = <0x02184200 0x200>;
interrupts = <0 40 0x04>;
+ clocks = <&clks 162>;
fsl,usbphy = <&usbphy2>;
status = "disabled";
};
@@ -647,6 +865,7 @@
compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
reg = <0x02184400 0x200>;
interrupts = <0 41 0x04>;
+ clocks = <&clks 162>;
status = "disabled";
};
@@ -654,6 +873,7 @@
compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
reg = <0x02184600 0x200>;
interrupts = <0 42 0x04>;
+ clocks = <&clks 162>;
status = "disabled";
};
@@ -661,6 +881,8 @@
compatible = "fsl,imx6q-fec";
reg = <0x02188000 0x4000>;
interrupts = <0 118 0x04 0 119 0x04>;
+ clocks = <&clks 117>, <&clks 117>;
+ clock-names = "ipg", "ahb";
status = "disabled";
};
@@ -673,6 +895,8 @@
compatible = "fsl,imx6q-usdhc";
reg = <0x02190000 0x4000>;
interrupts = <0 22 0x04>;
+ clocks = <&clks 163>, <&clks 163>, <&clks 163>;
+ clock-names = "ipg", "ahb", "per";
status = "disabled";
};
@@ -680,6 +904,8 @@
compatible = "fsl,imx6q-usdhc";
reg = <0x02194000 0x4000>;
interrupts = <0 23 0x04>;
+ clocks = <&clks 164>, <&clks 164>, <&clks 164>;
+ clock-names = "ipg", "ahb", "per";
status = "disabled";
};
@@ -687,6 +913,8 @@
compatible = "fsl,imx6q-usdhc";
reg = <0x02198000 0x4000>;
interrupts = <0 24 0x04>;
+ clocks = <&clks 165>, <&clks 165>, <&clks 165>;
+ clock-names = "ipg", "ahb", "per";
status = "disabled";
};
@@ -694,6 +922,8 @@
compatible = "fsl,imx6q-usdhc";
reg = <0x0219c000 0x4000>;
interrupts = <0 25 0x04>;
+ clocks = <&clks 166>, <&clks 166>, <&clks 166>;
+ clock-names = "ipg", "ahb", "per";
status = "disabled";
};
@@ -703,6 +933,7 @@
compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
reg = <0x021a0000 0x4000>;
interrupts = <0 36 0x04>;
+ clocks = <&clks 125>;
status = "disabled";
};
@@ -712,6 +943,7 @@
compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
reg = <0x021a4000 0x4000>;
interrupts = <0 37 0x04>;
+ clocks = <&clks 126>;
status = "disabled";
};
@@ -721,6 +953,7 @@
compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
reg = <0x021a8000 0x4000>;
interrupts = <0 38 0x04>;
+ clocks = <&clks 127>;
status = "disabled";
};
@@ -784,6 +1017,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x021e8000 0x4000>;
interrupts = <0 27 0x04>;
+ clocks = <&clks 160>, <&clks 161>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -791,6 +1026,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x021ec000 0x4000>;
interrupts = <0 28 0x04>;
+ clocks = <&clks 160>, <&clks 161>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -798,6 +1035,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x021f0000 0x4000>;
interrupts = <0 29 0x04>;
+ clocks = <&clks 160>, <&clks 161>;
+ clock-names = "ipg", "per";
status = "disabled";
};
@@ -805,6 +1044,8 @@
compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
reg = <0x021f4000 0x4000>;
interrupts = <0 30 0x04>;
+ clocks = <&clks 160>, <&clks 161>;
+ clock-names = "ipg", "per";
status = "disabled";
};
};
diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 8e46407..433c683 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -387,48 +387,11 @@ int __init mx6q_clocks_init(void)
pr_err("i.MX6q clk %d: register failed with %ld\n",
i, PTR_ERR(clk[i]));
+ of_clk_add_provider(np, of_clk_src_onecell_get, NULL);
+
clk_register_clkdev(clk[gpt_ipg], "ipg", "imx-gpt.0");
clk_register_clkdev(clk[gpt_ipg_per], "per", "imx-gpt.0");
clk_register_clkdev(clk[twd], NULL, "smp_twd");
- clk_register_clkdev(clk[apbh_dma], NULL, "110000.dma-apbh");
- clk_register_clkdev(clk[per1_bch], "per1_bch", "112000.gpmi-nand");
- clk_register_clkdev(clk[gpmi_bch_apb], "gpmi_bch_apb", "112000.gpmi-nand");
- clk_register_clkdev(clk[gpmi_bch], "gpmi_bch", "112000.gpmi-nand");
- clk_register_clkdev(clk[gpmi_apb], "gpmi_apb", "112000.gpmi-nand");
- clk_register_clkdev(clk[gpmi_io], "gpmi_io", "112000.gpmi-nand");
- clk_register_clkdev(clk[usboh3], NULL, "2184000.usb");
- clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
- clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
- clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
- clk_register_clkdev(clk[usbphy1], NULL, "20c9000.usbphy");
- clk_register_clkdev(clk[usbphy2], NULL, "20ca000.usbphy");
- clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
- clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "21e8000.serial");
- clk_register_clkdev(clk[uart_serial], "per", "21ec000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "21ec000.serial");
- clk_register_clkdev(clk[uart_serial], "per", "21f0000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "21f0000.serial");
- clk_register_clkdev(clk[uart_serial], "per", "21f4000.serial");
- clk_register_clkdev(clk[uart_ipg], "ipg", "21f4000.serial");
- clk_register_clkdev(clk[enet], NULL, "2188000.ethernet");
- clk_register_clkdev(clk[usdhc1], NULL, "2190000.usdhc");
- clk_register_clkdev(clk[usdhc2], NULL, "2194000.usdhc");
- clk_register_clkdev(clk[usdhc3], NULL, "2198000.usdhc");
- clk_register_clkdev(clk[usdhc4], NULL, "219c000.usdhc");
- clk_register_clkdev(clk[i2c1], NULL, "21a0000.i2c");
- clk_register_clkdev(clk[i2c2], NULL, "21a4000.i2c");
- clk_register_clkdev(clk[i2c3], NULL, "21a8000.i2c");
- clk_register_clkdev(clk[ecspi1], NULL, "2008000.ecspi");
- clk_register_clkdev(clk[ecspi2], NULL, "200c000.ecspi");
- clk_register_clkdev(clk[ecspi3], NULL, "2010000.ecspi");
- clk_register_clkdev(clk[ecspi4], NULL, "2014000.ecspi");
- clk_register_clkdev(clk[ecspi5], NULL, "2018000.ecspi");
- clk_register_clkdev(clk[sdma], NULL, "20ec000.sdma");
- clk_register_clkdev(clk[dummy], NULL, "20bc000.wdog");
- clk_register_clkdev(clk[dummy], NULL, "20c0000.wdog");
- clk_register_clkdev(clk[ssi1_ipg], NULL, "2028000.ssi");
clk_register_clkdev(clk[cko1_sel], "cko1_sel", NULL);
clk_register_clkdev(clk[ahb], "ahb", NULL);
clk_register_clkdev(clk[cko1], "cko1", NULL);
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 6f9c23b..957f5fe 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -94,7 +94,6 @@ static void __init imx6q_sabrelite_cko1_setup(void)
clk_set_parent(cko1_sel, ahb);
rate = clk_round_rate(cko1, 16000000);
clk_set_rate(cko1, rate);
- clk_register_clkdev(cko1, NULL, "0-000a");
put_clk:
if (!IS_ERR(cko1_sel))
clk_put(cko1_sel);
--
1.7.5.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <1345104526-14797-3-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2012-08-20 12:51 ` Rob Herring
[not found] ` <503232E0.3000007-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2012-08-20 12:51 UTC (permalink / raw)
To: Shawn Guo
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette,
Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 08/16/2012 03:08 AM, Shawn Guo wrote:
> It really becomes an issue that every time a device needs to look
> up (clk_get) a clock we have to patch kernel clock file to call
> clk_register_clkdev for that clock.
>
> Since clock DT support which is meant to resolve clock lookup in device
> tree is in place, the patch moves imx6q client devices' clock lookup
> over to device tree, so that any new lookup to be added at later time
> can just get done in DT instead of kernel.
>
> Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Looks good. For both patches:
Reviewed-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
> .../devicetree/bindings/clock/imx6q-clock.txt | 223 +++++++++++++++++
> arch/arm/boot/dts/imx6q-sabrelite.dts | 1 +
> arch/arm/boot/dts/imx6q.dtsi | 261 +++++++++++++++++++-
> arch/arm/mach-imx/clk-imx6q.c | 41 +---
> arch/arm/mach-imx/mach-imx6q.c | 1 -
> 5 files changed, 477 insertions(+), 50 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/imx6q-clock.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> new file mode 100644
> index 0000000..19d8126
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> @@ -0,0 +1,223 @@
> +* Clock bindings for Freescale i.MX6 Quad
> +
> +Required properties:
> +- compatible: Should be "fsl,imx6q-ccm"
> +- reg: Address and length of the register set
> +- interrupts: Should contain CCM interrupt
> +- #clock-cells: Should be <1>
> +- clock-output-names: A list of clock output names that CCM provides.
> + The full list of all valid clock names and IDs is below.
> +
> + Name ID
> + ---------------------------
> + dummy 0
> + ckil 1
> + ckih 2
> + osc 3
> + pll2_pfd0_352m 4
> + pll2_pfd1_594m 5
> + pll2_pfd2_396m 6
> + pll3_pfd0_720m 7
> + pll3_pfd1_540m 8
> + pll3_pfd2_508m 9
> + pll3_pfd3_454m 10
> + pll2_198m 11
> + pll3_120m 12
> + pll3_80m 13
> + pll3_60m 14
> + twd 15
> + step 16
> + pll1_sw 17
> + periph_pre 18
> + periph2_pre 19
> + periph_clk2_sel 20
> + periph2_clk2_sel 21
> + axi_sel 22
> + esai_sel 23
> + asrc_sel 24
> + spdif_sel 25
> + gpu2d_axi 26
> + gpu3d_axi 27
> + gpu2d_core_sel 28
> + gpu3d_core_sel 29
> + gpu3d_shader_sel 30
> + ipu1_sel 31
> + ipu2_sel 32
> + ldb_di0_sel 33
> + ldb_di1_sel 34
> + ipu1_di0_pre_sel 35
> + ipu1_di1_pre_sel 36
> + ipu2_di0_pre_sel 37
> + ipu2_di1_pre_sel 38
> + ipu1_di0_sel 39
> + ipu1_di1_sel 40
> + ipu2_di0_sel 41
> + ipu2_di1_sel 42
> + hsi_tx_sel 43
> + pcie_axi_sel 44
> + ssi1_sel 45
> + ssi2_sel 46
> + ssi3_sel 47
> + usdhc1_sel 48
> + usdhc2_sel 49
> + usdhc3_sel 50
> + usdhc4_sel 51
> + enfc_sel 52
> + emi_sel 53
> + emi_slow_sel 54
> + vdo_axi_sel 55
> + vpu_axi_sel 56
> + cko1_sel 57
> + periph 58
> + periph2 59
> + periph_clk2 60
> + periph2_clk2 61
> + ipg 62
> + ipg_per 63
> + esai_pred 64
> + esai_podf 65
> + asrc_pred 66
> + asrc_podf 67
> + spdif_pred 68
> + spdif_podf 69
> + can_root 70
> + ecspi_root 71
> + gpu2d_core_podf 72
> + gpu3d_core_podf 73
> + gpu3d_shader 74
> + ipu1_podf 75
> + ipu2_podf 76
> + ldb_di0_podf 77
> + ldb_di1_podf 78
> + ipu1_di0_pre 79
> + ipu1_di1_pre 80
> + ipu2_di0_pre 81
> + ipu2_di1_pre 82
> + hsi_tx_podf 83
> + ssi1_pred 84
> + ssi1_podf 85
> + ssi2_pred 86
> + ssi2_podf 87
> + ssi3_pred 88
> + ssi3_podf 89
> + uart_serial_podf 90
> + usdhc1_podf 91
> + usdhc2_podf 92
> + usdhc3_podf 93
> + usdhc4_podf 94
> + enfc_pred 95
> + enfc_podf 96
> + emi_podf 97
> + emi_slow_podf 98
> + vpu_axi_podf 99
> + cko1_podf 100
> + axi 101
> + mmdc_ch0_axi_podf 102
> + mmdc_ch1_axi_podf 103
> + arm 104
> + ahb 105
> + apbh_dma 106
> + asrc 107
> + can1_ipg 108
> + can1_serial 109
> + can2_ipg 110
> + can2_serial 111
> + ecspi1 112
> + ecspi2 113
> + ecspi3 114
> + ecspi4 115
> + ecspi5 116
> + enet 117
> + esai 118
> + gpt_ipg 119
> + gpt_ipg_per 120
> + gpu2d_core 121
> + gpu3d_core 122
> + hdmi_iahb 123
> + hdmi_isfr 124
> + i2c1 125
> + i2c2 126
> + i2c3 127
> + iim 128
> + enfc 129
> + ipu1 130
> + ipu1_di0 131
> + ipu1_di1 132
> + ipu2 133
> + ipu2_di0 134
> + ldb_di0 135
> + ldb_di1 136
> + ipu2_di1 137
> + hsi_tx 138
> + mlb 139
> + mmdc_ch0_axi 140
> + mmdc_ch1_axi 141
> + ocram 142
> + openvg_axi 143
> + pcie_axi 144
> + pwm1 145
> + pwm2 146
> + pwm3 147
> + pwm4 148
> + per1_bch 149
> + gpmi_bch_apb 150
> + gpmi_bch 151
> + gpmi_io 152
> + gpmi_apb 153
> + sata 154
> + sdma 155
> + spba 156
> + ssi1 157
> + ssi2 158
> + ssi3 159
> + uart_ipg 160
> + uart_serial 161
> + usboh3 162
> + usdhc1 163
> + usdhc2 164
> + usdhc3 165
> + usdhc4 166
> + vdo_axi 167
> + vpu_axi 168
> + cko1 169
> + pll1_sys 170
> + pll2_bus 171
> + pll3_usb_otg 172
> + pll4_audio 173
> + pll5_video 174
> + pll6_mlb 175
> + pll7_usb_host 176
> + pll8_enet 177
> + ssi1_ipg 178
> + ssi2_ipg 179
> + ssi3_ipg 180
> + rom 181
> + usbphy1 182
> + usbphy2 183
> + ldb_di0_div_3_5 184
> + ldb_di1_div_3_5 185
> +
> + The ID will be used by clock consumer in the first cell of "clocks"
> + phandle to specify the desired clock.
> +
> +Examples:
> +
> +clks: ccm@020c4000 {
> + compatible = "fsl,imx6q-ccm";
> + reg = <0x020c4000 0x4000>;
> + interrupts = <0 87 0x04 0 88 0x04>;
> + #clock-cells = <1>;
> + clock-output-names = ...
> + "uart_ipg",
> + "uart_serial",
> + ...;
> +};
> +
> +uart1: serial@02020000 {
> + compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
> + reg = <0x02020000 0x4000>;
> + interrupts = <0 26 0x04>;
> + clocks = <&clks 160>, <&clks 161>;
> + clock-names = "ipg", "per";
> + status = "disabled";
> +};
> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
> index 72f30f3..cfdbe53 100644
> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> @@ -111,6 +111,7 @@
> codec: sgtl5000@0a {
> compatible = "fsl,sgtl5000";
> reg = <0x0a>;
> + clocks = <&clks 169>;
> VDDA-supply = <®_2p5v>;
> VDDIO-supply = <®_3p3v>;
> };
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index 0052fe7..acff2dc 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -93,18 +93,23 @@
> dma-apbh@00110000 {
> compatible = "fsl,imx6q-dma-apbh", "fsl,imx28-dma-apbh";
> reg = <0x00110000 0x2000>;
> + clocks = <&clks 106>;
> };
>
> gpmi-nand@00112000 {
> - compatible = "fsl,imx6q-gpmi-nand";
> - #address-cells = <1>;
> - #size-cells = <1>;
> - reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
> - reg-names = "gpmi-nand", "bch";
> - interrupts = <0 13 0x04>, <0 15 0x04>;
> - interrupt-names = "gpmi-dma", "bch";
> - fsl,gpmi-dma-channel = <0>;
> - status = "disabled";
> + compatible = "fsl,imx6q-gpmi-nand";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
> + reg-names = "gpmi-nand", "bch";
> + interrupts = <0 13 0x04>, <0 15 0x04>;
> + interrupt-names = "gpmi-dma", "bch";
> + clocks = <&clks 152>, <&clks 153>, <&clks 151>,
> + <&clks 150>, <&clks 149>;
> + clock-names = "gpmi_io", "gpmi_apb", "gpmi_bch",
> + "gpmi_bch_apb", "per1_bch";
> + fsl,gpmi-dma-channel = <0>;
> + status = "disabled";
> };
>
> timer@00a00600 {
> @@ -146,6 +151,8 @@
> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
> reg = <0x02008000 0x4000>;
> interrupts = <0 31 0x04>;
> + clocks = <&clks 112>, <&clks 112>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -155,6 +162,8 @@
> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
> reg = <0x0200c000 0x4000>;
> interrupts = <0 32 0x04>;
> + clocks = <&clks 113>, <&clks 113>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -164,6 +173,8 @@
> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
> reg = <0x02010000 0x4000>;
> interrupts = <0 33 0x04>;
> + clocks = <&clks 114>, <&clks 114>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -173,6 +184,8 @@
> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
> reg = <0x02014000 0x4000>;
> interrupts = <0 34 0x04>;
> + clocks = <&clks 115>, <&clks 115>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -182,6 +195,8 @@
> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
> reg = <0x02018000 0x4000>;
> interrupts = <0 35 0x04>;
> + clocks = <&clks 116>, <&clks 116>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -189,6 +204,8 @@
> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
> reg = <0x02020000 0x4000>;
> interrupts = <0 26 0x04>;
> + clocks = <&clks 160>, <&clks 161>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -201,6 +218,7 @@
> compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
> reg = <0x02028000 0x4000>;
> interrupts = <0 46 0x04>;
> + clocks = <&clks 178>;
> fsl,fifo-depth = <15>;
> fsl,ssi-dma-events = <38 37>;
> status = "disabled";
> @@ -210,6 +228,7 @@
> compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
> reg = <0x0202c000 0x4000>;
> interrupts = <0 47 0x04>;
> + clocks = <&clks 179>;
> fsl,fifo-depth = <15>;
> fsl,ssi-dma-events = <42 41>;
> status = "disabled";
> @@ -219,6 +238,7 @@
> compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
> reg = <0x02030000 0x4000>;
> interrupts = <0 48 0x04>;
> + clocks = <&clks 180>;
> fsl,fifo-depth = <15>;
> fsl,ssi-dma-events = <46 45>;
> status = "disabled";
> @@ -358,6 +378,7 @@
> compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> reg = <0x020bc000 0x4000>;
> interrupts = <0 80 0x04>;
> + clocks = <&clks 0>;
> status = "disabled";
> };
>
> @@ -365,13 +386,203 @@
> compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> reg = <0x020c0000 0x4000>;
> interrupts = <0 81 0x04>;
> + clocks = <&clks 0>;
> status = "disabled";
> };
>
> - ccm@020c4000 {
> + clks: ccm@020c4000 {
> compatible = "fsl,imx6q-ccm";
> reg = <0x020c4000 0x4000>;
> interrupts = <0 87 0x04 0 88 0x04>;
> + #clock-cells = <1>;
> + clock-output-names =
> + "dummy", /* 0 */
> + "ckil", /* 1 */
> + "ckih", /* 2 */
> + "osc", /* 3 */
> + "pll2_pfd0_352m", /* 4 */
> + "pll2_pfd1_594m", /* 5 */
> + "pll2_pfd2_396m", /* 6 */
> + "pll3_pfd0_720m", /* 7 */
> + "pll3_pfd1_540m", /* 8 */
> + "pll3_pfd2_508m", /* 9 */
> + "pll3_pfd3_454m", /* 10 */
> + "pll2_198m", /* 11 */
> + "pll3_120m", /* 12 */
> + "pll3_80m", /* 13 */
> + "pll3_60m", /* 14 */
> + "twd", /* 15 */
> + "step", /* 16 */
> + "pll1_sw", /* 17 */
> + "periph_pre", /* 18 */
> + "periph2_pre", /* 19 */
> + "periph_clk2_sel", /* 20 */
> + "periph2_clk2_sel", /* 21 */
> + "axi_sel", /* 22 */
> + "esai_sel", /* 23 */
> + "asrc_sel", /* 24 */
> + "spdif_sel", /* 25 */
> + "gpu2d_axi", /* 26 */
> + "gpu3d_axi", /* 27 */
> + "gpu2d_core_sel", /* 28 */
> + "gpu3d_core_sel", /* 29 */
> + "gpu3d_shader_sel", /* 30 */
> + "ipu1_sel", /* 31 */
> + "ipu2_sel", /* 32 */
> + "ldb_di0_sel", /* 33 */
> + "ldb_di1_sel", /* 34 */
> + "ipu1_di0_pre_sel", /* 35 */
> + "ipu1_di1_pre_sel", /* 36 */
> + "ipu2_di0_pre_sel", /* 37 */
> + "ipu2_di1_pre_sel", /* 38 */
> + "ipu1_di0_sel", /* 39 */
> + "ipu1_di1_sel", /* 40 */
> + "ipu2_di0_sel", /* 41 */
> + "ipu2_di1_sel", /* 42 */
> + "hsi_tx_sel", /* 43 */
> + "pcie_axi_sel", /* 44 */
> + "ssi1_sel", /* 45 */
> + "ssi2_sel", /* 46 */
> + "ssi3_sel", /* 47 */
> + "usdhc1_sel", /* 48 */
> + "usdhc2_sel", /* 49 */
> + "usdhc3_sel", /* 50 */
> + "usdhc4_sel", /* 51 */
> + "enfc_sel", /* 52 */
> + "emi_sel", /* 53 */
> + "emi_slow_sel", /* 54 */
> + "vdo_axi_sel", /* 55 */
> + "vpu_axi_sel", /* 56 */
> + "cko1_sel", /* 57 */
> + "periph", /* 58 */
> + "periph2", /* 59 */
> + "periph_clk2", /* 60 */
> + "periph2_clk2", /* 61 */
> + "ipg", /* 62 */
> + "ipg_per", /* 63 */
> + "esai_pred", /* 64 */
> + "esai_podf", /* 65 */
> + "asrc_pred", /* 66 */
> + "asrc_podf", /* 67 */
> + "spdif_pred", /* 68 */
> + "spdif_podf", /* 69 */
> + "can_root", /* 70 */
> + "ecspi_root", /* 71 */
> + "gpu2d_core_podf", /* 72 */
> + "gpu3d_core_podf", /* 73 */
> + "gpu3d_shader", /* 74 */
> + "ipu1_podf", /* 75 */
> + "ipu2_podf", /* 76 */
> + "ldb_di0_podf", /* 77 */
> + "ldb_di1_podf", /* 78 */
> + "ipu1_di0_pre", /* 79 */
> + "ipu1_di1_pre", /* 80 */
> + "ipu2_di0_pre", /* 81 */
> + "ipu2_di1_pre", /* 82 */
> + "hsi_tx_podf", /* 83 */
> + "ssi1_pred", /* 84 */
> + "ssi1_podf", /* 85 */
> + "ssi2_pred", /* 86 */
> + "ssi2_podf", /* 87 */
> + "ssi3_pred", /* 88 */
> + "ssi3_podf", /* 89 */
> + "uart_serial_podf", /* 90 */
> + "usdhc1_podf", /* 91 */
> + "usdhc2_podf", /* 92 */
> + "usdhc3_podf", /* 93 */
> + "usdhc4_podf", /* 94 */
> + "enfc_pred", /* 95 */
> + "enfc_podf", /* 96 */
> + "emi_podf", /* 97 */
> + "emi_slow_podf", /* 98 */
> + "vpu_axi_podf", /* 99 */
> + "cko1_podf", /* 100 */
> + "axi", /* 101 */
> + "mmdc_ch0_axi_podf", /* 102 */
> + "mmdc_ch1_axi_podf", /* 103 */
> + "arm", /* 104 */
> + "ahb", /* 105 */
> + "apbh_dma", /* 106 */
> + "asrc", /* 107 */
> + "can1_ipg", /* 108 */
> + "can1_serial", /* 109 */
> + "can2_ipg", /* 110 */
> + "can2_serial", /* 111 */
> + "ecspi1", /* 112 */
> + "ecspi2", /* 113 */
> + "ecspi3", /* 114 */
> + "ecspi4", /* 115 */
> + "ecspi5", /* 116 */
> + "enet", /* 117 */
> + "esai", /* 118 */
> + "gpt_ipg", /* 119 */
> + "gpt_ipg_per", /* 120 */
> + "gpu2d_core", /* 121 */
> + "gpu3d_core", /* 122 */
> + "hdmi_iahb", /* 123 */
> + "hdmi_isfr", /* 124 */
> + "i2c1", /* 125 */
> + "i2c2", /* 126 */
> + "i2c3", /* 127 */
> + "iim", /* 128 */
> + "enfc", /* 129 */
> + "ipu1", /* 130 */
> + "ipu1_di0", /* 131 */
> + "ipu1_di1", /* 132 */
> + "ipu2", /* 133 */
> + "ipu2_di0", /* 134 */
> + "ldb_di0", /* 135 */
> + "ldb_di1", /* 136 */
> + "ipu2_di1", /* 137 */
> + "hsi_tx", /* 138 */
> + "mlb", /* 139 */
> + "mmdc_ch0_axi", /* 140 */
> + "mmdc_ch1_axi", /* 141 */
> + "ocram", /* 142 */
> + "openvg_axi", /* 143 */
> + "pcie_axi", /* 144 */
> + "pwm1", /* 145 */
> + "pwm2", /* 146 */
> + "pwm3", /* 147 */
> + "pwm4", /* 148 */
> + "per1_bch", /* 149 */
> + "gpmi_bch_apb", /* 150 */
> + "gpmi_bch", /* 151 */
> + "gpmi_io", /* 152 */
> + "gpmi_apb", /* 153 */
> + "sata", /* 154 */
> + "sdma", /* 155 */
> + "spba", /* 156 */
> + "ssi1", /* 157 */
> + "ssi2", /* 158 */
> + "ssi3", /* 159 */
> + "uart_ipg", /* 160 */
> + "uart_serial", /* 161 */
> + "usboh3", /* 162 */
> + "usdhc1", /* 163 */
> + "usdhc2", /* 164 */
> + "usdhc3", /* 165 */
> + "usdhc4", /* 166 */
> + "vdo_axi", /* 167 */
> + "vpu_axi", /* 168 */
> + "cko1", /* 169 */
> + "pll1_sys", /* 170 */
> + "pll2_bus", /* 171 */
> + "pll3_usb_otg", /* 172 */
> + "pll4_audio", /* 173 */
> + "pll5_video", /* 174 */
> + "pll6_mlb", /* 175 */
> + "pll7_usb_host", /* 176 */
> + "pll8_enet", /* 177 */
> + "ssi1_ipg", /* 178 */
> + "ssi2_ipg", /* 179 */
> + "ssi3_ipg", /* 180 */
> + "rom", /* 181 */
> + "usbphy1", /* 182 */
> + "usbphy2", /* 183 */
> + "ldb_di0_div_3_5", /* 184 */
> + "ldb_di1_div_3_5", /* 185 */
> + "end_of_list";
> };
>
> anatop@020c8000 {
> @@ -468,12 +679,14 @@
> compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
> reg = <0x020c9000 0x1000>;
> interrupts = <0 44 0x04>;
> + clocks = <&clks 182>;
> };
>
> usbphy2: usbphy@020ca000 {
> compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
> reg = <0x020ca000 0x1000>;
> interrupts = <0 45 0x04>;
> + clocks = <&clks 183>;
> };
>
> snvs@020cc000 {
> @@ -608,6 +821,9 @@
> compatible = "fsl,imx6q-sdma", "fsl,imx35-sdma";
> reg = <0x020ec000 0x4000>;
> interrupts = <0 2 0x04>;
> + clocks = <&clks 155>, <&clks 155>;
> + clock-names = "ipg", "ahb";
> + fsl,sdma-ram-script-name = "imx/sdma/sdma-imx6q-to1.bin";
> };
> };
>
> @@ -631,6 +847,7 @@
> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
> reg = <0x02184000 0x200>;
> interrupts = <0 43 0x04>;
> + clocks = <&clks 162>;
> fsl,usbphy = <&usbphy1>;
> status = "disabled";
> };
> @@ -639,6 +856,7 @@
> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
> reg = <0x02184200 0x200>;
> interrupts = <0 40 0x04>;
> + clocks = <&clks 162>;
> fsl,usbphy = <&usbphy2>;
> status = "disabled";
> };
> @@ -647,6 +865,7 @@
> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
> reg = <0x02184400 0x200>;
> interrupts = <0 41 0x04>;
> + clocks = <&clks 162>;
> status = "disabled";
> };
>
> @@ -654,6 +873,7 @@
> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
> reg = <0x02184600 0x200>;
> interrupts = <0 42 0x04>;
> + clocks = <&clks 162>;
> status = "disabled";
> };
>
> @@ -661,6 +881,8 @@
> compatible = "fsl,imx6q-fec";
> reg = <0x02188000 0x4000>;
> interrupts = <0 118 0x04 0 119 0x04>;
> + clocks = <&clks 117>, <&clks 117>;
> + clock-names = "ipg", "ahb";
> status = "disabled";
> };
>
> @@ -673,6 +895,8 @@
> compatible = "fsl,imx6q-usdhc";
> reg = <0x02190000 0x4000>;
> interrupts = <0 22 0x04>;
> + clocks = <&clks 163>, <&clks 163>, <&clks 163>;
> + clock-names = "ipg", "ahb", "per";
> status = "disabled";
> };
>
> @@ -680,6 +904,8 @@
> compatible = "fsl,imx6q-usdhc";
> reg = <0x02194000 0x4000>;
> interrupts = <0 23 0x04>;
> + clocks = <&clks 164>, <&clks 164>, <&clks 164>;
> + clock-names = "ipg", "ahb", "per";
> status = "disabled";
> };
>
> @@ -687,6 +913,8 @@
> compatible = "fsl,imx6q-usdhc";
> reg = <0x02198000 0x4000>;
> interrupts = <0 24 0x04>;
> + clocks = <&clks 165>, <&clks 165>, <&clks 165>;
> + clock-names = "ipg", "ahb", "per";
> status = "disabled";
> };
>
> @@ -694,6 +922,8 @@
> compatible = "fsl,imx6q-usdhc";
> reg = <0x0219c000 0x4000>;
> interrupts = <0 25 0x04>;
> + clocks = <&clks 166>, <&clks 166>, <&clks 166>;
> + clock-names = "ipg", "ahb", "per";
> status = "disabled";
> };
>
> @@ -703,6 +933,7 @@
> compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
> reg = <0x021a0000 0x4000>;
> interrupts = <0 36 0x04>;
> + clocks = <&clks 125>;
> status = "disabled";
> };
>
> @@ -712,6 +943,7 @@
> compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
> reg = <0x021a4000 0x4000>;
> interrupts = <0 37 0x04>;
> + clocks = <&clks 126>;
> status = "disabled";
> };
>
> @@ -721,6 +953,7 @@
> compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
> reg = <0x021a8000 0x4000>;
> interrupts = <0 38 0x04>;
> + clocks = <&clks 127>;
> status = "disabled";
> };
>
> @@ -784,6 +1017,8 @@
> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
> reg = <0x021e8000 0x4000>;
> interrupts = <0 27 0x04>;
> + clocks = <&clks 160>, <&clks 161>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -791,6 +1026,8 @@
> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
> reg = <0x021ec000 0x4000>;
> interrupts = <0 28 0x04>;
> + clocks = <&clks 160>, <&clks 161>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -798,6 +1035,8 @@
> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
> reg = <0x021f0000 0x4000>;
> interrupts = <0 29 0x04>;
> + clocks = <&clks 160>, <&clks 161>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
>
> @@ -805,6 +1044,8 @@
> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
> reg = <0x021f4000 0x4000>;
> interrupts = <0 30 0x04>;
> + clocks = <&clks 160>, <&clks 161>;
> + clock-names = "ipg", "per";
> status = "disabled";
> };
> };
> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> index 8e46407..433c683 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -387,48 +387,11 @@ int __init mx6q_clocks_init(void)
> pr_err("i.MX6q clk %d: register failed with %ld\n",
> i, PTR_ERR(clk[i]));
>
> + of_clk_add_provider(np, of_clk_src_onecell_get, NULL);
> +
> clk_register_clkdev(clk[gpt_ipg], "ipg", "imx-gpt.0");
> clk_register_clkdev(clk[gpt_ipg_per], "per", "imx-gpt.0");
> clk_register_clkdev(clk[twd], NULL, "smp_twd");
> - clk_register_clkdev(clk[apbh_dma], NULL, "110000.dma-apbh");
> - clk_register_clkdev(clk[per1_bch], "per1_bch", "112000.gpmi-nand");
> - clk_register_clkdev(clk[gpmi_bch_apb], "gpmi_bch_apb", "112000.gpmi-nand");
> - clk_register_clkdev(clk[gpmi_bch], "gpmi_bch", "112000.gpmi-nand");
> - clk_register_clkdev(clk[gpmi_apb], "gpmi_apb", "112000.gpmi-nand");
> - clk_register_clkdev(clk[gpmi_io], "gpmi_io", "112000.gpmi-nand");
> - clk_register_clkdev(clk[usboh3], NULL, "2184000.usb");
> - clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
> - clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
> - clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
> - clk_register_clkdev(clk[usbphy1], NULL, "20c9000.usbphy");
> - clk_register_clkdev(clk[usbphy2], NULL, "20ca000.usbphy");
> - clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
> - clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
> - clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
> - clk_register_clkdev(clk[uart_ipg], "ipg", "21e8000.serial");
> - clk_register_clkdev(clk[uart_serial], "per", "21ec000.serial");
> - clk_register_clkdev(clk[uart_ipg], "ipg", "21ec000.serial");
> - clk_register_clkdev(clk[uart_serial], "per", "21f0000.serial");
> - clk_register_clkdev(clk[uart_ipg], "ipg", "21f0000.serial");
> - clk_register_clkdev(clk[uart_serial], "per", "21f4000.serial");
> - clk_register_clkdev(clk[uart_ipg], "ipg", "21f4000.serial");
> - clk_register_clkdev(clk[enet], NULL, "2188000.ethernet");
> - clk_register_clkdev(clk[usdhc1], NULL, "2190000.usdhc");
> - clk_register_clkdev(clk[usdhc2], NULL, "2194000.usdhc");
> - clk_register_clkdev(clk[usdhc3], NULL, "2198000.usdhc");
> - clk_register_clkdev(clk[usdhc4], NULL, "219c000.usdhc");
> - clk_register_clkdev(clk[i2c1], NULL, "21a0000.i2c");
> - clk_register_clkdev(clk[i2c2], NULL, "21a4000.i2c");
> - clk_register_clkdev(clk[i2c3], NULL, "21a8000.i2c");
> - clk_register_clkdev(clk[ecspi1], NULL, "2008000.ecspi");
> - clk_register_clkdev(clk[ecspi2], NULL, "200c000.ecspi");
> - clk_register_clkdev(clk[ecspi3], NULL, "2010000.ecspi");
> - clk_register_clkdev(clk[ecspi4], NULL, "2014000.ecspi");
> - clk_register_clkdev(clk[ecspi5], NULL, "2018000.ecspi");
> - clk_register_clkdev(clk[sdma], NULL, "20ec000.sdma");
> - clk_register_clkdev(clk[dummy], NULL, "20bc000.wdog");
> - clk_register_clkdev(clk[dummy], NULL, "20c0000.wdog");
> - clk_register_clkdev(clk[ssi1_ipg], NULL, "2028000.ssi");
> clk_register_clkdev(clk[cko1_sel], "cko1_sel", NULL);
> clk_register_clkdev(clk[ahb], "ahb", NULL);
> clk_register_clkdev(clk[cko1], "cko1", NULL);
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index 6f9c23b..957f5fe 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -94,7 +94,6 @@ static void __init imx6q_sabrelite_cko1_setup(void)
> clk_set_parent(cko1_sel, ahb);
> rate = clk_round_rate(cko1, 16000000);
> clk_set_rate(cko1, rate);
> - clk_register_clkdev(cko1, NULL, "0-000a");
> put_clk:
> if (!IS_ERR(cko1_sel))
> clk_put(cko1_sel);
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] clk: add of_clk_src_onecell_get() support
[not found] ` <1345104526-14797-2-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2012-08-20 14:30 ` Shawn Guo
0 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2012-08-20 14:30 UTC (permalink / raw)
To: Mike Turquette
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Rob Herring,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Thu, Aug 16, 2012 at 04:08:45PM +0800, Shawn Guo wrote:
> 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.
>
> Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <503232E0.3000007-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2012-08-20 14:54 ` Matt Sealey
[not found] ` <CAKGA1bn2uCWE8YkGR4EzTxZ_bXJ_ov2diq63uxQ=dT7b6Fu9GQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Matt Sealey @ 2012-08-20 14:54 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring
Yet another arbitrary array...
I'm not sure why you would move the registration lookup into the DT
and use an arbitrarily ordered array again. Sure, you're looking it up
entirely within the device tree here, but a better solution would be
to not name clocks twice, and structure your clock definition
properly.
What's wrong with;
ccm@020c4000 {
...
my_clock: clock@0 {
name = "my_clock_name";
}
}
uart@nnnnnnnn {
...
clocks = <&my_clock>;
}
?
--
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.
On Mon, Aug 20, 2012 at 7:51 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 08/16/2012 03:08 AM, Shawn Guo wrote:
>> It really becomes an issue that every time a device needs to look
>> up (clk_get) a clock we have to patch kernel clock file to call
>> clk_register_clkdev for that clock.
>>
>> Since clock DT support which is meant to resolve clock lookup in device
>> tree is in place, the patch moves imx6q client devices' clock lookup
>> over to device tree, so that any new lookup to be added at later time
>> can just get done in DT instead of kernel.
>>
>> Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Looks good. For both patches:
>
> Reviewed-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>
>> ---
>> .../devicetree/bindings/clock/imx6q-clock.txt | 223 +++++++++++++++++
>> arch/arm/boot/dts/imx6q-sabrelite.dts | 1 +
>> arch/arm/boot/dts/imx6q.dtsi | 261 +++++++++++++++++++-
>> arch/arm/mach-imx/clk-imx6q.c | 41 +---
>> arch/arm/mach-imx/mach-imx6q.c | 1 -
>> 5 files changed, 477 insertions(+), 50 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/clock/imx6q-clock.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
>> new file mode 100644
>> index 0000000..19d8126
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
>> @@ -0,0 +1,223 @@
>> +* Clock bindings for Freescale i.MX6 Quad
>> +
>> +Required properties:
>> +- compatible: Should be "fsl,imx6q-ccm"
>> +- reg: Address and length of the register set
>> +- interrupts: Should contain CCM interrupt
>> +- #clock-cells: Should be <1>
>> +- clock-output-names: A list of clock output names that CCM provides.
>> + The full list of all valid clock names and IDs is below.
>> +
>> + Name ID
>> + ---------------------------
>> + dummy 0
>> + ckil 1
>> + ckih 2
>> + osc 3
>> + pll2_pfd0_352m 4
>> + pll2_pfd1_594m 5
>> + pll2_pfd2_396m 6
>> + pll3_pfd0_720m 7
>> + pll3_pfd1_540m 8
>> + pll3_pfd2_508m 9
>> + pll3_pfd3_454m 10
>> + pll2_198m 11
>> + pll3_120m 12
>> + pll3_80m 13
>> + pll3_60m 14
>> + twd 15
>> + step 16
>> + pll1_sw 17
>> + periph_pre 18
>> + periph2_pre 19
>> + periph_clk2_sel 20
>> + periph2_clk2_sel 21
>> + axi_sel 22
>> + esai_sel 23
>> + asrc_sel 24
>> + spdif_sel 25
>> + gpu2d_axi 26
>> + gpu3d_axi 27
>> + gpu2d_core_sel 28
>> + gpu3d_core_sel 29
>> + gpu3d_shader_sel 30
>> + ipu1_sel 31
>> + ipu2_sel 32
>> + ldb_di0_sel 33
>> + ldb_di1_sel 34
>> + ipu1_di0_pre_sel 35
>> + ipu1_di1_pre_sel 36
>> + ipu2_di0_pre_sel 37
>> + ipu2_di1_pre_sel 38
>> + ipu1_di0_sel 39
>> + ipu1_di1_sel 40
>> + ipu2_di0_sel 41
>> + ipu2_di1_sel 42
>> + hsi_tx_sel 43
>> + pcie_axi_sel 44
>> + ssi1_sel 45
>> + ssi2_sel 46
>> + ssi3_sel 47
>> + usdhc1_sel 48
>> + usdhc2_sel 49
>> + usdhc3_sel 50
>> + usdhc4_sel 51
>> + enfc_sel 52
>> + emi_sel 53
>> + emi_slow_sel 54
>> + vdo_axi_sel 55
>> + vpu_axi_sel 56
>> + cko1_sel 57
>> + periph 58
>> + periph2 59
>> + periph_clk2 60
>> + periph2_clk2 61
>> + ipg 62
>> + ipg_per 63
>> + esai_pred 64
>> + esai_podf 65
>> + asrc_pred 66
>> + asrc_podf 67
>> + spdif_pred 68
>> + spdif_podf 69
>> + can_root 70
>> + ecspi_root 71
>> + gpu2d_core_podf 72
>> + gpu3d_core_podf 73
>> + gpu3d_shader 74
>> + ipu1_podf 75
>> + ipu2_podf 76
>> + ldb_di0_podf 77
>> + ldb_di1_podf 78
>> + ipu1_di0_pre 79
>> + ipu1_di1_pre 80
>> + ipu2_di0_pre 81
>> + ipu2_di1_pre 82
>> + hsi_tx_podf 83
>> + ssi1_pred 84
>> + ssi1_podf 85
>> + ssi2_pred 86
>> + ssi2_podf 87
>> + ssi3_pred 88
>> + ssi3_podf 89
>> + uart_serial_podf 90
>> + usdhc1_podf 91
>> + usdhc2_podf 92
>> + usdhc3_podf 93
>> + usdhc4_podf 94
>> + enfc_pred 95
>> + enfc_podf 96
>> + emi_podf 97
>> + emi_slow_podf 98
>> + vpu_axi_podf 99
>> + cko1_podf 100
>> + axi 101
>> + mmdc_ch0_axi_podf 102
>> + mmdc_ch1_axi_podf 103
>> + arm 104
>> + ahb 105
>> + apbh_dma 106
>> + asrc 107
>> + can1_ipg 108
>> + can1_serial 109
>> + can2_ipg 110
>> + can2_serial 111
>> + ecspi1 112
>> + ecspi2 113
>> + ecspi3 114
>> + ecspi4 115
>> + ecspi5 116
>> + enet 117
>> + esai 118
>> + gpt_ipg 119
>> + gpt_ipg_per 120
>> + gpu2d_core 121
>> + gpu3d_core 122
>> + hdmi_iahb 123
>> + hdmi_isfr 124
>> + i2c1 125
>> + i2c2 126
>> + i2c3 127
>> + iim 128
>> + enfc 129
>> + ipu1 130
>> + ipu1_di0 131
>> + ipu1_di1 132
>> + ipu2 133
>> + ipu2_di0 134
>> + ldb_di0 135
>> + ldb_di1 136
>> + ipu2_di1 137
>> + hsi_tx 138
>> + mlb 139
>> + mmdc_ch0_axi 140
>> + mmdc_ch1_axi 141
>> + ocram 142
>> + openvg_axi 143
>> + pcie_axi 144
>> + pwm1 145
>> + pwm2 146
>> + pwm3 147
>> + pwm4 148
>> + per1_bch 149
>> + gpmi_bch_apb 150
>> + gpmi_bch 151
>> + gpmi_io 152
>> + gpmi_apb 153
>> + sata 154
>> + sdma 155
>> + spba 156
>> + ssi1 157
>> + ssi2 158
>> + ssi3 159
>> + uart_ipg 160
>> + uart_serial 161
>> + usboh3 162
>> + usdhc1 163
>> + usdhc2 164
>> + usdhc3 165
>> + usdhc4 166
>> + vdo_axi 167
>> + vpu_axi 168
>> + cko1 169
>> + pll1_sys 170
>> + pll2_bus 171
>> + pll3_usb_otg 172
>> + pll4_audio 173
>> + pll5_video 174
>> + pll6_mlb 175
>> + pll7_usb_host 176
>> + pll8_enet 177
>> + ssi1_ipg 178
>> + ssi2_ipg 179
>> + ssi3_ipg 180
>> + rom 181
>> + usbphy1 182
>> + usbphy2 183
>> + ldb_di0_div_3_5 184
>> + ldb_di1_div_3_5 185
>> +
>> + The ID will be used by clock consumer in the first cell of "clocks"
>> + phandle to specify the desired clock.
>> +
>> +Examples:
>> +
>> +clks: ccm@020c4000 {
>> + compatible = "fsl,imx6q-ccm";
>> + reg = <0x020c4000 0x4000>;
>> + interrupts = <0 87 0x04 0 88 0x04>;
>> + #clock-cells = <1>;
>> + clock-output-names = ...
>> + "uart_ipg",
>> + "uart_serial",
>> + ...;
>> +};
>> +
>> +uart1: serial@02020000 {
>> + compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>> + reg = <0x02020000 0x4000>;
>> + interrupts = <0 26 0x04>;
>> + clocks = <&clks 160>, <&clks 161>;
>> + clock-names = "ipg", "per";
>> + status = "disabled";
>> +};
>> diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts b/arch/arm/boot/dts/imx6q-sabrelite.dts
>> index 72f30f3..cfdbe53 100644
>> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
>> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
>> @@ -111,6 +111,7 @@
>> codec: sgtl5000@0a {
>> compatible = "fsl,sgtl5000";
>> reg = <0x0a>;
>> + clocks = <&clks 169>;
>> VDDA-supply = <®_2p5v>;
>> VDDIO-supply = <®_3p3v>;
>> };
>> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
>> index 0052fe7..acff2dc 100644
>> --- a/arch/arm/boot/dts/imx6q.dtsi
>> +++ b/arch/arm/boot/dts/imx6q.dtsi
>> @@ -93,18 +93,23 @@
>> dma-apbh@00110000 {
>> compatible = "fsl,imx6q-dma-apbh", "fsl,imx28-dma-apbh";
>> reg = <0x00110000 0x2000>;
>> + clocks = <&clks 106>;
>> };
>>
>> gpmi-nand@00112000 {
>> - compatible = "fsl,imx6q-gpmi-nand";
>> - #address-cells = <1>;
>> - #size-cells = <1>;
>> - reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
>> - reg-names = "gpmi-nand", "bch";
>> - interrupts = <0 13 0x04>, <0 15 0x04>;
>> - interrupt-names = "gpmi-dma", "bch";
>> - fsl,gpmi-dma-channel = <0>;
>> - status = "disabled";
>> + compatible = "fsl,imx6q-gpmi-nand";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
>> + reg-names = "gpmi-nand", "bch";
>> + interrupts = <0 13 0x04>, <0 15 0x04>;
>> + interrupt-names = "gpmi-dma", "bch";
>> + clocks = <&clks 152>, <&clks 153>, <&clks 151>,
>> + <&clks 150>, <&clks 149>;
>> + clock-names = "gpmi_io", "gpmi_apb", "gpmi_bch",
>> + "gpmi_bch_apb", "per1_bch";
>> + fsl,gpmi-dma-channel = <0>;
>> + status = "disabled";
>> };
>>
>> timer@00a00600 {
>> @@ -146,6 +151,8 @@
>> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>> reg = <0x02008000 0x4000>;
>> interrupts = <0 31 0x04>;
>> + clocks = <&clks 112>, <&clks 112>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -155,6 +162,8 @@
>> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>> reg = <0x0200c000 0x4000>;
>> interrupts = <0 32 0x04>;
>> + clocks = <&clks 113>, <&clks 113>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -164,6 +173,8 @@
>> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>> reg = <0x02010000 0x4000>;
>> interrupts = <0 33 0x04>;
>> + clocks = <&clks 114>, <&clks 114>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -173,6 +184,8 @@
>> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>> reg = <0x02014000 0x4000>;
>> interrupts = <0 34 0x04>;
>> + clocks = <&clks 115>, <&clks 115>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -182,6 +195,8 @@
>> compatible = "fsl,imx6q-ecspi", "fsl,imx51-ecspi";
>> reg = <0x02018000 0x4000>;
>> interrupts = <0 35 0x04>;
>> + clocks = <&clks 116>, <&clks 116>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -189,6 +204,8 @@
>> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>> reg = <0x02020000 0x4000>;
>> interrupts = <0 26 0x04>;
>> + clocks = <&clks 160>, <&clks 161>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -201,6 +218,7 @@
>> compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
>> reg = <0x02028000 0x4000>;
>> interrupts = <0 46 0x04>;
>> + clocks = <&clks 178>;
>> fsl,fifo-depth = <15>;
>> fsl,ssi-dma-events = <38 37>;
>> status = "disabled";
>> @@ -210,6 +228,7 @@
>> compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
>> reg = <0x0202c000 0x4000>;
>> interrupts = <0 47 0x04>;
>> + clocks = <&clks 179>;
>> fsl,fifo-depth = <15>;
>> fsl,ssi-dma-events = <42 41>;
>> status = "disabled";
>> @@ -219,6 +238,7 @@
>> compatible = "fsl,imx6q-ssi","fsl,imx21-ssi";
>> reg = <0x02030000 0x4000>;
>> interrupts = <0 48 0x04>;
>> + clocks = <&clks 180>;
>> fsl,fifo-depth = <15>;
>> fsl,ssi-dma-events = <46 45>;
>> status = "disabled";
>> @@ -358,6 +378,7 @@
>> compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>> reg = <0x020bc000 0x4000>;
>> interrupts = <0 80 0x04>;
>> + clocks = <&clks 0>;
>> status = "disabled";
>> };
>>
>> @@ -365,13 +386,203 @@
>> compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
>> reg = <0x020c0000 0x4000>;
>> interrupts = <0 81 0x04>;
>> + clocks = <&clks 0>;
>> status = "disabled";
>> };
>>
>> - ccm@020c4000 {
>> + clks: ccm@020c4000 {
>> compatible = "fsl,imx6q-ccm";
>> reg = <0x020c4000 0x4000>;
>> interrupts = <0 87 0x04 0 88 0x04>;
>> + #clock-cells = <1>;
>> + clock-output-names =
>> + "dummy", /* 0 */
>> + "ckil", /* 1 */
>> + "ckih", /* 2 */
>> + "osc", /* 3 */
>> + "pll2_pfd0_352m", /* 4 */
>> + "pll2_pfd1_594m", /* 5 */
>> + "pll2_pfd2_396m", /* 6 */
>> + "pll3_pfd0_720m", /* 7 */
>> + "pll3_pfd1_540m", /* 8 */
>> + "pll3_pfd2_508m", /* 9 */
>> + "pll3_pfd3_454m", /* 10 */
>> + "pll2_198m", /* 11 */
>> + "pll3_120m", /* 12 */
>> + "pll3_80m", /* 13 */
>> + "pll3_60m", /* 14 */
>> + "twd", /* 15 */
>> + "step", /* 16 */
>> + "pll1_sw", /* 17 */
>> + "periph_pre", /* 18 */
>> + "periph2_pre", /* 19 */
>> + "periph_clk2_sel", /* 20 */
>> + "periph2_clk2_sel", /* 21 */
>> + "axi_sel", /* 22 */
>> + "esai_sel", /* 23 */
>> + "asrc_sel", /* 24 */
>> + "spdif_sel", /* 25 */
>> + "gpu2d_axi", /* 26 */
>> + "gpu3d_axi", /* 27 */
>> + "gpu2d_core_sel", /* 28 */
>> + "gpu3d_core_sel", /* 29 */
>> + "gpu3d_shader_sel", /* 30 */
>> + "ipu1_sel", /* 31 */
>> + "ipu2_sel", /* 32 */
>> + "ldb_di0_sel", /* 33 */
>> + "ldb_di1_sel", /* 34 */
>> + "ipu1_di0_pre_sel", /* 35 */
>> + "ipu1_di1_pre_sel", /* 36 */
>> + "ipu2_di0_pre_sel", /* 37 */
>> + "ipu2_di1_pre_sel", /* 38 */
>> + "ipu1_di0_sel", /* 39 */
>> + "ipu1_di1_sel", /* 40 */
>> + "ipu2_di0_sel", /* 41 */
>> + "ipu2_di1_sel", /* 42 */
>> + "hsi_tx_sel", /* 43 */
>> + "pcie_axi_sel", /* 44 */
>> + "ssi1_sel", /* 45 */
>> + "ssi2_sel", /* 46 */
>> + "ssi3_sel", /* 47 */
>> + "usdhc1_sel", /* 48 */
>> + "usdhc2_sel", /* 49 */
>> + "usdhc3_sel", /* 50 */
>> + "usdhc4_sel", /* 51 */
>> + "enfc_sel", /* 52 */
>> + "emi_sel", /* 53 */
>> + "emi_slow_sel", /* 54 */
>> + "vdo_axi_sel", /* 55 */
>> + "vpu_axi_sel", /* 56 */
>> + "cko1_sel", /* 57 */
>> + "periph", /* 58 */
>> + "periph2", /* 59 */
>> + "periph_clk2", /* 60 */
>> + "periph2_clk2", /* 61 */
>> + "ipg", /* 62 */
>> + "ipg_per", /* 63 */
>> + "esai_pred", /* 64 */
>> + "esai_podf", /* 65 */
>> + "asrc_pred", /* 66 */
>> + "asrc_podf", /* 67 */
>> + "spdif_pred", /* 68 */
>> + "spdif_podf", /* 69 */
>> + "can_root", /* 70 */
>> + "ecspi_root", /* 71 */
>> + "gpu2d_core_podf", /* 72 */
>> + "gpu3d_core_podf", /* 73 */
>> + "gpu3d_shader", /* 74 */
>> + "ipu1_podf", /* 75 */
>> + "ipu2_podf", /* 76 */
>> + "ldb_di0_podf", /* 77 */
>> + "ldb_di1_podf", /* 78 */
>> + "ipu1_di0_pre", /* 79 */
>> + "ipu1_di1_pre", /* 80 */
>> + "ipu2_di0_pre", /* 81 */
>> + "ipu2_di1_pre", /* 82 */
>> + "hsi_tx_podf", /* 83 */
>> + "ssi1_pred", /* 84 */
>> + "ssi1_podf", /* 85 */
>> + "ssi2_pred", /* 86 */
>> + "ssi2_podf", /* 87 */
>> + "ssi3_pred", /* 88 */
>> + "ssi3_podf", /* 89 */
>> + "uart_serial_podf", /* 90 */
>> + "usdhc1_podf", /* 91 */
>> + "usdhc2_podf", /* 92 */
>> + "usdhc3_podf", /* 93 */
>> + "usdhc4_podf", /* 94 */
>> + "enfc_pred", /* 95 */
>> + "enfc_podf", /* 96 */
>> + "emi_podf", /* 97 */
>> + "emi_slow_podf", /* 98 */
>> + "vpu_axi_podf", /* 99 */
>> + "cko1_podf", /* 100 */
>> + "axi", /* 101 */
>> + "mmdc_ch0_axi_podf", /* 102 */
>> + "mmdc_ch1_axi_podf", /* 103 */
>> + "arm", /* 104 */
>> + "ahb", /* 105 */
>> + "apbh_dma", /* 106 */
>> + "asrc", /* 107 */
>> + "can1_ipg", /* 108 */
>> + "can1_serial", /* 109 */
>> + "can2_ipg", /* 110 */
>> + "can2_serial", /* 111 */
>> + "ecspi1", /* 112 */
>> + "ecspi2", /* 113 */
>> + "ecspi3", /* 114 */
>> + "ecspi4", /* 115 */
>> + "ecspi5", /* 116 */
>> + "enet", /* 117 */
>> + "esai", /* 118 */
>> + "gpt_ipg", /* 119 */
>> + "gpt_ipg_per", /* 120 */
>> + "gpu2d_core", /* 121 */
>> + "gpu3d_core", /* 122 */
>> + "hdmi_iahb", /* 123 */
>> + "hdmi_isfr", /* 124 */
>> + "i2c1", /* 125 */
>> + "i2c2", /* 126 */
>> + "i2c3", /* 127 */
>> + "iim", /* 128 */
>> + "enfc", /* 129 */
>> + "ipu1", /* 130 */
>> + "ipu1_di0", /* 131 */
>> + "ipu1_di1", /* 132 */
>> + "ipu2", /* 133 */
>> + "ipu2_di0", /* 134 */
>> + "ldb_di0", /* 135 */
>> + "ldb_di1", /* 136 */
>> + "ipu2_di1", /* 137 */
>> + "hsi_tx", /* 138 */
>> + "mlb", /* 139 */
>> + "mmdc_ch0_axi", /* 140 */
>> + "mmdc_ch1_axi", /* 141 */
>> + "ocram", /* 142 */
>> + "openvg_axi", /* 143 */
>> + "pcie_axi", /* 144 */
>> + "pwm1", /* 145 */
>> + "pwm2", /* 146 */
>> + "pwm3", /* 147 */
>> + "pwm4", /* 148 */
>> + "per1_bch", /* 149 */
>> + "gpmi_bch_apb", /* 150 */
>> + "gpmi_bch", /* 151 */
>> + "gpmi_io", /* 152 */
>> + "gpmi_apb", /* 153 */
>> + "sata", /* 154 */
>> + "sdma", /* 155 */
>> + "spba", /* 156 */
>> + "ssi1", /* 157 */
>> + "ssi2", /* 158 */
>> + "ssi3", /* 159 */
>> + "uart_ipg", /* 160 */
>> + "uart_serial", /* 161 */
>> + "usboh3", /* 162 */
>> + "usdhc1", /* 163 */
>> + "usdhc2", /* 164 */
>> + "usdhc3", /* 165 */
>> + "usdhc4", /* 166 */
>> + "vdo_axi", /* 167 */
>> + "vpu_axi", /* 168 */
>> + "cko1", /* 169 */
>> + "pll1_sys", /* 170 */
>> + "pll2_bus", /* 171 */
>> + "pll3_usb_otg", /* 172 */
>> + "pll4_audio", /* 173 */
>> + "pll5_video", /* 174 */
>> + "pll6_mlb", /* 175 */
>> + "pll7_usb_host", /* 176 */
>> + "pll8_enet", /* 177 */
>> + "ssi1_ipg", /* 178 */
>> + "ssi2_ipg", /* 179 */
>> + "ssi3_ipg", /* 180 */
>> + "rom", /* 181 */
>> + "usbphy1", /* 182 */
>> + "usbphy2", /* 183 */
>> + "ldb_di0_div_3_5", /* 184 */
>> + "ldb_di1_div_3_5", /* 185 */
>> + "end_of_list";
>> };
>>
>> anatop@020c8000 {
>> @@ -468,12 +679,14 @@
>> compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
>> reg = <0x020c9000 0x1000>;
>> interrupts = <0 44 0x04>;
>> + clocks = <&clks 182>;
>> };
>>
>> usbphy2: usbphy@020ca000 {
>> compatible = "fsl,imx6q-usbphy", "fsl,imx23-usbphy";
>> reg = <0x020ca000 0x1000>;
>> interrupts = <0 45 0x04>;
>> + clocks = <&clks 183>;
>> };
>>
>> snvs@020cc000 {
>> @@ -608,6 +821,9 @@
>> compatible = "fsl,imx6q-sdma", "fsl,imx35-sdma";
>> reg = <0x020ec000 0x4000>;
>> interrupts = <0 2 0x04>;
>> + clocks = <&clks 155>, <&clks 155>;
>> + clock-names = "ipg", "ahb";
>> + fsl,sdma-ram-script-name = "imx/sdma/sdma-imx6q-to1.bin";
>> };
>> };
>>
>> @@ -631,6 +847,7 @@
>> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>> reg = <0x02184000 0x200>;
>> interrupts = <0 43 0x04>;
>> + clocks = <&clks 162>;
>> fsl,usbphy = <&usbphy1>;
>> status = "disabled";
>> };
>> @@ -639,6 +856,7 @@
>> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>> reg = <0x02184200 0x200>;
>> interrupts = <0 40 0x04>;
>> + clocks = <&clks 162>;
>> fsl,usbphy = <&usbphy2>;
>> status = "disabled";
>> };
>> @@ -647,6 +865,7 @@
>> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>> reg = <0x02184400 0x200>;
>> interrupts = <0 41 0x04>;
>> + clocks = <&clks 162>;
>> status = "disabled";
>> };
>>
>> @@ -654,6 +873,7 @@
>> compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
>> reg = <0x02184600 0x200>;
>> interrupts = <0 42 0x04>;
>> + clocks = <&clks 162>;
>> status = "disabled";
>> };
>>
>> @@ -661,6 +881,8 @@
>> compatible = "fsl,imx6q-fec";
>> reg = <0x02188000 0x4000>;
>> interrupts = <0 118 0x04 0 119 0x04>;
>> + clocks = <&clks 117>, <&clks 117>;
>> + clock-names = "ipg", "ahb";
>> status = "disabled";
>> };
>>
>> @@ -673,6 +895,8 @@
>> compatible = "fsl,imx6q-usdhc";
>> reg = <0x02190000 0x4000>;
>> interrupts = <0 22 0x04>;
>> + clocks = <&clks 163>, <&clks 163>, <&clks 163>;
>> + clock-names = "ipg", "ahb", "per";
>> status = "disabled";
>> };
>>
>> @@ -680,6 +904,8 @@
>> compatible = "fsl,imx6q-usdhc";
>> reg = <0x02194000 0x4000>;
>> interrupts = <0 23 0x04>;
>> + clocks = <&clks 164>, <&clks 164>, <&clks 164>;
>> + clock-names = "ipg", "ahb", "per";
>> status = "disabled";
>> };
>>
>> @@ -687,6 +913,8 @@
>> compatible = "fsl,imx6q-usdhc";
>> reg = <0x02198000 0x4000>;
>> interrupts = <0 24 0x04>;
>> + clocks = <&clks 165>, <&clks 165>, <&clks 165>;
>> + clock-names = "ipg", "ahb", "per";
>> status = "disabled";
>> };
>>
>> @@ -694,6 +922,8 @@
>> compatible = "fsl,imx6q-usdhc";
>> reg = <0x0219c000 0x4000>;
>> interrupts = <0 25 0x04>;
>> + clocks = <&clks 166>, <&clks 166>, <&clks 166>;
>> + clock-names = "ipg", "ahb", "per";
>> status = "disabled";
>> };
>>
>> @@ -703,6 +933,7 @@
>> compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
>> reg = <0x021a0000 0x4000>;
>> interrupts = <0 36 0x04>;
>> + clocks = <&clks 125>;
>> status = "disabled";
>> };
>>
>> @@ -712,6 +943,7 @@
>> compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
>> reg = <0x021a4000 0x4000>;
>> interrupts = <0 37 0x04>;
>> + clocks = <&clks 126>;
>> status = "disabled";
>> };
>>
>> @@ -721,6 +953,7 @@
>> compatible = "fsl,imx6q-i2c", "fsl,imx1-i2c";
>> reg = <0x021a8000 0x4000>;
>> interrupts = <0 38 0x04>;
>> + clocks = <&clks 127>;
>> status = "disabled";
>> };
>>
>> @@ -784,6 +1017,8 @@
>> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>> reg = <0x021e8000 0x4000>;
>> interrupts = <0 27 0x04>;
>> + clocks = <&clks 160>, <&clks 161>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -791,6 +1026,8 @@
>> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>> reg = <0x021ec000 0x4000>;
>> interrupts = <0 28 0x04>;
>> + clocks = <&clks 160>, <&clks 161>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -798,6 +1035,8 @@
>> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>> reg = <0x021f0000 0x4000>;
>> interrupts = <0 29 0x04>;
>> + clocks = <&clks 160>, <&clks 161>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>>
>> @@ -805,6 +1044,8 @@
>> compatible = "fsl,imx6q-uart", "fsl,imx21-uart";
>> reg = <0x021f4000 0x4000>;
>> interrupts = <0 30 0x04>;
>> + clocks = <&clks 160>, <&clks 161>;
>> + clock-names = "ipg", "per";
>> status = "disabled";
>> };
>> };
>> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
>> index 8e46407..433c683 100644
>> --- a/arch/arm/mach-imx/clk-imx6q.c
>> +++ b/arch/arm/mach-imx/clk-imx6q.c
>> @@ -387,48 +387,11 @@ int __init mx6q_clocks_init(void)
>> pr_err("i.MX6q clk %d: register failed with %ld\n",
>> i, PTR_ERR(clk[i]));
>>
>> + of_clk_add_provider(np, of_clk_src_onecell_get, NULL);
>> +
>> clk_register_clkdev(clk[gpt_ipg], "ipg", "imx-gpt.0");
>> clk_register_clkdev(clk[gpt_ipg_per], "per", "imx-gpt.0");
>> clk_register_clkdev(clk[twd], NULL, "smp_twd");
>> - clk_register_clkdev(clk[apbh_dma], NULL, "110000.dma-apbh");
>> - clk_register_clkdev(clk[per1_bch], "per1_bch", "112000.gpmi-nand");
>> - clk_register_clkdev(clk[gpmi_bch_apb], "gpmi_bch_apb", "112000.gpmi-nand");
>> - clk_register_clkdev(clk[gpmi_bch], "gpmi_bch", "112000.gpmi-nand");
>> - clk_register_clkdev(clk[gpmi_apb], "gpmi_apb", "112000.gpmi-nand");
>> - clk_register_clkdev(clk[gpmi_io], "gpmi_io", "112000.gpmi-nand");
>> - clk_register_clkdev(clk[usboh3], NULL, "2184000.usb");
>> - clk_register_clkdev(clk[usboh3], NULL, "2184200.usb");
>> - clk_register_clkdev(clk[usboh3], NULL, "2184400.usb");
>> - clk_register_clkdev(clk[usboh3], NULL, "2184600.usb");
>> - clk_register_clkdev(clk[usbphy1], NULL, "20c9000.usbphy");
>> - clk_register_clkdev(clk[usbphy2], NULL, "20ca000.usbphy");
>> - clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
>> - clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
>> - clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
>> - clk_register_clkdev(clk[uart_ipg], "ipg", "21e8000.serial");
>> - clk_register_clkdev(clk[uart_serial], "per", "21ec000.serial");
>> - clk_register_clkdev(clk[uart_ipg], "ipg", "21ec000.serial");
>> - clk_register_clkdev(clk[uart_serial], "per", "21f0000.serial");
>> - clk_register_clkdev(clk[uart_ipg], "ipg", "21f0000.serial");
>> - clk_register_clkdev(clk[uart_serial], "per", "21f4000.serial");
>> - clk_register_clkdev(clk[uart_ipg], "ipg", "21f4000.serial");
>> - clk_register_clkdev(clk[enet], NULL, "2188000.ethernet");
>> - clk_register_clkdev(clk[usdhc1], NULL, "2190000.usdhc");
>> - clk_register_clkdev(clk[usdhc2], NULL, "2194000.usdhc");
>> - clk_register_clkdev(clk[usdhc3], NULL, "2198000.usdhc");
>> - clk_register_clkdev(clk[usdhc4], NULL, "219c000.usdhc");
>> - clk_register_clkdev(clk[i2c1], NULL, "21a0000.i2c");
>> - clk_register_clkdev(clk[i2c2], NULL, "21a4000.i2c");
>> - clk_register_clkdev(clk[i2c3], NULL, "21a8000.i2c");
>> - clk_register_clkdev(clk[ecspi1], NULL, "2008000.ecspi");
>> - clk_register_clkdev(clk[ecspi2], NULL, "200c000.ecspi");
>> - clk_register_clkdev(clk[ecspi3], NULL, "2010000.ecspi");
>> - clk_register_clkdev(clk[ecspi4], NULL, "2014000.ecspi");
>> - clk_register_clkdev(clk[ecspi5], NULL, "2018000.ecspi");
>> - clk_register_clkdev(clk[sdma], NULL, "20ec000.sdma");
>> - clk_register_clkdev(clk[dummy], NULL, "20bc000.wdog");
>> - clk_register_clkdev(clk[dummy], NULL, "20c0000.wdog");
>> - clk_register_clkdev(clk[ssi1_ipg], NULL, "2028000.ssi");
>> clk_register_clkdev(clk[cko1_sel], "cko1_sel", NULL);
>> clk_register_clkdev(clk[ahb], "ahb", NULL);
>> clk_register_clkdev(clk[cko1], "cko1", NULL);
>> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
>> index 6f9c23b..957f5fe 100644
>> --- a/arch/arm/mach-imx/mach-imx6q.c
>> +++ b/arch/arm/mach-imx/mach-imx6q.c
>> @@ -94,7 +94,6 @@ static void __init imx6q_sabrelite_cko1_setup(void)
>> clk_set_parent(cko1_sel, ahb);
>> rate = clk_round_rate(cko1, 16000000);
>> clk_set_rate(cko1, rate);
>> - clk_register_clkdev(cko1, NULL, "0-000a");
>> put_clk:
>> if (!IS_ERR(cko1_sel))
>> clk_put(cko1_sel);
>>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <CAKGA1bn2uCWE8YkGR4EzTxZ_bXJ_ov2diq63uxQ=dT7b6Fu9GQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-08-20 15:16 ` Shawn Guo
[not found] ` <20120820151609.GN24242-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2012-08-20 15:16 UTC (permalink / raw)
To: Matt Sealey
Cc: Mike Turquette, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring
On Mon, Aug 20, 2012 at 09:54:48AM -0500, Matt Sealey wrote:
> Yet another arbitrary array...
>
> I'm not sure why you would move the registration lookup into the DT
Because I do not want to patch kernel every time I need a new lookup.
Furthermore, looking at function imx53_qsb_init in imx53-dt.c, you
will find that lookup for sgtl5000 is really board specific and should
go to device tree.
> and use an arbitrarily ordered array again. Sure, you're looking it up
> entirely within the device tree here, but a better solution would be
> to not name clocks twice, and structure your clock definition
> properly.
>
> What's wrong with;
>
> ccm@020c4000 {
> ...
> my_clock: clock@0 {
> name = "my_clock_name";
> }
> }
>
> uart@nnnnnnnn {
> ...
> clocks = <&my_clock>;
> }
>
> ?
>
It turns a property into 185 nodes, which will just bloat the device
tree. This issue has been discussed for a plenty of times.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <20120820151609.GN24242-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
@ 2012-08-20 17:22 ` Matt Sealey
[not found] ` <CAKGA1bnfhMC8QG9OqBbpy5iq6ZAfEgrYMg_LVd50U7yikLhDAg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-08-21 12:53 ` Rob Herring
0 siblings, 2 replies; 17+ messages in thread
From: Matt Sealey @ 2012-08-20 17:22 UTC (permalink / raw)
To: Shawn Guo
Cc: Mike Turquette, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring
On Mon, Aug 20, 2012 at 10:16 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Aug 20, 2012 at 09:54:48AM -0500, Matt Sealey wrote:
>> Yet another arbitrary array...
>>
>> I'm not sure why you would move the registration lookup into the DT
>
> Because I do not want to patch kernel every time I need a new lookup.
> Furthermore, looking at function imx53_qsb_init in imx53-dt.c, you
> will find that lookup for sgtl5000 is really board specific and should
> go to device tree.
>
>> and use an arbitrarily ordered array again. Sure, you're looking it up
>> entirely within the device tree here, but a better solution would be
>> to not name clocks twice, and structure your clock definition
>> properly.
>>
>> What's wrong with;
>>
>> ccm@020c4000 {
>> ...
>> my_clock: clock@0 {
>> name = "my_clock_name";
>> }
>> }
>>
>> uart@nnnnnnnn {
>> ...
>> clocks = <&my_clock>;
>> }
>>
>> ?
>>
> It turns a property into 185 nodes, which will just bloat the device
> tree. This issue has been discussed for a plenty of times.
It's not bloat just because it is by its very definition "a big list", is it?
How do you explain duplicating the clock names in the array AND in the
device node as NOT being bloated?
You're going to have to define these clocks as a tree with parents and
leaf nodes anyway in the clock subsystem. Why not define these in the
device tree in total and reference them by handle when you build the
entire clock tree from the ground up? Or will it just be all the
clocks defined in Linux, but the lookups (which is what I see here)
moved into the DT? Why not form the lookups as part of the definition
of the clock tree?
--
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <CAKGA1bnfhMC8QG9OqBbpy5iq6ZAfEgrYMg_LVd50U7yikLhDAg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-08-21 12:27 ` Russell King - ARM Linux
[not found] ` <20120821122753.GU18957-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
2012-08-22 2:47 ` Shawn Guo
1 sibling, 1 reply; 17+ messages in thread
From: Russell King - ARM Linux @ 2012-08-21 12:27 UTC (permalink / raw)
To: Matt Sealey
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette,
Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Mon, Aug 20, 2012 at 12:22:56PM -0500, Matt Sealey wrote:
> You're going to have to define these clocks as a tree with parents and
> leaf nodes anyway in the clock subsystem. Why not define these in the
> device tree in total and reference them by handle when you build the
> entire clock tree from the ground up? Or will it just be all the
> clocks defined in Linux, but the lookups (which is what I see here)
> moved into the DT? Why not form the lookups as part of the definition
> of the clock tree?
Well, IMHO the DT conversion of the clk lookup stuff has been done
completely wrong.
What should have been done is rather than invent a totally new bloody
lookup interface that drivers have to use instead of clk_get(), is to
embed the OF lookup _inside_ clk_get().
What you do is this:
1. Have property names in the device node like:
clock_<connection-id> = <&provider-node output>
In the case of a NULL connection id:
clock = <&provider-node output>
Remember that the connection ID is _supposed_ to be something that
is described by the hardware (like - for the AACI primecell, the
clock which runs the functional side is called "AACICLK" by the TRM,
and for the MMCI primecell, it's "MMCICLK" - even though these two
clocks may be fed by the same source in an implementation.)
2. clkdev's lookup is then modified to look at the struct device, and
check for a DT node. If there is a DT node, it formats a property
string:
if (dev->of_node) {
char *propname, *clk_prop = NULL;
struct property *p;
if (conn_id) {
clk_prop = kasprintf("clock_%s", conn_id);
propname = clk_prop;
} else {
propname = "clock";
}
p = of_find_property(dev->of_node, propname, NULL);
if (clk_prop)
kfree(clk_prop);
if (p) {
clk = clk_get_from_of_property(p);
if (clk)
return clk;
}
/* Fallthrough to clkdev table lookup */
}
So now, you're not dealing with inventing a whole load of names for clocks
on a platform, instead what you're doing is describing _where_ the clock
comes from in the system for a particular device by device node and index
into it - just like we do for interrupts.
This means there's no need for huge tables and such like of clock names.
I did mention this idea long ago but got ignored.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
2012-08-20 17:22 ` Matt Sealey
[not found] ` <CAKGA1bnfhMC8QG9OqBbpy5iq6ZAfEgrYMg_LVd50U7yikLhDAg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-08-21 12:53 ` Rob Herring
[not found] ` <503384E3.4030705-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
1 sibling, 1 reply; 17+ messages in thread
From: Rob Herring @ 2012-08-21 12:53 UTC (permalink / raw)
To: Matt Sealey
Cc: Mike Turquette, devicetree-discuss, Shawn Guo, linux-arm-kernel
On 08/20/2012 12:22 PM, Matt Sealey wrote:
> On Mon, Aug 20, 2012 at 10:16 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> On Mon, Aug 20, 2012 at 09:54:48AM -0500, Matt Sealey wrote:
>>> Yet another arbitrary array...
>>>
>>> I'm not sure why you would move the registration lookup into the DT
>>
>> Because I do not want to patch kernel every time I need a new lookup.
>> Furthermore, looking at function imx53_qsb_init in imx53-dt.c, you
>> will find that lookup for sgtl5000 is really board specific and should
>> go to device tree.
>>
>>> and use an arbitrarily ordered array again. Sure, you're looking it up
>>> entirely within the device tree here, but a better solution would be
>>> to not name clocks twice, and structure your clock definition
>>> properly.
>>>
>>> What's wrong with;
>>>
>>> ccm@020c4000 {
>>> ...
>>> my_clock: clock@0 {
>>> name = "my_clock_name";
>>> }
>>> }
>>>
>>> uart@nnnnnnnn {
>>> ...
>>> clocks = <&my_clock>;
>>> }
>>>
>>> ?
>>>
>> It turns a property into 185 nodes, which will just bloat the device
>> tree. This issue has been discussed for a plenty of times.
>
> It's not bloat just because it is by its very definition "a big list", is it?
>
> How do you explain duplicating the clock names in the array AND in the
> device node as NOT being bloated?
Read the clock binding doc. Names are optional and the 2 names are not
the same. One is the name of the output on the CCM and one is the name
on input to the module.
While generally optional, Shawn has chosen to require at least the
output names. In the case of defining a signal clock controller node
with lots of outputs, that is the right choice.
Rob
>
> You're going to have to define these clocks as a tree with parents and
> leaf nodes anyway in the clock subsystem. Why not define these in the
> device tree in total and reference them by handle when you build the
> entire clock tree from the ground up? Or will it just be all the
> clocks defined in Linux, but the lookups (which is what I see here)
> moved into the DT? Why not form the lookups as part of the definition
> of the clock tree?
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <20120821122753.GU18957-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
@ 2012-08-21 13:11 ` Rob Herring
2012-08-22 8:32 ` Russell King - ARM Linux
0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2012-08-21 13:11 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 08/21/2012 07:27 AM, Russell King - ARM Linux wrote:
> On Mon, Aug 20, 2012 at 12:22:56PM -0500, Matt Sealey wrote:
>> You're going to have to define these clocks as a tree with parents and
>> leaf nodes anyway in the clock subsystem. Why not define these in the
>> device tree in total and reference them by handle when you build the
>> entire clock tree from the ground up? Or will it just be all the
>> clocks defined in Linux, but the lookups (which is what I see here)
>> moved into the DT? Why not form the lookups as part of the definition
>> of the clock tree?
>
> Well, IMHO the DT conversion of the clk lookup stuff has been done
> completely wrong.
>
> What should have been done is rather than invent a totally new bloody
> lookup interface that drivers have to use instead of clk_get(), is to
> embed the OF lookup _inside_ clk_get().
That is exactly what was done. Drivers only use clk_get. Only if you
don't have a struct device, then you can use of_clk_get.
Internally, you still need a conversion of clk provider node and cell to
a struct clk. It is up to each clk provider how to do this. The lookup
done here by Shawn is using the struct clk name and using the existing
clk framework lookup.
> What you do is this:
>
> 1. Have property names in the device node like:
>
> clock_<connection-id> = <&provider-node output>
The connection id is defined by the position in the list and
supplemented with "clock-names" property.
>
> In the case of a NULL connection id:
>
> clock = <&provider-node output>
>
> Remember that the connection ID is _supposed_ to be something that
> is described by the hardware (like - for the AACI primecell, the
> clock which runs the functional side is called "AACICLK" by the TRM,
> and for the MMCI primecell, it's "MMCICLK" - even though these two
> clocks may be fed by the same source in an implementation.)
>
> 2. clkdev's lookup is then modified to look at the struct device, and
> check for a DT node. If there is a DT node, it formats a property
> string:
>
> if (dev->of_node) {
> char *propname, *clk_prop = NULL;
> struct property *p;
>
> if (conn_id) {
> clk_prop = kasprintf("clock_%s", conn_id);
> propname = clk_prop;
> } else {
> propname = "clock";
> }
>
> p = of_find_property(dev->of_node, propname, NULL);
> if (clk_prop)
> kfree(clk_prop);
>
> if (p) {
> clk = clk_get_from_of_property(p);
> if (clk)
> return clk;
> }
>
> /* Fallthrough to clkdev table lookup */
> }
>
> So now, you're not dealing with inventing a whole load of names for clocks
> on a platform, instead what you're doing is describing _where_ the clock
> comes from in the system for a particular device by device node and index
> into it - just like we do for interrupts.
That is what we're doing. The names are optional for DT, but happen to
be required for struct clk now. If we don't put something in DT, then
the clock names will have to be something generic like ccm-1..ccm-185.
Rob
>
> This means there's no need for huge tables and such like of clock names.
>
> I did mention this idea long ago but got ignored.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <CAKGA1bnfhMC8QG9OqBbpy5iq6ZAfEgrYMg_LVd50U7yikLhDAg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-08-21 12:27 ` Russell King - ARM Linux
@ 2012-08-22 2:47 ` Shawn Guo
1 sibling, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2012-08-22 2:47 UTC (permalink / raw)
To: Matt Sealey
Cc: Mike Turquette, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring
On Mon, Aug 20, 2012 at 12:22:56PM -0500, Matt Sealey wrote:
> It's not bloat just because it is by its very definition "a big list", is it?
>
Grep for_each_node_by_*() and for_each_*_node() (include/linux/of.h)
in the tree to see how often these global device tree searching is
used, you may know how important not having so many nodes is.
> How do you explain duplicating the clock names in the array AND in the
> device node as NOT being bloated?
>
> You're going to have to define these clocks as a tree with parents and
> leaf nodes anyway in the clock subsystem. Why not define these in the
> device tree in total and reference them by handle when you build the
> entire clock tree from the ground up? Or will it just be all the
> clocks defined in Linux, but the lookups (which is what I see here)
> moved into the DT? Why not form the lookups as part of the definition
> of the clock tree?
>
This is something I had tried long time before, but it did not get
accepted because:
* It's unnecessary to encode the entire clock tree which is SoC
specific in device tree. Clock driver is a good place for that.
* Again, doing so will bloat device tree with hundreds of nodes.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
2012-08-21 13:11 ` Rob Herring
@ 2012-08-22 8:32 ` Russell King - ARM Linux
[not found] ` <20120822083223.GW18957-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Russell King - ARM Linux @ 2012-08-22 8:32 UTC (permalink / raw)
To: Rob Herring
Cc: Matt Sealey, devicetree-discuss, Shawn Guo, Mike Turquette,
linux-arm-kernel
On Tue, Aug 21, 2012 at 08:11:57AM -0500, Rob Herring wrote:
> On 08/21/2012 07:27 AM, Russell King - ARM Linux wrote:
> > So now, you're not dealing with inventing a whole load of names for clocks
> > on a platform, instead what you're doing is describing _where_ the clock
> > comes from in the system for a particular device by device node and index
> > into it - just like we do for interrupts.
>
> That is what we're doing. The names are optional for DT, but happen to
> be required for struct clk now. If we don't put something in DT, then
> the clock names will have to be something generic like ccm-1..ccm-185.
And that's what's wrong. Clocks themselves should _not_ be required to
be named.
If you use purely "producer node + index" then you don't need to name a
whole bunch of clocks, and you don't need to have an array of clock names
in the DT file. This also gets rid of the time consuming strcmp against
every clock, which has already been raised as a problem with the existing
clk_get().
And, like it or not, the way they're being describing them in the DT file
at the top of this sub-thread, the matching _is_ done only by producer
name, which is TOTALLY the wrong way to go about this (that's how folk
tried to use the connection ID in the clk API and IT DOESN'T WORK for
reusable drivers.)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <20120822083223.GW18957-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
@ 2012-08-22 9:27 ` Shawn Guo
0 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2012-08-22 9:27 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring
On Wed, Aug 22, 2012 at 09:32:23AM +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 21, 2012 at 08:11:57AM -0500, Rob Herring wrote:
> > On 08/21/2012 07:27 AM, Russell King - ARM Linux wrote:
> > > So now, you're not dealing with inventing a whole load of names for clocks
> > > on a platform, instead what you're doing is describing _where_ the clock
> > > comes from in the system for a particular device by device node and index
> > > into it - just like we do for interrupts.
> >
> > That is what we're doing. The names are optional for DT, but happen to
> > be required for struct clk now. If we don't put something in DT, then
> > the clock names will have to be something generic like ccm-1..ccm-185.
>
> And that's what's wrong. Clocks themselves should _not_ be required to
> be named.
>
> If you use purely "producer node + index" then you don't need to name a
> whole bunch of clocks, and you don't need to have an array of clock names
> in the DT file. This also gets rid of the time consuming strcmp against
> every clock, which has already been raised as a problem with the existing
> clk_get().
>
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<---
diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 433c683..d52f3f4 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -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");
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 06bc0b5..a010ed6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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);
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <503384E3.4030705-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
@ 2012-08-22 22:50 ` Matt Sealey
[not found] ` <CAKGA1bkNGJhsCGVBsHnaQn=Dg5T==QP_JnqOXycq8jQn-W-0Kw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Matt Sealey @ 2012-08-22 22:50 UTC (permalink / raw)
To: Rob Herring
Cc: Mike Turquette, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Tue, Aug 21, 2012 at 7:53 AM, Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org> wrote:
>
>> How do you explain duplicating the clock names in the array AND in the
>> device node as NOT being bloated?
>
> Read the clock binding doc. Names are optional and the 2 names are not
> the same. One is the name of the output on the CCM and one is the name
> on input to the module.
My understanding of the i.MX clock tree naming in the docs, though, is
that the names in the DT don't match the ones in the docs, regardless.
I also don't understand how lots of nodes in a tree *is* lots of
bloat, by Shawn's definition, but lots of entirely Linux- and
common-clock-framework-specific names in a table *isn't* bloat even if
most of these clocks and names are not used for devices in the tree
anyway.
Device trees shouldn't encode data that is entirely specific to a
Linux driver. Even if the only user is Linux, you are not describing
the Linux driver, you are describing the hardware. The names match the
hardware specification in the CCM chapters of the manual, but just
barely. All Shawn has done here is make the lookup in the DT, which is
at the very least internally consistent (it's not referencing the
order of an array elsewhere than the device tree), but an index into
an array of strings is not the way we generally encode things in
device trees since the dawn of time, and certainly shouldn't be
acceptable behavior now.
I might agree somewhat with Shawn that encoding every clock for the
chip in the tree (some 190+ entries) and it's bits and entries is a
ridiculous amount, but most boards don't need all the clocks or
selects defined if they're not using those peripherals. There are ways
to make sure boards do not over-define their clock tree (and any
clocks not defined will not get enabled anyway).
That the clock tree is SoC-specific doesn't mean it should not be
encoded in the tree; the fact that Sascha could write a fairly
comprehensive common clock subsystem shows that for certain families
of SoC, certain groupings of clock mux, select and associations
between unit clock (for instance to write to registers) and peripheral
output clock (for instance to clock data to an MMC card or SPI bus or
whatever) are fairly "common" as it stands. What is not common is the
naming and the meaning of that naming, which in the end is only useful
to drivers as debugging information anyway.
What I don't get is why you'd implement a clock lookup as <&clk 160>
<&clk 161> and then futher give them names like "ipg" "per", even if
these were seperate clocks, it makes no sense to NAME them. If clock
160 is "uart_ipg" and 161 is "uart_serial", what are you gaining from
the clock-names property inside the actual device definition? You're
encoding the "ipg" root clock name twice, and "per" doesn't make any
sense here.
What's the difference with moving ALL the definitions of clock values
from the driver (such as arch/arm/mach-imx/clk-mx51-mx53.c) into the
DT and parsing it all out of the DT? Once it's registered then you
have to look it up by name, but if each DT driver node (uart@ for
example) references the clock node by phandle, it will be able to look
up that exact clock by that phandle, correct? What I'm confused about
right now is what the difference is between naming them "ipg" and
"per" vs. "donkey" and "shrek"? This is just a driver-specific naming
such that it can find these things and turn on and off the right
clocks at the right time, but it doesn't make any sense to refer to
the CCM node, then an index to a an array of names, then two more
names defining the clock definition. uart_serial cannot be used for
ANYTHING besides the uart "per" clock, so just encode this as a clock
type in the individual clock node. The clkdev subsystem can resolve
"per" to some magic value or the string in the tree...
uart_ipg_clk: clock@foo {
clock-name = "uart";
fsl,clock-type = "ipg";
...
};
uart@bar {
clocks = <&uart_ipg_clk, &uart_per_clk>
};
I counted here and we're not "bloating" anything in terms of bytes
used to define the clocks - what we save in not doing an array lookup,
we lose in defining a node. Tthis is not going to end the world. What
you "waste" in the device tree, you get to automate in the Linux
driver anyway and remove a bunch of tables, which is what Shawn's
trying to do anyway. If the UART driver needs to do;
sport->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
Then it has all the information it needs here; one of the phandle
references points to a clock which has "ipg" as it's fsl,clock-type
property. Or something else.
It is going to be an SoC-specific binding, that's for sure, but since
there is a definition for a fixed clock, why can't there be a
definition for a gated clock (enable/disable), mux select defined as a
group and possible selections, and the right clocks being defined with
the right potential parents? The entire clock tree AND it's lookups
can be built from device tree, per documentation, per actual SoC
layout and the driver has to live with getting the information from
somewhere other than the heady mix of a table in DT, and a table in
Linux which don't necessarily depend on each other?
You only have to do these lookups once when the clock subsystem starts
on Linux, it's not like every time you want to go find a clock you
have to actually enter a firmware interface, parse the entire device
tree, come back out with a small snippet of data and then go off and
do the stuff; it's been put in a readily-accessible structure, and
gets done at device probe. Register them at boot, find them in the
clock subsystem, you're ready to go.
I would rather see every clock defined in the tree, as much "bloat" as
you would seem to think it is, it makes no sense to have a huge table
of clock definitions who's names and numbers are defined in the device
tree, yet register offsets and bit widths and values and defaults and
parents are defined in the driver itself along with lots of other
lists and tables. The table in the DT is making a huge assumption
about the subsystem needs to do the clock lookups and the way the
*Linux drivers* themself actually works; this is wrong, wrong, wrong
on so many levels.
> While generally optional, Shawn has chosen to require at least the
> output names. In the case of defining a signal clock controller node
> with lots of outputs, that is the right choice.
You only need to define the inputs and outputs you use, and in the end
I think referencing an array of strings by phandle and index into a
property contained in that handle, then looking up another string in
it's own device node to make sure which clock is which, is more
complex and takes longer than referencing a clock by phandle and
looking at two specific properties defining that clock. If we're going
this far, we should DEFINITELY be encoding the clock subsystems in the
SoC entirely in the device tree and automating clock setup. If the
common clock framework lives up to it's name, all you'd need is a
single arch/arm/mach-imx/clk.c to map the specific operation for every
i.MX SoC (i.e. bindings for i.MX) and not one for *every* i.MX SoC,
and all this data can be adequately encoded in the DT.
--
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <CAKGA1bkNGJhsCGVBsHnaQn=Dg5T==QP_JnqOXycq8jQn-W-0Kw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-08-23 0:08 ` Matt Sealey
[not found] ` <CAKGA1bnCk=FqANsVFD123y3uP5dn9AMZydR__mMXrzh6StrBjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 17+ messages in thread
From: Matt Sealey @ 2012-08-23 0:08 UTC (permalink / raw)
To: Rob Herring
Cc: Mike Turquette, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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.
--
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.
On Wed, Aug 22, 2012 at 5:50 PM, Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org> wrote:
> On Tue, Aug 21, 2012 at 7:53 AM, Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org> wrote:
>>
>>> How do you explain duplicating the clock names in the array AND in the
>>> device node as NOT being bloated?
>>
>> Read the clock binding doc. Names are optional and the 2 names are not
>> the same. One is the name of the output on the CCM and one is the name
>> on input to the module.
>
> My understanding of the i.MX clock tree naming in the docs, though, is
> that the names in the DT don't match the ones in the docs, regardless.
>
> I also don't understand how lots of nodes in a tree *is* lots of
> bloat, by Shawn's definition, but lots of entirely Linux- and
> common-clock-framework-specific names in a table *isn't* bloat even if
> most of these clocks and names are not used for devices in the tree
> anyway.
>
> Device trees shouldn't encode data that is entirely specific to a
> Linux driver. Even if the only user is Linux, you are not describing
> the Linux driver, you are describing the hardware. The names match the
> hardware specification in the CCM chapters of the manual, but just
> barely. All Shawn has done here is make the lookup in the DT, which is
> at the very least internally consistent (it's not referencing the
> order of an array elsewhere than the device tree), but an index into
> an array of strings is not the way we generally encode things in
> device trees since the dawn of time, and certainly shouldn't be
> acceptable behavior now.
>
> I might agree somewhat with Shawn that encoding every clock for the
> chip in the tree (some 190+ entries) and it's bits and entries is a
> ridiculous amount, but most boards don't need all the clocks or
> selects defined if they're not using those peripherals. There are ways
> to make sure boards do not over-define their clock tree (and any
> clocks not defined will not get enabled anyway).
>
> That the clock tree is SoC-specific doesn't mean it should not be
> encoded in the tree; the fact that Sascha could write a fairly
> comprehensive common clock subsystem shows that for certain families
> of SoC, certain groupings of clock mux, select and associations
> between unit clock (for instance to write to registers) and peripheral
> output clock (for instance to clock data to an MMC card or SPI bus or
> whatever) are fairly "common" as it stands. What is not common is the
> naming and the meaning of that naming, which in the end is only useful
> to drivers as debugging information anyway.
>
> What I don't get is why you'd implement a clock lookup as <&clk 160>
> <&clk 161> and then futher give them names like "ipg" "per", even if
> these were seperate clocks, it makes no sense to NAME them. If clock
> 160 is "uart_ipg" and 161 is "uart_serial", what are you gaining from
> the clock-names property inside the actual device definition? You're
> encoding the "ipg" root clock name twice, and "per" doesn't make any
> sense here.
>
> What's the difference with moving ALL the definitions of clock values
> from the driver (such as arch/arm/mach-imx/clk-mx51-mx53.c) into the
> DT and parsing it all out of the DT? Once it's registered then you
> have to look it up by name, but if each DT driver node (uart@ for
> example) references the clock node by phandle, it will be able to look
> up that exact clock by that phandle, correct? What I'm confused about
> right now is what the difference is between naming them "ipg" and
> "per" vs. "donkey" and "shrek"? This is just a driver-specific naming
> such that it can find these things and turn on and off the right
> clocks at the right time, but it doesn't make any sense to refer to
> the CCM node, then an index to a an array of names, then two more
> names defining the clock definition. uart_serial cannot be used for
> ANYTHING besides the uart "per" clock, so just encode this as a clock
> type in the individual clock node. The clkdev subsystem can resolve
> "per" to some magic value or the string in the tree...
>
> uart_ipg_clk: clock@foo {
> clock-name = "uart";
> fsl,clock-type = "ipg";
> ...
> };
>
> uart@bar {
> clocks = <&uart_ipg_clk, &uart_per_clk>
> };
>
> I counted here and we're not "bloating" anything in terms of bytes
> used to define the clocks - what we save in not doing an array lookup,
> we lose in defining a node. Tthis is not going to end the world. What
> you "waste" in the device tree, you get to automate in the Linux
> driver anyway and remove a bunch of tables, which is what Shawn's
> trying to do anyway. If the UART driver needs to do;
>
> sport->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
>
> Then it has all the information it needs here; one of the phandle
> references points to a clock which has "ipg" as it's fsl,clock-type
> property. Or something else.
>
> It is going to be an SoC-specific binding, that's for sure, but since
> there is a definition for a fixed clock, why can't there be a
> definition for a gated clock (enable/disable), mux select defined as a
> group and possible selections, and the right clocks being defined with
> the right potential parents? The entire clock tree AND it's lookups
> can be built from device tree, per documentation, per actual SoC
> layout and the driver has to live with getting the information from
> somewhere other than the heady mix of a table in DT, and a table in
> Linux which don't necessarily depend on each other?
>
> You only have to do these lookups once when the clock subsystem starts
> on Linux, it's not like every time you want to go find a clock you
> have to actually enter a firmware interface, parse the entire device
> tree, come back out with a small snippet of data and then go off and
> do the stuff; it's been put in a readily-accessible structure, and
> gets done at device probe. Register them at boot, find them in the
> clock subsystem, you're ready to go.
>
> I would rather see every clock defined in the tree, as much "bloat" as
> you would seem to think it is, it makes no sense to have a huge table
> of clock definitions who's names and numbers are defined in the device
> tree, yet register offsets and bit widths and values and defaults and
> parents are defined in the driver itself along with lots of other
> lists and tables. The table in the DT is making a huge assumption
> about the subsystem needs to do the clock lookups and the way the
> *Linux drivers* themself actually works; this is wrong, wrong, wrong
> on so many levels.
>
>> While generally optional, Shawn has chosen to require at least the
>> output names. In the case of defining a signal clock controller node
>> with lots of outputs, that is the right choice.
>
> You only need to define the inputs and outputs you use, and in the end
> I think referencing an array of strings by phandle and index into a
> property contained in that handle, then looking up another string in
> it's own device node to make sure which clock is which, is more
> complex and takes longer than referencing a clock by phandle and
> looking at two specific properties defining that clock. If we're going
> this far, we should DEFINITELY be encoding the clock subsystems in the
> SoC entirely in the device tree and automating clock setup. If the
> common clock framework lives up to it's name, all you'd need is a
> single arch/arm/mach-imx/clk.c to map the specific operation for every
> i.MX SoC (i.e. bindings for i.MX) and not one for *every* i.MX SoC,
> and all this data can be adequately encoded in the DT.
>
> --
> Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
> Product Development Analyst, Genesi USA, Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup
[not found] ` <CAKGA1bnCk=FqANsVFD123y3uP5dn9AMZydR__mMXrzh6StrBjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-08-23 1:32 ` Rob Herring
0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2012-08-23 1:32 UTC (permalink / raw)
To: Matt Sealey
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mike Turquette,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 08/22/2012 07:08 PM, Matt Sealey wrote:
> Ugh. Okay my blood sugar was super low. Can I just condense that into
> I agree with Russell and the binding makes no sense once it gets past
> the simple definition described in the binding?
I can't speak for Russell, but I think his issue is addressed in v2.
> If we take the pinctrl mess as an example, all the DT serves to do is
> define an SoC-specific or family-specific binding into data the
> pinctrl subsystem can use, via an SoC-specific abstraction in a driver
> to make a generic subsystem in Linux work the way it should.
pinctrl changes much more board to board, so it's more valuable to have
in device tree. The changes from board to board in the clock tree are
much less.
> The common clock subsystem is no different - divider, fixed factor,
> fixed rate, mux, gate, highbank (??) are defined in a way that
> abstracts most of the actual SoC-specific stuff out to register
> offsets, values, bit definitions and masks.
>
> Right now for i.MX we have several abstraction interfaces
> (imx_clk_XXX) which move values from tables around and call the
> appropriate common clock function to register the clock. These
> abstractions are identical for all the possible i.MX SoCs
> (imx_clk_gate2, imx_clk_mux etc.) and move the values around so they
> can be passed to clk_register_##same.
>
> So if we can define a fixed-clock, why can't we define an
> fsl,gated-clock or fsl,muxed-clock which defines an i.MX clock of that
> type, and a much smaller subset of code that stuffs these values in
> through a small abstraction and remove all these seperate
> clk-imx51-imx53.c, clk-imx6q.c, clk-imx31.c files which are sharing
> common functionality into one mega-abstraction which works across the
> whole family?
>
> The way I understand the binding right now (and I'm looking at this;
> http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob_plain;f=Documentation/devicetree/bindings/clock/clock-bindings.txt;hb=766e6a4ec602d0c107553b91b3434fe9c03474f4),
> this is entirely what the clock binding is saying we should do. But I
> don't understand why we're naming inputs and outputs, but not defining
> how these signals are routed (possibly through gate bits in registers)
> and leaving that down to some driver somewhere. Or why we have to name
> clocks twice, once for the canonical name of the output in reference
> to a higher level or root clock "uart_serial", vs. a driver-level name
> for that input or output in the driver ("per"), since most clocks at
> the driver level can't be re-used for other things anyway. If they
> could, the whole prepare/unprepare and some explicit parenting/muxing
> of clocks would handle that anyway so that the clock was enabled while
> there was at least one user.
The binding defines connections on clocks between nodes. Whether this is
a single clock controller node with many outputs or a node per mux,
divider, gate, etc. is up to the implementer. I did the latter on
highbank because I've got about 8 clocks and half are the same (pll
outputs). Having worked on many generations of iMX and knowing all the
pitfalls of their clock controllers, I would do exactly what Shawn has
done for any part with complex clock trees. I don't think trying to
abstract things to completely generic code will be worth the effort or
be able to describe all the interactions between clocks.
The value in device tree is handling board differences as there are 10's
of boards per SOC.
> I do understand that in Shawn's patch and using UART as an example
> again, "ipg" and "per" are definitely needed by the driver, and these
> names are defined by the driver and implicit in the documentation to
> enable the use of this unit, but at the end of the day the definition
> at the *device* level (uart@) makes no sense except to perform a
> lookup on a string, and in the end this string lookup only gets done
> at the driver probe time anyway. With a standard definition of these
> "ipg" and "per" strings per family of SoC or even in a generic fashion
> across all possible SoCs (in this case, both are defined using
> imx_clk_gate2()) then a node defining that clock and it's magical
> driver-specific name would work just as well by phandle reference, and
> it's parents are referenced by phandle, and all this stays within
> SoC-specific abstraction of the common clocks and turns into normal
> common clock structure stuff anyway (so you still get to do a string
> lookup, but it's being stuffed by an i.MX driver and registered from
> data it pulled from the DT).
Why don't you read v2 which removes the strings.
> Wouldn't we rather have a single device tree parser and clock
> registration for i.MX than the current 7 which would get split into 7
> clock registration drivers with some helpers that parsed half of it
> via device tree? I don't really see what benefit you get out of going
> for the halfway-defined model.
All this has been discussed already over the 2 years of iterations of
common struct clock and clock bindings.
Rob
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-08-23 1:32 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-16 8:08 [PATCH 0/2] Move imx6q clock lookup over to device tree Shawn Guo
[not found] ` <1345104526-14797-1-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2012-08-16 8:08 ` [PATCH 1/2] clk: add of_clk_src_onecell_get() support Shawn Guo
[not found] ` <1345104526-14797-2-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2012-08-20 14:30 ` Shawn Guo
2012-08-16 8:08 ` [PATCH 2/2] ARM: imx6q: replace clk_register_clkdev with clock DT lookup Shawn Guo
[not found] ` <1345104526-14797-3-git-send-email-shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2012-08-20 12:51 ` Rob Herring
[not found] ` <503232E0.3000007-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2012-08-20 14:54 ` Matt Sealey
[not found] ` <CAKGA1bn2uCWE8YkGR4EzTxZ_bXJ_ov2diq63uxQ=dT7b6Fu9GQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-08-20 15:16 ` Shawn Guo
[not found] ` <20120820151609.GN24242-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
2012-08-20 17:22 ` Matt Sealey
[not found] ` <CAKGA1bnfhMC8QG9OqBbpy5iq6ZAfEgrYMg_LVd50U7yikLhDAg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-08-21 12:27 ` Russell King - ARM Linux
[not found] ` <20120821122753.GU18957-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
2012-08-21 13:11 ` Rob Herring
2012-08-22 8:32 ` Russell King - ARM Linux
[not found] ` <20120822083223.GW18957-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
2012-08-22 9:27 ` Shawn Guo
2012-08-22 2:47 ` Shawn Guo
2012-08-21 12:53 ` Rob Herring
[not found] ` <503384E3.4030705-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
2012-08-22 22:50 ` Matt Sealey
[not found] ` <CAKGA1bkNGJhsCGVBsHnaQn=Dg5T==QP_JnqOXycq8jQn-W-0Kw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-08-23 0:08 ` Matt Sealey
[not found] ` <CAKGA1bnCk=FqANsVFD123y3uP5dn9AMZydR__mMXrzh6StrBjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-08-23 1:32 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).